Subject: [Gen-art] Genart last call review of draft-ietf-nfsv4-rpc-tls-07

From: Dale Worley via Datatracker <[email-masked]>
Date: Sun, 24 May 2020 13:50:32 -0700

Reviewer: Dale Worley
Review result: Ready with Nits

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document:  draft-ietf-nfsv4-rpc-tls-07
Reviewer:  Dale R. Worley
Review Date:  2020-05-24
IETF LC End Date:  2020-05-27
IESG Telechat date:  unknown

Summary:

Note that I am not familiar with the operations of TLS, so I have not
reviewed issues that are specific to security implementations.  I
assume the SECDIR review has adequately covered those.

This draft is quite solid and nearly ready for publication, but has
nits that should be fixed before publication.

Nits:

4.1.  Discovering Server-side TLS Support

Somewhere in this section you need to specify the semi-obvious:

   In principle, RPC is transport-agnostic.  In the context of
   RPC-Over-TLS, the initial request MUST be sent using either TCP or
   UDP.  If an initial TCP request is successful, a TLS connection is
   established.  If an initial UDP request is successful, a DTLS
   association is established.

--

   If the Reply's reply_stat is MSG_ACCEPTED but the verifier does not
   contain the "STARTTLS" token, or if the Reply's reply_stat is
   MSG_DENIED, the RPC client MUST NOT send a "ClientHello" message.

Strictly, this should list the three tests required by the preceding
paragraph:

   If the Reply's reply_stat is not MSG_ACCEPTED, if its verifier is
   not an AUTH_NONE, or if its verifier's contents are not "STARTTLS",
   the RPC client MUST NOT send a "ClientHello" message.

5.  TLS Requirements

   If the server responds incorrectly, the client MUST NOT establish a
   TLS session for use with RPC on this connection.

I think the condition can be made more specific as "If the "ServerHello"
message does not conform to the above requirement, ...".

5.1.1.  Protected Operation on TCP

   The server does not attempt to establish a TLS session
   on a TCP connection for backchannel operation.

I think "... does not attempt to establish a separate TLS session ..."
is clearer.

I can't find any discussion of "backchannel operation" in RFC 5531.
Might this need an additional reference?

5.2.1.  X.509 Certificates Using PKIX trust

   o  For services accessed by their network identifiers (netids) and
      universal network addresses (uaddr), the iPAddress subjectAltName
      SHOULD be present in the certificate and must exactly match the
      address represented by universal address.

I suspect that "iPAddress" is not capitalized correctly.

"universal address" probably should be either "universal network
address" or "uaddr".

5.2.2.  X.509 Certificates Using Fingerprints

   Implementations MUST support SHA-256
   [FIPS.180-4] or stronger as the hash algorithm for the fingerprint.

Perhaps there is a usage in security, but it seems to me that
"stronger" is not well-defined.  I expect that it means that the hash
algorithm must produce output of at least 256 bits, but that criterion
makes no allowances that over time algorithms might be deprecated.  Is
there a list of accepted "strong" hash algorithms that can be
referenced here to make this unambiguous and track best practice over
time?

Also -- here I'm assuming that one peer presents a certificate to the
other, and the second peer first hashes the certificate to create a
fingerprint and then checks the fingerprint against a list -- this
sentence does not require the peer to *use* an algorithm that meets
this criterion.  The text needs to be something like

   Implementations MUST use SHA-256 [FIPS.180-4] or stronger as the
   hash algorithm for the fingerprint.

7.1.  Limitations of an Opportunistic Approach

   Implementations of
   the mechanism described in the current document must take care to
   accurately represent to all RPC consumers the level of security that
   is actually in effect, ...

I think you want s/must/MUST/.  There's also an unstated requirement
that the RPC consumers have some way of accessing this information.

7.3.  Security Considerations for AUTH_SYS on TLS

   Using a TLS-protected transport when the AUTH_SYS authentication
   flavor is in use addresses several longstanding weaknesses (as
   detailed in Appendix A).

For clarity "... several longstanding weaknesses in AUTH_SYS (detailed
in Appendix A)."

   TLS guards against the
   insertion or deletion of messages, thus also ensuring the integrity
   of the message stream between RPC client and server.

