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 > > >> > > > >> > > > > > > > > >
