Hi Stanislav. Here are a few KIP comments.
<<<There are also additional regex validations for extension name and
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.
<<<public OAuthBearerExtensionsValidatorCallback(OAuthBearerToken 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<String, String> 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 <rajinisiva...@gmail.com>
> 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<String, Object> 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 <rndg...@gmail.com> 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<String, Object> 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 <rajinisiva...@gmail.com
> >
> > > 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 <rndg...@gmail.com>
> > 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 <
> > > rajinisiva...@gmail.com
> > > > >
> > > > > > 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 <
> > rndg...@gmail.com>
> > > > > > 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 <
> > > > rndg...@gmail.com>
> > > > > > > > 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_<extensionname>`.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 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
> > > > 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 <
> > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 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_<extensionName>
> > > > > 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
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > 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
> > > > > > > > > > > > > > > > > > > >> > > 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 <
> > > > > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > >> > 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<String, String> map();
> > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> This solves the issue of lack
> of
> > > > > clarity
> > > > > > on
> > > > > > > > > > > > > immutability,
> > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > > it
> > > > > > > > > > > > > > > > > > > also
> > > > > > > > > > > > > > > > > > > >> > >> eliminates copying, like this:
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > > > > > > > > >> > >> Map<String, String>
> myRetval =
> > > > > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsMap();
> > > > > > > > > > > > > > > > > > > >> > >> return new SaslExtensions()
> {
> > > > > > > > > > > > > > > > > > > >> > >> public Map<String,
> String>
> > > > > map() {
> > > > > > > > > > > > > > > > > > > >> > >> return myRetval;
> > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> Alternatively, we could do it
> > like
> > > > > this:
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> /**
> > > > > > > > > > > > > > > > > > > >> > >> * Supplier that returns
> immutable
> > > map
> > > > > > view
> > > > > > > of
> > > > > > > > > > SASL
> > > > > > > > > > > > > > > Extensions
> > > > > > > > > > > > > > > > > > > >> > >> */
> > > > > > > > > > > > > > > > > > > >> > >> public interface SaslExtensions
> > > > extends
> > > > > > > > > > > > > > > Supplier<Map<String,
> > > > > > > > > > > > > > > > > > > >> String>> {
> > > > > > > > > > > > > > > > > > > >> > >> // empty
> > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> The we could simply return the
> > > > instance
> > > > > > > like
> > > > > > > > > > this,
> > > > > > > > > > > > > again
> > > > > > > > > > > > > > > > > without
> > > > > > > > > > > > > > > > > > > >> > copying:
> > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > > > > > > > > >> > >> Map<String, String>
> 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 <
> > > > > > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > >> > 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
> > > > > > > > > <
> > > > > > > > > > > > > > > > > > > >> rajinisiva...@gmail.com
> > > > > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > > > > >> > >>> 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<String,
> > > String>
> > > > > > > > > > extensionMap;
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>> public
> > > SaslExtensions(Map<String,
> > > > > > > String>
> > > > > > > > > > > > > > > extensionMap) {
> > > > > > > > > > > > > > > > > > > >> > >>>> this.extensionMap =
> > > > > extensionMap;
> > > > > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>> public String
> > > > extensionValue(String
> > > > > > > > name) {
> > > > > > > > > > > > > > > > > > > >> > >>>> return
> > > > extensionMap.get(name);
> > > > > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>> public Set<String>
> > > > > extensionNames() {
> > > > > > > > > > > > > > > > > > > >> > >>>> return
> > > extensionMap.keySet();
> > > > > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > > > > >> > >>>>> On Sat, Jul 21, 2018 at 9:01
> > PM,
> > > > Ron
> > > > > > > > > > Dagostino <
> > > > > > > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > > > > > >> >
> > > > > > > > > > > > > > > > > > > >> > >>> 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<String,
> > > > String>
> > > > > > > > > > > extensionsMap;
> > > > > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > > > > >> > >>>>> public
> SaslExtensions(String
> > > > > mapStr,
> > > > > > > > > String
> > > > > > > > > > > > > > > > > > keyValueSeparator,
> > > > > > > > > > > > > > > > > > > >> > >> String
> > > > > > > > > > > > > > > > > > > >> > >>>>> elementSeparator,
> > > > > > > > > > > > > > > > > > > >> > >>>>> Pattern
> > > > > > saslNameRegexPattern,
> > > > > > > > > > Pattern
> > > > > > > > > > > > > > > > > > > >> > >> saslValueRegexPattern)
> > > > > > > > > > > > > > > > > > > >> > >>> {
> > > > > > > > > > > > > > > > > > > >> > >>>>>
> > > this(Utils.parseMap(mapStr,
> > > > > > > > > > > > keyValueSeparator,
> > > > > > > > > > > > > > > > > > > >> > >> elementSeparator),
> > > > > > > > > > > > > > > > > > > >> > >>>>> saslNameRegexPattern,
> > > > > > > > > saslValueRegexPattern);
> > > > > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > > > > >> > >>>>>
> > > > >
