Subject: [OAUTH-WG] Review of oauth-mtls-07

From: Justin Richer <[email-masked]>
Date: Tue, 20 Mar 2018 17:52:54 +0000

As promised in yesterday’s meeting, here’s my review of the oauth-mtls draft. We’ve recently implemented the spec from the AS and RS side for an as-yet-unreleased version of the Authlete service, and overall it’s in really good shape and very implementable as it stands today. Great work, and usable right now!

Comments, nits, and suggestions as follows:

§Abstract: Single sentence is a bit of a run-on that’s hard to follow. Suggested rewrite:

This document describes OAuth client authentication and sender-constrained tokens using Transport Layer Security (TLS) mutual authentication with X.509 certificates. OAuth clients are provided a mechanism for authentication to the authorization sever using mutual TLS, based on either single certificates or public key infrastructure (PKI). OAuth authorization servers are provided a mechanism for binding access tokens to a client’s mutual TLS certificate, and OAuth protected resources are provided a method for ensuring that such an access token presented to it was issued to the client presenting the token.

§1¶1 (and throughout): The document goes back and forth between “mutual TLS authentication” and “TLS mutual authentication”, one should be picked and used consistently throughout. I realize this is spelled out in 1.2 but it might be worth the effort to use one form most of the time.
§1¶3: maybe don’t call it a “basic bearer token” and instead just a “bearer token” to avoid sounding judgmental	
§2¶1: suggest turning parenthetical into a list: “(regardless of whether the client was dynamically registered, statically configured, or otherwise established)”
§2¶3: It seems this paragraph is trying to leave the door open to other MTLS bound client auth methods, but such methods would require the definition of a different auth method parameter value and a new spec, not really an extension of what’s here. Therefore, suggest changing the end of the paragraph into a single compact sentence:

 The authorization server MUST enforce the
   binding a certificate to a specific client as described in either Section 2.1 <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1> or
   Section 2.2 <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.2> below.

§2.1¶1: It would be helpful to have a pointer on methods of comparing DNs. In our implementation we serialize them to strings using a canonical format (RFC2253) and doing a string comparison based on that. There are probably other ways, but it would be good to help developers avoid doing something naive like comparing two different serializations as strings. 
§2.1¶1: “configured or registered” is an unnecessary distinction, 6749 calls it “registered” regardless of how it got there
§2.1.1¶1: Is it necessary to introduce the registry here instead of just pointing to it? I’m fine with stating that the values are used in both discovery and client registration. 
§2.1.2: I’m only just now seeing the reference to RFC4514 here so this reference needs to be in the parent section as well. I was previously under the impression that no format was prescribed. 
§2.2¶1: Might want to say explicitly in here that the cert is in the JWK for the client (instead of lower down), as it would make the description of the JWKS_URI method make more sense upfront. This could also live in the parent section.
§2.2¶1: "certificate chain is not validated” should probably more explicitly point to the *client’s* certificate not being validated to prevent clients from not validating the *server’s* certificate chain.
§2.2¶1: Extraneous comma: "successfully authenticated, if the subject”
§2.2.1: Same comment as §2.1.1
§3.1¶2: As Brian mentioned in another message, this should specify “no padding”.
§4.1¶1: Probably intend “set up” instead of “setup”
§4.1¶4: “separate host name” should be “separate host name or port”
§4.2¶1: Wording is a bit awkward, suggest:

Since the resource server relies on the authorization server to perform client authentication, there is no need for the resource server to validate
   the trust chain of the client's certificate in any of the methods
   defined in this document.  
§4.3¶1: I get what this section is trying to say but it is confusingly laid out. Might be better to say something like “MTLS client auth and sender-constrained MTLS bound tokens can be used independently of each other”. 
§4.3¶1: This advice doesn’t just apply to public clients, so we probably don’t mean “would not authenticate the client” here but instead “would not authenticate the client using mutual TLS”, since the client could authenticate in other methods. Though it is important to point out that public clients can do this :too:, it’s just as important to allow a client to use private_key_jwt or client_secret_basic and still get a constrained token.
§A¶2: This paragraph reads a bit overly defensive. I understand the need to position the two drafts in relationship to each other, but the tone here could be adjusted significantly without losing the thrust of the main argument.

---

From: Brian Campbell <[email-masked]>
Date: Fri, 23 Mar 2018 16:12:10 +0000

Thanks for the detailed review, Justin. Replies are inline below...

On Tue, Mar 20, 2018 at 5:52 PM, Justin Richer <[email-masked]> wrote:

