-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(auth): deprecate unsafe credentials JSON loading options #13397
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
base: main
Are you sure you want to change the base?
Conversation
* Add deprecation comments to credentials.DetectOptions.CredentialsFile and credentials.DetectOptions.CredentialsJSON. * Update documentation to recommend new safe load methods. * Add credentials.CredentialsType. * Add credentials.NewCredentialsFromJSON and credentials.NewCredentialsFromFile. * Add deprecation comments to idtoken.Options.CredentialsFile and idtoken.Options.CredentialsJSON. * Add idtoken.NewCredentialsFromJSON and idtoken.NewCredentialsFromFile.
* Replace internal integer-based `CredentialsType` (iota) with
public string-based constants for specifying credential file
types. This change:
* Eliminates the zero-value problem
* Improves readability and debugging
* Ensures consistency with JSON data
Summary of ChangesHello @quartzmo, 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 refactors how credential configurations are handled, primarily focusing on improving security and clarity. It introduces new, type-specific functions for loading credentials from files or JSON, which enforce type validation and provide explicit security warnings for certain credential types. Concurrently, older, less secure methods for loading credentials directly via Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant security improvements by deprecating unsafe credential loading methods and adding new, safer alternatives that require explicit credential type specification. The move from integer-based iota to string constants for credential types is a great change for readability and robustness. The new functions are well-documented with clear security warnings, and the deprecation messages for the old fields are very informative. The changes are supported by a good set of new tests. I have identified a couple of important issues regarding context.Context propagation in the new functions that should be addressed before merging.
2026c11 to
f66176d
Compare
| // CredType specifies the type of JSON credentials being provided | ||
| // to a loading function such as [NewCredentialsFromFile] or | ||
| // [NewCredentialsFromJSON]. | ||
| type CredType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why string and not an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Replace internal integer-based CredentialsType (iota) with public string-based constants for specifying credential file types. This change:
- Eliminates the zero-value problem.
- Improves readability and debugging.
- Ensures consistency with JSON data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you asked about this. I initially went with an iota enum, the standard approach. But the string typecodes were already very much baked into the library internally in the parseCredentialType function. And for error messages I would need a reverse conversion back to the iota enum, which I added in the first commit in this PR: ParseCredentialTypeString. Given that the string typecodes were used in every operation concerning the iota enum already, and would even be completely round-tripped for an error, I decided it made sense to elevate them to the exposed types. And thus in ad56c84 I was able to delete the functions below.
The GCA review above was very favorable: Credential Type Standardization: Introduced a new public CredentialsType string enum to replace an internal integer-based CredentialType, improving readability, debugging, and consistency across credential handling.
// parseCredentialType returns the associated filetype based on the parsed
// typeString provided.
func parseCredentialType(typeString string) CredentialType {
switch typeString {
case "service_account":
return ServiceAccountKey
case "authorized_user":
return UserCredentialsKey
case "impersonated_service_account":
return ImpersonatedServiceAccountKey
case "external_account":
return ExternalAccountKey
case "external_account_authorized_user":
return ExternalAccountAuthorizedUserKey
case "gdch_service_account":
return GDCHServiceAccountKey
default:
return UnknownCredType
}
}
// ParseCredentialTypeString returns the associated filetype string based
// on the parsed type code int provided.
func ParseCredentialTypeString(credType CredentialType) string {
switch credType {
case ServiceAccountKey:
return "service_account"
case UserCredentialsKey:
return "authorized_user"
case ImpersonatedServiceAccountKey:
return "impersonated_service_account"
case ExternalAccountKey:
return "external_account"
case ExternalAccountAuthorizedUserKey:
return "external_account_authorized_user"
case GDCHServiceAccountKey:
return "gdch_service_account"
default:
return "unknown"
return "", err
}
return f.Type, nil
}
and credentials.DetectOptions.CredentialsJSON.
and idtoken.Options.CredentialsJSON.
CredentialsType(iota) withpublic string-based constants for specifying credential file
types. This change: