hashicorp / packer-plugin-googlecompute

Packer plugin for Google Compute Builder
https://www.packer.io/docs/builders/googlecompute
Mozilla Public License 2.0
23 stars 51 forks source link

relax GoogleCompute auth file loading to allow OIDC creds #186

Closed msarahan closed 7 months ago

msarahan commented 9 months ago

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

hashicorp-cla commented 9 months ago

CLA assistant check
All committers have signed the CLA.

msarahan commented 9 months ago

Thanks for enabling CI. I see the failures here, and I'll work on addressing them.

msarahan commented 8 months ago

ping @hashicorp/packer

msarahan commented 8 months ago

Thanks @lbajolet-hashicorp!

adding an OIDC acceptance test

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.

lbajolet-hashicorp commented 8 months ago

Regarding the acceptance test, it's essentially a test on real infrastructure. It'll take some setup and a template that leverages it, and test it on one of our projects. I mentioned it to be clear, but since this will run on our environments, I'll take care of that once you've rerolled the code, this way we can be sure it works as intended in a Github CI environment (which is what you're aiming for if I take the examples you've added to the repo).

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 commented 8 months ago

@lbajolet-hashicorp I think this new approach is much less brittle. I try the JWT method first, and if it fails, fall back to google.CredentialsFromJSON. We don't actually use that result for anything right now, but it's stored as part of the ServiceAccount object. If both attempts to load from JSON fail, then an error message is propagated for both of the attempts.

msarahan commented 8 months ago

@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?

lbajolet-hashicorp commented 8 months ago

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).

lbajolet-hashicorp commented 8 months ago

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.

msarahan commented 8 months ago

By all means, have at it! If I can help at all, let me know.

lbajolet-hashicorp commented 7 months ago

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.

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. I'll also add some other tests for OIDC (I'd also like to confirm how the others work, and if they still do). This may come in a later PR though, since I'm reorganising the auth code (all the common code really), so that can be something that comes in along with this work.