This statement needs to be weakened somewhat for DTLS.

   o  When using RPCSEC_GSS, GSS/Kerberos provides adequate host
      authentication and a policy that requires GSS mutual
      authentication and rejection of a connection when host
      authentication fails.  GSS integrity and privacy services,
      therefore, can be disabled in favor of TLS encryption with peer
      authentication.

This paragraph is not constructed correctly.  The first sentence says
that GSS provides certain facilities, and the second sentence says
that "therefore" GSS (or certain parts of GSS) can be disabled.  There
are a number of possibilities for the form the argument should take,
but I don't know enough about security to know what was intended.

8.1.  RPC Authentication Flavor

   Description:  Signals the use of TLS to protect RPC messages on
      socket-based transports

Better would be "Used in probes for support of RPC-Over-TLS."

9.3.  URIs

Note that all references to URIs in this section are to be removed by
the RFC Editor, so this section should be removed also.

[END]

---

From: Chuck Lever <[email-masked]>
Date: Sun, 24 May 2020 17:55:52 -0400

> On May 24, 2020, at 4:50 PM, Dale Worley via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Dale Worley

Thank you, Dale, for the detailed feedback. I will reply to this
e-mail thread with specific text corrections in the next few days.

I do have a few initial reactions/follow-ups for you that appear
inline below.

>   association is established.

I can add something like this in Section 4.1, but note that Sections
5.1.1 and 5.1.2 already explain the relationships between TCP/UDP
and TLS/DTLS, respectively.

> Might this need an additional reference?

I agree that a deeper introduction of "backchannel operation" would
be helpful in this section.

There doesn't seem to be any adequate explanation for backchannel
operation in documents prior to RFC 8167, which explains reverse-
direction RPC operation over an RDMA transport.

Perhaps the best I can do here is add a paragraph introducing the
concept, and use the RFC 8167 terminology instead of "backchannel"?
Let me review RFC 8167 and see if I can reference it sensibly in
the context of RPC on TCP.

> I suspect that "iPAddress" is not capitalized correctly.

This is the capitalization used in RFC 6125, which is cited nearby
this text.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Mon, 25 May 2020 07:46:21 -0400

> direction RPC operation over an RDMA transport.

The term is introduced in RFC5661.   Not sure what you consider an adequate
explanation, but this would be a relevant reference that also might be
cited.

> concept, and use the RFC 8167 terminology instead of

This concept does need to be introduced at the RPC-level and rpc-tls, which
is to update RFC5531, is a good place to do it.   I think it would be best
to explain the connections between the RFC5661 and RFC8167 terminologies
Let me review RFC 8167 and see if I can reference it sensibly in
the context of RPC on TCP.

---

From: Trond Myklebust <[email-masked]>
Date: Mon, 25 May 2020 12:04:00 +0000

> direction RPC operation over an RDMA transport.

The term is introduced in RFC5661.   Not sure what you consider an adequate explanation, but this would be a relevant reference that also might be cited.

> concept, and use the RFC 8167 terminology instead of

This concept does need to be introduced at the RPC-level and rpc-tls, which is to update RFC5531, is a good place to do it.   I think it would be best to explain the connections between the RFC5661 and RFC8167 terminologies
Let me review RFC 8167 and see if I can reference it sensibly in
the context of RPC on TCP.

There really is no mention in RFC5531 of RPC being unidirectional either. It does describe a client-server model, but imposes no limitations on the number of services a single transport can support, or on directionality of the calls. Ultimately, the only special thing about a backchannel is that it is a service on one side of the transport that is associated with a different service on the other side of the channel.

Is that really a nuance that needs to be explained in this document? We've already determined the TLS behaviour when a transport supports multiple services.

On Sun, May 24, 2020 at 5:58 PM Chuck Lever <[email-masked]<mailto:[email-masked]>> wrote:

> On May 24, 2020, at 4:50 PM, Dale Worley via Datatracker <[email-masked]<mailto:[email-masked]>> wrote:
>
> Reviewer: Dale Worley

--
Chuck Lever

--

Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
www.hammer.space<http://www.hammer.space>

---

From: Chuck Lever <[email-masked]>
Date: Mon, 25 May 2020 11:33:42 -0400

Hi Trond-

> On May 25, 2020, at 8:04 AM, Trond Myklebust <[email-masked]> wrote:
> 

True. The discussion of reverse-direction operation here is implementation
advice rather than the statement of more compliance rules.

I added this paragraph because it feels valuable to point out this very
fact: reverse-direction operation is not an exception to the "multiple
services on a connection" rule. I believe that as a putative implementer
I would appreciate seeing this statement in clear black-and-white letters.

We are likely to get asked for clarification -- or worse, have to deal
with improper implementations -- without this statement.

> www.hammer.space

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Mon, 25 May 2020 11:34:01 -0400

Hi David-

> On May 25, 2020, at 7:46 AM, David Noveck <[email-masked]> wrote:
> 

I'm not comfortable citing an NFSv4 document to define a term used in
a document that discusses a generic RPC transport. To me that feels an
awful lot like a layering violation.

Moreover, after reviewing Section 2.10.3.1 of RFC 5661, the introduction
of the idea of "channel" does not require that backchannel operation
take place on the same connection with forward channel operation. Thus
"backchannel" does not seem to be precisely the terminology we want in
rpc-tls, where we are discussing specifically multiple services on one
connection, without any association to a particular Upper Layer RPC
consumer.

> This concept does need to be introduced at the RPC-level and rpc-tls, which is to update RFC5531, is a good place to do it.

The language in RFC 8167 has already been vetted by the IESG. We have
that definition now in a citable public document, so we should use that
rather than duplicating it.

Note that RFC 8167 Section 2 has a complete definition of this mode of
operation. In rpc-tls, such an extended discussion would be a distract-
ing sidebar, especially since the paragraph in question is not making
any normative addition to the RPC-on-TLS protocol.

Also I should point out that given the post-WGLC phase we are in, adding
significant and possibly normative new text to this document is probably
not appropriate.

> I think it would be best to explain the connections between the RFC5661
> and RFC8167 terminologies

I agree philosophically with that. However, rfc5661bis seems to me like
the more relevant and appropriate place to make such connections.

In rpc-tls, I'd rather replace the term "backchannel" with "reverse-
direction operation" plus a citation.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Mon, 25 May 2020 13:57:36 -0400

Hi Dale-

One additional thought.

> nits that should be fixed before publication.

 ...

> that the RPC consumers have some way of accessing this information.

IMHO the non-normative form of "must" is appropriate in this case.

 - The text is not specific enough to be part of an enforceable normative
   compliance statement.

 - The remark here is about an internal RPC consumer API, as you note.
   Such APIs are not something on which this document can place hard
   requirements.

However, I'm open to further comments and discussion.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Wed, 27 May 2020 12:43:31 -0400

Hi-

Below are proposals for addressing the Gen-ART comments.

>   association is established.

Original:

   When an RPC client is ready to begin a TLS session, it sends a NULL
   RPC procedure with an auth_flavor of AUTH_TLS.  The value of AUTH_TLS
   is defined in Section 8.1.  The NULL request is made to the same port
   as if TLS were not in use.

Replacement:

   In general, RPC is transport-agnostic.  However, RPC-on-TLS supports
   only operation on TCP (with TLS) and UDP (with DTLS). For further
   details, consult Section 5.1.

   When an RPC client is ready to begin a TLS session, it opens a TCP
   connection to an RPC server and sends a NULL RPC procedure with an
   auth_flavor of AUTH_TLS on that connection.  To begin a DTLS
   association, it sends the server a UDP datagram containing a NULL RPC
   procedure with an auth_flavor of AUTH_TLS.  The value of AUTH_TLS is
   defined in Section 8.1.  The NULL request is made to the same port as
   if TLS were not in use.

>   the RPC client MUST NOT send a "ClientHello" message.

