[RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
Hi!

I've been recently working on adding support for NFS4.1 ACLs and XDR storage
format for our vfs_nfs4acl_xattr module.

I'm mostly done, WIP patch attached. In fact the only WIP patch without a
signed-off is the new manpage.

What I'm trying to achieve is the following:

- modularize the exisiting module so choosing between existing IDL/NDR and a new
  XRD based encoding is feasible (done)

- add support for RFC 5661 NFS 4.1 based ACLs in both backends (done)

- rework how ACL inheritance is achieved and how files/directories without an
  ACL xattr get a default ACL, this now just works much the same as in
  vfs_acl_xattr (done)

- add options to choose between NFS 4.0 and 4.1 ACLs, the storage format, the
  xattr name and the default ACLA (done)

- add tests for both encodings and both NFS4 ACL levels (done)

- fix various issues along the way (done)

I've left the default behaviour as is, ie NDR marshalled blob in an xattr
"system.nfs4acl", NFS 4.0 ACLs, "everyone everything" default ACL but would like
to take the opportunity to discuss changing some of those defaults.

Is this module actually used anywhere besides for testing in autobuild? As the
module uses the system xattr namespace, the module will only work when stacked
with xattr_tdb. Any attempt to use it directly on a Linux filesystem that
supports xattrs will result in ENOTSUP from the xattr library functions, even as
root. So I doubt this is used anywhere in production, but who knows.

For the new XDR based backend I've set the default to "security.nfs4acl", so
with this backend the module is fully NT ACL compatible out-of-the-box and could
be used as a direct replacement for vfs_acl_xattr (if someone wishes).

Note that the latest NFS4 ACL patches by Andreas Gruenbacher already had a ACL
flags member in the ACL struct and ACL flags defines.

Question 1: do we want to change the default for the existing NDR backend to
"security.nfs4acl" as well?

Question 2: do we want to raise the default level to 4.1?

WIP patch attached, including the generated manpage for your convenience.

Thoughts?

Thanks!
-slow

--
Ralph Boehme, Samba Team       https://samba.org/samba/team/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

vfs-nfs4acls-xattr.patch (141K) Download Attachment
vfs_nfs4acl_xattr.8 (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Tue, Oct 24, 2017 at 10:58:13AM +0200, Ralph Böhme wrote:

> Hi!
>
> I've been recently working on adding support for NFS4.1 ACLs and XDR storage
> format for our vfs_nfs4acl_xattr module.
>
> I'm mostly done, WIP patch attached. In fact the only WIP patch without a
> signed-off is the new manpage.
>
> What I'm trying to achieve is the following:
>
> - modularize the exisiting module so choosing between existing IDL/NDR and a new
>   XRD based encoding is feasible (done)
>
> - add support for RFC 5661 NFS 4.1 based ACLs in both backends (done)
>
> - rework how ACL inheritance is achieved and how files/directories without an
>   ACL xattr get a default ACL, this now just works much the same as in
>   vfs_acl_xattr (done)
>
> - add options to choose between NFS 4.0 and 4.1 ACLs, the storage format, the
>   xattr name and the default ACLA (done)
>
> - add tests for both encodings and both NFS4 ACL levels (done)
>
> - fix various issues along the way (done)
>
> I've left the default behaviour as is, ie NDR marshalled blob in an xattr
> "system.nfs4acl", NFS 4.0 ACLs, "everyone everything" default ACL but would like
> to take the opportunity to discuss changing some of those defaults.
>
> Is this module actually used anywhere besides for testing in autobuild? As the
> module uses the system xattr namespace, the module will only work when stacked
> with xattr_tdb. Any attempt to use it directly on a Linux filesystem that
> supports xattrs will result in ENOTSUP from the xattr library functions, even as
> root. So I doubt this is used anywhere in production, but who knows.

Hmmm. Good point.

> For the new XDR based backend I've set the default to "security.nfs4acl", so
> with this backend the module is fully NT ACL compatible out-of-the-box and could
> be used as a direct replacement for vfs_acl_xattr (if someone wishes).

Yeah, that sounds like a good idea.

> Note that the latest NFS4 ACL patches by Andreas Gruenbacher already had a ACL
> flags member in the ACL struct and ACL flags defines.
>
> Question 1: do we want to change the default for the existing NDR backend to
> "security.nfs4acl" as well?
>
> Question 2: do we want to raise the default level to 4.1?
>
> WIP patch attached, including the generated manpage for your convenience.
>
> Thoughts?

I really like this. A couple of comments on the patch.

1). Shouldn't patches #2 and #3 be merged. In #2 you're
modifying a functions signature and then modifying the
caller in #3.

2). [PATCH 19/25] vfs_nfs4acl_xattr: do xattr ops as root
You need to save off the errno if SMB_VFS_NEXT_FGETXATTR()
fails before calling unbecome_root(), as this may corrupt
errno. Refer to saved_errno in the while() loop of course.

3). [PATCH 20/25] vfs_nfs4acl_xattr: add POSIX mode check and reset
Same errno issue.

FYI. I notice we don't do this in other parts of the code,
but looking at the internals of unbecome_root(), it can
call a bunch of system calls that may modify errno, so
moving forward, anytime we modify code calling unbecome_root()
after a system call, we should take care of this.

4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
In nfs4acl_get_xdrblob_size() - integer wrap checks
please on this:
size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);

Haven't gone through all patch #22 carefully, but I'd
recommend belt-and-braces integer wrap checks.

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
Hi Jeremy,

On Tue, Oct 24, 2017 at 08:56:45PM +0000, Jeremy Allison wrote:

> On Tue, Oct 24, 2017 at 10:58:13AM +0200, Ralph Böhme wrote:
> > Is this module actually used anywhere besides for testing in autobuild? As the
> > module uses the system xattr namespace, the module will only work when stacked
> > with xattr_tdb. Any attempt to use it directly on a Linux filesystem that
> > supports xattrs will result in ENOTSUP from the xattr library functions, even as
> > root. So I doubt this is used anywhere in production, but who knows.
>
> Hmmm. Good point.
>
> > For the new XDR based backend I've set the default to "security.nfs4acl", so
> > with this backend the module is fully NT ACL compatible out-of-the-box and could
> > be used as a direct replacement for vfs_acl_xattr (if someone wishes).
>
> Yeah, that sounds like a good idea.
I'm in favour of changing the default for the old NDR backend as well. What do
you think? Andrew? It's your code. :)

> > Note that the latest NFS4 ACL patches by Andreas Gruenbacher already had a ACL
> > flags member in the ACL struct and ACL flags defines.
> >
> > Question 1: do we want to change the default for the existing NDR backend to
> > "security.nfs4acl" as well?
> >
> > Question 2: do we want to raise the default level to 4.1?
> >
> > WIP patch attached, including the generated manpage for your convenience.
> >
> > Thoughts?
>
> I really like this. A couple of comments on the patch.
>
> 1). Shouldn't patches #2 and #3 be merged. In #2 you're
> modifying a functions signature and then modifying the
> caller in #3.
#2 modifies the function and the callers, it compiles just fine on its own and I
think it makes sense to keep them as seperate commits. #2 does a minor
preparatory modification to the function arguments and updates the callers
accordingly. #3 makes the changed function public and moves it somewhere else
without changing the function signature.

> 2). [PATCH 19/25] vfs_nfs4acl_xattr: do xattr ops as root
> You need to save off the errno if SMB_VFS_NEXT_FGETXATTR()
> fails before calling unbecome_root(), as this may corrupt
> errno. Refer to saved_errno in the while() loop of course.

Gna, sorry, fixed. :)

> 3). [PATCH 20/25] vfs_nfs4acl_xattr: add POSIX mode check and reset
> Same errno issue.

Also fixed.

> FYI. I notice we don't do this in other parts of the code,
> but looking at the internals of unbecome_root(), it can
> call a bunch of system calls that may modify errno, so
> moving forward, anytime we modify code calling unbecome_root()
> after a system call, we should take care of this.
>
> 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> In nfs4acl_get_xdrblob_size() - integer wrap checks
> please on this:
> size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
Done. Thanks for spotting this! I hope I guarded all relevant places in the
attached updated patchset.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/samba/team/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