> As promised in yesterday’s meeting, here’s my review of the oauth-mtls
> draft. We’ve recently implemented the spec from the AS and RS side for an
> as-yet-unreleased version of the Authlete service, and overall it’s in
> really good shape and very implementable as it stands today. Great work,
> and usable right now!
>

That's great to hear! Thanks.

> This document describes OAuth client authentication and sender-constrained
> tokens using Transport Layer Security (TLS) mutual authentication with
> X.509 certificates. OAuth clients are provided a mechanism for
> authentication to the authorization sever using mutual TLS, based on either
> single certificates or public key infrastructure (PKI). OAuth authorization
> servers are provided a mechanism for binding access tokens to a client’s
> mutual TLS certificate, and OAuth protected resources are provided a method
> for ensuring that such an access token presented to it was issued to the
> client presenting the token.
>
>
Yeah, that one sentence in the abstract is maybe more than just a bit of a
run-on. Your rewrite is easier to read.

>
> §1¶1 (and throughout): The document goes back and forth between “mutual
> TLS authentication” and “TLS mutual authentication”, one should be picked
> and used consistently throughout. I realize this is spelled out in 1.2 but
> it might be worth the effort to use one form most of the time.
>

I'll take another pass at this. Prior efforts to reconcile have proven to
be more difficult than one might expect. But I'll try.

> §1¶3: maybe don’t call it a “basic bearer token” and instead just a
> “bearer token” to avoid sounding judgmental
>

Okay.

> §2¶1: suggest turning parenthetical into a list: “(regardless of whether
> the client was dynamically registered, statically configured, or otherwise
> established)”
>

Will do.

> §2¶3: It seems this paragraph is trying to leave the door open to other
> MTLS bound client auth methods, but such methods would require the
> definition of a different auth method parameter value and a new spec, not
> really an extension of what’s here. Therefore, suggest changing the end of
> the paragraph into a single compact sentence:
>
> Yeah, the current language in that paragraph is a bit of a carry over from
previous revisions that didn't yet have the defined auth methods. I think
the text you suggest is better and more clear given the content of the
draft now.

>
> §2.1¶1: It would be helpful to have a pointer on methods of comparing DNs.
> In our implementation we serialize them to strings using a canonical format
> (RFC2253) and doing a string comparison based on that. There are probably
> other ways, but it would be good to help developers avoid doing something
> naive like comparing two different serializations as strings.
>

That's really an implementation detail but I can note that some kind of
normalization is likely needed in comparing DNs.

> §2.1¶1: “configured or registered” is an unnecessary distinction, 6749
> calls it “registered” regardless of how it got there
>

While I suppose that's true about 6749, I think colloquially 'registered'
and 'configured' have come to have more meaning to some/many people about
how the client came to be setup at the AS. So it might be strictly
unnecessary but I'd prefer to keep the "configured or registered" just to
help say that it doesn't matter how the AS came to get the expected DN for
client.

> §2.1.1¶1: Is it necessary to introduce the registry here instead of just
> pointing to it? I’m fine with stating that the values are used in both
> discovery and client registration.
>

I had a hard time describing things concisely here because of the history
of how and when the authentication methods registry came to be, it's name,
and where it's used.  That text in ¶1 is what I was able to come up with
that I thought adequately explained it. It's admittedly not the most
elegant prose ever written but it does convey the info and I'm inclined to
leave it. However, I would be happy to consider alternative text here, if
you've got something specific to propose.

> §2.1.2: I’m only just now seeing the reference to RFC4514 here so this
> reference needs to be in the parent section as well. I was previously under
> the impression that no format was prescribed.
>

§2.1.2 is meant just to prescribe a format for value of the client metadata
parameter. Not necessarily how comparison should done.

> §2.2¶1: Might want to say explicitly in here that the cert is in the JWK
> for the client (instead of lower down), as it would make the description of
> the JWKS_URI method make more sense upfront. This could also live in the
> parent section.
>

Makes sense. I'll add mention of that there.

> §2.2¶1: "certificate chain is not validated” should probably more
> explicitly point to the *client’s* certificate not being validated to
> prevent clients from not validating the *server’s* certificate chain.
>

Yes, good point. It is probably worthwhile to be very explicit about that.

> §2.2¶1: Extraneous comma: "successfully authenticated, if the subject”

Will remove the extraneous comma.

> §2.2.1: Same comment as §2.1.1

Also same.

> §3.1¶2: As Brian mentioned in another message, this should specify “no

Yes, will specify more specifically.

> §4.1¶1: Probably intend “set up” instead of “setup”

Probably, yes.

> §4.1¶4: “separate host name” should be “separate host name or port”

Good point. Will change.

> I'll endeavor to make §4.2¶1 a little less awkward.

