Hi Brian, Thanks for the detailed responses. Comments in line below (marked with ***).
Neil > 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.
signature.asc
Description: PGP signature
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
