feat: implement JWKS endpoint (#171)#199
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 38 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughJWT signing is migrated from HMAC shared secrets to RSA key pairs (RS256) across config, token service, and tests. ChangesRSA JWT Migration and JWKS Endpoint
Sequence DiagramsequenceDiagram
participant ResourceServer
participant GinRouter
participant JWKSHandler
participant TokenService
participant Config
ResourceServer->>GinRouter: GET /.well-known/jwks.json
GinRouter->>JWKSHandler: GetJWKS(c *gin.Context)
JWKSHandler->>Config: Read JWT.PublicKey, JWT.KeyID
JWKSHandler-->>ResourceServer: HTTP 200 { keys: [{ kty, alg, use, kid, n, e }] }
ResourceServer->>ResourceServer: Cache public key by kid
ResourceServer->>TokenService: Verify JWT (RS256, kid lookup)
TokenService->>Config: Load RSA PrivateKey for signing
TokenService-->>ResourceServer: Validated claims
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config.go`:
- Around line 197-204: The condition checking `err1 != nil || err2 != nil` is
too broad and treats all read failures (permission errors, invalid paths, config
errors) as reasons to generate temporary in-memory RSA keys. This can cause
silent key rotation and token invalidation across restarts. Instead, narrow the
fallback logic to only generate temporary keys when the RSA key files are
specifically not found (using os.IsNotExist() checks on err1 and err2), and let
other types of errors fail fast by returning or logging them as fatal errors
rather than silently falling back to temporary key generation.
In `@internal/handler/jwks_handler.go`:
- Around line 31-33: The error response in the pubKey == nil check is missing
the required `code` field that must be included in all handler error responses.
Update the gin.H map in this condition (currently only containing the `error`
field) to also include a `code` field with an appropriate error code identifier
to comply with the handler contract that requires the format
{"error":"message","code":"ERROR_CODE"}.
- Around line 22-28: The Swagger annotations on the GetJWKS method in
JWKSHandler are correctly defined, but the generated documentation artifacts in
the docs directory are out of date and do not reflect the new endpoint. Run the
make swagger command to regenerate the documentation files (swagger.json,
swagger.yaml, and docs.go) so that the /.well-known/jwks.json endpoint is
included in the API documentation.
In `@internal/service/token_service_test.go`:
- Around line 14-16: The getTestRSAKeys helper function ignores the error
returned by rsa.GenerateKey, which could cause a nil pointer panic if key
generation fails. Add a *testing.T parameter to the getTestRSAKeys function
signature, capture the error return value from rsa.GenerateKey instead of
ignoring it with the blank identifier, and add an explicit error check that
calls t.Fatalf to fail the test if the error is not nil, ensuring the helper
fails the test gracefully rather than potentially panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc793b31-f794-4842-bf01-ef22f6a481e0
📒 Files selected for processing (6)
internal/config/config.gointernal/dto/jwks.gointernal/handler/jwks_handler.gointernal/routes/routes.gointernal/service/token_service.gointernal/service/token_service_test.go
| if err1 != nil || err2 != nil { | ||
| log.Println("RSA keys not found at provided paths, generating temporary in-memory keys for development/testing...") | ||
| privKey, err := rsa.GenerateKey(rand.Reader, 2048) | ||
| if err != nil { | ||
| log.Fatalf("Failed to generate temp RSA key: %v", err) | ||
| } | ||
| return privKey, &privKey.PublicKey | ||
| } |
There was a problem hiding this comment.
Narrow RSA fallback to dev-only missing-file cases.
At Line 197, the err1 != nil || err2 != nil branch treats all read failures as “generate temp keys,” including permission/path/config errors. This can silently rotate signing keys and invalidate tokens across restarts/instances instead of failing fast.
Suggested fix
import (
+ "errors"
"crypto/rand"
"crypto/rsa"
"log"
"os"
"strconv"
@@
privBytes, err1 := os.ReadFile(privPath)
pubBytes, err2 := os.ReadFile(pubPath)
- if err1 != nil || err2 != nil {
- log.Println("RSA keys not found at provided paths, generating temporary in-memory keys for development/testing...")
- privKey, err := rsa.GenerateKey(rand.Reader, 2048)
- if err != nil {
- log.Fatalf("Failed to generate temp RSA key: %v", err)
- }
- return privKey, &privKey.PublicKey
- }
+ if err1 != nil || err2 != nil {
+ appEnv := getEnv("APP_ENV", "development")
+ bothMissing := errors.Is(err1, os.ErrNotExist) && errors.Is(err2, os.ErrNotExist)
+ if appEnv != "production" && bothMissing {
+ log.Println("RSA key files not found; generating temporary in-memory keys for development/testing")
+ privKey, err := rsa.GenerateKey(rand.Reader, 2048)
+ if err != nil {
+ log.Fatalf("Failed to generate temp RSA key: %v", err)
+ }
+ return privKey, &privKey.PublicKey
+ }
+ log.Fatalf("Failed to read RSA key files (private: %v, public: %v)", err1, err2)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/config.go` around lines 197 - 204, The condition checking
`err1 != nil || err2 != nil` is too broad and treats all read failures
(permission errors, invalid paths, config errors) as reasons to generate
temporary in-memory RSA keys. This can cause silent key rotation and token
invalidation across restarts. Instead, narrow the fallback logic to only
generate temporary keys when the RSA key files are specifically not found (using
os.IsNotExist() checks on err1 and err2), and let other types of errors fail
fast by returning or logging them as fatal errors rather than silently falling
back to temporary key generation.
|
|
I have read the CLA and agree to its terms. |
|
@TharunvenkateshN A couple of things worth looking at before merging:
|
2 participants
Description
Closes #171
This PR implements the JSON Web Key Set (JWKS) endpoint to allow downstream resource servers to cryptographically verify JWTs autonomously using asymmetric encryption.
Changes Made
HS256(symmetric) toRS256(asymmetric RSA).internal/config/config.goto parseprivate.pemandpublic.pem. Added fallback logic to dynamically generate an in-memory temporary key pair for seamless local development if keys are not provided.kid(Key ID) header during token generation so clients know which public key to use.internal/handler/jwks_handler.goto safely expose the RSA public key (modulus and exponent) in a standards-compliant JWK format atGET /.well-known/jwks.json.token_service_test.goand other tests to generate and utilize RS256 keys.How to Test
http://localhost:3000/.well-known/jwks.jsonto view the exposed public keys.RS256with akidheader.Summary by CodeRabbit
Release Notes
/.well-known/jwks.jsonendpoint for JWT key distribution following OpenID Connect standards.