Original:

   If the Reply's reply_stat is MSG_ACCEPTED but the verifier does not
   contain the "STARTTLS" token, or if the Reply's reply_stat is
   MSG_DENIED, the RPC client MUST NOT send a "ClientHello" message.
   RPC operation can continue, however it will be without any
   confidentiality, integrity or authentication protection from (D)TLS.

Replacement:

   Conversely, if the Reply's reply_stat is not MSG_ACCEPTED, if it's
   verifier flavor is not AUTH_NONE, or if it's verifier does not
   contain the "STARTTLS" token, the RPC client MUST NOT send a
   "ClientHello" message.  RPC operation can continue, however it will
   be without any confidentiality, integrity or authentication
   protection from (D)TLS.

> message does not conform to the above requirement, ...".

Original:

   If the server responds incorrectly, the client MUST NOT establish a
   TLS session for use with RPC on this connection.  See [RFC7301] for
   further details about how to form these messages properly.

Replacement:

   If the server responds incorrectly (for instance, if the
   "ServerHello" message does not conform to the above requirement), the
   client MUST NOT establish a TLS session for use with RPC on this
   connection.  See [RFC7301] for further details about how to form
   these messages properly.

> Might this need an additional reference?

Original:

   Backchannel operation occurs only on connected transports such as
   TCP.  To protect backchannel operations, an RPC server uses the
   existing TLS session on that connection to send backchannel
   operations.  The server does not attempt to establish a TLS session
   on a TCP connection for backchannel operation.

Replacement:

   Reverse-direction operation occurs only on connected transports such
   as TCP (see Section 2 of [RFC8166]).  To protect reverse-direction
   RPC operations, the RPC server does not establish a separate TLS
   session on the TCP connection, but instead uses the existing TLS
   session on that connection to protect these operations.

> I suspect that "iPAddress" is not capitalized correctly.

We believe that iPAddress is the correct spelling of this attribute's
name. No replacement proposed.

> "universal address" probably should be either "universal network

Original:

Replacement:

   o  For services accessed by their network identifiers (netids) and
      universal network addresses (uaddr), the iPAddress subjectAltName
      SHOULD be present in the certificate and must exactly match the
      address represented by the universal network address.

>   hash algorithm for the fingerprint.

Our proposal is to remove the SHA-256 requirement:
 • There is no standard specification for computing certificate
   fingerprints.
 • I have found no authority that claims SHA-1 (which is most common
   in fingerprinting tools) is inadequate.
 • Certificate fingerprints are constructed by system components other
   than the RPC implementation, thus a normative requirement here is
   very likely beyond the scope of this document.

Original:

   This mechanism is OPTIONAL to implement.  In this mode, the
   fingerprint of the presented certificate uniquely identifies the RPC
   peer.

   Implementations SHOULD allow the configuration of a list of trusted
   certificates, identified via fingerprint of the DER-encoded
   certificate octets.  Implementations MUST support SHA-256
   [FIPS.180-4] or stronger as the hash algorithm for the fingerprint.

Replacement:

   This mechanism is OPTIONAL to implement.  In this mode, the
   fingerprint of a certificate uniquely identifies an RPC peer.

   A "fingerprint" is typically defined as a cryptographic digest of the
   Distinguished Encoding Rules (DER) form [X.690] of an X.509v3
   certificate [X.509].  Implementations SHOULD allow the configuration
   of a list of trusted certificates that is indexed by fingerprint.

> that the RPC consumers have some way of accessing this information.

As discussed earlier, we believe that "must" is correct in this
context. No replacement proposed.

> For clarity "... several longstanding weaknesses in AUTH_SYS (detailed

Original:

Replacement:

   Using a TLS-protected transport when the AUTH_SYS authentication
   flavor is in use addresses several longstanding weaknesses in
   AUTH_SYS (as detailed in Appendix A).

> This statement needs to be weakened somewhat for DTLS.

Original:

                             TLS augments AUTH_SYS by providing both
   integrity protection and confidentiality that AUTH_SYS lacks.  TLS
   protects data payloads, RPC headers, and user identities against
   monitoring and alteration while in transit.  TLS guards against the
   insertion or deletion of messages, thus also ensuring the integrity
   of the message stream between RPC client and server.  Lastly,
   transport layer encryption plus peer authentication protects
   receiving XDR decoders from deserializing untrusted data, a common
   coding vulnerability.

