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

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

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.

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.

> 
> >
> > 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?)

And executing these steps in incorrect order can results in something
unexpected or something not compatible with IEEE POSIX draft.



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.

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

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