Hi all, To simplify dynamic update of SASL configs in future (e.g add a new SASL mechanism with a new callback handler or Login), I have separated out the broker-side configs with a mechanism prefix in the property name (similar to listener prefix) instead of including all the classes together as a map. This will also make it easier to configure new Login classes when new mechanisms are introduced alongside other mechanisms on a single listener.
Please let me know if you have any concerns. Thank you, Rajini On Wed, Jan 17, 2018 at 6:54 AM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi all, > > I have made some updates to this KIP to simplify addition of new SASL > mechanisms: > > 1. The Login interface has been made configurable as well (we have had > this interface for quite some time and it has been stable). > 2. The callback handler properties for client-side and server-side > have been separated out since we would never use the same classes for both. > > Any feedback or suggestions are welcome. > > Thank you, > > Rajini > > > On Mon, Apr 3, 2017 at 12:55 PM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > >> 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/kaf >>> ka/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 >>>> > > > > >>>> > > > >>>> > > >>>> > >>>> >>> >>> >> >
