Hi Ron, Agreed. `SaslExtensionsCallback` will be the only public API addition and new documentation for the extension strings. A question that came up - should the LoginCallbackHandler throw an exception or simply ignore key/value extension pairs who do not match the validation regex pattern? I guess it would be better to throw, as to avoid confusion.
And yes, I will make sure the key/value are validated on the client as well as in the server. Even then, I structured the getNegotiatedProperty() method such that the OAUTHBEARER.token can never be overridden. I considered adding a test for that, but I figured having the regex validation be enough of a guarantee. On Thu, Jul 19, 2018 at 9:49 AM 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 > > > -- Best, Stanislav
