Last Call Review of draft-ietf-nfsv4-minorversion2-39
review-ietf-nfsv4-minorversion2-39-genart-lc-davies-2015-12-14-00

• Status
• Email expansions
• History

• Versions
• 00

Review
review-ietf-nfsv4-minorversion2-39-genart-lc-davies-2015-12-14

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.