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 > > > > > > > >> > >>>>>>>>>>>>>> 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 > > > > > > > >> > > -- Best, Stanislav
