If there are no other concerns or suggestions on this KIP, I will start vote later this week.
Thank you... Regards, Rajini On Thu, Mar 30, 2017 at 9:42 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > 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 >> > > > > >> > > > >> > > >> > >> > >
