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