djc / gcp_auth

Minimal authentication library for Google Cloud Platform (GCP)
Other
63 stars 38 forks source link

Supporting more credential formats #75

Closed msdrigg closed 1 year ago

msdrigg commented 1 year ago

Solves #56.

This PR adds a new ServiceAccount implementation called FlexibleTokenSource. FlexibleTokenSource combines the functionality currently supplied by ConfigDefaultCredentials and CustomServiceAccount, and it improves the flexibility of each, currently supporting the service account, the user default credential account, and service account impersonation. All three of these approaches will work with GOOGLE_APPLICATION_CREDENTIALS and ~/.config/gcloud/application_default_credentials.

In AuthenticationManager::new(), I replace ConfigDefaultCredentials and CustomServiceAccount parsing attempts with the new FlexibleTokenSource, because gives these improvements to both cases.

Additionally, I removed the ConfigDefaultCredentials struct altogether. I was not able to remove CustomServiceAccount because it is exposed in the public API for this crate. It also seems like CustomServiceAccount serves a specialized use case because it exposes methods like CustomServiceAccount::signer(), which is only possible when using the credential format with a private_key field.

I am leaving this as a draft for now because I want to get feedback on if this is the kind of PR you are looking for before I do the necessary testing in the real world.

msdrigg commented 1 year ago

Also wondering if there are any documentation you would like to see for these items, and any automated tests.

msdrigg commented 1 year ago

The existing do tests pass with this PR though.

djc commented 1 year ago

This seems to be centralizing/rewriting a lot of the logic into the FlexibleSource implementation, which makes the library more monolithic (and thereby, in my opinion, harder to understand). I think the design I proposed in https://github.com/hrvolapeter/gcp_auth/issues/56#issuecomment-1669404489 would keep things more similar to how they were before, except that some of the discovery logic for the AuthenticationManager would hang off of the deserialized enum from the contents of the file referenced in GOOGLE_APPLICATION_CREDENTIALS.

Also in order to make large changes like these easy to review for the maintainers (at least, for me), these should be cleaned up and split up into smaller commits. For example, I would suggest improving ConfigDefaultCredentials and CustomServiceAccount in place, each in a separate commit or maybe even separate PRs.

msdrigg commented 1 year ago

Okay this morning I tested this in the real world

Had to make some changes, but everything is working now

I also added tests to parse all the key formats I added here.

I added some documentation of why I am doing things the way I do them. Since this is an internal module, I believe this documentation should be sufficient.

I would appreciate if someone interested in this PR to give this a whirl in their environment, but other than that I think this PR is ready

msdrigg commented 1 year ago

@djc Sorry I had typed up that comment before seeing your response.

Firstly, there is a problem with just checking GOOGLE_APPLICATION_CREDENTIALS and only using the FlexibleCredentialSource approach in that case. The problem is that we also need to check ~/.config/gcloud/application_default_credentials.json because I can run gcloud auth application-default login --impersonate-service-account <account>.

So here's what I propose to break this down into manageable chunks.

  1. PR for supporting both formats
    • Add an enum like FlexibleTokenSource that instead of handling the token refreshes itself, would use existing code from ConfigDefaultCredentials or CustomServiceAccount.
    • Use this enum in application_manager.rs to check both the ~/.config/gcloud/application_default_credentials.json file and the GOOGLE_APPLICATION_CREDENTIALS and then throw off to the existing ConfigDefaultCredentials or CustomServiceAccount code.
  2. PR for service account impersonation
    • Add independent struct similar to ConfigDefaultCredentials or CustomServiceAccount to support service account impersonation.
    • To keep this non-monolithic, I would need to have a struct like Box<dyn ServiceAccount> to handle getting the source credentials (does this sound okay?)
    • Update FlexibleTokenSource to support service account impersonation
  3. PR for general improvements
    • From looking over the documentation, there are discrepancies between here and the go google oauth source that should be reconciled (for example allowing a fixed audience as the audience instead of token_uri).
djc commented 1 year ago

Yes, breaking down into those three chunks sounds good!

msdrigg commented 1 year ago

See you in a future pr :)