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

From: Spencer Dawkins at IETF <[email-masked]>
Date: Mon, 7 Aug 2017 16:45:45 -0500

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

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?

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.

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.

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?

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)

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?

In this text,

   Specifically, the requirements of
   [I-D.ietf-nfsv4-rfc5666bis] ensure that this choice does not
   introduce new vulnerabilities. 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.

---

From: Chuck Lever <[email-masked]>
Date: Mon, 7 Aug 2017 18:28:03 -0400

> On Aug 7, 2017, at 5:45 PM, Spencer Dawkins at IETF <[email-masked]> 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?

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

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

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

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

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

---

From: Spencer Dawkins at IETF <[email-masked]>
Date: Mon, 7 Aug 2017 18:05:08 -0500

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 <[email-masked]> wrote:

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

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

> Forward reference sounds fine to me.

Check.

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

> situation is not improved by retrying. In other words, it will

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.

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

That sounds like a plan to me.

Check.

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

Check.

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

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.

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

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

---

From: Spencer Dawkins at IETF <[email-masked]>
Date: Tue, 8 Aug 2017 14:28:57 -0500

Hi, Chuck,

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

I see that you've submitted a -12, and I think it's ready for Last Call.

You had mentioned wanting to think about the Security Considerations a bit
more, so I thought a bit more, too. Here's what I'm wondering.

The current text says this:

8 <https://tools.ietf.org/html/draft-ietf-nfsv4-rfc5667bis-12#section-8>.
Security Considerations

   RPC-over-RDMA version 1 supports all RPC security models, including
   RPCSEC_GSS security and transport-level security [RFC7861
<https://tools.ietf.org/html/rfc7861>].  The
   choice of what Direct Data Placement mechanism to convey RPC argument
   and results does not affect this, since it changes only the method of
   data transfer.  Specifically, the requirements of [RFC8166
<https://tools.ietf.org/html/rfc8166>] ensure
   that this choice does not introduce new vulnerabilities.

   Because this document defines only the binding of the NFS protocols
   atop [RFC8166 <https://tools.ietf.org/html/rfc8166>], all relevant
security considerations are therefore to

What I think you're saying, is that the RPC security considerations have
already been analyzed in RFC 7861, and the RDMA security considerations
have already been analyzed in RFC 8166, so using RPC over RDMA shouldn't
introduce new security considerations. Am I getting that right?

Assuming so ... the sentence I was questioning, was

   Specifically, the requirements of [RFC8166
<https://tools.ietf.org/html/rfc8166>] ensure
   that this choice does not introduce new vulnerabilities.

Although I'm not a secdir reviewer (I don't even play one on the Internet),
I'm reading that sentence as an invitation to go look at the requirements
of [RFC8166] and see if that makes sense. Other reviewers might or might
not accept that invitation, but I find myself wondering whether you need
this sentence at all, since you've basically said, "we've analyzed the
pizza crust, and we've analyzed the pizza ingredients, so we don't have new
problems when we put the ingredients on the crust and make a pizza".

Do you think the working group needs to think about this some more? I'm
happy to hold off on Last Call, if the answer is "yes", of course.

Thanks,

Spencer (D)

---

From: Chuck Lever <[email-masked]>
Date: Tue, 8 Aug 2017 20:45:14 -0400

> On Aug 8, 2017, at 3:28 PM, Spencer Dawkins at IETF <[email-masked]> wrote:
> 
> Hi, Chuck,
> 
> On Mon, Aug 7, 2017 at 6:05 PM, Spencer Dawkins at IETF <[email-masked]> wrote:
> Hi, Chuck,
> 
> Thanks for the quick response. Getting back with an AD who hasn't slept since he sent his evaluation is ALWAYS appreciated. 
> 
> You had mentioned wanting to think about the Security Considerations a bit more, so I thought a bit more, too. Here's what I'm wondering.
> 
> 8.  Security Considerations
> 
> 
>    RPCSEC_GSS security and transport-level security [RFC7861].  The
>    data transfer.  Specifically, the requirements of [RFC8166] ensure
>    atop [RFC8166], all relevant security considerations are therefore to 
> What I think you're saying, is that the RPC security considerations have already been analyzed in RFC 7861, and the RDMA security considerations have already been analyzed in RFC 8166, so using RPC over RDMA shouldn't introduce new security considerations. Am I getting that right?

Yes.

>    Specifically, the requirements of [RFC8166] ensure
> Although I'm not a secdir reviewer (I don't even play one on the Internet), I'm reading that sentence as an invitation to go look at the requirements of [RFC8166] and see if that makes sense. Other reviewers might or might not accept that invitation, but I find myself wondering whether you need this sentence at all,

Yep, that's how I read your comment yesterday, and how I reread the
section upon review.

I started by mentally removing that sentence, then I was less sure
that the remaining text made sense.

> since you've basically said, "we've analyzed the pizza crust, and we've analyzed the pizza ingredients, so we don't have new problems when we put the ingredients on the crust and make a pizza".

OK, if you read it that way, then I guess good communication is
happening. That's pretty much what is intended.

> Do you think the working group needs to think about this some more? I'm happy to hold off on Last Call, if the answer is "yes", of course.

Given recent scrutiny of the Security Considerations in our WG work
products, it might be prudent for the WG to reconsider just this
section for a few moments.

Let's start with this:

   RPC-over-RDMA version 1 supports all RPC security models, including
   RPCSEC_GSS security and transport-level security [RFC7861].  The
   choice of what Direct Data Placement mechanism to convey RPC argument
   and results does not affect this, since it changes only the method of
   data transfer.

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

I suppose that could be OK. Any WG objections to going with this?

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Wed, 9 Aug 2017 06:53:44 -0400

I think this is OK.  I know that there is reason to worry about SECDIR but
there is only so much you can usefully do, until they actually have
something to look at.

I imagine that SECDIR might be worried that direct placement could somehow
open a hole whereby data might inadvertantly be transferred in the clear.

For example, if I ordered a "Large pizza with direct pepperoni placement"
:), the pepperoni cannot be directly placed if privcy is in effect, but
this sort of thing is aprropriately dealt with in RFC8166, as it should be.

On Tue, Aug 8, 2017 at 8:45 PM, Chuck Lever <[email-masked]> wrote:

> > You had mentioned wanting to think about the Security Considerations a
> bit more, so I thought a bit more, too. Here's what I'm wondering.
> >
> > Although I'm not a secdir reviewer (I don't even play one on the
> Internet), I'm reading that sentence as an invitation to go look at the
> requirements of [RFC8166] and see if that makes sense. Other reviewers
> might or might not accept that invitation, but I find myself wondering
> whether you need this sentence at all,
>
> > since you've basically said, "we've analyzed the pizza crust, and we've
> analyzed the pizza ingredients, so we don't have new problems when we put
> the ingredients on the crust and make a pizza".
>
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>

