[ES-1944G] Added JWT expire time as TTL for jti cache#2018
[ES-1944G] Added JWT expire time as TTL for jti cache#2018KashiwalHarsh wants to merge 12 commits into
Conversation
Signed-off-by: Harsh Kashiwal <harsh.kashiwal@infosys.com>
|
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:
WalkthroughJTI replay protection now uses explicit TTL values throughout the cache, token, and DPoP paths. Configuration adds client-assertion JTI TTL controls, ChangesTTL-aware JTI replay protection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java (1)
240-243: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCache TTL ignores the JWT validation clock-skew leeway; a replay window opens if
skew-buffer-seconds<leeway-seconds.The decoder (Line 201/292) accepts an assertion until
now <= exp + maxClockSkew, but the cache entry created here only lives untilexp + jtiCacheSkewBufferSeconds(TTL =(exp - now) + skewBuffer, set atnow). With the defaults (skewBuffer=30,maxClockSkew=5) this is safe, but the two values are independently configurable, so raisingmosip.esignet.client-assertion-jwt.leeway-secondsaboveskew-buffer-secondsleaves a window where the assertion still validates after the JTI entry has expired — i.e. a replayable token.Couple the buffer to the validation leeway so coverage is guaranteed regardless of config.
🛡️ Proposed fix
long jtiTtlSeconds = Math.min( - (exp - now) + jtiCacheSkewBufferSeconds, + (exp - now) + Math.max(jtiCacheSkewBufferSeconds, maxClockSkew), maxJtiCacheTtlSeconds );🤖 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 `@oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java` around lines 240 - 243, The cache TTL calculation for JTI entries uses jtiCacheSkewBufferSeconds, but the JWT decoder accepts tokens until now <= exp + maxClockSkew (the validation leeway). When the validation leeway is larger than the cache skew buffer, a replay vulnerability exists. Fix the TTL calculation in the jtiTtlSeconds assignment to use the actual maxClockSkew value (the validation leeway retrieved from the JWT decoder configuration) instead of jtiCacheSkewBufferSeconds, or ensure the buffer is at least as large as the leeway by using Math.max between the two values. This ensures cache entries remain valid for at least as long as the JWT validation allows the token to be accepted.oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java (1)
420-446: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest does not exercise the branch it claims.
The comment states this validates rejection via the derived-TTL (
jtiTtlSeconds <= 0) path, but withexp = now - 60sandmaxClockSkew = 5,jwtDecoder.decode()rejects the assertion first (60s past >> 5s leeway). The assertion never reachesenforceJtiReplayProtection's expiry guard, so the test passes for the wrong reason. To genuinely target that branch you'd need a token that survives decode yet yields a non-positive TTL — which, per the production-file note, is currently unreachable. Consider assertingcheckAndMarkJtiis never invoked here, or adjusting clock-skew to reflect intent.🤖 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 `@oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java` around lines 420 - 446, The test verifyClientAssertion_expiredBeyondSkew_thenFail has timestamps that cause jwtDecoder.decode() to reject the token before it reaches the enforceJtiReplayProtection method, so the test exercises the wrong rejection path despite the comment claiming otherwise. Either adjust the expiration and issue times to be within the maxClockSkew threshold so the token passes decode but fails the jtiTtlSeconds check in enforceJtiReplayProtection, or add a verification (mock assertion or capture) to confirm checkAndMarkJti is never invoked, demonstrating the actual early rejection by jwtDecoder.
🤖 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.
Outside diff comments:
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java`:
- Around line 240-243: The cache TTL calculation for JTI entries uses
jtiCacheSkewBufferSeconds, but the JWT decoder accepts tokens until now <= exp +
maxClockSkew (the validation leeway). When the validation leeway is larger than
the cache skew buffer, a replay vulnerability exists. Fix the TTL calculation in
the jtiTtlSeconds assignment to use the actual maxClockSkew value (the
validation leeway retrieved from the JWT decoder configuration) instead of
jtiCacheSkewBufferSeconds, or ensure the buffer is at least as large as the
leeway by using Math.max between the two values. This ensures cache entries
remain valid for at least as long as the JWT validation allows the token to be
accepted.
In
`@oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java`:
- Around line 420-446: The test verifyClientAssertion_expiredBeyondSkew_thenFail
has timestamps that cause jwtDecoder.decode() to reject the token before it
reaches the enforceJtiReplayProtection method, so the test exercises the wrong
rejection path despite the comment claiming otherwise. Either adjust the
expiration and issue times to be within the maxClockSkew threshold so the token
passes decode but fails the jtiTtlSeconds check in enforceJtiReplayProtection,
or add a verification (mock assertion or capture) to confirm checkAndMarkJti is
never invoked, demonstrating the actual early rejection by jwtDecoder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e01c3ecc-2d2f-4ec5-bfba-9e249ce6a8ad
📒 Files selected for processing (3)
oidc-service-impl/src/main/java/io/mosip/esignet/services/CacheUtilService.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java
💤 Files with no reviewable changes (1)
- oidc-service-impl/src/main/java/io/mosip/esignet/services/CacheUtilService.java
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java (1)
420-447: 📐 Maintainability & Code Quality | 🟡 MinorUpdate test comment to reflect that
decode()performs expiration validation before JTI replay protection logic executes.The test comment describes a
jtiTtl = min(-60 + 30, 3600) = -30calculation, but this code path is never reached. WithmaxClockSkew = 5and the token expired 60 seconds,jwtDecoder.decode()(line 202) rejects the token as expired beyond the tolerance beforeenforceJtiReplayProtectionis invoked (line 204). TheINVALID_CLIENTexception andnever().checkAndMarkJti(...)assertion pass due to thedecode()rejection, not the JTI TTL logic. Rewrite the comment to clarify this execution flow.🤖 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 `@oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java` around lines 420 - 447, Update the comment in the verifyClientAssertion_expiredBeyondSkew_thenFail test method to accurately reflect that jwtDecoder.decode() performs expiration validation before JTI replay protection logic. The current comment describes a jtiTtl calculation path that is never reached because with maxClockSkew set to 5 seconds and the token expired 60 seconds, the decode() method rejects the token as expired before enforceJtiReplayProtection is invoked, which is why the INVALID_CLIENT exception is thrown and checkAndMarkJti is never called. Rewrite the comment to clarify this actual execution flow instead of describing unreachable code paths.oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java (2)
232-238: 🩺 Stability & Availability | 🟠 MajorAssertion lifetime cap enforces security boundary; confirm intentionality with downstream clients.
The hard rejection of assertions with
declaredLifetime > maxJtiCacheTtlSecondsis an intentional security design to ensure the JTI replay cache always outlives assertion validity, closing potential replay windows. However, this is a breaking change: any client issuing assertions valid for longer than the default 3600 seconds will now fail withINVALID_CLIENT.RFC 7523 explicitly permits servers to reject assertions with "unreasonably far in future" expiration times, and security best practice recommends client assertion lifetimes of 1–5 minutes. The 3600-second default is already at the generous end of this spectrum.
If longer assertion lifetimes are required, the
mosip.esignet.client-assertion.jti.cache.max-ttl-secondsproperty can be increased to match or exceed the declared assertion lifetime. Verify with consuming clients whether this validation is acceptable or if assertion lifetime expectations need adjustment.🤖 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 `@oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java` around lines 232 - 238, The validation logic in TokenServiceImpl that rejects client assertions with declaredLifetime exceeding maxJtiCacheTtlSeconds is an intentional security control to prevent replay attacks, but it represents a breaking change for clients using longer assertion lifetimes. Verify with consuming clients whether their current assertion lifetime expectations align with the default maxJtiCacheTtlSeconds constraint (3600 seconds), and communicate that any assertions declaring lifetimes longer than this threshold will be rejected with INVALID_CLIENT. If downstream clients require longer assertion lifetimes, they can be directed to request an increase to the mosip.esignet.client-assertion.jti.cache.max-ttl-seconds configuration property to accommodate their needs, or they should adjust their client assertion lifetime to comply with the security boundary enforced by this validation.
240-248: 🔒 Security & Privacy | 🟠 MajorCache TTL can expire before assertion acceptance window closes, creating a replay vulnerability.
The decoder accepts assertions until
exp + maxClockSkew(viaJwtTimestampValidatorat line 202), but the JTI cache TTL is capped atmaxJtiCacheTtlSeconds(3600s by default). When an assertion withdeclaredLifetimenear or at the cap is fresh (now ≈ iat), the TTL calculation:jtiTtlSeconds = min((exp - now) + max(jtiCacheSkewBufferSeconds, maxClockSkew), maxJtiCacheTtlSeconds)collapses to
maxJtiCacheTtlSeconds, causing the cache entry to expire atnow + 3600while the token remains valid untilexp + maxClockSkew = (now + declaredLifetime) + 5seconds. This leaves a replay window of up tojtiCacheSkewBufferSeconds(ormaxClockSkewif smaller) seconds between cache expiry and acceptance window closure, contradicting the line 232 invariant.Adjust the TTL cap to include the skew: change
maxJtiCacheTtlSecondstomaxJtiCacheTtlSeconds + Math.max(jtiCacheSkewBufferSeconds, maxClockSkew)in theMath.min().Separately, the
jtiTtlSeconds <= 0guard (lines 245–248) is unreachable: the condition requiresnow ≥ exp + 30, but the upstreamjwtDecoder.decode()at line 202 already rejects ifnow ≥ exp + maxClockSkew(5s by default), making this branch dead code.🤖 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 `@oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java` around lines 240 - 248, The JTI cache TTL calculation in the Math.min() call for jtiTtlSeconds has a replay vulnerability where the cache expires before the token acceptance window closes. Adjust the first argument to Math.min() by adding Math.max(jtiCacheSkewBufferSeconds, maxClockSkew) to the maxJtiCacheTtlSeconds cap so the cache TTL accounts for the acceptance window skew. Additionally, the jtiTtlSeconds <= 0 guard block is unreachable dead code because upstream jwtDecoder.decode() validation already rejects expired tokens before this point, so this entire if block checking for expired client assertions can be removed.
🤖 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.
Outside diff comments:
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java`:
- Around line 232-238: The validation logic in TokenServiceImpl that rejects
client assertions with declaredLifetime exceeding maxJtiCacheTtlSeconds is an
intentional security control to prevent replay attacks, but it represents a
breaking change for clients using longer assertion lifetimes. Verify with
consuming clients whether their current assertion lifetime expectations align
with the default maxJtiCacheTtlSeconds constraint (3600 seconds), and
communicate that any assertions declaring lifetimes longer than this threshold
will be rejected with INVALID_CLIENT. If downstream clients require longer
assertion lifetimes, they can be directed to request an increase to the
mosip.esignet.client-assertion.jti.cache.max-ttl-seconds configuration property
to accommodate their needs, or they should adjust their client assertion
lifetime to comply with the security boundary enforced by this validation.
- Around line 240-248: The JTI cache TTL calculation in the Math.min() call for
jtiTtlSeconds has a replay vulnerability where the cache expires before the
token acceptance window closes. Adjust the first argument to Math.min() by
adding Math.max(jtiCacheSkewBufferSeconds, maxClockSkew) to the
maxJtiCacheTtlSeconds cap so the cache TTL accounts for the acceptance window
skew. Additionally, the jtiTtlSeconds <= 0 guard block is unreachable dead code
because upstream jwtDecoder.decode() validation already rejects expired tokens
before this point, so this entire if block checking for expired client
assertions can be removed.
In
`@oidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java`:
- Around line 420-447: Update the comment in the
verifyClientAssertion_expiredBeyondSkew_thenFail test method to accurately
reflect that jwtDecoder.decode() performs expiration validation before JTI
replay protection logic. The current comment describes a jtiTtl calculation path
that is never reached because with maxClockSkew set to 5 seconds and the token
expired 60 seconds, the decode() method rejects the token as expired before
enforceJtiReplayProtection is invoked, which is why the INVALID_CLIENT exception
is thrown and checkAndMarkJti is never called. Rewrite the comment to clarify
this actual execution flow instead of describing unreachable code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e680e279-7c56-4bf3-8abd-91f4d61db55b
📒 Files selected for processing (2)
oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/TokenServiceTest.java
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Signed-off-by: Harsh Kashiwal <harsh.kashiwal@infosys.com>
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Signed-off-by: Harsh Kashiwal <kashiwalharsh1234@gmail.com>
Merge branch 'ES-1944G' of https://github.com/Infosys/esignet into ES-1944G
Signed-off-by: Harsh Kashiwal <harsh.kashiwal@infosys.com>
3 participants
Summary by CodeRabbit