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/OAuthBearerSaslClient.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