---

From: Spencer Dawkins at IETF <[email-masked]>
Date: Wed, 9 Aug 2017 12:33:44 -0500

Just to say something more clearly than I actually said it last time (take
2) ...

> there is only so much you can usefully do, until they actually have

"Some of my best friends are SECDIR reviewers", so I'd encourage people not
to worry about them. My comments on the original text in this draft
reflected my thought that the text was potentially sending a SECDIR
reviewer down a rabbit hole

> this sort of thing is aprropriately dealt with in RFC8166, as it should be.

That's exactly the kind of thing I think SECDIR reviewers should be
thinking about - and working group participants as well.

Thanks for making that clearer than I could.

Spencer

---

From: Chuck Lever <[email-masked]>
Date: Wed, 9 Aug 2017 13:44:50 -0400

> On Aug 9, 2017, at 6:53 AM, David Noveck <[email-masked]> wrote:
> 
> I think this is OK.  I know that there is reason to worry about SECDIR but there is only so much you can usefully do, until they actually have something to look at.

"only so much you can ... do until [SECDIR] actually [has] something to look at"

Good point. I'll go with the edited text at the bottom of this e-mail.

> I imagine that SECDIR might be worried that direct placement could somehow open a hole whereby data might inadvertantly be transferred in the clear.
> 
> For example, if I ordered a "Large pizza with direct pepperoni placement" :), the pepperoni cannot be directly placed if privcy is in effect, but this sort of thing is aprropriately dealt with in RFC8166, as it should be.
> 

--
Chuck Lever