Last Call Review of draft-ietf-tls-exported-authenticator-09
review-ietf-tls-exported-authenticator-09-secdir-lc-sheffer-2019-07-16-00

Request Review of draft-ietf-tls-exported-authenticator
Requested rev. no specific revision (document currently at 09)
Type Last Call Review
Team Security Area Directorate (secdir)
Deadline 2019-07-16
Requested 2019-07-02
Draft last updated 2019-07-16
Completed reviews Genart Last Call review of -09 by Christer Holmberg
Secdir Last Call review of -09 by Yaron Sheffer
Assignment Reviewer Yaron Sheffer
State Completed
Review review-ietf-tls-exported-authenticator-09-secdir-lc-sheffer-2019-07-16
Posted at https://mailarchive.ietf.org/arch/msg/secdir/u732WnIG2NbNnjpPOgriMN3UhdY
Reviewed rev. 09
Review result Has Issues
Review completed: 2019-07-16

Review
review-ietf-tls-exported-authenticator-09-secdir-lc-sheffer-2019-07-16

The document is generally clear in both its motivation and the proposed solution.

I think it's playing a bit too liberal with TLS 1.3, in sort-of but not quite deprecating renegotiation, and in changing the IANA registry in a way that actually changes the protocol. Details below.

Other comments:

* Abstract: please reword to make it clear that it's the proof (not the cert) that is portable.
* Introduction: TLS 1.3 post-handshake auth "has the disadvantage of requiring additional state to be stored as part of the TLS state machine." Why is that an issue is practice, assuming this feature is already supported by TLS libraries? Also, are we in effect deprecating this TLS 1.3 feature because of the security issue of the mismatched record boundaries?
* "For simplicity, the mechanisms... require", maybe a normative REQUIRE?
* Authenticator Request: please clarify that we use the TLS 1.3 format even when running on TLS 1.2.
* Also, I suggest to change "is not encrypted with a handshake key" which is too specific to "is sent unencrypted within the normal TLS-protected data".
* Before diving into the detailed messages in Sec. 3, it would be nice to include a quick overview of the message sequence. 
* Sec. 3: "SHOULD use TLS", change to MUST. There's no way it can work otherwise anyway.
* "This message reuses the structure to the CertificateRequest message" - repeats the preceding paragraph.
* Sec. 4: again, SHOULD use TLS -> MUST.
* Is there a requirement for all protocol messages to be sent over a single TLS connection? If so, please mention it. Certainly this is true for the Authenticator message that must be sent over a single connection and cannot be multiplexed by the high-level protocol. I think this protocol actually assumes "nice" transport properties (no message reorder, reliability) that also require a single, clean TLS connection.
* Why no authenticator request for servers? Don't we care about replay? OTOH if the client wants to detect replays, it would need to keep state on received authenticators, which would be very messy.
* 4.1: when referencing Exporters, mention this is Sec. 7.5 of [TLS1.3].
* The discussion of Extended Master Secret only applies to TLS 1.2 - please clarify that, plus I suggest to reword this paragraph which I find hard to parse.
* 4.2: "the extensions are chosen from the TLS handshake." - What does it mean exactly, and how does an application even know which extensions were used at the TLS layer? Reading further, we mention "ClientHello extensions." Maybe the authenticator should also include a ClientHello message to indicate supported extensions. Assuming we can peek into ClientHello contradicts the hopeful "even if it is possible to implement it at the application layer" in Sec. 6.
* 4.2.1: the first sentence of the section is incomplete.
* Can I use TLS 1.3-specific extensions (oid_filters) if I'm running on a TLS 1.2 connection? Is there a situation where a certificate is acceptable for TLS 1.3 connections but not for TLS 1.2 connections?
* "using a value derived from the connection secrets" - I think we should recommend a specific construction here to ensure people do it securely.
* 4.2.3: Are we computing HMAC(MAC-key, Hash(a || b || c || d))? If so, please say so. 
* 4.2.4: "a constant-time comparison" - why is it actually required, what is the threat? An attacker can do very little because each authenticator being sent is different.
* 5: please say explicitly which messages are used in this case to construct the authenticator.
* 6.1: the "MUST" is strange in a section that's only supposed to be informative. Also, the library may provide this extension (and possibly others) without requiring it as input.
* 6.4: "The API MUST return a failure if the certificate_request_context of the authenticator was used in a previously validated authenticator." Are we requiring the library to keep previous contexts for replay protection? If so, please make it explicit.
*  7.1: this is changing TLS 1.3 by allowing this extension in Cert Request! I think it's a really bad idea. Better to hack special support to existing libraries, allowing this specific extension for this special case, than to change the base protocol. Otherwise, this document should UPDATE 8446.
* 8: I think the Security Considerations is the right place to talk about interaction between this protocol and TLS 1.3 (or 1.2) renegotiation. Even if we simply RECOMMEND not to do it. Or else explain the use cases where renegotiation is still useful if there are any.