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

[David Noveck <[e-mail]>]

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