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

Spencer Dawkins at IETF <spencerdawkins.ietf@gmail.com> Mon, 07 August 2017 23:05 UTC

Return-Path: <spencerdawkins.ietf@gmail.com>
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 48F4A131CF0; Mon, 7 Aug 2017 16:05:13 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.699
X-Spam-Level:
X-Spam-Status: No, score=-2.699 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
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 SbwCWtLu3lKO; Mon, 7 Aug 2017 16:05:10 -0700 (PDT)
Received: from mail-yw0-x235.google.com (mail-yw0-x235.google.com [IPv6:2607:f8b0:4002:c05::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id A4E86131CBF; Mon, 7 Aug 2017 16:05:10 -0700 (PDT)
Received: by mail-yw0-x235.google.com with SMTP id u207so11555037ywc.3; Mon, 07 Aug 2017 16:05:10 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fxYdU+vUWadRswMUkePjjZI6gr2AIFpLShrNB/71Hy4=; b=o+HbcFmMaAMBO5iBkIzLH71do4lWxHCH2YGD0jIb3IVOb2sVqON4Qt0iv97C5wXuEQ CFUyK6KTq9xDI8g9ncXK66dOEd/3GIatUEz4JNnQ0Xw9FcElDK+0xYxVUv3G6m29gSJk 0uqudnJ7WcHYjLoKceBeYRZh8thMtxWPUZrezSWuAl70Wka6psHsid/cpmaQo59/CRHZ PSTQGyv0UhnlhDTh2EMtqy5+aQYcQb1TZsEMr2URo7RGxuLvg9WnoHkd5zBufgaoCXcP 8QNFv0dRBzQTIupXzeokGLyIxhm7UINspabveQRs8V+ncZyCSK8+IzoyTld6zdP1UNy7 +ArA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fxYdU+vUWadRswMUkePjjZI6gr2AIFpLShrNB/71Hy4=; b=cb2hMxFZ78qsiTYA6Mi9NPpbccWLpmA6lh4OiV8TVq2EaUYqoFcIj4fkMW2riX/8rq MhIkcHXoNINp+8fzXhKcykrobkWu9zJJ4TFHwOH85F5GKSrA21cFnlAxGfDNJR1vdlun ThPzUc0zhAlSG/xETjfSOPWkEfEPjizWDg3Qz491WPVuLKj2ebXyHqkHq8xuSXLUo12B KkhUD4YmgLBqnFZzNGwGhwtOT0nOXHoNXaFYOAG/kg9AxSqAGBwUVsVplbk9TyYoQuB8 oxhHG4KuiTWzrhkhppiXIxZRGq9hYLXJjsqfFSZV45agfHeK9YP8egpZFn8czSr3UrUj ETuQ==
X-Gm-Message-State: AHYfb5hmIyP8cbMxMk/lGpHMMCS0iHzjXiQUgrkOOCrBQN2yVXosaGQj 9Zy6+MQMgDiVCuCtYg+/JiV2hc2KBw==
X-Received: by 10.37.36.14 with SMTP id k14mr1706779ybk.211.1502147109638; Mon, 07 Aug 2017 16:05:09 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.37.52.79 with HTTP; Mon, 7 Aug 2017 16:05:08 -0700 (PDT)
In-Reply-To: <C6965A7A-3A9F-466B-AEB7-73A8C96F3A9D@oracle.com>
References: <CAKKJt-fFFyxV=VhJxhe=3P283xZQG_yvdP6AHJkzU6iB=74JCw@mail.gmail.com> <C6965A7A-3A9F-466B-AEB7-73A8C96F3A9D@oracle.com>
From: Spencer Dawkins at IETF <spencerdawkins.ietf@gmail.com>
Date: Mon, 7 Aug 2017 18:05:08 -0500
Message-ID: <CAKKJt-fbpBjz9=z8RqSGseKZxCY3e=rb6TsBZCEmNtPxKe0sdw@mail.gmail.com>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: draft-ietf-nfsv4-rfc5667bis@ietf.org, nfsv4-chairs@ietf.org, NFSv4 <nfsv4@ietf.org>
Content-Type: multipart/alternative; boundary="001a113d4580dee1e7055631e066"
Archived-At: <https://mailarchive.ietf.org/arch/msg/nfsv4/KryT27j3Qo3dOZgDiKbFbejlKig>
Subject: Re: [nfsv4] AD review of draft-ietf-nfsv4-rfc5667bis-11
X-BeenThere: nfsv4@ietf.org
X-Mailman-Version: 2.1.22
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: Mon, 07 Aug 2017 23:05:13 -0000

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 <chuck.lever@oracle.com> wrote:

>
> > On Aug 7, 2017, at 5:45 PM, Spencer Dawkins at IETF <
> spencerdawkins.ietf@gmail.com> 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