ZOOKEEPER-4839: Clear user credentials when userName non-exist in sasl login module

[luoxiner]

Currently If a non-existent username is currently used along with the password of the last successfully logged-in user, this non-existent user can successfully log in. This issue occurs because the SaslServerCallbackHandler does not clear the information of the previously successfully logged-in user when the username does not exist. This Pull Request resolves this issue.

Assoiated jira issue: ZOOKEEPER-4839

[luoxiner]

@maoling Can you take a look on this fix

[luoxiner]

Hi, @kezhuw I push a new commit to resolve the CallbackHandler shared in multiple connections, can you give me some feed backs.

[luoxiner]

@kezhuw, thanks for your review. I will take a look and push a new commit later.

[luoxiner]

Hi, @kezhuw. I push a new commit to improve the codes with your suggestions. Can you take a look.

[luoxiner]

All commonts have been resolved, and fix some spelling and license check errors. Can you take a look? @kezhuw

[luoxiner]

The cpp-unit-test failed and I find an issue maybe associate with the failure ZOOKEEPER-4746. Let me take a look on it.

[kezhuw]

The cpp-unit-test failed and I find an issue maybe associate with the failure ZOOKEEPER-4746. Let me take a look on it.

That does not matter, it will not block this pr. I forgot it entirely and created ZOOKEEPER-4859 with attached log.

[kezhuw]

@anmolnar @tisonkun @eolivelli Would you mind take a look ?

[luoxiner]

Excellent candidate for 3.9.3 release. Thanks!

Question: why have you moved logic out of the class to static class SecurityUtils while you're not using it anywhere else. For instance getAppConfigurationEntryWithSection() is only used in SaslQuorumAuthServer, so it should be a private method of it.

getAppConfigurationEntryWithSection is also used in SecurityUtils‘s other methods. SecurityUtils::getDigestMd5Credentials -> SecurityUtils::getServerAppConfigurationEntry -> SecurityUtils::getAppConfigurationEntryWithSection.

[anmolnar]

Excellent candidate for 3.9.3 release. Thanks! Question: why have you moved logic out of the class to static class SecurityUtils while you're not using it anywhere else. For instance getAppConfigurationEntryWithSection() is only used in SaslQuorumAuthServer, so it should be a private method of it.

getAppConfigurationEntryWithSection is also used in SecurityUtils‘s other methods. SecurityUtils::getDigestMd5Credentials -> SecurityUtils::getServerAppConfigurationEntry -> SecurityUtils::getAppConfigurationEntryWithSection.

Following the call chain it still seems to me that every method is used only in ServerCnxnFactory abstract class, but correct me if I'm wrong. All of these methods should belong to there. You can still make them visible for testing.

[luoxiner]

Excellent candidate for 3.9.3 release. Thanks! Question: why have you moved logic out of the class to static class SecurityUtils while you're not using it anywhere else. For instance getAppConfigurationEntryWithSection() is only used in SaslQuorumAuthServer, so it should be a private method of it.

getAppConfigurationEntryWithSection is also used in SecurityUtils‘s other methods. SecurityUtils::getDigestMd5Credentials -> SecurityUtils::getServerAppConfigurationEntry -> SecurityUtils::getAppConfigurationEntryWithSection.

Following the call chain it still seems to me that every method is used only in ServerCnxnFactory abstract class, but correct me if I'm wrong. All of these methods should belong to there. You can still make them visible for testing.

Thanks for your reply! I find some dup codes in my pr, let me fix it.

[luoxiner]

Hi, @kezhuw @anmolnar I find something confused me.

[SaslQuorumServerCallbackHandler](https://github.com/apache/zookeeper/blob/bc9afbf8ef1bc6156643d3d05c87fcf8411e9d8f/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/auth/SaslQuorumServerCallbackHandler.java#L82)

The codes here are correct ?

I think if a section contains a module that is DigestLoginModule, we can set isDigestAuthn to true.

So i think the correct codes are

boolean isDigestAuthn = false;
for (AppConfigurationEntry entry : configurationEntries) {
    if (entry.getLoginModuleName().equals(DigestLoginModule.class.getName())) {
      Map<String, ?> options = entry.getOptions();
      // Populate DIGEST-MD5 user -> password map with JAAS configuration entries from the "QuorumServer" section.
      // Usernames are distinguished from other options by prefixing the username with a "user_" prefix.
      for (Map.Entry<String, ?> pair : options.entrySet()) {
          String key = pair.getKey();
          if (key.startsWith(USER_PREFIX)) {
              String userName = key.substring(USER_PREFIX.length());
              credentials.put(userName, (String) pair.getValue());
          }
      }
      isDigestAuthn = true;
   }
}

Am I right?

[kezhuw]

I guess it might express a preference over non DIGEST-MD5 module. But I doubt ZooKeeper will work probably in case of multiple login modules especially client-server authentication. cc @ztzg

I do noticed the repetition in reviewing(https://github.com/apache/zookeeper/pull/2176#discussion_r1751088413), I am ok for it to be unified somehow. But be careful and don't break ZOOKEEPER-4753.

Some code assumes there is only one login module and most mixes them.

https://github.com/apache/zookeeper/blob/bc9afbf8ef1bc6156643d3d05c87fcf8411e9d8f/zookeeper-server/src/main/java/org/apache/zookeeper/Login.java#L108-L120

[luoxiner]

Hi, @kezhuw @anmolnar I make some changes.

1. remove getAppConfigurationEntryWithSection and getServerAppConfigurationEntry in SecurityUtils because the caller alreay have an AppConfigurationEntry[] entries in the context, we can use it directly, we don’t need check and get again in the Utils methods.

   Codes here:

ServerCnxnFactory SaslQuorumAuthServer

2. move getServerCredentials to ServerCnxnFactory and make it private.

Please take a look and give some feed back, thanks !

[luoxiner]

Please remove this, it's not needed anymore:

https://github.com/apache/zookeeper/pull/2176/files#diff-dc881dad2c11cb59d7c710313e05fff31c098fbfa4cc02d250515e8c883583e5R120

@anmolnar I have removed it, please take a look.

[anmolnar]

Merged. Thanks @luoxiner !

[anmolnar]

@luoxiner What is your Jira id?

[luoxiner]

@luoxiner What is your Jira id?

luoxin