Hi Ron/Stanislav,
Do you think it makes sense to separate out OAuthBearerValidatorCallback
and SaslExtensionsValidatorCallback so that it is clearer that these are
two separate entities that need validation? When we add support
for customisable extensions in other mechanisms, we could reuse
SaslExtensionsValidatorCallback. We will invoke CallbackHandler with {
OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback } in that
order like we do { NameCallback, PasswordCallback }. So typically we expect
to validate tokens with no reference to extensions, but we may refer to
token to validate extensions. Only validated extensions will be available
as the server's negotiated properties. We will need to handle
UnsupportedCallbackException for SaslExtensionsValidatorCallback for
backwards compatibility, but that should be ok.
What do you think?
On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski <stanis...@confluent.io>
wrote:
> Hi Ron,
>
> Yes, I agree we should document it thoroughly
>
> On Wed, Aug 8, 2018 at 5:02 PM Ron Dagostino <rndg...@gmail.com> wrote:
>
> > Hi Stanislav. If the community agrees we should add it then we should
> at a
> > minimum add explicit warnings in the Javadoc making it very clear how
> this
> > should not be used.
> >
> > Ron
> >
> > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski <
> > stanis...@confluent.io>
> > wrote:
> >
> > > Hey Ron,
> > >
> > > I fully agree that token validation is a serious security operation.
> > > Although, I believe allowing the user to do more validation with the
> > > extensions does not hurt - the user is fully responsible for his
> security
> > > once he starts implementing custom code for token validation. You would
> > > expect people to take the appropriate considerations when validating
> > > unsecured extensions against the token.
> > > I also think that using the extensions as a secondary validation method
> > > might be useful. You could do your normal validation using the token
> and
> > > then have a second sanity-check validation on top (e.g validate
> > > hostname/port is what client expected). Keep in mind that the server
> > > exposes the properties via `getNegotiatedProperty` so it makes sense to
> > > allow the server to have custom validation on the extensions.
> > >
> > > Best,
> > > Stanislav
> > >
> > > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino <rndg...@gmail.com>
> wrote:
> > >
> > > > Hi Stanislav. If you wanted to do this a good way might be to add a
> > > > constructor to the org.apache.kafka.common.security.oauthbearer.
> > > > OAuthBearerValidatorCallback class that accepts a SaslExtensions
> > instance
> > > > in addition to a token value. Then it would give the callback
> handler
> > > the
> > > > option to introspect the callback to see what extensions were
> provided
> > > with
> > > > the
> > > > token.
> > > >
> > > > That being said, token validation is a very security-sensitive
> > operation,
> > > > and it would be a serious security issue if the result of applying
> the
> > > > validation algorithm (which yields a valid vs. not valid
> determination)
> > > > depended on anything provided by the client other than the actual
> token
> > > > value. Nobody should ever allow the client to specify a JWK Set URL,
> > for
> > > > example, or a whitelist of acceptable domains for retrieving JWK
> Sets.
> > > It
> > > > feels to me that while a use case might exist (some kind of trace ID,
> > for
> > > > example, to aid in debugging), someone might inadvertently hang
> > > themselves
> > > > if we give them the rope. The risk vs. reward value proposition here
> > > > doesn't feel like a good one at first glance. Thoughts?
> > > >
> > > > Ron
> > > >
> > > >
> > > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav Kozlovski <
> > > > stanis...@confluent.io>
> > > > wrote:
> > > >
> > > > > Hey everybody,
> > > > >
> > > > > Sorry for reviving this, but I neglected something the first time
> > > around.
> > > > > I believe it would be useful to provide the
> > > > > `OAuthBearerUnsecuredValidatorCallbackHandler` with the OAuth
> > > extensions
> > > > > too. This would enable use cases where validators want to reconcile
> > > > > information from the extensions with the token (e.g if users have
> > > > > implemented secured OAuth tokens).
> > > > > The implementation would be to instantiate
> > > > > `OAuthBearerUnsecuredValidatorCallback` with the extensions (also
> > leave
> > > > the
> > > > > current constructor, as its a public class).
> > > > >
> > > > > What are everybody's thoughts on this? If there are no objections,
> > I'll
> > > > > update the KIP in due time
> > > > >
> > > > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram <
> > > rajinisiva...@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Looks good. Thanks, Stanislav.
> > > > > >
> > > > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav Kozlovski <
> > > > > > stanis...@confluent.io
> > > > > > > wrote:
> > > > > >
> > > > > > > Hi Rajini,
> > > > > > >
> > > > > > > I updated the KIP. Please check if the clarification is okay
> > > > > > >
> > > > > > > On Wed, Jul 25, 2018 at 10:49 AM Rajini Sivaram <
> > > > > rajinisiva...@gmail.com
> > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Stanislav,
> > > > > > > >
> > > > > > > > 1. Can you clarify the following line in the KIP in the
> 'Public
> > > > > > > Interfaces'
> > > > > > > > section? When you are reading the KIP for the first time, it
> > > sounds
> > > > > > like
> > > > > > > we
> > > > > > > > adding a new Kafka config. But we are adding JAAS config
> > options
> > > > > with a
> > > > > > > > prefix that can be used with the default unsecured bearer
> > tokens.
> > > > We
> > > > > > > could
> > > > > > > > include the example in this section or at least link to the
> > > > example.
> > > > > > > >
> > > > > > > > - New config option for default, unsecured bearer tokens -
> > > > > > > > `unsecuredLoginExtension_<extensionname>`.
> > > > > > > >
> > > > > > > >
> > > > > > > > 2. Can you add the package for SaslExtensionsCallback class?
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav Kozlovski <
> > > > > > > > stanis...@confluent.io> wrote:
> > > > > > > >
> > > > > > > > > Hi Ron,
> > > > > > > > >
> > > > > > > > > Thanks for the suggestions. I have applied them to the KIP.
> > > > > > > > >
> > > > > > > > > On Tue, Jul 24, 2018 at 1:39 PM Ron Dagostino <
> > > rndg...@gmail.com
> > > > >
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Stanislav. The statement "New config option for
> > > > > > > > > OAuthBearerLoginModule"
> > > > > > > > > > is technically incorrect; it should be "New config option
> > for
> > > > > > > default,
> > > > > > > > > > unsecured bearer tokens" since that is what provides the
> > > > > > > functionality
> > > > > > > > > (as
> > > > > > > > > > opposed to the login module, which does not). Also,
> please
> > > > state
> > > > > > > that
> > > > > > > > > > "auth" is not supported as a custom extension name with
> any
> > > > > > > > > > SASL/OAUTHBEARER mechanism, including the unsecured one,
> > > since
> > > > it
> > > > > > is
> > > > > > > > > > reserved by the spec for what is normally sent in the
> HTTP
> > > > > > > > Authorization
> > > > > > > > > > header an attempt to use it will result in a
> configuration
> > > > > > exception.
> > > > > > > > > >
> > > > > > > > > > Finally, please also state that while the
> > > > OAuthBearerLoginModule
> > > > > > and
> > > > > > > > the
> > > > > > > > > > OAuthBearerSaslClient will be changed to request the
> > > extensions
> > > > > > from
> > > > > > > > its
> > > > > > > > > > callback handler, for backwards compatibility it is not
> > > > necessary
> > > > > > for
> > > > > > > > the
> > > > > > > > > > callback handler to support SaslExtensionsCallback -- any
> > > > > > > > > > UnsupportedCallbackException that is thrown will be
> ignored
> > > and
> > > > > no
> > > > > > > > > > extensions will be added.
> > > > > > > > > >
> > > > > > > > > > Ron
> > > > > > > > > >
> > > > > > > > > > On Tue, Jul 24, 2018 at 11:20 AM Stanislav Kozlovski <
> > > > > > > > > > stanis...@confluent.io>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hey everybody,
> > > > > > > > > > >
> > > > > > > > > > > I have updated the KIP to reflect the latest changes as
> > > best
> > > > > as I
> > > > > > > > > could.
> > > > > > > > > > If
> > > > > > > > > > > there aren't more suggestions, I intent to start the
> > [VOTE]
> > > > > > thread
> > > > > > > > > > > tomorrow.
> > > > > > > > > > >
> > > > > > > > > > > Best,
> > > > > > > > > > > Stanislav
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Jul 24, 2018 at 6:34 AM Ron Dagostino <
> > > > > rndg...@gmail.com
> > > > > > >
> > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Stanislav. Could you update the KIP to reflect
> the
> > > > latest
> > > > > > > > > > definition
> > > > > > > > > > > of
> > > > > > > > > > > > SaslExtensions and confirm or correct the impact it
> has
> > > to
> > > > > the
> > > > > > > > > > > > SCRAM-related classes? I'm not sure if the
> > > > > currently-described
> > > > > > > > > impact
> > > > > > > > > > is
> > > > > > > > > > > > still accurate. Also, could you mention the changes
> to
> > > > > > > > > > > > OAuthBearerUnsecuredLoginCallbackHandler in the
> text in
> > > > > > > addition to
> > > > > > > > > > > giving
> > > > > > > > > > > > the examples? The examples show the new
> > > > > > > > > > > > unsecuredLoginExtension_<extensionName> feature, but
> > that
> > > > > > > feature
> > > > > > > > is
> > > > > > > > > > not
> > > > > > > > > > > > described anywhere prior to it appearing there.
> > > > > > > > > > > >
> > > > > > > > > > > > Ron
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron Dagostino <
> > > > > > rndg...@gmail.com
> > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Hi Rajini. I think a class is fine as long as we
> > make
> > > > sure
> > > > > > the
> > > > > > > > > > > semantics
> > > > > > > > > > > > > of immutability are clear -- it would have to be a
> > > value
> > > > > > class,
> > > > > > > > and
> > > > > > > > > > any
> > > > > > > > > > > > > constructor that accepts a Map as input would have
> to
> > > > copy
> > > > > > that
> > > > > > > > Map
> > > > > > > > > > > > rather
> > > > > > > > > > > > > than store it in a member variable. Similarly, any
> > Map
> > > > > that
> > > > > > it
> > > > > > > > > might
> > > > > > > > > > > > > return would have to be unmodifiable.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Ron
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Jul 23, 2018 at 12:24 PM Rajini Sivaram <
> > > > > > > > > > > rajinisiva...@gmail.com
> > > > > > > > > > > > >
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > >> Hi Ron, Stanislav,
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> I agree with Stanislav that it would be better to
> > > leave
> > > > > > > > > > > `SaslExtensions`
> > > > > > > > > > > > >> as
> > > > > > > > > > > > >> a class rather than make it an interface. We
> don''t
> > > > really
> > > > > > > > expect
> > > > > > > > > > > users
> > > > > > > > > > > > to
> > > > > > > > > > > > >> extends this class, so it is convenient to have an
> > > > > > > > implementation
> > > > > > > > > > > since
> > > > > > > > > > > > >> users need to create an instance. The class
> provided
> > > by
> > > > > the
> > > > > > > > public
> > > > > > > > > > API
> > > > > > > > > > > > >> should be sufficient in the vast majority of the
> > > cases.
> > > > > Ron,
> > > > > > > do
> > > > > > > > > you
> > > > > > > > > > > > agree?
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> On Mon, Jul 23, 2018 at 11:35 AM, Ron Dagostino <
> > > > > > > > > rndg...@gmail.com>
> > > > > > > > > > > > >> wrote:
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> > Hi Stanislav. See
> > > > > > > > > > https://tools.ietf.org/html/rfc7628#section-3.1,
> > > > > > > > > > > > and
> > > > > > > > > > > > >> > that section refers to the core ABNF productions
> > > > defined
> > > > > > in
> > > > > > > > > > > > >> > https://tools.ietf.org/html/rfc5234#appendix-B.
> > > > > > > > > > > > >> >
> > > > > > > > > > > > >> > Ron
> > > > > > > > > > > > >> >
> > > > > > > > > > > > >> > > On Jul 23, 2018, at 1:30 AM, Stanislav
> > Kozlovski <
> > > > > > > > > > > > >> stanis...@confluent.io>
> > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > > Hey Ron and Rajini,
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > > Here are my thoughts:
> > > > > > > > > > > > >> > > Regarding separators in SaslExtensions -
> Agreed,
> > > > that
> > > > > > was
> > > > > > > a
> > > > > > > > > bad
> > > > > > > > > > > > move.
> > > > > > > > > > > > >> > > Should definitely not be a concern of
> > > > CallbackHandler
> > > > > > and
> > > > > > > > > > > > LoginModule
> > > > > > > > > > > > >> > > implementors.
> > > > > > > > > > > > >> > > SaslExtensions interface - Wouldn't
> implementing
> > > it
> > > > as
> > > > > > an
> > > > > > > > > > > interface
> > > > > > > > > > > > >> mean
> > > > > > > > > > > > >> > > that users will have to make sure they're
> > passing
> > > in
> > > > > an
> > > > > > > > > > > unmodifiable
> > > > > > > > > > > > >> map
> > > > > > > > > > > > >> > > themselves. I believe it would be better if we
> > > > > enforced
> > > > > > > that
> > > > > > > > > > > through
> > > > > > > > > > > > >> > class
> > > > > > > > > > > > >> > > constructors instead.
> > > > > > > > > > > > >> > > SaslExtensions#map() - I'd also prefer this.
> The
> > > > > reason
> > > > > > I
> > > > > > > > went
> > > > > > > > > > > with
> > > > > > > > > > > > >> > > `extensionValue` and `extensionNames` was
> > because
> > > I
> > > > > > > figured
> > > > > > > > it
> > > > > > > > > > > made
> > > > > > > > > > > > >> sense
> > > > > > > > > > > > >> > > to have `ScramExtensions` extend
> > `SaslExtensions`
> > > > and
> > > > > > > > > therefore
> > > > > > > > > > > have
> > > > > > > > > > > > >> > their
> > > > > > > > > > > > >> > > API be similar. In the end, do you think that
> it
> > > is
> > > > > > worth
> > > > > > > it
> > > > > > > > > to
> > > > > > > > > > > have
> > > > > > > > > > > > >> > > `ScramExtensions` extend `SaslExtensions`?
> > > > > > > > > > > > >> > > @Ron, could you point me to the SASL OAuth
> > > mechanism
> > > > > > > > specific
> > > > > > > > > > > > regular
> > > > > > > > > > > > >> > > expressions for keys/values you mentioned are
> in
> > > RFC
> > > > > > 7628
> > > > > > > (
> > > > > > > > > > > > >> > > https://tools.ietf.org/html/rfc7628) ? I
> could
> > > not
> > > > > find
> > > > > > > any
> > > > > > > > > > while
> > > > > > > > > > > > >> > > originally implementing this.
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > > Best,
> > > > > > > > > > > > >> > > Stanislav
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > >> On Sun, Jul 22, 2018 at 6:46 PM Ron
> Dagostino <
> > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > >
> > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> Hi again, Rajini and Stanislav. I wonder if
> > > making
> > > > > > > > > > > SaslExtensions
> > > > > > > > > > > > an
> > > > > > > > > > > > >> > >> interface rather than a class might be a good
> > > > > solution.
> > > > > > > > For
> > > > > > > > > > > > example:
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> public interface SaslExtensions {
> > > > > > > > > > > > >> > >> /**
> > > > > > > > > > > > >> > >> * @return an immutable map view of the
> SASL
> > > > > > extensions
> > > > > > > > > > > > >> > >> */
> > > > > > > > > > > > >> > >> Map<String, String> map();
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> This solves the issue of lack of clarity on
> > > > > > immutability,
> > > > > > > > and
> > > > > > > > > > it
> > > > > > > > > > > > also
> > > > > > > > > > > > >> > >> eliminates copying, like this:
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > >> > >> Map<String, String> myRetval =
> > > > > > > > > > > > getUnmodifiableSaslExtensionsMap();
> > > > > > > > > > > > >> > >> return new SaslExtensions() {
> > > > > > > > > > > > >> > >> public Map<String, String> map() {
> > > > > > > > > > > > >> > >> return myRetval;
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> Alternatively, we could do it like this:
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> /**
> > > > > > > > > > > > >> > >> * Supplier that returns immutable map view of
> > > SASL
> > > > > > > > Extensions
> > > > > > > > > > > > >> > >> */
> > > > > > > > > > > > >> > >> public interface SaslExtensions extends
> > > > > > > > Supplier<Map<String,
> > > > > > > > > > > > >> String>> {
> > > > > > > > > > > > >> > >> // empty
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> The we could simply return the instance like
> > > this,
> > > > > > again
> > > > > > > > > > without
> > > > > > > > > > > > >> > copying:
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> SaslExtensions myMethod() {
> > > > > > > > > > > > >> > >> Map<String, String> myRetval =
> > > > > > > > > > > > getUnmodifiableSaslExtensionsMap();
> > > > > > > > > > > > >> > >> return () -> myRetval;
> > > > > > > > > > > > >> > >> }
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> I think the main reason for making
> > SaslExtensions
> > > > > part
> > > > > > of
> > > > > > > > the
> > > > > > > > > > > > public
> > > > > > > > > > > > >> > >> interface is to avoid adding a Map to the
> > > Subject's
> > > > > > > public
> > > > > > > > > > > > >> credentials.
> > > > > > > > > > > > >> > >> Making SaslExtensions an interface meets that
> > > > > > requirement
> > > > > > > > and
> > > > > > > > > > > then
> > > > > > > > > > > > >> > allows
> > > > > > > > > > > > >> > >> us to be free to implement whatever we want
> > > > > internally.
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> Thoughts?
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >> Ron
> > > > > > > > > > > > >> > >>
> > > > > > > > > > > > >> > >>> On Sun, Jul 22, 2018 at 12:45 PM Ron
> > Dagostino <
> > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > >
> > > > > > > > > > > > >> > wrote:
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>> Hi Rajini. The SaslServer is going to have
> to
> > > > > > validate
> > > > > > > > the
> > > > > > > > > > > > >> extensions,
> > > > > > > > > > > > >> > >>> too, but I’m okay with keeping the
> validation
> > > > logic
> > > > > > > > > elsewhere
> > > > > > > > > > as
> > > > > > > > > > > > >> long
> > > > > > > > > > > > >> > as
> > > > > > > > > > > > >> > >> it
> > > > > > > > > > > > >> > >>> can be reused in both the client and the
> > secret.
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>> I strongly prefer exposing a map() method as
> > > > opposed
> > > > > > to
> > > > > > > > > > > > >> > extensionNames()
> > > > > > > > > > > > >> > >>> and extensionValue(String) methods. It is a
> > > > smaller
> > > > > > API
> > > > > > > (2
> > > > > > > > > > > methods
> > > > > > > > > > > > >> > >> instead
> > > > > > > > > > > > >> > >>> of 1), and it gives clients of the API full
> > > > > > map-related
> > > > > > > > > > > > >> functionality
> > > > > > > > > > > > >> > >>> (there’s a lot of support for dealing with
> > maps
> > > > in a
> > > > > > > > variety
> > > > > > > > > > of
> > > > > > > > > > > > >> ways).
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>> Regardless of whether we go with a map()
> > method
> > > or
> > > > > > > > > > > > extensionNames()
> > > > > > > > > > > > >> and
> > > > > > > > > > > > >> > >>> extensionValue(String) methods, the
> semantics
> > of
> > > > > > > > mutability
> > > > > > > > > > need
> > > > > > > > > > > > to
> > > > > > > > > > > > >> be
> > > > > > > > > > > > >> > >>> clear. I think either way we should never
> > > share a
> > > > > map
> > > > > > > > that
> > > > > > > > > > > anyone
> > > > > > > > > > > > >> else
> > > > > > > > > > > > >> > >>> could possibly mutate — either a map that
> > > someone
> > > > > > gives
> > > > > > > us
> > > > > > > > > or
> > > > > > > > > > a
> > > > > > > > > > > > map
> > > > > > > > > > > > >> > that
> > > > > > > > > > > > >> > >> we
> > > > > > > > > > > > >> > >>> might expose.
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>> Thoughts?
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>> Ron
> > > > > > > > > > > > >> > >>>
> > > > > > > > > > > > >> > >>>> On Jul 22, 2018, at 11:23 AM, Rajini
> Sivaram
> > <
> > > > > > > > > > > > >> rajinisiva...@gmail.com
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> Hmm.... I think we need a much simpler
> > > > > SaslExtensions
> > > > > > > > class
> > > > > > > > > > if
> > > > > > > > > > > we
> > > > > > > > > > > > >> are
> > > > > > > > > > > > >> > >>>> making it part of the public API.
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> 1. I don't see the point of including
> > separator
> > > > > > > anywhere
> > > > > > > > in
> > > > > > > > > > > > >> > >>> SaslExtensions.
> > > > > > > > > > > > >> > >>>> Extensions provide a map and we propagate
> the
> > > map
> > > > > > from
> > > > > > > > > client
> > > > > > > > > > > to
> > > > > > > > > > > > >> > server
> > > > > > > > > > > > >> > >>>> using the protocol associated with the
> > > mechanism
> > > > in
> > > > > > > use.
> > > > > > > > > The
> > > > > > > > > > > > >> separator
> > > > > > > > > > > > >> > >> is
> > > > > > > > > > > > >> > >>>> not configurable and should not be a
> concern
> > of
> > > > the
> > > > > > > > > > implementor
> > > > > > > > > > > > of
> > > > > > > > > > > > >> > >>>> SaslExtensionsCallback interface that
> > provides
> > > an
> > > > > > > > instance
> > > > > > > > > of
> > > > > > > > > > > > >> > >>> SaslExtensions
> > > > > > > > > > > > >> > >>>> .
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> 2. I agree with Ron that we need
> > > > mechanism-specific
> > > > > > > > > > validation
> > > > > > > > > > > of
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >>>> values from SaslExtensions. But I think we
> > > could
> > > > do
> > > > > > the
> > > > > > > > > > > > validation
> > > > > > > > > > > > >> in
> > > > > > > > > > > > >> > >> the
> > > > > > > > > > > > >> > >>>> appropriate `SaslClient` implementation of
> > that
> > > > > > > > mechanism.
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> I think we could just have a very simple
> > > > extensions
> > > > > > > class
> > > > > > > > > and
> > > > > > > > > > > > move
> > > > > > > > > > > > >> > >>>> everything else to appropriate internal
> > classes
> > > > of
> > > > > > the
> > > > > > > > > > > mechanisms
> > > > > > > > > > > > >> > using
> > > > > > > > > > > > >> > >>>> extensions. What do you think?
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> public class SaslExtensions {
> > > > > > > > > > > > >> > >>>> private final Map<String, String>
> > > extensionMap;
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> public SaslExtensions(Map<String, String>
> > > > > > > > extensionMap) {
> > > > > > > > > > > > >> > >>>> this.extensionMap = extensionMap;
> > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> public String extensionValue(String
> name) {
> > > > > > > > > > > > >> > >>>> return extensionMap.get(name);
> > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>> public Set<String> extensionNames() {
> > > > > > > > > > > > >> > >>>> return extensionMap.keySet();
> > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > >> > >>>> }
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>>
> > > > > > > > > > > > >> > >>>>> On Sat, Jul 21, 2018 at 9:01 PM, Ron
> > > Dagostino <
> > > > > > > > > > > > rndg...@gmail.com
> > > > > > > > > > > > >> >
> > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> Hi Stanislav and Rajini. If
> SaslExtensions
> > is
> > > > > going
> > > > > > > to
> > > > > > > > > part
> > > > > > > > > > > of
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >>> public
> > > > > > > > > > > > >> > >>>>> API, then it occurred to me that one of
> the
> > > > > > > requirements
> > > > > > > > > of
> > > > > > > > > > > all
> > > > > > > > > > > > >> SASL
> > > > > > > > > > > > >> > >>>>> extensions is that the keys and values
> need
> > to
> > > > > match
> > > > > > > > > > > > >> > >> mechanism-specific
> > > > > > > > > > > > >> > >>>>> regular expressions. For example, RFC
> 5802
> > (
> > > > > > > > > > > > >> > >>>>> https://tools.ietf.org/html/rfc5802)
> > > specifies
> > > > > the
> > > > > > > > > regular
> > > > > > > > > > > > >> > >> expressions
> > > > > > > > > > > > >> > >>> for
> > > > > > > > > > > > >> > >>>>> the SCRAM-specific SASL mechanisms, and
> RFC
> > > > 7628 (
> > > > > > > > > > > > >> > >>>>> https://tools.ietf.org/html/rfc7628)
> > > specifies
> > > > > > > > different
> > > > > > > > > > > > regular
> > > > > > > > > > > > >> > >>>>> expressions for the OAUTHBEARER SASL
> > > > mechanism. I
> > > > > > am
> > > > > > > > > > thinking
> > > > > > > > > > > > the
> > > > > > > > > > > > >> > >>>>> SaslExtensions class should probably
> > provide a
> > > > way
> > > > > > to
> > > > > > > > make
> > > > > > > > > > > sure
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >> keys
> > > > > > > > > > > > >> > >>>>> and values match the appropriate regular
> > > > > > expressions.
> > > > > > > > > What
> > > > > > > > > > do
> > > > > > > > > > > > you
> > > > > > > > > > > > >> > >>> think of
> > > > > > > > > > > > >> > >>>>> something along the lines of the below
> > > > definition
> > > > > > for
> > > > > > > > the
> > > > > > > > > > > > >> > >> SaslExtensions
> > > > > > > > > > > > >> > >>>>> class? It is missing Javadoc and
> > > > > > > > > > > toString()/hashCode()/equals()
> > > > > > > > > > > > >> > >>> methods,
> > > > > > > > > > > > >> > >>>>> of course, but aside from that, do you
> think
> > > > this
> > > > > is
> > > > > > > > > > > sufficient
> > > > > > > > > > > > >> and
> > > > > > > > > > > > >> > >>>>> appropriate?
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> Ron
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> public class SaslExtensions {
> > > > > > > > > > > > >> > >>>>> private final Map<String, String>
> > > > extensionsMap;
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> public SaslExtensions(String mapStr,
> > String
> > > > > > > > > > > keyValueSeparator,
> > > > > > > > > > > > >> > >> String
> > > > > > > > > > > > >> > >>>>> elementSeparator,
> > > > > > > > > > > > >> > >>>>> Pattern saslNameRegexPattern,
> > > Pattern
> > > > > > > > > > > > >> > >> saslValueRegexPattern)
> > > > > > > > > > > > >> > >>> {
> > > > > > > > > > > > >> > >>>>> this(Utils.parseMap(mapStr,
> > > > > keyValueSeparator,
> > > > > > > > > > > > >> > >> elementSeparator),
> > > > > > > > > > > > >> > >>>>> saslNameRegexPattern,
> > saslValueRegexPattern);
> > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> public SaslExtensions(Map<String,
> String>
> > > > > > > > extensionsMap,
> > > > > > > > > > > > Pattern
> > > > > > > > > > > > >> > >>>>> saslNameRegexPattern,
> > > > > > > > > > > > >> > >>>>> Pattern saslValueRegexPattern) {
> > > > > > > > > > > > >> > >>>>> Map<String, String> sanitizedCopy =
> > new
> > > > > > > > > > > > >> > >>>>> HashMap<>(extensionsMap.size());
> > > > > > > > > > > > >> > >>>>> for (Entry<String, String> entry :
> > > > > > > > > > > > >> extensionsMap.entrySet()) {
> > > > > > > > > > > > >> > >>>>> if (!saslNameRegexPattern.
> > > > > > > > > matcher(entry.getKey()).
> > > > > > > > > > > > >> > matches()
> > > > > > > > > > > > >> > >>>>> ||
> > > > > > > > > > > > >> > >>>>>
> > > > !saslValueRegexPattern.matcher(entry.getValue()).
> > > > > > > > > matches())
> > > > > > > > > > > > >> > >>>>> throw new
> > > > > > > > IllegalArgumentException("Invalid
> > > > > > > > > > key
> > > > > > > > > > > or
> > > > > > > > > > > > >> > >>>>> value");
> > > > > > > > > > > > >> > >>>>> sanitizedCopy.put(entry.getKe
> y(),
> > > > > > > > > > entry.getValue());
> > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > >> > >>>>> this.extensionsMap =
> > > > > > > > > > > > >> > >> Collections.unmodifiableMap(sanitizedCopy);
> > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> public Map<String, String> map() {
> > > > > > > > > > > > >> > >>>>> return extensionsMap;
> > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > >> > >>>>> }
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>> On Fri, Jul 20, 2018 at 12:49 PM Stanislav
> > > > > > Kozlovski <
> > > > > > > > > > > > >> > >>>>> stanis...@confluent.io>
> > > > > > > > > > > > >> > >>>>> wrote:
> > > > > > > > > > > > >> > >>>>>
> > > > > > > > > > > > >> > >>>>>> Hi Ron,
> > > > > > > > > > > > >> > >>>>>>
> > > > > > > > > > > > >> > >>>>>> I saw that and decided that would be the
> > best
> > > > > > > approach.
> > > > > > > > > The
> > > > > > > > > > > > >> current
> > > > > > > > > > > > >> > >>>>>> ScramExtensions implementation uses a Map
> > in
> > > > the
> > > > > > > public
> > > > > > > > > > > > >> credentials
> > > > > > > > > > > > >> > >>> and I
> > > > > > > > > > > > >> > >>>>>> thought I would follow convention rather
> > than
> > > > > > > introduce
> > > > > > > > > my
> > > > > > > > > > > own
> > > > > > > > > > > > >> > thing,
> > > > > > > > > > > > >> > >>> but
> > > > > > > > > > > > >> > >>>>>> maybe this is best
> > > > > > > > > > > > >> > >>>>>>
> > > > > > > > > > > > >> > >>>>>>> On Fri, Jul 20, 2018 at 8:39 AM Ron
> > > Dagostino
> > > > <
> > > > > > > > > > > > >> rndg...@gmail.com>
> > > > > > > > > > > > >> > >>> wrote:
> > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > >> > >>>>>>> Hi Stanislav. I'm wondering if we
> should
> > > make
> > > > > > > > > > > SaslExtensions
> > > > > > > > > > > > >> part
> > > > > > > > > > > > >> > >> of
> > > > > > > > > > > > >> > >>>>> the
> > > > > > > > > > > > >> > >>>>>>> public API. I mentioned this in my
> review
> > > of
> > > > > the
> > > > > > > PR,
> > > > > > > > > too
> > > > > > > > > > > (and
> > > > > > > > > > > > >> > >> tagged
> > > > > > > > > > > > >> > >>>>>>> Rajini to get her input). If we add a
> Map
> > > to
> > > > > the
> > > > > > > > > > Subject's
> > > > > > > > > > > > >> public
> > > > > > > > > > > > >> > >>>>>>> credentials we are basically making a
> > public
> > > > > > > > commitment
> > > > > > > > > > that
> > > > > > > > > > > > any
> > > > > > > > > > > > >> > Map
> > > > > > > > > > > > >> > >>>>>>> associated with the public credentials
> > > defines
> > > > > the
> > > > > > > > SASL
> > > > > > > > > > > > >> extensions
> > > > > > > > > > > > >> > >> and
> > > > > > > > > > > > >> > >>>>> we
> > > > > > > > > > > > >> > >>>>>>> can never add another instance
> > implementing
> > > > Map
> > > > > to
> > > > > > > the
> > > > > > > > > > > public
> > > > > > > > > > > > >> > >>>>>> credentials.
> > > > > > > > > > > > >> > >>>>>>> That's a very big constraint we are
> > > committing
> > > > > to,
> > > > > > > and
> > > > > > > > > I'm
> > > > > > > > > > > > >> > wondering
> > > > > > > > > > > > >> > >>> if
> > > > > > > > > > > > >> > >>>>>> we
> > > > > > > > > > > > >> > >>>>>>> should make SaslExtensions public and
> > attach
> > > > an
> > > > > > > > instance
> > > > > > > > > > of
> > > > > > > > > > > > >> that to
> > > > > > > > > > > > >> > >>> the
> > > > > > > > > > > > >> > >>>>>>> Subject's public credentials instead.
> > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > >> > >>>>>>> Ron
> > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > >> > >>>>>>> On Thu, Jul 19, 2018 at 8:15 PM
> Stanislav
> > > > > > Kozlovski
> > > > > > > <
> > > > > > > > > > > > >> > >>>>>>> stanis...@confluent.io>
> > > > > > > > > > > > >> > >>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > >> > >>>>>>>> I have updated the PR and KIP to
> address
> > > the
> > > > > > > comments
> > > > > > > > > > made
> > > > > > > > > > > so
> > > > > > > > > > > > >> far.
> > > > > > > > > > > > >> > >>>>>> Please
> > > > > > > > > > > > >> > >>>>>>>> take another look at them and share
> your
> > > > > > thoughts.
> > > > > > > > > > > > >> > >>>>>>>> KIP:
> > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>
> > > > > > > > > > > > >> > >>>>>> https://cwiki.apache.org/
> > > > > > > confluence/display/KAFKA/KIP-
> > > > > > > > > > > > >> > >>>>>
> > > > 342%3A+Add+support+for+Custom+SASL+extensions+in+
> > > > > > > > > > > > >> > >>>>> OAuthBearer+authentication
> > > > > > > > > > > > >> > >>>>>>>> PR: Pull request <
> > > > > > > > > > > https://github.com/apache/kafka/pull/5379>
> > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>> Best,
> > > > > > > > > > > > >> > >>>>>>>> Stanislav
> > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>> On Thu, Jul 19, 2018 at 1:58 PM
> Stanislav
> > > > > > > Kozlovski <
> > > > > > > > > > > > >> > >>>>>>>> stanis...@confluent.io>
> > > > > > > > > > > > >> > >>>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>> Hi Ron,
> > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>> Agreed. `SaslExtensionsCallback` will
> be
> > > the
> > > > > > only
> > > > > > > > > public
> > > > > > > > > > > API
> > > > > > > > > > > > >> > >>>>> addition
> > > > > > > > > > > > >> > >>>>>>> and
> > > > > > > > > > > > >> > >>>>>>>>> new documentation for the extension
> > > strings.
> > > > > > > > > > > > >> > >>>>>>>>> A question that came up - should the
> > > > > > > > > > LoginCallbackHandler
> > > > > > > > > > > > >> throw
> > > > > > > > > > > > >> > an
> > > > > > > > > > > > >> > >>>>>>>>> exception or simply ignore key/value
> > > > extension
> > > > > > > pairs
> > > > > > > > > who
> > > > > > > > > > > do
> > > > > > > > > > > > >> not
> > > > > > > > > > > > >> > >>>>> match
> > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>> validation regex pattern? I guess it
> > would
> > > > be
> > > > > > > better
> > > > > > > > > to
> > > > > > > > > > > > >> throw, as
> > > > > > > > > > > > >> > >>>>> to
> > > > > > > > > > > > >> > >>>>>>>> avoid
> > > > > > > > > > > > >> > >>>>>>>>> confusion.
> > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>> And yes, I will make sure the
> key/value
> > > are
> > > > > > > > validated
> > > > > > > > > on
> > > > > > > > > > > the
> > > > > > > > > > > > >> > >> client
> > > > > > > > > > > > >> > >>>>>> as
> > > > > > > > > > > > >> > >>>>>>>>> well as in the server. Even then, I
> > > > structured
> > > > > > the
> > > > > > > > > > > > >> > >>>>>>>> getNegotiatedProperty()
> > > > > > > > > > > > >> > >>>>>>>>> method such that the OAUTHBEARER.token
> > can
> > > > > never
> > > > > > > be
> > > > > > > > > > > > >> overridden. I
> > > > > > > > > > > > >> > >>>>>>>>> considered adding a test for that,
> but I
> > > > > figured
> > > > > > > > > having
> > > > > > > > > > > the
> > > > > > > > > > > > >> regex
> > > > > > > > > > > > >> > >>>>>>>>> validation be enough of a guarantee.
> > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>> On Thu, Jul 19, 2018 at 9:49 AM Ron
> > > > Dagostino
> > > > > <
> > > > > > > > > > > > >> rndg...@gmail.com
> > > > > > > > > > > > >> > >
> > > > > > > > > > > > >> > >>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>> Hi Rajini and Stanislav. Rajini,
> yes,
> > I
> > > > > think
> > > > > > > you
> > > > > > > > > are
> > > > > > > > > > > > right
> > > > > > > > > > > > >> > >> about
> > > > > > > > > > > > >> > >>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>> login callback handler being more
> > > > appropriate
> > > > > > for
> > > > > > > > > > > > retrieving
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >>>>>> SASL
> > > > > > > > > > > > >> > >>>>>>>>>> extensions than the login module
> itself
> > > > (how
> > > > > > many
> > > > > > > > > times
> > > > > > > > > > > am
> > > > > > > > > > > > I
> > > > > > > > > > > > >> > >> going
> > > > > > > > > > > > >> > >>>>>> to
> > > > > > > > > > > > >> > >>>>>>>> have
> > > > > > > > > > > > >> > >>>>>>>>>> to be encouraged to leverage the
> > callback
> > > > > > > > handlers?!?
> > > > > > > > > > > lol).
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule should ask its
> > > login
> > > > > > > > callback
> > > > > > > > > > > > handler
> > > > > > > > > > > > >> to
> > > > > > > > > > > > >> > >>>>>> handle
> > > > > > > > > > > > >> > >>>>>>>> an
> > > > > > > > > > > > >> > >>>>>>>>>> instance of SaslExtensionsCallback in
> > > > > addition
> > > > > > to
> > > > > > > > an
> > > > > > > > > > > > >> instance of
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerTokenCallback, and the
> > default
> > > > > login
> > > > > > > > > > callback
> > > > > > > > > > > > >> handler
> > > > > > > > > > > > >> > >>>>>>>>>> implementation
> > > > > > > > > > (OAuthBearerUnsecuredLoginCallbackHandler)
> > > > > > > > > > > > >> > should
> > > > > > > > > > > > >> > >>>>>>> either
> > > > > > > > > > > > >> > >>>>>>>>>> return an empty map via callback or
> it
> > > > should
> > > > > > > > > recognize
> > > > > > > > > > > > >> > >> additional
> > > > > > > > > > > > >> > >>>>>>> JAAS
> > > > > > > > > > > > >> > >>>>>>>>>> module options of the form
> > > > > > > > > > > > >> > >>>>>>>
> > > unsecuredLoginExtension_<extensionName>=value
> > > > > > > > > > > > >> > >>>>>>>>>> so
> > > > > > > > > > > > >> > >>>>>>>>>> that arbitrary extensions can be
> added
> > in
> > > > > > > > development
> > > > > > > > > > and
> > > > > > > > > > > > >> test
> > > > > > > > > > > > >> > >>>>>>> scenarios
> > > > > > > > > > > > >> > >>>>>>>>>> (similar to how arbitrary claims on
> > > > unsecured
> > > > > > > > tokens
> > > > > > > > > > can
> > > > > > > > > > > be
> > > > > > > > > > > > >> > >>>>> created
> > > > > > > > > > > > >> > >>>>>> in
> > > > > > > > > > > > >> > >>>>>>>>>> those scenarios via the JAAS module
> > > options
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > unsecuredLoginStringClaim_<claimName>=value,
> > > > > > > etc.).
> > > > > > > > > > Then
> > > > > > > > > > > > the
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule can add a map
> of
> > > any
> > > > > > > > > extensions
> > > > > > > > > > to
> > > > > > > > > > > > the
> > > > > > > > > > > > >> > >>>>>>> Subject's
> > > > > > > > > > > > >> > >>>>>>>>>> public credentials where the default
> > SASL
> > > > > > client
> > > > > > > > > > callback
> > > > > > > > > > > > >> > handler
> > > > > > > > > > > > >> > >>>>>>> class
> > > > > > > > > > > > >> > >>>>>>>> (
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerSaslClientCallbackH
> andler)
> > can
> > > > be
> > > > > > > > > amended to
> > > > > > > > > > > > >> support
> > > > > > > > > > > > >> > >>>>>>>>>> SaslExtensionsCallback and look on
> the
> > > > > Subject
> > > > > > > > > > > accordingly.
> > > > > > > > > > > > >> > >> There
> > > > > > > > > > > > >> > >>>>>>> would
> > > > > > > > > > > > >> > >>>>>>>>>> be
> > > > > > > > > > > > >> > >>>>>>>>>> no need to implement a custom
> > > > > > > > > > > sasl.client.callback.handler.
> > > > > > > > > > > > >> > class
> > > > > > > > > > > > >> > >>>>> in
> > > > > > > > > > > > >> > >>>>>>> this
> > > > > > > > > > > > >> > >>>>>>>>>> case, and no logic would need to be
> > moved
> > > > to
> > > > > a
> > > > > > > > public
> > > > > > > > > > > > static
> > > > > > > > > > > > >> > >>>>> method
> > > > > > > > > > > > >> > >>>>>> on
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule as I had
> > proposed
> > > > (at
> > > > > > > least
> > > > > > > > > not
> > > > > > > > > > > > right
> > > > > > > > > > > > >> > now,
> > > > > > > > > > > > >> > >>>>>>> anyway
> > > > > > > > > > > > >> > >>>>>>>>>> --
> > > > > > > > > > > > >> > >>>>>>>>>> there may come a time when a need
> for a
> > > > > custom
> > > > > > > > > > > > >> > >>>>>>>>>> sasl.client.callback.handler.class
> is
> > > > > > > identified,
> > > > > > > > > and
> > > > > > > > > > at
> > > > > > > > > > > > that
> > > > > > > > > > > > >> > >>>>> point
> > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>> default implementation would either
> > have
> > > to
> > > > > > made
> > > > > > > > part
> > > > > > > > > > of
> > > > > > > > > > > > the
> > > > > > > > > > > > >> > >>>>> public
> > > > > > > > > > > > >> > >>>>>>> API
> > > > > > > > > > > > >> > >>>>>>>>>> with protected rather than private
> > > methods
> > > > so
> > > > > > it
> > > > > > > > > could
> > > > > > > > > > be
> > > > > > > > > > > > >> > >> directly
> > > > > > > > > > > > >> > >>>>>>>>>> extended
> > > > > > > > > > > > >> > >>>>>>>>>> or its logic would have to be moved
> to
> > > > public
> > > > > > > > static
> > > > > > > > > > > > methods
> > > > > > > > > > > > >> on
> > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule).
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>> So, to try to summarize, I think
> > > > > > > > > SaslExtensionsCallback
> > > > > > > > > > > > will
> > > > > > > > > > > > >> be
> > > > > > > > > > > > >> > >>>>> the
> > > > > > > > > > > > >> > >>>>>>> only
> > > > > > > > > > > > >> > >>>>>>>>>> public API addition due to this KIP
> in
> > > > terms
> > > > > of
> > > > > > > > code,
> > > > > > > > > > and
> > > > > > > > > > > > >> then
> > > > > > > > > > > > >> > >>>>> maybe
> > > > > > > > > > > > >> > >>>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>> recognition of the
> > > > unsecuredLoginExtension_<
> > > > > > > > > > > > >> > extensionName>=value
> > > > > > > > > > > > >> > >>>>>>> module
> > > > > > > > > > > > >> > >>>>>>>>>> options in the default unsecured case
> > > > (which
> > > > > > > would
> > > > > > > > > be a
> > > > > > > > > > > > >> > >>>>>> documentation
> > > > > > > > > > > > >> > >>>>>>>>>> change and an internal implementation
> > > issue
> > > > > > > rather
> > > > > > > > > > than a
> > > > > > > > > > > > >> public
> > > > > > > > > > > > >> > >>>>> API
> > > > > > > > > > > > >> > >>>>>>> in
> > > > > > > > > > > > >> > >>>>>>>>>> terms of code). And then also the
> fact
> > > > that
> > > > > > > > > extension
> > > > > > > > > > > > names
> > > > > > > > > > > > >> and
> > > > > > > > > > > > >> > >>>>>>> values
> > > > > > > > > > > > >> > >>>>>>>>>> are
> > > > > > > > > > > > >> > >>>>>>>>>> accessed on the server side via
> > > negotiated
> > > > > > > > > properties.
> > > > > > > > > > > Do
> > > > > > > > > > > > I
> > > > > > > > > > > > >> > have
> > > > > > > > > > > > >> > >>>>>> that
> > > > > > > > > > > > >> > >>>>>>>>>> summary right?
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>> One thing I want to note is that the
> > code
> > > > > needs
> > > > > > > to
> > > > > > > > > make
> > > > > > > > > > > > sure
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >>>>>>>> extension
> > > > > > > > > > > > >> > >>>>>>>>>> names are composed of only ALPHA
> > [a-zA-Z]
> > > > > > > > characters
> > > > > > > > > as
> > > > > > > > > > > per
> > > > > > > > > > > > >> the
> > > > > > > > > > > > >> > >>>>> spec
> > > > > > > > > > > > >> > >>>>>>>> (not
> > > > > > > > > > > > >> > >>>>>>>>>> only for that reason, but to also
> make
> > > sure
> > > > > the
> > > > > > > > token
> > > > > > > > > > > > >> available
> > > > > > > > > > > > >> > >> at
> > > > > > > > > > > > >> > >>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>> OAUTHBEARER.token negotiated property
> > > can't
> > > > > be
> > > > > > > > > > > > overwritten).
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>> Ron
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>> On Thu, Jul 19, 2018 at 12:43 PM
> > > Stanislav
> > > > > > > > Kozlovski
> > > > > > > > > <
> > > > > > > > > > > > >> > >>>>>>>>>> stanis...@confluent.io>
> > > > > > > > > > > > >> > >>>>>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>> Hey Ron,
> > > > > > > > > > > > >> > >>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>> Come to think of it, I think what
> > Rajini
> > > > > said
> > > > > > > > makes
> > > > > > > > > > more
> > > > > > > > > > > > >> sense
> > > > > > > > > > > > >> > >>>>>> than
> > > > > > > > > > > > >> > >>>>>>> my
> > > > > > > > > > > > >> > >>>>>>>>>>> initial proposal. Having the
> > > > > > > > > > > > >> OAuthBearerClientCallbackHandler
> > > > > > > > > > > > >> > >>>>>>> populate
> > > > > > > > > > > > >> > >>>>>>>>>>> SaslExtensionsCallback by taking a
> Map
> > > > from
> > > > > > the
> > > > > > > > > > Subject
> > > > > > > > > > > > >> would
> > > > > > > > > > > > >> > >>>>> ease
> > > > > > > > > > > > >> > >>>>>>>>>> users'
> > > > > > > > > > > > >> > >>>>>>>>>>> implementation - they'd only have to
> > > > > provide a
> > > > > > > > login
> > > > > > > > > > > > >> callback
> > > > > > > > > > > > >> > >>>>>>> handler
> > > > > > > > > > > > >> > >>>>>>>>>> which
> > > > > > > > > > > > >> > >>>>>>>>>>> attaches extensions to the Subject.
> > > > > > > > > > > > >> > >>>>>>>>>>> I will now update the PR and the
> > > examples
> > > > in
> > > > > > the
> > > > > > > > > KIP.
> > > > > > > > > > > Let
> > > > > > > > > > > > me
> > > > > > > > > > > > >> > >>>>> know
> > > > > > > > > > > > >> > >>>>>>> what
> > > > > > > > > > > > >> > >>>>>>>>>> you
> > > > > > > > > > > > >> > >>>>>>>>>>> think
> > > > > > > > > > > > >> > >>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>> Hi Rajini,
> > > > > > > > > > > > >> > >>>>>>>>>>> Yes, I will switch both classes to
> > > > > > > private/public
> > > > > > > > as
> > > > > > > > > > it
> > > > > > > > > > > > >> makes
> > > > > > > > > > > > >> > >>>>>> total
> > > > > > > > > > > > >> > >>>>>>>>>> sense.
> > > > > > > > > > > > >> > >>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>> Best,
> > > > > > > > > > > > >> > >>>>>>>>>>> Stanislav
> > > > > > > > > > > > >> > >>>>>>>>>>> On Thu, Jul 19, 2018 at 9:02 AM
> Rajini
> > > > > > Sivaram <
> > > > > > > > > > > > >> > >>>>>>>> rajinisiva...@gmail.com
> > > > > > > > > > > > >> > >>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> Hi Stanislav,
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> Thanks for the KIP. Since
> > > SaslExtensions
> > > > > will
> > > > > > > be
> > > > > > > > an
> > > > > > > > > > > > >> internal
> > > > > > > > > > > > >> > >>>>>>> class,
> > > > > > > > > > > > >> > >>>>>>>>>> can
> > > > > > > > > > > > >> > >>>>>>>>>>> we
> > > > > > > > > > > > >> > >>>>>>>>>>>> remove it from the KIP to avoid
> > > > confusion?
> > > > > > > Also,
> > > > > > > > > can
> > > > > > > > > > we
> > > > > > > > > > > > add
> > > > > > > > > > > > >> > >>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>> package
> > > > > > > > > > > > >> > >>>>>>>>>>>> name for SaslExtensionsCallback?
> The
> > PR
> > > > has
> > > > > > it
> > > > > > > in
> > > > > > > > > > > > >> > >>>>>>>>>>>> org.apache.kafka.common.security
> > which
> > > is
> > > > > an
> > > > > > > > > internal
> > > > > > > > > > > > >> > >>>>> package.
> > > > > > > > > > > > >> > >>>>>> As
> > > > > > > > > > > > >> > >>>>>>> a
> > > > > > > > > > > > >> > >>>>>>>>>>> public
> > > > > > > > > > > > >> > >>>>>>>>>>>> class, it could be in
> > > > > > > > > > > > >> org.apache.kafka.common.security.auth.
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> Regards,
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> Rajini
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> On Thu, Jul 19, 2018 at 4:50 PM,
> > Rajini
> > > > > > > Sivaram <
> > > > > > > > > > > > >> > >>>>>>>>>> rajinisiva...@gmail.com
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>> Hi Ron,
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>> Is there a reason why wouldn't
> want
> > to
> > > > > > provide
> > > > > > > > > > > > extensions
> > > > > > > > > > > > >> > >>>>>> using
> > > > > > > > > > > > >> > >>>>>>> a
> > > > > > > > > > > > >> > >>>>>>>>>> login
> > > > > > > > > > > > >> > >>>>>>>>>>>>> callback handler in the same way
> as
> > we
> > > > > > inject
> > > > > > > > > > tokens?
> > > > > > > > > > > > The
> > > > > > > > > > > > >> > >>>>>>> easiest
> > > > > > > > > > > > >> > >>>>>>>>>> way
> > > > > > > > > > > > >> > >>>>>>>>>>> to
> > > > > > > > > > > > >> > >>>>>>>>>>>>> inject custom extensions would be
> > > using
> > > > > the
> > > > > > > JAAS
> > > > > > > > > > > config.
> > > > > > > > > > > > >> So
> > > > > > > > > > > > >> > >>>>> we
> > > > > > > > > > > > >> > >>>>>>>> could
> > > > > > > > > > > > >> > >>>>>>>>>>> have
> > > > > > > > > > > > >> > >>>>>>>>>>>>> both OAuthBearerTokenCallback and
> > > > > > > > > > > SaslExtensionsCallback
> > > > > > > > > > > > >> > >>>>>>>> processed
> > > > > > > > > > > > >> > >>>>>>>>>> by
> > > > > > > > > > > > >> > >>>>>>>>>>> a
> > > > > > > > > > > > >> > >>>>>>>>>>>>> login callback handler. And the
> map
> > > > > returned
> > > > > > > by
> > > > > > > > > > > > >> > >>>>>>>>>> SaslExtensionsCallback
> > > > > > > > > > > > >> > >>>>>>>>>>>>> could be added to Subject by the
> > > default
> > > > > > > > > > > > >> > >>>>>>>>>>>> OAuthBearerSaslClientCallbackH
> andler.
> > > > > > > > > > > > >> > >>>>>>>>>>>>> Since OAuth users have to provide
> a
> > > > login
> > > > > > > > callback
> > > > > > > > > > > > handler
> > > > > > > > > > > > >> > >>>>>>> anyway,
> > > > > > > > > > > > >> > >>>>>>>>>>>> wouldn't
> > > > > > > > > > > > >> > >>>>>>>>>>>>> it be a better fit?
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>> Thank you,
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>> Rajini
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>> On Thu, Jul 19, 2018 at 4:08 PM,
> Ron
> > > > > > > Dagostino <
> > > > > > > > > > > > >> > >>>>>>> rndg...@gmail.com
> > > > > > > > > > > > >> > >>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>> wrote:
> > > > > > > > > > > > >> > >>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> Hi Stanislav.
> > > > > > > > > > > > >> > >>>>>>>>>>>>>>
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> Implementers of a custom
> > > > > > > > > > > sasl.client.callback.handler.
> > > > > > > > > > > > >> > >>>>> class
> > > > > > > > > > > > >> > >>>>>>> must
> > > > > > > > > > > > >> > >>>>>>>> be
> > > > > > > > > > > > >> > >>>>>>>>>>> sure
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> to
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> provide the existing logic in
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> org.apache.kafka.common.
> > > > > > > security.oauthbearer.
> > > > > > > > > > > > >> > >>>>> internals.OAuth
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> BearerSaslClientCallbackHandler
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> that handles instances of
> > > > > > > > > OAuthBearerTokenCallback
> > > > > > > > > > > (by
> > > > > > > > > > > > >> > >>>>>>> retrieving
> > > > > > > > > > > > >> > >>>>>>>>>> the
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> private credential from the
> > > Subject); a
> > > > > > > custom
> > > > > > > > > > > > >> > >>>>> implementation
> > > > > > > > > > > > >> > >>>>>>>> that
> > > > > > > > > > > > >> > >>>>>>>>>>> fails
> > > > > > > > > > > > >> > >>>>>>>>>>>>>> to
> > > > > > > > > > > > >> > >>>>>>>>>>>>
>
>
>
> --
> Best,
> Stanislav
>