vfs-nfs4acls-xattr.patch (142K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> I'm in favour of changing the default for the old NDR backend as well. What do
> you think? Andrew? It's your code. :)

ping.

> > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > please on this:
> > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
>
> Done. Thanks for spotting this! I hope I guarded all relevant places in the
> attached updated patchset.

Another ping. :=)

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:

> On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > I'm in favour of changing the default for the old NDR backend as well. What do
> > you think? Andrew? It's your code. :)
>
> ping.
>
> > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > please on this:
> > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> >
> > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > attached updated patchset.
>
> Another ping. :=)

Next on my list to review once I've looked at the kernel
oplock bugs, honest ! :-).

FYI. Is this merely a WIP, or do you want me to push if
it passes review ?

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:

> On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > you think? Andrew? It's your code. :)
> >
> > ping.
> >
> > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > please on this:
> > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > >
> > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > attached updated patchset.
> >
> > Another ping. :=)
>
> Next on my list to review once I've looked at the kernel
> oplock bugs, honest ! :-).

Oh, so I hope you come out of that in a sane state of mind. :)

> FYI. Is this merely a WIP, or do you want me to push if
> it passes review ?

Still waiting for feedback on the questions I brought up in the first mail, from
you and especially from Andrew as it's his code.

Andrew?

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Sat, 2017-10-28 at 19:09 +0200, Ralph Böhme wrote:

> On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:
> > On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > > you think? Andrew? It's your code. :)
> > >
> > > ping.
> > >
> > > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > > please on this:
> > > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > > >
> > > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > > attached updated patchset.
> > >
> > > Another ping. :=)
> >
> > Next on my list to review once I've looked at the kernel
> > oplock bugs, honest ! :-).
>
> Oh, so I hope you come out of that in a sane state of mind. :)
>
> > FYI. Is this merely a WIP, or do you want me to push if
> > it passes review ?
>
> Still waiting for feedback on the questions I brought up in the first mail, from
> you and especially from Andrew as it's his code.
>
> Andrew?

I'm fine with the concept - my role was to dispel the myth that this
was un-testable, I'm not wedded to the NDR encoding.

I'm very happy for the defaults to change, I can't imagine it has been
used in production.

I've got a busy week this week so I'm not sure I'll be able to get you
a formal review however.

Sorry,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Sun, Oct 29, 2017 at 09:28:46AM +1300, Andrew Bartlett wrote:

> On Sat, 2017-10-28 at 19:09 +0200, Ralph Böhme wrote:
> > On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:
> > > On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > > > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > > > you think? Andrew? It's your code. :)
> > > >
> > > > ping.
> > > >
> > > > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > > > please on this:
> > > > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > > > >
> > > > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > > > attached updated patchset.
> > > >
> > > > Another ping. :=)
> > >
> > > Next on my list to review once I've looked at the kernel
> > > oplock bugs, honest ! :-).
> >
> > Oh, so I hope you come out of that in a sane state of mind. :)
> >
> > > FYI. Is this merely a WIP, or do you want me to push if
> > > it passes review ?
> >
> > Still waiting for feedback on the questions I brought up in the first mail, from
> > you and especially from Andrew as it's his code.
> >
> > Andrew?
>
> I'm fine with the concept - my role was to dispel the myth that this
> was un-testable, I'm not wedded to the NDR encoding.

:-))) Just in case, I'm keeping the default encoding as NDR.

> I'm very happy for the defaults to change, I can't imagine it has been
> used in production.

Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:

> On Sun, Oct 29, 2017 at 09:28:46AM +1300, Andrew Bartlett wrote:
> > On Sat, 2017-10-28 at 19:09 +0200, Ralph Böhme wrote:
> > > On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:
> > > > On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > > > > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > > > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > > > > you think? Andrew? It's your code. :)
> > > > >
> > > > > ping.
> > > > >
> > > > > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > > > > please on this:
> > > > > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > > > > >
> > > > > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > > > > attached updated patchset.
> > > > >
> > > > > Another ping. :=)
> > > >
> > > > Next on my list to review once I've looked at the kernel
> > > > oplock bugs, honest ! :-).
> > >
> > > Oh, so I hope you come out of that in a sane state of mind. :)
> > >
> > > > FYI. Is this merely a WIP, or do you want me to push if
> > > > it passes review ?
> > >
> > > Still waiting for feedback on the questions I brought up in the first mail, from
> > > you and especially from Andrew as it's his code.
> > >
> > > Andrew?
> >
> > I'm fine with the concept - my role was to dispel the myth that this
> > was un-testable, I'm not wedded to the NDR encoding.
>
> :-))) Just in case, I'm keeping the default encoding as NDR.
>
> > I'm very happy for the defaults to change, I can't imagine it has been
> > used in production.
>
> Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".

FYI. I'm reviewing your original patchset right now.

Do you want me to push if good, or shall I wait for a follow-up
patchset ?

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Thu, Nov 02, 2017 at 08:37:21AM -0700, Jeremy Allison wrote:

> On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > On Sun, Oct 29, 2017 at 09:28:46AM +1300, Andrew Bartlett wrote:
> > > On Sat, 2017-10-28 at 19:09 +0200, Ralph Böhme wrote:
> > > > On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:
> > > > > On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > > > > > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > > > > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > > > > > you think? Andrew? It's your code. :)
> > > > > >
> > > > > > ping.
> > > > > >
> > > > > > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > > > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > > > > > please on this:
> > > > > > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > > > > > >
> > > > > > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > > > > > attached updated patchset.
> > > > > >
> > > > > > Another ping. :=)
> > > > >
> > > > > Next on my list to review once I've looked at the kernel
> > > > > oplock bugs, honest ! :-).
> > > >
> > > > Oh, so I hope you come out of that in a sane state of mind. :)
> > > >
> > > > > FYI. Is this merely a WIP, or do you want me to push if
> > > > > it passes review ?
> > > >
> > > > Still waiting for feedback on the questions I brought up in the first mail, from
> > > > you and especially from Andrew as it's his code.
> > > >
> > > > Andrew?
> > >
> > > I'm fine with the concept - my role was to dispel the myth that this
> > > was un-testable, I'm not wedded to the NDR encoding.
> >
> > :-))) Just in case, I'm keeping the default encoding as NDR.
> >
> > > I'm very happy for the defaults to change, I can't imagine it has been
> > > used in production.
> >
> > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
>
> FYI. I'm reviewing your original patchset right now.
>
> Do you want me to push if good, or shall I wait for a follow-up
> patchset ?

oh, please wait a few more hours. I'm currently runnig one more final full
private autobuild after changing the defaults.

Individual tests passed the raw.acls tests, just want to make sure the full
patchset survives a full autobuild, currently at

[1155(5855)/2211 at 1h15m11s] samba4.rpc.mgmt with validate(ad_dc_ntvfs)

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Thu, Nov 02, 2017 at 04:52:23PM +0100, Ralph Böhme wrote:

