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

[Rick Macklem <[e-mail]>]

On Sat, Aug 10, 2024 at 3:36 PM Pali Rohár <[e-mail]>
wrote:

> On Saturday 10 August 2024 13:55:36 Rick Macklem wrote:
> > 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.
>
> I think that there is still an issue. Meaning of the mode attribute is
> defined differently in RFC 8881.
>
> RFC 8881 6.2.4. Attribute 33: mode says: "Bits MODE4_RGRP, MODE4_WGRP,
> and MODE4_XGRP apply to principals identified in the owner_group
> attribute but who are not identified in the owner attribute."
>
> RFC 8881 6.3.2.1. Discussion says: "Some server implementations also add
> bits permitted to named users and groups to the group bits (MODE4_RGRP,
> MODE4_WGRP, and MODE4_XGRP). Implementations are discouraged from doing
> this."
>
> So according to meaning of mode attribute, MODE4_RGRP, MODE4_WGRP, and
> MODE4_XGRP bits matches POSIXACE4_TAG_GROUP_OBJ. Which is not compatible
> with IEEE POSIX draft.
>
> I see that you have added following requirement into description.
>
>   "If the true form is ACL_MODEL_POSIX_DRAFT, synchronization is
>   described in [Grünbacher]."
>
> Which sounds like that it wants to override what is defined in the
> "RFC 8881 6.2.4. Attribute 33: mode", and change meaning of the NFS4
> mode attribute.
>
> I do not think that it is a good idea if some optional extension is
> going to change meaning or wants that server should implement something
> which RFC 8881 6.3.2.1. says that "Implementations are discouraged from
> doing this". It it contradiction.
>
> Example scenario why it is a bad idea.
>
> Some existing NFS4 server implements mode attribute according to
> RFC 8881 and always synchronize group bits with owner_group (who is not
> owner). So group bits are never POSIX mask. And also implements NFS4
> ACL support.
>
> There is some NFS4 client which is using that NFS4 server, and this
> client uses only mode attribute (does not touch acl/dacl/sacl), and is
> using chmod with group bits to set access for owner_group (who is not
> owner).
>
> Everything is working fine up to here.
>
> Now that NFS4 server wants to implement per-file scope POSIX ACL
> support. The only requirement is not break that existing NFS4 client.
> As the NFS4 client does not look or touch any ACL, POSIX ACL support on
> server filesystem should be harmless.
>
> But problem is that such server cannot implement POSIX ACL support
> according to this extension because of "mode" compatibility issues with
> that NFS4 client. For compatibility reasons for every file, server has
> to return MODE4_RGRP/MODE4_WGRP/MODE4_XGRP bits from owner_group (like
> before). But this POSIX ACL extension requires that mode group bits
> would be synchronized with POSIX MASK, so chmod of group bits stops
> working and breaks that NFS4 mode-only client.
>
That is a choice the server implementer makes. It is no different
than local use of POSIX draft ACLs on systems such as Linux.
The user chooses to set a POSIX draft ACL on the local file object
and in doing so, gets POSIX draft ACL semantics. If they do not set
a POSIX draft ACL on the object, they get "mode only" semantics.

If a server chooses to implement the extension proposed in this draft,
then there is no NFSv4 ACL and the behaviour looks exactly the same
as for local uses with/without POSIX draft ACLs on file objects on the
system.


> So how could this NFS4 server implements POSIX ACL support for Linux or
> FreeBSD client without breaking that one mentioned NFS4 client?


> I think that it is needed to define a new posix_mode attribute. I do not
> see right now any option how to not break backward compatibility for
> existing mode-only clients (which wants behavior of mode group bits as
> written in RFC 8881 / 6.2.4) and at the same time support POSIX ACL
> which requires that mode group bits contains either MASK (or group_obj
> if MASK is not defined).
>
> Or another option, putting content of POSIX-correct "mode" attribute
> into the "posix_access_acl" attribute (e.g. at beginning of the
> structure) as it is used only when POSIX ACL mode is active.
>
I do not think a separate mode is a good idea, just like I do not
think having both POSIX draft ACL(s) and a NFSv4 ACL stored/used for
permission checking on the same file
object at a given time is a good idea (the true form).

The simple summary for all this is the following:
RFC8881 was written for only one kind of ACL (NFSv4), so any discussions
in it w.r.t. the relationship between ACLs and mode are implicitly for
NFSv4 ACLs only and, understandably, did not take the POSIX draft ACLs
into account.
--> This optional extension changes all the above, so that server
implementers
    can choose to implement POSIX draft ACLs and use POSIX draft rules
related
    to ACLs and mode, if they choose to do so.



