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

"Adamson, Andy" <William.Adamson@netapp.com> Tue, 22 December 2015 16:32 UTC

Return-Path: <William.Adamson@netapp.com>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C1E831A86F6; Tue, 22 Dec 2015 08:32:38 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -5.711
X-Spam-Level:
X-Spam-Status: No, score=-5.711 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, J_CHICKENPOX_12=0.6, J_CHICKENPOX_29=0.6, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] autolearn=ham
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZYixxu82RWL5; Tue, 22 Dec 2015 08:32:34 -0800 (PST)
Received: from mx143.netapp.com (mx143.netapp.com [216.240.21.24]) (using TLSv1.2 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D6D881A6F07; Tue, 22 Dec 2015 08:32:33 -0800 (PST)
X-IronPort-AV: E=Sophos;i="5.20,465,1444719600"; d="scan'208";a="86588268"
Received: from hioexcmbx02-prd.hq.netapp.com ([10.122.105.35]) by mx143-out.netapp.com with ESMTP; 22 Dec 2015 08:27:33 -0800
Received: from HIOEXCMBX03-PRD.hq.netapp.com (10.122.105.36) by hioexcmbx02-prd.hq.netapp.com (10.122.105.35) with Microsoft SMTP Server (TLS) id 15.0.1130.7; Tue, 22 Dec 2015 08:27:33 -0800
Received: from HIOEXCMBX03-PRD.hq.netapp.com ([::1]) by hioexcmbx03-prd.hq.netapp.com ([fe80::d0b6:c2cf:8cbc:16b8%21]) with mapi id 15.00.1130.005; Tue, 22 Dec 2015 08:27:33 -0800
From: "Adamson, Andy" <William.Adamson@netapp.com>
To: "Adamson, Andy" <William.Adamson@netapp.com>
Thread-Topic: Gen-art LC review of draft-ietf-nfsv4-minorversion2-39
Thread-Index: AQHROhW95CLCz+Up80SePlrWjOoEj57XrSsAgAAQFYA=
Date: Tue, 22 Dec 2015 16:27:32 +0000
Message-ID: <797D56D1-84EE-4754-A952-80C3EA894C1A@netapp.com>
References: <566DC57E.5030307@dial.pipex.com> <5E3C518C-397B-435A-AF23-71AF6B69B71E@primarydata.com> <73522861-AA88-44B2-AC21-0B2B1B7C10E5@netapp.com>
In-Reply-To: <73522861-AA88-44B2-AC21-0B2B1B7C10E5@netapp.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-mailer: Apple Mail (2.2098)
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.122.56.79]
Content-Type: text/plain; charset="utf-8"
Content-ID: <55B2D866AF35904C90BD643B8D84A41D@hq.netapp.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <http://mailarchive.ietf.org/arch/msg/gen-art/_6vV4eBZjUwNx4zxM9EAsK5oTRk>
Cc: "draft-ietf-nfsv4-minorversion2.all@ietf.org" <draft-ietf-nfsv4-minorversion2.all@ietf.org>, General area reviewing team <gen-art@ietf.org>, Tom Haynes <thomas.haynes@primarydata.com>
Subject: Re: [Gen-art] Gen-art LC review of draft-ietf-nfsv4-minorversion2-39
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 22 Dec 2015 16:32:39 -0000

