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

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

On Tuesday 06 August 2024 16:50:32 Rick Macklem wrote:
> On Tue, Aug 6, 2024 at 4:10 PM Pali Rohár <[e-mail]>
> wrote:
> 
> > Hello Rick, I'm just writing a reply to few points. For rest I will look
> > later.
> >
> > On Tuesday 06 August 2024 15:20:50 Rick Macklem 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.
> >
> > It is prohibited to define a new extension for NFS v4.1? I just to not
> > see a technical reason why not to define extension for both protocols.
> > At least for me it looks like that in both sub-versions 4.1 and 4.2 it
> > would be exactly same attributes and it would work exactly same. And
> > seems that it can work also in 4.0.
> >
> See RFC8178. Just fyi, I am just one guy on the mailing list. I have
> never attended a working group meeting, so I'm not sure if I am
> considered a working group member.

Ok, so this is for somebody else to decide or clarify.

> 
> > > 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.
> > >   --> Again, an implementor can then add support for 4.2 optional
> > >       operations, attributes.. whenever needed.
> >
> > I understand what you mean. But enterprise grade ecosystem does not work
> > such trivially. NFS v4.2 is a new / different protocol, not compatible
> > with NFS v4.1 (at least because it needs different number in some
> > packet). Also it has different name, which indicates different protocol.
> >
> > Providing support for a new protocol is always a big task (and does not
> > matter how much work it is, if just adding a new if-else case in code or
> > completely rewriting client / server code from scratch).
> >
> > Adding support for some optional extension to existing protocol version
> > is always smaller task for enterprise ecosystem than new protocol
> > version.
> >
> > What from what I was told, NFS in industry is using v3 and v4.1
> > versions. And this is not going to be changed too soon.
> >
> I suspect you are correct w.r.t. NFSv3. Some (the Linux NFS Project,
> I think?) have talked about deprecating NFSv4.0.
> I'm certainly not an "enterprise guy" so I believe you, but for me
> NFSv4.1 and 4.2 are basically the same (4.2 is just a minor update,
> unlike NFSv4.0->4.1.)

I know that 4.1 and 4.2 are basically same. But it is not same for
industry. It is just engineers are looking at it differently than other
people.

> 
> > >
> > > > > 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.)
> >
> > If the extension document clearly says that servers must be care about
> > that ordering (even it is clearly specified in RFC 8881 for mode/ACL)
> > then servers implementing this extension would need to implement
> > processing correctly.
> >
> > >
> > > > > 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.
> > > 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?
> >
> > On systems which implements POSIX ACLs, all syscalls which creates a new
> > object (creat, mkdir, mknod, ...) takes an mode argument. But this mode
> > is the final mode of the file, but it interacts with POSIX default ACLs
> > and also with process umask. And if that syscall interacts with some
> > remote filesystem (e.g. NFS), it has to send both mode and umask to
> > server which will evaluate it together with server's default POSIX ACLs
> > and based on all these information evaluates the final mode during
> > creating a new file.
> >
> > Knowing umask is required for POSIX ACLs because when default POSIX ACL
> > is active then umask is ignored. So NFS client must not evaluate umask
> > on the client side and it has to send it over network.
> >
> > And this is possible only via RFC 8275 extension via "mode_umask"
> > attribute. So without this attribute it is not possible to correctly
> > figure out the mode which should be set for newly created file.
> >
> > That is why I think POSIX ACL client always must send "mode_umask"
> > attribute when creating a new file (instead of "mode" attribute).
> >
> I'll try requiring support for RFC8275 for the extensions in
> this draft. I don't see why I cannot require that?
> (If such a dependency is not permitted, someone will note that,
> I suspect.)

As umask is crucial part of new object creation in POSIX ACL model, I
would suggest to explicitly mention and describe in your extension
document what is umask and how it interacts with mode during creating a
new object on the server. This can prevent incorrect NFS4 server POSIX
ACL extension implementations by engineers not familiar with POSIX ACLs.

> 
> > >
> > > > > 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.
> > > --> For GPFS, "delete" seems to actually be defined as switch to a
> > >     minimal POSIX ACL.
> >
> > I understand what you mean.
> >
> > But "switch to a minimal POSIX ACL" means that POSIX mode is being set
> > on the object. And RFC 8881 section 6.4.2 is clear in this case
> > retrieved "ACL" must not be empty if somebody has some access.
> 
> 
> > "the ACL returned SHOULD represent the nine low-order bits of the mode
> > attribute (MODE4_R*, MODE4_W*, MODE4_X*) as described in Section 6.3.2."
> >
> > (and section 6.3.2. describes how to compute such mode)
> >
> > So you cannot simulate "mmdelacl" command via setting empty / zero
> > length NFS4 ACL attribute as this has different meaning.
> >
> Yes, I have already agreed that a zero length ACL is not a deleted
> ACL. The next draft will use a "contrived" ACL with a single ACE
> that has a type of ACE4_ERASED_ACE_TYPE to indicate a deleted ACL
> or erasure/deletion of an ACL.

It is really a good idea? I think such thing can completely confuse
regular NFS4 client which will not implement this extension and which do
not understand POSIX ACL at all and has no idea about it. Example of
such client can be Windows NFS4 client.

> 
> > >
> > > > 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;-).
> >
> > Zero length ACL in XDR structure means that ACL contains zero list of
> > ACE entries. This is just a normal ACL structure and standard evaluation
> > process is done on it. In RFC 8881 it is defined in section 6.2.1.
> >
> > "To determine if a request succeeds, the server processes each nfsace4
> > entry in order. ... When the ACL is fully processed, if there are bits
> > in the requester's mask that have not been ALLOWED or DENIED, access is
> > denied."
> >
> > So server will evaluate each nfsace4 in order. As there is empty list of
> > nfsace4, evaluation is trivial, nothing is evaluated and ACL is
> > immediately fully processed. As no bits were ALLOWED and neither DENIED
> > the last part of the quote apply "access is denied.".
> >
> > Which means that zero length ACL is for everything "access is denied".
> >
> > At least this is how I understand RFC 8881 and section 6.2.1 with empty
> > zero length ACL.
> >
> It may be a reasonable interpretation, but I do not see any explicit
> mention of zero length ACLs. Maybe the original author did intend it
> to mean "no access"?

For me it looks to be obvious. It is like mathematical operations over
empty sets. It is well-defined too.

> The next draft will avoid all mention of zero length ACLs, so this
> will no longer an issue I need to deal with.
> 
> rick
> 
> 
> > > 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?
> > >
> > > What do others think?
> >
> > I think that "zero length ACL" term can be confusing for readers. In ACL
> > context I think I never heard or read such thing. "Empty ACL" and
> > "NULL ACL" are common (but as I described above "NULL ACL" has different
> > meaning).
> >
> > It would be better to say "empty list of ACEs" (in case for ACL
> > attribute) or explicitly show an example of raw XDR structure how it
> > should look like.
> >