> §4.3¶1: I get what this section is trying to say but it is confusingly
> laid out. Might be better to say something like “MTLS client auth and
> sender-constrained MTLS bound tokens can be used independently of each
> other”.
>

Will incorporate something along those lines in place of the current first
sentence.

> §4.3¶1: This advice doesn’t just apply to public clients, so we probably
> don’t mean “would not authenticate the client” here but instead “would not
> authenticate the client using mutual TLS”, since the client could
> authenticate in other methods. Though it is important to point out that
> public clients can do this :too:, it’s just as important to allow a client
> to use private_key_jwt or client_secret_basic and still get a constrained
> token.
>

Makes sense. I'll adjust it accordingly.

§A¶2: This paragraph reads a bit overly defensive. I understand the need to
> position the two drafts in relationship to each other, but the tone here
> could be adjusted significantly without losing the thrust of the main
> argument.
>

The line about Token Binding not having a monopoly on the binding of tokens
is admittedly a bit tongue-in-cheek and also a nod to the point you made
the other day about running out of names.

Honestly though, this text wasn't intended to be defensive and, even when I
read it again, it doesn't come off that way to me. As usual, if you've got
specific text to propose that you think would be better, I'd be happy to
consider it. But I don't feel like the current text is particularly
problematic or in need of change.

-- 
*CONFIDENTIALITY NOTICE: This email may contain confidential and privileged 
material for the sole use of the intended recipient(s). Any review, use, 
distribution or disclosure by others is strictly prohibited.  If you have 
received this communication in error, please notify the sender immediately 
by e-mail and delete the message and any file attachments from your 
computer. Thank you.*

---

From: Justin Richer <[email-masked]>
Date: Wed, 28 Mar 2018 17:57:18 -0400

Thanks for the responses. I’ve cut out places where we seem to agree here and responded to the rest inline below. 

> That's really an implementation detail but I can note that some kind of normalization is likely needed in comparing DNs. 

Might be worth pointing to to RFC4514 in a non-normative example here. The thing is, there are equivalent DNs that aren’t exact string copies of each other. We’ll want to avoid developers either doing a naive string comparison (leading to false negatives) or doing their own home-made regexes (leading to probable breakage and potentially security holes).

> While I suppose that's true about 6749, I think colloquially 'registered' and 'configured' have come to have more meaning to some/many people about how the client came to be setup at the AS. So it might be strictly unnecessary but I'd prefer to keep the "configured or registered" just to help say that it doesn't matter how the AS came to get the expected DN for client.

That’s a fair assessment, and I’m fine with it as-is in that case.

> I had a hard time describing things concisely here because of the history of how and when the authentication methods registry came to be, it's name, and where it's used.  That text in ¶1 is what I was able to come up with that I thought adequately explained it. It's admittedly not the most elegant prose ever written but it does convey the info and I'm inclined to leave it. However, I would be happy to consider alternative text here, if you've got something specific to propose.

I guess I just don’t think all that history is really needed right here. So I’d replace it with:

For the PKI method of mutual TLS client authentication, this specification
   defines and registers the following authentication method metadata
   value into the "OAuth Token Endpoint Authentication Methods" registry
   [IANA.OAuth.Parameters <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#ref-IANA.OAuth.Parameters>].
If you feel it needs a reference, you can potentially put it in intro paragraph of the IANA section that sets the values, maybe? (§6.3)

In the end I’m fine if the text stays — it’s not incorrect, I just feel it’s superfluous. Same comments apply to the other sections so I’m not going to copy them here.

> The line about Token Binding not having a monopoly on the binding of tokens is admittedly a bit tongue-in-cheek and also a nod to the point you made the other day about running out of names. 
> 
> Honestly though, this text wasn't intended to be defensive and, even when I read it again, it doesn't come off that way to me. As usual, if you've got specific text to propose that you think would be better, I'd be happy to consider it. But I don't feel like the current text is particularly problematic or in need of change. 

I took a crack at rewriting the second paragraph (note that I removed the first sentence entirely), but in the end it’s up to you how you want to present the comparison between the documents:

   Token Binding uses bare keys that are generated on the client, which avoids many of
   the difficulties of creating, distributing, and managing the certificates used in this specification.
   However, Token Binding requires support across different portions of the application
   stack, including TLS and browser implementations. At the time of this writing,
   there is relatively little support for it in available application
   development platforms and tooling.  On the other hand, mutual TLS has been around for some time
   and enjoys widespread support in web servers and development
   platforms. As a consequence, Mutual TLS for OAuth 2.0 can be built and deployed now
   using existing platforms and tools. In the future, the two specifications are likely to be
   deployed in parallel for solving similar problems in different environments.

— Justin