Fix whitespace handling with gcr service accounts in mind. #221
Conversation
jacobstr
force-pushed
the
koobz/fix-gcr-sa
branch
from
c4b119c to
b447b8f
Compare
|
Note: #211 suggests using this exact strategy. Folks attempting that will surely run into this caveat. |
When encoding json service accounts, the base64 encoded json may contain newlines. These resulted in > 2 elements in the strings.Split. This modification makes the function resilient to differences whitespace within the embedded service account.json. --- drive-by fix to error string generation We're using a formatting string, but not the `f` variant of `Sprint`. --- address linter complaint -> use fmt.Errorf
jacobstr
force-pushed
the
koobz/fix-gcr-sa
branch
from
b447b8f to
a10732b
Compare
|
Looks like there's a bit more to this still. Going to close an re-open. |
jacobstr
force-pushed
the
koobz/fix-gcr-sa
branch
3 times, most recently
from
e349ad2 to
7a69f1f
Compare
The approach of base64 encoding the json-like structure appears to work fine. The shape of the moving parts (username, password) was largely reverse engineered by: 1. Generating a service account with gcr access and saving to `key.json`. 2. `cat key.json | docker login -u _json_key --password-stdin https://gcr.io` 3. `cat `~/.docker/config.json` should now contain somethin like:``` "gcr.io" : { "auth" : "..." } The password should typically itself be a json-encoded service account i.e. the contents of `key.json` above. We've replaced the parent commits sprintf json-encoding by using the upstream types. Note: we're panicking on the error here vs. propagating it. This is partially laziness, the perceived unlikihood, and finally the desire to keep this change set as straightforward as possible. ```
jacobstr
force-pushed
the
koobz/fix-gcr-sa
branch
from
7a69f1f to
e33c4e2
Compare
| "testing" | ||
| ) | ||
|
|
||
| var configFile = "../../fixtures/docker/config.json" | ||
|
|
||
| func TestGetRegistryAuth(t *testing.T) { | ||
| examples := map[string]string{ | ||
| "registry.company.io": "eyAidXNlcm5hbWUiOiAidXNlcjEiLCAicGFzc3dvcmQiOiAicGFzczEiIH0=", | ||
| "registry.hub.docker.com": "eyAidXNlcm5hbWUiOiAidXNlcjIiLCAicGFzc3dvcmQiOiAicGFzczIiIH0=", | ||
| "registry.company.io": "eyJ1c2VybmFtZSI6Il9qc29uX2tleSIsInBhc3N3b3JkIjoicGFzczEifQ==", |
There was a problem hiding this comment.
The json encoding via json.Marshal strips some of the whitespace, resulting in this base64 difference.
| @@ -55,15 +55,19 @@ func (c *Config) GetCredentials(registry string) (string, string, bool) { | |||
| } | |||
|
|
|||
| func getAuthJSONString(username, password string) string { | |||
| if username == "_json_key" { | |||
| return fmt.Sprintf("%s:%s", username, password) | |||
| b, err := json.Marshal(types.AuthConfig{ | |||
There was a problem hiding this comment.
Considering using an anonymous struct here as well to reduce the dependencies, but I wanted to denote where the fields come from - it may assist future contributors in discovering what authn knobs are available.
| @@ -55,15 +55,19 @@ func (c *Config) GetCredentials(registry string) (string, string, bool) { | |||
| } | |||
|
|
|||
| func getAuthJSONString(username, password string) string { | |||
| if username == "_json_key" { | |||
There was a problem hiding this comment.
I tried to grok this a bit - 005c3ac mentions that it was done to fix gcr authentication, but I could not get GCR working end to end until I made these changes.
It appeared to work initially - list operations against GCR succeeded. I saw subsequent push failures however which led to my original retraction of the MR.
| // happen. We preserve the non error-propagating API for callers, but want | ||
| // some visibility into this that's better than simply swallowing the error. | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Potentially a bit lazy here, but I didn't want to mess with your API's to much by changing the signature here.
There was a problem hiding this comment.
Cool, if I see this panic popping out anywhere, I would change, but anyway, this should never happen. 🙂
| @@ -103,7 +107,7 @@ func Load(fileName string) (*Config, error) { | |||
| } | |||
|
|
|||
| authenticationToken := string(b) | |||
| usernameAndPassword := strings.Split(authenticationToken, ":") | |||
| usernameAndPassword := strings.SplitN(authenticationToken, ":", 2) | |||
There was a problem hiding this comment.
This was the initial change I made in order to fix the decoding of the base64 data I gleaned from my ~/.docker/config.json. The commit message in e33c4e2 discusses how I replicated and tested this:
The shape of the moving parts (username, password) was largely reverse
engineered by:
1. Generating a service account with gcr access and saving to `key.json`.
2. `cat key.json | docker login -u _json_key --password-stdin https://gcr.io`
3. `cat `~/.docker/config.json` should now contain somethin like:```
"gcr.io" : {
"auth" : "..."
}
|
Thank you for this contribution! I'll review it ASAP. |
| examples := map[string]string{ | ||
| "registry.company.io": "user1:pass1", | ||
| "registry.hub.docker.com": "user2:pass2", | ||
| "us.gcr.io": fmt.Sprintf("%s:%s", "_json_key", string(gcrJSONKey)), |
At some point in time I followed the instructions here to generate a static
config.jsonusing a service account authenticated against GCR for use in our CI flows.The encoded password is actually json and the embedded ":" causes it to be split across multiple array elements. That's undesirable in this case and ends up getting manifest as:
Because the config loader ends up skipping the element in https://github.com/ivanilves/lstags/blob/master/docker/config/config.go#L108