Re: [nfsv4] WGLC review of draft-ietf-nfsv4-umask-02

[[e-mail] (J. Bruce Fields)]

By the way, thanks for the review--I'm working through the edits and
should have a reply soon (probably tomorrow).

--b.

On Sun, Nov 27, 2016 at 10:00:09AM -0500, David Noveck wrote:
> *General Comments*
> 
> *Overall Evaluation*
> 
> >From a technical point of view, this document has no issues that would
> prevent it from going forward fairly quickly.  Any technical issues are
> quite minor and should be easy to address.
> 
> Unfortunately, there are two significant issues regarding the presentation
> of this extension that need to be addressed:
> 
> 
>    - Right now this document makes no reference to the NFSv4 extension
>    framework.  It proposes something which, while sensible, is wholly at
>    variance with the text of RFC5661 and with the entire tenor of previous
>    standards-track treatment NFSv4 protocol extension.  More on this issue
>    below within *Presentation as Extension.*
>    - With regard to one critical reference it runs afoul of the
>    consequences of the working group's ambivalence about whether NFSv4.0 and
>    NFSv4.1 are best treated as two distinct protocols or as one. For the
>    specific reference, see *2. Problem Statement* which also discusses how
>    this issue might be addressed solely within the document under review.  For
>    other possibilities, see the discussion within *Potential Need for
>    Clarification Regarding post-NFSv4.0 Protocols*.
> 
> *Presentation as Extension*
> 
> The authors of the current document have chosen to avoid explicit reference
> to NFSv4 extension issues.  They seem to believe that because what they are
> proposing is sensible, addresses serious  protocol deficiencies, and can be
> implemented interoperably with existing clients and servers, the IESG is
> going to be OK with it.  I think this is too optimistic but feel that if
> the authors want to take this approach, I shouldn't stand in their way.  It
> may be that others in the working group will object to the document going
> forward in this form..
> 
> In some of the per-section comments I have offered suggestions on how the
> document could be modified to be compatible with the protocol extension
> model defined in draft-ietf-nfsv4-versioning.  Nevertheless, these are
> suggestions and the authors are free to decide on their own path in this
> respect.
> 
> *Potential Need for Clarification Regarding post-NFSv4.0 Protocols*
> 
> I feel that the working group has to eventually deal with some of
> the consequences of the fact that many necessary corrections/clarifications
> to NFSv4 were only applied/specified in RFC7530, and that an RFC5661bis
> could not be done in any reasonable time frame.  In the context of the
> current document these only concern  the discussion of mode/acl interaction
>  but eventually the working group is going to have to address the general
> issue.
> 
> In *2. Problem Statement*, I try to see what we can do to proceed on the
> current document without addressing the larger issue.  Whether it is
> appropriate to present that work to the IESG without addressing the larger
> issue is a matter for the authors and ultimately for the working group to
> decide.
> 
> To summarize  the longer-term issue, there are a number of areas where we
> have a clarification/correction that is appropriate and relevant to NFSv4.1
> that needed to be made and we have made the appropriate changes only
> with regard to NFSv4.0, in RFC7530.
> 
> Only the issue concerning the interactions of the various security-related
> attributes has relevance to this document.  I'll address the others that I
> know about later.
> 
> Since an rfc5661bis does not seem feasible in any reasonable period time,
> the best approach would be to produce a document updating RFC5661.  It
> could be based on chapter 6 of RFC7530, with appropriate changes to reflect
> the addition of the sacl, dacl, and set_mode_masked attributes.  The
> document could state that it supersedes chapter 6 of RFC5661.
> 
> *Comments by Section  *
> 
> *Header*
> 
> Note that this document, while it proposes an extension to NFSv4.2 and
> additions to the XDR for NFSv4.2, is not marked as updating RFCs 7862 and
> 7863.  The IESG is almost sure to ask about this.  You need to be prepared
> for this question.  My preference would be to build upon the versioning
> document but that's your choice.  Whatever you do, the question will be
> asked, perhaps repeatedly.
> 
> *Title*
> 
> I suggest capitalizing the words "inheritable", "override", and "umask".
> 
> *Abstract*
> 
> In the first sentence, the phrase "In some environments" seems too weak,
> given that this issue arises within Unix environments in general. Suggest
> "In many important environments" as a replacement.
> 
> Suggest the following replacement for the second sentence:
> 
> This issue can be addressed by transmitting the umask and create mode as
> separate pieces of data. Doing so via the protocol extension discussed in
> this document would allow servers to make intelligent decisions about the
> mode to be assigned to the new file.
> 
> *2. Problem Statement*
> 
> In the first sentence of the second paragraph, suggest replacing "currently
> does not" by "does not currently".
> 
> In the third sentence of the second paragraph, suggest replacing "on the
> safe side" by "on the the side of caution".
> 
> In the fourth sentence of the second paragraph, suggest replacing "Thus the
> mode" by "As a result, the mode attrbute".
> 
> The reference in the first sentence of the third paragraph may create
> difficulties since this document is directed to an extension within NFSv4.2
> and minor versions beyond NFSv4.0 essentially constitute a new protocol,
> making the RECOMMENDATIONS within RFC7530, officially speaking,
> non-normative. Unfortunately, when I looked for a parallel text in RFC5661,
> I didn't find one and am presuming that none exists.
> 
> In my suggestions below, I am going to attempt to dance around this issue
> as well as I can.  For other possibilities, see *Potential Need for
> Clarification Regarding post-NFSv4.0 Protocols*.
> 
> Suggest the following replacement for the third paragraph:
> 
> When applying the mode, section 6.4.1.1 of [RFC7530] RECOMMENDS that
> servers restrict permissions granted to any user or group named in the
> ACL so as to be no more extensive than the permissions granted by the
> MODE4_RGRP, MODE4_WGRP, and MODE4_XGRP bits.  While this
> RECOMMENDATION is not, strictly speaking, applicable to post-NFSv4.0
> protocols, NFSv4.1 server implementations have followed it.
> Apparently this is because sthe clarification within [RFC7530] applies
> to NFSv4.1 as well, since it also contains both mode and acl
> attributes which need to be coordinated. Also, servers aiming to
> provide clients with Unix-like chmod behavior may have been motivated
> by similar requirements in [SUSv4].  (See the discussion of additional
> and alternate access control mechanisms in Section 4.4 of that
> document, entitled "File Permissions".)
> 
> Suggest replacing the last sentence of the fourth paragraph by the
> following:
> 
> As a result, inherited ACEs describing the permissions to be granted
> to named users and groups are often ignored.
> 
> Suggest replacing the fifth and sixth paragraphs by the following:
> 
> This issue leads to file permissions which are more restrictive than
> they should be in common cases.  It appears that permission
> inheritance, included within NFSv4 as part ACL model, cannot be used
> effectively in many important environments and needs to be corrected.
> 
> To provide an appropriate correction, the server needs to apply the
> umask since depending on the client to do so has proved unworkable.
> 
> *Protocol Extension Considerations [Suggested new section]*
> 
> This document presents an extension to the NFSv4 protocol as described in
> [nfsv4-versioning].  It describes a new OPTIONAL feature and the extension
> is such as allow it to be implemented so that existing NFSv4.2 clients and
> servers, implemented without knowledge of this extensions will operate with
> severs and clients that are aware of this extension, including clients that
> use and do not use this extension and servers that support and do not
> support this extension.
> 
> Note that [RFC7862] does not define NFSv4.2 as non-extensible, so that it
> is considered by [nfsv4-versioning] to be an extensible minor version.  As
> a result, upon publication of this document as a Proposed Standard, the
> extension described herein will effectively be part of NFSv4.2, even though
> this document does not update [RFC7862] or [RFC7863].
> 
> *3. mode_umask Attribute*
> 
> Proposing the following as a new first paragraph for this section:
> 
> In order to allow the server to apply the umask, the new attribute
> mode_umask is used when creating objects.  This allows the server to
> apply the umask only when there are no inheritable permissions.
> 
> I'm not sure what you need to do with regard to XDR although I am fairly
> sure that what is there is not adequate. One possible approach is to follow
> the model used in draft-ietf-nfsv4-xattrs-03 but I can see why you might
> not want to do all of that, given that your XDR additions are so limited.
> 
> Nevertheless, I think you need to do the following:
> 
>    - Address the issue of code components licensing somehow.
>    - Describe how one can produce an XDR file for the extended protocol.
>    - Insert "<CODE BEGINS>" and <CODE ENDS> appropriately.
>    - Include XDR for the attribute code and typedef.
> 
> In the first sentence of the current first paragraph, the phrase "open
> mode" is potentially confusing.  I believe the reference is to the
> optional third parameter of the POSIX open call.  However, others
> might interpret "open mode" as referring to whether a file is opened
> for read or for write.  Suggest "mode value specified at open time" as
> a possible replacement.
> 
> With regard to the current second paragraph, the following issues need
> to be addressed:
> 
>    - I don't see any reason why this is a "SHOULD" (rather than a
> "MUST").  What else might a server do and how would interoperability
> be affected?
>    - Besides indicating that this be rejected, there should be an
> indication of how.  I suggest handling this like reading a write-only
> attribute and returning NFS4ERR_INVAL.
> 
> In the last bulleted paragraph (i.e. the current seventh paragraph),
> suggest replacing "the mode to use for creating the object" by "the
> mode to be assigned to the object being created"
> 
> *4.  Security Considerations*
> 
> In the second sentence of the first paragraph, suggest replacing "the
> recommendation that they SHOULD" by "the RECOMMENDATION that they".
> 
> Suggest replacing the second paragraph by the following:
> 
> The practice of ignoring the umask when there are inheritable
> permissions in the form of a "POSIX" default ACL is of long standing
> and has not given rise to security issues. The "POSIX" default ACL
> mechanism and the mechanism for permission inheiritance in NFSv4 are
> equivalent from a security perspective.
> 
> *IANA Considerations [Missing Section]*
> 
> Although it really doesn't make sense, you need something here.  Suggest
> adding the section with the following text:
> 
> This document does not require any actions by IANA.
> 
> *5.  Normative References*
> 
> Regarding POSIX-1003.1e, I don't see how a *withdrawn *POSIX draft can
> be consiered normative.  I think this should go in an "Informative
> References" section.
> 
> I note there is no explicit reference in the text to LEGAL but expect
> that once the XDR issues are resolved there will have to be,
> 
> Regarding RFCs 5661 and 5662, I note that neither is actually
> referenced in this document.  The details will depend on how you want
> to address the extension issue but I offer the following suggestions
> regaring version definition documents for post-v4.0 minor versions:
> 
> 
>    - Given that this document is to apply to the to the NFSv4.1 minor
> version group, it makes sense  to include this reference to RFC5661,
> even without an explicit reference in the text.  I'd leave this in,
> unless it becomes an issue with idnits.
>    - Even though this is an extension to NFSv4.2, I don't see a need
> to explicitly reference RFC7862, since none of the NFSV4.2 extensions
> has any relevance to this issue.
>    - Although this will depend on what you do with the XDR stuff (see
> *3. mode_umask Attribute*), I don;t see a need to reference RFC5662.
> You may wind up referencing RC7863.
> 
> Although this will depend on how you decide to address the extension
> issue, I believe a normative reference to draft-ietf-nfsv4-versioning
> is necessary.