Re: [bug?] nfs setgid inheritance

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2022-02-23 at 09:44 +0100, Christian Brauner wrote:
> On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust wrote:
> > On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> > > On Sat, Feb 19, 2022 at 08:34:30AM +0000,
> > > suy.fnst@fujitsu.com wrote:
> > > > Hi NFS folks,
> > > >   During our xfstests, we found generic/633 fails like:
> > > > ============================================
> > > > FSTYP         -- nfs
> > > > PLATFORM      -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > > > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > > > MKFS_OPTIONS  -- 127.0.0.1:/nfsscratch
> > > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> > > > 
> > > > generic/633 0s ... [failed, exit status 1]- output mismatch
> > > > (see
> > > > /root/xfstests-dev/results//generic/633.out.bad)
> > > >     --- tests/generic/633.out   2021-05-23 14:03:08.879999997
> > > > +0800
> > > >     +++ /root/xfstests-dev/results//generic/633.out.bad 2022-
> > > > 02-19
> > > > 16:31:28.660000013 +0800
> > > >     @@ -1,2 +1,4 @@
> > > >      QA output created by 633
> > > >      Silence is golden
> > > >     +idmapped-mounts.c: 7906: setgid_create - Success -
> > > > failure:
> > > > is_setgid
> > > >     +idmapped-mounts.c: 13907: run_test - Success - failure:
> > > > create
> > > > operations in directories with setgid bit set
> > > >     ...
> > > >     (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > > > /root/xfstests-dev/results//generic/633.out.bad'  to see the
> > > > entire
> > > > diff)
> > > > Ran: generic/633
> > > > Failures: generic/633
> > > > Failed 1 of 1 tests
> > > > ============================================
> > > > 
> > > > The failed test is about setgid inheritance. 
> > > > When  a file is created with S_ISGID in the directory with
> > > > S_ISGID,
> > > > NFS  doesn't strip the  setgid bit of the new created file but
> > > > others
> > > > (ext4/xfs/btrfs) do.  They call inode_init_owner() which does
> > > > the strip after new_inode().
> > > > However, NFS has its own logical to handle inode capacities. 
> > > > As the test says the behavior can be filesystem type specific,
> > > > I'd report to you NFS guys and ask whether it's a bug or not?
> > > 
> > > Thanks for the report. I'm not sure why NFS would have different
> > > rules
> > > for setgid inheritance. So I'm inclined to think that this is
> > > simply
> > > a
> > > bug similar to what we saw in xfs and ceph. But I'll let the NFS
> > > folks
> > > determine that.
> > > 
> > > XFS is only special in so far as it has a sysctl that lets it
> > > alter
> > > sgid
> > > inheritance behavior. And it only has that to allow for legacy
> > > irix
> > > semantics iiuc.
> > 
> > OK, so how do you expect this 'idmapped-mounts' functionality to
> > work
> > on NFS? I'm not asking about this bug in particular. I'm asking
> > about
> > what this is supposed to do in general.
> 
> Just to clarify, the bug has nothing to do with idmapped mounts. The
> idmapped mount testsuite always had a vfs generic part. That vfs
> generic
> part has been made available to all filesystems supporting xfstests a
> few weeks ago. (So far this setgid inheritance bug here has been
> present
> in 3 filesystems.)

The setgid stuff works just fine with regular use, when the server is
able to determine when to clear the bit. It only breaks with this kind
of test where the server is being lied to by the client's upper layers.

> 
> > 
> > At a quick glance, it looks to me as if these idmapped mount
> > helpers
> > are just hacking different values into the inode cache
> > representation
> > of files, and then somehow expecting that to result in different
> 
> (Just to clarify, the inode cache is never changed by an idmapped
> mount
> unless a new filesystem object is created/permantently changed. The
> inode cache is left alone otherwise and ownership is only transiently
> mapped when checking permissions.)
> 
> > behaviour.
> > That's not going to work with NFS, for two reasons:
> >    1. Security is enforced by the server and not the client. If you
> >       want these values you're poking into the inode cache to
> > change
> >       the behaviour of the server, then they have to be propagated
> > by
> >       the client to that server. 
> >    2. The NFS security model is authentication based. In
> > particular,
> >       when strong authentication is being used, then my identity is
> >       established by user+password that I've logged in as to the
> >       kerberos server (or whatever). So while the idmapped mount
> > stuff
> >       may be poking values into my credential or the inode cache,
> > the
> >       server is going to ignore all that and tell me that any file
> > I
> >       create is owned by user trond@domain. It will not allow me to
> >       change file ownership or to override access permissions
> > unless
> >       trond@domain happens to be a privileged user such as root.
> > 
> > I'm pretty sure the cifs/smb client will have the same problem.
> 
> I did a POC for ceph a while back and I did also have POC patches for
> cifs/smb. For ceph e.g. the {g,u}id with which a file is to be
> created
> with or will have its ownership changed to is sent from the client to
> the server with server being responsible for deciding whether that
> creation is allowed or not.
> NFS already has a notion of idmapping via upcalls afair. Whether or
> not
> support for idmapped mounts make sense for NFS depends on how the
> current idmapping feature is implemented and what use-cases aren't
> supported by it that idmapped mounts can.

The NFS idmapping by upcalls solves a very different issue. It exists
to make sure that the client and server are both on the same page
w.r.t. account to uid/gid mapping.

i.e. it solves a problem when they are using kerberos, and are not in
agreement over uid/gid assignment to the principal account names.

It will actually cause further conflicts with idmapped mounts.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com