showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r887769458
##########
build.gradle:
##########
@@ -1243,7 +1243,7 @@ project(':clients') {
testImplementation libs.bcpkix
testImplementation libs.junitJupiter
- testImplementation libs.mockitoCore
+ testImplementation libs.mockitoInline
Review Comment:
Could you explain why we need MockitoInline here? Any method only exists in
`mockitoInline`?
##########
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##########
@@ -781,14 +781,13 @@ public void testConnectDisconnectDuringInSinglePoll()
throws Exception {
when(kafkaChannel.selectionKey()).thenReturn(selectionKey);
when(selectionKey.channel()).thenReturn(SocketChannel.open());
when(selectionKey.readyOps()).thenReturn(SelectionKey.OP_CONNECT);
+ when(selectionKey.attachment()).thenReturn(kafkaChannel);
- selectionKey.attach(kafkaChannel);
Set<SelectionKey> selectionKeys = Utils.mkSet(selectionKey);
selector.pollSelectionKeys(selectionKeys, false, System.nanoTime());
assertFalse(selector.connected().contains(kafkaChannel.id()));
assertTrue(selector.disconnected().containsKey(kafkaChannel.id()));
- assertNull(selectionKey.attachment());
Review Comment:
Why we did this change? Did we change anything affect this test?
##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##########
@@ -679,10 +679,11 @@ private long
calcCompletionTimesAndReturnSessionLifetimeMs() {
else if (connectionsMaxReauthMs == null)
retvalSessionLifetimeMs =
zeroIfNegative(credentialExpirationMs - authenticationEndMs);
else
- retvalSessionLifetimeMs = zeroIfNegative(
- Math.min(credentialExpirationMs -
authenticationEndMs, connectionsMaxReauthMs));
+ retvalSessionLifetimeMs =
zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs,
connectionsMaxReauthMs));
- sessionExpirationTimeNanos = authenticationEndNanos + 1000 *
1000 * retvalSessionLifetimeMs;
+ if (connectionsMaxReauthMs != null) {
Review Comment:
OK, for @SamBarker , I think we can discuss the property change in a
separate thread since it's related to tests.
For this change:
```
if (connectionsMaxReauthMs != null) {
sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 *
retvalSessionLifetimeMs;
}
```
I understand why you did this @acsaki . It's because you think:
> when reauth is disabled (when max reauth ms is NOT set), leave
ReauthInfo#sessionExpirationTimeNanos as null but return millis until the token
expires in SaslAuthenticateResponse's sessionLifetimeMs
I think it's correct, **IF** the sasl client did close the connection after
the `sessionLifetimeMs`. But I don't think we should have this optimistic
assumption for this "potential" security issue. I agree with @SamBarker about
your original version of "removing the if condition" is a good fix.:
```
...
else
retvalSessionLifetimeMs =
zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs,
sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 *
retvalSessionLifetimeMs;
```
That is, no matter the reauth is enabled or not, we set the
`sessionExpirationTimeNanos`, to inform the server, too. So that we can make
sure when the session expired, either server or client will kill this
connection. WDYT?
##########
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##########
@@ -976,8 +975,11 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
SelectionKey selectionKey = mock(SelectionKey.class);
when(channel.selectionKey()).thenReturn(selectionKey);
when(selectionKey.isValid()).thenReturn(true);
+ when(selectionKey.isReadable()).thenReturn(true);
when(selectionKey.readyOps()).thenReturn(SelectionKey.OP_READ);
- selectionKey.attach(channel);
+ when(selectionKey.attachment())
+ .thenReturn(channel)
+ .thenReturn(null);
Review Comment:
Same here, why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
