Subject: [nfsv4] Benjamin Kaduk's No Objection on draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: (with COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Wed, 19 Feb 2020 17:10:17 -0800

Benjamin Kaduk has entered the following ballot position for
draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-nfsv4-rpcrdma-cm-pvt-data/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I agree with Alissa.

Section 4

   For RPC-over-RDMA version 1, the CM Private Data field is formatted
   as described in the following subsection.  RPC clients and servers
   use the same format.  If the capacity of the Private Data field is
   too small to contain this message format, the underlying RDMA
   transport is not managed by a Connection Manager, or the underlying
   RDMA transport uses Private Data for its own purposes, the CM Private
   Data field cannot be used on behalf of RPC-over-RDMA version 1.

How will an implementation know if "the underlying RDMA transport uses
Private Data for its own purposes"?

Section 5

   Although it is possible to reorganize the last three of the eight
   bytes in the existing format, extended formats are unlikely to do so.
   New formats would take the form of extensions of the format described
   in this document with added fields starting at byte eight of the
   format and changes to the definition of previously reserved flags.

I would suggest making it a (mandatory) invariant of this format to
retain these last three bytes' interpretation, requiring the use of a
different "magic word" for future versions that need to diverge from it.
The current text does not really give an implementation anything that it
can rely on.

Section 6

   The RPC-over-RDMA version 1 protocol framework depends on the
   semantics of the Reliable Connected (RC) queue pair (QP) type, as
   defined in Section 9.7.7 of [IBA].  The integrity of CM Private Data

It's interesting to see such a reference to [IBA], when IIUC the RFC
8166 protocol is not limited to Infiniband as the underlying transport.

   Additional analysis of RDMA transport security appears in the
   Security Considerations section of [RFC5042].  That document

nit: the actual analysis isn't *in* the security considerations section
(but is referenced from it).

   Improperly setting one of the fields in a version 1 Private Message
   can result in an increased risk of disconnection (i.e., self-imposed
   Denial of Service).  There is no additional risk of exposing upper-
   layer payloads after exchanging the Private Message format defined in
   the current document.

I'm not entirely sure where or how one might have expected such
additional exposures to occur.

We should probably mention the risk that some (other) CM-private data
item might inadvertenly produce in its payload the "magic number" that
we use to identify this protocol's data structure.  I *think* (but
please confirm) that erroneously doing so would lead only to (likely)
RDMA-channel disconnection and could not introduce (e.g.) data
corruption.

   In addition to describing the structure of a new format version, any
   document that extends the Private Data format described in the
   current document must discuss security considerations of new data
   items exchanged between connection peers.

In a similar vein, future extensions should consider what the risks of
erroneously identifying "random" data as the new format would be.

Section 7

Should the registry also include the length of the private data?

Similarly to the previous section's comments, should prospective
registrations also be analyzing the risks to their protocol of
interpreting "random" data as the data structure (as would happen upon
an inadvertent match of the "magic number")?

Section 7.1

   The new Reference field should contain a reference to that
   documentation.  The DE can assign new Format Identifiers at random as
   long as they do not conflict with existing entries in this registry.

Random may not be the best choice for this, if there are ways to produce
values that are less-likely-than-random to occur inadvertently in the
payload of any of the registered formats.

---

From: Chuck Lever <[email-masked]>
Date: Thu, 20 Feb 2020 10:13:26 -0500

Hi Ben, thanks for your comments! Responses and questions below.

> On Feb 19, 2020, at 8:10 PM, Benjamin Kaduk via Datatracker <[email-masked]> wrote:
> 

I assume by this you mean that you are seconding the requested change
to the registry policy to "Specification Required" ? I have that change
lined up for the Editor's copy waiting for confirmation that it would
not require another WGLC. See my reply yesterday to Alissa.

AI: Chuck

> Private Data for its own purposes"?

That's what the Format Identifier is for. It indicates where the
RPC/RDMA-specific data sits in the CM Private Data buffer. A
compliant RPC/RDMA implementation does not assume it is the only
user of the CM Private Data.

The "Interoperability Amongst RDMA Transports" section is intended
to explain this. I'm happy to clarify that text if needed.

> The current text does not really give an implementation anything that it

Our experience with RPC/RDMA version 1 itself is that the only field
that needs to be at a fixed location is the Version field. In fact,
we've found the need to change the semantics and data type of one or
more of the first four fields in the RPC/RDMA version 1 header.

We've had to work around the requirement that they remain the same in
all future versions of RPC/RDMA. The text in Section 5 reflects that
experience.

Can you elaborate on the risks that might arise if we were to keep
the existing text?

> 8166 protocol is not limited to Infiniband as the underlying transport.