> On Dec 22, 2015, at 10:30 AM, Adamson, Andy <William.Adamson@netapp.com> wrote:
> 
>> 
>> On Dec 18, 2015, at 11:28 PM, Tom Haynes <thomas.haynes@primarydata.com> wrote:
>> 
>> 
>>> On Dec 13, 2015, at 11:22 AM, Elwyn Davies <elwynd@dial.pipex.com> 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 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.
>>> 
>> 
>> 
>> Hi Elwyn,
>> 
>> Thanks as always for making me think!
>> 
>> Replies are inline, I have some questions for you and Andy.
>> 
>> Hi Andy,
>> 
>> There is a question about Inter-Server Copy Security below!
>> 
>> 
>>> 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?
>> 
>> Actually, from Section 5.8.1.2 of RFC 5661:
>> 
>>   o  The phrases "is an ordinary file" and "is a regular file" mean
>>      that the object's type attribute is NF4REG or NF4NAMEDATTR.
>> 
>> (I’ll quote from RFC 5661 and not RFC 7530 because of the pnfs aspects.)
>> 
>>> 
>>> 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.
>> 
>> 
>> Good question - I think even though we’ve published the Labeled NFS to allow for heterogeneous MAC deployments, sites are going to run a specific variant.
>> 
>> In that model, I would expect the object label to travel with the object and still be applied on the destination until modified by someone with those capabilities.
>> 
>> For the other model, the object label would control whether the file could be copied to another system or not. And if that system had a different MAC model, then according to the requirements in Section 3.3 of RFC 7204:
>> 
>>   A negotiation scheme SHOULD be provided, allowing systems from
>>   different Label Formats to agree on how they will interpret or
>>   translate each other's foreign labels.  Multiple concurrent
>>   agreements may be current between a server and a client.
>> 
>> 
>> 
>>> 
>>> 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.
>> 
>> Back to RFC 7530, 3.2.1.1, when we had a similar discussion in the WG about the required algorithms fodeplying Kerberos:
>> 
>>   At the time this document was specified, the Advanced Encryption
>>   Standard (AES) with HMAC-SHA1 was a required algorithm set for
>>   Kerberos V5.  In contrast, when NFSv4.0 was first specified in
>>   [RFC3530], weaker algorithm sets were REQUIRED for Kerberos V5, and
>>   were REQUIRED in the NFSv4.0 specification, because the Kerberos V5
>>   specification at the time did not specify stronger algorithms.  The
>>   NFSv4 specification does not specify required algorithms for Kerberos
>>   V5, and instead, the implementer is expected to track the evolution
>>   of the Kerberos V5 standard if and when stronger algorithms are
>>   specified.
>> 
>> 3.2.1.1.1.  Security Considerations for Cryptographic Algorithms in
>>            Kerberos V5
>> 
>>   When deploying NFSv4, the strength of the security achieved depends
>>   on the existing Kerberos V5 infrastructure.  The algorithms of
>>   Kerberos V5 are not directly exposed to or selectable by the client
>>   or server, so there is some due diligence required by the user of
>>   NFSv4 to ensure that security is acceptable where needed.  Guidance
>>   is provided in [RFC6649] as to why weak algorithms should be disabled
>>   by default.
>> 
>> I.e., I would prefer to not specify a policy on size and reuse of the random number as a shared secret. Well, strike that, the random number should not be reused and the length is determined by the algorithm used to generate that secret.
>> 
>> I can craft text to that effect.
>> 
>> 
>>> 
>>> Nits/editorial comments:
>>> General: bytes or octets?
>> 
>> RFC 5661:
>> 
>>   Byte:  In this document, a byte is an octet, i.e., a datum exactly 8
>>      bits in length.
>> 
>> So bytes.
>> 
>>> 
>>> Abstract: Probably ought to expand i/O.
>> 
>> done
>> 
>>> 
>>> 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.
>> 
>> done
>> 
>> 
>>> 
>>> s1.2, 3rd bullet: s/protocols.  I.e.,/protocols, that is/; s/apply/apply only/
>> Done
>> 
>> 
>>> 
>>> 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.
>>> 
>> 
>> 1.3.7.  Layout Enhancements
>> 
>>   In the parallel NFS implementations of NFSv4.1 (see Section 12 of
>>   [RFC5661]), the client cannot communicate back to the metadata server
>>   any errors or performance characteristics with the storage devices.
>>   NFSv4.2 provides two new operations to do so respectively: LAYOUTERROR
>>   (see Section 15.6) and LAYOUTSTATS (see Section 15.7).
>> 
>>> s1.4: Similarly, there is noting about the CLONE operation.
>> 
>> 1.3.1.  Server Side Clone and Copy
>> 
>>   A traditional file copy of a remotely accessed file, whether from one
>>   server to another or between locations in the same server, results in
>>   the data being put on the network twice - source to client and then
>>   client to destination.  New operations are introduced to allow
>>   unnecessary traffic to be eliminated:
>> 
>>   o  The intra-server clone feature allows the client to request a
>>      synchronous cloning, perhaps by copy-on-write semantics.
>> 
>>   o  The intra-server copy feature allows the client to request the
>>      server to perform the copy internally, avoiding unnecessary
>>      network traffic.
>> 
>>   o  The inter-server copy feature allows the client to authorize the
>>      source and destination servers to interact directly.
>> 
>>> 
>>> s1.4.1, para 1: s/remotely accessed/remotely accessed file/; s/location/locations/
>> 
>> done
>> 
>>> 
>>> 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.
>>> 
>> 
>> Done
>> 
>> 
>> 
>>> s1.4.2: (as with the Abstract) expand I/O on first occurrence.
>> 
>> Done
>> 
>>> 
>>> s1.4.2: It would be worth putting in the reference for posix_fadvise here.
>> 
>> Done
>> 
>>> 
>>> s1.4.3: If you write the data in the hole, it isn't really a hole ;-) ….
>> 
>> Ha!
>> 
>>> 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
>> 
>> Done
>> 
>>> 
>>> s1.5: Suggest s/I.e., READ becomes either/For instance, READ would have to be replaced or supplemented by, say, either/
>> 
>> Done
>> 
>>> 
>>> 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)
>> 
>> Done
>> 
>>> 
>>> 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.
>>> 
>> 
>> Done
>> 
>>> 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
>> 
>> 
>> done
>> 
>>> 
>>> s4.2.2, para 5:  s/support all these operations/support these operations/ ('all' is confusing given that only two are then explicitly mentioned).
>>> 
>> 
>> 
>> done - BTW note that we had some WG revisions made to introduce the CLONE operation here. Those
>> changes are considered part of this review cycle. In other words, please let me know when you want
>> a revised draft copy published.
>> 
>> 
>>> s4.3, para 1:  s/Inter-server copy is driven/The specification of inter-server copy is driven/
>>> 
>> 
>> done
>> 
>>> s4.4.1, last para: s/The source MUST equate/The source MUST interpret/
>> 
>> done
>> 
>>> 
>>> s4.6/Figures 4 and 5: Need a 'key' to explain os1 and os2.
>> 
>> Done in that I expanded the names in the figure and also made it clear which wa being released
>> 
>>> 
>>> s4.7.2:  Adding a reference to Section 15.3(.3) would help.
>> 
>> Went with 15.3 - let me know if you’d like 15.3.3 better. 
>> 
>> 
>>> 
>>> s4.7.3, para 2: idnits identifies RFC 2616 as obsoleted by RFC 7230, etc.  A more recent ref is desirable.
>> 
>> Done
>> 
>>> 
>>> s4.8, para after code fragment: LDAP and NIS  need to be expanded (DNS and URL are well-known).
>> 
>> Done
>> 
>>> 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.
>>> 
>> 
>> The stateid might be that of a lock, which has the provision that it cannot be 0
>> 
>> Section 8.2.2 of RFC 5661:
>> 
>>   When such a set of locks is first created, the server returns a
>>   stateid with seqid value of one.  On subsequent operations that
>>   modify the set of locks, the server is required to increment the
>>   "seqid" field by one whenever it returns a stateid for the same
>>   state-owner/file/type combination and there is some change in the set
>>   of locks actually designated.  In this case, the server will return a
>>   stateid with an "other" field the same as previously used for that
>>   state-owner/file/type combination, with an incremented "seqid" field.
>>   This pattern continues until the seqid is incremented past
>>   NFS4_UINT32_MAX, and one (not zero) is the next seqid value.
>> 
>> Let me know if you want a citation here (and a little bit of explanatory text).
>> 
>> Note, RFC 5661 uses “zero” and not “0”. I’ve changed this text to match.
>> 
>>> 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.
>>> 
>> 
> 
> The succinct definition is here:
> 
> From draft-ietf-nfsv4-rpcsec-gssv3-14 Section 2.7.1.4.  Structured Privilege Assertions first sentence, first paragraph:
> 
> "A structured privilege is an RPC application defined privilege.” 
> 
> and at the end of the first paragraph:
> 
> "Encoding, server verification and any policies for structured privileges are described by the RPC application definition.”
> 
> NFSv4.2 is the RPC application that defines the inter server to server copy structured privileges.
> 
> There is really nothing else to add to the definition.
> 
>> ^^ Andy???
>> 
>> 
>>> s4.10.1.1.1, 2nd bullet: Need to expand QOP.
>> 
>> 
>> Andy, RFC 2203 uses this term, but does not expand it.
> 
> Actually, it does in a very round-about manner.
> 
> rfc2203:
> 
> 5.2.1.  Mechanism and QOP Selection
> 
>   There is no facility in the RPCSEC_GSS protocol to negotiate GSS-API
>   mechanism identifiers or QOP values. At minimum, it is expected that
>   implementations of the RPCSEC_GSS protocol provide a means to:
> 
>   *    specify mechanism identifiers, QOP values, and RPCSEC_GSS
>        service values on the client side, and to
> 
>   *    enforce mechanism identifiers, QOP values, and RPCSEC_GSS
>        service values on a per-request basis on the server side.
> 
>   It is necessary that above capabilities exist so that applications
>   have the means to conform the required set of required set of
>   <mechanism, QOP, service> tuples (See the section entitled Set of GSS-API Mechanisms).
>                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> 6.  Set of GSS-API Mechanisms
> 
>   RPCSEC_GSS is effectively a "pass-through" to the GSS-API layer, and
>   as such it is inappropriate for the RPCSEC_GSS specification to
>   enumerate a minimum set of required security mechanisms and/or
>   quality of protections.
> 
> so QOP is "Quality of Protection"
> 
>> And RFC 5403 does not use it.
>> 
>> It is a field in one of the structures?
> 
> Here is are some references:
> 
> rfc2743  "Generic Security Service Application Program Interface Version 2, Update 1"
> 
> 
> Section 1.2.4:  Quality of Protection
> 
>   Some mech_types provide their users with fine granularity control
>   over the means used to provide per-message protection, allowing
>   callers to trade off security processing overhead dynamically against
>   the protection requirements of particular messages. A per-message
>   quality-of-protection parameter (analogous to quality-of-service, or
>   QOS) selects among different QOP options supported by that mechanism.
>   On context establishment for a multi-QOP mech_type, context-level
>   data provides the prerequisite data for a range of protection
>   qualities.
> 
> 
> 
> 
> rfc4121: "The Kerberos Version 5 Generic Security Service Application Program Interface (GSS-API) Mechanism: Version 2"
> 
> Section 3.  Quality of Protection
> 
> 
>   The GSS-API specification [RFC2743] provides Quality of Protection
>   (QOP) values that can be used by applications to request a certain
>   type of encryption or signing.  A zero QOP value is used to indicate
>   the "default" protection; applications that do not use the default
>   QOP are not guaranteed to be portable across implementations, or even
>   to inter-operate with different deployment configurations of the same
>   implementation.  Using a different algorithm than the one for which
>   the key is defined may not be appropriate.  Therefore, when the new
>   method in this document is used, the QOP value is ignored.
> 
>            ……...
> 
> 
>> 
>>> 
>>> s4.10.1.2, para 2: s/uiniquely/uniquely/
>> 
>> Done
>> 
>>> 
>>> s5, title: s?IO?I/O?
>> 
>> fixed
>> 
>>> 
>>> s6.1: This heading could be deleted turning the text in 6.1 into Section 6 which is then not empty.
>>> 
>> 
>> Done
>> 
>> 
>>> 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.
>> 
>> 
>> Let me try to weasel!
>> 
>> I’ve related it to metadata.
>> 
>> 
>>> 
>>> s6.1, para 2: s/can thought as holes/can thought of as holes/
>> 
>> can be thought of as holes
>> 
>> 
>>> 
>>> s6.1, para 4:  s/couple/few/  ('couple hundred' is a USA specific colloquialism - UK would use 'couple of’)
>> 
>> done
>> 
>>> 
>>> s6.1, last para: s/punching.  I.e., an application/punching, where an application/
>>> 
>> 
>> done
>> 
>> 
>>> s7, para 1: s/One example is the posix_fallocate/One example is the posix_fallocate operation/
>>> 
>> 
>> Done
>> 
>>> 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.
>> 
>> Done
>> 
>> 
>>> 
>>> 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
>> 
>> Done
>> 
>>> 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?
>>> 
>> 
>> Not on purpose - does this work?
>> 
>>   Further, assume the application writes a single ADB at 16k, changing
>>   the guard pattern to 0xcafedead, we would then have in memory:
>> 
>>       0k ->   (4k - 1) : 00 00 00 00 ... fe ed fa ce 00 00 ... 00
>>       4k ->   (8k - 1) : 00 00 00 01 ... fe ed fa ce 00 00 ... 00
>>       8k ->  (12k - 1) : 00 00 00 02 ... fe ed fa ce 00 00 ... 00
>>      12k ->  (16k - 1) : 00 00 00 03 ... fe ed fa ce 00 00 ... 00
>>      16k ->  (20k - 1) : 00 00 00 04 ... ca fe de ad 00 00 ... 00
>>      20k ->  (24k - 1) : 00 00 00 05 ... fe ed fa ce 00 00 ... 00
>>      24k ->  (28k - 1) : 00 00 00 06 ... fe ed fa ce 00 00 ... 00
>>         ...
>>     396k -> (400k - 1) : 00 00 00 63 ... fe ed fa ce 00 00 ... 00
>> 
>> 
>>> s9.1:  Again it would be best to delete the title of s9.1 and leave the text as s9.
>>> 
>> 
>> Done
>> 
>> 
>>> s9.1, para 2: Expand ACL please.
>> 
>> Done
>> 
>>> 
>>> 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.
>> 
>> 
>> Done
>> 
>> 
>>> [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
> 
> 
> Starting with draft-ietf-nfsv4-rpcsec-gssv3-14, rfc7204 is an informative reference, and each reference includes a section number.
> 
>>> - 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.]
> 
> 
> the information that draft-ietf-nfsv4-rpcsec-gssv3-14 references in rfc7402 is ‘big picture’ info:


Tom - I’ll review Section 9.6 "MAC Security NFS Modes of Operation" of draft-ietf-nfsv4-minorversion2 and suggest some changes (if needed) so that draft-ietf-nfsv4-rpcsec-gssv3 can simply reference draft-ietf-nfsv4-minorversion2 for MAC security issues and get rid of the RFC7204 reference.

—>Andy
> 
> 1.  Introduction and Motivation
> 
>   Labeled NFS (see Section 9 of [NFSv4.2]) uses an MLS policy with
>   Mandatory Access Control (MAC) systems as defined in [RFC4949].
>   Labeled NFS stores MAC file object labels on the NFS server and
>   enables client Guest Mode MAC as described in Section 4.3 of
>   [RFC7204].  RPCSEC_GSSv3 label assertions assert client MAC process
>   subject labels to enable Full Mode MAC when combined with Labeled NFS
>   as described in Section 3.3 of [RFC7204].
> 
> 
> 1.1.  Added Functionality
>           ………..
>  o  Security labels for Full Mode security type enforcement, and other
>      labeled security models (See Section 5.5.1 in [RFC7204]).
> 
>  An option for enumerating server supported label format specifiers
>   (LFS) is provided.  See Section 2 and Section 3.3 in [RFC7204] for
>   detail.
> 
> 
>>>  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.
> 
> I think WRT this issue, draft-ietf-nfsv4-rpcsec-gssv3-14 does give everything an implementor of rpcsec-gssv3 neeeds to know.
> 
> —>Andy
> 
>> 
>> Andy???
>> 
>> 
>>> 
>>> s9.2, MLS definition: RFC 2401 has been obsoleted by RFC 4301... can this be referenced instead?
>> 
>> MLS appears in RFC 2401 and not RFC 4301.
>> 
>> But I wasn’t aware of RFC 4949 - do you want to use it here as well?
>> 
>> 
>>> 
>>> 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).
>> 
>> Oh, I don’t like using them at all. I think I was supposed to go back and expand them. :-)
>> 
>> 
>>> 
>>> 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.
>> 
>> 
>> Done on the first part.
>> 
>> And the newer versions of xml2rfc has allowed me to add the horizontal separators
>> 
>> 
>>> 
>>> 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.
>> 
>> Fixed as per the email on 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.
>>> 
>> 
>> 
>> Researching this one!  Will get back to you...
>> 
>> 
>>> 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.
>>> 
>> 
>> Yes, I think it was taken out in an earlier draft - striking.
>> 
>> Table numbers added
>> 
>> 
>>> s13, Operations table: Does not include IO_ADVISE
>> 
>> Added
>> 
>> 
>>> ( and technically doesn't include ILLEGAL, and CB_ILLEGAL isn't in Callbacks).
>> 
>> Added
>> 
>>> 
>>> 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.
>> 
>> Agreed
>> 
>>> 
>>> 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.
>> 
>> Done - added a reference for the COMMIT semantics and the packing. 
>> 
>>> 
>>> 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.
>>> 
>> 
>> 
>> Note that we have been trying to shift from pulling in everything every minor version to just the bare minimum.
>> 
>> And I’ll note you are saying I’m not at that bar. :-)
>> 
>> Still not sure...
>> 
>>> s19.2, [Quigley14]:  This is now RFC 7569.  Also I think this should be normative.
>> 
>> Done
>> 
>>> 
>>> 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.
>>> 
>>> 
>> 
>> I don’t have a copy either - during the work on RFC 7569, Ran Atkinson provided this citation in the review process. Perhaps you can ask him? (Ask me offline and I’ll provide his email.)