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

[Tom Haynes <[e-mail]>]

Hi Elwyn,

Comments inline,

Hi Andy,

Look for [Andy]

> On Jan 16, 2016, at 12:09 PM, Elwyn Davies <[e-mail]> wrote:
> 
> 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.
> 
> 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.

^^ I should have peeked ahead!

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

I’ve fixed all of the royal we’s I found!

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

Done

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

done

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

done earlier

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

done earlier
> 
> 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),
> 

I think this is Table 5



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


Done earlier

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

[Andy]???

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

Done

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

Taken