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