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
> > > > > > > > > > > > > > >> > >>>>>>>>>> SaslExtensionsCallback and look
> on
> > > the
> > > > > > > Subject
> > > > > > > > > > > > > accordingly.
> > > > > > > > > > > > > > >> > >> There
> > > > > > > > > > > > > > >> > >>>>>>> would
> > > > > > > > > > > > > > >> > >>>>>>>>>> be
> > > > > > > > > > > > > > >> > >>>>>>>>>> no need to implement a custom
> > > > > > > > > > > > > sasl.client.callback.handler.
> > > > > > > > > > > > > > >> > class
> > > > > > > > > > > > > > >> > >>>>> in
> > > > > > > > > > > > > > >> > >>>>>>> this
> > > > > > > > > > > > > > >> > >>>>>>>>>> case, and no logic would need to
> be
> > > > moved
> > > > > > to
> > > > > > > a
> > > > > > > > > > public
> > > > > > > > > > > > > > static
> > > > > > > > > > > > > > >> > >>>>> method
> > > > > > > > > > > > > > >> > >>>>>> on
> > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule as I had
> > > > proposed
> > > > > > (at
> > > > > > > > > least
> > > > > > > > > > > not
> > > > > > > > > > > > > > right
> > > > > > > > > > > > > > >> > now,
> > > > > > > > > > > > > > >> > >>>>>>> anyway
> > > > > > > > > > > > > > >> > >>>>>>>>>> --
> > > > > > > > > > > > > > >> > >>>>>>>>>> there may come a time when a need
> > > for a
> > > > > > > custom
> > > > > > > > > > > > > > >> > >>>>>>>>>>
> sasl.client.callback.handler.class
> > > is
> > > > > > > > > identified,
> > > > > > > > > > > and
> > > > > > > > > > > > at
> > > > > > > > > > > > > > that
> > > > > > > > > > > > > > >> > >>>>> point
> > > > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > > > >> > >>>>>>>>>> default implementation would
> either
> > > > have
> > > > > to
> > > > > > > > made
> > > > > > > > > > part
> > > > > > > > > > > > of
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > >> > >>>>> public
> > > > > > > > > > > > > > >> > >>>>>>> API
> > > > > > > > > > > > > > >> > >>>>>>>>>> with protected rather than
> private
> > > > > methods
> > > > > > so
> > > > > > > > it
> > > > > > > > > > > could
> > > > > > > > > > > > be
> > > > > > > > > > > > > > >> > >> directly
> > > > > > > > > > > > > > >> > >>>>>>>>>> extended
> > > > > > > > > > > > > > >> > >>>>>>>>>> or its logic would have to be
> moved
> > > to
> > > > > > public
> > > > > > > > > > static
> > > > > > > > > > > > > > methods
> > > > > > > > > > > > > > >> on
> > > > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule).
> > > > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > > > >> > >>>>>>>>>> So, to try to summarize, I think
> > > > > > > > > > > SaslExtensionsCallback
> > > > > > > > > > > > > > will
> > > > > > > > > > > > > > >> be
> > > > > > > > > > > > > > >> > >>>>> the
> > > > > > > > > > > > > > >> > >>>>>>> only
> > > > > > > > > > > > > > >> > >>>>>>>>>> public API addition due to this
> KIP
> > > in
> > > > > > terms
> > > > > > > of
> > > > > > > > > > code,
> > > > > > > > > > > > and
> > > > > > > > > > > > > > >> then
> > > > > > > > > > > > > > >> > >>>>> maybe
> > > > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > > > >> > >>>>>>>>>> recognition of the
> > > > > > unsecuredLoginExtension_<
> > > > > > > > > > > > > > >> > extensionName>=value
> > > > > > > > > > > > > > >> > >>>>>>> module
> > > > > > > > > > > > > > >> > >>>>>>>>>> options in the default unsecured
> > case
> > > > > > (which
> > > > > > > > > would
> > > > > > > > > > > be a
> > > > > > > > > > > > > > >> > >>>>>> documentation
> > > > > > > > > > > > > > >> > >>>>>>>>>> change and an internal
> > implementation
> > > > > issue
> > > > > > > > > rather
> > > > > > > > > > > > than a
> > > > > > > > > > > > > > >> public
> > > > > > > > > > > > > > >> > >>>>> API
> > > > > > > > > > > > > > >> > >>>>>>> in
> > > > > > > > > > > > > > >> > >>>>>>>>>> terms of code). And then also
> the
> > > fact
> > > > > > that
> > > > > > > > > > > extension
> > > > > > > > > > > > > > names
> > > > > > > > > > > > > > >> and
> > > > > > > > > > > > > > >> > >>>>>>> values
> > > > > > > > > > > > > > >> > >>>>>>>>>> are
> > > > > > > > > > > > > > >> > >>>>>>>>>> accessed on the server side via
> > > > > negotiated
> > > > > > > > > > > properties.
> > > > > > >
