✨ Add authlib/ — shared token exchange building blocks (Phase 1)

[huang195]

Summary

Phase 1 of the modular token exchange library (#279). Extracts reusable auth building blocks from go-processor into a standalone Go module at authbridge/authlib/.

Six packages, 34 unit tests, zero Envoy/gRPC dependencies:

Package	Purpose	Tests
validation/	JWKS-backed JWT verifier (lestrrat-go/jwx)	3
exchange/	RFC 8693 token exchange + client credentials	5
cache/	SHA-256 keyed token cache with TTL eviction	7
bypass/	Path pattern matcher for public endpoints	5
spiffe/	File-based JWT-SVID source	4
routing/	Host-to-audience router with glob matching	10

Key improvements over current go-processor:

• Audience is a required parameter to Verify() (prevents confused deputy attacks)
• Exchanged-token cache (go-processor has none — every exchange hits the IdP)
• Configurable HTTP client with 30s timeout (go-processor uses default — no timeout)
• Typed ExchangeError with OAuth error/error_description fields
• Instance-based, not global mutable state
• Pure Go errors, no gRPC coupling

Related issue(s)

Ref #279

Test plan

• go test ./... — 34 tests passing
• go vet ./... — no issues
• CI passes

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

[pdettori]

Review Summary

Well-structured Phase 1 library with clean interfaces, minimal dependencies (jwx, gobwas/glob, yaml.v3), and strong test coverage (34 tests across 6 packages). The security-conscious design — SHA-256 cache keys, path normalization, response size limits, context propagation — is solid.

Highlights:

• Cache key collision resistance test and concurrent access test — good security-conscious testing
• Bypass matcher path normalization tests (double slash, dot segments, parent traversal)
• Exchange client edge case coverage: context cancellation, malformed JSON, connection failure

One must-fix on the audience validation contract, plus a couple of suggestions for Phase 2 readiness.

Areas reviewed: Go code (all 19 files), go.mod dependencies, security, tests
Commits: 3, all signed-off ✓
CI: All checks passing ✓

[pdettori]

must-fix: The PR description states "Audience is a required parameter to Verify() (prevents confused deputy attacks)" and the Verifier interface doc says "audience is a required parameter". But passing an empty string silently skips the audience check:

if audience != "" {
    parseOpts = append(parseOpts, jwt.WithAudience(audience))
}

This creates a silent failure mode: a caller who accidentally passes "" gets full JWT validation minus audience — exactly the confused deputy scenario this library is designed to prevent.

Suggestion: Return an error when audience is empty to match the documented invariant:

if audience == "" {
    return nil, fmt.Errorf("audience is required (prevents confused deputy attacks)")
}
parseOpts = append(parseOpts, jwt.WithAudience(audience))

[huang195]

Fixed in b2ca966. Empty audience now returns an error: "audience is required (prevents confused deputy attacks)". Added TestJWKSVerifier_EmptyAudience_Rejected test to verify.

[pdettori]

suggestion: c.auth.Apply(ctx, form, nil) passes nil for the http.Header parameter. The ClientAuth interface accepts headers (for use cases like Basic auth or bearer token injection), but a nil value means any future implementation that calls headers.Set(...) would panic with a nil pointer dereference.

Current implementations ignore the header parameter so this works today. Consider either:

1. Passing req.Header after creating the request (requires restructuring doTokenRequest), or
2. Passing make(http.Header) and merging into the request afterward, or
3. Removing the headers parameter from the ClientAuth interface if it's not needed for Phase 1

Same pattern at line 96 in ClientCredentials().

[huang195]

Fixed in b2ca966. Removed http.Header param from ClientAuth.Apply() — simplified to Apply(ctx, form). Eliminates the nil pointer risk. Header support can be added via a separate ApplyHeaders method if needed later.

[pdettori]

suggestion: The core security path — JWKSVerifier.Verify() — has zero test coverage. The Claims helper tests are good, but the actual JWT validation (signature verification, expiry, issuer check, audience enforcement) is untested.

I understand integration tests are deferred to Phase 2, but consider adding a unit test with an in-memory JWKS server (using httptest.NewServer + jwk.NewSet()) to cover at least:

• Valid token → success
• Expired token → error
• Wrong issuer → error
• Wrong audience → error
• Invalid signature → error

This would catch regressions in the parse options wiring (like the audience bypass above) without needing Keycloak.

[huang195]

Fixed in b2ca966. Added 6 JWKSVerifier unit tests with in-memory httptest JWKS server: valid token, expired token, wrong issuer, wrong audience, empty audience (rejected), invalid signature. These cover the core security path without needing Keycloak.

[pdettori]

nit: The nuclear eviction (clear all entries) when the cache is full and no expired entries exist could cause temporary cache-miss storms under sustained high-cardinality traffic. Not a problem for Phase 1 volumes, but worth a // TODO: consider LRU or random-sample eviction for Phase 2 marker so it doesn't get forgotten when this is wired into the hot path.

[huang195]

Added TODO comment in b2ca966. The nuclear eviction is acceptable for Phase 1 volumes. Will consider LRU or random-sample eviction when this is wired into the hot path.

[pdettori]

Follow-up Review

Fixes not pushed: All 4 comments from the prior review were addressed by the author in replies citing commit b2ca966, but that commit is not in the PR branch. The branch still has only the original 3 commits (4603cdb, ead07d9, ed00dab). The must-fix (empty audience bypass in validation/jwks.go:70) and the 3 suggestions are still present in the code.

Please push the fix commit so the changes can be verified and the PR approved.

One additional minor finding below.

Areas reviewed: Go code (all 19 files), go.mod dependencies, security, tests
Commits: 3, all signed-off ✓
CI: All checks passing ✓

[pdettori]

nit: TOCTOU race — os.Stat then os.ReadFile could give inconsistent results if the file is created/deleted between the two calls. The idiomatic Go pattern avoids this:

data, err := os.ReadFile(path)
if errors.Is(err, os.ErrNotExist) {
    return nil, nil
}
if err != nil {
    return nil, fmt.Errorf("reading routes config: %w", err)
}

Simpler and race-free. Not blocking — just a cleanup for whenever.

[huang195]

Fixed in a5623ea (cherry-picked onto Phase 1 branch). Uses os.ReadFile directly with errors.Is(err, os.ErrNotExist) check.

[huang195]

Fixes now pushed to this branch (cherry-picked from Phase 2):

• a5623ea: Enforce non-empty audience in Verify(), remove http.Header from ClientAuth.Apply(), add 6 JWKSVerifier unit tests, validate SubjectToken, fix TOCTOU in LoadRoutes, add cache eviction TODO
• 080f19f: Replace t.Fatal with http.Error in httptest handlers (exchange/client_test.go)

All 40 tests passing across 6 packages.

[huang195]

All Phase 1 code is now on main via the Phase 2 (#311) and Phase 3 (#312) merges, which included the Phase 1 commits in their history plus all review fixes. This PR can be closed as superseded.

The review findings (enforce audience, remove nil Header, JWKS tests, TOCTOU fix, SubjectToken validation, t.Fatal in handlers, cache eviction TODO) are all on main.

[huang195]

@pdettori thanks for the review. since phase 2 and phase 3 PRs are already merged, and phase 1 PR is already included in those, I'm just going to close this PR as we have everything we need.