Telechat Review of draft-ietf-nfsv4-minorversion2-40
review-ietf-nfsv4-minorversion2-40-genart-telechat-davies-2016-01-22-00

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