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