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 > > > > > > >> > >>>>>>>>>> to > > > > > > >> > >>>>>>>>>>> the > > > > > > >> > >>>>>>>>>>>>>>>> subject's public or private credentials > when > > > the > > > > > > commit > > > > > > >> > >>>>>>>>>> succeeds; > > > > > > >> > >>>>>>>>>>>>>> then I > > > > > > >> > >>>>>>>>>>>>>>>> think the sasl.client.callback.handler. > class > > > > would > > > > > > >> > >>>>> have > > > > > > >> > >>>>>> to > > > > > > >> > >>>>>>> be > > > > > > >> > >>>>>>>>>>>>>> explicitly > > > > > > >> > >>>>>>>>>>>>>>>> set to a class that extends the default > > > > > > implementation > > > > > > >> > >>>>>>>>>>>>>>>> (OAuthBearerSaslClientCallbackHandler) and > > > > > retrieves > > > > > > >> > >>>>> the > > > > > > >> > >>>>>>> map > > > > > > >> > >>>>>>>>>> when > > > > > > >> > >>>>>>>>>>>>>>> handling > > > > > > >> > >>>>>>>>>>>>>>>> the SaslExtensionsCallback. But maybe you > > are > > > > > > thinking > > > > > > >> > >>>>>>> about > > > > > > >> > >>>>>>>>>> it > > > > > > >> > >>>>>>>>>>>>>>>> differently? Some guidance on how to > > actually > > > > take > > > > > > >> > >>>>>>> advantage > > > > > > >> > >>>>>>>>>> of > > > > > > >> > >>>>>>>>>>> the > > > > > > >> > >>>>>>>>>>>>>>>> feature via an implementation would be a > > useful > > > > > > >> > >>>>> addition > > > > > > >> > >>>>>> to > > > > > > >> > >>>>>>>> the > > > > > > >> > >>>>>>>>>>> KIP. > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> Finally, I note that the extension parsing > > does > > > > not > > > > > > >> > >>>>>>> support a > > > > > > >> > >>>>>>>>>>> comma > > > > > > >> > >>>>>>>>>>>> in > > > > > > >> > >>>>>>>>>>>>>>> keys > > > > > > >> > >>>>>>>>>>>>>>>> or values. This should be addressed > somehow > > -- > > > > > > either > > > > > > >> > >>>>> by > > > > > > >> > >>>>>>>>>>> supporting > > > > > > >> > >>>>>>>>>>>>>> via > > > > > > >> > >>>>>>>>>>>>>>> an > > > > > > >> > >>>>>>>>>>>>>>>> escaping mechanism or by explicitly > > > acknowledging > > > > > > that > > > > > > >> > >>>>> it > > > > > > >> > >>>>>>> is > > > > > > >> > >>>>>>>>>>>>>> unsupported. > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> Thanks for the KIP and the simultaneous PR > -- > > > > > having > > > > > > >> > >>>>> both > > > > > > >> > >>>>>>> at > > > > > > >> > >>>>>>>>>> the > > > > > > >> > >>>>>>>>>>>> same > > > > > > >> > >>>>>>>>>>>>>>> time > > > > > > >> > >>>>>>>>>>>>>>>> really helped. > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> Ron > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> On Tue, Jul 17, 2018 at 6:22 PM Stanislav > > > > > Kozlovski < > > > > > > >> > >>>>>>>>>>>>>>>> stanis...@confluent.io> > > > > > > >> > >>>>>>>>>>>>>>>> wrote: > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>>> Hey group, > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>>> I just created a new KIP about adding > > > > customizable > > > > > > >> > >>>>> SASL > > > > > > >> > >>>>>>>>>>> extensions > > > > > > >> > >>>>>>>>>>>>>> to > > > > > > >> > >>>>>>>>>>>>>>> the > > > > > > >> > >>>>>>>>>>>>>>>>> OAuthBearer authentication mechanism. More > > > > details > > > > > > in > > > > > > >> > >>>>>> the > > > > > > >> > >>>>>>>>>>> proposal > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>>> KIP: > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > >> > >>>>> 342% > > > > > > >> > >>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>> > > > > > > >> > >>>>>>>> 3A+Add+support+for+Custom+SASL+extensions+in+ > > > > > > >> > >>>>> OAuthBearer+authentication > > > > > > >> > >>>>>>>>>>>>>>>>> JIRA: KAFKA-7169 < > > > > > > >> > >>>>>>>>>>>> https://issues.apache.org/ > jira/browse/KAFKA-7169 > > > > > > > > > >> > >>>>>>>>>>>>>>>>> PR: Pull request < > > > > > > >> > >>>>>>>> https://github.com/apache/kafka/pull/5379> > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>>> -- > > > > > > >> > >>>>>>>>>>>>>>>>> Best, > > > > > > >> > >>>>>>>>>>>>>>>>> Stanislav > > > > > > >> > >>>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>>> -- > > > > > > >> > >>>>>>>>>>>>>>> Best, > > > > > > >> > >>>>>>>>>>>>>>> Stanislav > > > > > > >> > >>>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>>> > > > > > > >> > >>>>>>>>>>> > > > > > > >> > >>>>>>>>>>> > > > > > > >> > >>>>>>>>>>> -- > > > > > > >> > >>>>>>>>>>> Best, > > > > > > >> > >>>>>>>>>>> Stanislav > > > > > > >> > >>>>>>>>>>> > > > > > > >> > >>>>>>>>>> > > > > > > >> > >>>>>>>>> > > > > > > >> > >>>>>>>>> > > > > > > >> > >>>>>>>>> -- > > > > > > >> > >>>>>>>>> Best, > > > > > > >> > >>>>>>>>> Stanislav > > > > > > >> > >>>>>>>>> > > > > > > >> > >>>>>>>> > > > > > > >> > >>>>>>>> > > > > > > >> > >>>>>>>> -- > > > > > > >> > >>>>>>>> Best, > > > > > > >> > >>>>>>>> Stanislav > > > > > > >> > >>>>>>>> > > > > > > >> > >>>>>>> > > > > > > >> > >>>>>> > > > > > > >> > >>>>>> > > > > > > >> > >>>>>> -- > > > > > > >> > >>>>>> Best, > > > > > > >> > >>>>>> Stanislav > > > > > > >> > >>>>>> > > > > > > >> > >>>>> > > > > > > >> > >>> > > > > > > >> > >> > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > -- > > > > > > >> > > Best, > > > > > > >> > > Stanislav > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best, > > > > > Stanislav > > > > > > > > > > > > > > > > > > -- > > > Best, > > > Stanislav > > > > > > > > -- > Best, > Stanislav >
