Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/auth/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@
}

// NewTokenValidator creates a new token validator.
func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts ...TokenValidatorOption) (*TokenValidator, error) {

Check failure on line 591 in pkg/auth/token.go

View workflow job for this annotation

GitHub Actions / Linting / Lint Go Code

cyclomatic complexity 17 of func `NewTokenValidator` is high (> 15) (gocyclo)
// Apply functional options
o := &tokenValidatorOptions{envReader: &env.OSReader{}}
for _, opt := range opts {
Expand Down Expand Up @@ -640,6 +640,19 @@
return nil, ErrMissingIssuerAndJWKSURL
}

// Google's tokeninfo endpoint returns no iss claim - parseGoogleResponse
// synthesises iss locally, so a configured-issuer check against it is
// self-satisfying and proves nothing about which OAuth client the token
// was minted for. The only real binding to this deployment is the
// audience check, which is skipped when audience is empty. Without it,
// ANY valid Google access token (e.g. one minted for an attacker's own
// unrelated OAuth client) passes validation. Refuse the combination at
// startup instead of silently accepting cross-client tokens at runtime.
if config.IntrospectionURL == GoogleTokeninfoURL && config.Audience == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this only catches a literally empty audience. If someone sets it to " " by accident (bad env templating etc.) it'd sail past this check, then fail every single token at runtime instead of failing loudly at startup. Might be worth trimming, same as the issuer check does elsewhere:

Suggested change
if config.IntrospectionURL == GoogleTokeninfoURL && config.Audience == "" {
if config.IntrospectionURL == GoogleTokeninfoURL && strings.TrimSpace(config.Audience) == "" {

Optional, not blocking.

return nil, fmt.Errorf("audience is required when using Google's tokeninfo endpoint for introspection: " +
"without it any valid Google access token is accepted regardless of the OAuth client it was minted for")
}

// Create HTTP client with CA bundle and auth token support for JWKS
httpClient, err := networking.NewHttpClientBuilder().
WithCABundle(config.CACertPath).
Expand Down
27 changes: 27 additions & 0 deletions pkg/auth/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3075,3 +3075,30 @@ func TestEnsureJWKSRegistered_NonFatalRegistrationErrors(t *testing.T) {
require.True(t, validator.jwksRegistered)
})
}

func TestNewTokenValidator_GoogleTokeninfoRequiresAudience(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional style nit: this reads fine as-is, but since it's really one test with two input variants (empty vs set audience), our testing rules lean toward table-driven for this shape rather than two sequential imperative blocks. Up to you whether it's worth the churn for two cases.

t.Setenv("TOOLHIVE_SKIP_OIDC_DISCOVERY", "true")

// Google tokeninfo + no audience must be rejected at startup: tokeninfo
// returns no iss claim (the provider synthesises it locally), so an
// issuer check is self-satisfying and the audience check is the only
// binding to this deployment. Without it, any valid Google access token
// passes regardless of which OAuth client minted it.
_, err := NewTokenValidator(context.Background(), TokenValidatorConfig{
Issuer: "https://accounts.google.com",
JWKSURL: "https://www.googleapis.com/oauth2/v3/certs",
IntrospectionURL: GoogleTokeninfoURL,
Audience: "",
})
require.Error(t, err)
require.Contains(t, err.Error(), "audience is required")

// With an audience configured, the same setup must be accepted.
_, err = NewTokenValidator(context.Background(), TokenValidatorConfig{
Issuer: "https://accounts.google.com",
JWKSURL: "https://www.googleapis.com/oauth2/v3/certs",
IntrospectionURL: GoogleTokeninfoURL,
Audience: "my-client-id.apps.googleusercontent.com",
})
require.NoError(t, err)
}
Loading