Hi Rajini. I think we are considering the following two options. Let me
try to describe them along with their relative advantages/disadvantages.
Option #1: Send two callbacks in a single array to the callback handler:
ch.handle(new Callback[] {tokenCallback, extensionsCallback});
Option #2: Send two callbacks separately, in two separate arrays, to the
callback handler:
ch.handle(new Callback[] {tokenCallback});
ch.handle(new Callback[] {extensionsCallback});
I actually don't see any objective disadvantage with #1. If we don't get
an exception then we know we have the information we need; if we do get an
exception then we can tell if the first callback was handled because either
its token() method or its errorStatus() method will return non-null; if
both return null then we just send the token callback by itself and we
don't publish any extension as negotiated properties. There is no
possibility of partial results, and I don't think there is a performance
penalty due to potential re-validation here, either.
I see a subjective disadvantage with #1. It feels awkward to me to
provide the token as context for extension validation via the first
callback.
Actually, it just occurred to me why it feels awkward, and I think this is
an objective disadvantage of this approach. It would be impossible to do
extension validation in such a scenario without also doing token validation
first. We are using the first callback as a way to provide context, but we
are also using that first callback to request token validation. We are
complecting two separate things -- context and a request for validation --
into one thing, so this approach has an element of complexity to it.
The second option has no such complexity. If we want to provide context to
the extension validation then we do that by adding a token to the
extensionsCallback instance before we provide it to the callback handler.
How we do that -- whether by Map<String, Object> or via a typed getter --
feels like a subjective decision, and assuming you agree with the
complexity argument and choose option #2, I would defer to your preference
as to how to implement it.
Ron
On Wed, Aug 8, 2018 at 3:10 PM Rajini Sivaram <rajinisiva...@gmail.com>
wrote:
> Hi Ron,
>
> Yes, I was thinking of a SaslExtensionsValidatorCallback with additional
> context as well initially, but I didn't like the idea of name-value pairs
> and I didn't want generic objects passed around through the callback So
> providing context through other callbacks felt like a neater fit. There
> are pros and cons for both approaches, so we could go with either.
>
> Callbacks are provided to callback handlers in an array and there is
> implicit ordering in the callbacks provided to the callback handler.
> In the typical example of {NameCallback, PasswordCallback}, we expect that
> ordering so that password callback knows what the user name is. Kafka
> guarantees ordering of server callbacks in each of its SASL mechanisms and
> this is explicitly stated in
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-86%3A+Configurable+SASL+callback+handlers
> .
> Until now, we didn't need to worry about ordering for OAuth.
>
> We currently do not have any optional callbacks - configured callback
> handlers have to process all the callbacks for the mechanism or else we
> fail authentication. We have to make SaslExtensionValidationCallback an
> exception, at least for backward compatibility. We will only include this
> callback if the client provided some extensions. I think it is reasonable
> to expect that in general, custom callback handlers will handle this
> callback if clients are likely to set extensions. In case it doesn't, we
> don't want to make any assumptions about which callbacks may have been
> handled. Instead, it would be better to invoke the callback handler again
> without the extensions callback and not expose any extensions as negotiated
> properties. Since we are doing this only for backward compatibility, the
> small performance hit would be reasonable, avoiding any assumptions about
> the callback handler implementation and partial results on hitting an
> exception.
>
> Back to the other approach of providing a Map. For OAuth, we would like
> extension validation to see the actual OAuthBearerToken object, for
> instance to validate extensions based on scope. Having
> these mechanism-specific objects in a Map doesn't feel ideal. It will
> probably be better to define OAuthBearerExtensionsValidatorCallback with a
> token getter that returns OAuthBearerToken in that case.
>
> Thoughts?
>
>
> On Wed, Aug 8, 2018 at 6:09 PM, Ron Dagostino <rndg...@gmail.com> wrote:
>
> > Hi Rajini. I also like that idea, but I think it might rely on one or
> > possibly two implicit assumptions that I'm not sure are guaranteed to be
> > true. First, I'm not sure if the CallbackHandler interface guarantees
> that
> > implementations must process callbacks in order. Second (and more
> > plausibly than the first), I'm not sure CallbackHandler guarantees that
> > callbacks are to be processed in order until either there are no more
> left
> > in the array or one of the elements is an unsupported callback. The
> > Javadoc simply says it throws UnsupportedCallbackException "if the
> > implementation of this method does not support one or more of the
> Callbacks
> > specified in the callbacks parameter." This statement does not preclude
> the
> > case that implementations might first check to make sure all of the
> > provided callbacks are supported before processing any of them.
> >
> > We could update the Javadoc for AuthenticateCallbackHandler to make it
> > clear how implementations must work -- i.e. they must process the
> callbacks
> > in order, and they must process all recognized callbacks before throwing
> > UnsupportedCallbackException due to an unrecognized one.
> >
> > Note that the above issue does not arise if we simply want the ability to
> > validate SASL extensions in isolation -- we could just give the callback
> > handler an array containing a single instance of the proposed
> > SaslExtensionsValidatorCallback.The issue only arises if we want to
> > provide
> > additional context (e.g. the token in the case of SASL/OATHBEARER) to the
> > validation mechanism.
> >
> > If it is not just SASL Extension validation that we are interested in
> > adding but in fact we want to be able to provide additional context to
> > SaslExtensionsValidatorCallback, then adding the ordering constraint
> above
> > is one way, but we could avoid the constraint by allowing
> > SaslExtensionsValidatorCallback to accept not only the extensions to
> > validate but also an arbitrary map of name/value pairs. Each SASL
> > mechanism implementation could declare what additional context it
> provides
> > (if any) and at what key(s) the information is available. This second
> > approach feels more direct than the first one and would be my preference
> > (assuming I',m not missing anything, which is certainly possible).
> > Thoughts?
> >
> > Ron
> >
> >
> > On Wed, Aug 8, 2018 at 12:39 PM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > Hi Rajini,
> > >
> > > That approach makes more sense to me. I like it
> > >
> > > On Wed, Aug 8, 2018 at 5:35 PM Rajini Sivaram <rajinisiva...@gmail.com
> >
> > > wrote:
> > >
> > > > Hi Ron/Stanislav,
> > > >
> > > > Do you think it makes sense to separate out
> > OAuthBearerValidatorCallback
> > > > and SaslExtensionsValidatorCallback so that it is clearer that these
> > are
> > > > two separate entities that need validation? When we add support
> > > > for customisable extensions in other mechanisms, we could reuse
> > > > SaslExtensionsValidatorCallback. We will invoke CallbackHandler with
> {
> > > > OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in
> > that
> > > > order like we do { NameCallback, PasswordCallback }. So typically we
> > > expect
> > > > to validate tokens with no reference to extensions, but we may refer
> to
> > > > token to validate extensions. Only validated extensions will be
> > available
> > > > as the server's negotiated properties. We will need to handle
> > > > UnsupportedCallbackException for SaslExtensionsValidatorCallback for
> > > > backwards compatibility, but that should be ok.
> > > >
> > > > What do you think?
> > > >
> > > >
> > > > On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski <
> > > > stanis...@confluent.io>
> > > > wrote:
> > > >
> > > > > Hi Ron,
> > > > >
> > > > > Yes, I agree we should document it thoroughly
> > > > >
> > > > > On Wed, Aug 8, 2018 at 5:02 PM Ron Dagostino <rndg...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Hi Stanislav. If the community agrees we should add it then we
> > > should
> > > > > at a
> > > > > > minimum add explicit warnings in the Javadoc making it very clear
> > how
> > > > > this
> > > > > > should not be used.
> > > > > >
> > > > > > Ron
> > > > > >
> > > > > > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > > > > > stanis...@confluent.io>
> > > > > > wrote:
> > > > > >
> > > > > > > Hey Ron,
> > > > > > >
> > > > > > > I fully agree that token validation is a serious security
> > > operation.
> > > > > > > Although, I believe allowing the user to do more validation
> with
> > > the
> > > > > > > extensions does not hurt - the user is fully responsible for
> his
> > > > > security
> > > > > > > once he starts implementing custom code for token validation.
> You
> > > > would
> > > > > > > expect people to take the appropriate considerations when
> > > validating
> > > > > > > unsecured extensions against the token.
> > > > > > > I also think that using the extensions as a secondary
> validation
> > > > method
> > > > > > > might be useful. You could do your normal validation using the
> > > token
> > > > > and
> > > > > > > then have a second sanity-check validation on top (e.g validate
> > > > > > > hostname/port is what client expected). Keep in mind that the
> > > server
> > > > > > > exposes the properties via `getNegotiatedProperty` so it makes
> > > sense
> > > > to
> > > > > > > allow the server to have custom validation on the extensions.
> > > > > > >
> > > > > > > Best,
> > > > > > > Stanislav
> > > > > > >
> > > > > > > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino <
> rndg...@gmail.com>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi Stanislav. If you wanted to do this a good way might be
> to
> > > add
> > > > a
> > > > > > > > constructor to the org.apache.kafka.common.
> > security.oauthbearer.
> > > > > > > > OAuthBearerValidatorCallback class that accepts a
> > SaslExtensions
> > > > > > instance
> > > > > > > > in addition to a token value. Then it would give the
> callback
> > > > > handler
> > > > > > > the
> > > > > > > > option to introspect the callback to see what extensions were
> > > > > provided
> > > > > > > with
> > > > > > > > the
> > > > > > > > token.
> > > > > > > >
> > > > > > > > That being said, token validation is a very
> security-sensitive
> > > > > > operation,
> > > > > > > > and it would be a serious security issue if the result of
> > > applying
> > > > > the
> > > > > > > > validation algorithm (which yields a valid vs. not valid
> > > > > determination)
> > > > > > > > depended on anything provided by the client other than the
> > actual
> > > > > token
> > > > > > > > value. Nobody should ever allow the client to specify a JWK
> > Set
> > > > URL,
> > > > > > for
> > > > > > > > example, or a whitelist of acceptable domains for retrieving
> > JWK
> > > > > Sets.
> > > > > > > It
> > > > > > > > feels to me that while a use case might exist (some kind of
> > trace
> > > > ID,
> > > > > > for
> > > > > > > > example, to aid in debugging), someone might inadvertently
> hang
> > > > > > > themselves
> > > > > > > > if we give them the rope. The risk vs. reward value
> > proposition
> > > > here
> > > > > > > > doesn't feel like a good one at first glance. Thoughts?
> > > > > > > >
> > > > > > > > Ron
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > > > > > > > stanis...@confluent.io>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hey everybody,
> > > > > > > > >
> > > > > > > > > Sorry for reviving this, but I neglected something the
> first
> > > time
> > > > > > > around.
> > > > > > > > > I believe it would be useful to provide the
> > > > > > > > > `OAuthBearerUnsecuredValidatorCallbackHandler` with the
> > OAuth
> > > > > > > extensions
> > > > > > > > > too. This would enable use cases where validators want to
> > > > reconcile
> > > > > > > > > information from the extensions with the token (e.g if
> users
> > > have
> > > > > > > > > implemented secured OAuth tokens).
> > > > > > > > > The implementation would be to instantiate
> > > > > > > > > `OAuthBearerUnsecuredValidatorCallback` with the extensions
> > > (also
> > > > > > leave
> > > > > > > > the
> > > > > > > > > current constructor, as its a public class).
> > > > > > > > >
> > > > > > > > > What are everybody's thoughts on this? If there are no
> > > > objections,
> > > > > > I'll
> > > > > > > > > update the KIP in due time
> > > > > > > > >
> > > > > > > > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram <
> > > > > > > rajinisiva...@gmail.com
> > > > > > > > >
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Looks good. Thanks, Stanislav.
> > > > > > > > > >
> > > > > > > > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > > > > > > > > stanis...@confluent.io
> > > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Rajini,
> > > > > > > > > > >
> > > > > > > > > > > I updated the KIP. Please check if the clarification is
> > > okay
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Jul 25, 2018 at 10:49 AM Rajini Sivaram <
> > > > > > > > > rajinisiva...@gmail.com
> > > > > > > > > > >
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Stanislav,
> > > > > > > > > > > >
> > > > > > > > > > > > 1. Can you clarify the following line in the KIP in
> the
> > > > > 'Public
> > > > > > > > > > > Interfaces'
> > > > > > > > > > > > section? When you are reading the KIP for the first
> > time,
> > > > it
> > > > > > > sounds
> > > > > > > > > > like
> > > > > > > > > > > we
> > > > > > > > > > > > adding a new Kafka config. But we are adding JAAS
> > config
> > > > > > options
> > > > > > > > > with a
> > > > > > > > > > > > prefix that can be used with the default unsecured
> > bearer
> > > > > > tokens.
> > > > > > > > We
> > > > > > > > > > > could
> > > > > > > > > > > > include the example in this section or at least link
> to
> > > the
> > > > > > > > example.
> > > > > > > > > > > >
> > > > > > > > > > > > - New config option for default, unsecured bearer
> > > > tokens -
> > > > > > > > > > > > `unsecuredLoginExtension_<extensionname>`.
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 2. Can you add the package for SaslExtensionsCallback
> > > > class?
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav
> Kozlovski <
> > > > > > > > > > > > stanis...@confluent.io> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Hi Ron,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for the suggestions. I have applied them to
> > the
> > > > KIP.
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, Jul 24, 2018 at 1:39 PM Ron Dagostino <
> > > > > > > rndg...@gmail.com
> > > > > > > > >
> > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Stanislav. The statement "New config option
> for
> > > > > > > > > > > > > OAuthBearerLoginModule"
> > > > > > > > > > > > > > is technically incorrect; it should be "New
> config
> > > > option
> > > > > > for
> > > > > > > > > > > default,
> > > > > > > > > > > > > > unsecured bearer tokens" since that is what
> > provides
> > > > the
> > > > > > > > > > > functionality
> > > > > > > > > > > > > (as
> > > > > > > > > > > > > > opposed to the login module, which does not).
> > Also,
> > > > > please
> > > > > > > > state
> > > > > > > > > > > that
> > > > > > > > > > > > > > "auth" is not supported as a custom extension
> name
> > > with
> > > > > any
> > > > > > > > > > > > > > SASL/OAUTHBEARER mechanism, including the
> unsecured
> > > > one,
> > > > > > > since
> > > > > > > > it
> > > > > > > > > > is
> > > > > > > > > > > > > > reserved by the spec for what is normally sent in
> > the
> > > > > HTTP
> > > > > > > > > > > > Authorization
> > > > > > > > > > > > > > header an attempt to use it will result in a
> > > > > configuration
> > > > > > > > > > exception.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Finally, please also state that while the
> > > > > > > > OAuthBearerLoginModule
> > > > > > > > > > and
> > > > > > > > > > > > the
> > > > > > > > > > > > > > OAuthBearerSaslClient will be changed to request
> > the
> > > > > > > extensions
> > > > > > > > > > from
> > > > > > > > > > > > its
> > > > > > > > > > > > > > callback handler, for backwards compatibility it
> is
> > > not
> > > > > > > > necessary
> > > > > > > > > > for
> > > > > > > > > > > > the
> > > > > > > > > > > > > > callback handler to support
> SaslExtensionsCallback
> > --
> > > > any
> > > > > > > > > > > > > > UnsupportedCallbackException that is thrown will
> be
> > > > > ignored
> > > > > > > and
> > > > > > > > > no
> > > > > > > > > > > > > > extensions will be added.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Ron
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav
> > Kozlovski
> > > <
> > > > > > > > > > > > > > stanis...@confluent.io>
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hey everybody,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I have updated the KIP to reflect the latest
> > > changes
> > > > as
> > > > > > > best
> > > > > > > > > as I
> > > > > > > > > > > > > could.
> > > > > > > > > > > > > > If
> > > > > > > > > > > > > > > there aren't more suggestions, I intent to
> start
> > > the
> > > > > > [VOTE]
> > > > > > > > > > thread
> > > > > > > > > > > > > > > tomorrow.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Best,
> > > > > > > > > > > > > > > Stanislav
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 6:34 AM Ron Dagostino <
> > > > > > > > > rndg...@gmail.com
> > > > > > > > > > >
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hi Stanislav. Could you update the KIP to
> > > reflect
> > > > > the
> > > > > > > > latest
> > > > > > > > > > > > > > definition
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > SaslExtensions and confirm or correct the
> > impact
> > > it
> > > > > has
> > > > > > > to
> > > > > > > > > the
> > > > > > > > > > > > > > > > SCRAM-related classes? I'm not sure if the
> > > > > > > > > currently-described
> > > > > > > > > > > > > impact
> > > > > > > > > > > > > > is
> > > > > > > > > > > > > > > > still accurate. Also, could you mention the
> > > > changes
> > > > > to
> > > > > > > > > > > > > > > > OAuthBearerUnsecuredLoginCallbackHandler in
> > the
> > > > > text in
> > > > > > > > > > > addition to
> > > > > > > > > > > > > > > giving
> > > > > > > > > > > > > > > > the examples? The examples show the new
> > > > > > > > > > > > > > > > unsecuredLoginExtension_<extensionName>
> > feature,
> > > > but
> > > > > > that
> > > > > > > > > > > feature
> > > > > > > > > > > > is
> > > > > > > > > > > > > > not
> > > > > > > > > > > > > > > > described anywhere prior to it appearing
> there.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Ron
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron
> Dagostino <
> > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > >
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Hi Rajini. I think a class is fine as long
> > as
> > > we
> > > > > > make
> > > > > > > > sure
> > > > > > > > > > the
> > > > > > > > > > > > > > > semantics
> > > > > > > > > > > > > > > > > of immutability are clear -- it would have
> to
> > > be
> > > > a
> > > > > > > value
> > > > > > > > > > class,
> > > > > > > > > > > > and
> > > > > > > > > > > > > > any
> > > > > > > > > > > > > > > > > constructor that accepts a Map as input
> would
> > > > have
> > > > > to
> > > > > > > > copy
> > > > > > > > > > that
> > > > > > > > > > > > Map
> > > > > > > > > > > > > > > > rather
> > > > > > > > > > > > > > > > > than store it in a member variable.
> > Similarly,
> > > > any
> > > > > > Map
> > > > > > > > > that
> > > > > > > > > > it
> > > > > > > > > > > > > might
> > > > > > > > > > > > > > > > > return would have to be unmodifiable.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Ron
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini
> > > Sivaram <
> > > > > > > > > > > > > > > rajinisiva...@gmail.com
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >> Hi Ron, Stanislav,
> > > > > > > > > > > > > > > > >>
> > > > > > > > > > > > > > > > >> I agree with Stanislav that it would be
> > better
> > > > to
> > > > > > > leave
> > > > > > > > > > > > > > > `SaslExtensions`
> > > > > > > > > > > > > > > > >> as
> > > > > > > > > > > > > > > > >> a class rather than make it an interface.
> We
> > > > > don''t
> > > > > > > > really
> > > > > > > > > > > > expect
> > > > > > > > > > > > > > > users
> > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > >> extends this class, so it is convenient to
> > > have
> > > > an
> > > > > > > > > > > > implementation
> > > > > > > > > > > > > > > since
> > > > > > > > > > > > > > > > >> users need to create an instance. The
> class
> > > > > provided
> > > > > > > by
> > > > > > > > > the
> > > > > > > > > > > > public
> > > > > > > > > > > > > > API
> > > > > > > > > > > > > > > > >> should be sufficient in the vast majority
> of
> > > the
> > > > > > > cases.
> > > > > > > > > Ron,
> > > > > > > > > > > do
> > > > > > > > > > > > > you
> > > > > > > > > > > > > > > > agree?
> > > > > > > > > > > > > > > > >>
> > > > > > > > > > > > > > > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron
> > > Dagostino
> > > > <
> > > > > > > > > > > > > rndg...@gmail.com>
> > > > > > > > > > > > > > > > >> wrote:
> > > > > > > > > > > > > > > > >>
> > > > > > > > > > > > > > > > >> > Hi Stanislav. See
> > > > > > > > > > > > > > https://tools.ietf.org/html/rfc7628#section-3.1,
> > > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > >> > that section refers to the core ABNF
> > > > productions
> > > > > > > > defined
> > > > > > > > > > in
> > > > > > > > > > > > > > > > >> >
> > > > https://tools.ietf.org/html/rfc5234#appendix-B.
> > > > > > > > > > > > > > > > >> >
> > > > > > > > > > > > > > > > >> > Ron
> > > > > > > > > > > > > > > > >> >
> > > > > > > > > > > > > > > > >> > > On Jul 23, 2018, at 1:30 AM, Stanislav
> > > > > > Kozlovski <
> > > > > > > > > > > > > > > > >> stanis...@confluent.io>
> > > > > > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > >> > > Hey Ron and Rajini,
> > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > >> > > Here are my thoughts:
> > > > > > > > > > > > > > > > >> > > Regarding separators in
> SaslExtensions -
> > > > > Agreed,
> > > > > > > > that
> > > > > > > > > > was
> > > > > > > > > > > a
> > > > > > > > > > > > > bad
> > > > > > > > > > > > > > > > move.
> > > > > > > > > > > > > > > > >> > > Should definitely not be a concern of
> > > > > > > > CallbackHandler
> > > > > > > > > > and
> > > > > > > > > > > > > > > > LoginModule
> > > > > > > > > > > > > > > > >> > > implementors.
> > > > > > > > > > > > > > > > >> > > SaslExtensions interface - Wouldn't
> > > > > implementing
> > > > > > > it
> > > > > > > > as
> > > > > > > > > > an
> > > > > > > > > > > > > > > interface
> > > > > > > > > > > > > > > > >> mean
> > > > > > > > > > > > > > > > >> > > that users will have to make sure
> > they're
> > > > > > passing
> > > > > > > in
> > > > > > > > > an
> > > > > > > > > > > > > > > unmodifiable
> > > > > > > > > > > > > > > > >> map
> > > > > > > > > > > > > > > > >> > > themselves. I believe it would be
> better
> > > if
> > > > we
> > > > > > > > > enforced
> > > > > > > > > > > that
> > > > > > > > > > > > > > > through
> > > > > > > > > > > > > > > > >> > class
> > > > > > > > > > > > > > > > >> > > constructors instead.
> > > > > > > > > > > > > > > > >> > > SaslExtensions#map() - I'd also prefer
> > > this.
> > > > > The
> > > > > > > > > reason
> > > > > > > > > > I
> > > > > > > > > > > > went
> > > > > > > > > > > > > > > with
> > > > > > > > > > > > > > > > >> > > `extensionValue` and `extensionNames`
> > was
> > > > > > because
> > > > > > > I
> > > > > > > > > > > figured
> > > > > > > > > > > > it
> > > > > > > > > > > > > > > made
> > > > > > > > > > > > > > > > >> sense
> > > > > > > > > > > > > > > > >> > > to have `ScramExtensions` extend
> > > > > > `SaslExtensions`
> > > > > > > > and
> > > > > > > > > > > > > therefore
> > > > > > > > > > > > > > > have
> > > > > > > > > > > > > > > > >> > their
> > > > > > > > > > > > > > > > >> > > API be similar. In the end, do you
> think
> > > > that
> > > > > it
> > > > > > > is
> > > > > > > > > > worth
> > > > > > > > > > > it
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > have
> > > > > > > > > > > > > > > > >> > > `ScramExtensions` extend
> > `SaslExtensions`?
> > > > > > > > > > > > > > > > >> > > @Ron, could you point me to the SASL
> > OAuth
> > > > > > > mechanism
> > > > > > > > > > > > specific
> > > > > > > > > > > > > > > > regular
> > > > > > > > > > > > > > > > >> > > expressions for keys/values you
> > mentioned
> > > > are
> > > > > in
> > > > > > > RFC
> > > > > > > > > > 7628
> > > > > > > > > > > (
> > > > > > > > > > > > > > > > >> > > https://tools.ietf.org/html/rfc7628)
> ?
> > I
> > > > > could
> > > > > > > not
> > > > > > > > > find
> > > > > > > > > > > any
> > > > > > > > > > > > > > while
> > > > > > > > > > > > > > > > >> > > originally implementing this.
> > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > >> > > Best,
> > > > > > > > > > > > > > > > >> > > Stanislav
> > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > >> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron
> > > > > Dagostino <
> > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> Hi again, Rajini and Stanislav. I
> > wonder
> > > > if
> > > > > > > making
> > > > > > > > > > > > > > > SaslExtensions
> > > > > > > > > > > > > > > > an
> > > > > > > > > > > > > > > > >> > >> interface rather than a class might
> be
> > a
> > > > good
> > > > > > > > > solution.
> > > > > > > > > > > > For
> > > > > > > > > > > > > > > > example:
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> public interface SaslExtensions {
> > > > > > > > > > > > > > > > >> > >> /**
> > > > > > > > > > > > > > > > >> > >> * @return an immutable map view of
> > the
> > > > > SASL
> > > > > > > > > > extensions
> > > > > > > > > > > > > > > > >> > >> */
> > > > > > > > > > > > > > > > >> > >> Map<String, String> map();
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> This solves the issue of lack of
> > clarity
> > > on
> > > > > > > > > > immutability,
> > > > > > > > > > > > and
> > > > > > > > > > > > > > it
> > > > > > > > > > > > > > > > also
> > > > > > > > > > > > > > > > >> > >> eliminates copying, like this:
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > > > > > >> > >> Map<String, String> myRetval =
> > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsMap();
> > > > > > > > > > > > > > > > >> > >> return new SaslExtensions() {
> > > > > > > > > > > > > > > > >> > >> public Map<String, String>
> > map() {
> > > > > > > > > > > > > > > > >> > >> return myRetval;
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> Alternatively, we could do it like
> > this:
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> /**
> > > > > > > > > > > > > > > > >> > >> * Supplier that returns immutable map
> > > view
> > > > of
> > > > > > > SASL
> > > > > > > > > > > > Extensions
> > > > > > > > > > > > > > > > >> > >> */
> > > > > > > > > > > > > > > > >> > >> public interface SaslExtensions
> extends
> > > > > > > > > > > > Supplier<Map<String,
> > > > > > > > > > > > > > > > >> String>> {
> > > > > > > > > > > > > > > > >> > >> // empty
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> The we could simply return the
> instance
> > > > like
> > > > > > > this,
> > > > > > > > > > again
> > > > > > > > > > > > > > without
> > > > > > > > > > > > > > > > >> > copying:
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > > > > > >> > >> Map<String, String> myRetval =
> > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsMap();
> > > > > > > > > > > > > > > > >> > >> return () -> myRetval;
> > > > > > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> I think the main reason for making
> > > > > > SaslExtensions
> > > > > > > > > part
> > > > > > > > > > of
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > > public
> > > > > > > > > > > > > > > > >> > >> interface is to avoid adding a Map to
> > the
> > > > > > > Subject's
> > > > > > > > > > > public
> > > > > > > > > > > > > > > > >> credentials.
> > > > > > > > > > > > > > > > >> > >> Making SaslExtensions an interface
> > meets
> > > > that
> > > > > > > > > > requirement
> > > > > > > > > > > > and
> > > > > > > > > > > > > > > then
> > > > > > > > > > > > > > > > >> > allows
> > > > > > > > > > > > > > > > >> > >> us to be free to implement whatever
> we
> > > want
> > > > > > > > > internally.
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> Thoughts?
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >> Ron
> > > > > > > > > > > > > > > > >> > >>
> > > > > > > > > > > > > > > > >> > >>> On Sun, Jul 22, 2018 at 12:45 PM Ron
> > > > > > Dagostino <
> > > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>> Hi Rajini. The SaslServer is going
> to
> > > > have
> > > > > to
> > > > > > > > > > validate
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > > >> extensions,
> > > > > > > > > > > > > > > > >> > >>> too, but I’m okay with keeping the
> > > > > validation
> > > > > > > > logic
> > > > > > > > > > > > > elsewhere
> > > > > > > > > > > > > > as
> > > > > > > > > > > > > > > > >> long
> > > > > > > > > > > > > > > > >> > as
> > > > > > > > > > > > > > > > >> > >> it
> > > > > > > > > > > > > > > > >> > >>> can be reused in both the client and
> > the
> > > > > > secret.
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>> I strongly prefer exposing a map()
> > > method
> > > > as
> > > > > > > > opposed
> > > > > > > > > > to
> > > > > > > > > > > > > > > > >> > extensionNames()
> > > > > > > > > > > > > > > > >> > >>> and extensionValue(String) methods.
> It
> > > is
> > > > a
> > > > > > > > smaller
> > > > > > > > > > API
> > > > > > > > > > > (2
> > > > > > > > > > > > > > > methods
> > > > > > > > > > > > > > > > >> > >> instead
> > > > > > > > > > > > > > > > >> > >>> of 1), and it gives clients of the
> API
> > > > full
> > > > > > > > > > map-related
> > > > > > > > > > > > > > > > >> functionality
> > > > > > > > > > > > > > > > >> > >>> (there’s a lot of support for
> dealing
> > > with
> > > > > > maps
> > > > > > > > in a
> > > > > > > > > > > > variety
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> ways).
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>> Regardless of whether we go with a
> > map()
> > > > > > method
> > > > > > > or
> > > > > > > > > > > > > > > > extensionNames()
> > > > > > > > > > > > > > > > >> and
> > > > > > > > > > > > > > > > >> > >>> extensionValue(String) methods, the
> > > > > semantics
> > > > > > of
> > > > > > > > > > > > mutability
> > > > > > > > > > > > > > need
> > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > >> be
> > > > > > > > > > > > > > > > >> > >>> clear. I think either way we should
> > > never
> > > > > > > share a
> > > > > > > > > map
> > > > > > > > > > > > that
> > > > > > > > > > > > > > > anyone
> > > > > > > > > > > > > > > > >> else
> > > > > > > > > > > > > > > > >> > >>> could possibly mutate — either a map
> > > that
> > > > > > > someone
> > > > > > > > > > gives
> > > > > > > > > > > us
> > > > > > > > > > > > > or
> > > > > > > > > > > > > > a
> > > > > > > > > > > > > > > > map
> > > > > > > > > > > > > > > > >> > that
> > > > > > > > > > > > > > > > >> > >> we
> > > > > > > > > > > > > > > > >> > >>> might expose.
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>> Thoughts?
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>> Ron
> > > > > > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > > > > > >> > >>>> On Jul 22, 2018, at 11:23 AM,
> Rajini
> > > > > Sivaram
> > > > > > <
> > > > > > > > > > > > > > > > >> rajinisiva...@gmail.com
> > > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> Hmm.... I think we need a much
> > simpler
> > > > > > > > > SaslExtensions
> > > > > > > > > > > > class
> > > > > > > > > > > > > > if
> > > > > > > > > > > > > > > we
> > > > > > > > > > > > > > > > >> are
> > > > > > > > > > > > > > > > >> > >>>> making it part of the public API.
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> 1. I don't see the point of
> including
> > > > > > separator
> > > > > > > > > > > anywhere
> > > > > > > > > > > > in
> > > > > > > > > > > > > > > > >> > >>> SaslExtensions.
> > > > > > > > > > > > > > > > >> > >>>> Extensions provide a map and we
> > > propagate
> > > > > the
> > > > > > > map
> > > > > > > > > > from
> > > > > > > > > > > > > client
> > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > >> > server
> > > > > > > > > > > > > > > > >> > >>>> using the protocol associated with
> > the
> > > > > > > mechanism
> > > > > > > > in
> > > > > > > > > > > use.
> > > > > > > > > > > > > The
> > > > > > > > > > > > > > > > >> separator
> > > > > > > > > > > > > > > > >> > >> is
> > > > > > > > > > > > > > > > >> > >>>> not configurable and should not be
> a
> > > > > concern
> > > > > > of
> > > > > > > > the
> > > > > > > > > > > > > > implementor
> > > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> > >>>> SaslExtensionsCallback interface
> that
> > > > > > provides
> > > > > > > an
> > > > > > > > > > > > instance
> > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> > >>> SaslExtensions
> > > > > > > > > > > > > > > > >> > >>>> .
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> 2. I agree with Ron that we need
> > > > > > > > mechanism-specific
> > > > > > > > > > > > > > validation
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> the
> > > > > > > > > > > > > > > > >> > >>>> values from SaslExtensions. But I
> > think
> > > > we
> > > > > > > could
> > > > > > > > do
> > > > > > > > > > the
> > > > > > > > > > > > > > > > validation
> > > > > > > > > > > > > > > > >> in
> > > > > > > > > > > > > > > > >> > >> the
> > > > > > > > > > > > > > > > >> > >>>> appropriate `SaslClient`
> > implementation
> > > > of
> > > > > > that
> > > > > > > > > > > > mechanism.
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> I think we could just have a very
> > > simple
> > > > > > > > extensions
> > > > > > > > > > > class
> > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > move
> > > > > > > > > > > > > > > > >> > >>>> everything else to appropriate
> > internal
> > > > > > classes
> > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > > > > > > mechanisms
> > > > > > > > > > > > > > > > >> > using
> > > > > > > > > > > > > > > > >> > >>>> extensions. What do you think?
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> public class SaslExtensions {
> > > > > > > > > > > > > > > > >> > >>>> private final Map<String, String>
> > > > > > > extensionMap;
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> public SaslExtensions(Map<String,
> > > > String>
> > > > > > > > > > > > extensionMap) {
> > > > > > > > > > > > > > > > >> > >>>> this.extensionMap =
> > extensionMap;
> > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> public String
> extensionValue(String
> > > > > name) {
> > > > > > > > > > > > > > > > >> > >>>> return
> extensionMap.get(name);
> > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>> public Set<String>
> > extensionNames() {
> > > > > > > > > > > > > > > > >> > >>>> return extensionMap.keySet();
> > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > > > > > >> > >>>>> On Sat, Jul 21, 2018 at 9:01 PM,
> Ron
> > > > > > > Dagostino <
> > > > > > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > > > > > >> >
> > > > > > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> Hi Stanislav and Rajini. If
> > > > > SaslExtensions
> > > > > > is
> > > > > > > > > going
> > > > > > > > > > > to
> > > > > > > > > > > > > part
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> the
> > > > > > > > > > > > > > > > >> > >>> public
> > > > > > > > > > > > > > > > >> > >>>>> API, then it occurred to me that
> one
> > > of
> > > > > the
> > > > > > > > > > > requirements
> > > > > > > > > > > > > of
> > > > > > > > > > > > > > > all
> > > > > > > > > > > > > > > > >> SASL
> > > > > > > > > > > > > > > > >> > >>>>> extensions is that the keys and
> > values
> > > > > need
> > > > > > to
> > > > > > > > > match
> > > > > > > > > > > > > > > > >> > >> mechanism-specific
> > > > > > > > > > > > > > > > >> > >>>>> regular expressions. For example,
> > RFC
> > > > > 5802
> > > > > > (
> > > > > > > > > > > > > > > > >> > >>>>>
> https://tools.ietf.org/html/rfc5802
> > )
> > > > > > > specifies
> > > > > > > > > the
> > > > > > > > > > > > > regular
> > > > > > > > > > > > > > > > >> > >> expressions
> > > > > > > > > > > > > > > > >> > >>> for
> > > > > > > > > > > > > > > > >> > >>>>> the SCRAM-specific SASL
> mechanisms,
> > > and
> > > > > RFC
> > > > > > > > 7628 (
> > > > > > > > > > > > > > > > >> > >>>>>
> https://tools.ietf.org/html/rfc7628
> > )
> > > > > > > specifies
> > > > > > > > > > > > different
> > > > > > > > > > > > > > > > regular
> > > > > > > > > > > > > > > > >> > >>>>> expressions for the OAUTHBEARER
> SASL
> > > > > > > > mechanism. I
> > > > > > > > > > am
> > > > > > > > > > > > > > thinking
> > > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > >> > >>>>> SaslExtensions class should
> probably
> > > > > > provide a
> > > > > > > > way
> > > > > > > > > > to
> > > > > > > > > > > > make
> > > > > > > > > > > > > > > sure
> > > > > > > > > > > > > > > > >> the
> > > > > > > > > > > > > > > > >> > >> keys
> > > > > > > > > > > > > > > > >> > >>>>> and values match the appropriate
> > > regular
> > > > > > > > > > expressions.
> > > > > > > > > > > > > What
> > > > > > > > > > > > > > do
> > > > > > > > > > > > > > > > you
> > > > > > > > > > > > > > > > >> > >>> think of
> > > > > > > > > > > > > > > > >> > >>>>> something along the lines of the
> > below
> > > > > > > > definition
> > > > > > > > > > for
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > > >> > >> SaslExtensions
> > > > > > > > > > > > > > > > >> > >>>>> class? It is missing Javadoc and
> > > > > > > > > > > > > > > toString()/hashCode()/equals()
> > > > > > > > > > > > > > > > >> > >>> methods,
> > > > > > > > > > > > > > > > >> > >>>>> of course, but aside from that, do
> > you
> > > > > think
> > > > > > > > this
> > > > > > > > > is
> > > > > > > > > > > > > > > sufficient
> > > > > > > > > > > > > > > > >> and
> > > > > > > > > > > > > > > > >> > >>>>> appropriate?
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> Ron
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> public class SaslExtensions {
> > > > > > > > > > > > > > > > >> > >>>>> private final Map<String,
> String>
> > > > > > > > extensionsMap;
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> public SaslExtensions(String
> > mapStr,
> > > > > > String
> > > > > > > > > > > > > > > keyValueSeparator,
> > > > > > > > > > > > > > > > >> > >> String
> > > > > > > > > > > > > > > > >> > >>>>> elementSeparator,
> > > > > > > > > > > > > > > > >> > >>>>> Pattern
> > > saslNameRegexPattern,
> > > > > > > Pattern
> > > > > > > > > > > > > > > > >> > >> saslValueRegexPattern)
> > > > > > > > > > > > > > > > >> > >>> {
> > > > > > > > > > > > > > > > >> > >>>>> this(Utils.parseMap(mapStr,
> > > > > > > > > keyValueSeparator,
> > > > > > > > > > > > > > > > >> > >> elementSeparator),
> > > > > > > > > > > > > > > > >> > >>>>> saslNameRegexPattern,
> > > > > > saslValueRegexPattern);
> > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> public
> SaslExtensions(Map<String,
> > > > > String>
> > > > > > > > > > > > extensionsMap,
> > > > > > > > > > > > > > > > Pattern
> > > > > > > > > > > > > > > > >> > >>>>> saslNameRegexPattern,
> > > > > > > > > > > > > > > > >> > >>>>> Pattern
> > > > saslValueRegexPattern) {
> > > > > > > > > > > > > > > > >> > >>>>> Map<String, String>
> > > sanitizedCopy
> > > > =
> > > > > > new
> > > > > > > > > > > > > > > > >> > >>>>> HashMap<>(extensionsMap.size());
> > > > > > > > > > > > > > > > >> > >>>>> for (Entry<String, String>
> > > entry :
> > > > > > > > > > > > > > > > >> extensionsMap.entrySet()) {
> > > > > > > > > > > > > > > > >> > >>>>> if
> (!saslNameRegexPattern.
> > > > > > > > > > > > > matcher(entry.getKey()).
> > > > > > > > > > > > > > > > >> > matches()
> > > > > > > > > > > > > > > > >> > >>>>> ||
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > !saslValueRegexPattern.matcher(entry.getValue()).
> > > > > > > > > > > > > matches())
> > > > > > > > > > > > > > > > >> > >>>>> throw new
> > > > > > > > > > > > IllegalArgumentException("Invalid
> > > > > > > > > > > > > > key
> > > > > > > > > > > > > > > or
> > > > > > > > > > > > > > > > >> > >>>>> value");
> > > > > > > > > > > > > > > > >> > >>>>>
> > > sanitizedCopy.put(entry.getKe
> > > > > y(),
> > > > > > > > > > > > > > entry.getValue());
> > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > >> > >>>>> this.extensionsMap =
> > > > > > > > > > > > > > > > >> > >>
> > > Collections.unmodifiableMap(sanitizedCopy);
> > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> public Map<String, String>
> map() {
> > > > > > > > > > > > > > > > >> > >>>>> return extensionsMap;
> > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>> On Fri, Jul 20, 2018 at 12:49 PM
> > > > Stanislav
> > > > > > > > > > Kozlovski <
> > > > > > > > > > > > > > > > >> > >>>>> stanis...@confluent.io>
> > > > > > > > > > > > > > > > >> > >>>>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > > > > > >> > >>>>>> Hi Ron,
> > > > > > > > > > > > > > > > >> > >>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>> I saw that and decided that would
> > be
> > > > the
> > > > > > best
> > > > > > > > > > > approach.
> > > > > > > > > > > > > The
> > > > > > > > > > > > > > > > >> current
> > > > > > > > > > > > > > > > >> > >>>>>> ScramExtensions implementation
> > uses a
> > > > Map
> > > > > > in
> > > > > > > > the
> > > > > > > > > > > public
> > > > > > > > > > > > > > > > >> credentials
> > > > > > > > > > > > > > > > >> > >>> and I
> > > > > > > > > > > > > > > > >> > >>>>>> thought I would follow convention
> > > > rather
> > > > > > than
> > > > > > > > > > > introduce
> > > > > > > > > > > > > my
> > > > > > > > > > > > > > > own
> > > > > > > > > > > > > > > > >> > thing,
> > > > > > > > > > > > > > > > >> > >>> but
> > > > > > > > > > > > > > > > >> > >>>>>> maybe this is best
> > > > > > > > > > > > > > > > >> > >>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>> On Fri, Jul 20, 2018 at 8:39 AM
> > Ron
> > > > > > > Dagostino
> > > > > > > > <
> > > > > > > > > > > > > > > > >> rndg...@gmail.com>
> > > > > > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>> Hi Stanislav. I'm wondering if
> we
> > > > > should
> > > > > > > make
> > > > > > > > > > > > > > > SaslExtensions
> > > > > > > > > > > > > > > > >> part
> > > > > > > > > > > > > > > > >> > >> of
> > > > > > > > > > > > > > > > >> > >>>>> the
> > > > > > > > > > > > > > > > >> > >>>>>>> public API. I mentioned this in
> > my
> > > > > review
> > > > > > > of
> > > > > > > > > the
> > > > > > > > > > > PR,
> > > > > > > > > > > > > too
> > > > > > > > > > > > > > > (and
> > > > > > > > > > > > > > > > >> > >> tagged
> > > > > > > > > > > > > > > > >> > >>>>>>> Rajini to get her input). If we
> > > add a
> > > > > Map
> > > > > > > to
> > > > > > > > > the
> > > > > > > > > > > > > > Subject's
> > > > > > > > > > > > > > > > >> public
> > > > > > > > > > > > > > > > >> > >>>>>>> credentials we are basically
> > making
> > > a
> > > > > > public
> > > > > > > > > > > > commitment
> > > > > > > > > > > > > > that
> > > > > > > > > > > > > > > > any
> > > > > > > > > > > > > > > > >> > Map
> > > > > > > > > > > > > > > > >> > >>>>>>> associated with the public
> > > credentials
> > > > > > > defines
> > > > > > > > > the
> > > > > > > > > > > > SASL
> > > > > > > > > > > > > > > > >> extensions
> > > > > > > > > > > > > > > > >> > >> and
> > > > > > > > > > > > > > > > >> > >>>>> we
> > > > > > > > > > > > > > > > >> > >>>>>>> can never add another instance
> > > > > > implementing
> > > > > > > > Map
> > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > > > > > public
> > > > > > > > > > > > > > > > >> > >>>>>> credentials.
> > > > > > > > > > > > > > > > >> > >>>>>>> That's a very big constraint we
> > are
> > > > > > > committing
> > > > > > > > > to,
> > > > > > > > > > > and
> > > > > > > > > > > > > I'm
> > > > > > > > > > > > > > > > >> > wondering
> > > > > > > > > > > > > > > > >> > >>> if
> > > > > > > > > > > > > > > > >> > >>>>>> we
> > > > > > > > > > > > > > > > >> > >>>>>>> should make SaslExtensions
> public
> > > and
> > > > > > attach
> > > > > > > > an
> > > > > > > > > > > > instance
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > >> that to
> > > > > > > > > > > > > > > > >> > >>> the
> > > > > > > > > > > > > > > > >> > >>>>>>> Subject's public credentials
> > > instead.
> > > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>> Ron
> > > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>> On Thu, Jul 19, 2018 at 8:15 PM
> > > > > Stanislav
> > > > > > > > > > Kozlovski
> > > > > > > > > > > <
> > > > > > > > > > > > > > > > >> > >>>>>>> stanis...@confluent.io>
> > > > > > > > > > > > > > > > >> > >>>>>>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>> I have updated the PR and KIP
> to
> > > > > address
> > > > > > > the
> > > > > > > > > > > comments
> > > > > > > > > > > > > > made
> > > > > > > > > > > > > > > so
> > > > > > > > > > > > > > > > >> far.
> > > > > > > > > > > > > > > > >> > >>>>>> Please
> > > > > > > > > > > > > > > > >> > >>>>>>>> take another look at them and
> > share
> > > > > your
> > > > > > > > > > thoughts.
> > > > > > > > > > > > > > > > >> > >>>>>>>> KIP:
> > > > > > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>> https://cwiki.apache.org/
> > > > > > > > > > > confluence/display/KAFKA/KIP-
> > > > > > > > > > > > > > > > >> > >>>>>
> > > > > > > > 342%3A+Add+support+for+Custom+SASL+extensions+in+
> > > > > > > > > > > > > > > > >> > >>>>> OAuthBearer+authentication
> > > > > > > > > > > > > > > > >> > >>>>>>>> PR: Pull request <
> > > > > > > > > > > > > > > https://github.com/apache/kafka/pull/5379>
> > > > > > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>> Best,
> > > > > > > > > > > > > > > > >> > >>>>>>>> Stanislav
> > > > > > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>> On Thu, Jul 19, 2018 at 1:58 PM
> > > > > Stanislav
> > > > > > > > > > > Kozlovski <
> > > > > > > > > > > > > > > > >> > >>>>>>>> stanis...@confluent.io>
> > > > > > > > > > > > > > > > >> > >>>>>>>> wrote:
> > > > > > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>>> Hi Ron,
> > > > > > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>>> Agreed.
> `SaslExtensionsCallback`
> > > > will
> > > > > be
> > > > > > > the
> > > > > > > > > > only
> > > > > > > > > > > > > public
> > > > > > > > > > > > > > > API
> > > > > > > > > > > > > > > > >> > >>>>> addition
> > > > > > > > > > > > > > > > >> > >>>>>>> and
> > > > > > > > > > > > > > > > >> > >>>>>>>>> new documentation for the
> > > extension
> > > > > > > strings.
> > > > > > > > > > > > > > > > >> > >>>>>>>>> A question that came up -
> should
> > > the
> > > > > > > > > > > > > > LoginCallbackHandler
> > > > > > > > > > > > > > > > >> throw
> > > > > > > > > > > > > > > > >> > an
> > > > > > > > > > > > > > > > >> > >>>>>>>>> exception or simply ignore
> > > key/value
> > > > > > > > extension
> > > > > > > > > > > pairs
> > > > > > > > > > > > > who
> > > > > > > > > > > > > > > do
> > > > > > > > > > > > > > > > >> not
> > > > > > > > > > > > > > > > >> > >>>>> match
> > > > > > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > > > > > >> > >>>>>>>>> validation regex pattern? I
> > guess
> > > it
> > > > > > would
> > > > > > > > be
> > > > > > > > > > > better
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > >> throw, as
> > > > > > > > > > > > > > > > >> > >>>>> to
> > > > > > > > > > > > > > > > >> > >>>>>>>> avoid
> > > > > > > > > > > > > > > > >> > >>>>>>>>> confusion.
> > > > > > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>>> And yes, I will make sure the
> > > > > key/value
> > > > > > > are
> > > > > > > > > > > > validated
> > > > > > > > > > > > > on
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > >> > >> client
> > > > > > > > > > > > > > > > >> > >>>>>> as
> > > > > > > > > > > > > > > > >> > >>>>>>>>> well as in the server. Even
> > then,
> > > I
> > > > > > > > structured
> > > > > > > > > > the
> > > > > > > > > > > > > > > > >> > >>>>>>>> getNegotiatedProperty()
> > > > > > > > > > > > > > > > >> > >>>>>>>>> method such that the
> > > > OAUTHBEARER.token
> > > > > > can
> > > > > > > > > never
> > > > > > > > > > > be
> > > > > > > > > > > > > > > > >> overridden. I
> > > > > > > > > > > > > > > > >> > >>>>>>>>> considered adding a test for
> > that,
> > > > > but I
> > > > > > > > > figured
> > > > > > > > > > > > > having
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > >> regex
> > > > > > > > > > > > > > > > >> > >>>>>>>>> validation be enough of a
> > > guarantee.
> > > > > > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > > > > > >> > >>>>>>>>> On Thu, Jul 19, 2018 at 9:49
> AM
> > > Ron
> > > > > > > > Dagostino
> > > > > > > > > <
> > > > > > > > > > >
