relax GoogleCompute auth file loading to allow OIDC creds #186
Conversation
|
|
|
Thanks for enabling CI. I see the failures here, and I'll work on addressing them. |
msarahan
force-pushed
the
relax-JWT-Error
branch
from
236fe0c
to
19a8272
Compare
|
ping @hashicorp/packer |
There was a problem hiding this comment.
Hi @msarahan,
Thanks for taking care of this subject, and sorry we haven't been able to review the code sooner.
While this seems functional (I would suggest adding an OIDC acceptance test to ensure it does work), I have some issues with the implementation.
First is a semantic problem, reusing the account_file attribute feels out-of-place, as the JWTConfigFromJSON expects an account file in order to proceed, so I would argue that circumventing an intentional check/limitation may not be optimal.
OIDC is what is recomended by GCE for accessing their APIs instead of account files, especially for CI environments where it is more secure than having such a file, so I think we should not mix the two together.
Looking at the docs, I would think that if we use a OAuth 2.0 exchange token like in your example, the SDK will take care of getting an up-to-date token, and probably automagically refresh it if needed, making this a better access_token.
If you don't mind for this approach I would suggest adding a new oidc_token attribute to the configs for example, and rather than using the data from the account_file attribute with extra logic, we instead add a new flow in the client creation function to handle this one separately, forwarding the data from the token to the client.
What do you think? Would that approach make sense? Please let us know, and if you need a hand to rework this code, don't hesitate to tell us and we can pick this up.
Thanks again for submitting this!
|
Thanks @lbajolet-hashicorp!
Can you explain what this looks like in pseudocode? I guess this test data isn't exercising the functionality that you'd like to see? https://github.com/hashicorp/packer-plugin-googlecompute/pull/186/files#diff-48f894a52ca9b0e41db72776833723e3784234fe64146ab5270b109c8aa1f270R26 I agree with your other suggestions, and I'll work on an update over the next day or two. |
|
Regarding the acceptance test, it's essentially a test on real infrastructure. The tests you added are OK, I would think they will change a bit if the OIDC auth takes a separate path from the account file one, and if my intuition is correct, I would think we won't check a lot in the code of the plugin itself (though it could be the case, I'd have to dig deeper in the go OAuth code to figure this out). |
msarahan
force-pushed
the
relax-JWT-Error
branch
from
8b7e0bc
to
fd253e4
Compare
|
@lbajolet-hashicorp I think this new approach is much less brittle. I try the JWT method first, and if it fails, fall back to |
There was a problem hiding this comment.
Hi @msarahan,
Thanks for the reroll!
I left a few comments on the approach, I probably wasn't clear enough when I first commented, apologies for that.
I would think that as the JSON payload we get here (OIDC or other) is not a account file (or its respective JSON contents), we should not try to adapt the logic for the ServiceAccount to accept those, instead I would argue it is better to segregate it completely, and use the credentials to connect to GCE after that.
Please let me know what you think, and if you need help with this PR, don't hesitate to ask!
msarahan
force-pushed
the
relax-JWT-Error
branch
from
5fb41bb
to
c555801
Compare
|
@lbajolet-hashicorp I think this is close, but probably not quite working. It really needs a deeper integration test, to ensure that the credentials stuff is plumbed in throughout. It went a lot further than I was expecting. Would you mind helping to set up the integration test, and then I'll clean up any remaining brokenness that I've missed here? |
|
Hi @msarahan, Looking at your last draft, I like how it's turning out, thanks for persisting on the rerolls! I'll take care of the acceptance tests, no worries; I'll let you know when I'm done with this (I'll take care of that this week unless something gets in the way). |
|
Well @msarahan, to quote you "It went a lot further than I was expecting.", that's two of us now. The more I look into this, the more I think we should revisit how we do authentication for the plugin, there's some inconsistencies and stuff that's not properly documented that may hinder us from moving forward, this plus the fact that both the builder and post-processors rely on the same code, all in all make me think we should introduce some kind of common package for all to rely on (right now post-processors use code from the builder, which while it works, it feels a bit off). Thanks for the initial work @msarahan, if you don't mind me tinkering in this, I'll probably reroll a few things on your branch, I've started some ground work there before working on the acceptance tests, apologies if I overstep, please let me know if you'd be OK in me continuing this work on your branch. |
|
By all means, have at it! If I can help at all, let me know. |
The information is not relevant for the artifact we created, so we leave it out of the artifact.
As with the credentials, this should not be part of the artifact. This can be a breaking change, but since the account_file may contain a raw JWT representing an account, this could be leaked to post-processors, which can turn in a security incident. To avoid this problem, we opt to remove it entierly from the artifact.
The error type being a pointer to a multierror made the check mandatory as otherwise the returned error interface would never count as nil since the type was pre-set, even if its value was indeed a nil pointer. To avoid this problem, we expose the error as an interface type, so that the final check can be avoided and we can return it as-is, without checking its contents first.
lbajolet-hashicorp
force-pushed
the
relax-JWT-Error
branch
from
df15a05
to
78778ba
Compare
This commit reifies authentication for account and other supported ways of authentication by the Google SDK, namely OIDC/account file/GCE client creds. The `account_file' option is deprecated as part of this work, as the credentials options support this method as well, and allows us to simplify how users setup their authentication methods. The next steps for authentication will be to reify the authentication logic, as for now it is duplicated among the different components of the plugin, while this logic may be extracted and unified under some common library.
lbajolet-hashicorp
force-pushed
the
relax-JWT-Error
branch
from
78778ba
to
549c516
Compare
There was a problem hiding this comment.
Overall I really like the re-roll. It is clean, provides the user with clear authentication options, and calls out the deprecation. Were you able to run all the acceptance tests?
I'm waiting to refresh my credentials to test. I'm approving in advance to not block this if you have already executed the acceptance tests.
| @@ -140,8 +140,6 @@ func (a *Artifact) State(name string) interface{} { | |||
| return a.image.Name | |||
| case "ImageSizeGb": | |||
| return a.image.SizeGb | |||
| case "AccountFilePath": | |||
There was a problem hiding this comment.
Nice! This removal needs to be added in the CHANGELOG to denote that the artifact response has changed.
There was a problem hiding this comment.
Yes! We should also call-out that with this gone, the googlecompute-export post-processor will need its own set of credentials, as it will not harvest it from the artifact after this change.
| log.Printf("account_file is deprecated, please use either credentials_json or credentials_file instead") | ||
| // Heuristic, but should be good enough to discriminate between | ||
| // the two somewhat reliably. | ||
| if strings.HasPrefix(strings.TrimSpace(p.config.AccountFile), "{") { |
There was a problem hiding this comment.
An alternative approach if you want to gain more confidence would be to create an anonymous struct and use json.Unmarshal to validate that one of the fields is populated or maybe just check that there is no unmarshal error.
Using os.Stat to check it the string is a file is not a good option as it may lead to false positives if the file doesn't exist. So that's not an alternative.
This is just a comment to provide an alternative not that it needs to be done.
There was a problem hiding this comment.
Yeah unmarshalling might be an idea as well indeed, I think however we're good with this heuristic.
It's less secure than the alternative as it's thrown it off with a filename that starts with {, but thankfully I'd think this is extremely rare, and in such cases, I would easily suggest the user moves to using credentials_file :)
I haven't run them yet, I would expect them to run as they don't rely on any auth method (so that defaults to the default creds set), but yes they should run. |
There was a problem hiding this comment.
Acceptance tests, with the last change from #198 do run and succeed, using the *google.Credentials structure. This should be ripe to merge now!
The idea here is to allow OIDC to work with this plugin. As documented in #139, this plugin currently errors out when encountering OIDC credentials because the type field does not match. I looked in the source code of this plugin, and it seems like the JWT-specific functionality is used for things that aren't strictly necessary. This PR takes the approach of squelching the error when the wrong type field is encountered, and just returning a ServiceAccount with a nil jwt pointer. We adjust some of the checks to be checks on account.jwt instead of just account.
I added some tests. I don't know exactly what is required for a valid JWT token, so I generated one on my google account and anonymized it a bit. The private key and key_id are unchanged, but I have deleted this key from my account to avoid leaking any important credentials.
Closes #139