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 <rajinisiva...@gmail.com>
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 <rajinisiva...@gmail.com>
> > 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 <rndg...@gmail.com>
> > wrote:
> > >
> > > > 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 OAuthBearerExtensionsValidator
> > > > Callback
> > > > > > that
> > > > > > provides OAuthBearerToken.
> > > > > >
> > > > > > To summarise, we chose option 2 out of these four options:
> > > > > >
> > > > > > 1. {OAuthBearerValidatorCallback,
> SaslExtensionsValidatorCallbac
> > k}
> > > > :
> > > > > 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
> > > SaslExtensionValidationCallbac
> > > > k
> > > > > 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 OAuthBearerExtensionsValidator
> > > > Callback
> > > > > > > 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 =
> > > > > > > > > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsM
> > ap();
> > > > > > > > > > > > > > > > > > > > > > > >> > >> 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 =
> > > > > > > > > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsM
> > ap();
> > > > > > > > > > > > > > > > > > > > > > > >> > >> return () ->
> myRetval;
> > > > > > > > > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > > > > > > > > >> > >> I think the main reason
> > for
> > > > > > making
> > > > > > > > > > > > > SaslExtensions
> > > > > > > > > > > > > > > > part
> > > > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > > > > the
> > > >
--
Best,
Stanislav
