Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting more credential formats #75

Closed
wants to merge 7 commits into from
Closed

Conversation

msdrigg
Copy link

@msdrigg msdrigg commented Aug 17, 2023

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.

- Supports service account impersonation
- Supports GOOGLE_APPLICATION_CREDENTIALS pointing to application_default_credentials.json

- Does not support external account credentials still
@msdrigg
Copy link
Author

msdrigg commented Aug 17, 2023

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

@msdrigg
Copy link
Author

msdrigg commented Aug 17, 2023

The existing do tests pass with this PR though.

msdrigg and others added 2 commits August 18, 2023 07:33
Co-authored-by: Fuyang Liu <liufuyang@users.noreply.github.com>
@djc
Copy link
Owner

djc commented Aug 18, 2023

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 #56 (comment) 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
Copy link
Author

msdrigg commented Aug 18, 2023

Okay this morning I tested this in the real world

  • I tested using GOOGLE_APPLICATION_CREDENTIALS and ~/.config/gcloud/application_default_credentials.json
  • I tested with a service account key (downloaded from google cloud console)
  • I tested with an impersonated service account key (created with gcloud auth application-default login --impersonate-service-account <account>)
  • I tested with a standard user credential (created with gcloud auth application-default login)

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 msdrigg marked this pull request as ready for review August 18, 2023 12:10
@msdrigg
Copy link
Author

msdrigg commented Aug 18, 2023

@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
Copy link
Owner

djc commented Aug 18, 2023

Yes, breaking down into those three chunks sounds good!

@msdrigg
Copy link
Author

msdrigg commented Aug 18, 2023

See you in a future pr :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants