Hi Brian,

Thanks for the detailed responses. Comments in line below (marked with ***).


> On Wednesday, Apr 11, 2018 at 9:47 pm, Brian Campbell 
> <bcampb...@pingidentity.com (mailto:bcampb...@pingidentity.com)> wrote:
> Thanks for the review and feedback, Neil. I apologize for my being slow to 
> respond. As I said to Justin recently 
> (https://mailarchive.ietf.org/arch/msg/oauth/cNmk8fSuxp37L-z8Rvr6_EnyCug), 
> I've been away from things for a while. Also there's a lot here to get 
> through so took me some time.
> It looks like John touched on some of your comments but not all. I'll try and 
> reply to them as best I can inline below.
> On Thu, Mar 29, 2018 at 9:18 AM, Neil Madden <neil.mad...@forgerock.com 
> (mailto:neil.mad...@forgerock.com)> wrote:
> > Hi,
> >
> > I have reviewed this draft and have a number of comments, below. ForgeRock 
> > have not yet implemented this draft, but there is interest in implementing 
> > it at some point. (Disclaimer: We have no firm commitments on this at the 
> > moment, I do not speak for ForgeRock, etc).
> >
> > 1. https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3.1 defines 
> > a new confirmation method “x5t#S256”. However, there is already a 
> > confirmation method “jwk” that can contain a JSON Web Key, which itself can 
> > contain a “x5t#S526” claim with exactly the same syntax and semantics. The 
> > draft proposes:
> >
> > { “cnf”: { “x5t#S256”: “…” } }
> >
> > but you can already do:
> >
> > { “cnf”: { “jwk”: { … , “x5t#S256”: “…” } } }
> >
> > If the intent is just to save some space and avoid the mandatory fields of 
> > the existing JWK types, maybe this would be better addressed by defining a 
> > new JWK type which only has a thumbprint? e.g., { “kty”: “x5t”, “x5t#S256”: 
> > “…” }.
> The intent of the x5t#S256 confirmation method was to be space efficient and 
> straightforward while utilizing the framework and registry that RFC 7800 
> gives. Even a new JWK type like that would still use more space.. And I'd 
> argue that the new confirmation method is considerably more straightforward 
> than registering a new JWK type (and the implications that would have on JWK 
> implementations in general) in order to use the existing "jwk" confirmation 
> method.
> *** OK, that is reasonable. Given that the draft says SHOULD rather than MUST 
> for using this confirmation key method, I think it is currently allowed to 
> use either representation.
> >
> > 2. I find the naming “mutual TLS” and “mTLS” a bit of a misnomer: it’s 
> > really only the client authentication that we are interested here, and the 
> > fact that the server also authenticates with a certificate is not hugely 
> > relevant to this particular spec (although it is to the overall security of 
> > OAuth). Also, TLS defines non-certificate based authentication mechanisms 
> > (e.g. TLS-SRP extension for password authenticated key exchange, PSK for 
> > pre-shared key authentication) and even non-X.509 certificate types 
> > (https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#tls-extensiontype-values-3).
> >  I’d prefer that the draft explicitly referred to “X.509 Client Certificate 
> > Authentication” rather than mutual TLS, and changed identifiers like 
> > ‘tls_client_auth’ 
> > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1.1) to 
> > something more explicit like ‘tls_x509_pki_client_auth’.
> >
> > This is especially confusing in section 3 on sender constrained access 
> > tokens, as there are two different servers involved: the AS and the 
> > protected resource server, but there is no “mutual” authentication between 
> > them, only between each of them and the client.
> Choosing names and terminology is difficult and the "right" wording is often 
> subjective. I believe that the current wording sufficiently conveys what is 
> going on in the draft to most readers. Most readers thus far seem to agree. 
> There is some text now that does say that the mutual auth in the draft is in 
> fact X.509 client cert authn but, in the next revision, I'll look for other 
> opportunities where it could be stated more clearly.
> *** Thanks.
> >
> > 3. The draft links to the TLS 1.2 RFC, while the original OAuth 2.0 RFC 
> > only specifies TLS 1.0. Is the intention that TLS 1.2+ is required? The 
> > wording in Section 5.1 doesn’t seem clear if this could also be used with 
> > TLS 1.0 or 1.1, or whether it is only referring to future TLS versions.
> The reference to BCP 195 (which unfortunately the original OAuth 2.0 RFC 
> doesn't have because it didn't exist then) is meant to account for changing 
> versions and recommendations around TLS. Currently that BCP says TLS 1.2 is a 
> must and suggests against 1.1 & 1.0 but doesn't outright prohibit them.
> *** OK, that seems good to me.
> >
> > 4. It might be useful to have a discussion for implementors of whether TLS 
> > session resumption (and PSK in TLS 1.3) and/or renegotiation impact the use 
> > of client certificates, if at all?
> That might well be useful but I don't myself know what it would say. I've 
> (maybe naively) figured those are deployment details that will just work out. 
> Perhaps you could propose some text around such a discussion that the WG 
> could consider?
> ***
> To be honest, when I raised this it was because I didn’t really know what the 
> implications were. I’ve done some reading around and I think it should all 
> just work - both session resumption and PSK-based resumption preserve the 
> original client and server authentication context so we can assume that any 
> presented client cert is still valid for the resumed session. So I think we 
> can leave out any discussion of this and assume it works as expected.
> >
> > 5. Section 3 defines sender-constrained access tokens in terms of the 
> > confirmation key claims (e.g., RFC 7800 for JWT). However, the OAuth 2..0 
> > Pop Architecture draft defines sender constraint and key confirmation as 
> > different things 
> > (https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-6.2).
> >  The draft should decide which of those it is implementing and if sender 
> > constraint is intended, then reusing the confirmation key claims seems 
> > misleading. (I think this mTLS draft is doing key confirmation so should 
> > drop the language about sender constrained tokens).
> I will say again that choosing names and terminology is difficult...
> Although I must admit that I started using "sender constrained" somewhat 
> indiscriminately at first and it's just sort of stuck. At the time I was 
> trying to incorporate bits of draft-sakimura-oauth-jpop in a way that might 
> help bring on and keep the authors of that draft onboard with this mtls 
> draft. In retrospect it looks like I did that part wrong anyway. But that was 
> the thinking at the time and the history, for whatever it's worth. More 
> recently, Nat was requesting that "sender constrained" be included in the 
> title. So there's that too.
> In defense of my mistake, however, if there's a line between "sender 
> constrained" and "key confirmation" tokens, it's a bit of a fuzzy line. And 
> I'd argue that what this draft is doing pretty close to the line..
> But ultimately I think you are right that what this mtls draft is doing with 
> access tokens is more accurately described with the key confirmation term.
> So, yes, the draft should probably drop (or at least minimize) the language 
> about sender constrained. I'll do that in the next draft version, barring big 
> objections from the WG.
> The tricky thing with making that change is that there a client and server 
> metadata parameters with the name 
> "mutual_tls_sender_constrained_access_tokens" that should probably also 
> change. But that would be a breaking change (of sorts anyway), which 
> shouldn't be taken lightly at this stage. I feel that some explicit okays 
> from the WG are needed before doing so (rough consensus stye) . Any WG 
> members want to weigh in here and help get a "sense of the group" concerning 
> changing those metadata names?
> *** Thanks. I agree this might cause compatibility issues. It is not a big 
> issue for us, but might cause some confusion.
> >
> > 6. The OAuth 2.0 PoP Architecture draft says 
> > (https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-5):
> >
> > Strong, fresh session keys:
> >
> > Session keys MUST be strong and fresh. Each session deserves an
> > independent session key, i.e., one that is generated specifically
> > for the intended use. In context of OAuth this means that keying
> > material is created in such a way that can only be used by the
> > combination of a client instance, protected resource, and
> > authorization scope.
> >
> >
> > However, the mTLS draft section 3 
> > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3) says:
> >
> > The client makes protected resource requests as described in
> > [RFC6750], however, those requests MUST be made over a mutually
> > authenticated TLS connection using the same certificate that was used
> > for mutual TLS at the token endpoint.
> >
> > These two statements are contradictory: the OAuth 2.0 PoP architecture 
> > effectively requires a fresh key-pair to be used for every access token 
> > request, whereas this draft proposes reusing the same long-lived client 
> > certificate for every single access token and every resource server.
> >
> > In the self-signed case (and even in the CA case, with a bit of work - 
> > e.g., https://www.vaultproject.io/docs/secrets/pki/index.html) it is 
> > perfectly possible for the client to generate a fresh key-pair for each 
> > access token and include the certificate on the token request (e.g., as per 
> > https://tools.ietf.org/html/draft-ietf-oauth-pop-key-distribution-03#section-5.1
> >  - in which case an appropriate “alg” value should probably be described). 
> > This should probably at least be an option.
> This draft doesn't necessarily seek to align with the (long expired) PoP 
> architecture draft. Rather it is aiming to provide a pragmatic solution for 
> PoP style access tokens and OAuth client auth using mTLS client certificates.
> That said, with the current draft, it's certainly possible for a client to 
> update its cert more frequently, even so far as using a new one for each 
> access token. The details of how that would work will vary some based on the 
> token endpoint authentication method. But it's not precluded.
> *** You are right, the text doesn’t preclude that. I am happy with that 
> solution. I suspect most people will deploy and be happy with reusing a 
> long-lived cert for every access token, so this may not matter in practice.
> >
> > 7. The use of a single client certificate with every resource server (RS) 
> > should be called out in a Privacy Considerations section, as it allows 
> > correlation of activity.
> Practically speaking the access tokens being presented likely already have 
> correlatable info in them about the client as well as the user. I don't know 
> that there's much of a (new) concern in reality. If you feel this concern is 
> unique and import enough though, perhaps there's some text you'd like to 
> propose for a Privacy Considerations section that the WG could consider? I 
> mean, I guess it doesn't hurt to mention it but I would like to avoid 
> overstating the issue.
> *** On reflection, I am going to withdraw this comment. As you say there are 
> other ways to correlate clients. The privacy issue would mainly arise in the 
> context of dynamic client registration e.g., a pattern I’ve seen described 
> where every instance of a mobile app does dynamic client registration to 
> avoid including credentials directly in the app bundle. This would make 
> clients one-to-one with users. But (a) those apps are fairly unlikely to be 
> using TLS certs, and (b) that is more of a privacy consideration for dynamic 
> client registration rather than this draft.
> >
> > 8. This is maybe a more general point, but RFC 6750 defines the 
> > Authorization: Bearer scheme 
> > (https://tools.ietf.org/html/rfc6750#section-2) for a client to communicate 
> > it’s access token to the RS in a standard way. As sender-constrained access 
> > tokens are not strictly bearer tokens any more, should this draft also 
> > register a new scheme for that? Should there be a generic PoP scheme?
> The thinking and general consensus (in this draft as well as the OAuth token 
> binding work) has been that continuing to use the RFC 6750 scheme while 
> putting the "not strictly bearer" stuff in (or referenced by) the token 
> itself will be easier on deployment and implementation. And better for 
> adoption as a result. I believe some early implementation work has borne that 
> out too.
> *** OK.
> >
> > 9. The Security Considerations should really make some mention of the long 
> > history of attacks against X.509 certificate chain validation, e.g. failure 
> > to check the “CA” bit in the basic constraints, errors in parsing DNs, etc. 
> > It should be strongly suggested to use an existing TLS library to perform 
> > these checks rather than implementing your own checks. This relates to 
> > Justin’s comments around DN parsing and normalisation.
> Suggesting to use an existing TLS library is certainly sound advice and I 
> sort of felt is implied in the draft. But saying so more strongly/explicitly 
> might be worthwhile. And pointing to historical reasons to do so would 
> probably be good too. Could you propose a new Security Considerations section 
> or maybe augmentation of §5.2 with that content?
> *** I’ll try and come up with some text.
> >
> > 10. The PKI client authentication method 
> > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1) makes no 
> > mention at all of certificate revocation and how to handle checking for 
> > that (CRLs, OCSP - with stapling?). Neither does the Security 
> > Considerations. If this is a detail to be agreed between then AS and the CA 
> > (or just left up to the AS TLS stack) then that should perhaps be made 
> > explicit. Again, there are privacy considerations with some of these 
> > mechanisms, as OCSP requests are typically sent in the clear (plain HTTP) 
> > and so allow an observer to see which clients are connecting to which AS.
> I didn't think that a TLS client could do OCSP stapling?
> *** I think you are right about this. I always assumed it was symmetric (and 
> I think it technically could work), but the spec only talks about stapling in 
> the server-side of the handshake.
> That aside, revocation checking (how and even if) is something that's at the 
> discretion of the AS. I can add something in §2.1 to say that more explicitly.
> *** Thanks.
> >
> > 11. The same comment applies to how the protected resource checks for 
> > revocation of the certificate presented during sender constrained access 
> > token usage. Should the RS make its own revocation checks based on the 
> > information in the certificate presented, or should it trust the 
> > certificate while the access token is still valid? If the latter case, is 
> > the AS responsible for revoking any access tokens whose certificate have 
> > been revoked (if so, should it be doing an OCSP call on every token 
> > introspection request, and should the RS be passing on the 
> > certificate/serial number on that request)? If the Client request uses OCSP 
> > Stapling (https://en..wikipedia.org/wiki/OCSP_stapling) how can the RS 
> > verify the signature on that if it does not have a separate trust 
> > relationship with the CA already?
> The draft effectively uses cert mtls at the RS as a proof-of-possession 
> method only and not as authentication. So revocation checking isn't really 
> applicable. In specific deployment situations, I suppose an RS could check 
> revocation. But that'd be a deployment decision for the RS that's beyond the 
> scope of this draft.
> *** OK, that is an interesting observation. If either the client or AS 
> suspect the key has been compromised they can revoke the access token(s) 
> instead.
> >
> > 12. The use of only SHA-256 fingerprints means that the security strength 
> > of the sender-constrained access tokens is limited by the collision 
> > resistance of SHA-256 - roughly “128-bit security" - without a new 
> > specification for a new thumbprint algorithm. An implication of this is 
> > that is is fairly pointless for the protected resource TLS stack to ever 
> > negotiate cipher suites/keys with a higher level of security. In more 
> > crystal ball territory, if a practical quantum computer becomes a 
> > possibility within the lifetime of this spec, then the expected collision 
> > resistance of SHA-256 would drop quadratically, allowing an attacker to 
> > find a colliding certificate in ~2^64 effort. If we are going to pick just 
> > one thumbprint hash algorithm, I would prefer we pick SHA-512.
> The idea behind haveing just one thumbprint hash algorithm was to keep things 
> simple. And SHA-256 seems good enough for the reasonably foreseeable future 
> (and space aware). Also a new little spec to register a different hash 
> algorithm, should the need arise, didn't seem particularity onerous.
> That was the thinking anyway. Maybe it is too short sighted though?
> I do think SHA-256 should stay regardless.
> But the draft could also define SHA-512 (and maybe others). What do you and 
> WG folks think about that?
> *** Yes please.
> It would probably then be useful for the metadata in §3.3 and §3.4 to change 
> from just boolean values to something to convey what hash alg/cnf method the 
> client expects and the list of what the server supports.. That's maybe 
> something that should be done anyway. That'd be a breaking change to the 
> metadata. But there's already another potential breaking change identified 
> earlier in this message. So maybe it's worth doing...
> How do folks feel about making this kind of change?
> > Cheers,
> >
> > Neil
> >
> >
> > > On 19 Mar 2018, at 22:34, Rifaat Shekh-Yusef <rifaat.i...@gmail.com 
> > > (mailto:rifaat.i...@gmail.com)> wrote:
> > >
> > > All,
> > >
> > > As discussed during the meeting today, we are starting a WGLC on the MTLS 
> > > document:
> > > https://tools.ietf.org/html/draft-ietf-oauth-mtls-07
> > >
> > > Please, review the document and provide feedback on any issues you see 
> > > with the document.
> > >
> > > The WGLC will end in two weeks, on April 2, 2018.
> > >
> > > Regards,
> > > Rifaat and Hannes
> > >
> > > _______________________________________________
> > > OAuth mailing list
> > > OAuth@ietf.org (mailto:OAuth@ietf.org)
> > > https://www.ietf.org/mailman/listinfo/oauth
> >
> >
> > _______________________________________________
> > OAuth mailing list
> > OAuth@ietf.org (mailto:OAuth@ietf.org)
> > https://www.ietf.org/mailman/listinfo/oauth
> >
> 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.

Attachment: signature.asc
Description: PGP signature

OAuth mailing list

Reply via email to