Subject: [Gen-art] Gen-art telechat review of draft-ietf-nfsv4-minorversion2-40 - update

From: Elwyn Davies <[email-masked]>
Date: Sun, 17 Jan 2016 23:15:21 +0000

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair. Please wait for direction from your
document shepherd or AD before posting a new version of the draft.

For more information, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-nfsv4-minorversion2-40.txt
Reviewer: Elwyn Davies
Review Date: 2016/01/16
IETF LC End Date: 2015/12/09
IESG Telechat date: 2016/01/21

Summary: Almost ready.  Thank you for addressing almost all the issues 
that I raised in my last call review.  A couple of additional points 
have arisen as documented below.  Also I missed the usage 'we' 
phraseology on the first pass and there are a couple of typos that 
appeared in the modified text of -40.

Update: I forgot that the last call discussion covered the (non-)reuse 
of the shared secret random number used in the structured privilege data 
structures.  A comment in this has not made it into -40.  See below.

Major issues:

Minor issues:
s4.10.1.1.1, bullet #2: A late-breaking issue with RPCSEC_GSS v3 was 
raised just prior to the last IESG meeting (see email [1] quoted below). 
  I think the requirement to use QoP rpc_gss_svc_privacy for at least 
the privileges copy_from_auth and copy_to_auth for other reasons (the 
shared secret being carried) effectively mitigates the problem 
identified which relates to multi-principal authorization. However I am 
not clear if the problem would apply to the third privilege defined in 
this document (copy_confirm_auth_priv).  If it does then presumably 
extending the use of the privacy QoP to all the privileges would 
mitigate the problem.  As I understand it there is ongoing discussion of 
the appropriate changes needed in the RPCSEC_GSS v3 draft and there is a 
possibility that fixes applied there might have a knock-on effect in 
this draft:  Please liaise with the authors of draft-ietf-nfscv4-gssv3.

s4.10.1.1.1, bullet #2 (again):  The LC review asked about the 
generation and reuse of the random number used as a shared secret.  I 
understood that the author's view was that it should not be reused for 
more than one copy.  I expected that a note to this effect would be 
included but I don't currently see one.

====================================

Nits/editorial comments:
General: I also missed a number of instances (17, I think) of the "we 
<do something>" construction familiar from scientific papers.   This is 
not appropriate phraseology for an RFC and needs to be changed to avoid 
the "we", e..g.,
s1.4.5: s/We introduce WRITE_SAME (see Section 15.12)/The WRITE_SAME 
operation (see Section 15.12) is introduced/

Genera: I realized that there is no general terminology section in this 
document.  Clearly most of it is taken over from either or both of RFC 
7530 (s1.5) and RFC 5661 (s1.6).  What triggered this was the point that 
stateid isn't actually defined in this doc.  A reference to one or both 
of these and/or possibly some copies of definitions would be helpful.

s2, last para: s/metadata sever/metadata server/

s3.3: s/E.g., as per Section 16.2.3 of [RFC5661],/For example, as per 
Section 16.2.3 of [RFC5661],/

s4.1: Removing the s4.1 header would be in keeping with usual style as 
you have already done for other sections.

s4.2, para 2: s/intra-sever/intra-server/

s4.4.2, para 1:
OLD:
Other operations are OPTIONAL in the context of a particular feature 
Section 13,
NEW:
Other operations are OPTIONAL in the context of a particular feature 
(see Table 6 in Section 13),

s4.9, last para:
I was supposed to be letting you know if some extra explanation of why 
seqid being zero is ambiguous.... so, yes, I do think a bit extra is 
needed. Here goes:

