> On Jan 22, 2016, at 6:11 PM, Tom Haynes <[email protected]> wrote: > > Hi Elwyn, > > Comments inline, > > Hi Andy, > > Look for [Andy] > >> On Jan 16, 2016, at 12:09 PM, Elwyn Davies <[email protected]> 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]???
Here is my answer from Jan 5th mail: > Well, I think it wiser to have it all or none - e.g. to always implement, and > let the policy define privileges appropriate to the role of the server. > Furthermore, once one structured privilege is coded, the others are not that > big a deal. So I think the existing text is good. Looking at this again, I still think there is no need for any more text on the subject - all three structured privileges are required. As far the error responses, I don’t think it matters if the implementation was missing or a policy was used as the caller will react in the same way (e.g. fail due to the error) for both cases. —>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 > _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
