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
