[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r895290288
##
build.gradle:
##
@@ -1243,7 +1243,7 @@ project(':clients') {
testImplementation libs.bcpkix
testImplementation libs.junitJupiter
-testImplementation libs.mockitoCore
+testImplementation libs.mockitoInline
Review Comment:
@acsaki , as @ijuma suggested, would you file another small PR to update the
tests to avoid mocking the static methods? For the
`ChannelBuilders#createPrincipalBuilder`, it looks like we actually don't need
it for these tests, right? (the tests still pass after I removed them)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r894213689
##
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##
@@ -673,30 +673,26 @@ private long
calcCompletionTimesAndReturnSessionLifetimeMs() {
Long credentialExpirationMs = (Long) saslServer
.getNegotiatedProperty(SaslInternalConfigs.CREDENTIAL_LIFETIME_MS_SASL_NEGOTIATED_PROPERTY_KEY);
Long connectionsMaxReauthMs =
connectionsMaxReauthMsByMechanism.get(saslMechanism);
-if (credentialExpirationMs != null || connectionsMaxReauthMs !=
null) {
+boolean maxReauthSet = connectionsMaxReauthMs != null &&
connectionsMaxReauthMs > 0;
Review Comment:
`boolean maxReauthSet = connectionsMaxReauthMs != null &&
connectionsMaxReauthMs > 0;`
Make sense to me.
##
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 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:
Thanks for the explanation. I agree with the change. Make sense.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r27862
##
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:
> While there is the Authenticator interface where comments suggest that
#serverSessionExpirationTimeNanos should be left as null when re-authentication
is "disabled"
Is
[this](https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/network/Authenticator.java#L90-L102)
javadoc what you mean? From the javadoc, I don't see it said we should return
null for re-auth disabled. I think it's OK to return the value when re-auth
disabled.
> here are some clients or SASL mechanisms where we don't expect
reauthentication to ever happen?
Yes, it is. You can check
[this](https://github.com/edenhill/librdkafka/issues/3304) for reference.
Thanks.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r887814333
##
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:
@rondagostino @rajinisivaram , do you have any comments? Thanks.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r887787750
##
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:
Also, the debug line in L698 confused me a little. When
`credentialExpirationMs != null && sessionExpirationTimeNanos == null`, we'll
log:
```
"Authentication complete; session max lifetime from broker config={} ms,
credential expiration={} ({} ms); no session expiration, sending 0 ms to client"
```
The point I'm confused here is `no session expiration`. Why no session
expiration? Since `credentialExpirationMs != null`, there must be some
credential expiration time, so in this case, sending 0 to client doesn't make
sense to me, right? I think the log should also be updated. WDYT?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
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 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r886343646
##
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:
> I was wondering if the property change was what was actually causing the
test failures that you found rather than making it un-conditional.
@SamBarker ,I don't understand this. Which property are you referring to?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179: URL: https://github.com/apache/kafka/pull/12179#discussion_r881686465 ## clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java: ## @@ -681,8 +681,8 @@ else if (connectionsMaxReauthMs == null) else retvalSessionLifetimeMs = zeroIfNegative( Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); -if (retvalSessionLifetimeMs > 0L) -sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; + +sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; Review Comment: After some investigation, I found the `null` value is indicating the re-auth is disabled: https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/network/KafkaChannel.java#L535-L543 So, maybe here, we should make sure we need re-auth (that is, we did have expiration time, if it's 0, which means it's expired, not re-auth disabled). ex: ```java if (retvalSessionLifetimeMs > 0L || credentialExpirationMs != null || connectionsMaxReauthMs != null) sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; ``` So, if the `retvalSessionLifetimeMs==0`, and we don't have credential expiration ms and connection max reauth ms, we should keep it as `null` Does that make sense? -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
showuon commented on code in PR #12179: URL: https://github.com/apache/kafka/pull/12179#discussion_r881686465 ## clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java: ## @@ -681,8 +681,8 @@ else if (connectionsMaxReauthMs == null) else retvalSessionLifetimeMs = zeroIfNegative( Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs)); -if (retvalSessionLifetimeMs > 0L) -sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; + +sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; Review Comment: After some investigation, I found the `null` value is indicating the re-auth is disabled: https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/network/KafkaChannel.java#L535-L543 So, maybe here, we should make sure we need re-auth (that is, we did have expiration time, if it's 0, which means it's expired, not re-auth disabled). ex: ```java if (retvalSessionLifetimeMs > 0L || credentialExpirationMs || connectionsMaxReauthMs) sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs; ``` Does that make sense? -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
