[nfsv4] Comments for draft-rmacklem-nfsv4-posix-acls-04

[Pali Rohár <[e-mail]>]

Hello Rick,

I have read your new version -04 of extension document and just now I
have figured out some new things there.

For better readability I have one small suggestion for section 9.
In description of each attribute could be list of all possible values.
Currently the possible values are defined only in XDR section 4. And it
is quite hard for me to read this section 9, because I need to scroll up
(to read list of possible values) and then back down to the description.
When I first time read this section in -02 I was quite lost and I had to
read it more times.


My comment from -02 which still applies for -04 is version is ambiguity
of NFS4 mode attribute which does not match POSIX mode as required by
POSIX ACL.


New comments for each section are below.


Section 5. POSIX ACL Considerations

> in a directory that has a POSIX default ACL, the low-order nine bits
> of the mode MUST be specified by mode_umask in the setable attributes
> for the operation.

I think that here is missing an important fact that mode is specified in
"mu_mode member of mode_umask" attribute and that the "mu_umask member
of mode_umask" is being ignored in this case (when POSIX default ACL is
being used).

Because from current description is not clear how mode_umask is being
used as this attribute contains two members ("mu_mode" and "mu_umask").


Section 6. POSIX ACL vs NFSv4 ACL Considerations

> Using SETATTR to set the dacl attribute to an array of non-zero length
> ... will result in the object's acl model being set to ACL_MODEL_NFS4.

Why setting NFS v4.1 dacl attribute to zero length is excluded here?
I think it is perfectly fine to set dacl with ACL4_PROTECTED flag and
with zero length (empty list of) ACEs. This is fully valid NFS4 model
ACL which overwrites automatic inheritance to ACL with no access.

In my opinion using SETATTR on dacl attribute should always change acl
model to ACL_MODEL_NFS4. I'm expecting that as a user I'm setting NFS4
dacl attribute which refers to NFS4 model, so I'm unconditionally
setting model to NFS4.

> In addition, if the object's acl model had been ACL_MODEL_POSIX_DRAFT,
> the existing default and access ACLs are to be deleted.

I think that in this document it is a good idea to always explicitly
mention ACL type. Because sometimes it can be really confusing. For
example: "existing POSIX default and access ACLs" make it clear.


Section 7. GETATTR/SETATTR Atomicity should handle VERIFY and NVERIFY
atomically in the same way as GETATTR.

For example, NFS4 client may want to check if POSIX ACL is set to some
specific structure, and VERIFY with both acl_trueform and
posix_access_acl attributes is the appropriate operation for it.

Maybe VERIFY/NVERIFY should be covered also in other sections which
refers to GETATTR/READDIR?


Section 9.3. Attribute 90: posix_default_acl

> Since a POSIX default ACL only applies to directories, a SETATTR of
> the posix_default_acl for a non-directory object MUST reply
> NFS4ERR_INVAL.

I guess that this MUST applies also for OPEN/OPEN4_CREATE and
CREATE/NON-NF4DIR operations.


Section 9.4. Attribute 91: posix_access_acl

> a successful setting of this attribute sets the value of acl_trueform
> 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 of the dacl attribute is a zero-length array.

In other email I already wrote similar comment which applies here too.
I think that this contradicts RFC 8881. Why?

* Per section 1. Introduction: "If the server chooses to support
  posix_default_acl and posix_access_acl for a file system, it MUST
  support the mode/mode_umask attributes for the file system."

* It is not explicitly mentioned but ACL_MODEL_NFS4 should imply that
  NFS4 server has to support dacl (or acl) attribute. (It would be nice
  to explicitly mention it.)

* Per RFC 8881 section 6.4. Requirements: "The server that supports both
  mode and ACL must take care to synchronize the MODE4_*USR, MODE4_*GRP,
  and MODE4_*OTH bits with the ACEs that have respective who fields of
  "OWNER@", "GROUP@", and "EVERYONE@". This way, the client can see if
  semantically equivalent access permissions exist whether the client
  asks for the owner, owner_group, and mode attributes or for just the
  ACL."

So we are in situation when mode, acl/dacl and posix_access_acl
attributes are supported.

And successful setting of posix_access_acl attribute results in
zero-length array of dacl attribute. This removal of dacl ACEs results
in synchronization of dacl to mode attribute. As there is no "OWNER@",
"GROUP@", and "EVERYONE@" ACE entry in dacl anymore (because dacl is
empty), this will result in synchronization of mode to empty access 000.

Per POSIX ACL, mode is being synchronized to POSIX ACL entities
user_obj, group_obj/mask, other.

If my understanding and steps above are correct then successful setting
of posix_access_acl attribute to any value results in immediate no
access for user_obj, group_obj/mask and other POSIX entities. And I
guess that this is not intended behavior.


> If SETATTR of a POSIX extended ACL does not have a POSIXACE4_TAG_MASK
> ACE, the SETATTR of the ACL MUST reply NFS4ERR_INVAL.

I think there should be also mentioned that SETATTR of POSIX ACL (does
not have to be extended) which does not have POSIXACE4_TAG_USER_OBJ,
POSIXACE4_TAG_GROUP_OBJ or POSIXACE4_TAG_OTHER ACE MUST result in
NFS4ERR_INVAL too. (Except zero-length.)


Regards,
Pali