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

Benjamin Kaduk <kaduk@mit.edu> Thu, 20 February 2020 19:52 UTC

Return-Path: <kaduk@mit.edu>
X-Original-To: nfsv4@ietfa.amsl.com
Delivered-To: nfsv4@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C32B6120072; Thu, 20 Feb 2020 11:52:04 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.199
X-Spam-Level:
X-Spam-Status: No, score=-4.199 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oHh8ZvT3-8n5; Thu, 20 Feb 2020 11:52:02 -0800 (PST)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id DE75B120018; Thu, 20 Feb 2020 11:52:01 -0800 (PST)
Received: from kduck.mit.edu ([24.16.140.251]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id 01KJptVL003726 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 20 Feb 2020 14:51:58 -0500
Date: Thu, 20 Feb 2020 11:51:55 -0800
From: Benjamin Kaduk <kaduk@mit.edu>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: The IESG <iesg@ietf.org>, draft-ietf-nfsv4-rpcrdma-cm-pvt-data@ietf.org, Tom Haynes <loghyr@gmail.com>, Spencer Shepler <spencer.shepler@gmail.com>, Brian Pawlowski <beepee@gmail.com>, nfsv4-chairs@ietf.org, nfsv4@ietf.org
Message-ID: <20200220195155.GJ97652@kduck.mit.edu>
References: <158216101788.17661.6926758160404035704.idtracker@ietfa.amsl.com> <E718C1B8-B96B-4A64-8B92-C4ABEC7B5E5F@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <E718C1B8-B96B-4A64-8B92-C4ABEC7B5E5F@oracle.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Archived-At: <https://mailarchive.ietf.org/arch/msg/nfsv4/n5Sqq2NKU1YSlbZSZYMT_blqpdA>
Subject: Re: [nfsv4] Benjamin Kaduk's No Objection on draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: (with COMMENT)
X-BeenThere: nfsv4@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: NFSv4 Working Group <nfsv4.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/nfsv4>, <mailto:nfsv4-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/nfsv4/>
List-Post: <mailto:nfsv4@ietf.org>
List-Help: <mailto:nfsv4-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/nfsv4>, <mailto:nfsv4-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 20 Feb 2020 19:52:05 -0000

Hi Chuck,

On Thu, Feb 20, 2020 at 10:13:26AM -0500, Chuck Lever wrote:
> Hi Ben, thanks for your comments! Responses and questions below.
> 
> 
> > On Feb 19, 2020, at 8:10 PM, Benjamin Kaduk via Datatracker <noreply@ietf.org> wrote:
> > 
> > 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.
> 
> 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.

Yes, that was it, and sounds good.

> AI: Chuck
> 
> 
> > 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"?
> 
> 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.

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.

> The "Interoperability Amongst RDMA Transports" section is intended
> to explain this. I'm happy to clarify that text if needed.
> 
> 
> > 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.
> 
> 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?

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.

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

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.

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

> AI: Chuck
> 
> 
> >   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.
> 
> 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.

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

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

Thanks.

> AI: Chuck
> 
> 
> >   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.
> 
> AI: Chuck
> 
> 
> > Section 7
> > 
> > Should the registry also include the length of the private data?
> 
> I can add that.
> 
> AI: Chuck
> 
> 
> > 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.
> 
> 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.

I agree :)

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

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