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