> >
> > >
> > > 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 ???)
>
> If acl attribute for some object contains only one ACE with
> "AUDIT SUCCESS|FAILURE EVERYONE@ FULL_ACCESS" then server should send
> into audit log any attempt to access that file.
>
> If I as admin want to remove auditing on that file then I need to remove
> this one ACE from acl attribute. So it means that I have to set acl
> attribute to empty.
>
> So setting zero length acl attribute has to get rid of all ACEs, ALLOW,
> DENY and also AUDIT and ALARM. Otherwise admin would not be able to stop
> auditing on file which has only AUDITing rules.
>
> > 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 thought that it is clear, but seems that there are confusions about
> zero length ACL. So proper clarification in new ACL draft would be
> really helpful.
>
> > I cannot tell if this was done by the FreeBSD guy that worked on ACLs or
> was
> > cribbed from OpenSolaris?
>
> I think it could be possible to check what the Oracle's Solaris is
> doing.
>
> > I think it would be nice to somehow "deprecate" acl in favour of
> dacl/sacl.
>
> I'm supporting the idea of deprecating "acl" attribute in favour of
> separated dacl and sacl attributes.
>
> But for backward compatibility with NFS v4.1 (and v4.2) it is
> impossible. Breaking existing NFS v4.1 clients which supports only "acl"
> attribute should be really avoided. So some future NFS v4.3 is the
> candidate for removal of this "acl" attribute.
>
> >
> > > > 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.
>
> I was suggesting this from the point of view of somebody new this area.
> For people familiar with POSIX ACL it is obvious. For somebody who is
> new to ACL it can be hard to figure out what is obvious and what not.
>
> But if you think it is OK then let 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.
>
> But the problem is still there. The extension document still says
> "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." which as I described above, according to RFC 8881 it
> can be interpreted as removal of all access for owner, group and others.
>
> It is not clear if the synchronization of the mode with true form ACL
> should be done before or after deleting of ALLOW/DENY rules.
>
> The problem is that SETATTR(posix_access_acl) is doing lot of things,
> including:
>
> * changing acl_trueform_scope to ACL_MODEL_POSIX_DRAFT
> * changing posix_access_acl to value specified in SETATTR
> * changing mode to value from posix_access_acl
> * changing acl and dacl values by removing all ALLOW and DENY rules
> * changing acl and dacl values to value from mode (RFC 8881 6.4)
> (Have I forgot something?)
>
Although it is hard to write in a format that works for this draft,
in summary, it is very simple:
- Set a POSIX draft ACL on the file object and delete any dacl.
Unfortunately, there is no well defined way to say the dacl has been
deleted.
(I thought a zero length ACL worked for this, since OpenZFS
does not allow setting of a zero length ACL, but you have pointed out
that that is not the case.)
--> So, until there is (if there ever is) a better way to say "dacl has
    been deleted", it ends up as "make the dacl a zero length ACL and get
rid
    of all allow/deny ACEs in acl.


> And executing these steps in incorrect order can results in something
> unexpected or something not compatible with IEEE POSIX draft.
>
Maybe I should just say "delete the dacl" and let server implementors
take it from there?


>
>
> And now I'm thinking, what should happen with dacl attribute's na41_flag
> value? Specially with ACL4_AUTO_INHERIT and ACL4_PROTECTED flags.
>
> I would say that ACL4_AUTO_INHERIT needs to be cleared (to prevent
> automatic inheritance to child objects) and ACL4_PROTECTED to be set (to
> prevent inheritance from parent objects). As POSIX ACL does not have any
> kind of automatic inheritance.
>
The intent of the above (and I have already noted that this is difficult
to write in a format acceptable for this draft) is "delete the dacl".
--> It goes away, no longer exists, has no effect on mode or access to
    the file.


> >
> > >
> > > > 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.
>
> Documentation should explicitly say what should happen = which error
> code to return when invalid POSIX ACL is specified in SETATTR (or
> OPEN/CREATE) operation.
>
> I was somehow surprised that XDR definition allows invalid POSIX ACL
> structure (the one without user_obj/group_obj/other parts). Because if
> it allows invalid structure then it is required to explicitly specify
> how to handle invalid cases.
>
I have already added a note that "one ACE for each of
POSIXACE4_USER_OBJ, POSIXACE4_GROUP_OBJ and POSIXACE4_OTHER is
required in posix_access_acl and posix_default_acl or NFS4ERR_INVAL
MUST be replied" in the next draft (I won't be updating the draft
on the datatracker for at least a week or two).

rick


> >
> > rick
> >
> >
> > >
> > > Regards,
> > > Pali
> > >
>