Hi Ron, Thanks for the summary - that matches my understanding. It is a good idea to support unsecuredLoginExtension_<extensionName>=value for the default implementation and that would make it easy to test the KIP. Also agree with extension name restrictions, we should keep the patterns in the initial client response as-is to conform to the spec.
Regards, Rajini On Thu, Jul 19, 2018 at 5:49 PM, Ron Dagostino <rndg...@gmail.com> wrote: > Hi Rajini and Stanislav. Rajini, yes, I think you are right about the > login callback handler being more appropriate for retrieving the SASL > extensions than the login module itself (how many times am I going to have > to be encouraged to leverage the callback handlers?!? lol). > OAuthBearerLoginModule should ask its login callback handler to handle an > instance of SaslExtensionsCallback in addition to an instance of > OAuthBearerTokenCallback, and the default login callback handler > implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either > return an empty map via callback or it should recognize additional JAAS > module options of the form unsecuredLoginExtension_<extensionName>=value > so > that arbitrary extensions can be added in development and test scenarios > (similar to how arbitrary claims on unsecured tokens can be created in > those scenarios via the JAAS module options > unsecuredLoginStringClaim_<claimName>=value, etc.). Then the > OAuthBearerLoginModule can add a map of any extensions to the Subject's > public credentials where the default SASL client callback handler class ( > OAuthBearerSaslClientCallbackHandler) can be amended to support > SaslExtensionsCallback and look on the Subject accordingly. There would be > no need to implement a custom sasl.client.callback.handler.class in this > case, and no logic would need to be moved to a public static method on > OAuthBearerLoginModule as I had proposed (at least not right now, anyway -- > there may come a time when a need for a custom > sasl.client.callback.handler.class is identified, and at that point the > default implementation would either have to made part of the public API > with protected rather than private methods so it could be directly extended > or its logic would have to be moved to public static methods on > OAuthBearerLoginModule). > > So, to try to summarize, I think SaslExtensionsCallback will be the only > public API addition due to this KIP in terms of code, and then maybe the > recognition of the unsecuredLoginExtension_<extensionName>=value module > options in the default unsecured case (which would be a documentation > change and an internal implementation issue rather than a public API in > terms of code). And then also the fact that extension names and values are > accessed on the server side via negotiated properties. Do I have that > summary right? > > One thing I want to note is that the code needs to make sure the extension > names are composed of only ALPHA [a-zA-Z] characters as per the spec (not > only for that reason, but to also make sure the token available at the > OAUTHBEARER.token negotiated property can't be overwritten). > > Ron > > On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski < > stanis...@confluent.io> > wrote: > > > Hey Ron, > > > > Come to think of it, I think what Rajini said makes more sense than my > > initial proposal. Having the OAuthBearerClientCallbackHandler populate > > SaslExtensionsCallback by taking a Map from the Subject would ease users' > > implementation - they'd only have to provide a login callback handler > which > > attaches extensions to the Subject. > > I will now update the PR and the examples in the KIP. Let me know what > you > > think > > > > Hi Rajini, > > Yes, I will switch both classes to private/public as it makes total > sense. > > > > Best, > > Stanislav > > On Thu, Jul 19, 2018 at 9:02 AM Rajini Sivaram <rajinisiva...@gmail.com> > > wrote: > > > > > Hi Stanislav, > > > > > > Thanks for the KIP. Since SaslExtensions will be an internal class, can > > we > > > remove it from the KIP to avoid confusion? Also, can we add the package > > > name for SaslExtensionsCallback? The PR has it in > > > org.apache.kafka.common.security which is an internal package. As a > > public > > > class, it could be in org.apache.kafka.common.security.auth. > > > > > > Regards, > > > > > > Rajini > > > > > > On Thu, Jul 19, 2018 at 4:50 PM, Rajini Sivaram < > rajinisiva...@gmail.com > > > > > > wrote: > > > > > > > Hi Ron, > > > > > > > > Is there a reason why wouldn't want to provide extensions using a > login > > > > callback handler in the same way as we inject tokens? The easiest way > > to > > > > inject custom extensions would be using the JAAS config. So we could > > have > > > > both OAuthBearerTokenCallback and SaslExtensionsCallback processed > by > > a > > > > login callback handler. And the map returned by > SaslExtensionsCallback > > > > could be added to Subject by the default > > > OAuthBearerSaslClientCallbackHandler. > > > > Since OAuth users have to provide a login callback handler anyway, > > > wouldn't > > > > it be a better fit? > > > > > > > > Thank you, > > > > > > > > Rajini > > > > > > > > > > > > On Thu, Jul 19, 2018 at 4:08 PM, Ron Dagostino <rndg...@gmail.com> > > > wrote: > > > > > > > >> Hi Stanislav. > > > >> > > > >> Implementers of a custom sasl.client.callback.handler.class must be > > sure > > > >> to > > > >> provide the existing logic in > > > >> org.apache.kafka.common.security.oauthbearer.internals.OAuth > > > >> BearerSaslClientCallbackHandler > > > >> that handles instances of OAuthBearerTokenCallback (by retrieving > the > > > >> private credential from the Subject); a custom implementation that > > fails > > > >> to > > > >> do this will not work, so the KIP should state this requirement. > > > >> > > > >> The question then arises: how should implementers provide the > existing > > > >> logic in the OAuthBearerSaslClientCallbackHandler class? That > class > > is > > > >> not > > > >> part of the public API, and its > > handleCallback(OAuthBearerTokenCallback) > > > >> method, which implements the logic, is private anyway (so even if > > > someone > > > >> took the risk of extending the non-API class the method would > > generally > > > >> not > > > >> be available in the subclass). So as it stands right now > implementers > > > are > > > >> left to copy/paste that logic into their code. A better solution > > might > > > be > > > >> to have the private method in OAuthBearerSaslClientCallbackHandler > > > >> invoke a > > > >> public static method on the > > > >> org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule > > > class > > > >> (which is part of the public API) to retrieve the credential (e.g. > > > public > > > >> static OAuthBearerToken retrieveCredential(Subject)) . The commit() > > > >> method > > > >> of the OAuthBearerLoginModule class is what puts the credential > there > > in > > > >> the first place, so it could make sense for the class to expose the > > > >> complementary logic for retrieving the credential in this way. > > > Regarding > > > >> your question about plugability of LoginModules, yes, the > LoginModule > > > >> class > > > >> is explicitly stated in the JAAS config, so it is indeed pluggable; > an > > > >> extending class would override the commit() method, call > > super.commit(), > > > >> and if the return value is true it would do whatever is necessary to > > add > > > >> the desired SASL extensions to the Subject -- probably in the public > > > >> credentials -- where a custom sasl.client.callback.handler.class > would > > > be > > > >> able to find them. The KIP might state this, too. > > > >> > > > >> I'll look forward to seeing a new PR once the fix for the 0x01 > > separator > > > >> issue in the SASL/OAUTHBEARER implementation (KAFKA-7182 > > > >> <https://issues.apache.org/jira/projects/KAFKA/issues/KAFKA-7182>) > is > > > >> merged. > > > >> > > > >> Ron > > > >> > > > >> On Wed, Jul 18, 2018 at 6:38 PM Stanislav Kozlovski < > > > >> stanis...@confluent.io> > > > >> wrote: > > > >> > > > >> > Hey Ron, > > > >> > > > > >> > You brought up some great points. I did my best to address them > and > > > >> updated > > > >> > the KIP. > > > >> > > > > >> > I should mention that I used commas to separate extensions in the > > > >> protocol, > > > >> > because we did not use the recommended Control-A character for > > > >> separators > > > >> > in the OAuth message and I figured I would not change it. > > > >> > Now that I saw your PR about implementing the proper separators in > > > OAUTH > > > >> > <https://github.com/apache/kafka/pull/5391> and will change my > > > >> > implementation once yours gets merged, meaning commas will be a > > > >> supported > > > >> > value for extensions. > > > >> > > > > >> > About the implementation: yes you're right, you should define ` > > > >> > sasl.client.callback.handler.class` which has the same > functionality > > > >> as ` > > > >> > OAuthBearerSaslClientCallbackHandler` plus the additional > > > >> functionality of > > > >> > handling the `SaslExtensionsCallback` by attaching extensions to > it. > > > >> > The only reason you'd populate the `Subject` object with the > > > extensions > > > >> is > > > >> > if you used the default `SaslClientCallbackHandler` (which handles > > the > > > >> > extensions callback by adding whatever's in the subject), as the > > SCRAM > > > >> > authentication does. > > > >> > > > > >> > https://github.com/stanislavkozlovski/kafka/blob/KAFKA-7169- > > > >> custom-sasl-extensions/clients/src/main/java/org/ > > > >> apache/kafka/common/security/oauthbearer/internals/OAuthBea > > > >> rerSaslClient.java#L92 > > > >> > And yes, in that case you would need a custom `LoginModule` which > > > >> populates > > > >> > the Subject in that case, although I'm not sure if Kafka supports > > > >> pluggable > > > >> > LoginModules. Does it? > > > >> > > > > >> > Best, > > > >> > Stanislav > > > >> > > > > >> > On Wed, Jul 18, 2018 at 9:50 AM Ron Dagostino <rndg...@gmail.com> > > > >> wrote: > > > >> > > > > >> > > Hi Stanislav. Could you add something to the KIP about the > > security > > > >> > > implications related to the CSV name/value pairs sent in the > > > >> extension? > > > >> > > For example, the OAuth access token may have a digital > signature, > > > but > > > >> the > > > >> > > extensions generally will not (unless one of the values is a JWS > > > >> compact > > > >> > > serialization, but I doubt anyone would go that far), so the > > server > > > >> > > generally cannot trust the extensions to be accurate for > anything > > > >> > > critical. You mentioned the "better tracing and > troubleshooting" > > > use > > > >> > case, > > > >> > > which I think is fine despite the lack of security; given that > > lack > > > of > > > >> > > security, though, I believe it is important to also state what > the > > > >> > > extensions should *not* be used for. > > > >> > > > > > >> > > Also, could you indicate in the KIP how the extensions might > > > actually > > > >> be > > > >> > > added? My take on that would be to extend > OAuthBearerLoginModule > > to > > > >> > > override the initialize() and commit() methods so that the > derived > > > >> class > > > >> > > would have access to the Subject instance and could add a map to > > the > > > >> > > subject's public or private credentials when the commit > succeeds; > > > >> then I > > > >> > > think the sasl.client.callback.handler.class would have to be > > > >> explicitly > > > >> > > set to a class that extends the default implementation > > > >> > > (OAuthBearerSaslClientCallbackHandler) and retrieves the map > when > > > >> > handling > > > >> > > the SaslExtensionsCallback. But maybe you are thinking about it > > > >> > > differently? Some guidance on how to actually take advantage of > > the > > > >> > > feature via an implementation would be a useful addition to the > > KIP. > > > >> > > > > > >> > > Finally, I note that the extension parsing does not support a > > comma > > > in > > > >> > keys > > > >> > > or values. This should be addressed somehow -- either by > > supporting > > > >> via > > > >> > an > > > >> > > escaping mechanism or by explicitly acknowledging that it is > > > >> unsupported. > > > >> > > > > > >> > > Thanks for the KIP and the simultaneous PR -- having both at the > > > same > > > >> > time > > > >> > > really helped. > > > >> > > > > > >> > > Ron > > > >> > > > > > >> > > On Tue, Jul 17, 2018 at 6:22 PM Stanislav Kozlovski < > > > >> > > stanis...@confluent.io> > > > >> > > wrote: > > > >> > > > > > >> > > > Hey group, > > > >> > > > > > > >> > > > I just created a new KIP about adding customizable SASL > > extensions > > > >> to > > > >> > the > > > >> > > > OAuthBearer authentication mechanism. More details in the > > proposal > > > >> > > > > > > >> > > > KIP: > > > >> > > > > > > >> > > > > > > >> > > > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-342% > > > >> > > 3A+Add+support+for+Custom+SASL+extensions+in+OAuthBearer+authentication > > > >> > > > JIRA: KAFKA-7169 < > > > https://issues.apache.org/jira/browse/KAFKA-7169> > > > >> > > > PR: Pull request <https://github.com/apache/kafka/pull/5379> > > > >> > > > > > > >> > > > -- > > > >> > > > Best, > > > >> > > > Stanislav > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> > -- > > > >> > Best, > > > >> > Stanislav > > > >> > > > > >> > > > > > > > > > > > > > > > > > -- > > Best, > > Stanislav > > >
