Hi Stanislav. I'm wondering if we should make SaslExtensions part of the public API. I mentioned this in my review of the PR, too (and tagged Rajini to get her input). If we add a Map to the Subject's public credentials we are basically making a public commitment that any Map associated with the public credentials defines the SASL extensions and we can never add another instance implementing Map to the public credentials. That's a very big constraint we are committing to, and I'm wondering if we should make SaslExtensions public and attach an instance of that to the Subject's public credentials instead.
Ron On Thu, Jul 19, 2018 at 8:15 PM Stanislav Kozlovski <stanis...@confluent.io> wrote: > I have updated the PR and KIP to address the comments made so far. Please > take another look at them and share your thoughts. > KIP: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-342%3A+Add+support+for+Custom+SASL+extensions+in+OAuthBearer+authentication > PR: Pull request <https://github.com/apache/kafka/pull/5379> > > Best, > Stanislav > > On Thu, Jul 19, 2018 at 1:58 PM Stanislav Kozlovski < > stanis...@confluent.io> > wrote: > > > 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 > > > > > -- > Best, > Stanislav >
