You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Rajini Sivaram <ra...@gmail.com> on 2018/04/18 11:49:07 UTC
Re: [DISCUSS] KIP-255: OAuth Authentication via SASL/OAUTHBEARER
Hi Ron,
A few more suggestions and questions:
1. The KIP says that various callback handlers and login have to be
configured in order to use OAuth. Could we also say that a default
implementation is included which is not suitable for production use, but
this would work out-of-the-box with no additional configuration required
for callback handlers and login class? So the default callback handler and
login class that we would use in SaslChannelBuilder for OAuth, if the
config is not overridden would be the classes that you are including here (
OAuthBearerUnsecuredValidatorCallbackHandler etc.)
2. Following on from 1) above, I think the default implementation
classes can be moved to the internal package, since they no longer need
to be part of the public API, if we just choose them automatically by
default. I think the only classes that really need to part of the public
API are OAuthBearerToken, OAuthBearerLoginModule, OAuthBearerLoginCallback
and OAuthBearerValidatorCallback. It is hard to tell from the current
package layout, but the packages that are public currently are
org.apache.kafka.common.security.auth,
org.apache.kafka.common.security.plain
and org.apache.kafka.common.security.scram. Callback handlers and login
class are not public for the other mechanisms.
3. Can we rename OAuthBearerLoginCallback to OAuthBearerTokenCallback or
something along those lines since it is used by SaslClient as well as the
login module?
4. We use `Ms` as the suffix for fields and methods that refer to
milliseconds. So, perhaps in OAuthBearerToken, we could have lifetimeMs
and startTimeMs? I thought initially that lifetime was a time interval
rather than the wall-clock time. Something like expiryTimeMs may be less
confusing. But that could just be me (and it is fine to use the
terminology used in OAuth RFCs, so I will leave it up to you).
5. I am wondering whether it would be better to define refresh
parameters as properties in SaslConfigs rather than in the JAAS config.
We already have similar properties defined for Kerberos, but with kerberos
prefix. I wonder if defining the properties in a mechanism-independent way
(e.g. sasl.login.refresh.window.factor) could work with different
mechanisms? We could use it initially for just OAuth, but if we did unify
refreshing logins in future, we could deprecate the current
Kerberos-specific properties and have just one set that works for any
mechanism that uses token refresh. What do you think?
Thanks,
Rajini
On Thu, Mar 29, 2018 at 11:39 PM, Rajini Sivaram <ra...@gmail.com>
wrote:
> Hi Ron,
>
> Thanks for the updates. I had a quick look and it is looking good.
>
> I have updated KIP-86 and the associated PR to with a new config
> sasl.login.callback.handler.class that matches what you are using in this
> KIP.
>
> On Thu, Mar 29, 2018 at 6:27 AM, Ron Dagostino <rn...@gmail.com> wrote:
>
>> Hi Rajini. I have adjusted the KIP to use callbacks and callback handlers
>>
>> throughout. I also clarified that production implementations of the
>> retrieval and validation callback handlers will require the use of an open
>> source JWT library, and the unsecured implementations are as far as
>> SASL/OAUTHBEARER will go out-of-the-box. Your suggestions, plus this
>> clarification, has allowed much of the code to move into the ".internal"
>> java package; the public-facing API now consists of just 8 Java classes, 1
>> Java interface, and a set of configuration requirements. I also added a
>> section outlinng those configuration requirements since they are extensive
>> (not onerously so -- just not something one can easily remember).
>>
>> Ron
>>
>> On Tue, Mar 13, 2018 at 11:44 AM, Rajini Sivaram <rajinisivaram@gmail.com
>> >
>> wrote:
>>
>> > Hi Ron,
>> >
>> > Thanks for the response. All sound good, I think the only outstanding
>> > question is around callbacks vs classes provided through the login
>> context.
>> > As you have pointed out, there are advantages of both approaches. Even
>> > though my preference is for callbacks, it is not a blocker since the
>> > current approach works fine too. I will make the case for callbacks
>> anyway,
>> > using OAuthTokenValidator as an example:
>> >
>> >
>> > - As you mentioned, the main advantage of using callbacks is
>> > consistency. It is the standard plug-in mechanism for SASL
>> > implementations
>> > in Java and keeps code consistent with built-in mechanisms like
>> > Kerberos as
>> > well as our own implementations like PLAIN and SCRAM.
>> > - With the current approach, there are two classes
>> OAuthTokenValidator
>> > and a default implementation OAuthBearerUnsecuredJwtValidator. I was
>> > thinking that we would have a public callback class
>> > OAuthTokenValidatorCallback
>> > instead and a default callback handler
>> > OAuthBearerUnsecuredJwtValidatorCallbackHandler. So it would be two
>> > classes either way?
>> > - JAAS config is very opaque, we don't log it because it could
>> contain
>> > passwords. Your option substitution classes could help here, but it
>> has
>> > generally made it difficult to diagnose failures in the past.
>> Callback
>> > handlers on the the other hand are logged as part of the broker
>> configs
>> > and
>> > can be easily made dynamically updatable.
>> > - In the current implementation, an instance of OAuthTokenValidator
>> > is created and configured for every SaslServer, i.e every
>> connection. We
>> > create one server callback handler instance per mechanism and cache
>> it.
>> > This is useful if we need to make an external connection or load
>> trust
>> > stores etc.
>> >
>> > For token retriever, I think either approach is fine, since it is tied
>> in
>> > with login anyway and would benefit from login manager cache either way.
>> >
>> > Regards,
>> >
>> > Rajini
>> >
>> > On Sat, Mar 10, 2018 at 4:19 AM, Ron Dagostino <rn...@gmail.com>
>> wrote:
>> >
>> > > Hi Rajini. Thanks for the great feedback. See below for my
>> > > thoughts/conclusions. I haven't implemented any of it yet or changed
>> the
>> > > KIP, but I will start to work on the areas where we are in agreement
>> > > immediately, and I will await your feedback on the areas where an
>> > > additional iteration is needed to arrive at a conclusion.
>> > >
>> > > Regarding (1), yes, we can and should eliminate some public API. See
>> > > below.
>> > >
>> > > Regarding (2), I will change the exception hierarchy so that it is
>> > > unchecked.
>> > >
>> > > Regarding (3) and (4), yes, I agree, the expiring/refresh code can and
>> > > should be simplified. The name of the Login class (I called it
>> > > ExpiringCredentialRefreshingLogin) must be part of the public API
>> > because
>> > > it is the class that must be set via the oauthbearer.sasl.login.class
>> > > property. Its underlying implementation doesn't have to be public,
>> but
>> > the
>> > > fully-qualified name has to be well-known and fixed so that it can be
>> > > associated with that configuration property. As you point out, we are
>> > not
>> > > unifying the refresh logic for OAUTHBEARER and GSSAPI, though it
>> could be
>> > > undertaken at some point in the future; the name "
>> > > ExpiringCredentialRefreshingLogin" should probably be used if/when
>> that
>> > > unification happens. In the meantime, the class that we expose should
>> > > probably be called "OAuthBearerLogin", and it's fully-qualified name
>> and
>> > > the fact that it recognizes several refresh-related property names in
>> the
>> > > config, with certain min/max/default values, are the only things that
>> > > should be public. I also agree from (4) that we can stipulate that
>> > > SASL/OAUTHBEARER only supports the case where OAUTHBEARER is the only
>> > SASL
>> > > mechanism communicated to the code, either because there is only one
>> SASL
>> > > mechanism defined for the cluster or because the config is done via
>> the
>> > new
>> > > dynamic functionality from KIP-226 that eliminates the
>> > > mechanism-to-login-module ambiguity associated with declaring multiple
>> > SASL
>> > > mechanisms in a single JAAS config file. Given all of this,
>> everything I
>> > > defined for token refresh could be internal implementation detail
>> except
>> > > for ExpiringCredentialLoginModule, which would no longer be needed,
>> and
>> > we
>> > > only have to expose a single class called OAuthBearerLogin.
>> > >
>> > > Regarding (5), I'm glad you agree the substitutable module options
>> > > functionality is generally useful, and I will publish a separate KIP
>> for
>> > > it. I'm thinking the package will be
>> > > org.apache.kafka.common.security.optionsubs (I'll gladly accept
>> anything
>> > > better if anyone can come up with something -- "optionsubs" is better
>> > than
>> > > "smo" but it still isn't that great, and unfortunately it is the best
>> > > relatively short thing I can think of at the moment). I'll also see
>> > what I
>> > > can do to minimize the surface area of the API; that discussion can be
>> > done
>> > > separately as part of that KIP's discussion thread.
>> > >
>> > > Regarding (6), I agree that exposing the validated token via a
>> publicly
>> > > defined SaslServer negotiated property name eliminates the need for
>> the
>> > > OAuthBearerSaslServer interface; I will make this change.
>> > >
>> > > Regarding (7), I agree that retrieving the validated token via a
>> callback
>> > > would be consistent with other mechanism implementations. I do have
>> one
>> > > concern about adopting this approach, though. The token validation
>> > > mechanism is typically going to require a decent amount of
>> configuration,
>> > > and that configuration is going to live in the login module options.
>> It
>> > > feels natural for the declaration of the validation mechanism to live
>> in
>> > > the same place, next to the configuration, which is how it currently
>> > > happens in the KIP. If we change it so that the validation mechanism
>> is
>> > > declared via the callback handler then we move that declaration
>> somewhere
>> > > else, where the callback handler class is defined, which is separate
>> from
>> > > where we define the options that configure it. I think there is some
>> > cost
>> > > associated with separating two things that should go together. Also,
>> now
>> > > that I think about it, the public API gets bigger with the callback
>> > > approach because we trade OAuthBearerTokenValidator for a callback
>> > handler
>> > > class and a callback class for it to recognize (the cost of 2 vs. 1 is
>> > not
>> > > much of a difference, but it does exist). Have I identified these
>> costs
>> > > correctly, and if so, do you feel the benefit of consistency outweighs
>> > > them, in which case I will make the change, or should we keep it the
>> way
>> > it
>> > > is?
>> > >
>> > > Regarding (8), I wonder if the same cost/benefit analysis applies to
>> the
>> > > token retriever. Let's decide on (7) first, then we can decide on
>> (8).
>> > >
>> > > Thanks again for the great feedback.
>> > >
>> > > Ron
>> > >
>> > >
>> > > On Thu, Mar 8, 2018 at 9:31 AM, Rajini Sivaram <
>> rajinisivaram@gmail.com>
>> > > wrote:
>> > >
>> > > > Hi Ron,
>> > > >
>> > > > Thanks for the KIP. Sorry for the delay in reviewing this. I have a
>> few
>> > > > questions/comments.
>> > > >
>> > > >
>> > > > 1. Are all of the classes listed in the KIP intended to be public
>> > > > classes/interfaces? Since it requires more effort to maintain
>> public
>> > > > classes, it will be good if we can make more of the.code
>> internal.
>> > For
>> > > > example, some of the classes in `oauthbearer.refresh` and `
>> > > > oauthbearer.smo` could be made internal?
>> > > > 2. Agree that it is better to use unchecked exceptions. Kafka
>> uses
>> > > > unchecked exceptions elsewhere for the same reasons you
>> described.
>> > > > 3. ExpiringCredentialxxx/RefreshConfigxxx: When OAuth is added,
>> we
>> > > will
>> > > > have two mechanisms using token refresh (Kerberos and OAuth). The
>> > KIP
>> > > > doesn't propose to unify the refresh logic for the two (that is
>> > fine).
>> > > > Since these interfaces are going to be used just for OAuth, I
>> would
>> > > keep
>> > > > them internal for now. If we do add another mechanism in future
>> that
>> > > can
>> > > > reuse this code, we could make them public, taking into account
>> any
>> > > > changes required. It doesn't look like we would want to customise
>> > > these
>> > > > for
>> > > > different OAuthBearer implementations.
>> > > > 4. ExpiringCredentialLoginModule/ExpiringCredentialRefreshingLo
>> gin:
>> > I
>> > > > understand why the interfaces have additional methods to
>> associate
>> > > login
>> > > > with mechanism. But in 1.1, we introduced sasl.jaas.config
>> property
>> > > for
>> > > > brokers, which associates login module to mechanism. The broker
>> > > property
>> > > > name is prefixed with listener name and mechanism (e.g.
>> > > > listener.name.sasl_ssl.oauthbearer.sasl.jaas.config). We will
>> > > continue
>> > > > to support multiple login modules within a login context in the
>> JAAS
>> > > > config
>> > > > file for backward compatibility, but I don't think we need to add
>> > > > additional interfaces to support this for new mechanisms since we
>> > can
>> > > > just
>> > > > use the property instead. With the new property that uses
>> different
>> > > > contexts for different mechanisms, LoginManager is different for
>> > each
>> > > > context (i.e. each mechanism). So it should be possible to make
>> this
>> > > > code simpler. The property also makes it easier to dynamically
>> > update
>> > > > configs corresponding to a mechanism.
>> > > > 5. SubstitutableModuleOptions: These look very generic and usable
>> > for
>> > > > other mechanisms. I think they ought to be in a separate package
>> not
>> > > > under
>> > > > oauthbearer. It will be good to enable this for PLAIN and SCRAM
>> as
>> > > well.
>> > > > This could even be in a separate KIP. Perhaps the package name
>> could
>> > > be
>> > > > a
>> > > > word since smo is not a standard abbreviation?
>> > > > 6. OAuthSaslServer: Another option is to keep this internal
>> without
>> > an
>> > > > additional interface and return OAuthBearerToken from
>> > > > *SaslServer.getNegotiatedProperty(String
>> > > > propName)* with a publicly defined property name.
>> > > > 7. OAuthBearerTokenValidator: I think this should be defined as a
>> > > > server-side Callback to be consistent with other mechanisms.
>> > Different
>> > > > OAuthBearer implementations could just use different callback
>> > > handlers,
>> > > > which will be configurable anyway.
>> > > > 8. OAuthBearerTokenRetriever: This could perhaps be a login
>> callback
>> > > if
>> > > > we made the login callback handler configurable.
>> > > >
>> > > > Regards,
>> > > >
>> > > > Rajini
>> > > >
>> > > >
>> > > > On Thu, Feb 22, 2018 at 4:16 AM, Ron Dagostino <rn...@gmail.com>
>> > > wrote:
>> > > >
>> > > > > Hi everyone. I implemented the ability to perform substitution in
>> > JAAS
>> > > > > config module options, which was the only part of KIP 255 that was
>> > not
>> > > > > implemented when I originally published the KIP last week. I have
>> > made
>> > > > > adjustments to that section of the KIP based on this
>> implementation
>> > > > > experience, including detailing how to add custom substitutions
>> > beyond
>> > > > the
>> > > > > 4 built-in ones (file, system property, environment variable, and
>> > > option
>> > > > > substitution). See
>> > > > > https://github.com/rondagostino/kafka/commit/548a95822b06b60
>> > > > > a92745084a3980d72295d2ce6
>> > > > > (code coverage 82%) for the detailed changes. The KIP code blocks
>> > are
>> > > > also
>> > > > > up-to-date.
>> > > > >
>> > > > > Ron
>> > > > >
>> > > > >
>> > > > > On Wed, Feb 14, 2018 at 8:11 PM, Ron Dagostino <rndgstn@gmail.com
>> >
>> > > > wrote:
>> > > > >
>> > > > > > Thanks, Ted. I've added the JIRA and mailing list links to the
>> > KIP,
>> > > > and
>> > > > > I
>> > > > > > added Javadoc addressing your questions -- both in the KIP code
>> > > blocks
>> > > > > and
>> > > > > > on GitHub (https://github.com/rondagostino/kafka/commit/
>> > > > > > c61f5bafad810b620ff1ebd04e1231d245183e36).
>> > > > > >
>> > > > > > Ron
>> > > > > >
>> > > > > > On Wed, Feb 14, 2018 at 7:19 PM, Ted Yu <yu...@gmail.com>
>> > wrote:
>> > > > > >
>> > > > > >> Nicely written KIP.
>> > > > > >>
>> > > > > >> Can you add link to this thread and fill in JIRA number ?
>> > > > > >>
>> > > > > >> For ExpiringCredential, why does expireTimeMillis() return long
>> > > while
>> > > > > >> other
>> > > > > >> methods return Long ?
>> > > > > >> Can you add some comment for WindowJitter in RefreshConfig ?
>> > > > > >>
>> > > > > >> Thanks
>> > > > > >>
>> > > > > >> On Wed, Feb 14, 2018 at 3:38 PM, Ron Dagostino <
>> rndgstn@gmail.com
>> > >
>> > > > > wrote:
>> > > > > >>
>> > > > > >> > Hi everyone.
>> > > > > >> >
>> > > > > >> > I created KIP-255: OAuth Authentication via SASL/OAUTHBEARER
>> > > > > >> > <https://cwiki.apache.org/confluence/pages/viewpage.action?
>> > > > > >> pageId=75968876
>> > > > > >> > >
>> > > > > >> > (https://cwiki.apache.org/confluence/pages/viewpage.
>> > > > > >> > action?pageId=75968876
>> > > > > >> > ).
>> > > > > >> >
>> > > > > >> > This KIP proposes adding the ability to authenticate to Kafka
>> > with
>> > > > > >> OAuth 2
>> > > > > >> > bearer tokens using the OAUTHBEARER SASL mechanism.
>> > > > > >> >
>> > > > > >> > The code blocks in the KIP all reflect the implementation at
>> > > > > >> > https://github.com/rondagostino/kafka/tree/KAFKA-4292_plus_
>> > > > > oauthbearer.
>> > > > > >> > Feel free to look there for more details if the code blocks
>> whet
>> > > > your
>> > > > > >> > appetite for more. Everything described in the KIP as
>> currently
>> > > > > worded
>> > > > > >> is
>> > > > > >> > implemented there (77% code coverage with tests) except for
>> the
>> > > > > ability
>> > > > > >> to
>> > > > > >> > perform substitution in JAAS config module options.
>> > > > > >> >
>> > > > > >> > Ron
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
Re: [DISCUSS] KIP-255: OAuth Authentication via SASL/OAUTHBEARER
Posted by Ron Dagostino <rn...@gmail.com>.
Hi everyone. KIP-255 is now updated to reflect all feedback to date. The
updated code is also available on the KAFKA-6562 branch in the repo at
https://github.com/rondagostino/kafka.git. We are now down to 1 public
interface and 3 public classes -- a dramatic reduction from the original
proposal reflecting the excellent feedback received to date. I will put
this KIP up for a vote in 3 days (no earlier than Thursday evening, EDT) in
the absence of feedback that would indicate the need for more discussion.
Ron
On Thu, Apr 19, 2018 at 11:24 AM, Ron Dagostino <rn...@gmail.com> wrote:
> Hi Rajini. Thanks for the feedback. I will adopt all of the
> suggestions. Regarding your question about moving the refresh config
> values out of the JAAS config and making them generic, yes, I think that
> would work, and it does advance us down the road toward an eventual
> unification. I'll post again when the KIP and the code are updated
> accordingly both for this change and for the removal of substitution as
> mentioned in the KIP-269 discussion.
>
> Ron
>
> On Wed, Apr 18, 2018 at 7:49 AM, Rajini Sivaram <ra...@gmail.com>
> wrote:
>
>> Hi Ron,
>>
>> A few more suggestions and questions:
>>
>>
>> 1. The KIP says that various callback handlers and login have to be
>> configured in order to use OAuth. Could we also say that a default
>> implementation is included which is not suitable for production use,
>> but
>> this would work out-of-the-box with no additional configuration
>> required
>> for callback handlers and login class? So the default callback handler
>> and
>> login class that we would use in SaslChannelBuilder for OAuth, if the
>> config is not overridden would be the classes that you are including
>> here (
>> OAuthBearerUnsecuredValidatorCallbackHandler etc.)
>> 2. Following on from 1) above, I think the default implementation
>> classes can be moved to the internal package, since they no longer need
>> to be part of the public API, if we just choose them automatically by
>> default. I think the only classes that really need to part of the
>> public
>> API are OAuthBearerToken, OAuthBearerLoginModule,
>> OAuthBearerLoginCallback
>> and OAuthBearerValidatorCallback. It is hard to tell from the current
>> package layout, but the packages that are public currently are
>> org.apache.kafka.common.security.auth,
>> org.apache.kafka.common.security.plain
>> and org.apache.kafka.common.security.scram. Callback handlers and
>> login
>> class are not public for the other mechanisms.
>> 3. Can we rename OAuthBearerLoginCallback to OAuthBearerTokenCallback
>> or
>> something along those lines since it is used by SaslClient as well as
>> the
>> login module?
>> 4. We use `Ms` as the suffix for fields and methods that refer to
>> milliseconds. So, perhaps in OAuthBearerToken, we could have lifetimeMs
>> and startTimeMs? I thought initially that lifetime was a time interval
>> rather than the wall-clock time. Something like expiryTimeMs may be
>> less
>> confusing. But that could just be me (and it is fine to use the
>> terminology used in OAuth RFCs, so I will leave it up to you).
>> 5. I am wondering whether it would be better to define refresh
>> parameters as properties in SaslConfigs rather than in the JAAS config.
>> We already have similar properties defined for Kerberos, but with
>> kerberos
>> prefix. I wonder if defining the properties in a mechanism-independent
>> way
>> (e.g. sasl.login.refresh.window.factor) could work with different
>> mechanisms? We could use it initially for just OAuth, but if we did
>> unify
>> refreshing logins in future, we could deprecate the current
>> Kerberos-specific properties and have just one set that works for any
>> mechanism that uses token refresh. What do you think?
>>
>> Thanks,
>>
>> Rajini
>>
>>
>> On Thu, Mar 29, 2018 at 11:39 PM, Rajini Sivaram <rajinisivaram@gmail.com
>> >
>> wrote:
>>
>> > Hi Ron,
>> >
>> > Thanks for the updates. I had a quick look and it is looking good.
>> >
>> > I have updated KIP-86 and the associated PR to with a new config
>> > sasl.login.callback.handler.class that matches what you are using in
>> this
>> > KIP.
>> >
>> > On Thu, Mar 29, 2018 at 6:27 AM, Ron Dagostino <rn...@gmail.com>
>> wrote:
>> >
>> >> Hi Rajini. I have adjusted the KIP to use callbacks and callback
>> handlers
>> >>
>> >> throughout. I also clarified that production implementations of the
>> >> retrieval and validation callback handlers will require the use of an
>> open
>> >> source JWT library, and the unsecured implementations are as far as
>> >> SASL/OAUTHBEARER will go out-of-the-box. Your suggestions, plus this
>> >> clarification, has allowed much of the code to move into the
>> ".internal"
>> >> java package; the public-facing API now consists of just 8 Java
>> classes, 1
>> >> Java interface, and a set of configuration requirements. I also added
>> a
>> >> section outlinng those configuration requirements since they are
>> extensive
>> >> (not onerously so -- just not something one can easily remember).
>> >>
>> >> Ron
>> >>
>> >> On Tue, Mar 13, 2018 at 11:44 AM, Rajini Sivaram <
>> rajinisivaram@gmail.com
>> >> >
>> >> wrote:
>> >>
>> >> > Hi Ron,
>> >> >
>> >> > Thanks for the response. All sound good, I think the only outstanding
>> >> > question is around callbacks vs classes provided through the login
>> >> context.
>> >> > As you have pointed out, there are advantages of both approaches.
>> Even
>> >> > though my preference is for callbacks, it is not a blocker since the
>> >> > current approach works fine too. I will make the case for callbacks
>> >> anyway,
>> >> > using OAuthTokenValidator as an example:
>> >> >
>> >> >
>> >> > - As you mentioned, the main advantage of using callbacks is
>> >> > consistency. It is the standard plug-in mechanism for SASL
>> >> > implementations
>> >> > in Java and keeps code consistent with built-in mechanisms like
>> >> > Kerberos as
>> >> > well as our own implementations like PLAIN and SCRAM.
>> >> > - With the current approach, there are two classes
>> >> OAuthTokenValidator
>> >> > and a default implementation OAuthBearerUnsecuredJwtValidator. I
>> was
>> >> > thinking that we would have a public callback class
>> >> > OAuthTokenValidatorCallback
>> >> > instead and a default callback handler
>> >> > OAuthBearerUnsecuredJwtValidatorCallbackHandler. So it would be
>> two
>> >> > classes either way?
>> >> > - JAAS config is very opaque, we don't log it because it could
>> >> contain
>> >> > passwords. Your option substitution classes could help here, but
>> it
>> >> has
>> >> > generally made it difficult to diagnose failures in the past.
>> >> Callback
>> >> > handlers on the the other hand are logged as part of the broker
>> >> configs
>> >> > and
>> >> > can be easily made dynamically updatable.
>> >> > - In the current implementation, an instance of
>> OAuthTokenValidator
>> >> > is created and configured for every SaslServer, i.e every
>> >> connection. We
>> >> > create one server callback handler instance per mechanism and
>> cache
>> >> it.
>> >> > This is useful if we need to make an external connection or load
>> >> trust
>> >> > stores etc.
>> >> >
>> >> > For token retriever, I think either approach is fine, since it is
>> tied
>> >> in
>> >> > with login anyway and would benefit from login manager cache either
>> way.
>> >> >
>> >> > Regards,
>> >> >
>> >> > Rajini
>> >> >
>> >> > On Sat, Mar 10, 2018 at 4:19 AM, Ron Dagostino <rn...@gmail.com>
>> >> wrote:
>> >> >
>> >> > > Hi Rajini. Thanks for the great feedback. See below for my
>> >> > > thoughts/conclusions. I haven't implemented any of it yet or
>> changed
>> >> the
>> >> > > KIP, but I will start to work on the areas where we are in
>> agreement
>> >> > > immediately, and I will await your feedback on the areas where an
>> >> > > additional iteration is needed to arrive at a conclusion.
>> >> > >
>> >> > > Regarding (1), yes, we can and should eliminate some public API.
>> See
>> >> > > below.
>> >> > >
>> >> > > Regarding (2), I will change the exception hierarchy so that it is
>> >> > > unchecked.
>> >> > >
>> >> > > Regarding (3) and (4), yes, I agree, the expiring/refresh code can
>> and
>> >> > > should be simplified. The name of the Login class (I called it
>> >> > > ExpiringCredentialRefreshingLogin) must be part of the public API
>> >> > because
>> >> > > it is the class that must be set via the
>> oauthbearer.sasl.login.class
>> >> > > property. Its underlying implementation doesn't have to be public,
>> >> but
>> >> > the
>> >> > > fully-qualified name has to be well-known and fixed so that it can
>> be
>> >> > > associated with that configuration property. As you point out, we
>> are
>> >> > not
>> >> > > unifying the refresh logic for OAUTHBEARER and GSSAPI, though it
>> >> could be
>> >> > > undertaken at some point in the future; the name "
>> >> > > ExpiringCredentialRefreshingLogin" should probably be used if/when
>> >> that
>> >> > > unification happens. In the meantime, the class that we expose
>> should
>> >> > > probably be called "OAuthBearerLogin", and it's fully-qualified
>> name
>> >> and
>> >> > > the fact that it recognizes several refresh-related property names
>> in
>> >> the
>> >> > > config, with certain min/max/default values, are the only things
>> that
>> >> > > should be public. I also agree from (4) that we can stipulate that
>> >> > > SASL/OAUTHBEARER only supports the case where OAUTHBEARER is the
>> only
>> >> > SASL
>> >> > > mechanism communicated to the code, either because there is only
>> one
>> >> SASL
>> >> > > mechanism defined for the cluster or because the config is done via
>> >> the
>> >> > new
>> >> > > dynamic functionality from KIP-226 that eliminates the
>> >> > > mechanism-to-login-module ambiguity associated with declaring
>> multiple
>> >> > SASL
>> >> > > mechanisms in a single JAAS config file. Given all of this,
>> >> everything I
>> >> > > defined for token refresh could be internal implementation detail
>> >> except
>> >> > > for ExpiringCredentialLoginModule, which would no longer be needed,
>> >> and
>> >> > we
>> >> > > only have to expose a single class called OAuthBearerLogin.
>> >> > >
>> >> > > Regarding (5), I'm glad you agree the substitutable module options
>> >> > > functionality is generally useful, and I will publish a separate
>> KIP
>> >> for
>> >> > > it. I'm thinking the package will be
>> >> > > org.apache.kafka.common.security.optionsubs (I'll gladly accept
>> >> anything
>> >> > > better if anyone can come up with something -- "optionsubs" is
>> better
>> >> > than
>> >> > > "smo" but it still isn't that great, and unfortunately it is the
>> best
>> >> > > relatively short thing I can think of at the moment). I'll also
>> see
>> >> > what I
>> >> > > can do to minimize the surface area of the API; that discussion
>> can be
>> >> > done
>> >> > > separately as part of that KIP's discussion thread.
>> >> > >
>> >> > > Regarding (6), I agree that exposing the validated token via a
>> >> publicly
>> >> > > defined SaslServer negotiated property name eliminates the need for
>> >> the
>> >> > > OAuthBearerSaslServer interface; I will make this change.
>> >> > >
>> >> > > Regarding (7), I agree that retrieving the validated token via a
>> >> callback
>> >> > > would be consistent with other mechanism implementations. I do
>> have
>> >> one
>> >> > > concern about adopting this approach, though. The token validation
>> >> > > mechanism is typically going to require a decent amount of
>> >> configuration,
>> >> > > and that configuration is going to live in the login module
>> options.
>> >> It
>> >> > > feels natural for the declaration of the validation mechanism to
>> live
>> >> in
>> >> > > the same place, next to the configuration, which is how it
>> currently
>> >> > > happens in the KIP. If we change it so that the validation
>> mechanism
>> >> is
>> >> > > declared via the callback handler then we move that declaration
>> >> somewhere
>> >> > > else, where the callback handler class is defined, which is
>> separate
>> >> from
>> >> > > where we define the options that configure it. I think there is
>> some
>> >> > cost
>> >> > > associated with separating two things that should go together.
>> Also,
>> >> now
>> >> > > that I think about it, the public API gets bigger with the callback
>> >> > > approach because we trade OAuthBearerTokenValidator for a callback
>> >> > handler
>> >> > > class and a callback class for it to recognize (the cost of 2 vs.
>> 1 is
>> >> > not
>> >> > > much of a difference, but it does exist). Have I identified these
>> >> costs
>> >> > > correctly, and if so, do you feel the benefit of consistency
>> outweighs
>> >> > > them, in which case I will make the change, or should we keep it
>> the
>> >> way
>> >> > it
>> >> > > is?
>> >> > >
>> >> > > Regarding (8), I wonder if the same cost/benefit analysis applies
>> to
>> >> the
>> >> > > token retriever. Let's decide on (7) first, then we can decide on
>> >> (8).
>> >> > >
>> >> > > Thanks again for the great feedback.
>> >> > >
>> >> > > Ron
>> >> > >
>> >> > >
>> >> > > On Thu, Mar 8, 2018 at 9:31 AM, Rajini Sivaram <
>> >> rajinisivaram@gmail.com>
>> >> > > wrote:
>> >> > >
>> >> > > > Hi Ron,
>> >> > > >
>> >> > > > Thanks for the KIP. Sorry for the delay in reviewing this. I
>> have a
>> >> few
>> >> > > > questions/comments.
>> >> > > >
>> >> > > >
>> >> > > > 1. Are all of the classes listed in the KIP intended to be
>> public
>> >> > > > classes/interfaces? Since it requires more effort to maintain
>> >> public
>> >> > > > classes, it will be good if we can make more of the.code
>> >> internal.
>> >> > For
>> >> > > > example, some of the classes in `oauthbearer.refresh` and `
>> >> > > > oauthbearer.smo` could be made internal?
>> >> > > > 2. Agree that it is better to use unchecked exceptions. Kafka
>> >> uses
>> >> > > > unchecked exceptions elsewhere for the same reasons you
>> >> described.
>> >> > > > 3. ExpiringCredentialxxx/RefreshConfigxxx: When OAuth is
>> added,
>> >> we
>> >> > > will
>> >> > > > have two mechanisms using token refresh (Kerberos and OAuth).
>> The
>> >> > KIP
>> >> > > > doesn't propose to unify the refresh logic for the two (that
>> is
>> >> > fine).
>> >> > > > Since these interfaces are going to be used just for OAuth, I
>> >> would
>> >> > > keep
>> >> > > > them internal for now. If we do add another mechanism in
>> future
>> >> that
>> >> > > can
>> >> > > > reuse this code, we could make them public, taking into
>> account
>> >> any
>> >> > > > changes required. It doesn't look like we would want to
>> customise
>> >> > > these
>> >> > > > for
>> >> > > > different OAuthBearer implementations.
>> >> > > > 4. ExpiringCredentialLoginModule/
>> ExpiringCredentialRefreshingLo
>> >> gin:
>> >> > I
>> >> > > > understand why the interfaces have additional methods to
>> >> associate
>> >> > > login
>> >> > > > with mechanism. But in 1.1, we introduced sasl.jaas.config
>> >> property
>> >> > > for
>> >> > > > brokers, which associates login module to mechanism. The
>> broker
>> >> > > property
>> >> > > > name is prefixed with listener name and mechanism (e.g.
>> >> > > > listener.name.sasl_ssl.oauthbearer.sasl.jaas.config). We will
>> >> > > continue
>> >> > > > to support multiple login modules within a login context in
>> the
>> >> JAAS
>> >> > > > config
>> >> > > > file for backward compatibility, but I don't think we need to
>> add
>> >> > > > additional interfaces to support this for new mechanisms
>> since we
>> >> > can
>> >> > > > just
>> >> > > > use the property instead. With the new property that uses
>> >> different
>> >> > > > contexts for different mechanisms, LoginManager is different
>> for
>> >> > each
>> >> > > > context (i.e. each mechanism). So it should be possible to
>> make
>> >> this
>> >> > > > code simpler. The property also makes it easier to dynamically
>> >> > update
>> >> > > > configs corresponding to a mechanism.
>> >> > > > 5. SubstitutableModuleOptions: These look very generic and
>> usable
>> >> > for
>> >> > > > other mechanisms. I think they ought to be in a separate
>> package
>> >> not
>> >> > > > under
>> >> > > > oauthbearer. It will be good to enable this for PLAIN and
>> SCRAM
>> >> as
>> >> > > well.
>> >> > > > This could even be in a separate KIP. Perhaps the package name
>> >> could
>> >> > > be
>> >> > > > a
>> >> > > > word since smo is not a standard abbreviation?
>> >> > > > 6. OAuthSaslServer: Another option is to keep this internal
>> >> without
>> >> > an
>> >> > > > additional interface and return OAuthBearerToken from
>> >> > > > *SaslServer.getNegotiatedProperty(String
>> >> > > > propName)* with a publicly defined property name.
>> >> > > > 7. OAuthBearerTokenValidator: I think this should be defined
>> as a
>> >> > > > server-side Callback to be consistent with other mechanisms.
>> >> > Different
>> >> > > > OAuthBearer implementations could just use different callback
>> >> > > handlers,
>> >> > > > which will be configurable anyway.
>> >> > > > 8. OAuthBearerTokenRetriever: This could perhaps be a login
>> >> callback
>> >> > > if
>> >> > > > we made the login callback handler configurable.
>> >> > > >
>> >> > > > Regards,
>> >> > > >
>> >> > > > Rajini
>> >> > > >
>> >> > > >
>> >> > > > On Thu, Feb 22, 2018 at 4:16 AM, Ron Dagostino <
>> rndgstn@gmail.com>
>> >> > > wrote:
>> >> > > >
>> >> > > > > Hi everyone. I implemented the ability to perform
>> substitution in
>> >> > JAAS
>> >> > > > > config module options, which was the only part of KIP 255 that
>> was
>> >> > not
>> >> > > > > implemented when I originally published the KIP last week. I
>> have
>> >> > made
>> >> > > > > adjustments to that section of the KIP based on this
>> >> implementation
>> >> > > > > experience, including detailing how to add custom substitutions
>> >> > beyond
>> >> > > > the
>> >> > > > > 4 built-in ones (file, system property, environment variable,
>> and
>> >> > > option
>> >> > > > > substitution). See
>> >> > > > > https://github.com/rondagostino/kafka/commit/548a95822b06b60
>> >> > > > > a92745084a3980d72295d2ce6
>> >> > > > > (code coverage 82%) for the detailed changes. The KIP code
>> blocks
>> >> > are
>> >> > > > also
>> >> > > > > up-to-date.
>> >> > > > >
>> >> > > > > Ron
>> >> > > > >
>> >> > > > >
>> >> > > > > On Wed, Feb 14, 2018 at 8:11 PM, Ron Dagostino <
>> rndgstn@gmail.com
>> >> >
>> >> > > > wrote:
>> >> > > > >
>> >> > > > > > Thanks, Ted. I've added the JIRA and mailing list links to
>> the
>> >> > KIP,
>> >> > > > and
>> >> > > > > I
>> >> > > > > > added Javadoc addressing your questions -- both in the KIP
>> code
>> >> > > blocks
>> >> > > > > and
>> >> > > > > > on GitHub (https://github.com/rondagostino/kafka/commit/
>> >> > > > > > c61f5bafad810b620ff1ebd04e1231d245183e36).
>> >> > > > > >
>> >> > > > > > Ron
>> >> > > > > >
>> >> > > > > > On Wed, Feb 14, 2018 at 7:19 PM, Ted Yu <yuzhihong@gmail.com
>> >
>> >> > wrote:
>> >> > > > > >
>> >> > > > > >> Nicely written KIP.
>> >> > > > > >>
>> >> > > > > >> Can you add link to this thread and fill in JIRA number ?
>> >> > > > > >>
>> >> > > > > >> For ExpiringCredential, why does expireTimeMillis() return
>> long
>> >> > > while
>> >> > > > > >> other
>> >> > > > > >> methods return Long ?
>> >> > > > > >> Can you add some comment for WindowJitter in RefreshConfig ?
>> >> > > > > >>
>> >> > > > > >> Thanks
>> >> > > > > >>
>> >> > > > > >> On Wed, Feb 14, 2018 at 3:38 PM, Ron Dagostino <
>> >> rndgstn@gmail.com
>> >> > >
>> >> > > > > wrote:
>> >> > > > > >>
>> >> > > > > >> > Hi everyone.
>> >> > > > > >> >
>> >> > > > > >> > I created KIP-255: OAuth Authentication via
>> SASL/OAUTHBEARER
>> >> > > > > >> > <https://cwiki.apache.org/conf
>> luence/pages/viewpage.action?
>> >> > > > > >> pageId=75968876
>> >> > > > > >> > >
>> >> > > > > >> > (https://cwiki.apache.org/confluence/pages/viewpage.
>> >> > > > > >> > action?pageId=75968876
>> >> > > > > >> > ).
>> >> > > > > >> >
>> >> > > > > >> > This KIP proposes adding the ability to authenticate to
>> Kafka
>> >> > with
>> >> > > > > >> OAuth 2
>> >> > > > > >> > bearer tokens using the OAUTHBEARER SASL mechanism.
>> >> > > > > >> >
>> >> > > > > >> > The code blocks in the KIP all reflect the implementation
>> at
>> >> > > > > >> > https://github.com/rondagostin
>> o/kafka/tree/KAFKA-4292_plus_
>> >> > > > > oauthbearer.
>> >> > > > > >> > Feel free to look there for more details if the code
>> blocks
>> >> whet
>> >> > > > your
>> >> > > > > >> > appetite for more. Everything described in the KIP as
>> >> currently
>> >> > > > > worded
>> >> > > > > >> is
>> >> > > > > >> > implemented there (77% code coverage with tests) except
>> for
>> >> the
>> >> > > > > ability
>> >> > > > > >> to
>> >> > > > > >> > perform substitution in JAAS config module options.
>> >> > > > > >> >
>> >> > > > > >> > Ron
>> >> > > > > >> >
>> >> > > > > >>
>> >> > > > > >
>> >> > > > > >
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>>
>
>
Re: [DISCUSS] KIP-255: OAuth Authentication via SASL/OAUTHBEARER
Posted by Ron Dagostino <rn...@gmail.com>.
Hi Rajini. Thanks for the feedback. I will adopt all of the suggestions.
Regarding your question about moving the refresh config values out of the
JAAS config and making them generic, yes, I think that would work, and it
does advance us down the road toward an eventual unification. I'll post
again when the KIP and the code are updated accordingly both for this
change and for the removal of substitution as mentioned in the KIP-269
discussion.
Ron
On Wed, Apr 18, 2018 at 7:49 AM, Rajini Sivaram <ra...@gmail.com>
wrote:
> Hi Ron,
>
> A few more suggestions and questions:
>
>
> 1. The KIP says that various callback handlers and login have to be
> configured in order to use OAuth. Could we also say that a default
> implementation is included which is not suitable for production use, but
> this would work out-of-the-box with no additional configuration required
> for callback handlers and login class? So the default callback handler
> and
> login class that we would use in SaslChannelBuilder for OAuth, if the
> config is not overridden would be the classes that you are including
> here (
> OAuthBearerUnsecuredValidatorCallbackHandler etc.)
> 2. Following on from 1) above, I think the default implementation
> classes can be moved to the internal package, since they no longer need
> to be part of the public API, if we just choose them automatically by
> default. I think the only classes that really need to part of the public
> API are OAuthBearerToken, OAuthBearerLoginModule,
> OAuthBearerLoginCallback
> and OAuthBearerValidatorCallback. It is hard to tell from the current
> package layout, but the packages that are public currently are
> org.apache.kafka.common.security.auth,
> org.apache.kafka.common.security.plain
> and org.apache.kafka.common.security.scram. Callback handlers and login
> class are not public for the other mechanisms.
> 3. Can we rename OAuthBearerLoginCallback to OAuthBearerTokenCallback or
> something along those lines since it is used by SaslClient as well as
> the
> login module?
> 4. We use `Ms` as the suffix for fields and methods that refer to
> milliseconds. So, perhaps in OAuthBearerToken, we could have lifetimeMs
> and startTimeMs? I thought initially that lifetime was a time interval
> rather than the wall-clock time. Something like expiryTimeMs may be less
> confusing. But that could just be me (and it is fine to use the
> terminology used in OAuth RFCs, so I will leave it up to you).
> 5. I am wondering whether it would be better to define refresh
> parameters as properties in SaslConfigs rather than in the JAAS config.
> We already have similar properties defined for Kerberos, but with
> kerberos
> prefix. I wonder if defining the properties in a mechanism-independent
> way
> (e.g. sasl.login.refresh.window.factor) could work with different
> mechanisms? We could use it initially for just OAuth, but if we did
> unify
> refreshing logins in future, we could deprecate the current
> Kerberos-specific properties and have just one set that works for any
> mechanism that uses token refresh. What do you think?
>
> Thanks,
>
> Rajini
>
>
> On Thu, Mar 29, 2018 at 11:39 PM, Rajini Sivaram <ra...@gmail.com>
> wrote:
>
> > Hi Ron,
> >
> > Thanks for the updates. I had a quick look and it is looking good.
> >
> > I have updated KIP-86 and the associated PR to with a new config
> > sasl.login.callback.handler.class that matches what you are using in
> this
> > KIP.
> >
> > On Thu, Mar 29, 2018 at 6:27 AM, Ron Dagostino <rn...@gmail.com>
> wrote:
> >
> >> Hi Rajini. I have adjusted the KIP to use callbacks and callback
> handlers
> >>
> >> throughout. I also clarified that production implementations of the
> >> retrieval and validation callback handlers will require the use of an
> open
> >> source JWT library, and the unsecured implementations are as far as
> >> SASL/OAUTHBEARER will go out-of-the-box. Your suggestions, plus this
> >> clarification, has allowed much of the code to move into the ".internal"
> >> java package; the public-facing API now consists of just 8 Java
> classes, 1
> >> Java interface, and a set of configuration requirements. I also added a
> >> section outlinng those configuration requirements since they are
> extensive
> >> (not onerously so -- just not something one can easily remember).
> >>
> >> Ron
> >>
> >> On Tue, Mar 13, 2018 at 11:44 AM, Rajini Sivaram <
> rajinisivaram@gmail.com
> >> >
> >> wrote:
> >>
> >> > Hi Ron,
> >> >
> >> > Thanks for the response. All sound good, I think the only outstanding
> >> > question is around callbacks vs classes provided through the login
> >> context.
> >> > As you have pointed out, there are advantages of both approaches. Even
> >> > though my preference is for callbacks, it is not a blocker since the
> >> > current approach works fine too. I will make the case for callbacks
> >> anyway,
> >> > using OAuthTokenValidator as an example:
> >> >
> >> >
> >> > - As you mentioned, the main advantage of using callbacks is
> >> > consistency. It is the standard plug-in mechanism for SASL
> >> > implementations
> >> > in Java and keeps code consistent with built-in mechanisms like
> >> > Kerberos as
> >> > well as our own implementations like PLAIN and SCRAM.
> >> > - With the current approach, there are two classes
> >> OAuthTokenValidator
> >> > and a default implementation OAuthBearerUnsecuredJwtValidator. I
> was
> >> > thinking that we would have a public callback class
> >> > OAuthTokenValidatorCallback
> >> > instead and a default callback handler
> >> > OAuthBearerUnsecuredJwtValidatorCallbackHandler. So it would be
> two
> >> > classes either way?
> >> > - JAAS config is very opaque, we don't log it because it could
> >> contain
> >> > passwords. Your option substitution classes could help here, but it
> >> has
> >> > generally made it difficult to diagnose failures in the past.
> >> Callback
> >> > handlers on the the other hand are logged as part of the broker
> >> configs
> >> > and
> >> > can be easily made dynamically updatable.
> >> > - In the current implementation, an instance of
> OAuthTokenValidator
> >> > is created and configured for every SaslServer, i.e every
> >> connection. We
> >> > create one server callback handler instance per mechanism and cache
> >> it.
> >> > This is useful if we need to make an external connection or load
> >> trust
> >> > stores etc.
> >> >
> >> > For token retriever, I think either approach is fine, since it is tied
> >> in
> >> > with login anyway and would benefit from login manager cache either
> way.
> >> >
> >> > Regards,
> >> >
> >> > Rajini
> >> >
> >> > On Sat, Mar 10, 2018 at 4:19 AM, Ron Dagostino <rn...@gmail.com>
> >> wrote:
> >> >
> >> > > Hi Rajini. Thanks for the great feedback. See below for my
> >> > > thoughts/conclusions. I haven't implemented any of it yet or
> changed
> >> the
> >> > > KIP, but I will start to work on the areas where we are in agreement
> >> > > immediately, and I will await your feedback on the areas where an
> >> > > additional iteration is needed to arrive at a conclusion.
> >> > >
> >> > > Regarding (1), yes, we can and should eliminate some public API.
> See
> >> > > below.
> >> > >
> >> > > Regarding (2), I will change the exception hierarchy so that it is
> >> > > unchecked.
> >> > >
> >> > > Regarding (3) and (4), yes, I agree, the expiring/refresh code can
> and
> >> > > should be simplified. The name of the Login class (I called it
> >> > > ExpiringCredentialRefreshingLogin) must be part of the public API
> >> > because
> >> > > it is the class that must be set via the
> oauthbearer.sasl.login.class
> >> > > property. Its underlying implementation doesn't have to be public,
> >> but
> >> > the
> >> > > fully-qualified name has to be well-known and fixed so that it can
> be
> >> > > associated with that configuration property. As you point out, we
> are
> >> > not
> >> > > unifying the refresh logic for OAUTHBEARER and GSSAPI, though it
> >> could be
> >> > > undertaken at some point in the future; the name "
> >> > > ExpiringCredentialRefreshingLogin" should probably be used if/when
> >> that
> >> > > unification happens. In the meantime, the class that we expose
> should
> >> > > probably be called "OAuthBearerLogin", and it's fully-qualified name
> >> and
> >> > > the fact that it recognizes several refresh-related property names
> in
> >> the
> >> > > config, with certain min/max/default values, are the only things
> that
> >> > > should be public. I also agree from (4) that we can stipulate that
> >> > > SASL/OAUTHBEARER only supports the case where OAUTHBEARER is the
> only
> >> > SASL
> >> > > mechanism communicated to the code, either because there is only one
> >> SASL
> >> > > mechanism defined for the cluster or because the config is done via
> >> the
> >> > new
> >> > > dynamic functionality from KIP-226 that eliminates the
> >> > > mechanism-to-login-module ambiguity associated with declaring
> multiple
> >> > SASL
> >> > > mechanisms in a single JAAS config file. Given all of this,
> >> everything I
> >> > > defined for token refresh could be internal implementation detail
> >> except
> >> > > for ExpiringCredentialLoginModule, which would no longer be needed,
> >> and
> >> > we
> >> > > only have to expose a single class called OAuthBearerLogin.
> >> > >
> >> > > Regarding (5), I'm glad you agree the substitutable module options
> >> > > functionality is generally useful, and I will publish a separate KIP
> >> for
> >> > > it. I'm thinking the package will be
> >> > > org.apache.kafka.common.security.optionsubs (I'll gladly accept
> >> anything
> >> > > better if anyone can come up with something -- "optionsubs" is
> better
> >> > than
> >> > > "smo" but it still isn't that great, and unfortunately it is the
> best
> >> > > relatively short thing I can think of at the moment). I'll also see
> >> > what I
> >> > > can do to minimize the surface area of the API; that discussion can
> be
> >> > done
> >> > > separately as part of that KIP's discussion thread.
> >> > >
> >> > > Regarding (6), I agree that exposing the validated token via a
> >> publicly
> >> > > defined SaslServer negotiated property name eliminates the need for
> >> the
> >> > > OAuthBearerSaslServer interface; I will make this change.
> >> > >
> >> > > Regarding (7), I agree that retrieving the validated token via a
> >> callback
> >> > > would be consistent with other mechanism implementations. I do have
> >> one
> >> > > concern about adopting this approach, though. The token validation
> >> > > mechanism is typically going to require a decent amount of
> >> configuration,
> >> > > and that configuration is going to live in the login module options.
> >> It
> >> > > feels natural for the declaration of the validation mechanism to
> live
> >> in
> >> > > the same place, next to the configuration, which is how it currently
> >> > > happens in the KIP. If we change it so that the validation
> mechanism
> >> is
> >> > > declared via the callback handler then we move that declaration
> >> somewhere
> >> > > else, where the callback handler class is defined, which is separate
> >> from
> >> > > where we define the options that configure it. I think there is
> some
> >> > cost
> >> > > associated with separating two things that should go together.
> Also,
> >> now
> >> > > that I think about it, the public API gets bigger with the callback
> >> > > approach because we trade OAuthBearerTokenValidator for a callback
> >> > handler
> >> > > class and a callback class for it to recognize (the cost of 2 vs. 1
> is
> >> > not
> >> > > much of a difference, but it does exist). Have I identified these
> >> costs
> >> > > correctly, and if so, do you feel the benefit of consistency
> outweighs
> >> > > them, in which case I will make the change, or should we keep it the
> >> way
> >> > it
> >> > > is?
> >> > >
> >> > > Regarding (8), I wonder if the same cost/benefit analysis applies to
> >> the
> >> > > token retriever. Let's decide on (7) first, then we can decide on
> >> (8).
> >> > >
> >> > > Thanks again for the great feedback.
> >> > >
> >> > > Ron
> >> > >
> >> > >
> >> > > On Thu, Mar 8, 2018 at 9:31 AM, Rajini Sivaram <
> >> rajinisivaram@gmail.com>
> >> > > wrote:
> >> > >
> >> > > > Hi Ron,
> >> > > >
> >> > > > Thanks for the KIP. Sorry for the delay in reviewing this. I have
> a
> >> few
> >> > > > questions/comments.
> >> > > >
> >> > > >
> >> > > > 1. Are all of the classes listed in the KIP intended to be
> public
> >> > > > classes/interfaces? Since it requires more effort to maintain
> >> public
> >> > > > classes, it will be good if we can make more of the.code
> >> internal.
> >> > For
> >> > > > example, some of the classes in `oauthbearer.refresh` and `
> >> > > > oauthbearer.smo` could be made internal?
> >> > > > 2. Agree that it is better to use unchecked exceptions. Kafka
> >> uses
> >> > > > unchecked exceptions elsewhere for the same reasons you
> >> described.
> >> > > > 3. ExpiringCredentialxxx/RefreshConfigxxx: When OAuth is
> added,
> >> we
> >> > > will
> >> > > > have two mechanisms using token refresh (Kerberos and OAuth).
> The
> >> > KIP
> >> > > > doesn't propose to unify the refresh logic for the two (that is
> >> > fine).
> >> > > > Since these interfaces are going to be used just for OAuth, I
> >> would
> >> > > keep
> >> > > > them internal for now. If we do add another mechanism in future
> >> that
> >> > > can
> >> > > > reuse this code, we could make them public, taking into account
> >> any
> >> > > > changes required. It doesn't look like we would want to
> customise
> >> > > these
> >> > > > for
> >> > > > different OAuthBearer implementations.
> >> > > > 4. ExpiringCredentialLoginModule/
> ExpiringCredentialRefreshingLo
> >> gin:
> >> > I
> >> > > > understand why the interfaces have additional methods to
> >> associate
> >> > > login
> >> > > > with mechanism. But in 1.1, we introduced sasl.jaas.config
> >> property
> >> > > for
> >> > > > brokers, which associates login module to mechanism. The broker
> >> > > property
> >> > > > name is prefixed with listener name and mechanism (e.g.
> >> > > > listener.name.sasl_ssl.oauthbearer.sasl.jaas.config). We will
> >> > > continue
> >> > > > to support multiple login modules within a login context in the
> >> JAAS
> >> > > > config
> >> > > > file for backward compatibility, but I don't think we need to
> add
> >> > > > additional interfaces to support this for new mechanisms since
> we
> >> > can
> >> > > > just
> >> > > > use the property instead. With the new property that uses
> >> different
> >> > > > contexts for different mechanisms, LoginManager is different
> for
> >> > each
> >> > > > context (i.e. each mechanism). So it should be possible to make
> >> this
> >> > > > code simpler. The property also makes it easier to dynamically
> >> > update
> >> > > > configs corresponding to a mechanism.
> >> > > > 5. SubstitutableModuleOptions: These look very generic and
> usable
> >> > for
> >> > > > other mechanisms. I think they ought to be in a separate
> package
> >> not
> >> > > > under
> >> > > > oauthbearer. It will be good to enable this for PLAIN and SCRAM
> >> as
> >> > > well.
> >> > > > This could even be in a separate KIP. Perhaps the package name
> >> could
> >> > > be
> >> > > > a
> >> > > > word since smo is not a standard abbreviation?
> >> > > > 6. OAuthSaslServer: Another option is to keep this internal
> >> without
> >> > an
> >> > > > additional interface and return OAuthBearerToken from
> >> > > > *SaslServer.getNegotiatedProperty(String
> >> > > > propName)* with a publicly defined property name.
> >> > > > 7. OAuthBearerTokenValidator: I think this should be defined
> as a
> >> > > > server-side Callback to be consistent with other mechanisms.
> >> > Different
> >> > > > OAuthBearer implementations could just use different callback
> >> > > handlers,
> >> > > > which will be configurable anyway.
> >> > > > 8. OAuthBearerTokenRetriever: This could perhaps be a login
> >> callback
> >> > > if
> >> > > > we made the login callback handler configurable.
> >> > > >
> >> > > > Regards,
> >> > > >
> >> > > > Rajini
> >> > > >
> >> > > >
> >> > > > On Thu, Feb 22, 2018 at 4:16 AM, Ron Dagostino <rndgstn@gmail.com
> >
> >> > > wrote:
> >> > > >
> >> > > > > Hi everyone. I implemented the ability to perform substitution
> in
> >> > JAAS
> >> > > > > config module options, which was the only part of KIP 255 that
> was
> >> > not
> >> > > > > implemented when I originally published the KIP last week. I
> have
> >> > made
> >> > > > > adjustments to that section of the KIP based on this
> >> implementation
> >> > > > > experience, including detailing how to add custom substitutions
> >> > beyond
> >> > > > the
> >> > > > > 4 built-in ones (file, system property, environment variable,
> and
> >> > > option
> >> > > > > substitution). See
> >> > > > > https://github.com/rondagostino/kafka/commit/548a95822b06b60
> >> > > > > a92745084a3980d72295d2ce6
> >> > > > > (code coverage 82%) for the detailed changes. The KIP code
> blocks
> >> > are
> >> > > > also
> >> > > > > up-to-date.
> >> > > > >
> >> > > > > Ron
> >> > > > >
> >> > > > >
> >> > > > > On Wed, Feb 14, 2018 at 8:11 PM, Ron Dagostino <
> rndgstn@gmail.com
> >> >
> >> > > > wrote:
> >> > > > >
> >> > > > > > Thanks, Ted. I've added the JIRA and mailing list links to
> the
> >> > KIP,
> >> > > > and
> >> > > > > I
> >> > > > > > added Javadoc addressing your questions -- both in the KIP
> code
> >> > > blocks
> >> > > > > and
> >> > > > > > on GitHub (https://github.com/rondagostino/kafka/commit/
> >> > > > > > c61f5bafad810b620ff1ebd04e1231d245183e36).
> >> > > > > >
> >> > > > > > Ron
> >> > > > > >
> >> > > > > > On Wed, Feb 14, 2018 at 7:19 PM, Ted Yu <yu...@gmail.com>
> >> > wrote:
> >> > > > > >
> >> > > > > >> Nicely written KIP.
> >> > > > > >>
> >> > > > > >> Can you add link to this thread and fill in JIRA number ?
> >> > > > > >>
> >> > > > > >> For ExpiringCredential, why does expireTimeMillis() return
> long
> >> > > while
> >> > > > > >> other
> >> > > > > >> methods return Long ?
> >> > > > > >> Can you add some comment for WindowJitter in RefreshConfig ?
> >> > > > > >>
> >> > > > > >> Thanks
> >> > > > > >>
> >> > > > > >> On Wed, Feb 14, 2018 at 3:38 PM, Ron Dagostino <
> >> rndgstn@gmail.com
> >> > >
> >> > > > > wrote:
> >> > > > > >>
> >> > > > > >> > Hi everyone.
> >> > > > > >> >
> >> > > > > >> > I created KIP-255: OAuth Authentication via
> SASL/OAUTHBEARER
> >> > > > > >> > <https://cwiki.apache.org/confluence/pages/viewpage.action
> ?
> >> > > > > >> pageId=75968876
> >> > > > > >> > >
> >> > > > > >> > (https://cwiki.apache.org/confluence/pages/viewpage.
> >> > > > > >> > action?pageId=75968876
> >> > > > > >> > ).
> >> > > > > >> >
> >> > > > > >> > This KIP proposes adding the ability to authenticate to
> Kafka
> >> > with
> >> > > > > >> OAuth 2
> >> > > > > >> > bearer tokens using the OAUTHBEARER SASL mechanism.
> >> > > > > >> >
> >> > > > > >> > The code blocks in the KIP all reflect the implementation
> at
> >> > > > > >> > https://github.com/rondagostino/kafka/tree/KAFKA-
> 4292_plus_
> >> > > > > oauthbearer.
> >> > > > > >> > Feel free to look there for more details if the code blocks
> >> whet
> >> > > > your
> >> > > > > >> > appetite for more. Everything described in the KIP as
> >> currently
> >> > > > > worded
> >> > > > > >> is
> >> > > > > >> > implemented there (77% code coverage with tests) except for
> >> the
> >> > > > > ability
> >> > > > > >> to
> >> > > > > >> > perform substitution in JAAS config module options.
> >> > > > > >> >
> >> > > > > >> > Ron
> >> > > > > >> >
> >> > > > > >>
> >> > > > > >
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>