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/ExpiringCredentialRefreshingLogin: > 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 > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >
