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 > > >
