Fix integer overflow in OIDC re-auth max_age checks - #52499
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The overflow fix is focused, covered by tests, and has no unresolved issues.
Pull request overview
Fixes integer overflow in OIDC re-authentication expiry calculations.
Changes:
- Uses
longfor timestamp-plus-age calculations. - Adds regression and baseline tests for affected authentication paths.
File summaries
| File | Description |
|---|---|
services/src/test/java/org/keycloak/protocol/oidc/OIDCLoginProtocolTest.java |
Tests large, normal, and absent max-age behavior. |
services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java |
Prevents overflow in re-authentication expiry comparisons. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mposolda
left a comment
There was a problem hiding this comment.
@groie Thanks!
The fix is good, but can you please add proper integration test instead of the test in the services module (which introduces lots of "mock" objects, which is not ideal)? Possibly new method in the OIDCAdvancedRequestParamsTest (there are already existing tests for maxAge and IMO, it is sufficient to add the itnegration test for the max_age supplied as a parameter)
|
Thanks for the Headsup @mposolda, I wasn't quite happy with the verbosity of the test myself. I'll follow your suggestion and see where the journey takes me. |
8ad2e8f to
dda4a19
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Add regression coverage for the required-action max_auth_age overflow path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
dda4a19 to
3d7d2d5
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The brokered OIDC passMaxAge path still has the same overflow and lacks regression coverage.
Review details
Suppressed comments (1)
services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java:554
- The same client-supplied
max_agestill overflows for brokered OIDC logins whenpassMaxAgeis enabled:OIDCIdentityProvider.java:548addsauthTimeInt + maxAgeIntasintand then rejects the IdP response. Please widen that comparison as well and cover the existingKcOidcBrokerPassMaxAgeTestpath so this fix applies consistently.
if ((long) authTimeInt + maxAgeInt < Time.currentTime()) {
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
3d7d2d5 to
cf329b0
Compare
isAuthTimeExpired() and isReAuthRequiredForKcAction() in OIDCLoginProtocol, and isAuthTimeExpired() in the OIDC identity broker (OIDCIdentityProvider, used when passMaxAge is enabled) summed authTime and maxAge as int before comparing against the current time. A very large max_age (e.g. Integer.MAX_VALUE, reachable via the client-supplied max_age request parameter, via a required action's configured max auth age, or via the passMaxAge broker config) overflows the sum to a negative number, making the comparison incorrectly report the authentication as expired and force an unwanted re-authentication or reject an otherwise valid brokered login. Widen the sum to long in all three call sites to avoid the overflow. Closes keycloak#52491 Signed-off-by: Ilkka <ilkka.harmanen@gmail.com>
cf329b0 to
86e81f7
Compare
isAuthTimeExpired() and isReAuthRequiredForKcAction() in OIDCLoginProtocol summed authTime and maxAge as int before comparing against the current time. A very large max_age (e.g. Integer.MAX_VALUE, reachable via the client-supplied max_age request parameter, or via a required action's configured max auth age) overflows the sum to a negative number, making the comparison incorrectly report the authentication as expired and force an unwanted re-authentication. Widen the sum to long to avoid the overflow.
Closes #52491