Hi Rajini, Ron
I've updated the KIP with the latest changes following our discussion.
Please do give it a read. If you feel it is alright, I will follow up with
a PR later.
Best,
Stanislav
On Thu, Aug 9, 2018 at 10:09 AM Rajini Sivaram <rajinisiva...@gmail.com>
wrote:
> Hi Ron/Stansilav,
>
> OK, let's just go with 2. I think it would be better to add a
> OAuth-specific extensions handler 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
> > > > > > > >
--
Best,
Stanislav
