Skip to content

refactor(proof/oidc): readability + maintainability sweep#68

Merged
jmgilman merged 4 commits into
masterfrom
session-039/proof-oidc-sweep
May 28, 2026
Merged

refactor(proof/oidc): readability + maintainability sweep#68
jmgilman merged 4 commits into
masterfrom
session-039/proof-oidc-sweep

Conversation

@jmgilman
Copy link
Copy Markdown
Contributor

Summary

Eighth slice of the multi-session readability sweep (after access/ #61, authz/ #62, exchange/ #63, http/ #64, management/ #65, onboarding/ #66, proof/apikey/ #67). Second of three PRs covering proof/. Same 10-criteria bar.

proof/oidc/ is the JWT trust gate for externally-issued OIDC tokens. The signature work this PR carries is annotating that gate.

Security inline comments on Verifier.VerifyToken

proof/oidc/verifier.go::VerifyToken is analogous to access/jwt/verifier.go (PR #61) but lacked the per-check security annotations. Each branch now carries a comment naming the attack class it defends against:

  • Empty-token rejection — signal amplification.
  • Unverified-issuer partial parse — lookup-then-verify rationale.
  • Trusted-issuer lookup — untrusted-issuer defense.
  • Provider-issuer consistency check — config-swap / storage-bug defense.
  • Provider revalidation — corrupt trust-store row defense.
  • jwt.Parse omnibus — forgery, expiration bypass, time confusion, key-use smuggling.
  • Explicit non-empty subject — blank-subject confusion (defense in depth over jwt.Parse's required-claim check).
  • Audience match — audience confusion.

Test split

verifier_test.go (616 LOC) split per-domain mirroring the production layout:

  • provider_test.goProvider.Validate tests.
  • source_test.goStaticProviderSource tests.
  • verifier_test.goVerifier.VerifyToken tests only.
  • helpers_test.go — fixtures, testIssuer, tokenRequest, failingProviderSource, fixedTime.

Other oidc changes

  • Godocs added to every private type, struct, and helper: options, defaultOptions, cachedKeySet, keySetRequest, the source.go clone family, claimPathKey, cloneClaimValue, validateHTTPSURL, signingAlgorithms, unverifiedIssuer, audienceAllowed, forwardClaims, mergedForwardedClaims, tokenClaim, setClaim, keySet, keySetCacheKey, fetchKeySet, constrainKeySet, verificationKey, keyAllowsVerification, unauthenticated.
  • Several godocs name the security invariant they preserve (HTTPS-only URLs, symmetric-algorithm refusal, maxJWKSBytes cap, fail-closed empty-key-set, JWK use/key_ops gating).
  • Compile-time var _ ProviderSource = (*StaticProviderSource)(nil) assertion added to source.go. Store-side ProviderSource/ProviderTrustStore assertions already live in the store packages' _test.go files.

Explicitly NOT changed

  • ProviderSource stays as an interface. The plan considered demoting it to a function type but it has three implementations (StaticProviderSource, store/memory.Store, store/postgres.Store) and is embedded by ProviderTrustStore. The premature-port demotion rule (one method + one implementation) does not apply.
  • No file reorganization. Current split (doc.go/options.go/source.go/types.go/verifier.go) is coherent.
  • No mockery entries. Package is a leaf consumer; existing hand-rolled failingProviderSource is appropriate.

Commits

  1. refactor(proof/oidc): godocs on private types and helpers
  2. refactor(proof/oidc): security inline comments on Verifier.VerifyToken
  3. test(proof/oidc): split verifier_test.go per domain
  4. refactor(proof/oidc): compile-time port assertion for StaticProviderSource

Test plan

  • moon run root:check --summary minimal — format, lint, build, unit, Testcontainers integration all green.
  • All existing tests pass; no test logic changed in the split.

🤖 Generated with Claude Code

jmgilman and others added 4 commits May 27, 2026 17:55
Add concise godocs to every private type, struct, and helper the
package exposes internally: options/defaultOptions, cachedKeySet,
keySetRequest, the source.go clone family, claimPathKey, cloneClaimValue,
validateHTTPSURL, signingAlgorithms, unverifiedIssuer, audienceAllowed,
forwardClaims, mergedForwardedClaims, tokenClaim, setClaim, keySet,
keySetCacheKey, fetchKeySet, constrainKeySet, verificationKey,
keyAllowsVerification, and unauthenticated.

Several godocs name the security invariant they preserve — HTTPS-only
issuer/JWKS URLs, symmetric-algorithm refusal, maxJWKSBytes cap,
forwarded-claim allow-list, fail-closed empty-key-set check, JWK
use/key_ops gating — to match the inline-comment style about to land
on Verifier.VerifyToken.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Annotate each gate in the OIDC JWT trust path so the security invariant
it preserves is explicit. Mirrors the access/jwt/verifier.go style from
PR #61.

The annotations cover: empty-token rejection (signal amplification),
unverified-issuer partial parse (lookup-then-verify rationale), trusted
issuer lookup (untrusted-issuer defense), provider-issuer consistency
(config-swap defense), revalidation of the resolved provider (corrupt
trust-store row), the jwt.Parse omnibus (forgery, expiration bypass,
time confusion, key-use smuggling), explicit non-empty subject check
(blank-subject confusion), and audience match (audience confusion).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lift the 616-LOC verifier_test.go into four files mirroring the
production layout: provider_test.go (Provider.Validate), source_test.go
(StaticProviderSource), verifier_test.go (Verifier.VerifyToken only),
helpers_test.go (fixtures + testIssuer + tokenRequest +
failingProviderSource + fixedTime). No test logic changes; each
function is lifted verbatim.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ource

Make StaticProviderSource's implementation of ProviderSource a
compile-time check so an accidental method signature drift breaks the
build rather than a downstream caller. Store-side ProviderSource and
ProviderTrustStore assertions already live in store/memory/oidc_test.go
and store/postgres/oidc_test.go.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jmgilman jmgilman merged commit 58bfdd4 into master May 28, 2026
2 checks passed
@jmgilman jmgilman deleted the session-039/proof-oidc-sweep branch May 28, 2026 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant