KAFKA-4292: Configurable SASL callback handlers by rajinisivaram · Pull Request #2022 · apache/kafka

rajinisivaram · 2016-10-13T12:19:39Z

Implementation of KIP-86: https://cwiki.apache.org/confluence/display/KAFKA/KIP-86%3A+Configurable+SASL+callback+handlers

asfbot · 2017-03-30T21:09:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2554/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-30T21:19:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2556/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-30T21:20:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2556/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-30T21:22:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2550/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-30T22:55:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2550/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-03-30T23:33:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2560/
Test FAILed (JDK 8 and Scala 2.11).

omkreddy · 2018-01-24T16:58:56Z

nit: letter "F" at the end of the line

omkreddy · 2018-01-24T17:50:04Z

nit: Similar to the other sasl config descriptions., we can use "SASL" Acronym in descriptions.

omkreddy · 2018-01-24T17:50:40Z

nit: same as above

rajinisivaram · 2018-01-26T15:57:30Z

@junrao Do you have time to review this PR? Thank you!

rondagostino · 2018-01-27T17:43:36Z

Example should use ":" instead of "=": PLAIN:CustomPlainCallbackHandler,SCRAM-SHA-256:CustomScramCallbackHandler

It also might be good to somehow include an indication that the values are fully-qualified class names ("A map between SASL mechanisms and fully-qualified SASL server callback handler class names", or "org.mypackage.CustomPlainCalbackHandler")

rondagostino · 2018-01-27T18:04:14Z

Would it ever be acceptable to use the config map to override the callback handlers for the built-in mechanism implementations (GSSAPI, PLAIN, and the two SCRAM-related ones)? If so then the code is fine, but if not then the built-in mechanism names should be explicitly checked for first. Same goes for the client side checks above.

@rondagostino Thanks for the review. Yes, it is acceptable to override callback handlers for built-in mechanisms. One of the motivations for the KIP is to enable customization of built-in mechanisms to integrate with existing authentication servers (e.g. use an existing server or database to verify passwords for PLAIN).

rondagostino · 2018-01-27T18:05:28Z

See comment below about the order of built-in mechanisms vs. non-built-in ones.

Same as before, callback handlers can be overridden for built-in mechanisms too.

asfgit · 2018-02-05T21:32:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-test-coverage/333/

rondagostino · 2018-03-22T15:15:45Z

