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.getKey(), 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
> > >>>>> (
> > >>>>>>> OAuthBearerSaslClientCallbackHandler) 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
> > >>>>>>>>> OAuthBearerSaslClientCallbackHandler.
> > >>>>>>>>>> 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
> > >>>>>>>>>>> do this will not work, so the KIP should state this
> > >>>> requirement.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The question then arises: how should implementers provide
> > >> the
> > >>>>>>> existing
> > >>>>>>>>>>> logic in the OAuthBearerSaslClientCallbackHandler class?
> > >>> That
> > >>>>>>> class
> > >>>>>>>> is
> > >>>>>>>>>>> not
> > >>>>>>>>>>> part of the public API, and its
> > >>>>>>>> handleCallback(OAuthBearerTokenCallback)
> > >>>>>>>>>>> method, which implements the logic, is private anyway (so
> > >>> even
> > >>>> if
> > >>>>>>>>> someone
> > >>>>>>>>>>> took the risk of extending the non-API class the method
> > >> would
> > >>>>>>>> generally
> > >>>>>>>>>>> not
> > >>>>>>>>>>> be available in the subclass). So as it stands right now
> > >>>>>>> implementers
> > >>>>>>>>> are
> > >>>>>>>>>>> left to copy/paste that logic into their code. A better
> > >>>> solution
> > >>>>>>>> might
> > >>>>>>>>> be
> > >>>>>>>>>>> to have the private method in
> > >>>>> OAuthBearerSaslClientCallbackHandler
> > >>>>>>>>>>> invoke a
> > >>>>>>>>>>> public static method on the
> > >>>>>>>>>>>
> > >>>>> org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule
> > >>>>>>>>> class
> > >>>>>>>>>>> (which is part of the public API) to retrieve the
> > >> credential
> > >>>>> (e.g.
> > >>>>>>>>> public
> > >>>>>>>>>>> static OAuthBearerToken retrieveCredential(Subject)) . The
> > >>>>>>> commit()
> > >>>>>>>>>>> method
> > >>>>>>>>>>> of the OAuthBearerLoginModule class is what puts the
> > >>> credential
> > >>>>>>> there
> > >>>>>>>> in
> > >>>>>>>>>>> the first place, so it could make sense for the class to
> > >>> expose
> > >>>>> the
> > >>>>>>>>>>> complementary logic for retrieving the credential in this
> > >>> way.
> > >>>>>>>>> Regarding
> > >>>>>>>>>>> your question about plugability of LoginModules, yes, the
> > >>>>>>> LoginModule
> > >>>>>>>>>>> class
> > >>>>>>>>>>> is explicitly stated in the JAAS config, so it is indeed
> > >>>>>>> pluggable; an
> > >>>>>>>>>>> extending class would override the commit() method, call
> > >>>>>>>> super.commit(),
> > >>>>>>>>>>> and if the return value is true it would do whatever is
> > >>>> necessary
> > >>>>>>> to
> > >>>>>>>> add
> > >>>>>>>>>>> the desired SASL extensions to the Subject -- probably in
> > >> the
> > >>>>>>> public
> > >>>>>>>>>>> credentials -- where a custom
> > >>>> sasl.client.callback.handler.class
> > >>>>>>> would
> > >>>>>>>>> be
> > >>>>>>>>>>> able to find them. The KIP might state this, too.
> > >>>>>>>>>>>
> > >>>>>>>>>>> I'll look forward to seeing a new PR once the fix for the
> > >>> 0x01
> > >>>>>>>> separator
> > >>>>>>>>>>> issue in the SASL/OAUTHBEARER implementation (KAFKA-7182
> > >>>>>>>>>>> <
> > >>>> https://issues.apache.org/jira/projects/KAFKA/issues/KAFKA-7182
> > >>>>>> )
> > >>>>>>> is
> > >>>>>>>>>>> merged.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Ron
> > >>>>>>>>>>>
> > >>>>>>>>>>> On Wed, Jul 18, 2018 at 6:38 PM Stanislav Kozlovski <
> > >>>>>>>>>>> stanis...@confluent.io>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> Hey Ron,
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> You brought up some great points. I did my best to
> > >> address
> > >>>> them
> > >>>>>>> and
> > >>>>>>>>>>> updated
> > >>>>>>>>>>>> the KIP.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I should mention that I used commas to separate
> > >> extensions
> > >>> in
> > >>>>> the
> > >>>>>>>>>>> protocol,
> > >>>>>>>>>>>> because we did not use the recommended Control-A
> > >> character
> > >>>> for
> > >>>>>>>>>>> separators
> > >>>>>>>>>>>> in the OAuth message and I figured I would not change it.
> > >>>>>>>>>>>> Now that I saw your PR about implementing the proper
> > >>>> separators
> > >>>>>>> in
> > >>>>>>>>> OAUTH
> > >>>>>>>>>>>> <https://github.com/apache/kafka/pull/5391> and will
> > >>> change
> > >>>> my
> > >>>>>>>>>>>> implementation once yours gets merged, meaning commas
> > >> will
> > >>>> be a
> > >>>>>>>>>>> supported
> > >>>>>>>>>>>> value for extensions.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> About the implementation: yes you're right, you should
> > >>>> define `
> > >>>>>>>>>>>> sasl.client.callback.handler.class` which has the same
> > >>>>>>> functionality
> > >>>>>>>>>>> as `
> > >>>>>>>>>>>> OAuthBearerSaslClientCallbackHandler` plus the
> > >> additional
> > >>>>>>>>>>> functionality of
> > >>>>>>>>>>>> handling the `SaslExtensionsCallback` by attaching
> > >>> extensions
> > >>>>> to
> > >>>>>>> it.
> > >>>>>>>>>>>> The only reason you'd populate the `Subject` object with
> > >>> the
> > >>>>>>>>> extensions
> > >>>>>>>>>>> is
> > >>>>>>>>>>>> if you used the default `SaslClientCallbackHandler`
> > >> (which
> > >>>>>>> handles
> > >>>>>>>> the
> > >>>>>>>>>>>> extensions callback by adding whatever's in the subject),
> > >>> as
> > >>>>> the
> > >>>>>>>> SCRAM
> > >>>>>>>>>>>> authentication does.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>> https://github.com/stanislavkozlovski/kafka/blob/KAFKA-7169-
> > >>>>>>>>>>> custom-sasl-extensions/clients/src/main/java/org/
> > >>>>>>>>>>> apache/kafka/common/security/
> > >> oauthbearer/internals/OAuthBea
> > >>>>>>>>>>> rerSaslClient.java#L92
> > >>>>>>>>>>>> And yes, in that case you would need a custom
> > >> `LoginModule`
> > >>>>> which
> > >>>>>>>>>>> populates
> > >>>>>>>>>>>> the Subject in that case, although I'm not sure if Kafka
> > >>>>> supports
> > >>>>>>>>>>> pluggable
> > >>>>>>>>>>>> LoginModules. Does it?
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Best,
> > >>>>>>>>>>>> Stanislav
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On Wed, Jul 18, 2018 at 9:50 AM Ron Dagostino <
> > >>>>> rndg...@gmail.com
> > >>>>>>>>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>> Hi Stanislav. Could you add something to the KIP about
> > >>> the
> > >>>>>>>> security
> > >>>>>>>>>>>>> implications related to the CSV name/value pairs sent
> > >> in
> > >>>> the
> > >>>>>>>>>>> extension?
> > >>>>>>>>>>>>> For example, the OAuth access token may have a digital
> > >>>>>>> signature,
> > >>>>>>>>> but
> > >>>>>>>>>>> the
> > >>>>>>>>>>>>> extensions generally will not (unless one of the values
> > >>> is
> > >>>> a
> > >>>>>>> JWS
> > >>>>>>>>>>> compact
> > >>>>>>>>>>>>> serialization, but I doubt anyone would go that far),
> > >> so
> > >>>> the
> > >>>>>>>> server
> > >>>>>>>>>>>>> generally cannot trust the extensions to be accurate
> > >> for
> > >>>>>>> anything
> > >>>>>>>>>>>>> critical. You mentioned the "better tracing and
> > >>>>>>> troubleshooting"
> > >>>>>>>>> use
> > >>>>>>>>>>>> case,
> > >>>>>>>>>>>>> which I think is fine despite the lack of security;
> > >> given
> > >>>>> that
> > >>>>>>>> lack
> > >>>>>>>>> of
> > >>>>>>>>>>>>> security, though, I believe it is important to also
> > >> state
> > >>>>> what
> > >>>>>>> the
> > >>>>>>>>>>>>> extensions should *not* be used for.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Also, could you indicate in the KIP how the extensions
> > >>>> might
> > >>>>>>>>> actually
> > >>>>>>>>>>> be
> > >>>>>>>>>>>>> added? My take on that would be to extend
> > >>>>>>> OAuthBearerLoginModule
> > >>>>>>>> to
> > >>>>>>>>>>>>> override the initialize() and commit() methods so that
> > >>> the
> > >>>>>>> derived
> > >>>>>>>>>>> class
> > >>>>>>>>>>>>> would have access to the Subject instance and could
> > >> add a
> > >>>> map
> > >>>>>>> to
> > >>>>>>>> the
> > >>>>>>>>>>>>> subject's public or private credentials when the commit
> > >>>>>>> succeeds;
> > >>>>>>>>>>> then I
> > >>>>>>>>>>>>> think the sasl.client.callback.handler.class would
> > >> have
> > >>> to
> > >>>> be
> > >>>>>>>>>>> explicitly
> > >>>>>>>>>>>>> set to a class that extends the default implementation
> > >>>>>>>>>>>>> (OAuthBearerSaslClientCallbackHandler) and retrieves
> > >> the
> > >>>> map
> > >>>>>>> when
> > >>>>>>>>>>>> handling
> > >>>>>>>>>>>>> the SaslExtensionsCallback. But maybe you are thinking
> > >>>> about
> > >>>>>>> it
> > >>>>>>>>>>>>> differently? Some guidance on how to actually take
> > >>>> advantage
> > >>>>>>> of
> > >>>>>>>> the
> > >>>>>>>>>>>>> feature via an implementation would be a useful
> > >> addition
> > >>> to
> > >>>>> the
> > >>>>>>>> KIP.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Finally, I note that the extension parsing does not
> > >>>> support a
> > >>>>>>>> comma
> > >>>>>>>>> in
> > >>>>>>>>>>>> keys
> > >>>>>>>>>>>>> or values. This should be addressed somehow -- either
> > >> by
> > >>>>>>>> supporting
> > >>>>>>>>>>> via
> > >>>>>>>>>>>> an
> > >>>>>>>>>>>>> escaping mechanism or by explicitly acknowledging that
> > >> it
> > >>>> is
> > >>>>>>>>>>> unsupported.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Thanks for the KIP and the simultaneous PR -- having
> > >> both
> > >>>> at
> > >>>>>>> the
> > >>>>>>>>> same
> > >>>>>>>>>>>> time
> > >>>>>>>>>>>>> really helped.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Ron
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> On Tue, Jul 17, 2018 at 6:22 PM Stanislav Kozlovski <
> > >>>>>>>>>>>>> stanis...@confluent.io>
> > >>>>>>>>>>>>> wrote:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Hey group,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> I just created a new KIP about adding customizable
> > >> SASL
> > >>>>>>>> extensions
> > >>>>>>>>>>> to
> > >>>>>>>>>>>> the
> > >>>>>>>>>>>>>> OAuthBearer authentication mechanism. More details in
> > >>> the
> > >>>>>>>> proposal
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> KIP:
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> 342%
> > >>>>>>>>>>>
> > >>>>>>>>
> > >>>>> 3A+Add+support+for+Custom+SASL+extensions+in+
> > >> OAuthBearer+authentication
> > >>>>>>>>>>>>>> JIRA: KAFKA-7169 <
> > >>>>>>>>> https://issues.apache.org/jira/browse/KAFKA-7169>
> > >>>>>>>>>>>>>> PR: Pull request <
> > >>>>> https://github.com/apache/kafka/pull/5379>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> --
> > >>>>>>>>>>>>>> Best,
> > >>>>>>>>>>>>>> Stanislav
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> --
> > >>>>>>>>>>>> Best,
> > >>>>>>>>>>>> Stanislav
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> --
> > >>>>>>>> Best,
> > >>>>>>>> Stanislav
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> --
> > >>>>>> Best,
> > >>>>>> Stanislav
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Best,
> > >>>>> Stanislav
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>> --
> > >>> Best,
> > >>> Stanislav
> > >>>
> > >>
> >
>
--
Best,
Stanislav
