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