I have made a minor change to the callback handler interface to pass in the JAAS configuration entries in *configure,* to work with the multiple listener configuration introduced in KIP-103. I have also renamed the interface to AuthenticateCallbackHandler instead of AuthCallbackHandler to avoid confusion with authorization.
I have rebased and updated the PR (https://github.com/apache/kafka/pull/2022) as well. Please let me know if there are any other comments or suggestions to move this forward. Thank you... Regards, Rajini On Thu, Dec 15, 2016 at 3:11 PM, Rajini Sivaram <rsiva...@pivotal.io> wrote: > Ismael, > > The reason for choosing CallbackHandler interface as the configurable > interface is flexibility. As you say, we could instead define a simpler > PlainCredentialProvider and ScramCredentialProvider. But that would tie > users to Kafka's SaslServer implementation for PLAIN and SCRAM. > SaslServer/SaslClient implementations are already pluggable using standard > Java security provider mechanism. Callback handlers are the configuration > mechanism for SaslServer/SaslClient. By making the handlers configurable, > SASL becomes fully configurable for mechanisms supported by Kafka as well > as custom mechanisms. From the 'Scenarios' section in the KIP, a simpler > PLAIN/SCRAM interface satisfies the first two, but configurable callback > handlers enables all five. I agree that most users don't require this level > of flexibility, but we have had discussions about custom mechanisms in the > past for integration with existing authentication servers. So I think it is > a feature worth supporting. > > On Thu, Dec 15, 2016 at 2:21 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Thanks Rajini, your answers make sense to me. One more general point: we > > are following the JAAS callback architecture and exposing that to the > user > > where the user has to write code like: > > > > @Override > > public void handle(Callback[] callbacks) throws IOException, > > UnsupportedCallbackException { > > String username = null; > > for (Callback callback: callbacks) { > > if (callback instanceof NameCallback) > > username = ((NameCallback) callback).getDefaultName(); > > else if (callback instanceof PlainAuthenticateCallback) { > > PlainAuthenticateCallback plainCallback = > > (PlainAuthenticateCallback) callback; > > boolean authenticated = authenticate(username, > > plainCallback.password()); > > plainCallback.authenticated(authenticated); > > } else > > throw new UnsupportedCallbackException(callback); > > } > > } > > > > protected boolean authenticate(String username, char[] password) > throws > > IOException { > > if (username == null) > > return false; > > else { > > String expectedPassword = > > JaasUtils.jaasConfig(LoginType.SERVER.contextName(), "user_" + username, > > PlainLoginModule.class.getName()); > > return Arrays.equals(password, expectedPassword.toCharArray() > > ); > > } > > } > > > > Since we need to create a new callback type for Plain, Scram and so on, > is > > it really worth it to do it this way? For example, in the code above, the > > `authenticate` method could be the only thing the user has to implement > and > > we could do the necessary work to unwrap the data from the various > > callbacks when interacting with the SASL API. More work for us, but a > much > > more pleasant API for users. What are the drawbacks? > > > > Ismael > > > > On Thu, Dec 15, 2016 at 1:06 AM, Rajini Sivaram <rsiva...@pivotal.io> > > wrote: > > > > > Ismael, > > > > > > 1. At the moment AuthCallbackHandler is not a public interface, so I am > > > assuming that it can be modified. Yes, agree that we should keep > > non-public > > > methods separate. Will do that as part of the implementation of this > KIP. > > > > > > 2. Callback handlers do tend to depend on ordering, including those > > > included in the JVM and these in Kafka. I have specified the ordering > in > > > the KIP. Will make sure they get included in documentation too. > > > > > > 3. Added a note to the KIP. Kafka needs access to the SCRAM credentials > > to > > > perform SCRAM authentication. For PLAIN, Kafka only needs to know if > the > > > password is valid for the user. We want to support external > > authentication > > > servers whose interface is to validate password, not retrieve it. > > > > > > 4. Added code of ScramCredential to the KIP. > > > > > > > > > On Wed, Dec 14, 2016 at 3:54 PM, Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > Thanks Rajini, that helps. A few comments: > > > > > > > > 1. The `AuthCallbackHandler` interface already exists and we are > making > > > > breaking changes (removing a parameter from `configure` and adding > > > > additional methods). Is the reasoning that it was not a public > > interface > > > > before? It would be good to clearly separate public versus non-public > > > > interfaces in the security code (and we should tweak Gradle to > publish > > > > javadoc for the public ones). > > > > > > > > 2. It seems like there is an ordering when it comes to the invocation > > of > > > > callbacks. At least the current code assumes that `NameCallback` is > > > called > > > > first. If I am interpreting this correctly, we should specify that > > > > ordering. > > > > > > > > 3. The approach taken by `ScramCredentialCallback` is different than > > the > > > > one taken by `PlainAuthenticateCallback`. The former lets the user > pass > > > the > > > > credentials information while the latter passes the credentials and > > lets > > > > the user do the authentication. It would be good to explain the > > > > inconsistency. > > > > > > > > 4. We reference `ScramCredential` in a few places, so it would be > good > > to > > > > define that class too. > > > > > > > > Ismael > > > > > > > > On Wed, Dec 14, 2016 at 7:32 AM, Rajini Sivaram < > > > > rajinisiva...@googlemail.com> wrote: > > > > > > > > > Have added sample callback handlers for PLAIN and SCRAM. > > > > > > > > > > On Tue, Dec 13, 2016 at 4:10 PM, Rajini Sivaram < > > > > > rajinisiva...@googlemail.com> wrote: > > > > > > > > > > > Ismael, > > > > > > > > > > > > Thank you for the review. I will add an example. > > > > > > > > > > > > On Tue, Dec 13, 2016 at 1:07 PM, Ismael Juma <ism...@juma.me.uk> > > > > wrote: > > > > > > > > > > > >> Hi Rajini, > > > > > >> > > > > > >> Thanks for the KIP. I think this is useful and users have asked > > for > > > > > >> something like that. I like that you have a scenarios section, > do > > > you > > > > > >> think > > > > > >> you could provide a rough sketch of what a callback handler > would > > > look > > > > > >> like > > > > > >> for the first 2 scenarios? They seem to be the common ones, so > it > > > > would > > > > > >> help to see a concrete example. > > > > > >> > > > > > >> Ismael > > > > > >> > > > > > >> On Tue, Oct 11, 2016 at 7:28 AM, Rajini Sivaram < > > > > > >> rajinisiva...@googlemail.com> wrote: > > > > > >> > > > > > >> > Hi all, > > > > > >> > > > > > > >> > I have just created KIP-86 make callback handlers in SASL > > > > configurable > > > > > >> so > > > > > >> > that credential providers for SASL/PLAIN (and SASL/SCRAM when > it > > > is > > > > > >> > implemented) can be used with custom credential callbacks: > > > > > >> > > > > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > >> > 86%3A+Configurable+SASL+callback+handlers > > > > > >> > > > > > > >> > Comments and suggestions are welcome. > > > > > >> > > > > > > >> > Thank you... > > > > > >> > > > > > > >> > > > > > > >> > Regards, > > > > > >> > > > > > > >> > Rajini > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Rajini > > > > > > > > > > > > > > >
