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

[Rick Macklem <[e-mail]>]

On Wed, Aug 7, 2024 at 5:07 AM David Noveck <[e-mail]> wrote:

>
>
> 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?
>
I will defer any mechanism w.r.t. erasing a NFSv4 ACL to your work.
I will allow a server to return a zero length ACL when the acl does
not exist, but will note that a client needs to check to see if
acl_trueform != ACL_MODEL_NONE to determine that it exists.


> 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.
>
Agreed. My draft will only note that, if an NFSv4 ACL has been deleted,
a zero length NFSv4 ACL is returned and acl_trueform == ACL_MODEL_NONE.

rick

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