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 <rndg...@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 <rajinisiva...@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 <rndg...@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 < > rajinisiva...@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 <rndg...@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 <rndg...@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 <yuzhih...@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 < > rndg...@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 > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > >
