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

[David Noveck <[e-mail]>]

On Tue, Aug 6, 2024, 6:21 PM Rick Macklem <[e-mail]> wrote:

>
>
> On Tue, Aug 6, 2024 at 10:05 AM Pali Rohár <[e-mail]>
> wrote:
>
>> Hello Rick,
>>
>> As we were discussed off-list, I'm sending to this list my comments for
>> your draft-rmacklem-nfsv4-posix-acls-02 document. Quoted text is
>> verbatim copy from your document.
>>
>> Your document describe POSIX ACL support only for NFS v4.2. The most
>> commonly used NFS4 version is 4.1 and it would stay for a longer time,
>> lot of enterprise NFS SW is being written as NFS3 + NFSv4.1 (no v4.2
>> support). So to make extension widely usable, it needs to be also for
>> NFS v4.1. Is there any reason why v4.1 is not included in your document?
>>
> As I understand it, new attributes cannot be added to 4.1.
>

According to RFC8178  they could only be added to correct a defect.  In the
acls document, I propose the addition of Aclchoice for that reason although
there has been some wg pushback.  I will make the best case I can for that
in acls-05 and  see if we can reach consensus. I may be forced to move this
to v4.2.

Also, adding support for 4.2 is not a lot of work (some would say
> it's trivial):
>
- For the client--> replace 1 with 2 in the minorversion field of the
>   compounds. That's all there is to it, since everything in 4.2 are
>   optional extensions to 4.1.
>   --> These optional extensions can be added if/when the implementor
>       needs them.
> - For the server--> accept minorversion set to 2 and make sure all
>   operation #s that are in 4.2 get a reply of NSF4ERR_NOTSUPP.
>

You also have to reject unsupported v4.2 attributes in SETATTR, VERIFY,
NVERIFY ...

  --> Again, an implementor can then add support for 4.2 optional
>       operations, attributes.. whenever needed.
>
>
>> > 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. This is required because [RFC8881] does not specify
>> > an ordering for setting attributes in a SETATTR operation.
>>
>> RFC 8881 in section "6.4.1. Setting the Mode and/or ACL Attributes"
>> describes the ordering of setting MODE, ACL attributes.
>>
>> In section "6.4.1.3. Setting Both ACL and Mode" is covered any ACL
>> attribute with any MODE attribute:
>> "When setting both the mode (includes use of either the mode attribute
>> or the mode_set_masked attribute) and the acl or dacl attributes in the
>> same operation, the attributes MUST be applied in this order: mode (or
>> mode_set_masked), then ACL."
>>
>> I think it would be better if your extension says that the new
>> POSIX_*_ACL attributes has to be handled as ACL attribute according to
>> RFC 8881 6.4.1.3. section. It would simplify MODE and ACL interop in
>> SETATTR operation.
>>
> Yea, I suppose it is more consistent.
> I'll change it.
>
> To be honest, at least for the FreeBSD and Linux clients, the
> acl attribute is never changed in the same SETATTR as mode/mode_set_masked
> so I am a little concerned that servers may not get this right.
> (I think the FreeBSD server does get the ordering right.)
>
>
>> > Therefore, to maintain compatible semantics with the POSIX draft, for
>> > NFSv4 operations that create new file objects (OPEN/OPEN4_CREATE,
>> > CREATE) in a directory that has a POSIX default ACL, the low order 9
>> > bits of the mode SHOULD be specified by either mode or mode_set_masked
>> > in the setable attributes for the operation.
>>
>> RFC 8881 in section 6.2.5. specifies:
>> "The mode_set_masked attribute is only valid in a SETATTR operation. If
>> it is used in a CREATE or OPEN operation, the server MUST return
>> NFS4ERR_INVAL."
>>
> Ok, I never noticed that
>


Good catch.  I didn't notice it either.


I'll fix this. Thanks for pointing it out.
>
>
>> So when creating a new object "mode_set_masked", cannot be used. For
>> inheritance you need to use "mode_umask" attribute which is defined in
>> RFC 8275. This attribute already handles this POSIX inheritance issue
>> and I think it is required for proper POSIX ACL implementation.
>>
> Hmm. I suppose the document should say something about the use of
> this attribute, if it is supported. (Since it is yet another
> optional 4.2 extension, I'd rather not have these attributes
> depend on support for it.)
> I suppose it can go as far as recommending support of it?
>

I think you can also RECOMMEND it or REQUIRE it.  I don''t have strong
feelings about this.


>
>> > For a SETATTR, setting a zero length 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.
>>
>> Why is this needed? Empty NFS4 ACL is same valid NFS4 ACL. If somebody
>> want to restrict access to some file then setting empty list of ACEs
>> which is zero-length acl. I think that ACL with no access should still
>> be in NFS4 model.
>>
> For the case of ACL_SCOPE_FILE_OBJECT, I believe there needs to be
> a way to "delete" the NFSv4 ACL. For example, here is a snippet from
> IBM's doc. on GPFS:
> You can also change the type of ACL by the mmdelacl command (causing the
> permissions to revert to the mode, which is in effect a POSIX ACL).
> As you might have guessed, "mmdelacl" deletes the ACL.
>
> As such, it seems that there must be a way to "delete" a NFSv4 ACL and
> setting "acl" to a 0 length ACL seemed the convenient way to do it.
>

I always thought that was the case but it might not be said
explicitly anywhere.

--> For GPFS, "delete" seems to actually be defined as switch to a
>     minimal POSIX ACL.
>
>
>> Comparing with Windows. On Windows (and in SMB protocol) there are two
>> different operations which can be done on object: Erasing DACL (by
>> setting it to NULL) and setting empty DACL (by setting empty list of
>> ACEs). Erasing is really erasing of the DACL in the storage (similar
>> like erasing POSIX ACL and reverting to mode_only) which has same effect
>> as ALLOW-EVERYONE-FULL-ACCESS. And setting empty list has effect of the
>> DENY-EVERYONE-FULL-ACCESS (as ACLs are default-DENY, there is no rule
>> which can grant some access).
>>
>> As NFS4 does not have a way to set ACL to NULL, it has only the section
>> option and matches Windows behavior of empty-ACE-list as NFS4 is also
>> default-DENY.
>>
> Thanks for pointing this out. I did not realize a zero length SMB/Windows
> ACL had a defined meaning. I think this draft should avoid assigning a
> meaning to a zero length ACL.
> --> I think that a "deleted ACL" case could be defined for both models
>     of ACL. (Something like an ACL with a single ACE that is defined
>     as "deleted ACL".)
>     I discuss this a bit more later.
>
> I cannot find anything in RFC8881 that defines what setting a zero
> length ACL means (although you've already seen that I am not good at
> reading such things;-).
> It might be better if this draft avoids the "setting of a zero length ACL"
> and let David's draft address the semantics of that?
>

I can do that for NFSv4 ACLs in acls-05.   You could then consder the
effect of
"deletion of the dacl" instead.

The only problem would be if you want want your draft to be
published before mine.

I can see how the windows handling (deny-everything) is different, but I
think I can deal with that differently since NFsv4 ACLs are
coordinated with mode and so the effect would be that
authorization decisions are controlled only by mode in this case.

>
> What do others think?
>
>
>> > A server that is configured for a acl_trueform_scope other that
>> > ACL_SCOPE_FILE_OBJECT MUST not support the posix_default_acl and
>> > posix_access_acl unless the true form for the file system is
>> > ACL_MODEL_POSIX_DRAFT.
>>
>> This is a restriction which basically means that no existing NFS4
>> server with native NFS4 ACL support can support "posix_access_acl".
>> In multiprotocol environment where is NFSv4.0 and NFSv4.1, the NFS4
>> server would have to provide server scope native NFS4 model (to support
>> NFSv4.1 as as your document is only for NFSv4.2) which implies that
>> server must not support POSIX ACL. And same applies in the multiprotocol
>> environment with NFS4 and SMB.
>>
> Not exactly. posix_access_acl refers to the proposed optional
> attribute for NFSv4.2 (same goes for acl_trueform_scope).
> What the server does w.r.t. "true form" is up to the server.
>

Actually it is up to the working group, although we have no means of
enforcement.


Same goes for the scope of it.
> (For example, Linux server always use POSIX draft ACLs.)
>
> The difference is that NFSv4.0 and 4.1 clients cannot know
> what the "true form" is and can only see NFSv4 ACLs (possibly
> mapped to/from POSIX draft ACLs).
>
> What the above restriction is meant to do is allow a NFSv4.2
> client to know that the "true form" is ACL_MODEL_POSIX_DRAFT
> when the scope is ACL_SCOPE_FILE_SYSTEM or ACL_SCOPE_SERVER
> by seeing that posix_access_acl and posix_default_acl are
> supported_attrs for the file system.
> --> Note that acl_trueform_scope is "per server", so a client
>     only needs to GETATTR this attribute once for the server.
>     Then, if that is ACL_SCOPE_FILE_SYSTEM or ACL_SCOPE_SERVER,
>     it can tell that the "true form" is ACL_MODEL_POSIX_DRAFT
>     as soon as it sees posix_access_acl and posix_default_acl
>     in supported_attrs.
>     --> It does not have to bother to check acl_trueform,
>         which would often result in an extra RPC.
>
>
>> I think that this is too strict and what would happen is either that
>> this extension would not be implemented OR section option (which I think
>> everybody would do) is ignoring this restriction.
>>
> As above, this does not define what the server does for NFSv4.0
> and NFSv4.1, it just simplifies what a NFSv4.2 client needs to
> check, if it knows about and the server supports the extension
> proposed by this draft.
>
>>
>> I guess that for compatibility with FreeBSD and Linux, servers would
>> implement it even on NFS4 server scope model.
>>
> Not sure what you mean by this. Linux always uses a "true form"
> of POSIX ACL and FreeBSD supports one of POSIX ACL or NFSv4 ACL
> on a per-file system basic (at least for now).
> - That is not changed by this extension. All this extension does
>   is allow NFSv4.2 clients to use the server more effectively
>   by choosing to get/set the "true form" and avoiding mapping,
>   assuming the client and server both implement the extension.
> Other file systems (IBM's GPFS seems to be the extant example
> I am aware of) also supports a "true form" on a per-file basis.
> (Again this is a server file system configuration choice and
> is not affected by whether or not this extension is supported
> by a NFSv4.2 server.)
>
>
>>
>> And I have there two additional points without quoting your document.
>>
>> 1)
>>
>> Your document does not describe what existing RFC 8881 acl and dacl
>> attributes should return when user sets new "posix_default_acl" or
>> "posix_access_acl" attribute. Also what should happen when another
>> client tries to change acl or dacl attribute when POSIX ACL is already
>> active.
>>
> The intent (if that is not clear, the words need to be improved)
> is that the server file system shall have one "true form" for each
> file object. If the attribute(s) for the other "true form" is used
> on the object (which assumes the server has chosen to put attributes for
> both in supported_attrs for the file syetem, the GETATTR/READIR/SETATTR
> will use mapping to translate it to/from the "true form".
> The intent is to discourage use of mapping. As such, the client using
> these extensions can avoid that by determining what the "true form" is
> for the object and using the appropriate attributes.
>

I think it is more correct to say that the intent is to eliminate the need
for mapping.  We don't need to discourage it since the inability to map
curately already provides sufficient discouragement.


> My thinking is that servers like FreeBSD (where the "true form" is per-file
> system and mapping is not supported) would:
> (A) - Support posix_access_acl and posix_default_acl, but not acl, for file
>   systems that use POSIX ACLs as their "true form".
> (B) - Support acl, but not posix_access_acl and posix_default_acl, for file
>   systems that ise NFSv4 ACLs as their "true form".
> For Linux servers, they might choose (A) above, or they might choose
> to support all of acl, posix_access_acl and posix_default_acl, where acl
> is supported via the mapping algorithms they already use.
>

If they did that, they would have o have some way of rejecting NFSv4 ACLs
that could not be translated to a POSIX ACL.


> It is the case of a per-file scope where things get difficult:
> The server will probably want to support all of acl, posix_access_acl
> and posix_default_acl, but since the "true form" is defined a one
> model for each file, then how should the attribute(s) for the other
> model be handled?
> - I would like to avoid mapping, so I think being able to return
>   "no ACL" would be preferred.
> - I had thought a zero length ACL would work for this, but that no
>   longer appears to be the case.
>

Why not?

Maybe, defining an ACL with a single ACE that says "not valid" would
> work?
> This could be the reply for GETATTR/READDIR and could be used to
> "erase" the ACL for SETATTR.
> --> I think this is what the next draft will propose.
>

If we want to go this way. It should be done in acls-xx for NFSv4 ACLs.

>
>
>> If interop between mode, dacl and posix_default_acl, posix_access_acl
>> attributes is not explicitly defined in specification then every server
>> implementation will invent its own rules and it would just lead to the
>> mess in multiprotocol environment where clients would not be sure what
>> can happen. Like it is already with "mode" attribute in NFS v4.1 (see 2).
>>
> For posix_access_acl and posix_default_acl the intent is that the
> POSIX draft specification defines the interaction between mode and
> the POSIX ACL.
>
> For NFSv4 ACLs, you are correct. It is a mess.
> And I wish David N. the best of luck w.r.t. cleaning it up.
>

Will do my best in acls-05.

(OpenZFS already has a knob that specifies 4 different ways
> of doing this. Another knob setting may be needed.)
>
> Thanks for the comments, rick
>
>>
>> 2)
>>
>> Then there is a problem with "group" bits of "mode" attribute.
>> In RFC 8881 section 6.2.4 is written:
>>
>> "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."
>>
>> And in section 6.3.2.1 is written:
>>
>> "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."
>> "The same user confusion seen when fetching the mode also results if
>> setting the mode does not effectively control permissions for the owner,
>> group, and other users; this motivates some of the requirements that
>> follow."
>>
>> POSIX ACL with at least one named user or named group requires that
>> mode group bits refers to POSIX ACL mask, and not to the POSIX ACL
>> group_obj (which represents owner_group).
>>
>> Those RFC 8881 parts of NFS mode attribute are basically not compatible
>> with POSIX ACL. Your extension does not mention this problem and does
>> not handle it.
>>
>> Existing NFS4 servers which follows the RFC 8881 meaning of mode group
>> bits, would not be able to implement this your extension for POSIX ACL,
>> because existing servers cannot change behavior of mode attribute due
>> backward compatibility. I think that this is a big problem.
>> Servers in multiprotocol environment mostly follows the RFC 8881 advice
>> to never return something like "POSIX mask" in group bits.
>>
>> How to solve this problem. One option could be including all mode bits
>> into your new "posix_access_acl" attribute and let NFS4 clients to use
>> only "posix_access_acl" on chown operation, and never use existing
>> "mode" attribute at all (if the extension is supported by server).
>>
>> Another option could be to introduce a new "posix_mode" attribute which
>> would be strictly POSIX and old "mode" attribute can stay as before for
>> backward compatibility. Again clients would not use "mode" attribute
>> anymore.
>>
>
A similar possibility is discussed in an appendix to acls-04.   The idea is
a read-only mode_info attribute
that clients could use if they want to display a mode that can be displayed
to reflect a summary of the ACL permissions.



>>
>> Regards,
>> Pali
>>
> _______________________________________________
> nfsv4 mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
>