KAFAK-14604: SASL session expiration time will be overflowed when calculation #16
Conversation
…culation Signed-off-by: PoAn Yang <[email protected]>
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses a potential overflow issue in the calculation of SASL session expiration time. The issue arises when a large session lifetime is configured, potentially leading to an ArithmeticException. To resolve this, I've implemented a change that uses Math.addExact and Utils.msToNs to perform the calculation, which throws an exception if an overflow occurs. Additionally, I've added a test case to verify the fix and ensure that the channel closes when a large re-authentication value is used.
Highlights
- Overflow Prevention: The core change involves replacing the direct multiplication and addition with
Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse))to prevent potential overflow when calculatingclientSessionReauthenticationTimeNanosinSaslClientAuthenticator.javaandsessionExpirationTimeNanosinSaslServerAuthenticator.java. - Utility Function: A new utility function
msToNsis introduced inUtils.javato convert milliseconds to nanoseconds, throwing anIllegalArgumentExceptionif an arithmetic overflow occurs during the conversion. - Test Case: A new test case
testReauthenticateWithLargeReauthValueis added inSaslAuthenticatorTest.javato simulate the overflow scenario and verify that the connection channel is closed as expected.
Changelog
Click here to see the changelog
- clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java
- Replaced direct calculation of
clientSessionReauthenticationTimeNanoswithMath.addExactandUtils.msToNsto prevent overflow.
- Replaced direct calculation of
- clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java
- Replaced direct calculation of
sessionExpirationTimeNanoswithMath.addExactandUtils.msToNsto prevent overflow.
- Replaced direct calculation of
- clients/src/main/java/org/apache/kafka/common/utils/Utils.java
- Added
msToNsutility function to convert milliseconds to nanoseconds with overflow check.
- Added
- clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java
- Added
needLargeExpirationflag to control the lifetime of the token. - Added
testReauthenticateWithLargeReauthValuetest case to verify overflow handling during re-authentication with a large re-authentication value. - Modified
createClientConnectionto usecreateCustomClientConnection.
- Added
- clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java
- Added
testSessionWontExpireWithLargeExpirationTimetest case to verify the overflow exception is thrown when a large expiration time is configured.
- Added
- clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
- Added
testMsToNstest case to verify themsToNsutility function.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In nanoseconds' dance,
Overflow's a risky chance,
Guard with Math's embrace,
Exceptions find their place,
Code's safety, we enhance.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request addresses a potential overflow issue in the calculation of SASL session expiration time. The fix involves using Math.addExact and Utils.msToNs to prevent overflow and throwing an exception if overflow occurs. Additionally, a test case is added to verify the fix. Overall, the changes seem reasonable and well-structured.
Summary of Findings
- Exception handling: The code throws an
IllegalArgumentExceptionwhen an arithmetic overflow occurs during the conversion of milliseconds to nanoseconds. While this addresses the immediate issue, consider whether a more specific exception type might be appropriate to provide better context to callers. - Test coverage: The added test case
testReauthenticateWithLargeReauthValueinSaslAuthenticatorTest.javaeffectively tests the overflow scenario. However, consider adding additional test cases with different large values to ensure robustness.
Merge Readiness
The pull request addresses a critical issue and includes a test case to verify the fix. However, before merging, it's recommended to consider the exception type and add more test cases for robustness. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); | ||
|
|
||
| sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; | ||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
There was a problem hiding this comment.
Similar to the change in SaslClientAuthenticator.java, this change uses Math.addExact and Utils.msToNs to prevent potential overflow. This is consistent and correct.
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
| public static long msToNs(long timeMs) { | ||
| try { | ||
| return Math.multiplyExact(1000 * 1000, timeMs); | ||
| } catch (ArithmeticException e) { | ||
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | ||
| } |
There was a problem hiding this comment.
The msToNs method converts milliseconds to nanoseconds and throws an IllegalArgumentException if an arithmetic overflow occurs. Consider using a more specific exception type, such as ArithmeticException, to provide better context to callers.
| public static long msToNs(long timeMs) { | |
| try { | |
| return Math.multiplyExact(1000 * 1000, timeMs); | |
| } catch (ArithmeticException e) { | |
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | |
| } | |
| public static long msToNs(long timeMs) { | |
| try { | |
| return Math.multiplyExact(1000 * 1000, timeMs); | |
| } catch (ArithmeticException e) { | |
| throw e; // re-throw the ArithmeticException | |
| } | |
| } |
| public void testMsToNs() { | ||
| assertEquals(1000000, Utils.msToNs(1)); | ||
| assertEquals(0, Utils.msToNs(0)); | ||
| assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE)); |
There was a problem hiding this comment.
This test case verifies that an IllegalArgumentException is thrown when msToNs is called with Long.MAX_VALUE. Consider adding additional test cases with different large values to ensure robustness.
assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE));
assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE - 1));
assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE / 2));
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSwitches session-expiration arithmetic to overflow-checked operations via Utils.msToNs and Math.addExact, adds ms-to-ns helper, and expands tests to cover extremely large expiration values and related client/server test helpers and assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Authenticator
participant Utils
participant Logger
note over Authenticator,Utils #e6f7ff: Compute session expiration (new flow)
Authenticator->>Utils: msToNs(sessionLifetimeMs)
Utils-->>Authenticator: nanos (Math.multiplyExact) or throw ArithmeticException
Authenticator->>Authenticator: Math.addExact(authenticationEndNanos, nanos)
alt no overflow
Authenticator->>Authenticator: set sessionExpirationTimeNanos
else multiply or add overflow
Authenticator->>Logger: warn about overflow
Authenticator->>Authenticator: set sessionExpirationTimeNanos = Long.MAX_VALUE
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Overflow Protection in Authentication Session Calculations👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | ||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | ||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | ||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
There was a problem hiding this comment.
Arithmetic Overflow Vulnerability
Direct multiplication without overflow checking can cause arithmetic overflow when large session lifetime values are used. This could lead to incorrect re-authentication timing calculations, potentially causing premature session expiration or authentication failures.
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
Standards
- CWE-190
- OWASP-A04
- NIST-SSDF-PW.1
| retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); | ||
|
|
||
| sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; | ||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
There was a problem hiding this comment.
Authentication Timing Overflow
Server-side authentication expiration calculation is vulnerable to arithmetic overflow with large session lifetime values. This could cause incorrect session expiration timing, potentially leading to authentication failures or premature session termination.
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
Standards
- CWE-190
- OWASP-A04
- NIST-SSDF-PW.1
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | ||
| } |
There was a problem hiding this comment.
Incorrect Error Message
The error message uses plural 'milliseconds' but singular 'nanosecond'. This inconsistency in plurality creates a grammatical error. The message should use either both singular or both plural forms for consistency.
Standards
- Logic-Verification-Error-Handling
- Business-Rule-Message-Consistency
| assertEquals(1000000, Utils.msToNs(1)); | ||
| assertEquals(0, Utils.msToNs(0)); | ||
| assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE)); |
There was a problem hiding this comment.
Inconsistent Test Values
The test verifies positive and zero cases but lacks testing for negative values. Since the msToNs method doesn't explicitly prohibit negative values, the test should verify correct handling of negative millisecond inputs to ensure complete logical coverage.
Standards
- Logic-Verification-Test-Coverage
- Algorithm-Correctness-Edge-Cases
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | ||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | ||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | ||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
There was a problem hiding this comment.
Code Duplication
The same overflow handling pattern is duplicated in SaslClientAuthenticator and SaslServerAuthenticator. Consider extracting a common helper method that both classes can use for session expiration time calculation to improve maintainability.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Duplication
- Design-Pattern-Extract-Method
| assertEquals(1000000, Utils.msToNs(1)); | ||
| assertEquals(0, Utils.msToNs(0)); | ||
| assertThrows(IllegalArgumentException.class, () -> Utils.msToNs(Long.MAX_VALUE)); |
There was a problem hiding this comment.
Boundary Value Testing
Test only checks extreme values (0, 1, Long.MAX_VALUE) but misses important boundary cases. Adding tests for values near overflow threshold (Long.MAX_VALUE/1_000_000) would better verify the reliability of the msToNs method under edge conditions.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Testing-Reliability
| TestUtils.waitForCondition(() -> { | ||
| selector.poll(1000); | ||
| // this channel should be closed due to session timeout calculation overflow | ||
| return selector.channel(node) == null; | ||
| }, "channel didn't close with large re-authentication value"); |
There was a problem hiding this comment.
Potential Exception Overhead
The test waits for an exception to occur inside the poll operation, which may cause performance overhead in test execution. Exception-based control flow can introduce latency due to stack trace generation and exception handling machinery.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Exception-Handling
- Performance-Testing-Efficiency
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Authentication Session Overflow Protection👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); | ||
|
|
||
| sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; | ||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
There was a problem hiding this comment.
Authentication Session Calculation
Direct arithmetic calculation of session expiration time is vulnerable to integer overflow when large values (like Long.MAX_VALUE) are used. This could cause premature session expiration or authentication failures. The fix properly handles this by using Math.addExact and a new Utils.msToNs method with overflow detection.
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
Standards
- CWE-190
- OWASP-A07
- NIST-SSDF-PW.1
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | ||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | ||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | ||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
There was a problem hiding this comment.
Client Authentication Overflow
Client-side session re-authentication time calculation is vulnerable to arithmetic overflow when large session lifetimes are used. This could lead to premature session expiration or authentication failures. The fix properly handles this by using Math.addExact and a new Utils.msToNs method with overflow detection.
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
Standards
- CWE-190
- OWASP-A07
- NIST-SSDF-PW.1
| TestUtils.waitForCondition(() -> { | ||
| selector.poll(1000); | ||
| // this channel should be closed due to session timeout calculation overflow | ||
| return selector.channel(node) == null; | ||
| }, "channel didn't close with large re-authentication value"); |
There was a problem hiding this comment.
Exception Handling Missing
TestUtils.waitForCondition lacks exception handling for timeout scenarios. If the channel doesn't close as expected, the test will fail with a timeout exception without clear diagnostic information, making troubleshooting difficult.
| TestUtils.waitForCondition(() -> { | |
| selector.poll(1000); | |
| // this channel should be closed due to session timeout calculation overflow | |
| return selector.channel(node) == null; | |
| }, "channel didn't close with large re-authentication value"); | |
| try { | |
| TestUtils.waitForCondition(() -> { | |
| selector.poll(1000); | |
| // this channel should be closed due to session timeout calculation overflow | |
| return selector.channel(node) == null; | |
| }, "channel didn't close with large re-authentication value"); | |
| } catch (InterruptedException e) { | |
| throw new AssertionError("Test interrupted while waiting for channel to close: " + e.getMessage(), e); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| */ | ||
| public static long msToNs(long timeMs) { | ||
| try { | ||
| return Math.multiplyExact(1000 * 1000, timeMs); |
There was a problem hiding this comment.
Potential Integer Overflow
The multiplication 1000 * 1000 is performed first before multiplying with timeMs, which could lead to integer overflow for large timeMs values. Reordering to Math.multiplyExact(timeMs, 1000 * 1000) would be safer to catch overflow earlier.
| return Math.multiplyExact(1000 * 1000, timeMs); | |
| return Math.multiplyExact(timeMs, 1000L * 1000L); |
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Precondition
| TestUtils.waitForCondition(() -> { | ||
| selector.poll(1000); | ||
| // this channel should be closed due to session timeout calculation overflow | ||
| return selector.channel(node) == null; | ||
| }, "channel didn't close with large re-authentication value"); |
There was a problem hiding this comment.
Missing Error Handling
The test asserts channel closure but doesn't verify the specific error condition. Without checking logs or exception type, it cannot confirm whether the channel closed due to the expected overflow error or some other unrelated issue.
Standards
- Algorithm-Correctness-Exception-Handling
- Logic-Verification-Test-Validation
- Business-Rule-Error-Verification
| assertEquals(ArithmeticException.class, t.getCause().getClass()); | ||
| assertEquals("Cannot convert " + Long.MAX_VALUE + " millisecond to nanosecond due to arithmetic overflow", |
There was a problem hiding this comment.
Inconsistent Error Message
The test expects a specific error message that doesn't match the actual implementation in Utils.msToNs(). The test checks for "arithmetic overflow" while the implementation throws "due to arithmetic overflow", which will cause the test to fail.
Standards
- Logic-Verification-Test-Correctness
- Algorithm-Correctness-Error-Messaging
- Business-Rule-Consistency
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | ||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | ||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | ||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
There was a problem hiding this comment.
Method Extraction Opportunity
The calculation of session expiration time appears in both SaslClientAuthenticator and SaslServerAuthenticator classes with identical logic. This duplication creates maintenance burden where changes must be made in multiple places consistently.
Standards
- Clean-Code-DRY
- Refactoring-Extract-Method
- Maintainability-Quality-Duplication
| SaslChannelBuilder clientChannelBuilder; | ||
| if (!withSaslAuthenticateHeader) { | ||
| clientChannelBuilder = saslChannelBuilderWithoutHeader(securityProtocol, saslMechanism, jaasContexts, listenerName); | ||
| } else { | ||
| clientChannelBuilder = new SaslChannelBuilder(ConnectionMode.CLIENT, jaasContexts, | ||
| securityProtocol, listenerName, false, saslMechanism, | ||
| null, null, null, time, new LogContext(), null) { | ||
|
|
||
| @Override | ||
| protected SaslClientAuthenticator buildClientAuthenticator(Map<String, ?> configs, | ||
| AuthenticateCallbackHandler callbackHandler, | ||
| String id, | ||
| String serverHost, | ||
| String servicePrincipal, | ||
| TransportLayer transportLayer, | ||
| Subject subject) { | ||
|
|
||
| return new SaslClientAuthenticator(configs, callbackHandler, id, subject, | ||
| servicePrincipal, serverHost, saslMechanism, transportLayer, time, new LogContext()); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Extract Common Method
The SaslChannelBuilder creation with header could be extracted into a separate method similar to saslChannelBuilderWithoutHeader. This would improve code symmetry, reduce duplication, and make the createCustomClientConnection method more concise and easier to maintain.
Standards
- Clean-Code-Functions
- Refactoring-Extract-Method
- Maintainability-Quality-Symmetry
| try { | ||
| return Math.multiplyExact(1000 * 1000, timeMs); | ||
| } catch (ArithmeticException e) { | ||
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); |
There was a problem hiding this comment.
Defensive Boundary Check
Adding a boundary check before the multiplication could prevent the exception entirely for known large values. For values exceeding Long.MAX_VALUE/1_000_000, an early check would be more efficient than throwing and catching exceptions, which have performance overhead.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Early-Validation
- Exception-Handling-Performance-Impact
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Authentication Session Overflow Protection👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| @@ -690,7 +690,7 @@ public void setAuthenticationEndAndSessionReauthenticationTimes(long nowNanos) { | |||
| double pctToUse = pctWindowFactorToTakeNetworkLatencyAndClockDriftIntoAccount + RNG.nextDouble() | |||
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | |||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | |||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | |||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |||
There was a problem hiding this comment.
Authentication Session Overflow
Direct arithmetic calculation of session expiration time can cause integer overflow with large millisecond values. This could lead to premature session expiration or authentication failures when using long-lived tokens, potentially causing service disruption or authentication bypass.
clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse));
Commitable Suggestion
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); |
Standards
- CWE-190
- OWASP-A02
- NIST-SSDF-PW.1
| @@ -681,7 +681,7 @@ else if (!maxReauthSet) | |||
| else | |||
| retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); | |||
|
|
|||
| sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; | |||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |||
There was a problem hiding this comment.
Server Authentication Overflow
Direct calculation of session expiration time without overflow protection can cause arithmetic overflow with large values. This could lead to incorrect session expiration times, potentially causing premature disconnection of authenticated clients or authentication state inconsistency.
sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs));
Commitable Suggestion
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); |
Standards
- CWE-190
- OWASP-A02
- NIST-SSDF-PW.1
| @@ -690,7 +690,7 @@ public void setAuthenticationEndAndSessionReauthenticationTimes(long nowNanos) { | |||
| double pctToUse = pctWindowFactorToTakeNetworkLatencyAndClockDriftIntoAccount + RNG.nextDouble() | |||
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | |||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | |||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | |||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |||
There was a problem hiding this comment.
Missing Exception Handling
The code now correctly uses Math.addExact and Utils.msToNs to detect overflow, but doesn't handle the potential IllegalArgumentException that Utils.msToNs can throw. If sessionLifetimeMsToUse is too large, the exception will propagate up the call stack without proper handling.
Standards
- Algorithm-Correctness-Exception-Handling
- Logic-Verification-Error-Paths
- Business-Rule-Robustness
| @@ -681,7 +681,7 @@ else if (!maxReauthSet) | |||
| else | |||
| retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); | |||
|
|
|||
| sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; | |||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |||
There was a problem hiding this comment.
Inconsistent Error Handling
The error handling approach differs between client and server authenticators. SaslServerAuthenticator directly propagates the IllegalArgumentException from Utils.msToNs() while SaslClientAuthenticator catches and handles it in the test. This inconsistency makes maintenance and debugging more difficult.
Standards
- Clean-Code-Error-Handling
- Maintainability-Quality-Consistency
| public static long msToNs(long timeMs) { | ||
| try { | ||
| return Math.multiplyExact(1000 * 1000, timeMs); | ||
| } catch (ArithmeticException e) { | ||
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Exception Handling Overhead
Using exception handling for control flow can introduce performance overhead. The try-catch block creates additional stack frames and exception objects when overflow occurs. For frequently called utility methods, consider using a bounds check before multiplication instead of relying on exceptions.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Exception-Avoidance
- Algorithmic-Complexity-Constant-Time
Co-authored-by: refacto-visz[bot] <230139763+refacto-visz[bot]@users.noreply.github.com>
|
/refacto-visz |
1 similar comment
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java(1 hunks)clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java(1 hunks)clients/src/main/java/org/apache/kafka/common/utils/Utils.java(1 hunks)clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java(7 hunks)clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java(1 hunks)clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java (1)
clients/src/main/java/org/apache/kafka/common/utils/Utils.java (1)
Utils(93-1716)
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java (1)
clients/src/main/java/org/apache/kafka/common/utils/Utils.java (1)
Utils(93-1716)
clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java (1)
clients/src/main/java/org/apache/kafka/common/utils/Utils.java (1)
Utils(93-1716)
clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java (3)
clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java (1)
SaslConfigs(21-230)clients/src/test/java/org/apache/kafka/test/TestUtils.java (1)
TestUtils(84-708)clients/src/main/java/org/apache/kafka/common/security/JaasContext.java (1)
JaasContext(39-217)
🔇 Additional comments (7)
clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java (1)
1112-1117: Good coverage on the new converter.The test hits both normal and overflow paths, so once the converter compiles, this will catch regressions. Nice.
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java (1)
693-695: Overflow-safe addition looks good.Using
Math.addExactwith the sharedUtils.msToNshelper is the right fix here.clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java (1)
684-685: Server-side overflow handling aligned.Mirroring the client change keeps the session expiration math consistent and safe.
clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java (1)
272-298: Nice guardrail for extreme expirations.The test validates both the wrapped exception type and message when the conversion overflows, so we’ll notice if the helper regresses.
clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java (3)
2065-2098: LGTM! Good refactoring to extract helper method.The extraction of
saslChannelBuilderWithoutHeader()improves code maintainability by consolidating the logic for creating channel builders that don't use SASL_AUTHENTICATE headers. The implementation follows existing patterns and makes the codebase more DRY.
2100-2138: LGTM! Well-designed helper method.The
createCustomClientConnection()method effectively consolidates client connection setup with configurable SASL_AUTHENTICATE header behavior. The parameterized design makes the intent clear at call sites and reduces code duplication across tests.
2582-2586: LGTM! Appropriate use of conditional lifetime.The conditional token lifetime logic correctly supports both the overflow test scenario (Long.MAX_VALUE) and normal test scenarios (1 second for quick refresh). The implementation is clean and the flag usage is appropriate for test control.
| public static long msToNs(long timeMs) { | ||
| try { | ||
| return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs); | ||
| } catch (ArithmeticException e) { | ||
| } catch (ArithmeticException e) { | ||
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | ||
| } |
There was a problem hiding this comment.
Fix duplicate catch causing compilation failure.
The method currently declares two identical catch (ArithmeticException e) blocks, leaving the first one empty. This does not compile (duplicate exception parameter). Collapse them into a single catch that wraps the overflow into the IllegalArgumentException.
public static long msToNs(long timeMs) {
try {
return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs);
- } catch (ArithmeticException e) {
- } catch (ArithmeticException e) {
- throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e);
- }
+ } catch (ArithmeticException e) {
+ throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static long msToNs(long timeMs) { | |
| try { | |
| return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs); | |
| } catch (ArithmeticException e) { | |
| } catch (ArithmeticException e) { | |
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | |
| } | |
| public static long msToNs(long timeMs) { | |
| try { | |
| return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs); | |
| } catch (ArithmeticException e) { | |
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In clients/src/main/java/org/apache/kafka/common/utils/Utils.java around lines
1708 to 1714, there are two identical catch blocks for ArithmeticException (one
empty and one that throws an IllegalArgumentException) which causes a
duplicate-catch compilation error; replace the two catches with a single catch
(ArithmeticException e) that throws new IllegalArgumentException("Cannot convert
" + timeMs + " millisecond to nanosecond due to arithmetic overflow", e) so the
overflow is wrapped and the code compiles.
Code Review: Authentication Session Timing Overflow Protection👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
| @@ -690,7 +690,7 @@ public void setAuthenticationEndAndSessionReauthenticationTimes(long nowNanos) { | |||
| double pctToUse = pctWindowFactorToTakeNetworkLatencyAndClockDriftIntoAccount + RNG.nextDouble() | |||
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | |||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | |||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | |||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |||
There was a problem hiding this comment.
Authentication Timing Overflow
Direct arithmetic multiplication of large millisecond values can cause integer overflow leading to incorrect session expiration timing. This could result in premature session expiration or authentication bypass when session lifetimes approach Long.MAX_VALUE. The fix properly uses Math.addExact and Utils.msToNs to detect overflow conditions.
try {
clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse));
} catch (IllegalArgumentException e) {
log.warn("Session lifetime too large, setting to maximum allowed value", e);
clientSessionReauthenticationTimeNanos = Long.MAX_VALUE;
}
Commitable Suggestion
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| try { | |
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| } catch (IllegalArgumentException e) { | |
| log.warn("Session lifetime too large, setting to maximum allowed value", e); | |
| clientSessionReauthenticationTimeNanos = Long.MAX_VALUE; | |
| } |
Standards
- CWE-190
- CWE-682
- OWASP-A07
- NIST-SSDF-PW.1
...ts/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java
Outdated
Show resolved
Hide resolved
| } catch (ArithmeticException e) { | ||
| } catch (ArithmeticException e) { |
There was a problem hiding this comment.
Duplicate Exception Catch
Duplicate catch block for ArithmeticException creates unreachable code. Second catch block will never execute because first catch handles same exception type. This violates control flow logic and creates dead code that could confuse maintainers.
Standards
- Algorithm-Correctness-Control-Flow
- Logic-Verification-Dead-Code-Elimination
- Business-Rule-Exception-Handling
| @@ -690,7 +690,7 @@ public void setAuthenticationEndAndSessionReauthenticationTimes(long nowNanos) { | |||
| double pctToUse = pctWindowFactorToTakeNetworkLatencyAndClockDriftIntoAccount + RNG.nextDouble() | |||
| * pctWindowJitterToAvoidReauthenticationStormAcrossManyChannelsSimultaneously; | |||
| sessionLifetimeMsToUse = (long) (positiveSessionLifetimeMs * pctToUse); | |||
| clientSessionReauthenticationTimeNanos = authenticationEndNanos + 1000 * 1000 * sessionLifetimeMsToUse; | |||
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |||
There was a problem hiding this comment.
Duplicate Code Pattern
Identical overflow protection pattern duplicated in both SaslClientAuthenticator and SaslServerAuthenticator classes. This violates DRY principle and creates maintenance burden where changes must be synchronized across multiple locations. Extract common session expiration calculation logic into shared utility method.
clientSessionReauthenticationTimeNanos = Utils.calculateExpirationTimeNanos(authenticationEndNanos, sessionLifetimeMsToUse, log);
Commitable Suggestion
| clientSessionReauthenticationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(sessionLifetimeMsToUse)); | |
| clientSessionReauthenticationTimeNanos = Utils.calculateExpirationTimeNanos(authenticationEndNanos, sessionLifetimeMsToUse, log); |
Standards
- Org-Guideline-Reuse code wherever possible and avoid redundant code by refactoring with utils and static method across application
- Clean-Code-DRY
- Refactoring-Extract-Method
| try { | ||
| return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs); | ||
| } catch (ArithmeticException e) { |
There was a problem hiding this comment.
Exception Control Flow
Exception handling for control flow creates performance overhead in hot authentication paths. Stack frame creation and exception object allocation add latency when processing large volumes of authentication requests, especially when timeMs approaches overflow thresholds.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Exception-Avoidance
- Exception-Handling-Performance
| TestUtils.waitForCondition(() -> { | ||
| selector.poll(1000); | ||
| // this channel should be closed due to session timeout calculation overflow | ||
| return selector.channel(node) == null; | ||
| }, "channel didn't close with large re-authentication value"); |
There was a problem hiding this comment.
Test Timeout Risk
TestUtils.waitForCondition lacks timeout exception handling for scenarios where channel doesn't close as expected. Test execution performance degrades when condition never satisfies, causing indefinite waiting rather than failing with clear timeout diagnostics.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Timeout-Handling
- Test-Performance-Reliability
| clientChannelBuilder = new SaslChannelBuilder(ConnectionMode.CLIENT, jaasContexts, | ||
| securityProtocol, listenerName, false, saslMechanism, | ||
| null, null, null, time, new LogContext(), null) { | ||
|
|
||
| @Override | ||
| protected SaslClientAuthenticator buildClientAuthenticator(Map<String, ?> configs, | ||
| AuthenticateCallbackHandler callbackHandler, | ||
| String id, | ||
| String serverHost, | ||
| String servicePrincipal, | ||
| TransportLayer transportLayer, | ||
| Subject subject) { | ||
|
|
||
| return new SaslClientAuthenticator(configs, callbackHandler, id, subject, | ||
| servicePrincipal, serverHost, saslMechanism, transportLayer, time, new LogContext()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Extract Common Method
SaslChannelBuilder creation with header duplicates similar logic to saslChannelBuilderWithoutHeader method. Extract common channel builder creation pattern into reusable helper method to improve code symmetry and reduce duplication. This aligns with organization guideline for code reuse.
Standards
- Org-Guideline-Reuse code wherever possible and avoid redundant code by refactoring with utils and static method across application
- Refactoring-Extract-Method
- Maintainability-Quality-Symmetry
|
/refacto-visz |
|
PR already reviewed at the latest commit: ffa68e2. |
…icator/SaslServerAuthenticator.java Co-authored-by: refacto-visz[bot] <230139763+refacto-visz[bot]@users.noreply.github.com>
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Authentication Overflow Protection👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
| } catch (ArithmeticException e) { | ||
| } catch (ArithmeticException e) { | ||
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); |
There was a problem hiding this comment.
Compilation Error Vulnerability
Duplicate catch blocks prevent compilation, breaking overflow protection in authentication timing calculations. This compilation failure disables the entire security fix for arithmetic overflow vulnerabilities. Critical system security depends on this utility function compiling correctly.
try {
return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs);
} catch (ArithmeticException e) {
throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e);
}
Commitable Suggestion
| } catch (ArithmeticException e) { | |
| } catch (ArithmeticException e) { | |
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | |
| try { | |
| return Math.multiplyExact(NANOS_PER_MILLISECOND, timeMs); | |
| } catch (ArithmeticException e) { | |
| throw new IllegalArgumentException("Cannot convert " + timeMs + " millisecond to nanosecond due to arithmetic overflow", e); | |
| } |
Standards
- CWE-754
- OWASP-A06
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java (1)
clients/src/main/java/org/apache/kafka/common/utils/Utils.java (1)
Utils(93-1716)
| try { | ||
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | ||
| } catch (IllegalArgumentException e) { | ||
| log.warn("Session lifetime too large, setting to maximum allowed value", e); | ||
| sessionExpirationTimeNanos = Long.MAX_VALUE; | ||
| } |
There was a problem hiding this comment.
Critical: Fix logger reference and complete exception handling.
Two critical issues in the overflow handling code:
- Compilation error: Line 687 references
log(lowercase) but the logger is defined asLOG(uppercase) at line 94. - Incomplete exception handling: The catch block only handles
IllegalArgumentExceptionfromUtils.msToNs, butMath.addExactthrowsArithmeticExceptionon addition overflow. If the addition overflows (even when the conversion doesn't), the exception will propagate uncaught.
Apply this diff to fix both issues:
try {
sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs));
- } catch (IllegalArgumentException e) {
- log.warn("Session lifetime too large, setting to maximum allowed value", e);
+ } catch (IllegalArgumentException | ArithmeticException e) {
+ LOG.warn("Session lifetime too large, setting to maximum allowed value", e);
sessionExpirationTimeNanos = Long.MAX_VALUE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| } catch (IllegalArgumentException e) { | |
| log.warn("Session lifetime too large, setting to maximum allowed value", e); | |
| sessionExpirationTimeNanos = Long.MAX_VALUE; | |
| } | |
| try { | |
| sessionExpirationTimeNanos = Math.addExact(authenticationEndNanos, Utils.msToNs(retvalSessionLifetimeMs)); | |
| } catch (IllegalArgumentException | ArithmeticException e) { | |
| LOG.warn("Session lifetime too large, setting to maximum allowed value", e); | |
| sessionExpirationTimeNanos = Long.MAX_VALUE; | |
| } |
🤖 Prompt for AI Agents
In
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java
around lines 684 to 689, fix the logger reference and complete exception
handling for overflow: replace the lowercase `log` with the proper `LOG`, and
catch both the conversion IllegalArgumentException and any ArithmeticException
from Math.addExact (either combine into a multi-catch IllegalArgumentException |
ArithmeticException or add a separate catch for ArithmeticException) so the
overflow is handled and sessionExpirationTimeNanos is set to Long.MAX_VALUE with
the exception logged via LOG.warn(..., e).
3 participants
The timeout value may be overflowed if users set a large expiration time.
Fixed it by throwing exception if the value is overflowed.
Committer Checklist (excluded from commit message)
Summary by CodeRabbit
Bug Fixes
Tests
Chores