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
>
