Hi Michael, Many thanks for reading the document and taking time to provide comments! They are greatly appreciated. I've filed #33 ( https://github.com/mami-project/draft-pauly-transport-security/issues/33) to address them. Please see inline below for responses.
Best, Chris > The main problem that I see is a missing "red thread” - e.g. when deriving the security features in section 4: as a reader, I would expect there to be a 1:1 mapping from the protocol features in section 3 to the security features in section 4, yet there isn’t. For example: sec. 3.10, CurveCP, has, in sec. 3.10.2, protocol features such as “1-RTT session bootstrapping” and “Sender-chosen explicit nonces, …”, and these are not listed in section 4 as such. I guess this is okay because “Protocol features” are internal details, not exposed to an application? But then this should be made clear in section 3, and the security features that we see listed in section 4 should be listed in sec. 3 per protocol with the same wording so that we can see a connection from section 3 to section 4. Excellent point. Tying the two together will go a long way in making selection for mandatory and optional protocol features more clear. > Similarly, what is the rationale for making some features mandatory and some optional in section 4? Reasons for this choice should be explicitly laid out at the beginning of section 4. In general, our rationale for mandatory features is that they were offered by most popular protocols and are "critical" for the security protocol to operate correctly. Optional features are those protocols can work around. > Maybe the reason can be derived from section 5… see below: > Once you’ve laid out the “interface” (I’ll come to that) as you do in section 5, just from looking at this list, it’s easy to see that TLS gives us everything except: > - Authentication Delegation > - Explicit (as opposed to “direct”) pre-shared key import > - Transport mobility > … and none of these are mandatory according to section 4 (so maybe that could be seen as a reason to not make them mandatory?). TLS could be a general recommendation, combined with an explanation that, if you want one of these things listed above, you need to implement a protocol other than TLS, but then you need to be aware of the protocol dependencies that such a protocol choice brings with it. > Generally, as an implementer of a transport system, this document now tells me quite clearly which features I must offer (and it tells me which protocols I then must support). However, the trade-offs are not so clear. I see trade-offs in the protocol dependencies that you list in section 3; then there are Transport Dependencies listed in section 4, but only for the optional features, and it says “None” almost everywhere… yet, in section 3, there are tons of “Protocol Dependencies”, so I find this surprising. Indeed. These sections fell out of sync. I'll fix that. > Another missing “red thread”: why do the transport features have different names in sections 4 and 5? I would expect to find “Transport mobility” in section 4, but there it’s “Connection mobility”. Similarly, I’d expect to find “Session cache management” (from sec. 5.1) in section 4, but there it’s called “Session caching and management”. I think the whole document would look much more uniform and better structured with some clearer mappings - from section 3 to the listed security features in section 4 and the “interfaces” in section 5. This is a simple oversight. In addressing your first comment, I'll make we unify the terminology too. > This brings me to the next issue: I think there’s a terminology problem with this document. The terminology section, in line with all other TAPS documents, mentions “confidentiality” as an example of a “Transport Feature”. Further down, “Security Feature” is introduced. Given the description of Transport Feature, a Security Feature is a special case of a Transport Feature. Maybe the first sentence of the Security Feature definition could be changed to say “A Transport Feature that a network security layer provides to applications.” Section 3 then talks about “Protocol features”; these are maybe not offered to applications, so I think it’s fine to use this term, but you should also define it in the Terminology section. Then, Section 5 talks about Transport Security Protocol Interfaces - but that term isn’t defined anywhere. I think these “interfaces” are really Security Features, no? Not quite. They're interfaces to the security protocols. Those interfaces may of course drive or control certain features. > Actually, given that they seem to be the same but with slightly different wording, I wonder if sections 4 and 5 could be merged … or maybe they should be switched in order: section 5 should appear first, calling them Security Features, and then what is now Section 4 could come after, deriving which ones of them are mandatory and optional, as well as list transport / application dependencies. Interesting suggestion. I'll see if that can be done. > Some smaller things / nits: > I find it strange that this draft cites only RFC 8095 but no other TAPS documents. > In particular, because it complements draft-ietf-taps-minset in the process of TAPS, I think it should reference back to it. > (The minset draft says that it doesn’t discuss security because this is covered in draft-ietf-taps-transport-security, but then draft-ietf-taps-transport-security only says that it complements RFC 8095 - this gives readers following the link from minset a confusing context). Good point. I'll add the citation. > Section 3.2.3: Path MTU Discovery surprised me here. What does that mean? That DTLS relies on an application implementing PMTUD (as it’s over UDP), and then telling DTLS about it? Doesn’t DTLS do this on its own? Why not? Sorry for asking a DTLS question, surely it’s me… but just listing PMTUD as a dependency isn’t a good enough explanation, I think. Clients have to set the maximum DTLS fragment (record) size, as each record must fit within a single datagram. > sec. 3.3: when I first read “A mapping for QUIC over TLS 1.3 has been specified to provide this handshake”, I was surprised. I mean, given that TLS runs over TCP, this sentence sounds like something really doesn’t match here. Only in sec. 3.3.3 you explain “TLS within QUIC relies on a reliable stream abstraction for its handshake”. Well that’s what I thought, but then this should probably be brought earlier - “mapping for QUIC over TLS 1.3” just sounds like QUIC uses TLS, which uses TCP, which is of course wrong. Maybe it’s just the word “over” - using “with” instead would already make this clearer. Agreed. "Over" is not the correct word here. I'll fix that. > sec. 3 sentence 1: > “…security protocols that _are_ currently used to …” > sec 3.3.1 1st sentence after bullet list: > “The QUIC transport layer support_S_ multiple streams…” > Same para, 3rd sentence: “The TLS handshake, along with further records, are sent over this stream.” - did you mean to say “The TLS handshake messages” ? > sec 3.4, line 2: > “either as for creating tunnels (tunnel-mode)” - I think the “as” should be removed here? > penultimate sentence of sec. 3.5.1: > “…DTLS-offered certificates match that which are offered over SIP” - is “that” correct here? (should it be “the ones” or something like that?) It should, and Colin refactored that section since your email to fix it. > 3.10.1 par 5: > This begins with “Unlike TCP, …” - isn’t this strange to say, for being too obvious? Or did you mean to say tcpcrypt? > Well actually even TCP has cookies for some form of “source validation”, with TFO. Of course that’s a limited use, but … maybe simply removing “Unlike TCP” here would make this clearer. Good point! I'll fix that, or add a blurb about excluding TFO. > 4.2, “Mutual authentication” and “Source validation”: the sentence describing these features says “must”, yet it’s in the section of optional features. While this doesn’t really matter, it makes for a slightly surprising read, and would IMO be better as a factual statement - e.g. “Transport security protocols allow each endpoint to authenticate the other …” and “Source validation is provided to mitigate …” +1 > Sec 5.1.: missing line break after: > - Session Cache Management > - Connection Termination > - Pre-Shared Key Export Thanks! Will fix. _______________________________________________ Taps mailing list Taps@ietf.org https://www.ietf.org/mailman/listinfo/taps