> On Thu, Nov 02, 2017 at 08:37:21AM -0700, Jeremy Allison wrote:
> > On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > > On Sun, Oct 29, 2017 at 09:28:46AM +1300, Andrew Bartlett wrote:
> > > > On Sat, 2017-10-28 at 19:09 +0200, Ralph Böhme wrote:
> > > > > On Fri, Oct 27, 2017 at 01:56:04PM -0700, Jeremy Allison wrote:
> > > > > > On Fri, Oct 27, 2017 at 10:32:33PM +0200, Ralph Böhme wrote:
> > > > > > > On Wed, Oct 25, 2017 at 10:59:38AM +0200, Ralph Böhme wrote:
> > > > > > > > I'm in favour of changing the default for the old NDR backend as well. What do
> > > > > > > > you think? Andrew? It's your code. :)
> > > > > > >
> > > > > > > ping.
> > > > > > >
> > > > > > > > > 4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
> > > > > > > > > In nfs4acl_get_xdrblob_size() - integer wrap checks
> > > > > > > > > please on this:
> > > > > > > > > size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);
> > > > > > > >
> > > > > > > > Done. Thanks for spotting this! I hope I guarded all relevant places in the
> > > > > > > > attached updated patchset.
> > > > > > >
> > > > > > > Another ping. :=)
> > > > > >
> > > > > > Next on my list to review once I've looked at the kernel
> > > > > > oplock bugs, honest ! :-).
> > > > >
> > > > > Oh, so I hope you come out of that in a sane state of mind. :)
> > > > >
> > > > > > FYI. Is this merely a WIP, or do you want me to push if
> > > > > > it passes review ?
> > > > >
> > > > > Still waiting for feedback on the questions I brought up in the first mail, from
> > > > > you and especially from Andrew as it's his code.
> > > > >
> > > > > Andrew?
> > > >
> > > > I'm fine with the concept - my role was to dispel the myth that this
> > > > was un-testable, I'm not wedded to the NDR encoding.
> > >
> > > :-))) Just in case, I'm keeping the default encoding as NDR.
> > >
> > > > I'm very happy for the defaults to change, I can't imagine it has been
> > > > used in production.
> > >
> > > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
> >
> > FYI. I'm reviewing your original patchset right now.
> >
> > Do you want me to push if good, or shall I wait for a follow-up
> > patchset ?
>
> oh, please wait a few more hours. I'm currently runnig one more final full
> private autobuild after changing the defaults.
>
> Individual tests passed the raw.acls tests, just want to make sure the full
> patchset survives a full autobuild, currently at
>
> [1155(5855)/2211 at 1h15m11s] samba4.rpc.mgmt with validate(ad_dc_ntvfs)

OK, let me know when you think it's good.

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".

attached.

Please review & push if happy. Thanks!

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

