[APIP] Add Backend JWT Policy#200
Open
DDH13 wants to merge 22 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements a new Backend JWT gateway policy: metadata and README entry, full user documentation and policy-definition schema, Go module, policy factory and runtime handler, PEM private-key loading/parsing (RSA/EC/PKCS#8), signing (RSA/EC/NONE), deterministic token cache keys, in-memory token/key caches with eviction, $ctx: runtime claim resolution, requireAuthentication handling, and a comprehensive Go test suite. Sequence DiagramsequenceDiagram
participant Client
participant GatewayPolicy as Backend JWT Policy
participant AuthContext
participant KeyCache
participant TokenCache
participant Signer as JWT Signer
participant Backend
Client->>GatewayPolicy: incoming HTTP request
GatewayPolicy->>AuthContext: read authentication context
alt requireAuthentication=true && no auth
GatewayPolicy->>Client: 401 Unauthorized
else
GatewayPolicy->>TokenCache: lookup token by identity+path+claims hash
alt cached token exists & valid
TokenCache->>GatewayPolicy: return cached token
else
GatewayPolicy->>KeyCache: load & parse private key
GatewayPolicy->>GatewayPolicy: resolve $ctx: claims
GatewayPolicy->>Signer: build and sign JWT
Signer->>TokenCache: cache token with TTL (half expiry, min 30s)
TokenCache->>GatewayPolicy: return signed token
end
GatewayPolicy->>Backend: forward request with token in configured header
end
Backend->>Backend: verify JWT using gateway public key
Suggested reviewers
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
policies/backend-jwt/backendjwt_test.go (1)
232-292: ⚡ Quick winConsider testing overlapping claim names to document precedence.
The test suite covers
customClaimsandclaimMappingsindependently. Adding a test where both configurations produce a claim with the same name would explicitly document which takes precedence. Based on the implementation, custom claims are added after mapped claims, so custom claims override in case of collision. A test clarifying this behavior would aid future maintainability and make the design intent explicit.Example test structure
func TestCustomClaimsOverrideClaimMappings(t *testing.T) { rsaKey, keyPEM := generateRSAKey(t) p := &BackendJWTPolicy{keyCache: make(map[[32]byte]crypto.PrivateKey), tokenCache: make(map[string]cachedToken)} params := baseParams(keyPEM) params["claimMappings"] = map[string]interface{}{ "dept": "department", // Maps auth property "dept" to JWT claim "department" } params["customClaims"] = map[string]interface{}{ "department": "override-value", // Custom claim with same name } reqCtx := newRequestContext(&policy.AuthContext{ Authenticated: true, AuthType: "jwt", Subject: "test-user", Properties: map[string]string{"dept": "engineering"}, }) result := p.OnRequestHeaders(context.Background(), reqCtx, params) mods := result.(policy.UpstreamRequestHeaderModifications) claims := decodeJWT(t, mods.HeadersToSet[defaultHeader], &rsaKey.PublicKey) if claims["department"] != "override-value" { t.Errorf("expected customClaims to override claimMappings, got %v", claims["department"]) } }🤖 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 `@policies/backend-jwt/backendjwt_test.go` around lines 232 - 292, Add a new unit test that verifies customClaims override claimMappings by creating a BackendJWTPolicy (keyCache/tokenCache), using baseParams with both "claimMappings" mapping an auth property (e.g., "dept" -> "department") and "customClaims" containing the same claim name ("department": "override-value"), then call p.OnRequestHeaders with a request context whose Properties include dept, decode the JWT (using decodeJWT and rsa key from generateRSAKey) and assert the "department" claim equals the customClaims value; place this alongside TestCustomClaims/TestClaimMappings to document precedence.
🤖 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 `@policies/backend-jwt/backendjwt.go`:
- Around line 156-165: The cache key built by buildTokenCacheKey omits
authCtx.Audience so different audiences can share one cached token; update
buildTokenCacheKey to accept an audience parameter (e.g., audience string or
[]string normalized to a canonical string) and include it in the key/hash
computation, then update every callsite (including the call in backendjwt.go
that currently passes authCtx.Subject, authCtx.CredentialID, authCtx.AuthType,
authCtx.Issuer, reqCtx.OperationPath, issuer, algorithm, extras) to also pass
authCtx.Audience, and ensure getCachedToken and any cache-set logic uses the new
key so tokens with different aud claims are cached separately.
---
Nitpick comments:
In `@policies/backend-jwt/backendjwt_test.go`:
- Around line 232-292: Add a new unit test that verifies customClaims override
claimMappings by creating a BackendJWTPolicy (keyCache/tokenCache), using
baseParams with both "claimMappings" mapping an auth property (e.g., "dept" ->
"department") and "customClaims" containing the same claim name ("department":
"override-value"), then call p.OnRequestHeaders with a request context whose
Properties include dept, decode the JWT (using decodeJWT and rsa key from
generateRSAKey) and assert the "department" claim equals the customClaims value;
place this alongside TestCustomClaims/TestClaimMappings to document precedence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dd323af-3ebb-4dce-9816-68105f578b45
📒 Files selected for processing (6)
docs/README.mddocs/backend-jwt/v1.0/docs/backend-jwt.mddocs/backend-jwt/v1.0/metadata.jsonpolicies/backend-jwt/backendjwt.gopolicies/backend-jwt/backendjwt_test.gopolicies/backend-jwt/policy-definition.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@policies/backend-jwt/backendjwt.go`:
- Around line 185-188: The cache key currently built by buildTokenCacheKey omits
authCtx.AuthType, authCtx.Issuer and the policy configuration parameters
(issuer, algorithm, tokenExpiry), causing incorrect cache reuse; update
buildTokenCacheKey to accept authCtx.AuthType and authCtx.Issuer plus the policy
config values (issuer, algorithm, tokenExpiry) in its signature and include them
in the hash/input used to build the cache key (ensure the call site where
buildTokenCacheKey is invoked with authCtx.CredentialID, authCtx.Subject,
reqCtx.APIContext, reqCtx.APIVersion, authCtx.Audience, extras is updated to
pass the new fields as well).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 295e666f-558e-43bb-a87b-88511271bbe9
📒 Files selected for processing (6)
docs/README.mddocs/backend-jwt/v1.0/docs/backend-jwt.mddocs/backend-jwt/v1.0/metadata.jsonpolicies/backend-jwt/backendjwt.gopolicies/backend-jwt/backendjwt_test.gopolicies/backend-jwt/policy-definition.yaml
✅ Files skipped from review due to trivial changes (2)
- docs/README.md
- docs/backend-jwt/v1.0/docs/backend-jwt.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/backend-jwt/v1.0/metadata.json
- policies/backend-jwt/policy-definition.yaml
- policies/backend-jwt/backendjwt_test.go
DDH13
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
policies/backend-jwt/backendjwt.go (2)
132-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject invalid or non-positive
tokenExpiryvalues during validation.
parseDurationsilently falls back to the default on parse errors and also accepts0or negative durations. That lets a bad policy config validate successfully but mint tokens with an unintended or already-expired lifetime. Please validatetokenExpiryinValidate()and require a positive duration.Also applies to: 177-177, 502-510
🤖 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 `@policies/backend-jwt/backendjwt.go` around lines 132 - 149, In BackendJWTPolicy.Validate, explicitly validate the "tokenExpiry" param: call parseDuration on getString(params, "tokenExpiry", "") (or the same retrieval used elsewhere), and if parsing fails or the resulting duration is <= 0 return an error (e.g., "invalid tokenExpiry: must be a positive duration"); do this before returning success so malformed, zero or negative expiries are rejected. Apply the same check where tokenExpiry is validated elsewhere (the other Validate/creation sites referenced around the other occurrences) to ensure all policy paths enforce a positive token lifetime.
523-570:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
auth.propertykey casing during$ctx:resolution.The code lowercases the full
$ctx:reference before lookup, so$ctx:auth.property.ClientNamebecomesclientname. Mixed-case property keys will be skipped even when present. Lowercase only the fixed prefix and keep the property suffix unchanged. Based on the documented$ctx:auth.property.<key>mapping toAuthContext.Properties.🤖 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 `@policies/backend-jwt/backendjwt.go` around lines 523 - 570, The issue is that the code lowercases the entire ctx value (variable := strings.ToLower(strings.TrimPrefix(value, ctxPrefix))), so keys like auth.property.ClientName are lowercased and not found; change the logic to only do case-insensitive matching on the fixed prefix/field names while preserving the suffix/key casing for auth.property lookups. Concretely: compute variable := strings.TrimPrefix(value, ctxPrefix) (preserve original), use a lowercased helper (e.g., lower := strings.ToLower(variable)) for the switch/HasPrefix checks (request.path, request.header., auth.property., etc.), and when handling the "auth.property." case extract the key with strings.TrimPrefix(variable, "auth.property.") (not from the lowercased string) so the original casing of the property key is preserved when accessing reqCtx.SharedContext.AuthContext.Properties.
🧹 Nitpick comments (1)
policies/backend-jwt/backendjwt_test.go (1)
997-1030: ⚡ Quick winCover the full reserved-claim set in this regression test.
The implementation also blocks
aud,exp, andiat, but this case only assertsissandsub. Adding the remaining assertions would make the new restricted-claim behavior fully regression-safe.🤖 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 `@policies/backend-jwt/backendjwt_test.go` around lines 997 - 1030, TestCustomClaims_RestrictedClaimsIgnored only asserts iss and sub but the implementation also treats aud, exp and iat as restricted; update the test (TestCustomClaims_RestrictedClaimsIgnored) to assert that claims["aud"] remains the original audience, and that claims["exp"] and claims["iat"] are not overridden by the customClaims (validate their values/types from the decoded JWT via decodeJWT and the token produced by p.OnRequestHeaders / defaultHeader), so the test covers the full reserved-claim set (iss, sub, aud, exp, iat) and ensures non-restricted claims like env are still set.
🤖 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 `@policies/backend-jwt/backendjwt.go`:
- Around line 300-305: The runtime currently drops keys in restrictedClaims when
merging params["customClaims"], which conflicts with policy-definition.yaml that
states customClaims run last and may override derived claims; update the merge
logic in backendjwt.go (around the params["customClaims"] handling and the
restrictedClaims check) to allow custom claim keys to overwrite existing claims
instead of warning/continuing (i.e., remove or change the block that calls
slog.Warn and continue), ensuring customClaims are applied after derived claims
so they take precedence, or alternatively update the policy-definition.yaml to
document that reserved claims cannot be overridden—choose one approach in this
PR so runtime behavior and schema/docs stay consistent.
---
Outside diff comments:
In `@policies/backend-jwt/backendjwt.go`:
- Around line 132-149: In BackendJWTPolicy.Validate, explicitly validate the
"tokenExpiry" param: call parseDuration on getString(params, "tokenExpiry", "")
(or the same retrieval used elsewhere), and if parsing fails or the resulting
duration is <= 0 return an error (e.g., "invalid tokenExpiry: must be a positive
duration"); do this before returning success so malformed, zero or negative
expiries are rejected. Apply the same check where tokenExpiry is validated
elsewhere (the other Validate/creation sites referenced around the other
occurrences) to ensure all policy paths enforce a positive token lifetime.
- Around line 523-570: The issue is that the code lowercases the entire ctx
value (variable := strings.ToLower(strings.TrimPrefix(value, ctxPrefix))), so
keys like auth.property.ClientName are lowercased and not found; change the
logic to only do case-insensitive matching on the fixed prefix/field names while
preserving the suffix/key casing for auth.property lookups. Concretely: compute
variable := strings.TrimPrefix(value, ctxPrefix) (preserve original), use a
lowercased helper (e.g., lower := strings.ToLower(variable)) for the
switch/HasPrefix checks (request.path, request.header., auth.property., etc.),
and when handling the "auth.property." case extract the key with
strings.TrimPrefix(variable, "auth.property.") (not from the lowercased string)
so the original casing of the property key is preserved when accessing
reqCtx.SharedContext.AuthContext.Properties.
---
Nitpick comments:
In `@policies/backend-jwt/backendjwt_test.go`:
- Around line 997-1030: TestCustomClaims_RestrictedClaimsIgnored only asserts
iss and sub but the implementation also treats aud, exp and iat as restricted;
update the test (TestCustomClaims_RestrictedClaimsIgnored) to assert that
claims["aud"] remains the original audience, and that claims["exp"] and
claims["iat"] are not overridden by the customClaims (validate their
values/types from the decoded JWT via decodeJWT and the token produced by
p.OnRequestHeaders / defaultHeader), so the test covers the full reserved-claim
set (iss, sub, aud, exp, iat) and ensures non-restricted claims like env are
still set.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fe58fb7-cb37-4450-9f46-10212825dd8c
⛔ Files ignored due to path filters (1)
policies/backend-jwt/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
docs/backend-jwt/v1.0/docs/backend-jwt.mdpolicies/backend-jwt/backendjwt.gopolicies/backend-jwt/backendjwt_test.gopolicies/backend-jwt/go.modpolicies/backend-jwt/policy-definition.yaml
💤 Files with no reviewable changes (1)
- policies/backend-jwt/policy-definition.yaml
✅ Files skipped from review due to trivial changes (2)
- policies/backend-jwt/go.mod
- docs/backend-jwt/v1.0/docs/backend-jwt.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
policies/backend-jwt/backendjwt_test.go (1)
1030-1059: 💤 Low valueTest name suggests warning verification but doesn't verify logs.
The test name
TestCustomClaims_RestrictedClaimsOverrideWithWarnincludes "WithWarn", suggesting it verifies that a warning is logged when restricted claims are overridden. However, the test only verifies the override behavior itself (thatissandsubcan be overridden viacustomClaims) without checking for any warning logs.Consider either removing "WithWarn" from the name if warnings aren't logged, or adding log verification if warnings are intended.
🤖 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 `@policies/backend-jwt/backendjwt_test.go` around lines 1030 - 1059, The test TestCustomClaims_RestrictedClaimsOverrideWithWarn either needs to assert the expected warning is emitted or be renamed to remove "WithWarn"; to fix, decide which is intended: if a warning should be logged when restricted claims ("iss", "sub") are overridden, update the test to capture the policy logger (or inject a test logger) around the call to p.OnRequestHeaders and assert the logger received a warning message mentioning the restricted claim override after calling p.OnRequestHeaders (referencing the BackendJWTPolicy instance p and the OnRequestHeaders call); otherwise rename the test to TestCustomClaims_RestrictedClaimsOverride (remove "WithWarn") and leave the existing assertions unchanged.
🤖 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.
Nitpick comments:
In `@policies/backend-jwt/backendjwt_test.go`:
- Around line 1030-1059: The test
TestCustomClaims_RestrictedClaimsOverrideWithWarn either needs to assert the
expected warning is emitted or be renamed to remove "WithWarn"; to fix, decide
which is intended: if a warning should be logged when restricted claims ("iss",
"sub") are overridden, update the test to capture the policy logger (or inject a
test logger) around the call to p.OnRequestHeaders and assert the logger
received a warning message mentioning the restricted claim override after
calling p.OnRequestHeaders (referencing the BackendJWTPolicy instance p and the
OnRequestHeaders call); otherwise rename the test to
TestCustomClaims_RestrictedClaimsOverride (remove "WithWarn") and leave the
existing assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6418e044-9af5-41b0-b40a-167a5d53c855
📒 Files selected for processing (4)
docs/backend-jwt/v1.0/docs/backend-jwt.mdpolicies/backend-jwt/backendjwt.gopolicies/backend-jwt/backendjwt_test.gopolicies/backend-jwt/policy-definition.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/backend-jwt/v1.0/docs/backend-jwt.md
- policies/backend-jwt/backendjwt.go
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
1 participant
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.
Related to wso2/api-platform#2131
This pull request introduces a new "Backend JWT" policy, which enables the gateway to generate and inject a signed JWT containing authenticated user information into upstream request headers. This policy is designed to work after authentication policies, allowing backend services to verify caller identity using the gateway's public key, without needing access to the original client credentials. The PR includes comprehensive documentation, configuration metadata, and policy definition files for seamless integration.
Backend JWT Policy Introduction and Documentation:
docs/README.md, describing its purpose and usage.docs/backend-jwt/v1.0/docs/backend-jwt.md, covering how the policy works, claim handling, configuration options, caching behavior, dynamic context claims, and usage examples.docs/backend-jwt/v1.0/metadata.jsonwith name, version, provider, categories, and a concise description.Policy Definition and Configuration:
policies/backend-jwt/policy-definition.yaml, specifying user and system parameters (e.g., header name, claim mappings, custom claims, signing key, algorithm, issuer, expiry, caching), including validation rules and documentation for each field.policies/backend-jwt/go.mod, listing dependencies and a local SDK replacement for development.