> s15.8.3 notes that there can be multiple file copies associated with a
> single file going on at the same time.  This is only implicit up to
> that point I think.  It would be helpful to add a note about this
> possibility and the availability of asynchronous copy in general to
> the intro of section 4.
>
> In the following I may not have exactly grokked what the copy offload
> stateid represents... if so please adjust the words
>
> Add to intro (was in s4.1, s/b in s4) as new last para:
> ADD:
> The copy feature allows the server to perform the copying either
> synchronously or asynchronously.  The client can request synchronous
> copying but the server may not be able to honor this request.  If the
> server intends to perform asynchronous copying, it supplies the client
> with a request identifier that  the client can use to monitor the
> progress of the copying and, if appropriate, cancel a request in
> progress.   The request identifier is a stateid representing the
> internal locks held by the server while the copying is performed.
> Multiple asynchronous copies of all or part of a file may be in
> progress in parallel on a server; the stateid request identifier
> allows monitoring and canceling to be applied to the correct request.
> END
>
> Then modify the last para of s4.9:
> OLD:
>    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>    copy offload operation, it is ambiguous to indicate the most recent
>    copy offload operation using a stateid with seqid of zero. Therefore
>    a copy offload stateid with seqid of zero MUST be considered invalid.
> NEW:
>    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>    copy offload operation, it is inappropriate to indicate "the most
> recent
>    copy offload operation" using a stateid with seqid of zero (see
> Section 8.2.2
>    of [RFC5661] for the meaning of a seqid of zero).  It is inappropriate
>    because the stateid refers to internal state in the server and
> there may
>    be several asynchronous copy operations being performed in parallel
>    on the same file by the server.  Therefore
>    a copy offload stateid with seqid of zero MUST be considered invalid.
> END

s4.10, para 2:  Is it essential that every server implements all three 
structured privileges?  As I understand the specification, a server that 
only acted as a source would only need copy_from_auth whereas a server 
that only acted as a destination would only need copy_to_auth  and 
copy_confirm_auth privileges.  Presumably this could alternatively be 
covered by appropriate policies in a server that implemented all three.. 
I am not sure whether the error responses would be clearer if the 
implementation was missing or the policy was used.  Is this worthy of a 
comment?

s4.10.1.1, para 3: s/This features allow/This feature allows/

s4.10.1.1: Some explanatory text has been added to the specification of 
structured privileges in draft-ietf-nfsv4-rpcsec-gssv3-15.  I suggest 
that some minor updates to s4.10.1.1 should be  made to tie in with this 
specification.  In particular minorversion2 needs to specify how the 
data structure is encoded as specified in the GSS draft - RPCSEC_GSSv3 
doesn't know or care since it is treated as opaque data at the GSS 
level.  Clearly, for NFSv4.2, it is intended that XDR encoding is used 
but this should be stated explicitly.   I suggest adding a new para 
after the existing para 3 and making it clear that the string at the 
beginning of each section is passed in the rp_name field (also alter the 
"We define" which is not the correct style) :
OLD (para 4):

    We define three RPCSEC_GSSv3 structured privilege assertions that
    work in tandem to authorize the copy:

NEW:
    For each structured privilege assertion defined by a RPC application
    RPCSEC_GSSv3 requires the application to define a name string and a
    data structure that will be encoded and passed between client and server
    as opaque data.  For NFSv4 the data structures specified below MUST
    be serialized using XDR.

    Three RPCSEC_GSSv3 structured privilege assertions that
    work together to authorize the copy are defined here.  For each of
    the assertions the description starts with the name string passed in
    the rp_name field of the rgss3_privs structure defined in
    Section 2.7.1.4 of [rpcsec_gssv3] and specifies the XDR encoding of
    the associated structured data passed via the rp_privilege field of
    the structure.
END

---

From: Jari Arkko <[email-masked]>
Date: Thu, 21 Jan 2016 13:40:45 +0100

Thanks for your detailed (as always!) review, Elwyn.

I definitely agree that there should be a note about non-reuse of the random number.

Jari

On 18 Jan 2016, at 00:15, Elwyn Davies <[email-masked]> wrote:

