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
> > > > > > > > > > > > > > > > > > > > > > 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
> > > > > > > > > > > > > > > > > >
>
>
>
> --
> Best,
> Stanislav
>
