[PATCH] Patches required for POSIX ACL support of GPOs

[Jeremy Allison]

On Thu, May 10, 2012 at 11:38:48AM +1000, Andrew Bartlett wrote:
> These patches are in my master-devel branch, and are needed for GPO
> support to create the correct POSIX ACL.  I would very much appreciate
> review, so we can consider enabling s3fs by default, and making the 4.0
> Beta release.
> 
> This patch is needed for the same idea, in the NFSv4 ACL code.  It removes the sidmap as discussed, but I can't test it.
> 
> commit 5193b7f00181831c3d631e8b6f88cd3b783fd577
> Author: Andrew Bartlett <abartlet at samba.org>
> Date:   Mon May 7 08:48:24 2012 +1000
> 
>     s3-nfs4acls: Remove lookup_sid and sidmap from NFSv4 ACL mapping and check gid first
>     
>     By checking just the IDMAP, and by removing the sidmap and lookup_sid calls, we support
>     IDMAP_BOTH.  This is because by checking for a mapping to a GID first, we can rely on
>     the fact that IDMAP_BOTH will resolve to a GID.
>     
>     If the sidmap idea is valued - it allows multiple SIDs to map to a single unix ID, this should
>     be done in the IDMAP layer.
>     
>     Andrew Bartlett

Hmmmmm. I'm just not sure of the above patch. I'll look more carefully
at this.

> This came up when looking over the debug logs while fixing another bug,
> but I think is worthwhile.  It isn't strictly required, but avoids going
> via NSS to build the fake token. 
> 
> commit abf6ca1c560e1bec5656d830c61227cfb8af6133
> Author: Andrew Bartlett <abartlet at samba.org>
> Date:   Thu May 10 09:19:46 2012 +1000
> 
>     s3-smbd: Create a shortcut for building the token of a user by SID for posix_acls
>     
>     When a user owns a file, but does not have specific permissions on that file, we need to
>     make up the user permissions.  This change ensures that the first thing that we do
>     is to look up the SID, and confirm it is a user.  Then, we avoid the getpwnam()
>     and directly create the token via the SID.
>     
>     Andrew Bartlett

Unfortunately the above patch is wrong, and has a really serious
side effect.

You add a new call:

+static NTSTATUS create_token_from_sid(TALLOC_CTX *mem_ctx, 
+                                     const struct dom_sid *user_sid,
+                                     bool is_guest,
+                                     uid_t *uid, gid_t *gid,
+                                     char **found_username,
+                                     struct security_token **token)

which you rewrite create_token_from_username() to call (this
is a nice idea BTW, and good in theory).

Note that the user_sid parameter is "const struct dom_sid *"
(emphasis on the const).

Inside that new function you do :

-               uid_to_unix_users_sid(*uid, &user_sid);
+               uid_to_unix_users_sid(*uid, &tmp_sid);
+               user_sid = &tmp_sid;

BOOP ! BOOP ! WARNING ! GO NO FURTHER !!!!

You just overwrote the passed in user_sid, which is explicitly
supposted to be const (yeah, I know it's a const pointer, but
still....).

When you're invoking a new "tmp_sid" variable to get around a
const violation, this is a sign you're doing things wrong.

Now this doesn't matter when calling from the new
create_token_from_username() call, as you throw away
the SID anyway (and this is a hint that user_sid in
the new create_token_from_sid() shouldn't be defined
as const).

However it's *fatal* when you call it from your new :

+bool user_sid_in_group_sid(const struct dom_sid *sid, const struct dom_sid *group_sid)

which you call directly from the change in source3/smbd/posix_acls.c
to uid_entry_in_group().

+       return user_sid_in_group_sid(&uid_ace->trustee, &group_ace->trustee);

This *COULD* modify &uid_ace->trustee now - which I really
don't think is what we want.

It's a shame, as this looks like a nice optimization, but is
it really *needed* to make the code work ?

This is where I'm up to so far, just wanted to let you know.

The patchset is good in theory, but you should try not to
mix up optimizations that look like a good idea with real
code changes to make the thing just basically work, and this
patchset is a mix of the two.

I don't think this one is needed to make it just work, so
I'm going to omit this in my updated patchset.

Jeremy.