feat(auth): add OAuth2/OIDC authentication package #452
Conversation
cpunion
commented
Dec 31, 2025
cpunion
commented
- Add AuthConfig, AuthCredential, AuthScheme types
- Add AuthHandler for OAuth2 flow orchestration
- Add CredentialManager and CredentialService for token management
- Add OAuth2 token exchange helpers
- Add unit tests for auth package
- Add AuthConfig, AuthCredential, AuthScheme types - Add AuthHandler for OAuth2 flow orchestration - Add CredentialManager and CredentialService for token management - Add OAuth2 token exchange helpers - Add unit tests for auth package
Summary of ChangesHello @cpunion, 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 introduces a new, robust authentication package designed to streamline the handling of various authentication methods, particularly focusing on OAuth2 and OpenID Connect. It provides foundational types for defining authentication schemes and credentials, along with a manager to orchestrate the entire credential lifecycle, from initial acquisition to refresh and persistence. This enhancement aims to simplify secure integration with external services requiring authentication. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive authentication package for handling OAuth2/OIDC, API keys, and other schemes. The implementation is well-structured with clear separation of concerns for configuration, credentials, handlers, and services. The code is generally of high quality with good test coverage for the new components. My review has identified several critical and high-severity issues, primarily related to error handling and the correct use of the OAuth2 library. Specifically, there are multiple places where errors are silently ignored, which can lead to hard-to-debug failures and potential security vulnerabilities. Additionally, the implementation of the OAuth2 client credentials flow is incorrect, and there's a logic bug in handling OIDC schemes. Addressing these points will significantly improve the robustness and security of this new authentication package.
|
|
||
| // generateAuthURI generates the OAuth authorization URI. | ||
| func (h *AuthHandler) generateAuthURI() *AuthCredential { | ||
| oauth2Scheme, ok := h.authConfig.AuthScheme.(*OAuth2Scheme) |
There was a problem hiding this comment.
GenerateAuthRequest is designed to work with both SecuritySchemeTypeOAuth2 and SecuritySchemeTypeOpenIDConnect. However, generateAuthURI only performs a type assertion for *OAuth2Scheme. If an *OpenIDConnectScheme is passed, this assertion will fail, ok will be false, and the function will return nil. This causes auth URI generation to fail silently for OIDC. The logic must be updated to handle *OpenIDConnectScheme as well, likely by extracting the necessary flow information from it.
| // generateRandomState generates a random state string for OAuth CSRF protection. | ||
| func generateRandomState() string { | ||
| b := make([]byte, 16) | ||
| rand.Read(b) |
There was a problem hiding this comment.
The error from crypto/rand.Read is ignored. If this call fails (e.g., due to entropy exhaustion on the system), the buffer b will not be filled with random data, resulting in a predictable OAuth state parameter. This completely undermines the CSRF protection that the state parameter is meant to provide. This error is critical and must be handled, for example by panicking, as the application cannot proceed securely.
Example fix:
if _, err := rand.Read(b); err != nil {
panic(fmt.Sprintf("CRITICAL: failed to generate random state for OAuth: %v", err))
}| } | ||
|
|
||
| // Use client credentials grant | ||
| token, err := config.Exchange(ctx, "", oauth2.SetAuthURLParam("grant_type", "client_credentials")) |
There was a problem hiding this comment.
This is not the correct way to perform an OAuth2 client credentials grant with the golang.org/x/oauth2 library. The config.Exchange function is intended for the authorization code flow. For the client credentials flow, you should use the golang.org/x/oauth2/clientcredentials package. Using Exchange this way is undocumented, likely to fail, and could break in future library versions.
Here is the correct approach:
import "golang.org/x/oauth2/clientcredentials"
// ...
conf := &clientcredentials.Config{
ClientID: cred.OAuth2.ClientID,
ClientSecret: cred.OAuth2.ClientSecret,
TokenURL: oauth2Scheme.Flows.ClientCredentials.TokenURL,
Scopes: scopeKeys(oauth2Scheme.Flows.ClientCredentials.Scopes),
}
token, err := conf.Token(ctx)
if err != nil {
return nil, fmt.Errorf("failed to exchange client credentials: %w", err)
}| schemeJSON, _ := json.Marshal(c.AuthScheme) | ||
| h := sha256.Sum256(schemeJSON) | ||
| schemePart = fmt.Sprintf("%s_%x", schemeType, h[:8]) | ||
| } | ||
|
|
||
| var credPart string | ||
| if c.RawAuthCredential != nil { | ||
| credJSON, _ := json.Marshal(c.RawAuthCredential) |
There was a problem hiding this comment.
Errors from json.Marshal are ignored on lines 54 and 61. If marshalling fails (e.g., due to an unsupported type in the future), this will lead to silent failures and incorrect credential key generation, potentially causing key collisions. These errors should be handled, for example by having generateCredentialKey return an error which is propagated up to NewAuthConfig.
| if len(credentialService) > 0 && credentialService[0] != nil { | ||
| svc = credentialService[0] | ||
| if credential == nil { | ||
| loaded, _ := svc.LoadCredential(ctx, m.authConfig) |
There was a problem hiding this comment.
The error returned by svc.LoadCredential is ignored. If the credential service fails to load a credential (e.g., due to a database connection issue or I/O error), the error is silently dropped. This can hide underlying infrastructure problems and may lead to unnecessary re-authentication flows. The error should be handled, at a minimum by logging it to aid in debugging.
| if err != nil { | ||
| // On refresh failure, return original | ||
| return cred, false | ||
| } |
There was a problem hiding this comment.
The error from ref.Refresh is silently ignored. If token refresh fails, the manager returns the original, likely expired, credential. This will cause subsequent API calls to fail with an authentication error, which can be hard to debug. The refresh error should be propagated to the caller of GetAuthCredential so it can be handled appropriately (e.g., by initiating a new auth flow).
| if err := json.Unmarshal(credentialJSON, &saCred); err == nil { | ||
| cred.ServiceAccount.ServiceAccountCredential = &saCred | ||
| } |
There was a problem hiding this comment.
The error from json.Unmarshal is silently ignored. If the credentialJSON is invalid, the function will proceed without setting ServiceAccountCredential, and the caller will not be notified of the configuration error. This can be very difficult to debug. The function should be modified to return an error to the caller if unmarshalling fails.
1 participant
