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.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 > OAuthBearerSaslClientCallbackH > > > > > andler > > > > > > > > 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 > > > > > > > > >> > >>>>>>>
