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 >> > >> > >
