Skip to content

[KAFKA-13848] Clients remain connected after SASL re-authentication f… #12179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
showuon merged 2 commits into from
Jun 10, 2022
Merged

[KAFKA-13848] Clients remain connected after SASL re-authentication f… #12179

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfbd750
[KAFKA-13848] Clients remain connected after SASL re-authentication f…
May 19, 2022
c3ef32e
[KAFKA-13848] Clients remain connected after SASL re-authentication f…
Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,7 +1243,7 @@ project(':clients') {

testImplementation libs.bcpkix
testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
testImplementation libs.mockitoInline

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why we need MockitoInline here? Any method only exists in mockitoInline?

@acsaki acsaki Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed this to mock out ChannelBuilders#createPrincipalBuilder and Sasl#createSaslServer. It's a petty there are these static calls, hard to test them.

@ijuma ijuma Jun 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChannelBuilders is our code, we should avoid mocking static methods.

@showuon showuon Jun 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

@stanislavkozlovski stanislavkozlovski Aug 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump, I second we should aim to remove this


testRuntimeOnly libs.slf4jlog4j
testRuntimeOnly libs.jacksonDatabind
Expand Down
7 changes: 4 additions & 3 deletions ...src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,10 +679,11 @@ private long calcCompletionTimesAndReturnSessionLifetimeMs() {
else if (connectionsMaxReauthMs == null)
retvalSessionLifetimeMs = zeroIfNegative(credentialExpirationMs - authenticationEndMs);
else
retvalSessionLifetimeMs = zeroIfNegative(
Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));
if (retvalSessionLifetimeMs > 0L)
retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));

if (connectionsMaxReauthMs != null) {

@SamBarker SamBarker Jun 1, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 I have a different understanding. My mental model was that if either of credentialExpirationMs != null || connectionsMaxReauthMs != null was true we have re-auth enabled. The re-auth disabled case I was thinking of was when both were false. Now that I think about that I'm not sure that is a valid scenario.

I think your original change to make setting sessionExpirationTimeNanos un-conditional was the right one, my >=0 test is effectively the same as making it un-conditional. Therefore my suggestion of delayed initialisation becomes moot as we don't need to worry about excluding the default value from the test.
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 SamBarker Jun 1, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@showuon what is your thinking on this?

@showuon showuon Jun 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rondagostino @rajinisivaram , do you have any comments? Thanks.

@acsaki acsaki Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@showuon this is what I've also found rather confusing. I agree that after the token expires the connection should be closed sooner or later which isn't going to happen when sessionExpirationTimeNanos is not set. While there is the Authenticator interface where comments suggest that #serverSessionExpirationTimeNanos should be left as null when re-authentication is "disabled". Does it make sense for reauth to be disabled? Or rather there are some clients or SASL mechanisms where we don't expect reauthentication to ever happen?

@showuon showuon Jun 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While there is the Authenticator interface where comments suggest that #serverSessionExpirationTimeNanos should be left as null when re-authentication is "disabled"

Is this 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 for reference.

Thanks.

sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs;
}
}
if (credentialExpirationMs != null) {
if (sessionExpirationTimeNanos != null)
Expand Down
8 changes: 5 additions & 3 deletions clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Comment on lines +784 to -791

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we did this change? Did we change anything affect this test?

@acsaki acsaki Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test failed after my change to use mockitoInline to mock statics. The selectionKey.attach line called the mock's method directly (pointless?) while it looked like the actual intention was selectionKey.attachment() to return the kafkaChannel. That's how it worked actually. Calling the mock method directly in the test and later asserting on attachment being null seemed confusing to me and the assert actually fails too. (with #attachment returning the kafkaChannel). Maybe this was sort of overbearing, is there a simpler way to fix the test?

@showuon showuon Jun 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I agree with the change. Make sense.


verify(kafkaChannel, atLeastOnce()).ready();
verify(kafkaChannel).disconnect();
Expand Down Expand Up @@ -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);
Comment on lines +978 to +982

@showuon showuon Jun 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why this change?

@acsaki acsaki Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

selectionKeys.add(selectionKey);

NetworkReceive receive = mock(NetworkReceive.class);
Expand Down
Loading