nfsv41.acls (149K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Thu, Nov 02, 2017 at 07:44:21PM +0100, Ralph Böhme via samba-technical wrote:
> On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
>
> attached.
>
> Please review & push if happy. Thanks!

Doing a thorough review on this.

One quick question - the default for

nfs4acl_xattr:default acl style

is everyone. That's the least secure one.

Why was that chosen ? Did I miss or forget
the discussion ?

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Mon, Nov 06, 2017 at 04:26:13PM -0800, Jeremy Allison wrote:

> On Thu, Nov 02, 2017 at 07:44:21PM +0100, Ralph Böhme via samba-technical wrote:
> > On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
> >
> > attached.
> >
> > Please review & push if happy. Thanks!
>
> Doing a thorough review on this.
>
> One quick question - the default for
>
> nfs4acl_xattr:default acl style
>
> is everyone. That's the least secure one.
>
> Why was that chosen ? Did I miss or forget
> the discussion ?

that's just the current behaviour of the unpatched module...

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Tue, Nov 07, 2017 at 01:16:27PM +0100, Ralph Böhme wrote:

> On Mon, Nov 06, 2017 at 04:26:13PM -0800, Jeremy Allison wrote:
> > On Thu, Nov 02, 2017 at 07:44:21PM +0100, Ralph Böhme via samba-technical wrote:
> > > On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > > > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > > > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
> > >
> > > attached.
> > >
> > > Please review & push if happy. Thanks!
> >
> > Doing a thorough review on this.
> >
> > One quick question - the default for
> >
> > nfs4acl_xattr:default acl style
> >
> > is everyone. That's the least secure one.
> >
> > Why was that chosen ? Did I miss or forget
> > the discussion ?
>
> that's just the current behaviour of the unpatched module...

OK, RB+ and pushed with just one change, in make_default_acl_everyone()
you had:

struct security_ace aces[4];

which was cut-n-paste left over - we are only adding one
ace here so it should be:

struct security_ace aces[1];

Cheers !

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Tue, Nov 07, 2017 at 01:47:33PM -0800, Jeremy Allison via samba-technical wrote:

> On Tue, Nov 07, 2017 at 01:16:27PM +0100, Ralph Böhme wrote:
> > On Mon, Nov 06, 2017 at 04:26:13PM -0800, Jeremy Allison wrote:
> > > On Thu, Nov 02, 2017 at 07:44:21PM +0100, Ralph Böhme via samba-technical wrote:
> > > > On Thu, Nov 02, 2017 at 11:56:23AM +0100, Ralph Böhme wrote:
> > > > > Ok, I'll then follow-up with a patchset that changes the ACL version to 4.1 and
> > > > > the xattr name to "security.nfs4acl_ndr" instead of "security.nfs4acl".
> > > >
> > > > attached.
> > > >
> > > > Please review & push if happy. Thanks!
> > >
> > > Doing a thorough review on this.
> > >
> > > One quick question - the default for
> > >
> > > nfs4acl_xattr:default acl style
> > >
> > > is everyone. That's the least secure one.
> > >
> > > Why was that chosen ? Did I miss or forget
> > > the discussion ?
> >
> > that's just the current behaviour of the unpatched module...
>
> OK, RB+ and pushed with just one change, in make_default_acl_everyone()
> you had:
>
> struct security_ace aces[4];
>
> which was cut-n-paste left over - we are only adding one
> ace here so it should be:
>
> struct security_ace aces[1];

Ah. On submission to autobuild it caught a use-before-initialize
error I missed. Can you also fix the initialization errors below
and re-submit ?

Sorry,

Jeremy.

In:

source3/modules/nfs4acl_xattr_xdr.c

static NTSTATUS nfs4acl_to_smb4acl(struct vfs_handle_struct *handle,
                                   TALLOC_CTX *mem_ctx,
                                   nfsacl41 *nacl,
                                   struct SMB4ACL_T **_smb4acl)
{
        struct nfs4acl_config *config = NULL;
        struct SMB4ACL_T *smb4acl = NULL;
        unsigned nfsacl41_flag;
        uint16_t smb4acl_flags;            <------------------------ uninitialized.
        unsigned naces = nfs4acl_get_naces(nacl);
        int i;

        SMB_VFS_HANDLE_GET_DATA(handle, config,
                                struct nfs4acl_config,
                                return NT_STATUS_INTERNAL_ERROR);

        smb4acl = smb_create_smb4acl(mem_ctx);
        if (smb4acl == NULL) {
                return NT_STATUS_INTERNAL_ERROR;
        }

        if (config->nfs_version > ACL4_XATTR_VERSION_40) {
                nfsacl41_flag = nfs4acl_get_flags(nacl);
                smb4acl_flags = nfs4acl_to_smb4acl_flags(nfsacl41_flag); <--------- only set here.
                smbacl4_set_controlflags(smb4acl, smb4acl_flags);
        }

        DBG_DEBUG("flags [%x] nace [%u]\n", smb4acl_flags, naces); <--------- use without initialize.


Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Tue, Nov 07, 2017 at 02:15:33PM -0800, Jeremy Allison wrote:
> > OK, RB+ and pushed with just one change, in make_default_acl_everyone()
> > you had:
> >
> > struct security_ace aces[4];
> >
> > which was cut-n-paste left over - we are only adding one
> > ace here so it should be:
> >
> > struct security_ace aces[1];

good catch, thanks!

> Ah. On submission to autobuild it caught a use-before-initialize
> error I missed.

Hm, I wonder why that doesn't fail here with --pick-developer.

> Can you also fix the initialization errors below
> and re-submit ?

Attached.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

Samba - samba-technical mailing list
On Tue, Nov 07, 2017 at 11:31:42PM +0100, Ralph Böhme wrote:

> On Tue, Nov 07, 2017 at 02:15:33PM -0800, Jeremy Allison wrote:
> > > OK, RB+ and pushed with just one change, in make_default_acl_everyone()
> > > you had:
> > >
> > > struct security_ace aces[4];
> > >
> > > which was cut-n-paste left over - we are only adding one
> > > ace here so it should be:
> > >
> > > struct security_ace aces[1];
>
> good catch, thanks!
>
> > Ah. On submission to autobuild it caught a use-before-initialize
> > error I missed.
>
> Hm, I wonder why that doesn't fail here with --pick-developer.
>
> > Can you also fix the initialization errors below
> > and re-submit ?
>
> Attached.
now.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

nfsv41-acls.patch (149K) Download Attachment