+                                                     JaasContext.Type contextType,
+                                                     String saslMechanism,
+                                                     Class<? extends Login> defaultLoginClass) {
+        String prefix  = contextType == JaasContext.Type.SERVER ? ListenerName.saslMechanismPrefix(saslMechanism) : "";


From SaslConfigs#SASL_LOGIN_CLASS_DOC:

For brokers, login config must be prefixed with listener prefix and SASL mechanism name in lower-case. For example, listener.name.sasl_ssl.scram-sha-256.sasl.login.class=com.example.CustomScramLogin

This code is only using the SASL mechanism name in lower-case as the prefix. Assuming this observation is in fact a problem, I think it raises a broader issue, which is that the listener (SASL_SSL vs. SASL_PLAIN) is not known at this point in the code (or at least it isn't readily available).

Listener prefix is removed from the config name before it gets here. This code will find scram-sha-256.sasl.login.class in configs only if it was prefixed with the listener name that this method is being invoked for. It is the same with callback handlers too.

Ok, so no problem in the code at this point. This behavior -- stripping the listener prefix off before passing the config map to the code when it runs for a particular listener -- probably warrants documentation, but I'm not sure where that documentation should be (or if it exists already and I just haven't seen). Thanks for the clarification.

rajinisivaram · 2018-03-22T21:40:30Z

@rondagostino KIP-86 didnt make login callbacks configurable since we didn't have a mechanism where we needed it. We currently use the same LoginCallbackHandler for all mechanisms. If we want to use callbacks for OAuth login, I think it will be better to do this with a separate configuration option sasl.login.callback.handler.class rather than reuse the client or server callbacks since these serve different purposes. I can add the option to this PR and update KIP-86 if you agree that it is the right approach for OAuth.

rondagostino · 2018-03-23T16:11:28Z

@rajinisivaram Ok, I now understand the difference between the two client-side callback handlers. The SASL Client callback handler is supposed to mediate between the SASL Client and the JAAS Login Module -- when the SASL client needs something it should use the SASL Client callback handler to get it rather than going to get it by itself (for example, looking in the Subject's public or private credentials). By using the callback handler like this (as opposed to hard-coding the SASL Client to look somewhere) everything becomes pluggable. I hadn't been doing that, so I will make that change in the OAuth code. Now that I understand this difference, yes, we do need to be able to configure the Login callback handler. In the OAuth case this callback handler is responsible for providing the OAuthBearerToken. Can you add the sasl.login.callback.handler.class configuration option as suggested?

rondagostino · 2018-03-30T14:42:35Z

+                         LoginMetadata<?> loginMetadata) throws IOException, LoginException {
+        this.loginMetadata = loginMetadata;
+        this.login = Utils.newInstance(loginMetadata.loginClass);
+        AuthenticateCallbackHandler callbackHandler = Utils.newInstance(loginMetadata.loginCallbackClass);


@rajinisivaram The login callback handler class isn't getting its #configure(Map<String, ?>, String, List) invoked. Perhaps it might be better to treat the login callback handler class the same way the client and server callback handler classes are treated, which is to create/configure them in SaslChannelBuilder? Note that the login callback handler class is potentially used both on the client side and on the server side (it is used on the broker when the mechanism is the inter-broker protocol).

@rondagostino Thanks for the review. Good catch. Added invocation of configure and missing handler in KafkaConfig. The lifecycle of LoginCallbackHandler is the same as that of Login, both managed by LoginManager. In particular, LoginCallbackHandler/Login can outlive SaslChannelBuilder. Hence these instances are created in LoginManager.

rondagostino · 2018-03-30T15:21:15Z

+        + "that implements the AuthenticateCallbackHandler interface.";
+
+    public static final String SASL_LOGIN_CALLBACK_HANDLER_CLASS = "sasl.login.callback.handler.class";
+    public static final String SASL_LOGIN_CALLBACK_HANDLER_CLASS_DOC = "The fully qualified name of a SASL login callback handler class "


I think this documentation should state that "For brokers, login callback handler config must be prefixed with listener prefix and SASL mechanism name in lower-case" because this is valid to use on both the client-side and the broker side.

junrao

@rajinisivaram : Thanks for the patch. Looks good overall. Just a few minor comments below.

junrao · 2018-03-30T20:44:28Z

     * @param saslMechanism Negotiated SASL mechanism
+     * @param jaasConfigEntries JAAS configuration entries from the JAAS login context.
+     *        This list contains a single entry for clients and may contain more than
+     *        one entry for servers if multiple mechanisms are enabled on a listener.


Hmm, in the ChannelBuilder, it seems that we always pass in the entry for one mechanism.

Also, could we add some comments to clarify the difference between configs and jaasConfigEntries? For example, if a key config is present in both, which one takes precedence.

jaasConfigEntries can contain multiple entries if using static JAAS config with multiple entries in a single KafkaServer login context when multiple mechanisms are enabled. Updated the doc to reflect that and also added clarification for configs and jaasConfigEntries.

junrao · 2018-03-30T21:47:34Z

     */
-    void configure(Map<String, ?> configs, JaasContext jaasContext);
+    void configure(Map<String, ?> configs, String contextName, Configuration configuration,
+                   AuthenticateCallbackHandler loginCallbackHandler);


It would be useful to add a comment to describe the difference between configs and Configuration.

junrao · 2018-03-30T21:54:43Z

 import org.apache.kafka.common.config.ConfigResource
 import org.apache.kafka.common.security.JaasUtils
 import org.apache.kafka.common.security.scram._
+import org.apache.kafka.common.security.scram.internal.{ScramCredentialUtils, ScramFormatter, ScramMechanism}


The import for org.apache.kafka.common.security.scram._ seems unused now?

junrao · 2018-03-30T21:57:40Z

     * If login module name is specified, return option value only from that module.
     */
    public String configEntryOption(String key, String loginModuleName) {
+        return configEntryOption(configurationEntries, key, loginModuleName);


It seems configEntryOption() is never used?

junrao · 2018-03-30T22:00:37Z

      .define(SaslMechanismInterBrokerProtocolProp, STRING, Defaults.SaslMechanismInterBrokerProtocol, MEDIUM, SaslMechanismInterBrokerProtocolDoc)
      .define(SaslJaasConfigProp, PASSWORD, null, MEDIUM, SaslJaasConfigDoc)
      .define(SaslEnabledMechanismsProp, LIST, Defaults.SaslEnabledMechanisms, MEDIUM, SaslEnabledMechanismsDoc)
+      .define(SaslServerCallbackHandlerClassProp, STRING, null, MEDIUM, SaslServerCallbackHandlerClassProp)


The last param should be Doc instead of Prop?

junrao · 2018-03-30T23:11:14Z

    private final Map<String, ?> configs;
    private final KafkaPrincipalBuilder principalBuilder;
    private final DelegationTokenCache tokenCache;
+    private final Map<String, AuthenticateCallbackHandler> callbackHandlers;


tokenCache, credentialCache and jaasContexts are no longer used?

junrao · 2018-03-30T23:33:54Z

+                TestServerCallbackHandler.class.getName());
+        server = createEchoServer(securityProtocol);
+
+        jaasConfig.setClientOptions("PLAIN", TestServerCallbackHandler.USERNAME, TestServerCallbackHandler.PASSWORD);


These are the configs for the server. So, the name setClientOptions is a bit mis-leading.

It is setting the client options based on the values used in the server callback. Added a comment to clarify.

junrao · 2018-03-30T23:43:47Z

+    @Test
+    public void testAuthenticateCallbackHandlerMechanisms() throws Exception {
+        SecurityProtocol securityProtocol = SecurityProtocol.SASL_PLAINTEXT;
+        configureMechanisms("DIGEST-MD5", Arrays.asList("DIGEST-MD5", "PLAIN"));


Do we need to add DIGEST-MD5?

The test is using two mechanisms with two different callbacks to verify that the right callback is used when there are multiple mechanisms. Added comment and also updated the test to verify both mechanisms.

junrao · 2018-03-30T23:49:09Z

+        String prefix = ListenerName.forSecurityProtocol(securityProtocol).saslMechanismConfigPrefix("PLAIN");
+        saslServerConfigs.put(prefix + SaslConfigs.SASL_LOGIN_CLASS, TestLogin.class.getName());
+        server = createEchoServer(securityProtocol);
+        assertEquals(1, TestLogin.loginCount.get());


Hmm, there is no connection yet, why would count be 1?

login() is performed when the server channel builder is created, hence the count is one when the server starts even before any clients connect.

junrao · 2018-03-31T00:00:19Z

+
+// This test uses SASL callback handler overrides for server connections of Kafka broker
+// and client connections of Kafka producers and consumers. Client connections of Kafka brokers
+// use default callback handlers. The second client used in the multi-user test


"Client connections of Kafka brokers use default callback handlers." It seems that the clients are using customized callbacks?

Broker use default client callback handlers for inter-broker communication. Producers/consumers are configured with customized callbacks. Updated the comment.

rajinisivaram · 2018-04-03T13:38:59Z

@rondagostino @junrao Thanks for the reviews. I have addressed the comments.

junrao

@rajinisivaram Thanks for the updated patch. LGTM. Just a few minor comments below.

junrao · 2018-04-03T20:01:26Z

+     *        from `jaasConfiguration`.
+     * @param contextName JAAS context name for this login which may be used to obtain
+     *        the login context from `jaasConfiguration`.
+     * @param loginCallbackHandler Login callback handler instance to use for this Login.


Could we add the java doc for jaasConfiguration?

junrao · 2018-04-03T21:07:07Z

+
+        @Override
+        public int hashCode() {
+            return Objects.hash(configInfo, loginClass);


Does the hashcode need to include loginCallbackClass ?

junrao · 2018-04-03T21:25:16Z

+
+// This test uses SASL callback handler overrides for server connections of Kafka broker
+// and client connections of Kafka producers and consumers. Client connections from Kafka brokers
+// used for inter-broker communication use default callback handlers. The second client used in


We set KafkaConfig.SaslClientCallbackHandlerClassProp in line 106. So, it seems that the broker connection also has a customized callback?

Sorry, I had missed that earlier, updated the comment.

rajinisivaram · 2018-04-04T09:54:01Z

@junrao Thanks for the review. I have addressed the comments. If there are no other comments, I will merge later on today after the builds complete.

rajinisivaram · 2018-04-05T08:32:17Z

@rondagostino @junrao Thanks for the reviews, merging to trunk.

Implementation of KIP-86. Client, server and login callback handlers have been made configurable for both brokers and clients. Reviewers: Jun Rao <junrao@gmail.com>, Ron Dagostino <rndgstn@gmail.com>, Manikumar Reddy <manikumar.reddy@gmail.com>

rajinisivaram force-pushed the KAFKA-4292 branch from f7189cf to 7f42041 Compare October 14, 2016 13:13

rajinisivaram force-pushed the KAFKA-4292 branch from 7f42041 to 27b132a Compare November 2, 2016 12:27

rajinisivaram force-pushed the KAFKA-4292 branch 2 times, most recently from 38213b8 to dfd26c9 Compare March 30, 2017 20:32

rajinisivaram mentioned this pull request Apr 11, 2017

KAFKA-5050 add ldap authentication support on PlainSaslServer #2833

Closed

rajinisivaram force-pushed the KAFKA-4292 branch 2 times, most recently from b40d503 to 1d54a0e Compare January 18, 2018 15:20

omkreddy reviewed Jan 25, 2018

View reviewed changes

rajinisivaram force-pushed the KAFKA-4292 branch from 1d54a0e to 27ffbc7 Compare January 26, 2018 15:49

rondagostino reviewed Jan 27, 2018

View reviewed changes

rajinisivaram force-pushed the KAFKA-4292 branch from 792e52d to 97f5c9b Compare January 29, 2018 10:30

rajinisivaram force-pushed the KAFKA-4292 branch from 97f5c9b to 3301fd0 Compare February 5, 2018 18:43

rajinisivaram force-pushed the KAFKA-4292 branch from 3301fd0 to 0394612 Compare February 7, 2018 15:43

rajinisivaram force-pushed the KAFKA-4292 branch 2 times, most recently from bdf84b9 to 84d14ef Compare February 22, 2018 15:11

rondagostino reviewed Mar 22, 2018

View reviewed changes

KAFKA-4292: Configurable SASL callback handlers and login

6cd1533

rajinisivaram force-pushed the KAFKA-4292 branch from 84d14ef to 6cd1533 Compare March 29, 2018 22:35

rondagostino reviewed Mar 30, 2018

View reviewed changes

junrao reviewed Mar 31, 2018

View reviewed changes

Address review comments

a8aaa90

rajinisivaram force-pushed the KAFKA-4292 branch from 2bea156 to a8aaa90 Compare April 3, 2018 13:38

junrao approved these changes Apr 3, 2018

View reviewed changes

Address review comments

cc6cf29

rajinisivaram merged commit 9f8c316 into apache:trunk Apr 5, 2018

Uh oh!

Conversation

rajinisivaram commented Oct 13, 2016

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Jan 26, 2018

Uh oh!

rondagostino Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Mar 22, 2018

Uh oh!

rondagostino commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rondagostino Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rondagostino Jan 27, 2018 •

edited

Loading

rondagostino Mar 30, 2018 •

edited

Loading