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

[Rick Macklem <[e-mail]>]

I have updated the draft to -05. I think I have
addressed your comments.

A few inline responses below.

On Fri, Aug 9, 2024 at 5:31 PM Pali Rohár <[e-mail]>
wrote:

> 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.
>
I think I have addressed this now. It notes that mode is sychronized
with the true form ACL for the file object.


>
> 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").
>
I added a "See RFC8275..." for this.


>
>
> 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.
>
I compromised by saying any setting of dacl does this and any setting
of acl, if it has ALLOW/DENY ACEs in it does this. (I don't know if a 0
length
setting for acl means set the ACL model to NFSv4 or get rid of all
ALARM/AUDIT ACEs or ???)

I hope that David Noveck can resolve what a 0 length ACL actually means
in his draft. As you have noted, SMB considers one of these "normal" and,
as such, I see the argument that it should be that way for NFSv4 ACLs as
well.
However, I see that the FreeBSD port of OpenZFS (which uses ACL_MODEL_NFS4
as
its true form) returns an error if a setting of a zero length ACL is
attempted.
I cannot tell if this was done by the FreeBSD guy that worked on ACLs or was
cribbed from OpenSolaris?

I think it would be nice to somehow "deprecate" acl in favour of dacl/sacl.


> > 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.
>
Sorry, but if ACL_MODEL_POSIX_DRAFT does not obviously refer to POSIX draft
ACLs,
I don't know what does. I left it as is.


>
> 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?
>
Good catch. I think this is fixed now in the draft.


>
>
> 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.
>
Again, good catch. I think it is fixed now.


>
>
> 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.
>
I added a short paragraph that states synchronization of mode with
the true form ACL SHOULD be done. Using either RFC8881 or Grunbacher
as references.


>
> > 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.)
>
I felt the description that noted these three ACEs are always in a POSIX
draft
ACL was enough but, yes, I suppose it should be added.
I missed this one for -05, but will do so for -06.

rick


>
> Regards,
> Pali
>