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 treat these comments just
like any other last call comments.
For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Document: draft-ietf-nfsv4-minorversion2-39.txt
Reviewer: Elwyn Davies
Review Date: 2015-12-13
IETF LC End Date: 2015-12-09
IESG Telechat date: (if known) -
Summary: Almost ready. There are a fair number of minor nits and
editorial suggestions. The couple of 'minor issues' are predominantly
questions that came into my mind that might be interesting to a
reader/implementer but are probably unlikely to affect the performance
of the protocol. However, the comments on s11.2, s12.1, s13 and s15.2.3
appear to be actual omissions or errors.
Major issues:
None.
Minor issues:
s4: There is no discussion in s4 of the Named Attributes associated
with a file and the restriction of server-to-server copy to 'regular
files' seems to indicate that Named Attributes cannot be copied by this
mechanism. Is there anything to be said about getting the Named
Attributes across?
s4/s9: Does server-to-server copy interact (badly, or otherwise) with
Labeled NFS? Nothing is said, but I wonder whether there are issues
around atomicity and MAC between the different servers for inter-server
copy.
s4.10.1.1.1: Size and reuse for random number used as a shared secret
for inter-server copy.
No advice is given on the desirable size/length of the random number or
the risks or otherwise of reusing the same context for multiple file
copies. This maybe a non-issue - I defer to those with more security
clue than me.
Nits/editorial comments:
General: bytes or octets?
Abstract: Probably ought to expand i/O.
s1/s1.1: I think you could safely delete the s1.1 header leaving the
text as the body of s1 - this is generally considered better practice
than leaving s1 itself empty.
s1.2, 3rd bullet: s/protocols. I.e.,/protocols, that is/; s/apply/apply
only/
s1.4: The overview doesn't cover the two LAYOUT related operations and
there is is no section describing what their usage might be in with
sections 4-10.
s1.4: Similarly, there is noting about the CLONE operation.
s1.4.1, para 1: s/remotely accessed/remotely accessed file/;
s/location/locations/
s1.4.1: (very nitty!) Suggest making the descriptions of the two cases
(intra- and inter-server) into bulleted paragraphs to make it clearer
that they are separate features.
s1.4.2: (as with the Abstract) expand I/O on first occurrence.
s1.4.2: It would be worth putting in the reference for posix_fadvise here.
s1.4.3: If you write the data in the hole, it isn't really a hole ;-) ....
OLD:
Such holes are typically transferred as 0s during I/O.
NEW:
Such holes are typically transferred as 0s when read from the file.
END
s1.5: Suggest s/I.e., READ becomes either/For instance, READ would have
to be replaced or supplemented by, say, either/
s2, last bullet: Need to expand pNFS on first use (and maybe remind
readers that it is a feature of NFS4.1 - Section 12 of RFC 5661)
s2, last bullet: s/metadata sever/metadata server/ - again a pointer to
(say) Sections 1.7.2.2 and 12.2.2 of RFC 5661 would clarify what a
metadata server is.
s4.2.1: The first para reads:
OLD:
COPY_NOTIFY: Used by the client to notify the source server of a
future file copy from a given destination server for the given
user. (Section 15.3)
I completely misread this on first pass, but I understand that it is
correct. Having checked with s15.3 and thought a bit more about it, it
is the 'copy from a given destination' that threw me - I guess it means
'the copy will be made from' rather than 'the file being copied from the
destination'... Doh!
A client sends this
operation in a COMPOUND request to the source server to authorize a
destination server identified by cna_destination_server to read the
file specified by CURRENT_FH on behalf of the given user.
I suggest the following might be avoid this brain fade:
NEW:
COPY_NOTIFY: Used by the client to request the source server to
authorize a
future file copy that will be made by a given destination server
on behalf of the given
user. (Section 15.3)
END
s4.2.2, para 5: s/support all these operations/support these
operations/ ('all' is confusing given that only two are then explicitly
mentioned).
s4.3, para 1: s/Inter-server copy is driven/The specification of
inter-server copy is driven/
s4.4.1, last para: s/The source MUST equate/The source MUST interpret/
s4.6/Figures 4 and 5: Need a 'key' to explain os1 and os2.
s4.7.2: Adding a reference to Section 15.3(.3) would help.
s4.7.3, para 2: idnits identifies RFC 2616 as obsoleted by RFC 7230,
etc. A more recent ref is desirable.
s4.8, para after code fragment: LDAP and NIS need to be expanded (DNS
and URL are well-known).
s4.9, para 3: Is it possible to add a little explanation as to *why*
seqid = 0 is ambiguous? My limited knowledge doesn't grok this.
s4.10: Is it possible to provide an abstract definition of 'structured
privilege'? The rpcsec-gssv3 draft appears to punt on this, pointing to
the 'example' in the NFSv4.2 draft. I think I get the idea but a
succinct definition would help I believe. I guess the definition ought
to be in the rpcsec-gss document and referenced from this doc.
s4.10.1.1.1, 2nd bullet: Need to expand QOP.
s4.10.1.2, para 2: s/uiniquely/uniquely/
s5, title: s?IO?I/O?
s6.1: This heading could be deleted turning the text in 6.1 into Section
6 which is then not empty.
s6.1: The term 'inode' is used four times in this section. Whilst this
is well understood, it is strictly associated with *nix file systems. It
would be desirable to find an alternative term or, if you can't think of
suitable short generic term (I spent a little time thinking about it and
couldn't think of one immediately), some weasel words added to the first
occurrence saying that it is intended to cover equivalent structures in
other sorts of filing system.
s6.1, para 2: s/can thought as holes/can thought of as holes/
s6.1, para 4: s/couple/few/ ('couple hundred' is a USA specific
colloquialism - UK would use 'couple of')
s6.1, last para: s/punching. I.e., an application/punching, where an
application/
s7, para 1: s/One example is the posix_fallocate/One example is the
posix_fallocate operation/
s7
OLD:
space_freed This attribute specifies the space freed when a file is
deleted, taking block sharing into consideration.
NEW:
space_freed This attribute reports the space that would be freed
when a file is
deleted, taking block sharing into consideration.
s8, bullet #2:
I found the 2nd sentence hard to parse. Suggested alternative below.
OLD:
2. Fields to describe the state of the ADB and a means to detect
block corruption. For both pieces of data, a useful property is
that allowed values be unique in that if passed across the
network, corruption due to translation between big and little
endian architectures are detectable. For example, 0xF0DEDEF0 has
the same bit pattern in both architectures.
NEW:
2. Fields to describe the state of the ADB and a means to detect
block corruption. For both pieces of data, a useful property
would be
that the allowed values are specially selected so that if passed
across the
network, corruption due to translation between big and little
endian architectures is detectable. For example, 0xF0DEDEF0 has
the same (32 wide) bit pattern in both architectures, making it
inappropriate.
END
PS: The example is relevant to 32 bit memory architectures... It has
never occurred to me to ask if there are 64 bit big and little endian
architectures! Well the IA-64 is bi-endian...
s8.3: The pictures of the memory patterns don't match the specification
in that the guard pattern appears to be at octet offset 4 in each ADB -
You can't tell where the checksum is from the pictures, but as specified
there would be a 4 octet gap between the guard pattern and the checksum
- I presume this is intentional. Would it be guaranteed to be zero?
s9.1: Again it would be best to delete the title of s9.1 and leave the
text as s9.
s9.1, para 2: Expand ACL please.
s9.1/s9.6.1.3: I am dubious about referring back to the requirements doc
[RFC7204] for definitions (rather than indicating what requirements were
met/not met) . For the ref in s9.1, I think that the MAC definition in
RFC 4949 would suffice. [I noted when reviewing the rpcsec-gssv3 draft a
couple of days ago that it makes *normative* reference to RFC 7204 -
this is even more undesirable - my feeling is that it should be possible
to find out what implementers need to know from the NFSv4.2 standard,
other standards or the security glossary as there is no certainty that
what is said in the requirements was actually put into the standard,
which could cause confusion for future implementers.] Is it possible to
get everything an rpcsec-gssv3 implementer needs to know into this
document? My impression is that it is almost all there already.
s9.2, MLS definition: RFC 2401 has been obsoleted by RFC 4301... can
this be referenced instead?
s9.4: Expand DS and MDS on first occurrence (these should probably go
back in s3 where metadata servers and data servers are referred to but
without the abbreviations).
s11.2, Table 2: The operation IO_ADVISE is missing from the table.
[CAVEAT: I don't claim to have checked the possible error codes!]
Aesthetically, this table would be better with horizontal separators
between the operation items.
s12.1, Table 4: The data types of clone_blksize (length4 vs uint32_t)
and space_freed (length4 vs uint64_t) do not match between this draft
and the XDR draft.
s12.2.3, NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR:
Monotonic is not really a good name for this I think. This is because
the technical definition is either non-increasing or non-decreasing so
that it would theoretically cover situations where the change attribute
doesn't actually change! IMO a better name would be
NFS4_CHANGE_TYPE_STRICTLY_INCR. This would imply that the value
actually increases whenever the info changes.
s12.2.3: With reference to the previous comment...
OLD:
If either NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR,
NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, or
NFS4_CHANGE_TYPE_IS_TIME_METADATA are set, then the client knows at
the very least that the change attribute is monotonically increasing,
which is sufficient to resolve the question of which value is the
most recent.
NEW:
If either NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR,
NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, or
NFS4_CHANGE_TYPE_IS_TIME_METADATA are set, then the client knows at
the very least that the change attribute is strictly increasing,
which is sufficient to resolve the question of which value is the
most recent.
END
s13, Operations table: The abbreviation EOL in the title line does not
seem to be expanded anywhere (and isn't used in the table either). It
would be good to have table numbers for the tables.
s13, Operations table: Does not include IO_ADVISE ( and technically
doesn't include ILLEGAL, and CB_ILLEGAL isn't in Callbacks).
s15.2.3:
If it cannot make that determination, it must set to false the one it
can and set to true the other.
The setting of true and false appears to be the inverse of the meaning
implied in the previous part of the section.
s15.5.6/s15.5.6.1: Pointers into the appropriate parts of the NFS v4.1
RFC would be helpful in giving the background needed to understand what
is going on here. The discussion of dense and sparse packing in
s15.5.6.1 is particularly obscure if you are not very well versed in
pNFS lore.
ss15.6 and 15.7: An overview of the LAYOUT* operations in the earlier
part of the document would be helpful, as mentioned above, plus some
pointers to parts of the NFSv4.1 document that ties in with the pNFS
layout story would be helpful.
s19.2, [Quigley14]: This is now RFC 7569. Also I think this should be
normative.
s19.2, [BL73]: Can you point me to a copy of this? I have found some
closely related work which was originally published as MITRE Technical
Report 2547 Vols I and II at
http://www.albany.edu/acc/courses/ia/classics/belllapadula1.pdf
(belllapadula2.pdf) and in the Journal of Computer Security but the
original doesn't seem to be visible.
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
