Add more config variables #9
Conversation
WalkthroughAdds configurable OIDC/OAuth2 options: Scopes, three Skip* boolean flags, and TokenValidators; exposes builder methods and FromEnv parsing, wires flags and validators into provider/validator initialization and OAuth2/discovery metadata; adds parseBoolEnv helper and necessary imports. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / ConfigBuilder
participant Env as FromEnv
participant Prov as Provider.Init
participant Val as OIDCValidator
participant TF as TokenValidators
App->>Env: FromEnv / WithScopes / WithSkipX / WithTokenValidators
Env-->>App: Config{Scopes, Skip* flags, TokenValidators}
App->>Prov: createProvider(cfg)
Prov->>Val: init validator (apply Skip* flags, assign TokenValidators)
Val->>Val: perform core checks (issuer/audience/expiry) — some may be skipped
alt core checks pass or are skipped
Val->>TF: for each -> TokenValidator(claims)
TF-->>Val: error / nil
end
Val-->>Prov: validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-12-13T17:03:53.298ZApplied to files:
📚 Learning: 2025-12-13T17:03:53.298ZApplied to files:
📚 Learning: 2025-12-13T17:03:53.298ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.go(4 hunks)provider/provider.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config.go (2)
provider/provider.go (1)
Logger(24-29)logger.go (1)
Logger(22-27)
🔇 Additional comments (4)
provider/provider.go (2)
33-40: LGTM with security caveat.The new configuration fields are correctly defined. However, these flags bypass critical security validations.
Ensure these flags are:
- Documented with clear security warnings
- Used only in development/testing environments
- Never enabled in production
Consider adding runtime warnings when these checks are disabled.
209-211: LGTM!The validator configuration correctly maps the new config flags to the OIDC verifier settings, replacing the previously hardcoded
falsevalues.config.go (2)
34-38: LGTM!The configuration fields are correctly defined with appropriate documentation.
318-320: Missing test coverage for security bypass flags.Verification confirms no tests exist for the new skip flags. Critical test coverage gaps:
- Environment variable parsing tests (
OIDC_SKIP_AUDIENCE_CHECK,OIDC_SKIP_ISSUER_CHECK,OIDC_SKIP_EXPIRY_CHECK)- Validation that flags default to
false(secure-by-default)- Independent flag behavior verification
- Combined flag scenarios
Add tests covering all above cases before merging, particularly to ensure security defaults are maintained.
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
from
bd04072 to
8d50963
Compare
WhammyLeaf
changed the title
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
2 times, most recently
from
31b7df3 to
5fbe515
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config.go (2)
36-39: LGTM: Skip validation flags added correctly.The three boolean fields are well-named and properly integrated into the Config struct. They default to false (validation enabled), which is the safe default.
Optional: Consider adding doc comments to each field explaining the security implications of skipping these validation checks, especially for users who may not fully understand OIDC security requirements.
327-338: LGTM: Robust boolean parsing with safe defaults.The
parseBoolEnvhelper correctly usesstrconv.ParseBoolto handle standard boolean representations and safely falls back to the default value on parse errors or empty values. This fail-safe approach ensures validation remains enabled by default.Optional: Consider logging parse errors to aid debugging when users provide invalid boolean values (e.g., "yes" instead of "true"). However, the current silent fallback is safe and acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.go(5 hunks)provider/provider.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/provider.go
🧰 Additional context used
🧬 Code graph analysis (1)
config.go (2)
provider/provider.go (1)
Logger(24-29)logger.go (1)
Logger(22-27)
🔇 Additional comments (4)
config.go (4)
5-5: LGTM: Import added for boolean parsing.The
strconvimport is necessary for theparseBoolEnvhelper and is correctly placed.
127-135: LGTM: Skip flags correctly propagated to provider config.The mapping from
Configtoprovider.Configis correct, including the previously identified copy-paste error on line 135 which has been properly fixed (now correctly mapsSkipExpiryChecktocfg.SkipExpiryCheck).
235-251: LGTM: Builder methods follow the pattern correctly.The three new builder methods are properly implemented following the fluent builder pattern. Documentation comments are correct and consistent (previous typos have been fixed).
319-321: LGTM: Environment variable parsing implemented correctly.The skip flags are properly initialized from environment variables using the
parseBoolEnvhelper, which correctly handles boolean values. Safe defaults (false = validation enabled) are used, and the environment variable names follow clear naming conventions.
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
from
5fbe515 to
4d5a1f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/provider.go (1)
264-267: Explicit audience validation runs even whenSkipAudienceCheckis true.When
cfg.SkipAudienceCheckis set totrue, the go-oidc library skips its audience check (viaSkipClientIDCheckat line 211), butvalidateAudience()at line 265 still runs unconditionally. This makes the skip flag ineffective.- // Validate audience claim for security (explicit check) - if err := v.validateAudience(rawClaims); err != nil { - return nil, fmt.Errorf("audience validation failed: %w", err) - } + // Validate audience claim for security (explicit check) unless skipped + if !v.skipAudienceCheck { + if err := v.validateAudience(rawClaims); err != nil { + return nil, fmt.Errorf("audience validation failed: %w", err) + } + }You'll need to store
cfg.SkipAudienceCheckinOIDCValidatorduring initialization.
🧹 Nitpick comments (4)
metadata.go (1)
245-245: Inconsistent scopes in metadata endpoints.
HandleOIDCDiscoverynow uses dynamich.config.Scopes, butGetAuthorizationServerMetadata()(lines 285 and 317) still uses hardcoded[]string{"openid", "profile", "email"}. This inconsistency could confuse clients depending on which metadata endpoint they query.Consider updating
GetAuthorizationServerMetadata()to also useh.config.Scopesfor consistency:// In native mode (line 285): - "scopes_supported": []string{"openid", "profile", "email"}, + "scopes_supported": h.config.Scopes, // In proxy mode (line 317): - "scopes_supported": []string{"openid", "profile", "email"}, + "scopes_supported": h.config.Scopes,config.go (2)
234-262: Consider adding builder method forTokenValidationFuncs.For consistency with the fluent builder pattern, consider adding a
WithTokenValidationFuncsmethod:// WithTokenValidationFuncs sets custom token validation functions func (b *ConfigBuilder) WithTokenValidationFuncs(funcs []func(claims jwt.MapClaims) error) *ConfigBuilder { b.config.TokenValidationFuncs = funcs return b }This allows users to configure custom validation functions via the builder API.
38-43: Add security warnings for skip flags.Skipping issuer, audience, or expiry checks significantly weakens token security and should only be used in development/testing. Consider logging warnings in
Validate()or during initialization when these flags are enabled:// In Validate() or SetupOAuth(): if c.SkipIssuerCheck { logger.Warn("SECURITY: Issuer check disabled - not recommended for production") } if c.SkipAudienceCheck { logger.Warn("SECURITY: Audience check disabled - not recommended for production") } if c.SkipExpiryCheck { logger.Warn("SECURITY: Expiry check disabled - tokens may be accepted after expiration") }provider/provider.go (1)
66-82:HMACValidatordoesn't respect skip flags.The new skip flags (
SkipIssuerCheck,SkipAudienceCheck,SkipExpiryCheck) andTokenValidatorFuncsare only implemented forOIDCValidator.HMACValidatorstill performs audience validation unconditionally (line 117) and expiry validation (line 112 viavalidateTokenClaims).For API consistency, consider implementing these flags for
HMACValidatoras well, or document that these flags only apply to OIDC providers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.go(7 hunks)handlers.go(4 hunks)metadata.go(1 hunks)provider/provider.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config.go (2)
provider/provider.go (1)
Logger(24-29)logger.go (1)
Logger(22-27)
🔇 Additional comments (2)
handlers.go (1)
181-195: LGTM!The scopes configuration with sensible defaults is well implemented. The fallback to
["openid", "profile", "email"]when no scopes are provided ensures backward compatibility.provider/provider.go (1)
269-275: LGTM!The custom token validation function execution is correctly implemented. Ranging over a nil slice is safe in Go, so no nil check is needed.
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
from
4d5a1f7 to
8beec21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config.go (1)
321-321: EmptyOIDC_SCOPESproduces[""]instead of empty slice.When
OIDC_SCOPESis unset or empty,strings.Split("", " ")returns[""](a slice with one empty string), not an empty slice. This bypasses the default scopes fallback inNewOAuth2ConfigFromConfig(which checkslen(scopes) == 0).Use
strings.Fieldswhich handles empty strings correctly:- scopes := strings.Split(getEnv("OIDC_SCOPES", ""), " ") + scopesEnv := getEnv("OIDC_SCOPES", "") + var scopes []string + if scopesEnv != "" { + scopes = strings.Fields(scopesEnv) + }Alternatively, using
strings.Fieldsdirectly works because it returnsnilfor empty/whitespace-only input:- scopes := strings.Split(getEnv("OIDC_SCOPES", ""), " ") + scopes := strings.Fields(getEnv("OIDC_SCOPES", ""))
🧹 Nitpick comments (1)
metadata.go (1)
271-322: Inconsistent scopes handling between discovery endpoints.
HandleOIDCDiscovery(line 245) now uses dynamich.config.Scopes, whileGetAuthorizationServerMetadata(lines 285 and 317) still uses hardcoded[]string{"openid", "profile", "email"}.This inconsistency could confuse clients querying different metadata endpoints. Consider using
h.config.Scopesconsistently across both methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.go(7 hunks)handlers.go(4 hunks)metadata.go(1 hunks)provider/provider.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/provider.go
🧰 Additional context used
🧬 Code graph analysis (1)
config.go (2)
provider/provider.go (1)
Logger(24-29)logger.go (1)
Logger(22-27)
🔇 Additional comments (8)
config.go (5)
3-10: LGTM on imports.The new imports (
strconv,strings, andjwt/v5) are correctly added to support the new boolean parsing, scope handling, and token validation functions.
24-44: LGTM on new configuration fields.The new
Scopes,Skip*Check, andTokenValidationFuncsfields are well-structured and provide the flexibility needed for customizable OIDC validation.
131-141: LGTM on provider config mapping.The field mappings are now correct. The previously reported copy-paste error (where
SkipExpiryCheckwas incorrectly mapped tocfg.SkipAudienceCheck) has been fixed.
234-262: LGTM on builder methods.The new fluent builder methods follow the established pattern and the documentation comments have been corrected.
339-351: LGTM onparseBoolEnvhelper.The implementation correctly uses
strconv.ParseBoolto handle standard boolean values (true,false,1,0,t,f, etc.) and provides sensible defaults for empty or invalid values.handlers.go (3)
49-49: LGTM on newScopesfield.The new
Scopesfield inOAuth2Configproperly supports dynamic scope configuration.
96-101: LGTM on OAuth2 config initialization.The scopes are correctly passed to the underlying
oauth2.Config.
181-195: LGTM on scopes initialization with default fallback.The fallback logic is correct: if no scopes are configured, default to
["openid", "profile", "email"]. This will work correctly once thestrings.Splitissue inconfig.goline 321 is fixed (empty env should yield an empty slice, not[""]).
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
2 times, most recently
from
8a7df45 to
4be258d
Compare
WhammyLeaf
changed the title
|
@tuannvm Could be nice if you could have a look at this 😄 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/provider.go (1)
265-267: Explicit audience validation doesn't respect SkipAudienceCheck flag.When
SkipAudienceCheckis true (line 211 setsSkipClientIDCheck: cfg.SkipAudienceCheck), the OIDC verifier skips audience validation. However, the explicitvalidateAudiencecall on line 265 always runs, regardless of the flag. This creates inconsistent behavior where the audience check is only partially skipped.Apply this diff to conditionally skip the explicit audience check:
if err := idToken.Claims(&rawClaims); err != nil { return nil, fmt.Errorf("failed to extract raw claims: %w", err) } - // Validate audience claim for security (explicit check) - if err := v.validateAudience(rawClaims); err != nil { - return nil, fmt.Errorf("audience validation failed: %w", err) + // Validate audience claim for security (explicit check) if not skipped + if v.audience != "" { + if err := v.validateAudience(rawClaims); err != nil { + return nil, fmt.Errorf("audience validation failed: %w", err) + } }Note: This assumes that when
SkipAudienceCheckis true, the audience field should be set to empty string during initialization. If that's not the case, you'll need to store the skip flag in the validator struct or pass it through another mechanism.
🧹 Nitpick comments (2)
provider/provider.go (1)
269-276: Consider adding validation function index to error message.The error message for failed validation functions could be more informative by including which function failed. This would help with debugging when multiple validation functions are registered.
// Run extra validation functions - for _, fn := range v.TokenValidatorFuncs { + for i, fn := range v.TokenValidatorFuncs { err := fn(rawClaims) if err != nil { - return nil, fmt.Errorf("validation function failed with error: %w", err) + return nil, fmt.Errorf("validation function %d failed: %w", i, err) } }config.go (1)
132-141: Consider renaming TokenValidationFuncs for consistency.There's a naming inconsistency between the two Config structs:
oauth.Config.TokenValidationFuncs(line 43 in this file)provider.Config.TokenValidatorFuncs(line 41 in provider/provider.go)While the mapping on line 140 correctly uses
TokenValidatorFuncs, having different names for the same concept can cause confusion. Consider renaming one to match the other for consistency across the codebase.Suggested approach: Rename
TokenValidationFuncstoTokenValidatorFuncsin this file to match the provider package:- TokenValidationFuncs []func(claims jwt.MapClaims) error + TokenValidatorFuncs []func(claims jwt.MapClaims) errorThen update line 140:
- TokenValidatorFuncs: cfg.TokenValidationFuncs, + TokenValidatorFuncs: cfg.TokenValidatorFuncs,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.go(7 hunks)handlers.go(4 hunks)metadata.go(2 hunks)provider/provider.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- handlers.go
- metadata.go
🧰 Additional context used
🧬 Code graph analysis (1)
config.go (2)
provider/provider.go (1)
Logger(24-29)logger.go (1)
Logger(22-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-review
🔇 Additional comments (7)
provider/provider.go (2)
59-63: LGTM!The OIDCValidator struct properly reflects the new configuration fields. The addition of
TokenValidatorFuncsand explicitloggerfield storage aligns well with the validation flow enhancements.
208-214: Based on the web search results, I can now provide the verification findings:The skip flags are properly documented and work as documented in go-oidc v3.
The search confirmed:
- SkipClientIDCheck — exists and skips audience validation; must be true if ClientID is empty
- SkipExpiryCheck — exists and skips token expiry check (documented for special/testing cases)
- SkipIssuerCheck — exists and skips issuer verification (with warnings to independently verify)
However, there is one potential edge case worth noting: The go-oidc documentation states that
SkipClientIDCheckmust be set to true ifClientIDis empty, otherwise it should be false. Since the code mapscfg.Audienceto theClientIDfield, verify that your configuration validates this constraint—specifically, ifcfg.Audienceis empty, ensurecfg.SkipAudienceCheckis true, or the verifier will fail. Check that this validation exists in your config validation logic.
The skip flags work as expected and are secure when used appropriately. The mapping from
cfg.SkipAudienceChecktoSkipClientIDCheckis consistent with go-oidc's design. Ensure your config validation enforces the constraint that when audience is empty, skip-audience-check must be true.config.go (5)
3-10: LGTM!The new imports (
strconv,strings,jwt/v5) are all properly utilized:strconvfor boolean parsing,stringsfor splitting scopes, andjwtfor theMapClaimstype in validation functions.
234-262: LGTM!The new builder methods (
WithScopes,WithSkipIssuerCheck,WithSkipAudienceCheck,WithSkipExpiryCheck) correctly follow the fluent API pattern and properly set the corresponding Config fields.
322-326: LGTM!The scopes parsing correctly handles the empty environment variable case by initializing an empty slice and only splitting when the value is non-empty. This avoids the
[""]issue that would bypass default scope fallback logic.
336-339: LGTM!The
FromEnvmethod correctly integrates the new configuration options. The use ofparseBoolEnvwithfalseas the default is appropriate for security-sensitive skip flags, ensuring checks are enabled by default.
345-356: LGTM!The
parseBoolEnvhelper properly handles boolean environment variables usingstrconv.ParseBool, which supports standard formats (true/false, 1/0, t/f, etc.). The fallback to default values for empty or invalid inputs is appropriate.
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
from
8aee3d8 to
1d569b3
Compare
Signed-off-by: Christian Troelsen <christian.troelsen@tryg.dk> fix suggestions by coderabbit Signed-off-by: Christian Troelsen <christian.troelsen@tryg.dk> a few more changes
WhammyLeaf
force-pushed
the
add-more-custom-config-vars
branch
from
1d569b3 to
8351f51
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
TokenValidators never run for HMAC tokens, and go.mod still targets older toolchain/dependencies (Go 1.24.9, go-oidc v3.16.0, mark3labs/mcp-go v0.41.1, MCP go-sdk v1.0.0, golang.org/x/oauth2 v0.32.0) even though proxy.golang.org reports 1.25.5 / 3.17.0 / 0.43.2 / 1.1.0 / 0.34.0; please plan upgrades.
|
@tuannvm Is this something you would be interested in merging into main? |
1 participant
This PR adds support a couple of new config variables, which are useful to have available in an enterprise setting with a more complex azure setup. They are:
Scopes, which defaults to the currently hardcoded values when not setTokenValidatonFuncs, which is a list custom validation functions to apply to the raw acccesSkipIssuerCheck,SkipExpiryCheck,SkipAudienceCheck, which are booleans that, when true, bypass the corresponding checks on the access tokenI still have not added any documentation, but I can do that once I get the go-ahead.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.