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

[Chuck Lever <[e-mail]>]

Hi-

Below are proposals for addressing the Gen-ART comments.

> On May 24, 2020, at 4:50 PM, Dale Worley via Datatracker <[e-mail]> wrote:
> 
> 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.

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.


>   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.

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.


> 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, ...".

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.


> 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?

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.


> 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.

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


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

Original:

   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.

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.


> 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.

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.


> 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.

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


> 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)."

Original:

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

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).


>   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.

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.


>   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.

Original:

   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.

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.


> 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."

Original:

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

Replacement:

   Description:  Indicates support for RPC-on-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.

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