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
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
