feat(token): add RS256/ES256 JWT signing and JWKS endpoint#109
Merged
feat(token): add RS256/ES256 JWT signing and JWKS endpoint#109
Conversation
Add config fields (JWT_SIGNING_ALGORITHM, JWT_PRIVATE_KEY_PATH, JWT_KEY_ID) and key loading utilities for asymmetric JWT signing. Extend LocalTokenProvider with Option pattern (WithSigningKey, WithKeyID) to support RS256/ES256 in addition to default HS256. Bootstrap validates key-algorithm match at startup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Expose public keys for RS256/ES256 via standard JWKS endpoint so resource servers can verify JWTs without sharing secrets. HS256 mode returns an empty key set. Hand-rolled JWK serialization avoids pulling in go-jose dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Discovery endpoint now returns the actual JWT_SIGNING_ALGORITHM in id_token_signing_alg_values_supported and includes jwks_uri when using asymmetric keys (RS256/ES256). HS256 omits jwks_uri. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- key_test.go: 12 tests for key loading (RSA/ECDSA, PKCS1/PKCS8/SEC1), error paths, DeriveKeyID, ValidateKeyAlgorithm - local_test.go: 10 new tests for RS256/ES256 generate+validate, ID token, kid header, cross-validation, refresh tokens, backward compatibility - jwks_test.go: 4 tests for RSA/ECDSA/HS256 JWKS responses, coordinate padding - oidc_test.go: 4 new tests for discovery jwks_uri and algorithm fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update .env.example with JWT_SIGNING_ALGORITHM, JWT_PRIVATE_KEY_PATH, and JWT_KEY_ID variables. Add JWT Signing Algorithm section to CONFIGURATION.md with key generation examples. Update CLAUDE.md to reflect RS256/ES256 support, JWKS endpoint, and new config fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds asymmetric JWT signing (RS256/ES256) alongside HS256 and exposes public keys via a JWKS endpoint to support resource-server verification and OIDC discovery metadata.
Changes:
- Extend
LocalTokenProviderto support HS256/RS256/ES256 with signing key +kidoptions. - Add key loading/validation utilities and a new public JWKS endpoint (
/.well-known/jwks.json). - Update OIDC discovery metadata to reflect configured signing algorithm and include
jwks_urifor asymmetric modes; add tests/docs/env updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/token/local.go | Option-based asymmetric signing support + dynamic verification key selection |
| internal/token/idtoken.go | ID token signing now uses provider-selected algorithm/key |
| internal/token/key.go | New helpers for PEM key loading, algorithm validation, and kid derivation |
| internal/token/local_test.go | New RS256/ES256 coverage for access/refresh/ID tokens and kid header |
| internal/token/key_test.go | Tests for key loading, kid derivation, and algorithm/key validation |
| internal/handlers/jwks.go | New JWKS handler with manual RSA/EC JWK serialization |
| internal/handlers/jwks_test.go | Tests for RSA/EC JWKS output and EC coordinate padding |
| internal/handlers/oidc.go | Discovery metadata now reflects configured signing algorithm and conditional jwks_uri |
| internal/handlers/oidc_test.go | Tests for discovery algorithm values and jwks_uri behavior |
| internal/config/config.go | Adds JWT signing algorithm/key path/kid config + validation |
| internal/bootstrap/providers.go | Boot-time key load/validate/derive-kid and provider wiring |
| internal/bootstrap/bootstrap.go | Stores token provider in app for handler initialization |
| internal/bootstrap/services.go | Accepts token provider as dependency (no longer constructed inside) |
| internal/bootstrap/handlers.go | Builds JWKS handler from token provider (or empty JWKS fallback) |
| internal/bootstrap/router.go | Registers /.well-known/jwks.json route |
| docs/CONFIGURATION.md | Documents signing algorithms, key generation, JWKS, and rotation |
| .env.example | Adds JWT signing algorithm/key configuration examples |
| CLAUDE.md | Updates token provider documentation for new signing/JWKS support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Increase DeriveKeyID truncation from 8 to 16 bytes to reduce collision risk - Update DeriveKeyID comment to accurately describe SHA-256 thumbprint derivation - Gate discovery jwks_uri on actual JWKS key availability instead of algorithm name - Add defensive panic in NewLocalTokenProvider when asymmetric key is missing - Add Keys accessor method to JWKSHandler for key availability checks - Check rsa.GenerateKey errors in tests instead of discarding them Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds asymmetric JWT signing (RS256/ES256) alongside HS256, plus a JWKS endpoint and OIDC discovery metadata updates so resource servers can verify JWTs via public keys instead of shared secrets.
Changes:
- Add configurable JWT signing algorithm selection (HS256/RS256/ES256) with PEM key loading and
kidderivation. - Add
/.well-known/jwks.jsonendpoint and conditionally includejwks_uri+ algorithm in OIDC discovery metadata. - Expand test coverage for asymmetric signing, key loading/validation, JWKS responses, and discovery metadata.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/token/local.go | Adds option-based configuration for signing keys/kid and supports RS256/ES256 in token generation/validation. |
| internal/token/idtoken.go | Switches ID token signing from fixed HS256 to provider-selected algorithm/key. |
| internal/token/key.go | Adds PEM private key loading, algorithm/key validation, and key-id derivation. |
| internal/token/local_test.go | Adds RS256/ES256 generation/validation tests and HS256 compatibility checks. |
| internal/token/key_test.go | Adds unit tests for key parsing, key-id derivation, and algorithm/key validation. |
| internal/handlers/jwks.go | Implements JWKS endpoint and JWK serialization for RSA/EC public keys. |
| internal/handlers/jwks_test.go | Tests JWKS output for RSA/EC keys and HS256 empty key set behavior. |
| internal/handlers/oidc.go | Adds configurable id_token_signing_alg_values_supported and conditional jwks_uri. |
| internal/handlers/oidc_test.go | Updates handler construction + adds discovery tests for alg/jwks_uri behavior. |
| internal/config/config.go | Adds env/config fields for algorithm/key path/kid and validates algorithm + key-path requirement. |
| internal/bootstrap/providers.go | Loads/validates asymmetric keys at startup and wires provider options (signing key + kid). |
| internal/bootstrap/handlers.go | Builds JWKS handler from token provider and passes jwks availability into OIDC handler. |
| internal/bootstrap/router.go | Registers /.well-known/jwks.json route. |
| internal/bootstrap/services.go | Plumbs token provider into service initialization rather than constructing inside. |
| internal/bootstrap/bootstrap.go | Stores token provider on the app struct for handler initialization. |
| docs/CONFIGURATION.md | Documents JWT algorithms, key generation, JWKS endpoint, and rotation guidance. |
| .env.example | Adds commented config examples for signing algorithm/private key path/kid. |
| CLAUDE.md | Updates project documentation to reflect configurable JWT signing and JWKS endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clarify the JWT key rotation procedure in the documentation and explicitly note that only a single active JWKS key is supported, with guidance on handling rotation downtime - Tighten the JWKS public key interface to return a crypto.PublicKey instead of an untyped value - Remove a defensive panic in the local token provider and rely on bootstrap validation for signing key requirements Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds asymmetric JWT signing (RS256/ES256) alongside existing HS256 and exposes a JWKS endpoint so relying parties can verify signatures using published public keys.
Changes:
- Extend
LocalTokenProviderto support configurable signing algorithms and optionalkidJWT header. - Add PEM key loading/validation utilities and a new
/.well-known/jwks.jsonhandler + route. - Update OIDC discovery metadata to advertise the configured signing algorithm and (when applicable)
jwks_uri, plus add extensive tests/docs/configuration.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/token/local.go | Adds algorithm-aware signing/verification and option pattern for keys/kid. |
| internal/token/idtoken.go | Uses provider-selected signing method/key (no longer hardcoded HS256). |
| internal/token/key.go | New helpers for loading PEM private keys, deriving kid, and validating key/alg compatibility. |
| internal/handlers/jwks.go | New JWKS endpoint with manual RSA/EC JWK serialization. |
| internal/handlers/oidc.go | Discovery response now reflects signing algorithm and conditionally includes jwks_uri. |
| internal/bootstrap/providers.go | Loads/validates asymmetric keys and wires LocalTokenProvider options at startup. |
| internal/bootstrap/handlers.go | Builds JWKS handler and passes JWKS availability into OIDC handler. |
| internal/bootstrap/router.go | Registers /.well-known/jwks.json route. |
| internal/bootstrap/services.go | Accepts token provider as dependency instead of creating it internally. |
| internal/bootstrap/bootstrap.go | Stores token provider on Application for handler initialization. |
| internal/config/config.go | Adds new JWT signing config fields and validation. |
| internal/token/local_test.go | Adds RS256/ES256 coverage and backward-compat HS256 tests. |
| internal/token/key_test.go | New tests for key loading, kid derivation, and key/alg validation. |
| internal/handlers/jwks_test.go | Tests JWKS responses for RSA/EC and coordinate padding. |
| internal/handlers/oidc_test.go | Tests discovery metadata behavior for alg + jwks_uri. |
| docs/CONFIGURATION.md | Documents algorithm selection, key generation, JWKS endpoint, and rotation guidance. |
| .env.example | Adds example env vars for JWT signing algorithm and key settings. |
| CLAUDE.md | Updates project documentation to reflect configurable signing and JWKS endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ning - fix(config): only require JWT_PRIVATE_KEY_PATH when TOKEN_PROVIDER_MODE=local (RS256/ES256 with http_api token provider no longer triggers false validation error) - fix(token): NewLocalTokenProvider returns error if RS256/ES256 used without WithSigningKey (prevents nil pointer panic at signing time; update all call sites) - fix(token): LoadSigningKey loops through all PEM blocks instead of only the first (correctly handles EC keys with leading EC PARAMETERS block) - fix(token): DeriveKeyID uses full 32-byte SHA-256 hash instead of truncated 16 bytes (reduces collision risk; update length comment in tests) - fix(handlers): Keys() returns a copy of the internal slice to prevent external mutation - test: fix ignored rsa/ecdsa.GenerateKey errors in key_test.go with require.NoError - test: add TestNewLocalTokenProvider_RS256/ES256_NoKey_Error for constructor validation - test: add TestLoadSigningKey_MultiBlock_ECAfterUnknown for PEM loop fix - test: add TestDeriveKeyID_FullHashLength for 43-char base64url output - test: add TestJWKS_Keys_ReturnsCopy for immutability guarantee - test: add TestConfig_Validate_JWTSigningAlgorithm covering all algorithm/mode combos Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Replace fmt.Errorf with errors.New for static error strings (perfsprint) - Fix gofmt alignment in config_test.go struct literal - Wrap long assert.Equal line in jwks_test.go (golines) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for asymmetric JWT signing (RS256/ES256) alongside HS256, exposes a JWKS endpoint for public key distribution, and updates OIDC discovery metadata to reflect the active signing configuration.
Changes:
- Extend
LocalTokenProviderto support HS256/RS256/ES256 with key/kid options and validation behavior updates across token generation/validation (access/refresh/ID tokens). - Add PEM key loading +
kidderivation utilities and wire asymmetric key loading at bootstrap startup. - Introduce
/.well-known/jwks.jsonand update OIDC discovery to conditionally advertisejwks_uriand supported ID token signing algorithms.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/token/local.go | Adds algorithm selection, key options, kid header support, and asymmetric verification logic. |
| internal/token/local_test.go | Updates for new constructor signature + adds RS256/ES256 coverage and kid header checks. |
| internal/token/key.go | New utilities for PEM key loading, kid derivation, and algorithm/key validation. |
| internal/token/key_test.go | Tests for key parsing formats, kid derivation determinism, and algorithm validation. |
| internal/token/idtoken.go | ID token signing now uses the provider’s configured method/key (not hardcoded HS256). |
| internal/handlers/jwks.go | New JWKS handler and manual JWK serialization for RSA/EC public keys. |
| internal/handlers/jwks_test.go | Tests JWKS responses for RSA/EC, padding rules, and defensive copying. |
| internal/handlers/oidc.go | Discovery response now reflects configured signing algorithm and conditionally includes jwks_uri. |
| internal/handlers/oidc_test.go | Tests discovery behavior for HS256/RS256/ES256 and jwks_uri presence/absence. |
| internal/bootstrap/providers.go | Bootstraps local provider with asymmetric key loading/validation and kid derivation. |
| internal/bootstrap/handlers.go | Builds JWKS handler from the active token provider and passes JWKS availability into OIDC handler. |
| internal/bootstrap/router.go | Registers the new public JWKS endpoint route. |
| internal/bootstrap/services.go | Threads the initialized token provider into service initialization (avoids duplicate init). |
| internal/bootstrap/bootstrap.go | Stores initialized token provider on the application and passes it into handlers/services. |
| internal/config/config.go | Adds env/config fields for signing algorithm, private key path, and key id + validation logic. |
| internal/config/config_test.go | Adds validation tests for algorithm/key-path rules under different provider modes. |
| internal/services/token_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| internal/services/token_introspect_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| internal/services/token_client_credentials_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| internal/handlers/token_introspect_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| internal/handlers/token_client_credentials_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| internal/handlers/session_test.go | Updates tests for NewLocalTokenProvider returning an error. |
| docs/CONFIGURATION.md | Documents algorithm selection, key generation, JWKS endpoint behavior, and rotation notes. |
| .env.example | Adds example env vars for JWT algorithm and key configuration. |
| CLAUDE.md | Updates repo docs to reflect configurable signing algorithm + JWKS endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rs, and improve test diagnostics - Reject unsupported JWT signing algorithms explicitly instead of silently defaulting to HS256 - Change DeriveKeyID to return (string, error) so callers handle marshal failures - Add omitempty to JWKS kid field to avoid emitting empty string values - Replace panic with require.NoError in test helper for better failure diagnostics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Only return RSA and ECDSA keys from the PKCS#8 parsing branch - Skip unsupported key types (e.g. Ed25519) and continue scanning remaining PEM blocks, matching the function documented contract Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+603
to
+617
| func TestLocalTokenProvider_RS256_GenerateAndValidate(t *testing.T) { | ||
| rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) | ||
| require.NoError(t, err) | ||
|
|
||
| cfg := &config.Config{ | ||
| JWTSigningAlgorithm: "RS256", | ||
| JWTExpiration: 1 * time.Hour, | ||
| RefreshTokenExpiration: 30 * 24 * time.Hour, | ||
| BaseURL: "http://localhost:8080", | ||
| } | ||
| provider, err := NewLocalTokenProvider(cfg, | ||
| WithSigningKey(rsaKey, &rsaKey.PublicKey), | ||
| WithKeyID("test-rsa-kid"), | ||
| ) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
These RS256/ES256 tests generate fresh 2048-bit RSA keys multiple times across the file, which can noticeably slow the unit test suite. Consider using a test helper that reuses a single generated key per package (e.g., via sync.Once) or embedding a static PEM test key to reduce runtime while keeping coverage.
Comment on lines
+27
to
+45
| func TestLoadSigningKey_RSA(t *testing.T) { | ||
| rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) | ||
| require.NoError(t, err) | ||
| der := x509.MarshalPKCS1PrivateKey(rsaKey) | ||
| path := writeTestPEM(t, "RSA PRIVATE KEY", der) | ||
|
|
||
| key, err := LoadSigningKey(path) | ||
| require.NoError(t, err) | ||
| _, ok := key.(*rsa.PrivateKey) | ||
| assert.True(t, ok, "expected *rsa.PrivateKey, got %T", key) | ||
| } | ||
|
|
||
| func TestLoadSigningKey_RSA_PKCS8(t *testing.T) { | ||
| rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) | ||
| require.NoError(t, err) | ||
| der, err := x509.MarshalPKCS8PrivateKey(rsaKey) | ||
| require.NoError(t, err) | ||
| path := writeTestPEM(t, "PRIVATE KEY", der) | ||
|
|
There was a problem hiding this comment.
This test file generates several 2048-bit RSA keys (and EC keys) which can make the test suite slower and flakier on constrained CI runners. Consider reusing a single generated key across tests (sync.Once) or using embedded static PEM fixtures for faster, deterministic tests.
…nc.Once - Extract shared signClaims method to eliminate duplicate JWT signing logic between generateJWT and GenerateIDToken - Add sync.Once shared test key fixtures in local_test.go and jwks_test.go to avoid repeated 2048-bit RSA key generation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Store key generation errors and assert with require.NoError after Do - Avoids crashing entire package test run on key generation failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… error - Reject RSA keys shorter than 2048 bits at construction time - Use %w for both errors in signClaims to enable proper error unwrapping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+100
| if cfg.JWTSigningAlgorithm == "HS256" || cfg.JWTSigningAlgorithm == "" { | ||
| log.Printf("Token signing: HS256 (symmetric)") | ||
| p, err := token.NewLocalTokenProvider(cfg) | ||
| if err != nil { | ||
| log.Fatalf("Failed to create token provider: %v", err) | ||
| } | ||
| return p | ||
| } | ||
|
|
||
| // Load asymmetric key | ||
| privateKey, err := token.LoadSigningKey(cfg.JWTPrivateKeyPath) | ||
| if err != nil { | ||
| log.Fatalf("Failed to load JWT private key from %s: %v", cfg.JWTPrivateKeyPath, err) | ||
| } | ||
|
|
||
| // Derive kid if not explicitly set | ||
| kid := cfg.JWTKeyID | ||
| if kid == "" { | ||
| var err error | ||
| kid, err = token.DeriveKeyID(privateKey.Public()) | ||
| if err != nil { | ||
| log.Fatalf("Failed to derive JWT key ID: %v", err) | ||
| } | ||
| } | ||
|
|
||
| log.Printf("Token signing: %s (kid=%s)", cfg.JWTSigningAlgorithm, kid) | ||
| p, err := token.NewLocalTokenProvider(cfg, | ||
| token.WithSigningKey(privateKey, privateKey.Public()), | ||
| token.WithKeyID(kid), | ||
| ) | ||
| if err != nil { | ||
| log.Fatalf("Failed to create token provider: %v", err) | ||
| } | ||
| return p |
There was a problem hiding this comment.
newLocalTokenProvider treats any non-HS256 algorithm as requiring an asymmetric key and attempts to read JWTPrivateKeyPath before verifying the algorithm is one of the supported asymmetric values (RS256/ES256). If JWT_SIGNING_ALGORITHM is mis-set (or future algorithms are added), this will fail with a confusing “read key file” fatal instead of an “unsupported algorithm” error. Consider switching on cfg.JWTSigningAlgorithm and only loading a key for RS256/ES256; for other values, let token.NewLocalTokenProvider return the unsupported-algorithm error (or fatal with that message).
Suggested change
| if cfg.JWTSigningAlgorithm == "HS256" || cfg.JWTSigningAlgorithm == "" { | |
| log.Printf("Token signing: HS256 (symmetric)") | |
| p, err := token.NewLocalTokenProvider(cfg) | |
| if err != nil { | |
| log.Fatalf("Failed to create token provider: %v", err) | |
| } | |
| return p | |
| } | |
| // Load asymmetric key | |
| privateKey, err := token.LoadSigningKey(cfg.JWTPrivateKeyPath) | |
| if err != nil { | |
| log.Fatalf("Failed to load JWT private key from %s: %v", cfg.JWTPrivateKeyPath, err) | |
| } | |
| // Derive kid if not explicitly set | |
| kid := cfg.JWTKeyID | |
| if kid == "" { | |
| var err error | |
| kid, err = token.DeriveKeyID(privateKey.Public()) | |
| if err != nil { | |
| log.Fatalf("Failed to derive JWT key ID: %v", err) | |
| } | |
| } | |
| log.Printf("Token signing: %s (kid=%s)", cfg.JWTSigningAlgorithm, kid) | |
| p, err := token.NewLocalTokenProvider(cfg, | |
| token.WithSigningKey(privateKey, privateKey.Public()), | |
| token.WithKeyID(kid), | |
| ) | |
| if err != nil { | |
| log.Fatalf("Failed to create token provider: %v", err) | |
| } | |
| return p | |
| switch cfg.JWTSigningAlgorithm { | |
| case "", "HS256": | |
| log.Printf("Token signing: HS256 (symmetric)") | |
| p, err := token.NewLocalTokenProvider(cfg) | |
| if err != nil { | |
| log.Fatalf("Failed to create token provider: %v", err) | |
| } | |
| return p | |
| case "RS256", "ES256": | |
| // Load asymmetric key | |
| privateKey, err := token.LoadSigningKey(cfg.JWTPrivateKeyPath) | |
| if err != nil { | |
| log.Fatalf("Failed to load JWT private key from %s: %v", cfg.JWTPrivateKeyPath, err) | |
| } | |
| // Derive kid if not explicitly set | |
| kid := cfg.JWTKeyID | |
| if kid == "" { | |
| var err error | |
| kid, err = token.DeriveKeyID(privateKey.Public()) | |
| if err != nil { | |
| log.Fatalf("Failed to derive JWT key ID: %v", err) | |
| } | |
| } | |
| log.Printf("Token signing: %s (kid=%s)", cfg.JWTSigningAlgorithm, kid) | |
| p, err := token.NewLocalTokenProvider(cfg, | |
| token.WithSigningKey(privateKey, privateKey.Public()), | |
| token.WithKeyID(kid), | |
| ) | |
| if err != nil { | |
| log.Fatalf("Failed to create token provider: %v", err) | |
| } | |
| return p | |
| default: | |
| // Let token.NewLocalTokenProvider validate and report unsupported algorithms. | |
| p, err := token.NewLocalTokenProvider(cfg) | |
| if err != nil { | |
| log.Fatalf("Failed to create token provider: %v", err) | |
| } | |
| return p | |
| } |
Comment on lines
+512
to
+523
| // Validate token provider mode | ||
| switch c.TokenProviderMode { | ||
| case "", TokenProviderModeLocal, TokenProviderModeHTTPAPI: | ||
| // ok | ||
| default: | ||
| return fmt.Errorf( | ||
| "invalid TOKEN_PROVIDER_MODE value: %q (must be empty, %q, or %q)", | ||
| c.TokenProviderMode, | ||
| TokenProviderModeLocal, | ||
| TokenProviderModeHTTPAPI, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Config.Validate currently accepts an empty TokenProviderMode, but bootstrap validation (validateTokenProviderConfig) only accepts explicit "local" or "http_api". Even if env loading normally defaults this to "local", having two validators disagree increases the risk of confusing behavior for programmatic configs/tests. Consider normalizing "" to TokenProviderModeLocal in one place (or disallowing "" here) so the validation rules are consistent across the app.
| func (p *LocalTokenProvider) keyFunc(token *jwt.Token) (any, error) { | ||
| if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { | ||
| if token.Method.Alg() != p.method.Alg() { | ||
| return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) |
There was a problem hiding this comment.
keyFunc verifies the algorithm via token.Method.Alg() but the error message prints token.Header["alg"], which can be missing or differ in formatting and makes debugging harder. Consider including both the expected algorithm (p.method.Alg()) and the actual algorithm (token.Method.Alg()) in the error message, and avoid relying on the raw header value.
Suggested change
| return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) | |
| return nil, fmt.Errorf("unexpected signing method: expected %s, got %s", p.method.Alg(), token.Method.Alg()) |
…improve error messages - Check algorithm is RS256/ES256 before attempting key file read in bootstrap - Normalize empty TokenProviderMode to local in Config.Validate - Include expected and actual algorithm in keyFunc error message Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/handlers/oidc.go:1
/.well-known/openid-configurationis an OIDC Discovery document, andid_token_signing_alg_values_supportedis a required field for OIDC Providers (and many clients will expect it to be present). Withomitempty+idTokenSupported == false, this field is omitted entirely, which can break standards-compliant discovery clients. If this server intentionally supports “OAuth-only” mode, consider returning a non-OIDC metadata document (or disabling this endpoint) in that mode; otherwise, always includeid_token_signing_alg_values_supported(and the OpenID scopes) when serving OIDC discovery.
package handlers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ation test key - Remove stale 'must be empty' from TOKEN_PROVIDER_MODE error message - Add Cache-Control: public, max-age=3600 to JWKS endpoint - Cache second RSA key via sync.Once for cross-validation test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
JWT_SIGNING_ALGORITHMenv varGET /.well-known/jwks.jsonexposes public keys for resource servers to verify JWTs without shared secretsid_token_signing_alg_values_supportedreflects configured algorithm;jwks_uriincluded for asymmetric keysNew Config Fields
JWT_SIGNING_ALGORITHMHS256HS256,RS256, orES256JWT_PRIVATE_KEY_PATHJWT_KEY_IDkidheader value (auto-derived from key fingerprint)Key Files Changed
internal/token/key.gointernal/token/local.goWithSigningKey,WithKeyID)internal/token/idtoken.gointernal/handlers/jwks.gointernal/handlers/oidc.gojwks_uriinternal/config/config.gointernal/bootstrap/providers.goBackward Compatibility
HS256mode is fully backward compatible — no config changes needed{"keys":[]}for HS256 (symmetric secrets never exposed)Test plan
make testpasses (all existing + 30 new tests)make lintpasses (0 issues)openssl genrsa -out /tmp/rsa.pem 2048+JWT_SIGNING_ALGORITHM=RS256 JWT_PRIVATE_KEY_PATH=/tmp/rsa.pem→ verify tokens and JWKSopenssl ecparam -genkey -name prime256v1 -noout -out /tmp/ec.pem+JWT_SIGNING_ALGORITHM=ES256 JWT_PRIVATE_KEY_PATH=/tmp/ec.pem→ verify tokens and JWKScurl /.well-known/openid-configurationincludesjwks_urifor RS256/ES256🤖 Generated with Claude Code