Re: [nfsv4] AD review of draft-ietf-nfsv4-rfc5667bis-11

[Chuck Lever <[e-mail]>]

> On Aug 7, 2017, at 5:45 PM, Spencer Dawkins at IETF <[e-mail]> wrote:
> 
> Hi, Chuck,
> 
> This is another one of the NFSv4 docs that is easy to review. Thanks for that.
> 
> I have some comments, but most of them are about clarity, rather than about technical concerns.
> 
> Please let me know what you think, after you've had an opportunity to look these over.
> 
> Thanks,
> 
> Spencer (D)
> 
> --
> 
> Pretty much every reviewer is going to ask you to update the reference to draft-ietf-nfsv4-rfc5666bis to point to RFC 8166. Might as well do that sooner, rather than later, because when reviewers start typing, they rarely stop ...

OK. This revision is older than RFC 8166. Looks like there are some
other editorial changes needed (like, switching to "RPC-over-RDMA
version 1").

May I take this opportunity to address the issues you raise by
submitting a fresh revision of this document?


> When I saw this text
> 
>    This document contains material required of Upper Layer Bindings, as
>    specified in [I-D.ietf-nfsv4-rfc5666bis], for the following NFS
>    protocol versions:
> 
>    o  NFS Version 2 [RFC1094]
> 
>    o  NFS Version 3 [RFC1813]
> 
>    o  NFS Version 4.0 [RFC7530]
> 
>    o  NFS Version 4.1 [RFC5661]
> 
>    o  NFS Version 4.2 [RFC7862]
> 
>    Upper Layer Bindings are also provided for auxiliary protocols used
>    with NFS versions 2 and 3.
>    
> I found myself wondering "what auxiliary protocols?" When you define the Upper Layer Bindings for them in Section 4, that's very clear. Could you consider either moving some of the Section 4 text here or just adding a forward reference here, pointing to Section 4?

Forward reference sounds fine to me.


> I found this text 
> 
>    (to avoid hitting in a Duplicate Reply Cache)
>    
> a trifle unclear - is this saying 
> 
>    (to avoid receiving the same error again from a Duplicate Reply Cache)
>    
> or something else? But even s/hitting in/hitting/ would be clearer for me.

Hrm. "hitting in" is more sensible to me.

A new XID is assigned to the retransmitted request to avoid
matching a cached reply that caches the original error. I will
try to clarify this sentence.


> In this text, 
> 
>    Unless
>    explicitly mentioned as applicable, short Reply chunk retry should
>    not be used.
>    
> I wondered if you're saying "this is less efficient", or "this will probably fail". Or some other possibility. But perhaps being clear about likely consequences of doing something you're recommending against, would help people talk themselves into believing you.

I'll come up with something. Essentially, in many cases, the
situation is not improved by retrying. In other words, it will
only fail again.


> This text
> 
>    A transport error does not give an indication of whether the server
>    has processed the arguments of the RPC Call, or whether the server
>    has accessed or modified client memory associated with that RPC.
>    
> appears in Section 3, which covers NFS 2 and NFS 3/legacy NFS. Is this also true for more recent versions? Or is that not a meaningful concept in non-legacy NFS?

Section 5.6 discusses how NFSv4.1 and newer handles such transport
errors.

IMO the document does not cover NFSv4.0, which would have the
same issue as NFSv2 and NFSv3. It might be appropriate to copy
the above paragraph to section 5.6, with a suitable caveat
that it applies only to NFSv4.0.


> Pardon my unfamiliarity, but in this text,
> 
>    Historically, NFS/RDMA implementations have chosen to convey the
>    MOUNT, NLM, and NSM protocols via TCP.  To enable interoperation of
>    these protocols when NFS/RDMA is in use, a legacy NFS server MUST
>    provide TCP-based MOUNT, NLM, and NSM services.
>    
> are these auxiliary protocols a set (so, they would all be present), or are they independent (so some might be present while others might not be)? If they're a set, "MUST provide x, y, and z" is the right answer, but if they're independent, "MUST provide support for these protocols via TCP" would be more appropriate. (I only ask about this because this text is inside a MUST)

These are not a set. MOUNT has to be present, but NLM and NSM
are optional. I can adopt the "MUST provide support for these
protocols via TCP" language.


> I see that NFSACL isn't mentioned in the previous paragraph I asked about. I see
> 
>    Legacy clients and servers that support the NFSACL RPC Program
>    typically convey NFSACL procedures on the same connection as NFS RPC
>    Programs.  This obviates the need for separate rpcbind queries to
>    discover server support for this RPC Program.
>    
> in the following section, and I'm assuming that the question of transport protocol support for NFSACL has already been answered because it's using the same connection as some other protocol that you've already answered the question about. Did I get that right?

Yes: "NFS RPC Programs" means just program 100003. Perhaps that
should be "the NFS RPC program (100003).".


> In this text,
> 
>    Specifically, the requirements of
>    [I-D.ietf-nfsv4-rfc5666bis] ensure that this choice does not
>    introduce new vulnerabilities.
>    
> I believe you, but the people who write security reviews might appreciate a pointer to a specific section in [I-D.ietf-nfsv4-rfc5666bis] to bound their search, if there's one you can point to.

So, here's section 7 of RFC 5667 in its entirety:

   The RDMA transport for RPC [RFC5666] supports all RPC [RFC5531]
   security models, including RPCSEC_GSS [RFC2203] security and link-
   level security.  The choice of RDMA Read and RDMA Write to return RPC
   argument and results, respectively, does not affect this, since it
   only changes the method of data transfer.  Specifically, the
   requirements of [RFC5666] ensure that this choice does not introduce
   new vulnerabilities.

   Because this document defines only the binding of the NFS protocols
   atop [RFC5666], all relevant security considerations are therefore to
   be described at that layer.

The Security Considerations section of rfc5667bis is a copy
of this section, with a few terminology tweaks and updated
references.

At this point I'm not finding the rfc5667bis language to be
adequate. I think I can address your other comments quickly,
but this one I'd like to consider further.


--
Chuck Lever