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

[Stanislav Kozlovski]

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 `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 `OAuthBearerExtensionsValidatorCallback` from
> `OAuthBearerValidatorCallback`
> was to ensure that tokens are securely validated and created without any
> reference to insecure extensions. So it is critical that
> `OAuthBearerSaslServer` never uses the token returned by the extensions
> callback. As for the extensions, we should have two methods -
> {`extensions()`, `validatedExtensions()`} as I suggested in the last note
> OR {`defaultExtensions()`, `extensions()`} as used in NameCallback. I don't
> think we should make extensions mutable to use the same value for input and
> output. Btw, on error, we shouldn't care about the values in the returned
> extensions at all, we should simply fail authentication.
>
>
> On Sat, Aug 11, 2018 at 1:21 PM, Stanislav Kozlovski <
> stanis...@confluent.io
> > wrote:
>
> > 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 constructor - it is that they can be nullified on a
> > validation error occurring (like it is done in the #error() method on
> > `OAuthBearerValidatorCallback`). Maybe it doesn't make too much sense to
> > nullify the token, but I believe it is worth it to do the same with the
> > extensions.
> >
> > I agree that the callback handler should himself populate the callback
> with
> > the *validated* extensions only. Will change implementation and KIP in
> due
> > time.
> >
> > Please share what you think about nullifying token/extensions on
> validation
> > error.
> >
> > Best,
> > Stanislav
> >
> >
> > On Fri, Aug 10, 2018 at 7:24 PM Rajini Sivaram 
> > wrote:
> >
> > > 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 if
> > > token is non-null. Can we clarify that in the javadoc?
> > >
> > >- public SaslExtensions extensions() : Extensions should be non-null
> > >- public OAuthBearerToken token() : Token should be non-null
> > >
> > >
> > > Also agree with Ron that we should have the ability to return errors
> for
> > > all invalid extensions, even if a callback handler may choose to stop
> on
> > > first failure.
> > >
> > > I think we also need another method to return the extensions that were
> > > validated and will be made available as negotiated properties. As per
> the
> > > RFC, server should ignore unknown extensions. So callback handlers need
> > to
> > > be able to validate the ones they know of and return those. Other
> > > extensions should not be added to the SaslServer's negotiated
> properties.
> > >
> > >- public SaslExtensions validatedExtensions()
> > >
> > >
> > >
> > > On Fri, Aug 10, 2018 at 3:26 PM, Ron Dagostino 
> > wrote:
> > >
> > > > Hi Stanislav.  Here are a few KIP comments.
> > > >
> > > > << > > > values to ensure they conform to the OAuth standard
> > > > It is the SASL/OAUTHBEARER standard that defines the regular
> > expressions
> > > > (specifically, https://tools.ietf.org/html/rfc7628#section-3.1)
> rather
> > > > than
> > > > any of the OAuth specifications.  It would be good to make this
> > > > clarification.
> > > >
> > > > << > token,
> > > > SaslExtensions extensions)
> > > > This constructor lacks Javadoc in the KIP.  Could you add it, and
> also
> > > > indicate which of the two parameters are required vs. optional?  The
> > > > Javadoc for the token() method indicates that the return value could
> be
> > > > null, but that would only be true if the constructor accepted a null
> > > value
> > > > for the token.  I'm okay with the constructor accepting a null token
> > > > (Rajini, you may differ in opinion, in which case I defer to your
> > > > preference).  But please do clarify this issue.
> > > >
> > > > I also am not sure if exposing just one invalid extension name and
> > error
> > > > message in the OAuthBearerExtensionsValidatorCallback class is good
> > > > enough.  An alternative to invalidExtensionName() and errorMessage()
> > > > methods would be to return an always 

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

[Rajini Sivaram]

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 `OAuthBearerExtensionsValidatorCallback` from
`OAuthBearerValidatorCallback`
was to ensure that tokens are securely validated and created without any
reference to insecure extensions. So it is critical that
`OAuthBearerSaslServer` never uses the token returned by the extensions
callback. As for the extensions, we should have two methods -
{`extensions()`, `validatedExtensions()`} as I suggested in the last note
OR {`defaultExtensions()`, `extensions()`} as used in NameCallback. I don't
think we should make extensions mutable to use the same value for input and
output. Btw, on error, we shouldn't care about the values in the returned
extensions at all, we should simply fail authentication.


On Sat, Aug 11, 2018 at 1:21 PM, Stanislav Kozlovski  wrote:

> 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 constructor - it is that they can be nullified on a
> validation error occurring (like it is done in the #error() method on
> `OAuthBearerValidatorCallback`). Maybe it doesn't make too much sense to
> nullify the token, but I believe it is worth it to do the same with the
> extensions.
>
> I agree that the callback handler should himself populate the callback with
> the *validated* extensions only. Will change implementation and KIP in due
> time.
>
> Please share what you think about nullifying token/extensions on validation
> error.
>
> Best,
> Stanislav
>
>
> On Fri, Aug 10, 2018 at 7:24 PM Rajini Sivaram 
> wrote:
>
> > 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 if
> > token is non-null. Can we clarify that in the javadoc?
> >
> >- public SaslExtensions extensions() : Extensions should be non-null
> >- public OAuthBearerToken token() : Token should be non-null
> >
> >
> > Also agree with Ron that we should have the ability to return errors for
> > all invalid extensions, even if a callback handler may choose to stop on
> > first failure.
> >
> > I think we also need another method to return the extensions that were
> > validated and will be made available as negotiated properties. As per the
> > RFC, server should ignore unknown extensions. So callback handlers need
> to
> > be able to validate the ones they know of and return those. Other
> > extensions should not be added to the SaslServer's negotiated properties.
> >
> >- public SaslExtensions validatedExtensions()
> >
> >
> >
> > On Fri, Aug 10, 2018 at 3:26 PM, Ron Dagostino 
> wrote:
> >
> > > Hi Stanislav.  Here are a few KIP comments.
> > >
> > > << > > values to ensure they conform to the OAuth standard
> > > It is the SASL/OAUTHBEARER standard that defines the regular
> expressions
> > > (specifically, https://tools.ietf.org/html/rfc7628#section-3.1) rather
> > > than
> > > any of the OAuth specifications.  It would be good to make this
> > > clarification.
> > >
> > > << token,
> > > SaslExtensions extensions)
> > > This constructor lacks Javadoc in the KIP.  Could you add it, and also
> > > indicate which of the two parameters are required vs. optional?  The
> > > Javadoc for the token() method indicates that the return value could be
> > > null, but that would only be true if the constructor accepted a null
> > value
> > > for the token.  I'm okay with the constructor accepting a null token
> > > (Rajini, you may differ in opinion, in which case I defer to your
> > > preference).  But please do clarify this issue.
> > >
> > > I also am not sure if exposing just one invalid extension name and
> error
> > > message in the OAuthBearerExtensionsValidatorCallback class is good
> > > enough.  An alternative to invalidExtensionName() and errorMessage()
> > > methods would be to return an always non-null but potentially empty
> > > Map so that potentially all of the provided extensions
> > > could be validated and the list of invalid extension names could be
> > > returned (along with the error message for each of them).  If we
> adopted
> > > this alternative then the error(String invalidExtensionName, String
> > > errorMessage) method might need to be renamed addError(String
> > > invalidExtensionName, String errorMessage).  I suspect it would be
> better
> > > to go with the map approach to support returning multiple error
> 

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

[Stanislav Kozlovski]

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 constructor - it is that they can be nullified on a
validation error occurring (like it is done in the #error() method on
`OAuthBearerValidatorCallback`). Maybe it doesn't make too much sense to
nullify the token, but I believe it is worth it to do the same with the
extensions.

I agree that the callback handler should himself populate the callback with
the *validated* extensions only. Will change implementation and KIP in due
time.

Please share what you think about nullifying token/extensions on validation
error.

Best,
Stanislav


On Fri, Aug 10, 2018 at 7:24 PM Rajini Sivaram 
wrote:

> 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 if
> token is non-null. Can we clarify that in the javadoc?
>
>- public SaslExtensions extensions() : Extensions should be non-null
>- public OAuthBearerToken token() : Token should be non-null
>
>
> Also agree with Ron that we should have the ability to return errors for
> all invalid extensions, even if a callback handler may choose to stop on
> first failure.
>
> I think we also need another method to return the extensions that were
> validated and will be made available as negotiated properties. As per the
> RFC, server should ignore unknown extensions. So callback handlers need to
> be able to validate the ones they know of and return those. Other
> extensions should not be added to the SaslServer's negotiated properties.
>
>- public SaslExtensions validatedExtensions()
>
>
>
> On Fri, Aug 10, 2018 at 3:26 PM, Ron Dagostino  wrote:
>
> > Hi Stanislav.  Here are a few KIP comments.
> >
> > << > values to ensure they conform to the OAuth standard
> > It is the SASL/OAUTHBEARER standard that defines the regular expressions
> > (specifically, https://tools.ietf.org/html/rfc7628#section-3.1) rather
> > than
> > any of the OAuth specifications.  It would be good to make this
> > clarification.
> >
> > << > SaslExtensions extensions)
> > This constructor lacks Javadoc in the KIP.  Could you add it, and also
> > indicate which of the two parameters are required vs. optional?  The
> > Javadoc for the token() method indicates that the return value could be
> > null, but that would only be true if the constructor accepted a null
> value
> > for the token.  I'm okay with the constructor accepting a null token
> > (Rajini, you may differ in opinion, in which case I defer to your
> > preference).  But please do clarify this issue.
> >
> > I also am not sure if exposing just one invalid extension name and error
> > message in the OAuthBearerExtensionsValidatorCallback class is good
> > enough.  An alternative to invalidExtensionName() and errorMessage()
> > methods would be to return an always non-null but potentially empty
> > Map so that potentially all of the provided extensions
> > could be validated and the list of invalid extension names could be
> > returned (along with the error message for each of them).  If we adopted
> > this alternative then the error(String invalidExtensionName, String
> > errorMessage) method might need to be renamed addError(String
> > invalidExtensionName, String errorMessage).  I suspect it would be better
> > to go with the map approach to support returning multiple error messages
> > even if the default unsecured token validator implementation only adds
> the
> > first invalid extension name -- at least it would allow others to be more
> > complete if they wish.  It might also be worth discussing whether a has
> > Error() method would be appropriate to add (returning true if the map is
> > non-empty).  I don't have a strong preference on the issue of supporting
> 1
> > vs. multiple errors (though I lean slightly towards supporting multiple
> > errors).  I defer to the preference of others in this regard.
> >
> > Finally, now that we are actually validating extensions, the comment that
> > "An attempt to use [auth] will result in an exception" might cause
> > confusion and perhaps needs to be clarified to state that the exception
> > occurs on the client side before the extensions are sent to the server
> > rather than during extension validation on the server side (e.g. "An
> > attempt to send [auth] will result in an exception on the client").
> >
> > Ron
> >
> >
> > On Fri, Aug 10, 2018 at 7:22 AM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > Hi Rajini, Ron
> > >
> > > I've updated the KIP with the latest changes following our discussion.
> > > Please do 

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

[Rajini Sivaram]

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 if
token is non-null. Can we clarify that in the javadoc?

   - public SaslExtensions extensions() : Extensions should be non-null
   - public OAuthBearerToken token() : Token should be non-null


Also agree with Ron that we should have the ability to return errors for
all invalid extensions, even if a callback handler may choose to stop on
first failure.

I think we also need another method to return the extensions that were
validated and will be made available as negotiated properties. As per the
RFC, server should ignore unknown extensions. So callback handlers need to
be able to validate the ones they know of and return those. Other
extensions should not be added to the SaslServer's negotiated properties.

   - public SaslExtensions validatedExtensions()



On Fri, Aug 10, 2018 at 3:26 PM, Ron Dagostino  wrote:

> Hi Stanislav.  Here are a few KIP comments.
>
> << values to ensure they conform to the OAuth standard
> It is the SASL/OAUTHBEARER standard that defines the regular expressions
> (specifically, https://tools.ietf.org/html/rfc7628#section-3.1) rather
> than
> any of the OAuth specifications.  It would be good to make this
> clarification.
>
> << SaslExtensions extensions)
> This constructor lacks Javadoc in the KIP.  Could you add it, and also
> indicate which of the two parameters are required vs. optional?  The
> Javadoc for the token() method indicates that the return value could be
> null, but that would only be true if the constructor accepted a null value
> for the token.  I'm okay with the constructor accepting a null token
> (Rajini, you may differ in opinion, in which case I defer to your
> preference).  But please do clarify this issue.
>
> I also am not sure if exposing just one invalid extension name and error
> message in the OAuthBearerExtensionsValidatorCallback class is good
> enough.  An alternative to invalidExtensionName() and errorMessage()
> methods would be to return an always non-null but potentially empty
> Map so that potentially all of the provided extensions
> could be validated and the list of invalid extension names could be
> returned (along with the error message for each of them).  If we adopted
> this alternative then the error(String invalidExtensionName, String
> errorMessage) method might need to be renamed addError(String
> invalidExtensionName, String errorMessage).  I suspect it would be better
> to go with the map approach to support returning multiple error messages
> even if the default unsecured token validator implementation only adds the
> first invalid extension name -- at least it would allow others to be more
> complete if they wish.  It might also be worth discussing whether a has
> Error() method would be appropriate to add (returning true if the map is
> non-empty).  I don't have a strong preference on the issue of supporting 1
> vs. multiple errors (though I lean slightly towards supporting multiple
> errors).  I defer to the preference of others in this regard.
>
> Finally, now that we are actually validating extensions, the comment that
> "An attempt to use [auth] will result in an exception" might cause
> confusion and perhaps needs to be clarified to state that the exception
> occurs on the client side before the extensions are sent to the server
> rather than during extension validation on the server side (e.g. "An
> attempt to send [auth] will result in an exception on the client").
>
> Ron
>
>
> On Fri, Aug 10, 2018 at 7:22 AM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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. I think it would be better to add a
> > > OAuth-specific extensions handler OAuthBearerExtensionsValidator
> Callback
> > > that
> > > provides OAuthBearerToken.
> > >
> > > To summarise, we chose option 2 out of these four options:
> > >
> > >1. {OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback}
> :
> > We
> > >don't want to use multiple ordered callbacks since we don't want the
> > >context of one callback to come from another.callback,
> > >2. OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token,
> > >SaslExtensions ext): This allows extensions to be validated using
> > >context from the token, we are ok with this.
> > >3. SaslExtensionsValidatorCallback(Map context,
> > >SaslExtensions ext): This doesn't really offer any real advantage
> over
> > > 2.
> > 

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

[Ron Dagostino]

Hi Stanislav.  Here are a few KIP comments.

< 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. 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, SaslExtensionsValidatorCallback} :
> We
> >don't want to use multiple ordered callbacks since we don't want the
> >context of one callback to come from another.callback,
> >2. OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token,
> >SaslExtensions ext): This allows extensions to be validated using
> >context from the token, we are ok with this.
> >3. SaslExtensionsValidatorCallback(Map context,
> >SaslExtensions ext): This doesn't really offer any real advantage over
> > 2.
> >4. OAuthBearerValidatorCallback(String token, SaslExtensions ext): We
> >don't want token validator to see extensions since these are insecure
> > but
> >token validation needs to be secure. So we prefer to use a second
> > callback
> >handler to validate extensions after securely validating token.
> >
> >
> >
> > On Wed, Aug 8, 2018 at 8:52 PM, Ron Dagostino  wrote:
> >
> > > 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 #2: Send two callbacks separately, in two separate arrays, to
> the
> > > callback handler:
> > > ch.handle(new Callback[] {tokenCallback});
> > > ch.handle(new Callback[] {extensionsCallback});
> > >
> > > I actually don't see any objective disadvantage with #1.  If we don't
> get
> > > an exception then we know we have the information we need; if we do get
> > an
> > > exception then we can tell if the first callback was handled because
> > either
> > > its token() method or its errorStatus() method will return non-null; if
> > > both return null then we just send the token callback by itself and we
> > > don't publish any extension as negotiated properties.  There is no
> > > possibility of partial results, and I don't think there is a
> performance
> > > penalty due to potential re-validation here, either.
> > >
> > > I  see a subjective disadvantage with #1.  It feels awkward to me to
> > > provide the token as context for extension validation via the first
> > > callback.
> > >
> > > Actually, it just occurred to me why it feels awkward, and I think this
> > is
> > > an objective disadvantage of this approach.  It would be impossible to
> do
> > > extension validation in such a scenario without also doing token
> > validation
> > > first.  We are using the first callback as a way to provide context,
> but
> > we
> > > are also using that first callback to request token validation.  We are
> > > complecting two separate things -- context and a request for validation
> 

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

[Stanislav Kozlovski]

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. 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, SaslExtensionsValidatorCallback} : We
>don't want to use multiple ordered callbacks since we don't want the
>context of one callback to come from another.callback,
>2. OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token,
>SaslExtensions ext): This allows extensions to be validated using
>context from the token, we are ok with this.
>3. SaslExtensionsValidatorCallback(Map context,
>SaslExtensions ext): This doesn't really offer any real advantage over
> 2.
>4. OAuthBearerValidatorCallback(String token, SaslExtensions ext): We
>don't want token validator to see extensions since these are insecure
> but
>token validation needs to be secure. So we prefer to use a second
> callback
>handler to validate extensions after securely validating token.
>
>
>
> On Wed, Aug 8, 2018 at 8:52 PM, Ron Dagostino  wrote:
>
> > 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 #2: Send two callbacks separately, in two separate arrays, to the
> > callback handler:
> > ch.handle(new Callback[] {tokenCallback});
> > ch.handle(new Callback[] {extensionsCallback});
> >
> > I actually don't see any objective disadvantage with #1.  If we don't get
> > an exception then we know we have the information we need; if we do get
> an
> > exception then we can tell if the first callback was handled because
> either
> > its token() method or its errorStatus() method will return non-null; if
> > both return null then we just send the token callback by itself and we
> > don't publish any extension as negotiated properties.  There is no
> > possibility of partial results, and I don't think there is a performance
> > penalty due to potential re-validation here, either.
> >
> > I  see a subjective disadvantage with #1.  It feels awkward to me to
> > provide the token as context for extension validation via the first
> > callback.
> >
> > Actually, it just occurred to me why it feels awkward, and I think this
> is
> > an objective disadvantage of this approach.  It would be impossible to do
> > extension validation in such a scenario without also doing token
> validation
> > first.  We are using the first callback as a way to provide context, but
> we
> > are also using that first callback to request token validation.  We are
> > complecting two separate things -- context and a request for validation
> --
> > into one thing, so this approach has an element of complexity to it.
> >
> > The second option has no such complexity.  If we want to provide context
> to
> > the extension validation then we do that by adding a token to the
> > extensionsCallback instance before we provide it to the callback handler.
> > How we do that -- whether by Map or via a typed getter --
> > feels like a subjective decision, and assuming you agree with the
> > complexity argument and choose option #2, I would defer to your
> preference
> > as to how to implement it.
> >
> > Ron
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > On Wed, Aug 8, 2018 at 3:10 PM Rajini Sivaram 
> > wrote:
> >
> > > 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. There
> > > are pros and cons for both approaches, so we could go with either.
> > >
> > > Callbacks are provided to callback handlers in an array and there is
> > > implicit ordering in the callbacks provided to the callback handler.
> > > In the typical example of {NameCallback, PasswordCallback}, we expect
> > that
> > > ordering so that password callback knows what the user name is. Kafka
> > > guarantees ordering of server callbacks in each of its SASL mechanisms
> > and
> > > this is explicitly stated in
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 86%3A+Configurable+SASL+callback+handlers
> > > .
> > > Until now, we didn't need to worry about ordering for OAuth.
> > >
> > > We currently do not have any optional callbacks - configured callback
> > > handlers 

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

[Rajini Sivaram]

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, SaslExtensionsValidatorCallback} : We
   don't want to use multiple ordered callbacks since we don't want the
   context of one callback to come from another.callback,
   2. OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token,
   SaslExtensions ext): This allows extensions to be validated using
   context from the token, we are ok with this.
   3. SaslExtensionsValidatorCallback(Map context,
   SaslExtensions ext): This doesn't really offer any real advantage over 2.
   4. OAuthBearerValidatorCallback(String token, SaslExtensions ext): We
   don't want token validator to see extensions since these are insecure but
   token validation needs to be secure. So we prefer to use a second callback
   handler to validate extensions after securely validating token.



On Wed, Aug 8, 2018 at 8:52 PM, Ron Dagostino  wrote:

> 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 #2: Send two callbacks separately, in two separate arrays, to the
> callback handler:
> ch.handle(new Callback[] {tokenCallback});
> ch.handle(new Callback[] {extensionsCallback});
>
> I actually don't see any objective disadvantage with #1.  If we don't get
> an exception then we know we have the information we need; if we do get an
> exception then we can tell if the first callback was handled because either
> its token() method or its errorStatus() method will return non-null; if
> both return null then we just send the token callback by itself and we
> don't publish any extension as negotiated properties.  There is no
> possibility of partial results, and I don't think there is a performance
> penalty due to potential re-validation here, either.
>
> I  see a subjective disadvantage with #1.  It feels awkward to me to
> provide the token as context for extension validation via the first
> callback.
>
> Actually, it just occurred to me why it feels awkward, and I think this is
> an objective disadvantage of this approach.  It would be impossible to do
> extension validation in such a scenario without also doing token validation
> first.  We are using the first callback as a way to provide context, but we
> are also using that first callback to request token validation.  We are
> complecting two separate things -- context and a request for validation --
> into one thing, so this approach has an element of complexity to it.
>
> The second option has no such complexity.  If we want to provide context to
> the extension validation then we do that by adding a token to the
> extensionsCallback instance before we provide it to the callback handler.
> How we do that -- whether by Map or via a typed getter --
> feels like a subjective decision, and assuming you agree with the
> complexity argument and choose option #2, I would defer to your preference
> as to how to implement it.
>
> Ron
>
>
>
>
>
>
>
>
>
>
>
>
> On Wed, Aug 8, 2018 at 3:10 PM Rajini Sivaram 
> wrote:
>
> > 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. There
> > are pros and cons for both approaches, so we could go with either.
> >
> > Callbacks are provided to callback handlers in an array and there is
> > implicit ordering in the callbacks provided to the callback handler.
> > In the typical example of {NameCallback, PasswordCallback}, we expect
> that
> > ordering so that password callback knows what the user name is. Kafka
> > guarantees ordering of server callbacks in each of its SASL mechanisms
> and
> > this is explicitly stated in
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 86%3A+Configurable+SASL+callback+handlers
> > .
> > Until now, we didn't need to worry about ordering for OAuth.
> >
> > We currently do not have any optional callbacks - configured callback
> > handlers have to process all the callbacks for the mechanism or else we
> > fail authentication. We have to make SaslExtensionValidationCallback an
> > exception, at least for backward compatibility. We will only include this
> > callback if the client provided some extensions. I think it is reasonable
> > to expect that in general, custom callback handlers will handle this
> > callback if clients are likely to set extensions.  In case it doesn't, we
> > don't want to make any 

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

[Ron Dagostino]

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 #2: Send two callbacks separately, in two separate arrays, to the
callback handler:
ch.handle(new Callback[] {tokenCallback});
ch.handle(new Callback[] {extensionsCallback});

I actually don't see any objective disadvantage with #1.  If we don't get
an exception then we know we have the information we need; if we do get an
exception then we can tell if the first callback was handled because either
its token() method or its errorStatus() method will return non-null; if
both return null then we just send the token callback by itself and we
don't publish any extension as negotiated properties.  There is no
possibility of partial results, and I don't think there is a performance
penalty due to potential re-validation here, either.

I  see a subjective disadvantage with #1.  It feels awkward to me to
provide the token as context for extension validation via the first
callback.

Actually, it just occurred to me why it feels awkward, and I think this is
an objective disadvantage of this approach.  It would be impossible to do
extension validation in such a scenario without also doing token validation
first.  We are using the first callback as a way to provide context, but we
are also using that first callback to request token validation.  We are
complecting two separate things -- context and a request for validation --
into one thing, so this approach has an element of complexity to it.

The second option has no such complexity.  If we want to provide context to
the extension validation then we do that by adding a token to the
extensionsCallback instance before we provide it to the callback handler.
How we do that -- whether by Map or via a typed getter --
feels like a subjective decision, and assuming you agree with the
complexity argument and choose option #2, I would defer to your preference
as to how to implement it.

Ron












On Wed, Aug 8, 2018 at 3:10 PM Rajini Sivaram 
wrote:

> 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. There
> are pros and cons for both approaches, so we could go with either.
>
> Callbacks are provided to callback handlers in an array and there is
> implicit ordering in the callbacks provided to the callback handler.
> In the typical example of {NameCallback, PasswordCallback}, we expect that
> ordering so that password callback knows what the user name is. Kafka
> guarantees ordering of server callbacks in each of its SASL mechanisms and
> this is explicitly stated in
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-86%3A+Configurable+SASL+callback+handlers
> .
> Until now, we didn't need to worry about ordering for OAuth.
>
> We currently do not have any optional callbacks - configured callback
> handlers have to process all the callbacks for the mechanism or else we
> fail authentication. We have to make SaslExtensionValidationCallback an
> exception, at least for backward compatibility. We will only include this
> callback if the client provided some extensions. I think it is reasonable
> to expect that in general, custom callback handlers will handle this
> callback if clients are likely to set extensions.  In case it doesn't, we
> don't want to make any assumptions about which callbacks may have been
> handled. Instead, it would be better to invoke the callback handler again
> without the extensions callback and not expose any extensions as negotiated
> properties. Since we are doing this only for backward compatibility, the
> small performance hit would be reasonable, avoiding any assumptions about
> the callback handler implementation and partial results on hitting an
> exception.
>
> Back to the other approach of providing a Map. For OAuth, we would like
> extension validation to see the actual OAuthBearerToken object, for
> instance to validate extensions based on scope. Having
> these mechanism-specific objects in a Map doesn't feel ideal. It will
> probably be better to define OAuthBearerExtensionsValidatorCallback with a
> token getter that returns OAuthBearerToken in that case.
>
> Thoughts?
>
>
> On Wed, Aug 8, 2018 at 6:09 PM, Ron Dagostino  wrote:
>
> > 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 than the first), I'm not sure 

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

[Rajini Sivaram]

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. There
are pros and cons for both approaches, so we could go with either.

Callbacks are provided to callback handlers in an array and there is
implicit ordering in the callbacks provided to the callback handler.
In the typical example of {NameCallback, PasswordCallback}, we expect that
ordering so that password callback knows what the user name is. Kafka
guarantees ordering of server callbacks in each of its SASL mechanisms and
this is explicitly stated in
https://cwiki.apache.org/confluence/display/KAFKA/KIP-86%3A+Configurable+SASL+callback+handlers.
Until now, we didn't need to worry about ordering for OAuth.

We currently do not have any optional callbacks - configured callback
handlers have to process all the callbacks for the mechanism or else we
fail authentication. We have to make SaslExtensionValidationCallback an
exception, at least for backward compatibility. We will only include this
callback if the client provided some extensions. I think it is reasonable
to expect that in general, custom callback handlers will handle this
callback if clients are likely to set extensions.  In case it doesn't, we
don't want to make any assumptions about which callbacks may have been
handled. Instead, it would be better to invoke the callback handler again
without the extensions callback and not expose any extensions as negotiated
properties. Since we are doing this only for backward compatibility, the
small performance hit would be reasonable, avoiding any assumptions about
the callback handler implementation and partial results on hitting an
exception.

Back to the other approach of providing a Map. For OAuth, we would like
extension validation to see the actual OAuthBearerToken object, for
instance to validate extensions based on scope. Having
these mechanism-specific objects in a Map doesn't feel ideal. It will
probably be better to define OAuthBearerExtensionsValidatorCallback with a
token getter that returns OAuthBearerToken in that case.

Thoughts?


On Wed, Aug 8, 2018 at 6:09 PM, Ron Dagostino  wrote:

> 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 than the first), I'm not sure CallbackHandler guarantees that
> callbacks are to be processed in order until either there are no more left
> in the array or one of the elements is an unsupported callback.  The
> Javadoc simply says it throws UnsupportedCallbackException "if the
> implementation of this method does not support one or more of the Callbacks
> specified in the callbacks parameter." This statement does not preclude the
> case that implementations might first check to make sure all of the
> provided callbacks are supported before processing any of them.
>
> We could update the Javadoc for AuthenticateCallbackHandler to make it
> clear how implementations must work -- i.e. they must process the callbacks
> in order, and they must process all recognized callbacks before throwing
> UnsupportedCallbackException due to an unrecognized one.
>
> Note that the above issue does not arise if we simply want the ability to
> validate SASL extensions in isolation -- we could just give the callback
> handler an array containing a single instance of the proposed
> SaslExtensionsValidatorCallback.The issue only arises if we want to
> provide
> additional context (e.g. the token in the case of SASL/OATHBEARER) to the
> validation mechanism.
>
> If it is not just SASL Extension validation that we are interested in
> adding but in fact we want to be able to provide additional context to
> SaslExtensionsValidatorCallback, then adding the ordering constraint above
> is one way, but we could avoid the constraint by allowing
> SaslExtensionsValidatorCallback to accept not only the extensions to
> validate but also an arbitrary map of name/value pairs.  Each SASL
> mechanism implementation could declare what additional context it provides
> (if any) and at what key(s) the information is available.  This second
> approach feels more direct than the first one and would be my preference
> (assuming I',m not missing anything, which is certainly possible).
> Thoughts?
>
> Ron
>
>
> On Wed, Aug 8, 2018 at 12:39 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 

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

[Stanislav Kozlovski]

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 SaslExtensionsValidatorCallback accept arbitrary fields is a
good idea, that way we could reuse it for any other SASL mechanism which
implements extensions. And of course, you could simply validate the
extensions and ignore the additional information - that would be valid too.
I think this is the way to go!

Best,
Stanislav

On Wed, Aug 8, 2018 at 6:10 PM Ron Dagostino  wrote:

> 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 than the first), I'm not sure CallbackHandler guarantees that
> callbacks are to be processed in order until either there are no more left
> in the array or one of the elements is an unsupported callback.  The
> Javadoc simply says it throws UnsupportedCallbackException "if the
> implementation of this method does not support one or more of the Callbacks
> specified in the callbacks parameter." This statement does not preclude the
> case that implementations might first check to make sure all of the
> provided callbacks are supported before processing any of them.
>
> We could update the Javadoc for AuthenticateCallbackHandler to make it
> clear how implementations must work -- i.e. they must process the callbacks
> in order, and they must process all recognized callbacks before throwing
> UnsupportedCallbackException due to an unrecognized one.
>
> Note that the above issue does not arise if we simply want the ability to
> validate SASL extensions in isolation -- we could just give the callback
> handler an array containing a single instance of the proposed
> SaslExtensionsValidatorCallback.The issue only arises if we want to provide
> additional context (e.g. the token in the case of SASL/OATHBEARER) to the
> validation mechanism.
>
> If it is not just SASL Extension validation that we are interested in
> adding but in fact we want to be able to provide additional context to
> SaslExtensionsValidatorCallback, then adding the ordering constraint above
> is one way, but we could avoid the constraint by allowing
> SaslExtensionsValidatorCallback to accept not only the extensions to
> validate but also an arbitrary map of name/value pairs.  Each SASL
> mechanism implementation could declare what additional context it provides
> (if any) and at what key(s) the information is available.  This second
> approach feels more direct than the first one and would be my preference
> (assuming I',m not missing anything, which is certainly possible).
> Thoughts?
>
> Ron
>
>
> On Wed, Aug 8, 2018 at 12:39 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 separate entities that need validation? When we add support
> > > for customisable extensions in other mechanisms, we could reuse
> > > SaslExtensionsValidatorCallback. We will invoke CallbackHandler with {
> > > OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in that
> > > order like we do { NameCallback, PasswordCallback }. So typically we
> > expect
> > > to validate tokens with no reference to extensions, but we may refer to
> > > token to validate extensions. Only validated extensions will be
> available
> > > as the server's negotiated properties. We will need to handle
> > > UnsupportedCallbackException for SaslExtensionsValidatorCallback for
> > > backwards compatibility, but that should be ok.
> > >
> > > What do you think?
> > >
> > >
> > > On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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
> > > > >
> > > > > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > > > > stanis...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > Hey Ron,
> > > > > >
> > > > > > I fully agree that token validation is a serious security
> > operation.
> > > > > > Although, I believe allowing the user to do more 

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

[Ron Dagostino]

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 than the first), I'm not sure CallbackHandler guarantees that
callbacks are to be processed in order until either there are no more left
in the array or one of the elements is an unsupported callback.  The
Javadoc simply says it throws UnsupportedCallbackException "if the
implementation of this method does not support one or more of the Callbacks
specified in the callbacks parameter." This statement does not preclude the
case that implementations might first check to make sure all of the
provided callbacks are supported before processing any of them.

We could update the Javadoc for AuthenticateCallbackHandler to make it
clear how implementations must work -- i.e. they must process the callbacks
in order, and they must process all recognized callbacks before throwing
UnsupportedCallbackException due to an unrecognized one.

Note that the above issue does not arise if we simply want the ability to
validate SASL extensions in isolation -- we could just give the callback
handler an array containing a single instance of the proposed
SaslExtensionsValidatorCallback.The issue only arises if we want to provide
additional context (e.g. the token in the case of SASL/OATHBEARER) to the
validation mechanism.

If it is not just SASL Extension validation that we are interested in
adding but in fact we want to be able to provide additional context to
SaslExtensionsValidatorCallback, then adding the ordering constraint above
is one way, but we could avoid the constraint by allowing
SaslExtensionsValidatorCallback to accept not only the extensions to
validate but also an arbitrary map of name/value pairs.  Each SASL
mechanism implementation could declare what additional context it provides
(if any) and at what key(s) the information is available.  This second
approach feels more direct than the first one and would be my preference
(assuming I',m not missing anything, which is certainly possible).
Thoughts?

Ron


On Wed, Aug 8, 2018 at 12:39 PM Stanislav Kozlovski 
wrote:

> 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 separate entities that need validation? When we add support
> > for customisable extensions in other mechanisms, we could reuse
> > SaslExtensionsValidatorCallback. We will invoke CallbackHandler with {
> > OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in that
> > order like we do { NameCallback, PasswordCallback }. So typically we
> expect
> > to validate tokens with no reference to extensions, but we may refer to
> > token to validate extensions. Only validated extensions will be available
> > as the server's negotiated properties. We will need to handle
> > UnsupportedCallbackException for SaslExtensionsValidatorCallback for
> > backwards compatibility, but that should be ok.
> >
> > What do you think?
> >
> >
> > On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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
> > > >
> > > > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > > > stanis...@confluent.io>
> > > > wrote:
> > > >
> > > > > 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
> > > > > expect people to take the appropriate considerations when
> validating
> > > > > unsecured extensions against the token.
> > > > > I also think that using the extensions as a secondary validation
> > method
> > > > > might be useful. You could do your normal validation using the
> token
> > > and
> > > > > then have a second sanity-check validation on top (e.g validate
> > > > > hostname/port is what client expected). Keep in mind that the
> server
> > > > > exposes the properties via `getNegotiatedProperty` so it makes
> sense
> > to
> > > > > allow the server to have custom validation on the extensions.
> > > > >
> > > > > Best,
> > > > > Stanislav
> > > > >
> > > > 

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

[Stanislav Kozlovski]

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 separate entities that need validation? When we add support
> for customisable extensions in other mechanisms, we could reuse
> SaslExtensionsValidatorCallback. We will invoke CallbackHandler with {
> OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in that
> order like we do { NameCallback, PasswordCallback }. So typically we expect
> to validate tokens with no reference to extensions, but we may refer to
> token to validate extensions. Only validated extensions will be available
> as the server's negotiated properties. We will need to handle
> UnsupportedCallbackException for SaslExtensionsValidatorCallback for
> backwards compatibility, but that should be ok.
>
> What do you think?
>
>
> On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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
> > >
> > > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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
> > > > expect people to take the appropriate considerations when validating
> > > > unsecured extensions against the token.
> > > > I also think that using the extensions as a secondary validation
> method
> > > > might be useful. You could do your normal validation using the token
> > and
> > > > then have a second sanity-check validation on top (e.g validate
> > > > hostname/port is what client expected). Keep in mind that the server
> > > > exposes the properties via `getNegotiatedProperty` so it makes sense
> to
> > > > allow the server to have custom validation on the extensions.
> > > >
> > > > Best,
> > > > Stanislav
> > > >
> > > > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino 
> > wrote:
> > > >
> > > > > 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 introspect the callback to see what extensions were
> > provided
> > > > with
> > > > > the
> > > > > token.
> > > > >
> > > > > That being said, token validation is a very security-sensitive
> > > operation,
> > > > > and it would be a serious security issue if the result of applying
> > the
> > > > > validation algorithm (which yields a valid vs. not valid
> > determination)
> > > > > depended on anything provided by the client other than the actual
> > token
> > > > > value.  Nobody should ever allow the client to specify a JWK Set
> URL,
> > > for
> > > > > example, or a whitelist of acceptable domains for retrieving JWK
> > Sets.
> > > > It
> > > > > feels to me that while a use case might exist (some kind of trace
> ID,
> > > for
> > > > > example, to aid in debugging), someone might inadvertently hang
> > > > themselves
> > > > > if we give them the rope.  The risk vs. reward value proposition
> here
> > > > > doesn't feel like a good one at first glance.  Thoughts?
> > > > >
> > > > > Ron
> > > > >
> > > > >
> > > > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > > > > stanis...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > 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 extensions with the token (e.g if users have
> > > > > > implemented secured OAuth tokens).
> > > > > > The implementation would be to instantiate
> > > > > > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also
> > > leave
> > > > > the
> > > > > > current constructor, as its a public class).
> > > > > >
> > > > > > What are everybody's thoughts on this? If there are no
> objections,
> > > I'll
> > > > > > update the KIP in due time
> > > > > 

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

[Rajini Sivaram]

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
SaslExtensionsValidatorCallback. We will invoke CallbackHandler with {
OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in that
order like we do { NameCallback, PasswordCallback }. So typically we expect
to validate tokens with no reference to extensions, but we may refer to
token to validate extensions. Only validated extensions will be available
as the server's negotiated properties. We will need to handle
UnsupportedCallbackException for SaslExtensionsValidatorCallback for
backwards compatibility, but that should be ok.

What do you think?


On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski 
wrote:

> 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
> >
> > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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
> > > expect people to take the appropriate considerations when validating
> > > unsecured extensions against the token.
> > > I also think that using the extensions as a secondary validation method
> > > might be useful. You could do your normal validation using the token
> and
> > > then have a second sanity-check validation on top (e.g validate
> > > hostname/port is what client expected). Keep in mind that the server
> > > exposes the properties via `getNegotiatedProperty` so it makes sense to
> > > allow the server to have custom validation on the extensions.
> > >
> > > Best,
> > > Stanislav
> > >
> > > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino 
> wrote:
> > >
> > > > 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 introspect the callback to see what extensions were
> provided
> > > with
> > > > the
> > > > token.
> > > >
> > > > That being said, token validation is a very security-sensitive
> > operation,
> > > > and it would be a serious security issue if the result of applying
> the
> > > > validation algorithm (which yields a valid vs. not valid
> determination)
> > > > depended on anything provided by the client other than the actual
> token
> > > > value.  Nobody should ever allow the client to specify a JWK Set URL,
> > for
> > > > example, or a whitelist of acceptable domains for retrieving JWK
> Sets.
> > > It
> > > > feels to me that while a use case might exist (some kind of trace ID,
> > for
> > > > example, to aid in debugging), someone might inadvertently hang
> > > themselves
> > > > if we give them the rope.  The risk vs. reward value proposition here
> > > > doesn't feel like a good one at first glance.  Thoughts?
> > > >
> > > > Ron
> > > >
> > > >
> > > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > > > stanis...@confluent.io>
> > > > wrote:
> > > >
> > > > > 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 extensions with the token (e.g if users have
> > > > > implemented secured OAuth tokens).
> > > > > The implementation would be to instantiate
> > > > > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also
> > leave
> > > > the
> > > > > current constructor, as its a public class).
> > > > >
> > > > > What are everybody's thoughts on this? If there are no objections,
> > I'll
> > > > > update the KIP in due time
> > > > >
> > > > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram <
> > > rajinisiva...@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Looks good. Thanks, Stanislav.
> > > > > >
> > > > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > > > > stanis...@confluent.io
> > > > > > > wrote:
> > > > > >
> > > > > > > Hi Rajini,
> > > > > > >
> > > > > > > I updated the KIP. Please 

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

[Stanislav Kozlovski]

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
>
> On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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
> > expect people to take the appropriate considerations when validating
> > unsecured extensions against the token.
> > I also think that using the extensions as a secondary validation method
> > might be useful. You could do your normal validation using the token and
> > then have a second sanity-check validation on top (e.g validate
> > hostname/port is what client expected). Keep in mind that the server
> > exposes the properties via `getNegotiatedProperty` so it makes sense to
> > allow the server to have custom validation on the extensions.
> >
> > Best,
> > Stanislav
> >
> > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino  wrote:
> >
> > > 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 introspect the callback to see what extensions were provided
> > with
> > > the
> > > token.
> > >
> > > That being said, token validation is a very security-sensitive
> operation,
> > > and it would be a serious security issue if the result of applying the
> > > validation algorithm (which yields a valid vs. not valid determination)
> > > depended on anything provided by the client other than the actual token
> > > value.  Nobody should ever allow the client to specify a JWK Set URL,
> for
> > > example, or a whitelist of acceptable domains for retrieving JWK Sets.
> > It
> > > feels to me that while a use case might exist (some kind of trace ID,
> for
> > > example, to aid in debugging), someone might inadvertently hang
> > themselves
> > > if we give them the rope.  The risk vs. reward value proposition here
> > > doesn't feel like a good one at first glance.  Thoughts?
> > >
> > > Ron
> > >
> > >
> > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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 extensions with the token (e.g if users have
> > > > implemented secured OAuth tokens).
> > > > The implementation would be to instantiate
> > > > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also
> leave
> > > the
> > > > current constructor, as its a public class).
> > > >
> > > > What are everybody's thoughts on this? If there are no objections,
> I'll
> > > > update the KIP in due time
> > > >
> > > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram <
> > rajinisiva...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > > > Looks good. Thanks, Stanislav.
> > > > >
> > > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > > > stanis...@confluent.io
> > > > > > 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 <
> > > > rajinisiva...@gmail.com
> > > > > >
> > > > > > 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
> > > > > 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
> tokens.
> > > We
> > > > > > could
> > > > > > > include the example in this section or at least link to the
> > > example.
> > > > > > >
> > > > > > >- New config option for default, unsecured bearer tokens -
> > > > > > >`unsecuredLoginExtension_`.
> > > > > > >
> > > > > > >
> > > > > > > 2. Can you add the package for SaslExtensionsCallback class?
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > > > > > stanis...@confluent.io> wrote:
> > > > > > >
> > > > > > > > 

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

[Ron Dagostino]

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 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
> expect people to take the appropriate considerations when validating
> unsecured extensions against the token.
> I also think that using the extensions as a secondary validation method
> might be useful. You could do your normal validation using the token and
> then have a second sanity-check validation on top (e.g validate
> hostname/port is what client expected). Keep in mind that the server
> exposes the properties via `getNegotiatedProperty` so it makes sense to
> allow the server to have custom validation on the extensions.
>
> Best,
> Stanislav
>
> On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino  wrote:
>
> > 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 introspect the callback to see what extensions were provided
> with
> > the
> > token.
> >
> > That being said, token validation is a very security-sensitive operation,
> > and it would be a serious security issue if the result of applying the
> > validation algorithm (which yields a valid vs. not valid determination)
> > depended on anything provided by the client other than the actual token
> > value.  Nobody should ever allow the client to specify a JWK Set URL, for
> > example, or a whitelist of acceptable domains for retrieving JWK Sets.
> It
> > feels to me that while a use case might exist (some kind of trace ID, for
> > example, to aid in debugging), someone might inadvertently hang
> themselves
> > if we give them the rope.  The risk vs. reward value proposition here
> > doesn't feel like a good one at first glance.  Thoughts?
> >
> > Ron
> >
> >
> > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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 extensions with the token (e.g if users have
> > > implemented secured OAuth tokens).
> > > The implementation would be to instantiate
> > > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also leave
> > the
> > > current constructor, as its a public class).
> > >
> > > What are everybody's thoughts on this? If there are no objections, I'll
> > > update the KIP in due time
> > >
> > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram <
> rajinisiva...@gmail.com
> > >
> > > wrote:
> > >
> > > > Looks good. Thanks, Stanislav.
> > > >
> > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > > stanis...@confluent.io
> > > > > 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 <
> > > rajinisiva...@gmail.com
> > > > >
> > > > > 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
> > > > 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 tokens.
> > We
> > > > > could
> > > > > > include the example in this section or at least link to the
> > example.
> > > > > >
> > > > > >- New config option for default, unsecured bearer tokens -
> > > > > >`unsecuredLoginExtension_`.
> > > > > >
> > > > > >
> > > > > > 2. Can you add the package for SaslExtensionsCallback class?
> > > > > >
> > > > > >
> > > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > > > > stanis...@confluent.io> wrote:
> > > > > >
> > > > > > > Hi Ron,
> > > > > > >
> > > > > > > Thanks for the suggestions. I have applied them to the KIP.
> > > > > > >
> > > > > > > On Tue, Jul 24, 2018 at 1:39 PM Ron Dagostino <
> rndg...@gmail.com
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi Stanislav.  The statement "New config option for
> > > > > > > OAuthBearerLoginModule"
> > > > > > > > is technically incorrect; it should be "New 

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

[Stanislav Kozlovski]

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
expect people to take the appropriate considerations when validating
unsecured extensions against the token.
I also think that using the extensions as a secondary validation method
might be useful. You could do your normal validation using the token and
then have a second sanity-check validation on top (e.g validate
hostname/port is what client expected). Keep in mind that the server
exposes the properties via `getNegotiatedProperty` so it makes sense to
allow the server to have custom validation on the extensions.

Best,
Stanislav

On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino  wrote:

> 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 introspect the callback to see what extensions were provided with
> the
> token.
>
> That being said, token validation is a very security-sensitive operation,
> and it would be a serious security issue if the result of applying the
> validation algorithm (which yields a valid vs. not valid determination)
> depended on anything provided by the client other than the actual token
> value.  Nobody should ever allow the client to specify a JWK Set URL, for
> example, or a whitelist of acceptable domains for retrieving JWK Sets.  It
> feels to me that while a use case might exist (some kind of trace ID, for
> example, to aid in debugging), someone might inadvertently hang themselves
> if we give them the rope.  The risk vs. reward value proposition here
> doesn't feel like a good one at first glance.  Thoughts?
>
> Ron
>
>
> On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 extensions with the token (e.g if users have
> > implemented secured OAuth tokens).
> > The implementation would be to instantiate
> > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also leave
> the
> > current constructor, as its a public class).
> >
> > What are everybody's thoughts on this? If there are no objections, I'll
> > update the KIP in due time
> >
> > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram  >
> > wrote:
> >
> > > Looks good. Thanks, Stanislav.
> > >
> > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > stanis...@confluent.io
> > > > 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 <
> > rajinisiva...@gmail.com
> > > >
> > > > 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
> > > 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 tokens.
> We
> > > > could
> > > > > include the example in this section or at least link to the
> example.
> > > > >
> > > > >- New config option for default, unsecured bearer tokens -
> > > > >`unsecuredLoginExtension_`.
> > > > >
> > > > >
> > > > > 2. Can you add the package for SaslExtensionsCallback class?
> > > > >
> > > > >
> > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > > > stanis...@confluent.io> wrote:
> > > > >
> > > > > > 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 tokens" since that is what provides the
> > > > functionality
> > > > > > (as
> > > > > > > opposed to the login module, which does not).  Also, please
> state
> > > > that
> > > > > > > "auth" is not supported as a custom extension name with any
> > > > > > > SASL/OAUTHBEARER mechanism, including the unsecured one, since
> it
> > > is
> > > > > > > reserved by the spec for what is normally sent in the HTTP
> > > > > Authorization
> > > > > > > header an attempt to 

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

[Ron Dagostino]

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 introspect the callback to see what extensions were provided with
the
token.

That being said, token validation is a very security-sensitive operation,
and it would be a serious security issue if the result of applying the
validation algorithm (which yields a valid vs. not valid determination)
depended on anything provided by the client other than the actual token
value.  Nobody should ever allow the client to specify a JWK Set URL, for
example, or a whitelist of acceptable domains for retrieving JWK Sets.  It
feels to me that while a use case might exist (some kind of trace ID, for
example, to aid in debugging), someone might inadvertently hang themselves
if we give them the rope.  The risk vs. reward value proposition here
doesn't feel like a good one at first glance.  Thoughts?

Ron


On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski 
wrote:

> 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 extensions with the token (e.g if users have
> implemented secured OAuth tokens).
> The implementation would be to instantiate
> `OAuthBearerUnsecuredValidatorCallback` with the extensions (also leave the
> current constructor, as its a public class).
>
> What are everybody's thoughts on this? If there are no objections, I'll
> update the KIP in due time
>
> On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram 
> wrote:
>
> > Looks good. Thanks, Stanislav.
> >
> > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > stanis...@confluent.io
> > > 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 <
> rajinisiva...@gmail.com
> > >
> > > 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
> > 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 tokens. We
> > > could
> > > > include the example in this section or at least link to the example.
> > > >
> > > >- New config option for default, unsecured bearer tokens -
> > > >`unsecuredLoginExtension_`.
> > > >
> > > >
> > > > 2. Can you add the package for SaslExtensionsCallback class?
> > > >
> > > >
> > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > > stanis...@confluent.io> wrote:
> > > >
> > > > > 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 tokens" since that is what provides the
> > > functionality
> > > > > (as
> > > > > > opposed to the login module, which does not).  Also, please state
> > > that
> > > > > > "auth" is not supported as a custom extension name with any
> > > > > > SASL/OAUTHBEARER mechanism, including the unsecured one, since it
> > is
> > > > > > reserved by the spec for what is normally sent in the HTTP
> > > > Authorization
> > > > > > header an attempt to use it will result in a configuration
> > exception.
> > > > > >
> > > > > > Finally, please also state that while the OAuthBearerLoginModule
> > and
> > > > the
> > > > > > OAuthBearerSaslClient will be changed to request the extensions
> > from
> > > > its
> > > > > > callback handler, for backwards compatibility it is not necessary
> > for
> > > > the
> > > > > > callback handler to support SaslExtensionsCallback -- any
> > > > > > UnsupportedCallbackException that is thrown will be ignored and
> no
> > > > > > extensions will be added.
> > > > > >
> > > > > > Ron
> > > > > >
> > > > > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > > > > > stanis...@confluent.io>
> > > > > > wrote:
> > > > > >
> > > > > > > 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 <
> 

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

[Stanislav Kozlovski]

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 extensions with the token (e.g if users have
implemented secured OAuth tokens).
The implementation would be to instantiate
`OAuthBearerUnsecuredValidatorCallback` with the extensions (also leave the
current constructor, as its a public class).

What are everybody's thoughts on this? If there are no objections, I'll
update the KIP in due time

On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram 
wrote:

> Looks good. Thanks, Stanislav.
>
> On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> stanis...@confluent.io
> > 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 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 tokens. We
> > could
> > > include the example in this section or at least link to the example.
> > >
> > >- New config option for default, unsecured bearer tokens -
> > >`unsecuredLoginExtension_`.
> > >
> > >
> > > 2. Can you add the package for SaslExtensionsCallback class?
> > >
> > >
> > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > stanis...@confluent.io> wrote:
> > >
> > > > 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 tokens" since that is what provides the
> > functionality
> > > > (as
> > > > > opposed to the login module, which does not).  Also, please state
> > that
> > > > > "auth" is not supported as a custom extension name with any
> > > > > SASL/OAUTHBEARER mechanism, including the unsecured one, since it
> is
> > > > > reserved by the spec for what is normally sent in the HTTP
> > > Authorization
> > > > > header an attempt to use it will result in a configuration
> exception.
> > > > >
> > > > > Finally, please also state that while the OAuthBearerLoginModule
> and
> > > the
> > > > > OAuthBearerSaslClient will be changed to request the extensions
> from
> > > its
> > > > > callback handler, for backwards compatibility it is not necessary
> for
> > > the
> > > > > callback handler to support SaslExtensionsCallback -- any
> > > > > UnsupportedCallbackException that is thrown will be ignored and no
> > > > > extensions will be added.
> > > > >
> > > > > Ron
> > > > >
> > > > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > > > > stanis...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > 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 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
> > > > > > > OAuthBearerUnsecuredLoginCallbackHandler in the text in
> > addition to
> > > > > > giving
> > > > > > > the examples?  The examples show the new
> > > > > > > unsecuredLoginExtension_ feature, but that
> > feature
> > > is
> > > > > not
> > > > > > > described anywhere prior to it appearing there.
> > > > > > >
> > > > > > > Ron
> > > > > > >
> > > > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino <
> rndg...@gmail.com
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > 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 would have to be unmodifiable.
> > > > > > > >
> > > > > > > > Ron
> > > > > > > >
> > > > > > > > 

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

[Rajini Sivaram]

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 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 tokens. We
> could
> > include the example in this section or at least link to the example.
> >
> >- New config option for default, unsecured bearer tokens -
> >`unsecuredLoginExtension_`.
> >
> >
> > 2. Can you add the package for SaslExtensionsCallback class?
> >
> >
> > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > stanis...@confluent.io> wrote:
> >
> > > 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 tokens" since that is what provides the
> functionality
> > > (as
> > > > opposed to the login module, which does not).  Also, please state
> that
> > > > "auth" is not supported as a custom extension name with any
> > > > SASL/OAUTHBEARER mechanism, including the unsecured one, since it is
> > > > reserved by the spec for what is normally sent in the HTTP
> > Authorization
> > > > header an attempt to use it will result in a configuration exception.
> > > >
> > > > Finally, please also state that while the OAuthBearerLoginModule and
> > the
> > > > OAuthBearerSaslClient will be changed to request the extensions from
> > its
> > > > callback handler, for backwards compatibility it is not necessary for
> > the
> > > > callback handler to support SaslExtensionsCallback -- any
> > > > UnsupportedCallbackException that is thrown will be ignored and no
> > > > extensions will be added.
> > > >
> > > > Ron
> > > >
> > > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > > > stanis...@confluent.io>
> > > > wrote:
> > > >
> > > > > 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 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
> > > > > > OAuthBearerUnsecuredLoginCallbackHandler in the text in
> addition to
> > > > > giving
> > > > > > the examples?  The examples show the new
> > > > > > unsecuredLoginExtension_ feature, but that
> feature
> > is
> > > > not
> > > > > > described anywhere prior to it appearing there.
> > > > > >
> > > > > > Ron
> > > > > >
> > > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino  >
> > > > wrote:
> > > > > >
> > > > > > > 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 would have to be unmodifiable.
> > > > > > >
> > > > > > > Ron
> > > > > > >
> > > > > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> > > > > rajinisiva...@gmail.com
> > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > >> 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 the
> > public
> > > > API
> > > > > > >> should be sufficient in the vast majority of the cases. Ron,
> do
> > > you
> > > > > > agree?
> > > > > > >>
> > > > > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino <
> > > rndg...@gmail.com>
> > > > > > >> wrote:
> > > > > > >>
> > > > > > >> > Hi Stanislav.  See
> > > > https://tools.ietf.org/html/rfc7628#section-3.1,
> > > > > > and
> > > > > > >> > that section refers to the 

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

[Stanislav Kozlovski]

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 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 tokens. We could
> include the example in this section or at least link to the example.
>
>- New config option for default, unsecured bearer tokens -
>`unsecuredLoginExtension_`.
>
>
> 2. Can you add the package for SaslExtensionsCallback class?
>
>
> On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> stanis...@confluent.io> wrote:
>
> > 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 tokens" since that is what provides the functionality
> > (as
> > > opposed to the login module, which does not).  Also, please state that
> > > "auth" is not supported as a custom extension name with any
> > > SASL/OAUTHBEARER mechanism, including the unsecured one, since it is
> > > reserved by the spec for what is normally sent in the HTTP
> Authorization
> > > header an attempt to use it will result in a configuration exception.
> > >
> > > Finally, please also state that while the OAuthBearerLoginModule and
> the
> > > OAuthBearerSaslClient will be changed to request the extensions from
> its
> > > callback handler, for backwards compatibility it is not necessary for
> the
> > > callback handler to support SaslExtensionsCallback -- any
> > > UnsupportedCallbackException that is thrown will be ignored and no
> > > extensions will be added.
> > >
> > > Ron
> > >
> > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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 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
> > > > > OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to
> > > > giving
> > > > > the examples?  The examples show the new
> > > > > unsecuredLoginExtension_ feature, but that feature
> is
> > > not
> > > > > described anywhere prior to it appearing there.
> > > > >
> > > > > Ron
> > > > >
> > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino 
> > > wrote:
> > > > >
> > > > > > 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 would have to be unmodifiable.
> > > > > >
> > > > > > Ron
> > > > > >
> > > > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> > > > rajinisiva...@gmail.com
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > >> 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 the
> public
> > > API
> > > > > >> should be sufficient in the vast majority of the cases. Ron, do
> > you
> > > > > agree?
> > > > > >>
> > > > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino <
> > rndg...@gmail.com>
> > > > > >> wrote:
> > > > > >>
> > > > > >> > 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 <
> > > > > >> stanis...@confluent.io>
> > > > > >> > wrote:
> > > > > >> > >
> > > > > >> > > Hey Ron and Rajini,
> > > > > >> > >
> > > > > >> > > Here are my thoughts:
> > > > > >> > 

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

[Rajini Sivaram]

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 tokens. We could
include the example in this section or at least link to the example.

   - New config option for default, unsecured bearer tokens -
   `unsecuredLoginExtension_`.


2. Can you add the package for SaslExtensionsCallback class?


On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
stanis...@confluent.io> wrote:

> 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 tokens" since that is what provides the functionality
> (as
> > opposed to the login module, which does not).  Also, please state that
> > "auth" is not supported as a custom extension name with any
> > SASL/OAUTHBEARER mechanism, including the unsecured one, since it is
> > reserved by the spec for what is normally sent in the HTTP Authorization
> > header an attempt to use it will result in a configuration exception.
> >
> > Finally, please also state that while the OAuthBearerLoginModule and the
> > OAuthBearerSaslClient will be changed to request the extensions from its
> > callback handler, for backwards compatibility it is not necessary for the
> > callback handler to support SaslExtensionsCallback -- any
> > UnsupportedCallbackException that is thrown will be ignored and no
> > extensions will be added.
> >
> > Ron
> >
> > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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 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
> > > > OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to
> > > giving
> > > > the examples?  The examples show the new
> > > > unsecuredLoginExtension_ feature, but that feature is
> > not
> > > > described anywhere prior to it appearing there.
> > > >
> > > > Ron
> > > >
> > > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino 
> > wrote:
> > > >
> > > > > 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 would have to be unmodifiable.
> > > > >
> > > > > Ron
> > > > >
> > > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> > > rajinisiva...@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > >> 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 the public
> > API
> > > > >> should be sufficient in the vast majority of the cases. Ron, do
> you
> > > > agree?
> > > > >>
> > > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino <
> rndg...@gmail.com>
> > > > >> wrote:
> > > > >>
> > > > >> > 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 <
> > > > >> stanis...@confluent.io>
> > > > >> > wrote:
> > > > >> > >
> > > > >> > > 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 sure they're passing in an
> > > unmodifiable
> > > > >> map

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

[Stanislav Kozlovski]

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 tokens" since that is what provides the functionality (as
> opposed to the login module, which does not).  Also, please state that
> "auth" is not supported as a custom extension name with any
> SASL/OAUTHBEARER mechanism, including the unsecured one, since it is
> reserved by the spec for what is normally sent in the HTTP Authorization
> header an attempt to use it will result in a configuration exception.
>
> Finally, please also state that while the OAuthBearerLoginModule and the
> OAuthBearerSaslClient will be changed to request the extensions from its
> callback handler, for backwards compatibility it is not necessary for the
> callback handler to support SaslExtensionsCallback -- any
> UnsupportedCallbackException that is thrown will be ignored and no
> extensions will be added.
>
> Ron
>
> On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 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
> > > OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to
> > giving
> > > the examples?  The examples show the new
> > > unsecuredLoginExtension_ feature, but that feature is
> not
> > > described anywhere prior to it appearing there.
> > >
> > > Ron
> > >
> > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino 
> wrote:
> > >
> > > > 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 would have to be unmodifiable.
> > > >
> > > > Ron
> > > >
> > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> > rajinisiva...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > >> 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 the public
> API
> > > >> should be sufficient in the vast majority of the cases. Ron, do you
> > > agree?
> > > >>
> > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino 
> > > >> wrote:
> > > >>
> > > >> > 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 <
> > > >> stanis...@confluent.io>
> > > >> > wrote:
> > > >> > >
> > > >> > > 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 sure they're passing in an
> > unmodifiable
> > > >> map
> > > >> > > themselves. I believe it would be better if we enforced that
> > through
> > > >> > class
> > > >> > > constructors instead.
> > > >> > > SaslExtensions#map() - I'd also prefer this. The reason I went
> > with
> > > >> > > `extensionValue` and `extensionNames` was because I figured it
> > made
> > > >> sense
> > > >> > > to have `ScramExtensions` extend `SaslExtensions` and therefore
> > have
> > > >> > their
> > > >> > > API be similar. In the end, do you think that it is worth it to
> > have
> > > >> > > `ScramExtensions` extend `SaslExtensions`?
> > > >> > > @Ron, could you point me to the SASL OAuth mechanism specific
> > > regular
> > > >> > > expressions for keys/values you mentioned are in RFC 7628 (
> > > >> > > https://tools.ietf.org/html/rfc7628) ? I could not find any
> while
> > > >> > > originally implementing this.
> > > >> > >
> > > >> > > Best,
> > > >> > > 

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

[Ron Dagostino]

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
"auth" is not supported as a custom extension name with any
SASL/OAUTHBEARER mechanism, including the unsecured one, since it is
reserved by the spec for what is normally sent in the HTTP Authorization
header an attempt to use it will result in a configuration exception.

Finally, please also state that while the OAuthBearerLoginModule and the
OAuthBearerSaslClient will be changed to request the extensions from its
callback handler, for backwards compatibility it is not necessary for the
callback handler to support SaslExtensionsCallback -- any
UnsupportedCallbackException that is thrown will be ignored and no
extensions will be added.

Ron

On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski 
wrote:

> 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 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
> > OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to
> giving
> > the examples?  The examples show the new
> > unsecuredLoginExtension_ feature, but that feature is not
> > described anywhere prior to it appearing there.
> >
> > Ron
> >
> > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino  wrote:
> >
> > > 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 would have to be unmodifiable.
> > >
> > > Ron
> > >
> > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> rajinisiva...@gmail.com
> > >
> > > wrote:
> > >
> > >> 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 the public API
> > >> should be sufficient in the vast majority of the cases. Ron, do you
> > agree?
> > >>
> > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino 
> > >> wrote:
> > >>
> > >> > 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 <
> > >> stanis...@confluent.io>
> > >> > wrote:
> > >> > >
> > >> > > 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 sure they're passing in an
> unmodifiable
> > >> map
> > >> > > themselves. I believe it would be better if we enforced that
> through
> > >> > class
> > >> > > constructors instead.
> > >> > > SaslExtensions#map() - I'd also prefer this. The reason I went
> with
> > >> > > `extensionValue` and `extensionNames` was because I figured it
> made
> > >> sense
> > >> > > to have `ScramExtensions` extend `SaslExtensions` and therefore
> have
> > >> > their
> > >> > > API be similar. In the end, do you think that it is worth it to
> have
> > >> > > `ScramExtensions` extend `SaslExtensions`?
> > >> > > @Ron, could you point me to the SASL OAuth mechanism specific
> > regular
> > >> > > expressions for keys/values you mentioned are in RFC 7628 (
> > >> > > https://tools.ietf.org/html/rfc7628) ? I could not find any while
> > >> > > originally implementing this.
> > >> > >
> > >> > > Best,
> > >> > > Stanislav
> > >> > >
> > >> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino  >
> > >> > wrote:
> > >> > >>
> > >> > >> 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 

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

[Stanislav Kozlovski]

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 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
> OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to giving
> the examples?  The examples show the new
> unsecuredLoginExtension_ feature, but that feature is not
> described anywhere prior to it appearing there.
>
> Ron
>
> On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino  wrote:
>
> > 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 would have to be unmodifiable.
> >
> > Ron
> >
> > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram  >
> > wrote:
> >
> >> 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 the public API
> >> should be sufficient in the vast majority of the cases. Ron, do you
> agree?
> >>
> >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino 
> >> wrote:
> >>
> >> > 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 <
> >> stanis...@confluent.io>
> >> > wrote:
> >> > >
> >> > > 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 sure they're passing in an unmodifiable
> >> map
> >> > > themselves. I believe it would be better if we enforced that through
> >> > class
> >> > > constructors instead.
> >> > > SaslExtensions#map() - I'd also prefer this. The reason I went with
> >> > > `extensionValue` and `extensionNames` was because I figured it made
> >> sense
> >> > > to have `ScramExtensions` extend `SaslExtensions` and therefore have
> >> > their
> >> > > API be similar. In the end, do you think that it is worth it to have
> >> > > `ScramExtensions` extend `SaslExtensions`?
> >> > > @Ron, could you point me to the SASL OAuth mechanism specific
> regular
> >> > > expressions for keys/values you mentioned are in RFC 7628 (
> >> > > https://tools.ietf.org/html/rfc7628) ? I could not find any while
> >> > > originally implementing this.
> >> > >
> >> > > Best,
> >> > > Stanislav
> >> > >
> >> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino 
> >> > wrote:
> >> > >>
> >> > >> 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 clarity on immutability, and it
> also
> >> > >> eliminates copying, like this:
> >> > >>
> >> > >> SaslExtensions myMethod() {
> >> > >>Map myRetval =
> getUnmodifiableSaslExtensionsMap();
> >> > >>return new SaslExtensions() {
> >> > >>public Map map() {
> >> > >>return myRetval;
> >> > >>}
> >> > >>}
> >> > >> }
> >> > >>
> >> > >> Alternatively, we could do it like this:
> >> > >>
> >> > >> /**
> >> > >> * Supplier that returns immutable map view of SASL Extensions
> >> > >> */
> >> > >> public interface SaslExtensions extends Supplier >> String>> {
> >> > >>// empty
> >> > >> }
> >> > >>
> >> > >> The we could simply return the instance like this, again without
> >> > copying:
> >> > >>
> >> > >> SaslExtensions myMethod() {
> >> > >>Map myRetval =
> getUnmodifiableSaslExtensionsMap();
> >> > >>return () -> myRetval;
> >> > >> }
> >> > >>
> >> > >> I think the main reason for making SaslExtensions part of the
> public
> >> > >> interface is to avoid adding a Map to the Subject's public
> >> credentials.
> >> > >> Making SaslExtensions an interface meets that requirement and then
> >> 

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

[Ron Dagostino]

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
OAuthBearerUnsecuredLoginCallbackHandler in the text in addition to giving
the examples?  The examples show the new
unsecuredLoginExtension_ feature, but that feature is not
described anywhere prior to it appearing there.

Ron

On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino  wrote:

> 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 would have to be unmodifiable.
>
> Ron
>
> On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram 
> wrote:
>
>> 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 the public API
>> should be sufficient in the vast majority of the cases. Ron, do you agree?
>>
>> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino 
>> wrote:
>>
>> > 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 <
>> stanis...@confluent.io>
>> > wrote:
>> > >
>> > > 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 sure they're passing in an unmodifiable
>> map
>> > > themselves. I believe it would be better if we enforced that through
>> > class
>> > > constructors instead.
>> > > SaslExtensions#map() - I'd also prefer this. The reason I went with
>> > > `extensionValue` and `extensionNames` was because I figured it made
>> sense
>> > > to have `ScramExtensions` extend `SaslExtensions` and therefore have
>> > their
>> > > API be similar. In the end, do you think that it is worth it to have
>> > > `ScramExtensions` extend `SaslExtensions`?
>> > > @Ron, could you point me to the SASL OAuth mechanism specific regular
>> > > expressions for keys/values you mentioned are in RFC 7628 (
>> > > https://tools.ietf.org/html/rfc7628) ? I could not find any while
>> > > originally implementing this.
>> > >
>> > > Best,
>> > > Stanislav
>> > >
>> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino 
>> > wrote:
>> > >>
>> > >> 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 clarity on immutability, and it also
>> > >> eliminates copying, like this:
>> > >>
>> > >> SaslExtensions myMethod() {
>> > >>Map myRetval = getUnmodifiableSaslExtensionsMap();
>> > >>return new SaslExtensions() {
>> > >>public Map map() {
>> > >>return myRetval;
>> > >>}
>> > >>}
>> > >> }
>> > >>
>> > >> Alternatively, we could do it like this:
>> > >>
>> > >> /**
>> > >> * Supplier that returns immutable map view of SASL Extensions
>> > >> */
>> > >> public interface SaslExtensions extends Supplier> String>> {
>> > >>// empty
>> > >> }
>> > >>
>> > >> The we could simply return the instance like this, again without
>> > copying:
>> > >>
>> > >> SaslExtensions myMethod() {
>> > >>Map myRetval = getUnmodifiableSaslExtensionsMap();
>> > >>return () -> myRetval;
>> > >> }
>> > >>
>> > >> I think the main reason for making SaslExtensions part of the public
>> > >> interface is to avoid adding a Map to the Subject's public
>> credentials.
>> > >> Making SaslExtensions an interface meets that requirement and then
>> > allows
>> > >> us to be free to implement whatever we want internally.
>> > >>
>> > >> Thoughts?
>> > >>
>> > >> Ron
>> > >>
>> > >>> On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino 
>> > wrote:
>> > >>>
>> > >>> 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 

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

[Ron Dagostino]

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 would have to be unmodifiable.

Ron

On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram 
wrote:

> 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 the public API
> should be sufficient in the vast majority of the cases. Ron, do you agree?
>
> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino  wrote:
>
> > 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 <
> stanis...@confluent.io>
> > wrote:
> > >
> > > 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 sure they're passing in an unmodifiable
> map
> > > themselves. I believe it would be better if we enforced that through
> > class
> > > constructors instead.
> > > SaslExtensions#map() - I'd also prefer this. The reason I went with
> > > `extensionValue` and `extensionNames` was because I figured it made
> sense
> > > to have `ScramExtensions` extend `SaslExtensions` and therefore have
> > their
> > > API be similar. In the end, do you think that it is worth it to have
> > > `ScramExtensions` extend `SaslExtensions`?
> > > @Ron, could you point me to the SASL OAuth mechanism specific regular
> > > expressions for keys/values you mentioned are in RFC 7628 (
> > > https://tools.ietf.org/html/rfc7628) ? I could not find any while
> > > originally implementing this.
> > >
> > > Best,
> > > Stanislav
> > >
> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino 
> > wrote:
> > >>
> > >> 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 clarity on immutability, and it also
> > >> eliminates copying, like this:
> > >>
> > >> SaslExtensions myMethod() {
> > >>Map myRetval = getUnmodifiableSaslExtensionsMap();
> > >>return new SaslExtensions() {
> > >>public Map map() {
> > >>return myRetval;
> > >>}
> > >>}
> > >> }
> > >>
> > >> Alternatively, we could do it like this:
> > >>
> > >> /**
> > >> * Supplier that returns immutable map view of SASL Extensions
> > >> */
> > >> public interface SaslExtensions extends Supplier>
> {
> > >>// empty
> > >> }
> > >>
> > >> The we could simply return the instance like this, again without
> > copying:
> > >>
> > >> SaslExtensions myMethod() {
> > >>Map myRetval = getUnmodifiableSaslExtensionsMap();
> > >>return () -> myRetval;
> > >> }
> > >>
> > >> I think the main reason for making SaslExtensions part of the public
> > >> interface is to avoid adding a Map to the Subject's public
> credentials.
> > >> Making SaslExtensions an interface meets that requirement and then
> > allows
> > >> us to be free to implement whatever we want internally.
> > >>
> > >> Thoughts?
> > >>
> > >> Ron
> > >>
> > >>> On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino 
> > wrote:
> > >>>
> > >>> 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 extensionValue(String) methods. It is a smaller API (2 methods
> > >> instead
> > >>> of 1), and it gives clients of the API full map-related functionality
> > >>> (there’s a lot of support for dealing with maps in a variety of
> ways).
> > >>>
> > >>> Regardless of whether we go with a map() method or extensionNames()
> and
> > >>> extensionValue(String) methods, the semantics of mutability need to
> be
> > >>> clear.  I think either way we should never share a map that anyone
> else
> > >>> could possibly mutate — either a map that someone gives us or a map
> > that
> > >> we
> > >>> might expose.
> > >>>
> > >>> Thoughts?
> > >>>
> > >>> Ron
> 

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

[Rajini Sivaram]

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 the public API
should be sufficient in the vast majority of the cases. Ron, do you agree?

On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino  wrote:

> 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 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 sure they're passing in an unmodifiable map
> > themselves. I believe it would be better if we enforced that through
> class
> > constructors instead.
> > SaslExtensions#map() - I'd also prefer this. The reason I went with
> > `extensionValue` and `extensionNames` was because I figured it made sense
> > to have `ScramExtensions` extend `SaslExtensions` and therefore have
> their
> > API be similar. In the end, do you think that it is worth it to have
> > `ScramExtensions` extend `SaslExtensions`?
> > @Ron, could you point me to the SASL OAuth mechanism specific regular
> > expressions for keys/values you mentioned are in RFC 7628 (
> > https://tools.ietf.org/html/rfc7628) ? I could not find any while
> > originally implementing this.
> >
> > Best,
> > Stanislav
> >
> >> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino 
> wrote:
> >>
> >> 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 clarity on immutability, and it also
> >> eliminates copying, like this:
> >>
> >> SaslExtensions myMethod() {
> >>Map myRetval = getUnmodifiableSaslExtensionsMap();
> >>return new SaslExtensions() {
> >>public Map map() {
> >>return myRetval;
> >>}
> >>}
> >> }
> >>
> >> Alternatively, we could do it like this:
> >>
> >> /**
> >> * Supplier that returns immutable map view of SASL Extensions
> >> */
> >> public interface SaslExtensions extends Supplier> {
> >>// empty
> >> }
> >>
> >> The we could simply return the instance like this, again without
> copying:
> >>
> >> SaslExtensions myMethod() {
> >>Map myRetval = getUnmodifiableSaslExtensionsMap();
> >>return () -> myRetval;
> >> }
> >>
> >> I think the main reason for making SaslExtensions part of the public
> >> interface is to avoid adding a Map to the Subject's public credentials.
> >> Making SaslExtensions an interface meets that requirement and then
> allows
> >> us to be free to implement whatever we want internally.
> >>
> >> Thoughts?
> >>
> >> Ron
> >>
> >>> On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino 
> wrote:
> >>>
> >>> 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 extensionValue(String) methods. It is a smaller API (2 methods
> >> instead
> >>> of 1), and it gives clients of the API full map-related functionality
> >>> (there’s a lot of support for dealing with maps in a variety of ways).
> >>>
> >>> Regardless of whether we go with a map() method or extensionNames() and
> >>> extensionValue(String) methods, the semantics of mutability need to be
> >>> clear.  I think either way we should never share a map that anyone else
> >>> could possibly mutate — either a map that someone gives us or a map
> that
> >> we
> >>> might expose.
> >>>
> >>> Thoughts?
> >>>
> >>> Ron
> >>>
>  On Jul 22, 2018, at 11:23 AM, Rajini Sivaram  >
> >>> wrote:
> 
>  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 mechanism in use. The separator
> >> is
>  not configurable and should not be a concern of the implementor of
>  SaslExtensionsCallback interface that provides an instance of
> >>> SaslExtensions
>  .
> 
>  2. I agree with Ron that we need 

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

[Ron Dagostino]

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 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 sure they're passing in an unmodifiable map
> themselves. I believe it would be better if we enforced that through class
> constructors instead.
> SaslExtensions#map() - I'd also prefer this. The reason I went with
> `extensionValue` and `extensionNames` was because I figured it made sense
> to have `ScramExtensions` extend `SaslExtensions` and therefore have their
> API be similar. In the end, do you think that it is worth it to have
> `ScramExtensions` extend `SaslExtensions`?
> @Ron, could you point me to the SASL OAuth mechanism specific regular
> expressions for keys/values you mentioned are in RFC 7628 (
> https://tools.ietf.org/html/rfc7628) ? I could not find any while
> originally implementing this.
> 
> Best,
> Stanislav
> 
>> On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino  wrote:
>> 
>> 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 clarity on immutability, and it also
>> eliminates copying, like this:
>> 
>> SaslExtensions myMethod() {
>>Map myRetval = getUnmodifiableSaslExtensionsMap();
>>return new SaslExtensions() {
>>public Map map() {
>>return myRetval;
>>}
>>}
>> }
>> 
>> Alternatively, we could do it like this:
>> 
>> /**
>> * Supplier that returns immutable map view of SASL Extensions
>> */
>> public interface SaslExtensions extends Supplier> {
>>// empty
>> }
>> 
>> The we could simply return the instance like this, again without copying:
>> 
>> SaslExtensions myMethod() {
>>Map myRetval = getUnmodifiableSaslExtensionsMap();
>>return () -> myRetval;
>> }
>> 
>> I think the main reason for making SaslExtensions part of the public
>> interface is to avoid adding a Map to the Subject's public credentials.
>> Making SaslExtensions an interface meets that requirement and then allows
>> us to be free to implement whatever we want internally.
>> 
>> Thoughts?
>> 
>> Ron
>> 
>>> On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino  wrote:
>>> 
>>> 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 extensionValue(String) methods. It is a smaller API (2 methods
>> instead
>>> of 1), and it gives clients of the API full map-related functionality
>>> (there’s a lot of support for dealing with maps in a variety of ways).
>>> 
>>> Regardless of whether we go with a map() method or extensionNames() and
>>> extensionValue(String) methods, the semantics of mutability need to be
>>> clear.  I think either way we should never share a map that anyone else
>>> could possibly mutate — either a map that someone gives us or a map that
>> we
>>> might expose.
>>> 
>>> Thoughts?
>>> 
>>> Ron
>>> 
 On Jul 22, 2018, at 11:23 AM, Rajini Sivaram 
>>> wrote:
 
 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 mechanism in use. The separator
>> is
 not configurable and should not be a concern of the implementor of
 SaslExtensionsCallback interface that provides an instance of
>>> SaslExtensions
 .
 
 2. I agree with Ron that we need mechanism-specific validation of the
 values from SaslExtensions. But I think we could do the validation in
>> the
 appropriate `SaslClient` implementation of that mechanism.
 
 I think we could just have a very simple extensions class and move
 everything else to appropriate internal classes of the mechanisms using
 extensions. What do you think?
 
 public class SaslExtensions {
   private final Map extensionMap;
 
   public SaslExtensions(Map extensionMap) {
   this.extensionMap = extensionMap;
   }
 
   public String extensionValue(String name) {
   return extensionMap.get(name);
   }
 
  

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

[Stanislav Kozlovski]

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 sure they're passing in an unmodifiable map
themselves. I believe it would be better if we enforced that through class
constructors instead.
SaslExtensions#map() - I'd also prefer this. The reason I went with
`extensionValue` and `extensionNames` was because I figured it made sense
to have `ScramExtensions` extend `SaslExtensions` and therefore have their
API be similar. In the end, do you think that it is worth it to have
`ScramExtensions` extend `SaslExtensions`?
@Ron, could you point me to the SASL OAuth mechanism specific regular
expressions for keys/values you mentioned are in RFC 7628 (
https://tools.ietf.org/html/rfc7628) ? I could not find any while
originally implementing this.

Best,
Stanislav

On Sun, Jul 22, 2018 at 6:46 PM Ron Dagostino  wrote:

> 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 clarity on immutability, and it also
> eliminates copying, like this:
>
> SaslExtensions myMethod() {
> Map myRetval = getUnmodifiableSaslExtensionsMap();
> return new SaslExtensions() {
> public Map map() {
> return myRetval;
> }
> }
> }
>
> Alternatively, we could do it like this:
>
> /**
>  * Supplier that returns immutable map view of SASL Extensions
>  */
> public interface SaslExtensions extends Supplier> {
> // empty
> }
>
> The we could simply return the instance like this, again without copying:
>
> SaslExtensions myMethod() {
> Map myRetval = getUnmodifiableSaslExtensionsMap();
> return () -> myRetval;
> }
>
> I think the main reason for making SaslExtensions part of the public
> interface is to avoid adding a Map to the Subject's public credentials.
> Making SaslExtensions an interface meets that requirement and then allows
> us to be free to implement whatever we want internally.
>
> Thoughts?
>
> Ron
>
> On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino  wrote:
>
> > 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 extensionValue(String) methods. It is a smaller API (2 methods
> instead
> > of 1), and it gives clients of the API full map-related functionality
> > (there’s a lot of support for dealing with maps in a variety of ways).
> >
> > Regardless of whether we go with a map() method or extensionNames() and
> > extensionValue(String) methods, the semantics of mutability need to be
> > clear.  I think either way we should never share a map that anyone else
> > could possibly mutate — either a map that someone gives us or a map that
> we
> > might expose.
> >
> > Thoughts?
> >
> > Ron
> >
> > > On Jul 22, 2018, at 11:23 AM, Rajini Sivaram 
> > wrote:
> > >
> > > 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 mechanism in use. The separator
> is
> > > not configurable and should not be a concern of the implementor of
> > > SaslExtensionsCallback interface that provides an instance of
> > SaslExtensions
> > > .
> > >
> > > 2. I agree with Ron that we need mechanism-specific validation of the
> > > values from SaslExtensions. But I think we could do the validation in
> the
> > > appropriate `SaslClient` implementation of that mechanism.
> > >
> > > I think we could just have a very simple extensions class and move
> > > everything else to appropriate internal classes of the mechanisms using
> > > extensions. What do you think?
> > >
> > > public class SaslExtensions {
> > >private final Map extensionMap;
> > >
> > >public SaslExtensions(Map extensionMap) {
> > >this.extensionMap = extensionMap;
> > >}
> > >
> > >public String extensionValue(String name) {
> > >return extensionMap.get(name);
> > >}
> > >
> > >public Set extensionNames() {
> > >return extensionMap.keySet();
> > >}
> > > }
> > >
> > >
> > >
> > >> On Sat, Jul 21, 2018 at 9:01 PM, Ron Dagostino 
> > wrote:
> > >>
> > >> Hi Stanislav and Rajini.  If SaslExtensions is going to part of the
> > public
> > >> API, then it occurred to me that one of the 

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

[Ron Dagostino]

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 clarity on immutability, and it also
eliminates copying, like this:

SaslExtensions myMethod() {
Map myRetval = getUnmodifiableSaslExtensionsMap();
return new SaslExtensions() {
public Map map() {
return myRetval;
}
}
}

Alternatively, we could do it like this:

/**
 * Supplier that returns immutable map view of SASL Extensions
 */
public interface SaslExtensions extends Supplier> {
// empty
}

The we could simply return the instance like this, again without copying:

SaslExtensions myMethod() {
Map myRetval = getUnmodifiableSaslExtensionsMap();
return () -> myRetval;
}

I think the main reason for making SaslExtensions part of the public
interface is to avoid adding a Map to the Subject's public credentials.
Making SaslExtensions an interface meets that requirement and then allows
us to be free to implement whatever we want internally.

Thoughts?

Ron

On Sun, Jul 22, 2018 at 12:45 PM Ron Dagostino  wrote:

> 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 extensionValue(String) methods. It is a smaller API (2 methods instead
> of 1), and it gives clients of the API full map-related functionality
> (there’s a lot of support for dealing with maps in a variety of ways).
>
> Regardless of whether we go with a map() method or extensionNames() and
> extensionValue(String) methods, the semantics of mutability need to be
> clear.  I think either way we should never share a map that anyone else
> could possibly mutate — either a map that someone gives us or a map that we
> might expose.
>
> Thoughts?
>
> Ron
>
> > On Jul 22, 2018, at 11:23 AM, Rajini Sivaram 
> wrote:
> >
> > 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 mechanism in use. The separator is
> > not configurable and should not be a concern of the implementor of
> > SaslExtensionsCallback interface that provides an instance of
> SaslExtensions
> > .
> >
> > 2. I agree with Ron that we need mechanism-specific validation of the
> > values from SaslExtensions. But I think we could do the validation in the
> > appropriate `SaslClient` implementation of that mechanism.
> >
> > I think we could just have a very simple extensions class and move
> > everything else to appropriate internal classes of the mechanisms using
> > extensions. What do you think?
> >
> > public class SaslExtensions {
> >private final Map extensionMap;
> >
> >public SaslExtensions(Map extensionMap) {
> >this.extensionMap = extensionMap;
> >}
> >
> >public String extensionValue(String name) {
> >return extensionMap.get(name);
> >}
> >
> >public Set extensionNames() {
> >return extensionMap.keySet();
> >}
> > }
> >
> >
> >
> >> On Sat, Jul 21, 2018 at 9:01 PM, Ron Dagostino 
> wrote:
> >>
> >> 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) specifies the regular expressions
> for
> >> the SCRAM-specific SASL mechanisms, and RFC 7628 (
> >> https://tools.ietf.org/html/rfc7628) specifies different regular
> >> expressions for the OAUTHBEARER SASL mechanism.  I am thinking the
> >> SaslExtensions class should probably provide a way to make sure the keys
> >> and values match the appropriate regular expressions.  What do you
> think of
> >> something along the lines of the below definition for the SaslExtensions
> >> class?  It is missing Javadoc and toString()/hashCode()/equals()
> methods,
> >> of course, but aside from that, do you think this is sufficient and
> >> appropriate?
> >>
> >> Ron
> >>
> >> public class SaslExtensions {
> >>private final Map extensionsMap;
> >>
> >>public SaslExtensions(String mapStr, String keyValueSeparator, String
> >> elementSeparator,
> >>Pattern saslNameRegexPattern, Pattern saslValueRegexPattern)
> {
> >>this(Utils.parseMap(mapStr, keyValueSeparator, elementSeparator),
> >> saslNameRegexPattern, saslValueRegexPattern);
> >>}
> >>
> >>public SaslExtensions(Map 

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

[Ron Dagostino]

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 
extensionValue(String) methods. It is a smaller API (2 methods instead of 1), 
and it gives clients of the API full map-related functionality (there’s a lot 
of support for dealing with maps in a variety of ways).

Regardless of whether we go with a map() method or extensionNames() and 
extensionValue(String) methods, the semantics of mutability need to be clear.  
I think either way we should never share a map that anyone else could possibly 
mutate — either a map that someone gives us or a map that we might expose.

Thoughts?

Ron

> On Jul 22, 2018, at 11:23 AM, Rajini Sivaram  wrote:
> 
> 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 mechanism in use. The separator is
> not configurable and should not be a concern of the implementor of
> SaslExtensionsCallback interface that provides an instance of SaslExtensions
> .
> 
> 2. I agree with Ron that we need mechanism-specific validation of the
> values from SaslExtensions. But I think we could do the validation in the
> appropriate `SaslClient` implementation of that mechanism.
> 
> I think we could just have a very simple extensions class and move
> everything else to appropriate internal classes of the mechanisms using
> extensions. What do you think?
> 
> public class SaslExtensions {
>private final Map extensionMap;
> 
>public SaslExtensions(Map extensionMap) {
>this.extensionMap = extensionMap;
>}
> 
>public String extensionValue(String name) {
>return extensionMap.get(name);
>}
> 
>public Set extensionNames() {
>return extensionMap.keySet();
>}
> }
> 
> 
> 
>> On Sat, Jul 21, 2018 at 9:01 PM, Ron Dagostino  wrote:
>> 
>> 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) specifies the regular expressions for
>> the SCRAM-specific SASL mechanisms, and RFC 7628 (
>> https://tools.ietf.org/html/rfc7628) specifies different regular
>> expressions for the OAUTHBEARER SASL mechanism.  I am thinking the
>> SaslExtensions class should probably provide a way to make sure the keys
>> and values match the appropriate regular expressions.  What do you think of
>> something along the lines of the below definition for the SaslExtensions
>> class?  It is missing Javadoc and toString()/hashCode()/equals() methods,
>> of course, but aside from that, do you think this is sufficient and
>> appropriate?
>> 
>> Ron
>> 
>> public class SaslExtensions {
>>private final Map extensionsMap;
>> 
>>public SaslExtensions(String mapStr, String keyValueSeparator, String
>> elementSeparator,
>>Pattern saslNameRegexPattern, Pattern saslValueRegexPattern) {
>>this(Utils.parseMap(mapStr, keyValueSeparator, elementSeparator),
>> saslNameRegexPattern, saslValueRegexPattern);
>>}
>> 
>>public SaslExtensions(Map extensionsMap, Pattern
>> saslNameRegexPattern,
>>Pattern saslValueRegexPattern) {
>>Map sanitizedCopy = new
>> HashMap<>(extensionsMap.size());
>>for (Entry entry : extensionsMap.entrySet()) {
>>if (!saslNameRegexPattern.matcher(entry.getKey()).matches()
>>||
>> !saslValueRegexPattern.matcher(entry.getValue()).matches())
>>throw new IllegalArgumentException("Invalid key or
>> value");
>>sanitizedCopy.put(entry.getKey(), entry.getValue());
>>}
>>this.extensionsMap = Collections.unmodifiableMap(sanitizedCopy);
>>}
>> 
>>public Map map() {
>>return extensionsMap;
>>}
>> }
>> 
>> On Fri, Jul 20, 2018 at 12:49 PM Stanislav Kozlovski <
>> stanis...@confluent.io>
>> wrote:
>> 
>>> 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:
 
 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 

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

[Rajini Sivaram]

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 mechanism in use. The separator is
not configurable and should not be a concern of the implementor of
SaslExtensionsCallback interface that provides an instance of SaslExtensions
.

2. I agree with Ron that we need mechanism-specific validation of the
values from SaslExtensions. But I think we could do the validation in the
appropriate `SaslClient` implementation of that mechanism.

I think we could just have a very simple extensions class and move
everything else to appropriate internal classes of the mechanisms using
extensions. What do you think?

public class SaslExtensions {
private final Map extensionMap;

public SaslExtensions(Map extensionMap) {
this.extensionMap = extensionMap;
}

public String extensionValue(String name) {
return extensionMap.get(name);
}

public Set extensionNames() {
return extensionMap.keySet();
}
}



On Sat, Jul 21, 2018 at 9:01 PM, Ron Dagostino  wrote:

> 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) specifies the regular expressions for
> the SCRAM-specific SASL mechanisms, and RFC 7628 (
> https://tools.ietf.org/html/rfc7628) specifies different regular
> expressions for the OAUTHBEARER SASL mechanism.  I am thinking the
> SaslExtensions class should probably provide a way to make sure the keys
> and values match the appropriate regular expressions.  What do you think of
> something along the lines of the below definition for the SaslExtensions
> class?  It is missing Javadoc and toString()/hashCode()/equals() methods,
> of course, but aside from that, do you think this is sufficient and
> appropriate?
>
> Ron
>
> public class SaslExtensions {
> private final Map extensionsMap;
>
> public SaslExtensions(String mapStr, String keyValueSeparator, String
> elementSeparator,
> Pattern saslNameRegexPattern, Pattern saslValueRegexPattern) {
> this(Utils.parseMap(mapStr, keyValueSeparator, elementSeparator),
> saslNameRegexPattern, saslValueRegexPattern);
> }
>
> public SaslExtensions(Map extensionsMap, Pattern
> saslNameRegexPattern,
> Pattern saslValueRegexPattern) {
> Map sanitizedCopy = new
> HashMap<>(extensionsMap.size());
> for (Entry entry : extensionsMap.entrySet()) {
> if (!saslNameRegexPattern.matcher(entry.getKey()).matches()
> ||
> !saslValueRegexPattern.matcher(entry.getValue()).matches())
> throw new IllegalArgumentException("Invalid key or
> value");
> sanitizedCopy.put(entry.getKey(), entry.getValue());
> }
> this.extensionsMap = Collections.unmodifiableMap(sanitizedCopy);
> }
>
> public Map map() {
> return extensionsMap;
> }
> }
>
> On Fri, Jul 20, 2018 at 12:49 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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:
> >
> > > 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 the public credentials defines the SASL extensions and
> we
> > > can never add another instance implementing Map to the public
> > credentials.
> > > That's a very big constraint we are committing to, and I'm wondering if
> > we
> > > should make SaslExtensions public and attach an instance of that to the
> > > Subject's public credentials instead.
> > >
> > > Ron
> > >
> > > On Thu, Jul 19, 2018 at 8:15 PM Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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 
> > > >
> > > > Best,
> > > > Stanislav
> > > >
> > > > On Thu, Jul 19, 2018 at 1:58 

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

[Ron Dagostino]

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) specifies the regular expressions for
the SCRAM-specific SASL mechanisms, and RFC 7628 (
https://tools.ietf.org/html/rfc7628) specifies different regular
expressions for the OAUTHBEARER SASL mechanism.  I am thinking the
SaslExtensions class should probably provide a way to make sure the keys
and values match the appropriate regular expressions.  What do you think of
something along the lines of the below definition for the SaslExtensions
class?  It is missing Javadoc and toString()/hashCode()/equals() methods,
of course, but aside from that, do you think this is sufficient and
appropriate?

Ron

public class SaslExtensions {
private final Map extensionsMap;

public SaslExtensions(String mapStr, String keyValueSeparator, String
elementSeparator,
Pattern saslNameRegexPattern, Pattern saslValueRegexPattern) {
this(Utils.parseMap(mapStr, keyValueSeparator, elementSeparator),
saslNameRegexPattern, saslValueRegexPattern);
}

public SaslExtensions(Map extensionsMap, Pattern
saslNameRegexPattern,
Pattern saslValueRegexPattern) {
Map sanitizedCopy = new
HashMap<>(extensionsMap.size());
for (Entry entry : extensionsMap.entrySet()) {
if (!saslNameRegexPattern.matcher(entry.getKey()).matches()
||
!saslValueRegexPattern.matcher(entry.getValue()).matches())
throw new IllegalArgumentException("Invalid key or value");
sanitizedCopy.put(entry.getKey(), entry.getValue());
}
this.extensionsMap = Collections.unmodifiableMap(sanitizedCopy);
}

public Map map() {
return extensionsMap;
}
}

On Fri, Jul 20, 2018 at 12:49 PM Stanislav Kozlovski 
wrote:

> 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:
>
> > 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 the public credentials defines the SASL extensions and we
> > can never add another instance implementing Map to the public
> credentials.
> > That's a very big constraint we are committing to, and I'm wondering if
> we
> > should make SaslExtensions public and attach an instance of that to the
> > Subject's public credentials instead.
> >
> > Ron
> >
> > On Thu, Jul 19, 2018 at 8:15 PM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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 
> > >
> > > Best,
> > > Stanislav
> > >
> > > On Thu, Jul 19, 2018 at 1:58 PM Stanislav Kozlovski <
> > > stanis...@confluent.io>
> > > wrote:
> > >
> > > > 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 guess it would be better to throw, as to
> > > avoid
> > > > confusion.
> > > >
> > > > And yes, I will make sure the key/value are validated on the client
> as
> > > > well as in the server. Even then, I structured the
> > > getNegotiatedProperty()
> > > > method such that the OAUTHBEARER.token can never be overridden. I
> > > > considered adding a test for that, but I figured having the regex
> > > > validation be enough of a guarantee.
> > > >
> > > > On Thu, Jul 19, 2018 at 9:49 AM Ron Dagostino 
> > wrote:
> > > >
> > > >> 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 should ask its login callback handler to
> handle
> > > an
> > > >> instance of SaslExtensionsCallback in addition to an instance of
> > > >> 

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

[Stanislav Kozlovski]

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:

> 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 the public credentials defines the SASL extensions and we
> can never add another instance implementing Map to the public credentials.
> That's a very big constraint we are committing to, and I'm wondering if we
> should make SaslExtensions public and attach an instance of that to the
> Subject's public credentials instead.
>
> Ron
>
> On Thu, Jul 19, 2018 at 8:15 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 
> >
> > Best,
> > Stanislav
> >
> > On Thu, Jul 19, 2018 at 1:58 PM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > 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 guess it would be better to throw, as to
> > avoid
> > > confusion.
> > >
> > > And yes, I will make sure the key/value are validated on the client as
> > > well as in the server. Even then, I structured the
> > getNegotiatedProperty()
> > > method such that the OAUTHBEARER.token can never be overridden. I
> > > considered adding a test for that, but I figured having the regex
> > > validation be enough of a guarantee.
> > >
> > > On Thu, Jul 19, 2018 at 9:49 AM Ron Dagostino 
> wrote:
> > >
> > >> 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 should ask its login callback handler to handle
> > an
> > >> instance of SaslExtensionsCallback in addition to an instance of
> > >> OAuthBearerTokenCallback, and the default login callback handler
> > >> implementation (OAuthBearerUnsecuredLoginCallbackHandler) should
> either
> > >> return an empty map via callback or it should recognize additional
> JAAS
> > >> module options of the form
> unsecuredLoginExtension_=value
> > >> so
> > >> that arbitrary extensions can be added in development and test
> scenarios
> > >> (similar to how arbitrary claims on unsecured tokens can be created in
> > >> those scenarios via the JAAS module options
> > >> unsecuredLoginStringClaim_=value, etc.).  Then the
> > >> OAuthBearerLoginModule can add a map of any extensions to the
> Subject's
> > >> public credentials where the default SASL client callback handler
> class
> > (
> > >> OAuthBearerSaslClientCallbackHandler) can be amended to support
> > >> SaslExtensionsCallback and look on the Subject accordingly.  There
> would
> > >> be
> > >> no need to implement a custom sasl.client.callback.handler.class in
> this
> > >> case, and no logic would need to be moved to a public static method on
> > >> OAuthBearerLoginModule as I had proposed (at least not right now,
> anyway
> > >> --
> > >> there may come a time when a need for a custom
> > >> sasl.client.callback.handler.class is identified, and at that point
> the
> > >> default implementation would either have to made part of the public
> API
> > >> with protected rather than private methods so it could be directly
> > >> extended
> > >> or its logic would have to be moved to public static methods on
> > >> OAuthBearerLoginModule).
> > >>
> > >> So, to try to summarize, I think SaslExtensionsCallback will be the
> only
> > >> public API addition due to this KIP in terms of code, and then maybe
> the
> > >> recognition of the unsecuredLoginExtension_=value
> module
> > >> options in the default unsecured case (which would be a documentation
> > >> change and an internal implementation issue rather than a public API
> in
> > >> terms of code).  And then also the fact that extension names and
> values
> > >> are
> > >> accessed on the server side via negotiated properties.  Do I have that
> > >> summary right?
> > >>
> > >> One 

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

[Ron Dagostino]

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 the public credentials defines the SASL extensions and we
can never add another instance implementing Map to the public credentials.
That's a very big constraint we are committing to, and I'm wondering if we
should make SaslExtensions public and attach an instance of that to the
Subject's public credentials instead.

Ron

On Thu, Jul 19, 2018 at 8:15 PM Stanislav Kozlovski 
wrote:

> 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 
>
> Best,
> Stanislav
>
> On Thu, Jul 19, 2018 at 1:58 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 guess it would be better to throw, as to
> avoid
> > confusion.
> >
> > And yes, I will make sure the key/value are validated on the client as
> > well as in the server. Even then, I structured the
> getNegotiatedProperty()
> > method such that the OAUTHBEARER.token can never be overridden. I
> > considered adding a test for that, but I figured having the regex
> > validation be enough of a guarantee.
> >
> > On Thu, Jul 19, 2018 at 9:49 AM Ron Dagostino  wrote:
> >
> >> 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 should ask its login callback handler to handle
> an
> >> instance of SaslExtensionsCallback in addition to an instance of
> >> OAuthBearerTokenCallback, and the default login callback handler
> >> implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either
> >> return an empty map via callback or it should recognize additional JAAS
> >> module options of the form unsecuredLoginExtension_=value
> >> so
> >> that arbitrary extensions can be added in development and test scenarios
> >> (similar to how arbitrary claims on unsecured tokens can be created in
> >> those scenarios via the JAAS module options
> >> unsecuredLoginStringClaim_=value, etc.).  Then the
> >> OAuthBearerLoginModule can add a map of any extensions to the Subject's
> >> public credentials where the default SASL client callback handler class
> (
> >> OAuthBearerSaslClientCallbackHandler) can be amended to support
> >> SaslExtensionsCallback and look on the Subject accordingly.  There would
> >> be
> >> no need to implement a custom sasl.client.callback.handler.class in this
> >> case, and no logic would need to be moved to a public static method on
> >> OAuthBearerLoginModule as I had proposed (at least not right now, anyway
> >> --
> >> there may come a time when a need for a custom
> >> sasl.client.callback.handler.class is identified, and at that point the
> >> default implementation would either have to made part of the public API
> >> with protected rather than private methods so it could be directly
> >> extended
> >> or its logic would have to be moved to public static methods on
> >> OAuthBearerLoginModule).
> >>
> >> So, to try to summarize, I think SaslExtensionsCallback will be the only
> >> public API addition due to this KIP in terms of code, and then maybe the
> >> recognition of the unsecuredLoginExtension_=value module
> >> options in the default unsecured case (which would be a documentation
> >> change and an internal implementation issue rather than a public API in
> >> terms of code).  And then also the fact that extension names and values
> >> are
> >> accessed on the server side via negotiated properties.  Do I have that
> >> summary right?
> >>
> >> One thing I want to note is that the code needs to make sure the
> extension
> >> names are composed of only ALPHA [a-zA-Z] characters as per the spec
> (not
> >> only for that reason, but to also make sure the token available at the
> >> OAUTHBEARER.token negotiated property can't be overwritten).
> >>
> >> Ron
> >>
> >> On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski <
> >> stanis...@confluent.io>
> >> wrote:
> >>
> >> > Hey Ron,
> >> >
> >> > Come to think of it, I think what Rajini said makes more sense than my
> >> > initial proposal. Having 

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

[Stanislav Kozlovski]

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 

Best,
Stanislav

On Thu, Jul 19, 2018 at 1:58 PM Stanislav Kozlovski 
wrote:

> 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 guess it would be better to throw, as to avoid
> confusion.
>
> And yes, I will make sure the key/value are validated on the client as
> well as in the server. Even then, I structured the getNegotiatedProperty()
> method such that the OAUTHBEARER.token can never be overridden. I
> considered adding a test for that, but I figured having the regex
> validation be enough of a guarantee.
>
> On Thu, Jul 19, 2018 at 9:49 AM Ron Dagostino  wrote:
>
>> 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 should ask its login callback handler to handle an
>> instance of SaslExtensionsCallback in addition to an instance of
>> OAuthBearerTokenCallback, and the default login callback handler
>> implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either
>> return an empty map via callback or it should recognize additional JAAS
>> module options of the form unsecuredLoginExtension_=value
>> so
>> that arbitrary extensions can be added in development and test scenarios
>> (similar to how arbitrary claims on unsecured tokens can be created in
>> those scenarios via the JAAS module options
>> unsecuredLoginStringClaim_=value, etc.).  Then the
>> OAuthBearerLoginModule can add a map of any extensions to the Subject's
>> public credentials where the default SASL client callback handler class (
>> OAuthBearerSaslClientCallbackHandler) can be amended to support
>> SaslExtensionsCallback and look on the Subject accordingly.  There would
>> be
>> no need to implement a custom sasl.client.callback.handler.class in this
>> case, and no logic would need to be moved to a public static method on
>> OAuthBearerLoginModule as I had proposed (at least not right now, anyway
>> --
>> there may come a time when a need for a custom
>> sasl.client.callback.handler.class is identified, and at that point the
>> default implementation would either have to made part of the public API
>> with protected rather than private methods so it could be directly
>> extended
>> or its logic would have to be moved to public static methods on
>> OAuthBearerLoginModule).
>>
>> So, to try to summarize, I think SaslExtensionsCallback will be the only
>> public API addition due to this KIP in terms of code, and then maybe the
>> recognition of the unsecuredLoginExtension_=value module
>> options in the default unsecured case (which would be a documentation
>> change and an internal implementation issue rather than a public API in
>> terms of code).  And then also the fact that extension names and values
>> are
>> accessed on the server side via negotiated properties.  Do I have that
>> summary right?
>>
>> One thing I want to note is that the code needs to make sure the extension
>> names are composed of only ALPHA [a-zA-Z] characters as per the spec (not
>> only for that reason, but to also make sure the token available at the
>> OAUTHBEARER.token negotiated property can't be overwritten).
>>
>> Ron
>>
>> On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski <
>> stanis...@confluent.io>
>> wrote:
>>
>> > 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 > >
>> > 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
>> > 

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

[Stanislav Kozlovski]

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 guess it would be better to throw, as to avoid
confusion.

And yes, I will make sure the key/value are validated on the client as well
as in the server. Even then, I structured the getNegotiatedProperty()
method such that the OAUTHBEARER.token can never be overridden. I
considered adding a test for that, but I figured having the regex
validation be enough of a guarantee.

On Thu, Jul 19, 2018 at 9:49 AM Ron Dagostino  wrote:

> 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 should ask its login callback handler to handle an
> instance of SaslExtensionsCallback in addition to an instance of
> OAuthBearerTokenCallback, and the default login callback handler
> implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either
> return an empty map via callback or it should recognize additional JAAS
> module options of the form unsecuredLoginExtension_=value so
> that arbitrary extensions can be added in development and test scenarios
> (similar to how arbitrary claims on unsecured tokens can be created in
> those scenarios via the JAAS module options
> unsecuredLoginStringClaim_=value, etc.).  Then the
> OAuthBearerLoginModule can add a map of any extensions to the Subject's
> public credentials where the default SASL client callback handler class (
> OAuthBearerSaslClientCallbackHandler) can be amended to support
> SaslExtensionsCallback and look on the Subject accordingly.  There would be
> no need to implement a custom sasl.client.callback.handler.class in this
> case, and no logic would need to be moved to a public static method on
> OAuthBearerLoginModule as I had proposed (at least not right now, anyway --
> there may come a time when a need for a custom
> sasl.client.callback.handler.class is identified, and at that point the
> default implementation would either have to made part of the public API
> with protected rather than private methods so it could be directly extended
> or its logic would have to be moved to public static methods on
> OAuthBearerLoginModule).
>
> So, to try to summarize, I think SaslExtensionsCallback will be the only
> public API addition due to this KIP in terms of code, and then maybe the
> recognition of the unsecuredLoginExtension_=value module
> options in the default unsecured case (which would be a documentation
> change and an internal implementation issue rather than a public API in
> terms of code).  And then also the fact that extension names and values are
> accessed on the server side via negotiated properties.  Do I have that
> summary right?
>
> One thing I want to note is that the code needs to make sure the extension
> names are composed of only ALPHA [a-zA-Z] characters as per the spec (not
> only for that reason, but to also make sure the token available at the
> OAUTHBEARER.token negotiated property can't be overwritten).
>
> Ron
>
> On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 
> > 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 

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

[Rajini Sivaram]

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 response as-is to conform to the spec.

Regards,

Rajini

On Thu, Jul 19, 2018 at 5:49 PM, Ron Dagostino  wrote:

> 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 should ask its login callback handler to handle an
> instance of SaslExtensionsCallback in addition to an instance of
> OAuthBearerTokenCallback, and the default login callback handler
> implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either
> return an empty map via callback or it should recognize additional JAAS
> module options of the form unsecuredLoginExtension_=value
> so
> that arbitrary extensions can be added in development and test scenarios
> (similar to how arbitrary claims on unsecured tokens can be created in
> those scenarios via the JAAS module options
> unsecuredLoginStringClaim_=value, etc.).  Then the
> OAuthBearerLoginModule can add a map of any extensions to the Subject's
> public credentials where the default SASL client callback handler class (
> OAuthBearerSaslClientCallbackHandler) can be amended to support
> SaslExtensionsCallback and look on the Subject accordingly.  There would be
> no need to implement a custom sasl.client.callback.handler.class in this
> case, and no logic would need to be moved to a public static method on
> OAuthBearerLoginModule as I had proposed (at least not right now, anyway --
> there may come a time when a need for a custom
> sasl.client.callback.handler.class is identified, and at that point the
> default implementation would either have to made part of the public API
> with protected rather than private methods so it could be directly extended
> or its logic would have to be moved to public static methods on
> OAuthBearerLoginModule).
>
> So, to try to summarize, I think SaslExtensionsCallback will be the only
> public API addition due to this KIP in terms of code, and then maybe the
> recognition of the unsecuredLoginExtension_=value module
> options in the default unsecured case (which would be a documentation
> change and an internal implementation issue rather than a public API in
> terms of code).  And then also the fact that extension names and values are
> accessed on the server side via negotiated properties.  Do I have that
> summary right?
>
> One thing I want to note is that the code needs to make sure the extension
> names are composed of only ALPHA [a-zA-Z] characters as per the spec (not
> only for that reason, but to also make sure the token available at the
> OAUTHBEARER.token negotiated property can't be overwritten).
>
> Ron
>
> On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > 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 
> > 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,
> > > 

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

[Ron Dagostino]

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 should ask its login callback handler to handle an
instance of SaslExtensionsCallback in addition to an instance of
OAuthBearerTokenCallback, and the default login callback handler
implementation (OAuthBearerUnsecuredLoginCallbackHandler) should either
return an empty map via callback or it should recognize additional JAAS
module options of the form unsecuredLoginExtension_=value so
that arbitrary extensions can be added in development and test scenarios
(similar to how arbitrary claims on unsecured tokens can be created in
those scenarios via the JAAS module options
unsecuredLoginStringClaim_=value, etc.).  Then the
OAuthBearerLoginModule can add a map of any extensions to the Subject's
public credentials where the default SASL client callback handler class (
OAuthBearerSaslClientCallbackHandler) can be amended to support
SaslExtensionsCallback and look on the Subject accordingly.  There would be
no need to implement a custom sasl.client.callback.handler.class in this
case, and no logic would need to be moved to a public static method on
OAuthBearerLoginModule as I had proposed (at least not right now, anyway --
there may come a time when a need for a custom
sasl.client.callback.handler.class is identified, and at that point the
default implementation would either have to made part of the public API
with protected rather than private methods so it could be directly extended
or its logic would have to be moved to public static methods on
OAuthBearerLoginModule).

So, to try to summarize, I think SaslExtensionsCallback will be the only
public API addition due to this KIP in terms of code, and then maybe the
recognition of the unsecuredLoginExtension_=value module
options in the default unsecured case (which would be a documentation
change and an internal implementation issue rather than a public API in
terms of code).  And then also the fact that extension names and values are
accessed on the server side via negotiated properties.  Do I have that
summary right?

One thing I want to note is that the code needs to make sure the extension
names are composed of only ALPHA [a-zA-Z] characters as per the spec (not
only for that reason, but to also make sure the token available at the
OAUTHBEARER.token negotiated property can't be overwritten).

Ron

On Thu, Jul 19, 2018 at 12:43 PM Stanislav Kozlovski 
wrote:

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

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

[Stanislav Kozlovski]

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

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

[Rajini Sivaram]

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 Stanislav
for pointing that out!

Regards,

Rajini

On Thu, Jul 19, 2018 at 4:50 PM, Rajini Sivaram 
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  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
>> ) 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
>> >  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 

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

[Rajini Sivaram]

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

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

[Rajini Sivaram]

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

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

[Ron Dagostino]

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
) is
merged.

Ron

On Wed, Jul 18, 2018 at 6:38 PM Stanislav Kozlovski 
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
>  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  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 

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

[Stanislav Kozlovski]

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
 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  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 
> > PR: Pull request 
> >
> > --
> > Best,
> > Stanislav
> >
>


-- 
Best,
Stanislav

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

[Ron Dagostino]

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 
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 
> PR: Pull request 
>
> --
> Best,
> Stanislav
>

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

[Stanislav Kozlovski]

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 
PR: Pull request 

-- 
Best,
Stanislav