feat: add --impersonate-service-account flag for keyless per-mount id…#4463
Conversation
…entity Implements GitHub issue GoogleCloudPlatform#4422. Adds support for service account impersonation using short-lived credentials via the IAM Credentials API, eliminating the need for persistent SA key files. Changes: - Add impersonate-service-account param to cfg/params.yaml - Add ImpersonateServiceAccount field to GcsAuthConfig struct - Register CLI flag and viper binding in config.go - Add validation: reject with anonymous-access, validate email format - Implement impersonation in GetTokenSource() (oauth2 path) - Implement impersonation in GetClientAuthOptionsAndToken() (new auth path) - Wire config through StorageClientConfig and legacy_main.go - Add unit tests for validation, auth, and flag parsing Closes GoogleCloudPlatform#4422
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the authentication mechanism by enabling service account impersonation. Users can now specify a target service account via a new CLI flag, allowing the application to obtain short-lived credentials through the IAM Credentials API. This change improves security and simplifies credential management for Google Cloud Storage access by eliminating the need for persistent service account key files. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to impersonate a service account using the --impersonate-service-account flag. However, a critical vulnerability exists where the code fails to use the user-provided base credentials for impersonation requests, falling back to Application Default Credentials (ADC). This can lead to unexpected behavior and potential security risks if ADC has broader permissions. Additionally, the email validation for the service account is weak, and there's a lack of test coverage for the new impersonation flow.
| // wrapWithImpersonation returns a token source that impersonates the given | ||
| // service account. It uses the ambient credentials (ADC, metadata server, etc.) | ||
| // to obtain short-lived access tokens for the target SA via the IAM Credentials API. | ||
| func wrapWithImpersonation(ctx context.Context, targetServiceAccount string) (oauth2.TokenSource, error) { | ||
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | ||
| TargetPrincipal: targetServiceAccount, | ||
| Scopes: []string{storagev1.DevstorageFullControlScope}, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | ||
| } | ||
| return ts, nil | ||
| } |
There was a problem hiding this comment.
This function incorrectly discards the existing token source and defaults to using Application Default Credentials (ADC) for impersonation. This is a critical bug because it means credentials from token-url or key-file will be ignored when impersonation is active.
The function signature should be updated to accept the baseTokenSource, and this source should be passed to impersonate.CredentialsConfig. The call sites on lines 42 and 62 will also need to be updated to pass the tokenSrc.
This function is also a duplicate of newImpersonatedTokenSource in internal/auth/auth.go. It would be best to consolidate them into a single, shared utility function.
| // wrapWithImpersonation returns a token source that impersonates the given | |
| // service account. It uses the ambient credentials (ADC, metadata server, etc.) | |
| // to obtain short-lived access tokens for the target SA via the IAM Credentials API. | |
| func wrapWithImpersonation(ctx context.Context, targetServiceAccount string) (oauth2.TokenSource, error) { | |
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | |
| TargetPrincipal: targetServiceAccount, | |
| Scopes: []string{storagev1.DevstorageFullControlScope}, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | |
| } | |
| return ts, nil | |
| } | |
| // wrapWithImpersonation returns a token source that impersonates the given | |
| // service account. It uses the provided base token source to obtain short-lived | |
| // access tokens for the target SA via the IAM Credentials API. | |
| func wrapWithImpersonation(ctx context.Context, baseTokenSource oauth2.TokenSource, targetServiceAccount string) (oauth2.TokenSource, error) { | |
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | |
| TargetPrincipal: targetServiceAccount, | |
| Scopes: []string{storagev1.DevstorageFullControlScope}, | |
| TokenSource: baseTokenSource, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | |
| } | |
| return ts, nil | |
| } |
| func newImpersonatedTokenSource(ctx context.Context, baseTokenSource oauth2.TokenSource, targetServiceAccount string) (oauth2.TokenSource, error) { | ||
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | ||
| TargetPrincipal: targetServiceAccount, | ||
| Scopes: []string{storagev1.DevstorageFullControlScope}, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | ||
| } | ||
| return ts, nil | ||
| } |
There was a problem hiding this comment.
The newImpersonatedTokenSource function takes a baseTokenSource argument but does not use it when calling impersonate.CredentialsTokenSource. This means that if a user provides a specific key file or token URL for authentication, it will be ignored during the impersonation process, and the application will fall back to using Application Default Credentials (ADC). This violates the principle of least privilege if the ADC has more permissions than the provided credentials, and it contradicts the function's documentation. Additionally, this function is nearly identical to wrapWithImpersonation in internal/storage/storageutil/auth_client_option.go. Consider refactoring to a single shared function to avoid code duplication and ensure consistent behavior.
| func newImpersonatedTokenSource(ctx context.Context, baseTokenSource oauth2.TokenSource, targetServiceAccount string) (oauth2.TokenSource, error) { | |
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | |
| TargetPrincipal: targetServiceAccount, | |
| Scopes: []string{storagev1.DevstorageFullControlScope}, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | |
| } | |
| return ts, nil | |
| } | |
| func newImpersonatedTokenSource(ctx context.Context, baseTokenSource oauth2.TokenSource, targetServiceAccount string) (oauth2.TokenSource, error) { | |
| ts, err := impersonate.CredentialsTokenSource(ctx, impersonate.CredentialsConfig{ | |
| TargetPrincipal: targetServiceAccount, | |
| Scopes: []string{storagev1.DevstorageFullControlScope}, | |
| }, option.WithTokenSource(baseTokenSource)) | |
| if err != nil { | |
| return nil, fmt.Errorf("impersonate.CredentialsTokenSource(%s): %w", targetServiceAccount, err) | |
| } | |
| return ts, nil | |
| } |
| if config.ImpersonateServiceAccount != "" { | ||
| tokenSrc, err = wrapWithImpersonation(ctx, config.ImpersonateServiceAccount) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("while impersonating service account: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
In GetClientAuthOptionsAndToken, when ImpersonateServiceAccount is specified, the code calls wrapWithImpersonation which creates a new token source using ambient credentials (ADC). This new token source overwrites the tokenSrc previously created from TokenUrl or KeyFile. Consequently, any user-provided credentials for authentication are ignored, and the application incorrectly relies on ADC for the impersonation request. The wrapWithImpersonation function should be updated to accept and use the base token source.
| if config.ImpersonateServiceAccount != "" { | ||
| tokenSrc, err = wrapWithImpersonation(ctx, config.ImpersonateServiceAccount) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("while impersonating service account: %w", err) | ||
| } | ||
| clientOpts := []option.ClientOption{option.WithTokenSource(tokenSrc)} | ||
| return clientOpts, tokenSrc, nil | ||
| } |
There was a problem hiding this comment.
Similar to the issue in the TokenUrl path, when using a KeyFile with impersonation, the tokenSrc derived from the KeyFile is overwritten by a new token source that uses ADC. This results in the KeyFile being ignored for the impersonation request, potentially leading to unintended credential usage if the environment's ADC has different permissions than the specified KeyFile.
| // Validate service account email format: must contain @ and end with .iam.gserviceaccount.com or similar. | ||
| saEmail := authConfig.ImpersonateServiceAccount | ||
| if !strings.Contains(saEmail, "@") || !strings.Contains(saEmail, ".") { | ||
| return errors.New(ImpersonateInvalidEmailError) | ||
| } |
There was a problem hiding this comment.
The validation for the service account email is weak and doesn't fully align with the comment's description. The current check allows some invalid email formats to pass. Using net/mail.ParseAddress from the standard library would provide a more robust validation.
Additionally, please note that impersonate.CredentialsTokenSource can impersonate user accounts as well, not just service accounts. The current flag name and error messages are specific to service accounts, which might be misleading if user account impersonation is also a possibility. If only service accounts are intended, the validation could be even stricter (e.g., checking for a .iam.gserviceaccount.com suffix).
To use mail.ParseAddress, you'll need to add "net/mail" to your imports.
| // Validate service account email format: must contain @ and end with .iam.gserviceaccount.com or similar. | |
| saEmail := authConfig.ImpersonateServiceAccount | |
| if !strings.Contains(saEmail, "@") || !strings.Contains(saEmail, ".") { | |
| return errors.New(ImpersonateInvalidEmailError) | |
| } | |
| // Validate that the impersonate-service-account is a valid email address. | |
| saEmail := authConfig.ImpersonateServiceAccount | |
| if _, err := mail.ParseAddress(saEmail); err != nil { | |
| return errors.New(ImpersonateInvalidEmailError) | |
| } |
| func (t *AuthTest) TestGetTokenSource_WithKeyFile() { | ||
| tokenSrc, err := GetTokenSource( | ||
| context.Background(), | ||
| "testdata/google_creds.json", | ||
| "", | ||
| false, | ||
| "", | ||
| ) | ||
|
|
||
| assert.NoError(t.T(), err) | ||
| assert.NotNil(t.T(), tokenSrc) | ||
| } | ||
|
|
||
| func (t *AuthTest) TestGetTokenSource_WithEmptyImpersonation() { | ||
| // When impersonate SA is empty, it should fall through to default token source. | ||
| // With testdata key file, this should succeed without impersonation wrapping. | ||
| tokenSrc, err := GetTokenSource( | ||
| context.Background(), | ||
| "testdata/google_creds.json", | ||
| "", | ||
| false, | ||
| "", | ||
| ) | ||
|
|
||
| assert.NoError(t.T(), err) | ||
| assert.NotNil(t.T(), tokenSrc) | ||
| } | ||
|
|
||
| func (t *AuthTest) TestGetTokenSource_WithInvalidKeyFile() { | ||
| tokenSrc, err := GetTokenSource( | ||
| context.Background(), | ||
| "non-existent-key-file.json", | ||
| "", | ||
| false, | ||
| "", | ||
| ) | ||
|
|
||
| assert.Error(t.T(), err) | ||
| assert.Nil(t.T(), tokenSrc) | ||
| } |
There was a problem hiding this comment.
The new tests for GetTokenSource do not cover the scenario where service account impersonation is enabled. Adding a test case for this would improve test coverage and help ensure the feature works as expected. Such a test would likely have caught the issue where the base token source is ignored during impersonation.
- Fix critical bug: pass base credentials to impersonate.CredentialsTokenSource via option.WithTokenSource(baseTokenSource) instead of falling back to ADC - Remove duplicate wrapWithImpersonation in auth_client_option.go; reuse exported auth.NewImpersonatedTokenSource from auth.go - Strengthen validation: use net/mail.ParseAddress and enforce .iam.gserviceaccount.com suffix - Add tests: TestNewImpersonatedTokenSource_UsesBaseTokenSource and TestGetTokenSource_WithImpersonation for impersonation flow coverage - Add validation test cases for non-SA emails and display-name injection
…entity
Implements GitHub issue #4422. Adds support for service account impersonation using short-lived credentials via the IAM Credentials API, eliminating the need for persistent SA key files.
Changes:
Closes #4422