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