security: fix OIDC audience validation bypass#1024
Conversation
Remove unconditional SkipClientIDCheck: true from OIDC verifier configuration. When AllowedAudiences is configured, set ClientID to the first audience so the go-oidc library enforces the standard aud claim validation. Fall back to SkipClientIDCheck only when no audiences are configured, for backward compatibility. Add firstAudience() helper to extract the primary audience from comma-separated audience strings. Previously, tokens issued for any client by the same OIDC issuer were accepted, allowing cross-service token reuse. Signed-off-by: Matteo Merli <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR hardens OIDC token verification to prevent cross-service token reuse by ensuring the aud claim is validated by go-oidc when audiences are configured.
Changes:
- Adds a
firstAudience()helper to derive a primary audience from a comma-separatedAllowedAudiencesstring. - Updates OIDC verifier configuration to set
oidc.Config.ClientID(enabling library-levelaudenforcement) when audiences are configured, and only usesSkipClientIDCheckas a fallback. - Threads
AllowedAudiencesthrough verifier creation for both remote JWKS and static key verification paths.
Comments suppressed due to low confidence (1)
oxiad/common/rpc/auth/oidc.go:264
- parseAllowedAudiences() currently inserts empty-string keys when allowedAudiences contains trailing commas or empty segments (eg "aud1," or "aud1,,aud2"). This can make the allow-list contain "" unexpectedly and also makes it harder to reason about audience validation. Consider skipping empty trimmed entries (and ideally treating a fully-empty result as a validation error).
// parseAllowedAudiences converts a comma-separated string of audiences into a map.
func parseAllowedAudiences(audiences string) map[string]string {
allowedAudienceMap := map[string]string{}
allowedAudienceArr := strings.Split(audiences, ",")
for i := range allowedAudienceArr {
allowedAudience := strings.TrimSpace(allowedAudienceArr[i])
allowedAudienceMap[allowedAudience] = AllowedAudienceDefaultValue
}
return allowedAudienceMap
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // createStaticKeyVerifier creates a verifier using static keys from a file. | ||
| func createStaticKeyVerifier(issuerURL, keyFile string) (*oidc.IDTokenVerifier, error) { | ||
| // When allowedAudiences is non-empty, the first audience is used as the expected | ||
| // client ID for the library-level audience check. If empty, the library check is | ||
| // skipped (the custom multi-audience check in Authenticate still applies). | ||
| func createStaticKeyVerifier(issuerURL, keyFile, allowedAudiences string) (*oidc.IDTokenVerifier, error) { | ||
| publicKeys, err := loadPublicKeysFromFile(keyFile) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to load static key file for issuer %s", issuerURL) | ||
| } | ||
|
|
||
| staticKeySet := &oidc.StaticKeySet{PublicKeys: publicKeys} | ||
| config := &oidc.Config{ | ||
| SkipClientIDCheck: true, | ||
| Now: time.Now, | ||
| Now: time.Now, | ||
| } | ||
| if clientID := firstAudience(allowedAudiences); clientID != "" { | ||
| config.ClientID = clientID | ||
| } else { | ||
| config.SkipClientIDCheck = true | ||
| } |
There was a problem hiding this comment.
Setting oidc.Config.ClientID to the first entry in allowedAudiences means the verifier will reject tokens whose aud matches any other configured audience (eg allowedAudiences="aud1,aud2" will fail for tokens with aud=["aud2"]). This contradicts the existing "any of the client audience in the allowed is passed" behavior in Authenticate and the CLI docs that describe allowedAudiences as a comma-separated allow-list. Consider either (a) building/trying verifiers for each allowed audience (accept if any verify), or (b) introducing an explicit primary ClientID/ExpectedAudience field and documenting that additional audiences are no longer supported.
| // firstAudience returns the first audience from a comma-separated string, trimmed. | ||
| // Returns empty string if no audiences are configured. | ||
| func firstAudience(audiences string) string { | ||
| if audiences == "" { | ||
| return "" | ||
| } | ||
| parts := strings.SplitN(audiences, ",", 2) | ||
| return strings.TrimSpace(parts[0]) | ||
| } |
There was a problem hiding this comment.
firstAudience() treats inputs like ",aud1" or " , aud1" as having no configured audiences (returns empty), which causes SkipClientIDCheck=true and disables library-level aud validation even though allowedAudiences is non-empty. It would be safer to trim and return the first non-empty audience token (skipping empty/whitespace segments) so misformatted configs don’t silently degrade validation.
| // createRemoteVerifier creates a verifier using remote JWKS. | ||
| func createRemoteVerifier(ctx context.Context, issuerURL string) (*oidc.Provider, *oidc.IDTokenVerifier, error) { | ||
| // When allowedAudiences is non-empty, the first audience is used as the expected | ||
| // client ID for the library-level audience check. If empty, the library check is | ||
| // skipped (the custom multi-audience check in Authenticate still applies). | ||
| func createRemoteVerifier(ctx context.Context, issuerURL, allowedAudiences string) (*oidc.Provider, *oidc.IDTokenVerifier, error) { | ||
| provider, err := oidc.NewProvider(ctx, issuerURL) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrapf(err, "failed to create OIDC provider for issuer %s", issuerURL) | ||
| } | ||
|
|
||
| config := &oidc.Config{ | ||
| SkipClientIDCheck: true, | ||
| Now: time.Now, | ||
| Now: time.Now, | ||
| } | ||
| if clientID := firstAudience(allowedAudiences); clientID != "" { | ||
| config.ClientID = clientID | ||
| } else { | ||
| config.SkipClientIDCheck = true | ||
| } | ||
| verifier := provider.Verifier(config) |
There was a problem hiding this comment.
With ClientID set, audience mismatches will now be rejected by verifier.Verify() before the custom ErrForbiddenAudience check runs, changing the error surfaced to callers (and likely breaking existing tests expecting ErrForbiddenAudience). Consider translating go-oidc audience-validation errors into ErrForbiddenAudience (or otherwise updating expectations) to keep a stable error contract.
Revert the ClientID-based approach which broke multi-audience support. The go-oidc library only validates against a single ClientID, but Oxia supports multiple allowed audiences via comma-separated config. Instead, keep SkipClientIDCheck at the library level and rely on the existing custom multi-audience check in Authenticate() which correctly validates that at least one token audience matches the configured AllowedAudiences. Add explicit documentation explaining this design. Remove the firstAudience() helper which had edge-case issues with malformed inputs like ",aud1". Fixes all three Copilot review comments: - Multi-audience breakage when ClientID was set to first audience - firstAudience() edge case with leading commas - Error contract change (ErrForbiddenAudience vs go-oidc error) Signed-off-by: Matteo Merli <[email protected]>
Summary
SkipClientIDCheck: truefrom OIDC verifier configurationAllowedAudiencesis configured, setClientIDto the first audience sogo-oidcenforces the standardaudclaim validationSkipClientIDCheckonly when no audiences are configured, for backward compatibilityfirstAudience()helper to extract the primary audience from comma-separated stringsPreviously, tokens issued for any client by the same OIDC issuer were accepted, allowing cross-service token reuse.
GHSA: GHSA-fhvp-9hcj-6m33
Test plan
go test -v ./oxiad/common/rpc/auth/...)AllowedAudiencesis empty