Hi Ron, I saw that and decided that would be the best approach. The current ScramExtensions implementation uses a Map in the public credentials and I thought I would follow convention rather than introduce my own thing, but maybe this is best
On Fri, Jul 20, 2018 at 8:39 AM Ron Dagostino <rndg...@gmail.com> wrote: > 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 > > > -- Best, Stanislav
