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

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

Thanks for the review.  I ignored or reworded a couple suggestions, but
don't think there's any substantive disagreement.

Also I still need to figure out what if anything to do about the xdr.

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..

Yes, I still don't understand why we need the NFSv4 extension draft, but
I also don't necessarily have any major disagreements with it, and it's
not important enough to me to put up a fight.  So a reference to that's
probably the thing to do.

> 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".

OK, done.

> *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.

OK, done.

> 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.

Fiddled with it some more and came up with:

	This can be addressed by transmitting the umask and create mode
	as separate pieces of data, allowing the server to make more
	intelligent decisions about the permissions to set on new files.
	This document proposes a protocol extension which accomplishes
	that.

> *2. Problem Statement*
> 
> In the first sentence of the second paragraph, suggest replacing "currently
> does not" by "does not currently".

OK.

> In the third sentence of the second paragraph, suggest replacing "on the
> safe side" by "on the the side of caution".

OK.

> In the fourth sentence of the second paragraph, suggest replacing "Thus the
> mode" by "As a result, the mode attrbute".

After a little more fiddling:

	As a result, the server receives an OPEN with a mode attribute
	that already has the umask applied.

> 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.

I agree, but I think it's an unnecessary digression here; leaving as is
for now.

> 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.

Makes sense, done.

> Suggest replacing the fifth and sixth paragraphs by the following:

Hm.  I don't like the existing language either:

> This issue leads to file permissions which are more restrictive than
> they should be in common cases.

"More restrictive than they should be", here and in existing
language--true but I think the real problem is that the inherited ACEs
are ignored--and the previous sentence just said that.

> 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.

Replaced both paragraphs by:

     This issue leaves inheritable ACLs useless in some common cases.
     We make them useful in these cases by defining a new attribute
     which allows the server to ignore the umask in the presence of
     inheritable permissions.

> *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].

OK, added with some rewording.

> *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.

We already have words to this effect in a couple places; left as is.

> 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.

OK, not done yet, but I'll look into it.

> 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.

OK, thanks, done.

> 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.

OK, both done.

> 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"

OK, made that "the mode assigned to the new object becomes..."

> *4.  Security Considerations*
> 
> In the second sentence of the first paragraph, suggest replacing "the
> recommendation that they SHOULD" by "the RECOMMENDATION that they".

OK, done.

> 
> 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.

OK, done.

> *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.

OK, done.

> *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.

Done.

> 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.

OK, I think that was done by including your suggested "Protocol
Extension Considerations" section.

--b.