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
