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

[Spencer Dawkins at IETF <[e-mail]>]

Hi, Chuck,

Thanks for the quick response. Getting back with an AD who hasn't slept
since he sent his evaluation is ALWAYS appreciated.

Responses inline, but Tl;Dr is "this all looks excellent".

Specifically,

On Mon, Aug 7, 2017 at 5:28 PM, Chuck Lever <[e-mail]> wrote:

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

That would be great (unless your shepherd thinks it wouldn't be).


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


Check.


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

Of course, I don't know if "hitting in" is common NFS terminology, but I do
appreciate this explanation. Perhaps other readers would also benefit.

Check.


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

That would be helpful for me to know.

I should also mention that I volunteer with teenagers, where reasons work
better ... your mileage may vary.


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

That sounds like a plan to me.

Check.


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


Check.


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


I did know something about NFS in the Version 2 timeframe, but even having
not thought about that since then, your proposed text is clear to me.
Thanks.

Check.


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


Sure. I think you can reasonably submit a revision with the changes we
discussed above, but you may want to propose any text changes to the
working group mailing list before submitting a draft with them.

And thanks again. You guys are always a pleasure to work with.

Spencer