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
