[nfsv4] Re: Review of draft-rmacklem-nfsv4-posix-acls-02

[Rick Macklem <[e-mail]>]

I have updated the draft to draft-rmacklem-nfsv4-posix-acls-03.
I think I have addressed all the comments in David's review.

There are a few substantive changes:
- The draft no longer refers to the setting of a 0 length NFSv4
  ACL. (It appears that a 0 length NFSv4 ACL may just be a NFSv4
  with no permissions. This is different from an erased Windows/SMB ACL,
  which was what I was using a 0 length NFSv4 ACL for.)
- I changed the ordering of setting mode vs POSIX ACL to defining the
  order if they are in the set of setable attributes.
- I think I now refer to mode/mode_set_masked/mode_umask correctly
  and require support of mode_umask and mode for this extension.
- When a NFSv4 ACL is being replaced by a POSIX ACL, it now notes that
  the allow/deny ACEs will be deleted, so that the audit/alarm ACEs
  remain intact.
- It now allows a SETATTR to flip between NFSv4 and POSIX ACLs when
  the scope is ACL_SCOPE_FILE_OBJECT. (The old draft restricted that.)

Thanks go to David Noveck for his review.

rick


On Tue, Aug 6, 2024 at 8:26 AM David Noveck <[e-mail]> wrote:

> *Overall Evaluation *
>
> In good shape.  There are a few  non-editorial issues that need to be
> dealt with but not many.
>
> I am prepared to support working group adoption as soon as the author is
> ready to deal with the inevitable requests for a completion date.
>
> Based on what I have seen so far, I am planning to refocus work on
> acls-05.  I had been hoping to enhance the NFSv4 ACL model so that the
> POSIX ACL model could be a submodel.  As I read Andreas' document, this
> kept getting more and more difficult and it now appears that, with the
> document being reviewed, we have a reasonable path out of the existing ACL
> mess 😳
>
> *General Issues*
>
> *Dealing with Sacls*
>
> There are a number of places where sacls are treated as parts of acls.
> They shouldn't be and I will discuss below ways of having posix acls
> co-exist with use of the parts of  NFSv4 ACLs  that are not related to
> authorization.
>
> *Interaction with Existing ACL Attributes*
>
> There are some missing cases in the list in *6. POSIX ACL vs NFSv4 ACL
> Considerations.* The existing treatment which deals only with deleting
> ACLs does not address issues involving creating new ACLs of either type.
>
> *Ordering Issues*
>
> My issues with the substance of the attribute ordering restrictions have
> been addressed but I still have issues with the stated justification.  See 5.
> POSIX ACL Considerations for details.
>
>
> *Per-section Issues*
>
> *Abstract *
>
> Suggest rewriting the first sentence as follows:
>
> This document describes a potential protocol extension involving the
> addition of four new attributes to be used by server to provide support for
> POSIX ACLs.
>
> *1. Introduction*
>
> I have problems with the first sentence which says that mapping was
> attempted because of the semantic differences.   This implies that if there
> were no semantic differences, mapping would  not have been attempted.  In
> fact, it still would have been attempted but would have been sucessful.  I
> would start the section with something like the following:
>
> In response to the very different over-the-wire formats, attempts have been made to map between these two sorts of ACLs.  However, because of the
>
> large number of semantic differences, implementation experience with mapping between NFSv4 and POSIX ACLs has not been completely successful.
>
>  For example, if a NFSv4 ACL is applied to a server file via SETATTR on a server that stores POSIX ACLs and then retrieved via
>
> GETATTR/READDIR, the ACL will often not be the same, since the mapping algorithm cannot do an exact mapping between them.
>
>
> Suggest rewriting the last sentence of the last paragraph to read as
> follows:
>
> In addition, issues related to the over-the-wire format of POSIX ACLs and the interactions among the various new attributes and with
>
> existing attributes are dealt with in this document.
>
>
> *5.  POSIX ACL Considerations*
>
>
> In the first sentence, you are stating a true fact, but you need to make some normative statement to follow up.
>
>
> I assume that that would be that setting a POSIX ACL that does not follow that is invalid.
>
>
> In the second paragraph, know what you mean, but it is not clear that a reader would.  Suggest the following:
>
>
> In the who value within the posixace4 structure that appear in these new attributes, the field is interpreted as follows:
>
>
>    - For ACEs whose tag field is POSIXACE4_TAG_USER or POSIXACE4_TAG_GROUP the who value is a UTF8-encoded Unicode string.
>
> that has the same format as a user or group as represented within other NFSv4 operations and designates the same entity.
>
> In these cases, the distinction between users and groups derives from the tag rather than a flag bit, as is done in NFSv4 ACLs.
>
> This in in contrast to how the corresponding structures are described in <xref target="Gr&#252;nbacher">, where numeric uids and gds are
>
> specified.
>
>
>    - For ACEs whose tag field has other values, the who field is ignored by the receiver and there is no reason for the sender to set it to any
>
> particular value.
>
> With regards to the third paragraph, I no longer object to the result.  I had objected but you have made it clear why this is the way it is but that differs from what is in the document now.  Below I quote the existing paragraph with my comments interspersed.
>
> For POSIX ACLs, setting of the low order 9 bits of mode can change the ACL and setting of the POSIX access ACL can change the low order 9 bits of mode.
>
> This is true.
>
> As such, the ordering of setting the attributes related to mode and POSIX ACLs is important.
>
> It is certainly significant.
>
> Therefore, a client MUST not set the low order 9 bits of mode via the mode or mode_set_masked attributes in the same SETATTR operation as one that sets the posix_access_acl and/or posix_default_acl proposed in this document.
>
> I don't agree with the "therefore".
>
> This is required because [RFC8881] does not specify an ordering for setting attributes in a SETATTR operation.
>
> That is* NOT *true. There is nothing in rfc8881 that prevents you from
> specifying an ordering as rfcs8881 does for NFSv4 ACLs.
>
> You are as free to specify an ordering as you are to specify that these are incompatible.
>
>
> I suggest rewriting this to say something like the following:
>
>
> A client MUST not set any of the nine-low order bits of mode via the mode or mode_set_masked attributes in the same SETATTR operation as one that sets the posix_access_acl and/or posix_default_acl proposed in this document.  Similarly, it MUST not set these together when creating a new file. These restrictions apply because:
>
>
>    - Setting these together is not required by the POSIX interface.
>    - To do so, would require dealing with difficult ordering issues left unaddressed by the existing definitionof SETATTR.
>
> For the last paragraph, I have the following editorial suggestions:
>
>
>    - Suggest replacing "low order 9" by "low-order nine"
>    - In the second sentence I suggest replacing "SHOULD" by "should".
>    - In the third sentence suggest replacing "may use" by "can use".
>    - In this last sentence suggest replacing "will avoid" by "would avoid"
>
>
> *6.  POSIX ACL vs NFSv4 ACL Considerations*
>
>
> In the first sentence of the second paragraph, suggest replace "a" with  "an".
>
>
> Suggest rewriting the third paragraph as follows:
>
> For servers configured as described above, and not for any others,  the following apply:
>
> For the third bulleted item, propose rewriting it as two paragraphs like
> the following.
>
> Using SETATTR to set a zero-length posix_default_acl or posix_access_acl (for a file object with a true form ACL of ACL_MODEL_POSIX_DRAFT)
>
> will delete the true form ACL(s) from the file object and revert it to ACL_MODEL_NONE.
>
> Similarly, using  SETATTR to eliminate the authorization-related portions of the ACL for a file object with a true form ACL of ACL_MODEL_NFS4  will
>
> delete the true form ACL(s) from the file object and revert it to ACL_MODEL_NONE.  This can occur when setting the sacl or dacl to an array of length
>
> zero or by settting the acl attribute to an array of ACEs  in which none are ALLOW or DENY ACEs.
>
>
> Propose to add the following items to the existing list:
>
>    -
>
>    Using SETATTR to set a posix_default_acl or posix_access_acl of non-zero length (for a file object with a true form ACL of
>
>    ACL_MODEL_POSIX_DRAFT) will result in the object's acl model being set
>    to ACL_MODEL_POSIX_DRAFT. In addition, if the object's acl model had
>    been  ACL_MODEL_NFS4, all ALLOW and DENY ACEs will be deleted so that
>    the value dacl attribute is a zero-length array.
>    -
>
>    Using SETATTR to set the dacl attribute to an array of non-zero length or to st the acl attribute so that it contains one or more ALLOW or DENY ACEs
>
>    will result in the object's acl model being set to
>    ACL_MODEL_POSIX_NFS4. In addition, if the object's acl model had been  ACL_MODEL_POSIX_DRAFT,
>    the existing default and access ACLs ae to be deleted.
>
>
>