Implement JWKS key rotation with multi-key support and HTTP caching #2
Conversation
- Add KeyState enum (ACTIVE, PASSIVE, RETIRED) for key lifecycle management - Refactor KeyManager to maintain a key ring of multiple keys - Update GetJWKS() to return all ACTIVE and PASSIVE keys - Add methods for key lifecycle: AddKeyPassive, PromoteKey, DemoteKey, RetireKey, ListKeys - Implement HTTP cache headers (Cache-Control, ETag) for JWKS endpoint - Add comprehensive tests for key rotation and lifecycle management - Maintain backwards compatibility with single-key usage Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
- Create comprehensive KEY_ROTATION.md guide with procedures and best practices - Add working example demonstrating the complete rotation workflow - Document key lifecycle states, rotation timeline, and troubleshooting - Include monitoring recommendations and emergency rotation procedures Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
- Ensure only one ACTIVE key exists by demoting existing ACTIVE keys - Update GenerateKeyPair to demote existing ACTIVE key before adding new one - Update InitializeFromPEM to demote existing ACTIVE key before adding new one - Add tests to validate single ACTIVE key constraint - Fix linting issue in example code Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
- Simplify key rotation test to avoid unused helper function - Remove unused crypto/rsa and protobuf imports from test file - Improve test comments for clarity Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
- Add KeyInfo named struct type for ListKeys return value - Improve ETag comment to explain size choice and HTTP spec compliance - All tests passing, builds successful Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
|
@copilot Change the name of internal/infrastructure/security/jkws.go into the right one |
The file was incorrectly named jkws.go (JSON Web Key Set with letters transposed). Renamed to jwks.go to match the standard JWKS acronym. Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements JWKS key rotation infrastructure for Identra's JWT access tokens, enabling zero-downtime key updates with proper cache management.
Changes:
- Renamed jkws.go to jwks.go (typo fix) and refactored KeyManager to support multi-key rotation with ACTIVE/PASSIVE/RETIRED states
- Added HTTP caching headers (Cache-Control, ETag) to the JWKS endpoint with appropriate max-age settings
- Comprehensive test coverage for key lifecycle management and rotation scenarios
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/infrastructure/security/jwks.go | New implementation of KeyManager with key ring support for rotation, replacing the old single-key jkws.go |
| internal/infrastructure/security/jkws.go | Deleted old file (renamed to jwks.go with typo fix) |
| internal/infrastructure/security/tokens_test.go | Added comprehensive tests for key rotation, lifecycle management, and multi-key scenarios |
| internal/application/identra/service.go | Added Cache-Control and ETag headers to GetJWKS endpoint for efficient client-side caching |
| cmd/identra-gateway/main.go | Updated header matcher to pass through Cache-Control and ETag headers to HTTP responses |
| docs/KEY_ROTATION.md | Comprehensive documentation on key rotation procedures, timelines, and best practices |
| docs/examples/key_rotation.go | Working example demonstrating the complete key rotation workflow |
| .gitignore | Added build artifact patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use first 16 bytes (128 bits) for shorter ETag - balances uniqueness with header size | ||
| // Quoted per HTTP ETag specification (RFC 7232) | ||
| return fmt.Sprintf(`"%x"`, hash[:16]) |
There was a problem hiding this comment.
The ETag generation uses only the first 16 bytes of the hash. While this is mentioned in the comment, consider that hash collisions, though unlikely, could occur if many different key sets are in use. For production systems with critical caching requirements, consider using the full hash (32 bytes for SHA-256) to minimize collision risk, or document why 16 bytes is sufficient for this use case.
| // Use first 16 bytes (128 bits) for shorter ETag - balances uniqueness with header size | |
| // Quoted per HTTP ETag specification (RFC 7232) | |
| return fmt.Sprintf(`"%x"`, hash[:16]) | |
| // Use the full 32 bytes (256 bits) of the SHA-256 hash to minimize collision risk. | |
| // Quoted per HTTP ETag specification (RFC 7232) | |
| return fmt.Sprintf(`"%x"`, hash[:]) |
| for _, entry := range km.keys { | ||
| if entry.state == KeyStateActive || entry.state == KeyStatePassive { | ||
| n := base64.RawURLEncoding.EncodeToString(entry.publicKey.N.Bytes()) | ||
| e := base64.RawURLEncoding.EncodeToString(big.NewInt(int64(entry.publicKey.E)).Bytes()) | ||
|
|
||
| keys = append(keys, &identra_v1_pb.JSONWebKey{ | ||
| Kty: "RSA", | ||
| Alg: KeyAlgorithm, | ||
| Use: KeyUsage, | ||
| Kid: entry.keyID, | ||
| N: &n, | ||
| E: &e, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The GetJWKS function iterates over a map (km.keys) which has non-deterministic iteration order in Go. This means the order of keys in the JWKS response may vary between calls even when the key set hasn't changed, potentially affecting the ETag consistency. Consider sorting the keys by KeyID before building the response to ensure deterministic output and stable ETags.
| // Concatenate all key IDs and hash them | ||
| var keyIDs string | ||
| for _, key := range jwks.Keys { | ||
| keyIDs += key.Kid | ||
| } | ||
|
|
||
| hash := sha256.Sum256([]byte(keyIDs)) |
There was a problem hiding this comment.
The ETag generation in generateJWKSETag concatenates key IDs without delimiters. If key IDs can have variable lengths, this could theoretically lead to hash collisions (e.g., key IDs "ab" + "cd" produces the same string as "a" + "bcd"). While the current implementation uses fixed-length base64url encoded hashes making this unlikely, consider adding a delimiter between key IDs for robustness.
| // Concatenate all key IDs and hash them | |
| var keyIDs string | |
| for _, key := range jwks.Keys { | |
| keyIDs += key.Kid | |
| } | |
| hash := sha256.Sum256([]byte(keyIDs)) | |
| // Join all key IDs with a delimiter to avoid ambiguous concatenations, then hash them | |
| keyIDs := make([]string, 0, len(jwks.Keys)) | |
| for _, key := range jwks.Keys { | |
| keyIDs = append(keyIDs, key.Kid) | |
| } | |
| hash := sha256.Sum256([]byte(strings.Join(keyIDs, ","))) |
|
@copilot apply changes based on the comments in this thread |
- Use full 32-byte SHA-256 hash for ETag instead of 16 bytes to minimize collision risk - Add delimiters between key IDs when generating ETag to avoid ambiguous concatenations - Sort JWKS keys by KeyID to ensure deterministic output and stable ETags - Add test to verify deterministic JWKS ordering across multiple calls Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
Applied all three code review suggestions in commit 1ad61ef:
Also added a new test |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/poly-workshop/identra/internal/infrastructure/security" | ||
| ) |
There was a problem hiding this comment.
The example code imports from the internal package path, which may not be accessible when the example is run from the docs/examples directory. In Go, packages under 'internal' can only be imported by packages within the same module subtree. Consider either moving the example to a location within the internal tree, or creating a public API in a non-internal package that wraps the necessary functionality.
|
@copilot apply changes based on the comments in this thread |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 28. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
@copilot Fix golangci-lint issues |
Per code review feedback, moved docs/examples/key_rotation.go to internal/examples/key_rotation.go to properly reflect that this example uses internal APIs which are not accessible to external packages per Go's internal package restrictions. Added clarifying comments explaining: - Why the example is in the internal/ tree - That it's intended for internal operators - Go's internal package access rules Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
Fixed in commit 300af81. Moved the example from |
- Remove trailing whitespace - Fix alignment of struct fields and constant declarations - All changes are formatting-only, no logic changes Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com>
Implementation Plan: JWKS Key Rotation Support
Phase 1: Key Ring Data Structure ✅
KeyStateenum (ACTIVE, PASSIVE, RETIRED) to track key lifecycleKeyManagerto store multiple keys with their states in a key ringGetPrivateKey()to return the ACTIVE signing keyGetJWKS()to return both ACTIVE and PASSIVE keysPhase 2: Key Lifecycle Management ✅
Phase 3: HTTP Cache Headers ✅
Cache-Controlheader support to JWKS endpointETagheader support for efficient cache refreshesPhase 4: Testing ✅
Phase 5: Documentation ✅
Code Quality ✅
Out of Scope (as specified) ✅
Summary
✅ ALL REQUIREMENTS COMPLETED
All planned features have been successfully implemented, tested, and documented. The JWKS key rotation infrastructure is production-ready with:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.