Correct, RPC/RDMA can operate on iWARP and RoCE as well. However:

 - both of those RDMA fabric types also support RC QPs, and
 - [IBA] appears to be the only publication where "RC QP type"
   is defined, though I could be wrong.

I don't read this text as suggesting that RPC/RDMA cannot operate
on those alternate fabrics, but perhaps I am "too close to the
code".

> (but is referenced from it).

I can revisit RFC 5042 and extract those references.

AI: Chuck

> additional exposures to occur.

Agreed. However, I was asked to make this statement here to make it
clear that the proposed CM Private Data format does not put data
further at risk. I'm open to suggested rewording.

> RDMA-channel disconnection and could not introduce (e.g.) data

Yes, my expectation is that it would have the same consequences as
someone tampering with the RPC/RDMA CM Private Data.

I will see about mentioning this failure mode in the text.

AI: Chuck

> erroneously identifying "random" data as the new format would be.

AI: Chuck

> Should the registry also include the length of the private data?

I can add that.

AI: Chuck

> payload of any of the registered formats.

I chose the current Format Identifier for RPC/RDMA CM Private Data
by extracting the middle bits from a randomly-generated UUID. That's
why the text uses the "random" wording.

IMO we want to avoid choosing values like 0x00000007, precisely because
they are likely to look like a payload. I think we are agreeing here.

Another sentence might be necessary to explain that a new Identifier
should be chosen so that it will not look like a payload of other
registered format. That might be challenging for DEs to carry out in
practice.

AI: Chuck

--
Chuck Lever

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 20 Feb 2020 11:51:55 -0800

Hi Chuck,

> not require another WGLC. See my reply yesterday to Alissa.

Yes, that was it, and sounds good.

> user of the CM Private Data.

Sure ... but now we have some text saying that other uses of CM Private
Data can't coexist with RPC-over-RDMA v1 ("the underlying RDMA transport
uses Private Data for its own purposes [...] cannot be used on behalf of
RPC-over-RDMA version 1") that seems in conflict with the goal of using the
Format Identifier "magic word" to identify the RPC-over-RDMA private data
within a larger assembly.  So we should probably try to resolve the
apparent inconsistency.

> Can you elaborate on the risks that might arise if we were to keep

Not really; it just seemed like it would make future implementations
simpler if they could make that assumption (and the 32-bit Format
Identifier should provide enough flexibility to freely make new formats if
needed).  But the current text doesn't seem flawed, to me.

> on those alternate fabrics, but perhaps I am "too close to the

I don't read it that way, either; I was just noting that it was surprising
that this is the only reference available.  If it is, then it is, though,
so no need to change anything.

> I can revisit RFC 5042 and extract those references.

I don't think you need to bother pulling in the specific section
references; I was just suggesting a minor tweak to something like "are
indicated by the Security Considerations section of [RFC5042]".

> further at risk. I'm open to suggested rewording.

I think the key points are: "Exchanging the data contained in the Private
Message format defined in this document does not directly expose any
upper-layer payloads to an attacker; furthermore, the behavior changes that
may occur as a result of processing the Private Data do not introduce any
new risk of exposure of upper-layer payload data to an attacker, since the
RDMA transport protection continues to apply, and misconfiguration results
only in disconnection".

> I will see about mentioning this failure mode in the text.

Thanks.

> they are likely to look like a payload. I think we are agreeing here.

I agree :)

> registered format. That might be challenging for DEs to carry out in

I don't think we need to require the DEs to perform an exhaustive analysis
that any given value is provably not possible to occur in a compliant
implementation of any other extant payload type (that sounds really hard!).
Mostly I just want it written down that that's a risk to keep in mind when
making new allocations.

> AI: Chuck

Thanks,

Ben

---

From: Chuck Lever <[email-masked]>
Date: Wed, 26 Feb 2020 08:33:21 -0800

Hi Ben-

> On Feb 20, 2020, at 11:51 AM, Benjamin Kaduk <[email-masked]> wrote:
> 
> Hi Chuck,
> 

I want to make sure you are happy with how I addressed this comment.
The third paragraph of Section 4 in -08 now reads:

   For RPC-over-RDMA version 1, the CM Private Data field is formatted
   as described in the following subsection.  RPC clients and servers
   use the same format.  If the capacity of the Private Data field is
   too small to contain this message format or the underlying RDMA
   transport is not managed by a Connection Manager, the CM Private Data
   field cannot be used on behalf of RPC-over-RDMA version 1.

I've removed the conflicting item from the last sentence.

Please confirm this is a satisfactory solution.

--
Chuck Lever

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 26 Feb 2020 11:43:30 -0800

Hi Chuck,

> Please confirm this is a satisfactory solution.

That should work.

Thanks!

-Ben