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 >
