Skip to content

Add GitHub Actions OIDC token verification - #3

Merged
golift-bot merged 4 commits into
pr02-signerfrom
pr03-oidc
Aug 24, 2026
Merged

golift-bot merged 4 commits into
pr02-signerfrom
pr03-oidc

Conversation

@davidnewhall

Copy link
Copy Markdown
Contributor

PR 3 of 9, stacked on #2.

  • oidc.Verifier: RS256 signature against the issuer's JWKS (discovery + 10-minute key cache), iss/aud/exp enforcement, and a fail-closed allowed_repositories list — empty list rejects every token.
  • Repository match is case-insensitive; weak/symmetric algorithms rejected via WithValidMethods.
  • Table tests against a fake httptest issuer: valid, wrong aud/iss, expired, missing exp, missing/unknown kid, HS256 downgrade, missing repository claim, empty allowlist.

Next: #4 (signing backends).

Made with Cursor

Copilot AI left a comment

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.

Pull request overview

Adds GitHub Actions OIDC token verification with RS256/JWKS validation, claim enforcement, and repository allowlisting.

Changes:

  • Added issuer discovery and JWKS caching.
  • Enforced issuer, audience, expiration, algorithm, and repository checks.
  • Added comprehensive verifier tests and JWT dependencies.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Review summary
oidc/oidc.go Contains one critical audience-validation issue and three moderate issues involving unknown-kid refreshes, stale-key behavior during issuer outages, and missing timeouts for custom HTTP clients.
oidc/oidc_test.go Adds fake-issuer tests covering valid and invalid token scenarios.
go.sum Adds dependency checksums.
go.mod Adds the JWT dependency.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread oidc/oidc.go Outdated
Comment on lines +147 to +151
if key, ok := v.keys[keyID]; ok && time.Since(v.fetched) < keyCacheTTL {
return key, nil
}

err := v.fetchKeys(ctx)
Comment thread oidc/oidc.go Outdated
Comment on lines +153 to +155
// Ride out a fetch blip when the key is already cached.
if key, ok := v.keys[keyID]; ok {
return key, nil
Comment thread oidc/oidc.go Outdated
Comment on lines +227 to +228
func (v *Verifier) getJSON(ctx context.Context, url string, output any) error {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody)
Comment thread oidc/oidc.go
},
jwt.WithValidMethods([]string{jwt.SigningMethodRS256.Alg()}),
jwt.WithIssuer(v.config.Issuer),
jwt.WithAudience(v.config.Audience),

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

oidc/oidc.go:168

  • This mutex remains held while discovery and JWKS HTTP requests run. Because kid is attacker-controlled, one unknown-key token can initiate a refresh and block every concurrent verification—including tokens whose keys are already cached—for up to fetchTimeout; this can recur every cooldown during an issuer slowdown/outage. Keep cache reads available during I/O, and serialize refreshes separately (for example, fetch into a local map via singleflight/refresh mutex, then briefly lock to swap the cache).
func (v *Verifier) key(ctx context.Context, keyID string) (*rsa.PublicKey, error) {
	v.mu.Lock()
	defer v.mu.Unlock()

oidc/oidc.go:187

  • The cache can trust a previously fetched signing key for up to 70 minutes (keyCacheTTL + maxKeyStale) when refresh fails, despite the PR's stated 10-minute key-cache guarantee. That permits tokens signed by a key removed or revoked by the issuer to remain valid for an extra hour if issuer connectivity is unavailable. Either stop accepting the key once the 10-minute TTL expires, or explicitly make this stale-key availability policy part of the security contract/configuration.
	// Refresh failed or was rate-limited. Ride out a bounded issuer outage
	// with the cached key, then fail closed.
	if cached && time.Since(v.fetched) < keyCacheTTL+maxKeyStale {
		return key, nil

davidnewhall and others added 3 commits August 21, 2026 00:18
Co-authored-by: Cursor <cursoragent@cursor.com>
…-limited refresh

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Empty allowlist and audience still fail closed.

Co-authored-by: Cursor <cursoragent@cursor.com>

@oceanplexian oceanplexian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OIDC verification is solid and fail-closed: empty allowlist/audience reject everything, RS256-only (HS256 bypass tested), kid rate-limiting so attacker-controlled key IDs cannot generate a JWKS fetch per token, jwks_uri host pinned to the issuer origin, response size capped, 10-minute key TTL. The test suite covers the important attack paths. Approving.

@golift-bot
golift-bot merged commit e2e199c into main Aug 24, 2026
7 checks passed
@golift-bot
golift-bot deleted the pr03-oidc branch August 24, 2026 03:53
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.

4 participants