Repository navigation
Fix ZkClient timeout after SASL DIGEST-MD5 authentication - #3185
adityaanikam wants to merge 4 commits into
Conversation
isKerberosAuthEnabled (added by a prior fix for Kerberos-enabled ZooKeeper connections) only recognized Krb5LoginModule, so waitUntilConnected waited for SyncConnected for any other SASL login module. When a ZooKeeper server requires SASL client authentication, the client reaches SaslAuthenticated for DIGEST-MD5 the same way it does for Kerberos, and SyncConnected never follows, so the controller and participant timed out even after a successful login. Extract the login module check into isSaslAuthLoginModule and extend it to also match DigestLoginModule, so DIGEST-MD5 sessions wait for the same terminal state Kerberos sessions already do. Kerberos behavior is unchanged. Added a unit test for the extracted predicate covering Kerberos, Digest and a non-SASL module. It does not require a live SASL handshake, since the earlier connection-based approach was unreliable against a non-SASL test server. Fixes apache#3184
There was a problem hiding this comment.
Should this method name change now? Same with the Javadoc for the return statement
There was a problem hiding this comment.
Good point, the method covers DIGEST as well now, so I renamed it to isSaslAuthEnabled and updated its javadoc (including the @return), the log message, and the tests that reach it by reflection. Pushed as a separate commit.
The method also returns true for DIGEST login modules since the previous commit, so the name and javadoc no longer said what it checks. Renamed it, updated the javadoc, the step comments and the log message, and renamed the tests and the reflection helper that reach it.
|
This logic hard-codes only Krb5LoginModule and DigestLoginModule as SASL authentication. ZooKeeper clients can use a custom or wrapped JAAS LoginModule; in that case, SASL may still complete and emit SaslAuthenticated, but this method returns false and waitUntilConnected() waits for SyncConnected instead. That can reintroduce the connection timeout this change is trying to fix. Can we determine the expected connection state from whether client SASL is enabled/configured, rather than from a substring match on two specific login-module class names? At minimum, please add coverage for a non-built-in/custom login-module configuration. |
|
Thanks, that makes sense. I checked how ZooKeeper decides this: ZooKeeperSaslClient builds its SASL client whenever the JAAS config has an entry for the client context, whatever login module that entry names, and clientTunneledAuthenticationInProgress() uses the same check. So the module class name is not what decides it. I removed the Krb5LoginModule/DigestLoginModule match. isSaslAuthEnabled() now returns true when the SASL client is enabled and the JAAS config has an entry for the client context. I also replaced the predicate test with tests for a DIGEST module and a custom module. They enable SASL after the client is built, because with SASL expected a live connection to the non-SASL test server would wait for a SaslAuthenticated event that never arrives. |
junkaixue
left a comment
junkaixue
left a comment
There was a problem hiding this comment.
Could we add an integration test that configures a DIGEST-enabled server and client before building ZkClient, then asserts that the client connects successfully? This helper deliberately connects to the non-SASL server first and enables SASL afterward. The current tests therefore verify isSaslAuthEnabled() by reflection, but do not exercise the waitUntilConnected() path this PR is intended to fix. A regression that still times out during a real DIGEST connection would pass these tests.
The test starts a ZooKeeper server that requires DIGEST SASL, builds the client with SASL already configured, and waits for the connection after authentication has completed. It fails when waitUntilConnected() expects SyncConnected.
Fixes #3184
waitUntilConnecteddecides whether to wait forSaslAuthenticatedinstead ofSyncConnectedwith a check that was added in #3102 to fix #3101 (it was then namedisKerberosAuthEnabled). It only recognizedKrb5LoginModule. When a ZooKeeper server requires SASL client authentication (sessionRequireClientSASLAuth=true), a DIGEST-MD5 client reachesSaslAuthenticatedthe same way a Kerberos client does, andSyncConnectednever follows, sowaitUntilConnectedstill waits for the wrong state and the controller or participant times out, even though authentication itself succeeded.The check no longer matches login module class names.
isSaslAuthEnabled(renamed fromisKerberosAuthEnabled) now returns true when the SASL client is enabled and the JAAS config has an entry for the client context. ZooKeeper builds its SASL client whenever that entry exists, whatever login module it names, so this covers Kerberos, DIGEST and custom modules. The javadoc and log message are updated to match.Testing:
TestZkClientKerberoshas new tests for a DIGEST login module and a custom login module. They build the client first and enable SASL afterwards, because a live connection to the non-SASL test server would wait for aSaslAuthenticatedevent that never arrives. The custom module test failed before the change and passes now (4 tests, 0 failures). The fullzookeeper-apimodule run passes (104 tests, 0 failures).