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
> > >
> >
>
