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