Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi, I've written up an initial implementation of what has been discussed. Take a look at it here: https://github.com/apache/kafka/pull/5497/ I will make sure to update the KIP once a review of the PR passes On Mon, Aug 13, 2018 at 10:19 AM Rajini Sivaram wrote: > Hi Stanislav, > > I think

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav, I think `token` and `extensions` on `OAuthBearerExtensionsValidatorCallback` should be immutable. The getters should return whatever was provided in the constructor and these should be stored as `final` objects. The whole point of separating out

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi, @Ron Agreed, tracking multiple errors would be better and would help diagnose bad extensions faster I've updated the KIP to address your two comments. Regarding the Javadoc, please read below: @Rajini The idea of the potentially-null token and extensions is not that they can be passed to the

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav, For the point that Ron made above for: public OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token, SaslExtensions extensions) I don't think we should ever invoke extensions callback without the token. We can first validate the token and invoke extensions callback only

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. Here are a few KIP comments. <

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini, Ron I've updated the KIP with the latest changes following our discussion. Please do give it a read. If you feel it is alright, I will follow up with a PR later. Best, Stanislav On Thu, Aug 9, 2018 at 10:09 AM Rajini Sivaram wrote: > Hi Ron/Stansilav, > > OK, let's just go with 2.

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron/Stansilav, OK, let's just go with 2. I think it would be better to add a OAuth-specific extensions handler OAuthBearerExtensionsValidatorCallback that provides OAuthBearerToken. To summarise, we chose option 2 out of these four options: 1. {OAuthBearerValidatorCallback,

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini. I think we are considering the following two options. Let me try to describe them along with their relative advantages/disadvantages. Option #1: Send two callbacks in a single array to the callback handler: ch.handle(new Callback[] {tokenCallback, extensionsCallback}); Option

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Yes, I was thinking of a SaslExtensionsValidatorCallback with additional context as well initially, but I didn't like the idea of name-value pairs and I didn't want generic objects passed around through the callback So providing context through other callbacks felt like a neater fit.

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, I was also thinking along the lines of send the callbacks separately. Initially I thought the `CallbackHandler` can simply maintain state but that's not a good idea since it'll be dependent on the implementation of the `SaslServer` rather than the interface. Having the

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini. I also like that idea, but I think it might rely on one or possibly two implicit assumptions that I'm not sure are guaranteed to be true. First, I'm not sure if the CallbackHandler interface guarantees that implementations must process callbacks in order. Second (and more plausibly

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini, That approach makes more sense to me. I like it On Wed, Aug 8, 2018 at 5:35 PM Rajini Sivaram wrote: > Hi Ron/Stanislav, > > Do you think it makes sense to separate out OAuthBearerValidatorCallback > and SaslExtensionsValidatorCallback so that it is clearer that these are > two

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron/Stanislav, Do you think it makes sense to separate out OAuthBearerValidatorCallback and SaslExtensionsValidatorCallback so that it is clearer that these are two separate entities that need validation? When we add support for customisable extensions in other mechanisms, we could reuse

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Yes, I agree we should document it thoroughly On Wed, Aug 8, 2018 at 5:02 PM Ron Dagostino wrote: > Hi Stanislav. If the community agrees we should add it then we should at a > minimum add explicit warnings in the Javadoc making it very clear how this > should not be used. > > Ron > >

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. If the community agrees we should add it then we should at a minimum add explicit warnings in the Javadoc making it very clear how this should not be used. Ron On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski wrote: > Hey Ron, > > I fully agree that token validation is a

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hey Ron, I fully agree that token validation is a serious security operation. Although, I believe allowing the user to do more validation with the extensions does not hurt - the user is fully responsible for his security once he starts implementing custom code for token validation. You would

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. If you wanted to do this a good way might be to add a constructor to the org.apache.kafka.common.security.oauthbearer. OAuthBearerValidatorCallback class that accepts a SaslExtensions instance in addition to a token value. Then it would give the callback handler the option to

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hey everybody, Sorry for reviving this, but I neglected something the first time around. I believe it would be useful to provide the `OAuthBearerUnsecuredValidatorCallbackHandler` with the OAuth extensions too. This would enable use cases where validators want to reconcile information from the

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Looks good. Thanks, Stanislav. On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski wrote: > Hi Rajini, > > I updated the KIP. Please check if the clarification is okay > > On Wed, Jul 25, 2018 at 10:49 AM Rajini Sivaram > wrote: > > > Hi Stanislav, > > > > 1. Can you clarify the following

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini, I updated the KIP. Please check if the clarification is okay On Wed, Jul 25, 2018 at 10:49 AM Rajini Sivaram wrote: > Hi Stanislav, > > 1. Can you clarify the following line in the KIP in the 'Public Interfaces' > section? When you are reading the KIP for the first time, it sounds

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav, 1. Can you clarify the following line in the KIP in the 'Public Interfaces' section? When you are reading the KIP for the first time, it sounds like we adding a new Kafka config. But we are adding JAAS config options with a prefix that can be used with the default unsecured bearer

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Thanks for the suggestions. I have applied them to the KIP. On Tue, Jul 24, 2018 at 1:39 PM Ron Dagostino wrote: > Hi Stanislav. The statement "New config option for OAuthBearerLoginModule" > is technically incorrect; it should be "New config option for default, > unsecured bearer

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. The statement "New config option for OAuthBearerLoginModule" is technically incorrect; it should be "New config option for default, unsecured bearer tokens" since that is what provides the functionality (as opposed to the login module, which does not). Also, please state that

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hey everybody, I have updated the KIP to reflect the latest changes as best as I could. If there aren't more suggestions, I intent to start the [VOTE] thread tomorrow. Best, Stanislav On Tue, Jul 24, 2018 at 6:34 AM Ron Dagostino wrote: > Hi Stanislav. Could you update the KIP to reflect the

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. Could you update the KIP to reflect the latest definition of SaslExtensions and confirm or correct the impact it has to the SCRAM-related classes? I'm not sure if the currently-described impact is still accurate. Also, could you mention the changes to

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini. I think a class is fine as long as we make sure the semantics of immutability are clear -- it would have to be a value class, and any constructor that accepts a Map as input would have to copy that Map rather than store it in a member variable. Similarly, any Map that it might return

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Stanislav, I agree with Stanislav that it would be better to leave `SaslExtensions` as a class rather than make it an interface. We don''t really expect users to extends this class, so it is convenient to have an implementation since users need to create an instance. The class provided by

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. See https://tools.ietf.org/html/rfc7628#section-3.1, and that section refers to the core ABNF productions defined in https://tools.ietf.org/html/rfc5234#appendix-B. Ron > On Jul 23, 2018, at 1:30 AM, Stanislav Kozlovski > wrote: > > Hey Ron and Rajini, > > Here are my

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hey Ron and Rajini, Here are my thoughts: Regarding separators in SaslExtensions - Agreed, that was a bad move. Should definitely not be a concern of CallbackHandler and LoginModule implementors. SaslExtensions interface - Wouldn't implementing it as an interface mean that users will have to make

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi again, Rajini and Stanislav. I wonder if making SaslExtensions an interface rather than a class might be a good solution. For example: public interface SaslExtensions { /** * @return an immutable map view of the SASL extensions */ Map map(); } This solves the issue of lack of

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Rajini. The SaslServer is going to have to validate the extensions, too, but I’m okay with keeping the validation logic elsewhere as long as it can be reused in both the client and the secret. I strongly prefer exposing a map() method as opposed to extensionNames() and

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hmm I think we need a much simpler SaslExtensions class if we are making it part of the public API. 1. I don't see the point of including separator anywhere in SaslExtensions. Extensions provide a map and we propagate the map from client to server using the protocol associated with the

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav and Rajini. If SaslExtensions is going to part of the public API, then it occurred to me that one of the requirements of all SASL extensions is that the keys and values need to match mechanism-specific regular expressions. For example, RFC 5802 ( https://tools.ietf.org/html/rfc5802)

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, I saw that and decided that would be the best approach. The current ScramExtensions implementation uses a Map in the public credentials and I thought I would follow convention rather than introduce my own thing, but maybe this is best On Fri, Jul 20, 2018 at 8:39 AM Ron Dagostino wrote:

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Stanislav. I'm wondering if we should make SaslExtensions part of the public API. I mentioned this in my review of the PR, too (and tagged Rajini to get her input). If we add a Map to the Subject's public credentials we are basically making a public commitment that any Map associated with

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Thanks for the summary - that matches my understanding. It is a good idea to support unsecuredLoginExtension_=value for the default implementation and that would make it easy to test the KIP. Also agree with extension name restrictions, we should keep the patterns in the initial client

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

Hi Ron, Sorry, I meant that the login callback handler would populate Subject with extensions (along with OAuthBearerToken) using a custom login callback handler and the default OAuthBearerSaslClientCallbackHandler would obtain extensions along with OAuthBearerToken from Subject. Thanks to

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

Re: [DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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

[DISCUSS] KIP-342 Add Customizable SASL extensions to OAuthBearer authentication

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: