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

[Rick Macklem <[e-mail]>]

I have updated the draft yet again to -04. Hopefully this
is it for a while.
The changes were:
- Added a short section w.r.t. atomicity of the ACL related
  attributes for GETATTR/READDIR vs SETATTR.
- Fixed a few typos introduced in the last revision.
- Reworded one case so that it now says that
  "if the acl_truform is ACL_MODEL_NONE, the
   posix_access_acl MUST be returned 0 length".

rick


On Wed, Aug 7, 2024 at 4:15 PM Rick Macklem <[e-mail]> wrote:

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