Replacement:

                                          TLS augments AUTH_SYS by
   providing both integrity protection and confidentiality that AUTH_SYS
   lacks.  TLS protects data payloads, RPC headers, and user identities
   against monitoring and alteration while in transit.

   TLS guards against in-transit insertion and deletion of RPC messages,
   thus ensuring the integrity of the message stream between RPC client
   and server.  DTLS does not provide full message stream protection,
   but it does enable receivers to reject non-participant messages.  In
   particular, transport layer encryption plus peer authentication
   protects receiving XDR decoders from deserializing untrusted data, a
   common coding vulnerability.

> but I don't know enough about security to know what was intended.

Original:

Replacement:

   o  RCPSEC_GSS provides integrity and privacy services which are
      redundant when TLS is in use.  These services should be disabled
      in that case.

> Better would be "Used in probes for support of RPC-Over-TLS."

Original:

Replacement:

   Description:  Indicates support for RPC-on-TLS.

> the RFC Editor, so this section should be removed also.

Section 9.3 is generated by xml2rfc. It is our understanding that the
RFC Editor invokes xml2rfc for the final publication in a mode where
this section is not generated and thus it will automatically be
removed.

--
Chuck Lever

---

From: [email-masked] (Dale R. Worley)
Date: Wed, 27 May 2020 23:02:45 -0400

Chuck Lever <[email-masked]> writes:
>>   [...]

Hmmm, I want to answer "yes and no".  I think those passages were
written with the presupposition that those relationships were already
known and specified, and the text talks *about* that relationship.
E.g., 5.1.1 qualifies the sentence with "Typically", and neither section
uses normative language.

The point is that if you upgrade, if you start with TCP, you MUST
upgrade to TLS, and if you start with UDP, you MUST upgrade to DTLS.
Whereas it is conceivable that one could start with UDP to port 111,
discover rpc-tls support and then do a TLS connection to TCP port 111
("the same port") to continue.  (After all, every NFS server listens on
111 both with UDP and TCP, right?)  And you have to state that
explicitly as a requirement.

> the context of RPC on TCP.

I wouldn't even go that far.  Reading the I-D, I did not feel that I
needed any additional knowledge of how "backchannel" is done to
understand what the I-D was requiring.  But the fact that I couldn't
trace from any reference to the specification of backchannel seems like
an inadequacy.  IMO just a reference here to 8167 would suffice.

> This is the capitalization used in RFC 6125, which is cited nearby

So I'm wrong there!

Dale

---

From: [email-masked] (Dale R. Worley)
Date: Wed, 27 May 2020 23:05:28 -0400

> awful lot like a layering violation.

If doing that would be a layering violation, then this passage is also a
layering violation:

   To protect backchannel operations, an RPC server uses the
   existing TLS session on that connection to send backchannel
   operations.  The server does not attempt to establish a TLS session
   on a TCP connection for backchannel operation.

Dale

---

From: [email-masked] (Dale R. Worley)
Date: Wed, 27 May 2020 23:06:54 -0400

> However, I'm open to further comments and discussion.

Hmmm, I think I'll leave that to the Security Directorate.

Dale

---

From: Chuck Lever <[email-masked]>
Date: Wed, 27 May 2020 23:43:53 -0400

> On May 27, 2020, at 11:05 PM, Dale R. Worley <[email-masked]> wrote:
> 

Yes, it is a layering violation. That's why I proposed replacing the
use of the term "backchannel" with the more correct "reverse-direction
operation", in addition to adding a citation of RFC 8167.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Wed, 27 May 2020 23:57:31 -0400

> On May 27, 2020, at 11:02 PM, [email-masked] wrote:
> 

NFSv4 servers don't have to provide an rpcbind service on port 111 any
more... but I get your point.

The proposed replacement text I posted earlier today explains how it is
supposed to work but does not use normative language. It needs to take
another step and /require/ that a client uses the appropriate protection
mechanism for the transport it wants to use.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Fri, 29 May 2020 19:36:33 -0400