> Summary: Almost ready.  Thank you for addressing almost all the issues that I raised in my last call review.  A couple of additional points have arisen as documented below.  Also I missed the usage 'we' phraseology on the first pass and there are a couple of typos that appeared in the modified text of -40.
> 
> Update: I forgot that the last call discussion covered the (non-)reuse of the shared secret random number used in the structured privilege data structures.  A comment in this has not made it into -40.  See below.
> 
> Major issues:
> 
> Minor issues:
> s4.10.1.1.1, bullet #2: A late-breaking issue with RPCSEC_GSS v3 was raised just prior to the last IESG meeting (see email [1] quoted below).  I think the requirement to use QoP rpc_gss_svc_privacy for at least the privileges copy_from_auth and copy_to_auth for other reasons (the shared secret being carried) effectively mitigates the problem identified which relates to multi-principal authorization. However I am not clear if the problem would apply to the third privilege defined in this document (copy_confirm_auth_priv).  If it does then presumably extending the use of the privacy QoP to all the privileges would mitigate the problem.  As I understand it there is ongoing discussion of the appropriate changes needed in the RPCSEC_GSS v3 draft and there is a possibility that fixes applied there might have a knock-on effect in this draft:  Please liaise with the authors of draft-ietf-nfscv4-gssv3.
> 
> s4.10.1.1.1, bullet #2 (again):  The LC review asked about the generation and reuse of the random number used as a shared secret.  I understood that the author's view was that it should not be reused for more than one copy.  I expected that a note to this effect would be included but I don't currently see one.
> 
> General: I also missed a number of instances (17, I think) of the "we <do something>" construction familiar from scientific papers.   This is not appropriate phraseology for an RFC and needs to be changed to avoid the "we", e..g.,
> s1.4.5: s/We introduce WRITE_SAME (see Section 15.12)/The WRITE_SAME operation (see Section 15.12) is introduced/
> 
> Genera: I realized that there is no general terminology section in this document.  Clearly most of it is taken over from either or both of RFC 7530 (s1.5) and RFC 5661 (s1.6).  What triggered this was the point that stateid isn't actually defined in this doc.  A reference to one or both of these and/or possibly some copies of definitions would be helpful.
> 
> s3.3: s/E.g., as per Section 16.2.3 of [RFC5661],/For example, as per Section 16.2.3 of [RFC5661],/
> 
> s4.1: Removing the s4.1 header would be in keeping with usual style as you have already done for other sections.
> 
> Other operations are OPTIONAL in the context of a particular feature Section 13,
> NEW:
> Other operations are OPTIONAL in the context of a particular feature (see Table 6 in Section 13),
> 
> s4.9, last para:
> I was supposed to be letting you know if some extra explanation of why seqid being zero is ambiguous.... so, yes, I do think a bit extra is needed. Here goes:
> 
> s4.10, para 2:  Is it essential that every server implements all three structured privileges?  As I understand the specification, a server that only acted as a source would only need copy_from_auth whereas a server that only acted as a destination would only need copy_to_auth  and copy_confirm_auth privileges.  Presumably this could alternatively be covered by appropriate policies in a server that implemented all three.. I am not sure whether the error responses would be clearer if the implementation was missing or the policy was used.  Is this worthy of a comment?
> 
> s4.10.1.1: Some explanatory text has been added to the specification of structured privileges in draft-ietf-nfsv4-rpcsec-gssv3-15.  I suggest that some minor updates to s4.10.1.1 should be  made to tie in with this specification.  In particular minorversion2 needs to specify how the data structure is encoded as specified in the GSS draft - RPCSEC_GSSv3 doesn't know or care since it is treated as opaque data at the GSS level.  Clearly, for NFSv4.2, it is intended that XDR encoding is used but this should be stated explicitly.   I suggest adding a new para after the existing para 3 and making it clear that the string at the beginning of each section is passed in the rp_name field (also alter the "We define" which is not the correct style) :
> OLD (para 4):
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art