Re: [nfsv4] AD review of draft-ietf-nfsv4-rpc-tls

[Chuck Lever <[e-mail]>]

> On Apr 22, 2020, at 11:57 AM, Magnus Westerlund <[e-mail]> wrote:
> 
> Hi,
> 
> I have reviewed the diff for the editor's copy and have the below comments. 
> Only E I think needs any more significant consideration. My evaluation is that 
> all my previous AD comments has been resolved or answered. Thanks for the 
> work.
> 
> A. Section 4.1:
> 
> RPC operation can continue, but in the clear.
> 
> I would propose that this sentence is reformulated to make it even more 
> evident that there are no security protection.
> 
> NEW:
> 
> RPC operation can continue, however it will be without any confidentiality, 
> integrity or authentication protection from (D)TLS.
> 
> B. Section 4.2.1:
> 
> RPCSEC GSS can also protect per-request integrity or confidentiality.
> 
> I think the language is botched here. I guess something like this was 
> intended:
> 
> NEW: RPCSEC GSS can also perform per-request integrity or confidentiality 
> protection.
> 
> C. Section 5.
> 
> Implementations MUST NOT negotiate TLS versions prior to v1.3 ([RFC8446] or 
> [I-D.ietf-tls-dtls13]).
> 
> I think it need to be more explicit about that the second reference is DTLS 
> 1.3. Propose
> 
> Implementations MUST NOT negotiate TLS versions prior to v1.3 (for TLS 
> [RFC8446] or DTLS [I-D.ietf-tls-dtls13] respectively).
> 
> D. Section 5.:
> 
> Client implementations MUST include the 
> "application_layer_protocol_negotiation(16)" extension in their "ClientHello" 
> message and MUST include the protocol identifier defined in Section 8.2 in 
> that message's ProtocolNameList value.
> 
> I propose to include an reference here to what the 
> "application_layer_protocol_negotiation(16)" is. So proposed wording:
> 
> Client implementations MUST include the 
> "application_layer_protocol_negotiation(16)" extension [RFC7301] in their 
> "ClientHello" message and MUST include the protocol identifier defined in 
> Section 8.2 in that message's ProtocolNameList value.
> 
> Likely it makes sense to make a similar change to the next sentence to:
> 
> Similary, in response to the "ClientHello" message, server implementations 
> MUST include the "application_layer_protocol_negotiation(16)" extension 
> [RFC7301] in their "ServerHello" message and MUST include only the protocol 
> identifier defined in Section 8.2 in that message's ProtocolNameList value.

Your proposed changes look sensible to me. I'll see to applying them after
today's meeting.


> E. Section 5.1.2:
> 
> When using connectionless operation, each DTLS session endpoint is identified 
> by its 5-tuple -- the source and destination IP address and port numbers, 
> along with the UDP transport protocol number (17). When using connected 
> operation, the DTLS session endpoints are identified by connection ID (CID), 
> as described in [I-D.ietf-tls-dtls-connection-id]. Connected operation is 
> strongly RECOMMENDED.
> 
> So I don't know if I looked to quickly at DTLS 1.3 and the dtls-connection-id 
> draft. However, I don't see any discussion of a connectionless operation. This 
> might be a terminology issue primarily. However, my understanding which might 
> be flawed are that the DTLS connection is created by the DTLS handshake. That 
> DTLS connection can either be identified by the 5-tuple or use this CID 
> extension.
> 
> The primary goal of the CID extension is to avoid need for a new DTLS 
> handshake and creating a new DTLS connection if the 5-tuple changes, for 
> example due to NAT port rebinding.
> 
> In addition my understanding is that [I-D.ietf-tls-dtls-connection-id] is 
> defining the extension for CID for DTLS 1.2. Section 9 in the DTLS1.3 
> specification imports that extension into DTSL 1.3. Thus, if you want to 
> recommend its usage it might be better to point to Section 9 and only use 
> [I-D.ietf-tls-dtls-connection-id] informationally.

Yes, I noticed that connection-id refers specifically to DTLS 1.2, but
assumed (erroneously) that it would also apply to DTLS 1.3. I didn't
think to look that the CID extension was already integrated into tls-dtls13.

I'll check into this further. It might be fine to simply drop the reference
to connection-id, replacing it as you suggest above.


--
Chuck Lever