Hi Ron,
I was also thinking along the lines of send the callbacks separately.
Initially I thought the `CallbackHandler` can simply maintain state but
that's not a good idea since it'll be dependent on the implementation of
the `SaslServer` rather than the interface.
Having the SaslExtensionsValidatorCallback accept arbitrary fields is a
good idea, that way we could reuse it for any other SASL mechanism which
implements extensions. And of course, you could simply validate the
extensions and ignore the additional information - that would be valid too.
I think this is the way to go!
Best,
Stanislav
On Wed, Aug 8, 2018 at 6:10 PM Ron Dagostino <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
> > > > > > > > <
> > > > > > > > > > > > > > > >> rndg...@gmail.com
> > > > > > > > > > > > > > > >> > >
> > > > > > > > > > > > > > > >> > >>>>>>> wrote:
> > > > > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > > > > >> > >>>>>>>>>> Hi Rajini and Stanislav.
> Rajini,
> > > > yes,
> > > > > I
> > > > > > > > think
> > > > > > > > > > you
> > > > > > > > > > > > are
> > > > > > > > > > > > > > > right
> > > > > > > > > > > > > > > >> > >> about
> > > > > > > > > > > > > > > >> > >>>>>> the
> > > > > > > > > > > > > > > >> > >>>>>>>>>> login callback handler being
> more
> > > > > > > appropriate
> > > > > > > > > for
> > > > > > > > > > > > > > > retrieving
> > > > > > > > > > > > > > > >> the
> > > > > > > > > > > > > > > >> > >>>>>> SASL
> > > > > > > > > > > > > > > >> > >>>>>>>>>> extensions than the login
> module
> > > > itself
> > > > > > > (how
> > > > > > > > > many
> > > > > > > > > > > > times
> > > > > > > > > > > > > > am
> > > > > > > > > > > > > > > I
> > > > > > > > > > > > > > > >> > >> going
> > > > > > > > > > > > > > > >> > >>>>>> to
> > > > > > > > > > > > > > > >> > >>>>>>>> have
> > > > > > > > > > > > > > > >> > >>>>>>>>>> to be encouraged to leverage
> the
> > > > > callback
> > > > > > > > > > > handlers?!?
> > > > > > > > > > > > > > lol).
> > > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule should
> ask
> > > its
> > > > > > login
> > > > > > > > > > > callback
> > > > > > > > > > > > > > > handler
> > > > > > > > > > > > > > > >> to
> > > > > > > > > > > > > > > >> > >>>>>> handle
> > > > > > > > > > > > > > > >> > >>>>>>>> an
> > > > > > > > > > > > > > > >> > >>>>>>>>>> instance of
> > SaslExtensionsCallback
> > > in
> > > > > > > > addition
> > > > > > > > > to
> > > > > > > > > > > an
> > > > > > > > > > > > > > > >> instance of
> > > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerTokenCallback, and
> the
> > > > > default
> > > > > > > > login
> > > > > > > > > > > > > callback
> > > > > > > > > > > > > > > >> handler
> > > > > > > > > > > > > > > >> > >>>>>>>>>> implementation
> > > > > > > > > > > > > (OAuthBearerUnsecuredLoginCallbackHandler)
> > > > > > > > > > > > > > > >> > should
> > > > > > > > > > > > > > > >> > >>>>>>> either
> > > > > > > > > > > > > > > >> > >>>>>>>>>> return an empty map via
> callback
> > or
> > > > it
> > > > > > > should
> > > > > > > > > > > > recognize
> > > > > > > > > > > > > > > >> > >> additional
> > > > > > > > > > > > > > > >> > >>>>>>> JAAS
> > > > > > > > > > > > > > > >> > >>>>>>>>>> module options of the form
> > > > > > > > > > > > > > > >> > >>>>>>>
> > > > > > unsecuredLoginExtension_<extensionName>=value
> > > > > > > > > > > > > > > >> > >>>>>>>>>> so
> > > > > > > > > > > > > > > >> > >>>>>>>>>> that arbitrary extensions can
> be
> > > > added
> > > > > in
> > > > > > > > > > > development
> > > > > > > > > > > > > and
> > > > > > > > > > > > > > > >> test
> > > > > > > > > > > > > > > >> > >>>>>>> scenarios
> > > > > > > > > > > > > > > >> > >>>>>>>>>> (similar to how arbitrary
> claims
> > on
> > > > > > > unsecured
> > > > > > > > > > > tokens
> > > > > > > > > > > > > can
> > > > > > > > > > > > > > be
> > > > > > > > > > > > > > > >> > >>>>> created
> > > > > > > > > > > > > > > >> > >>>>>> in
> > > > > > > > > > > > > > > >> > >>>>>>>>>> those scenarios via the JAAS
> > module
> > > > > > options
> > > > > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > unsecuredLoginStringClaim_<claimName>=value,
> > > > > > > > > > etc.).
> > > > > > > > > > > > > Then
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule can add
> a
> > > map
> > > > of
> > > > > > any
> > > > > > > > > > > > extensions
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > >> > >>>>>>> Subject's
> > > > > > > > > > > > > > > >> > >>>>>>>>>> public credentials where the
> > > default
> > > > > SASL
> > > > > > > > > client
> > > > > > > > > > > > > callback
> > > > > > > > > > > > > > > >> > handler
> > > > > > > > > > > > > > > >> > >>>>>>> class
> > > > > > > > > > > > > > > >> > >>>>>>>> (
> > > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerSaslClientCallbackH
> > > > andler)
> > > > > can
> > > > > > > be
> > > > > > > > > > > > amended to
> > > > > > > > > > > > > > > >> support
> >
--
Best,
Stanislav