> On May 27, 2020, at 11:57 PM, Chuck Lever <[email-masked]> wrote:
> 
> 
> 

Proposing the following text to replace all of Section 4.1. It better
describes the initiation of an AUTH_TLS probe, and provides additional
normative requirements to help bridge the gap cleanly between the probe
and the establishment of a TLS session.

   The mechanism described in the current document interoperates fully
   with RPC implementations that do not support RPC-on-TLS.  Policy
   settings on the RPC-on-TLS-enabled peer determine whether RPC
   operation continues without the use of TLS or RPC operation is not
   permitted.

   To achieve this interoperability, we introduce a new RPC
   authentication flavor called AUTH_TLS.  The AUTH_TLS authentication
   flavor signals that the client wants to initiate TLS negotiation if
   the server supports it.  Except for the modifications described in
   this section, the RPC protocol is unaware of security encapsulation
   at the transport layer.  The value of AUTH_TLS is defined in
   Section 8.1.

   An RPC client begins its communication with an RPC server by
   selecting a transport and destination port.  The choice of transport
   and port is typically based on the RPC program that is to be used.
   The RPC client might query the RPC server's rpcbind service to make
   this selection.  In all cases, an RPC server MUST listen on the same
   ports for (D)TLS-protected RPC programs as the ports used when (D)TLS
   is not available.

   To protect RPC traffic to a TCP port, the RPC client opens a TCP
   connection to that port and sends a NULL RPC procedure with an
   auth_flavor of AUTH_TLS on that connection.  To protect RPC traffic
   to a UDP port, the RPC client sends a UDP datagram to that port
   containing a NULL RPC procedure with an auth_flavor of AUTH_TLS.  The
   mechanism described in the current document does not support RPC
   transports other than TCP and UDP.

   The length of the opaque data constituting the credential sent in the
   RPC Call message MUST be zero.  The verifier accompanying the
   credential MUST be an AUTH_NONE verifier of length zero.

   The flavor value of the verifier in the RPC Reply message received
   from the server MUST be AUTH_NONE.  The length of the verifier's body
   field is eight.  The bytes of the verifier's body field encode the
   ASCII characters "STARTTLS" as a fixed-length opaque.

   If the RPC server replies with a reply_stat of MSG_ACCEPTED and an
   AUTH_NONE verifier containing the "STARTTLS" token, the client SHOULD
   proceed with TLS session establishment, even if the Reply's
   accept_stat is not SUCCESS.  If the AUTH_TLS probe was done via TCP,
   the RPC client MUST send the "ClientHello" message on the same
   connection.  If the AUTH_TLS probe was done via UDP, the RPC client
   MUST send the "ClientHello" message to the same UDP destination port.

   Conversely, if the Reply's reply_stat is not MSG_ACCEPTED, if its
   verifier flavor is not AUTH_NONE, or if its verifier does not contain
   the "STARTTLS" token, the RPC client MUST NOT send a "ClientHello"
   message.  RPC operation can continue, however it will be without any
   confidentiality, integrity or authentication protection from (D)TLS.

   If, after a successful RPC AUTH_TLS probe, the subsequent (D)TLS
   handshake should fail for any reason, the RPC client reports this
   failure to the upper-layer application the same way it reports an
   AUTH_ERROR rejection from the RPC server.

   If an RPC client uses the AUTH_TLS authentication flavor on any
   procedure other than the NULL procedure, or an RPC client sends an
   RPC AUTH_TLS probe within an existing (D)TLS session, the RPC server
   MUST reject that RPC Call by setting the reply_stat field to
   MSG_DENIED, the reject_stat field to AUTH_ERROR, and the auth_stat
   field to AUTH_BADCRED.

   Once the TLS session handshake is complete, the RPC client and server
   have established a secure channel for communicating.  A successful
   AUTH_TLS probe on one particular port/transport tuple never implies
   RPC-on-TLS is available on that same server using a different port/
   transport tuple.

--
Chuck Lever

---

From: [email-masked] (Dale R. Worley)
Date: Fri, 29 May 2020 21:44:31 -0400

These changes look sufficient to me.

Dale