[Gen-art] Gen-art LC review of draft-ietf-nfsv4-minorversion2-39

[Elwyn Davies <[e-mail]>]

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.