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