Bug 10286 - %G is not expanded in a service path
%G is not expanded in a service path
Status: REOPENED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.1.0
All All
: P5 normal
: ---
Assigned To: Andreas Schneider
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-21 19:26 UTC by Andreas Schneider
Modified: 2016-01-21 22:04 UTC (History)
3 users (show)

See Also:


Attachments
Patch for master (981 bytes, patch)
2013-11-21 19:29 UTC, Andreas Schneider
ambi: review-
jra: review-
Details
Patch for master v2 (1.13 KB, patch)
2013-11-27 16:22 UTC, Andreas Schneider
no flags Details
v4-1-test patch (1.41 KB, patch)
2013-12-11 13:11 UTC, Andreas Schneider
ambi: review+
Details
v4-0-test patch (1.41 KB, patch)
2013-12-11 13:11 UTC, Andreas Schneider
ambi: review+
Details
Patchset for master to fix the regression (26.15 KB, patch)
2016-01-21 20:48 UTC, Michael Adam
jra: review+
obnox: review? (asn)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2013-11-21 19:26:40 UTC
%G is not expaned in a service path.

Using this in a share name:

[test_sub]
    path = /homes/D%D/u%u/U%U/g%g/G%G

does not expand %G. The following patch fixes this.
Comment 1 Andreas Schneider 2013-11-21 19:29:12 UTC
Created attachment 9461 [details]
Patch for master
Comment 2 Christian Ambach 2013-11-21 19:51:37 UTC
Comment on attachment 9461 [details]
Patch for master

man smb.conf says:
      %G primary group name of %U.

If I am not mistaken, your patch will print the gid as a number. Right?
Comment 3 Jeremy Allison 2013-11-21 23:38:34 UTC
Comment on attachment 9461 [details]
Patch for master

This doesn't look right to me. Look at the definition of %G in the smb.conf man page:

-------------------------------------------
       %U
           session username (the username that the client wanted, not necessarily the same as the one they got).

       %G
           primary group name of %U.
-------------------------------------------

This needs to be a string, not a number I think.

Jeremy.
Comment 4 Andreas Schneider 2013-11-22 09:52:36 UTC
Oh then the bug is incorrect. Cause there is no %U (session username) so also no %G. There is only a %u which is the service username and %g is the service groupname.
Comment 5 Andreas Schneider 2013-11-22 09:55:29 UTC
Well there might be more broken. We pass the session username and session groupname to talloc_sub_advanced() but it should be the service username and service groupname so probably 'force user' needs to be considered, right?
Comment 6 Andreas Schneider 2013-11-27 16:22:06 UTC
Created attachment 9486 [details]
Patch for master v2
Comment 7 Andreas Schneider 2013-12-04 22:24:02 UTC
Ping!
Comment 8 Christian Ambach 2013-12-07 10:26:40 UTC
(In reply to comment #5)
> Well there might be more broken. We pass the session username and session
> groupname to talloc_sub_advanced() but it should be the service username and
> service groupname so probably 'force user' needs to be considered, right?

make_connection_snum calls make_connection_snum that replaces the information in the session info with the forced values. So we should be good in that regard.
Comment 9 Andreas Schneider 2013-12-07 14:42:04 UTC
Yes, I know that now :) I've fixed it correctly, but someone needs to review the patch ...
Comment 10 Christian Ambach 2013-12-10 13:57:00 UTC
Patch is on its way through autobuild
Comment 11 Andreas Schneider 2013-12-11 13:11:06 UTC
Created attachment 9522 [details]
v4-1-test patch
Comment 12 Andreas Schneider 2013-12-11 13:11:54 UTC
Created attachment 9523 [details]
v4-0-test patch
Comment 13 Christian Ambach 2013-12-11 20:04:41 UTC
Hi Karolin,

please include in 4.0.x and 4.1.x
Comment 14 Karolin Seeger 2013-12-14 19:23:46 UTC
Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 15 Karolin Seeger 2013-12-14 20:54:32 UTC
(In reply to comment #14)
> Pushed to autobuild-v4-1-test and autobuild-v4-0-test.

Both autobuilds failed, retrying...
Comment 16 Karolin Seeger 2013-12-16 08:13:02 UTC
Pushed to v4-1-test and v4-0-test.
Closing out bug report.

Thanks!
Comment 17 Karolin Seeger 2013-12-30 18:25:49 UTC
(In reply to comment #16)
> Pushed to v4-1-test and v4-0-test.
> Closing out bug report.
> 
> Thanks!

Pushed to v4-1-test, but not to v4-0-test yet.
Retrying...
Comment 18 Karolin Seeger 2013-12-31 18:48:56 UTC
Patch for 4.0 breaks autobuild.
Re-assigning to Andreas.
Comment 19 TioNisla 2015-07-21 12:26:18 UTC
%G is not expaned in a service "valid users = @%G"

all versions after 4.1.3 and all 4.2.x

[workgroups]
    path = /home/samba/groups/%G
    valid users = @%G

---------8<-----------
[2015/07/21 14:39:05.402715,  3, pid=3439, effective(0, 0), real(0, 0)] ../libcli/security/dom_sid.c:208(dom_sid_parse_endp)
  string_to_sid: SID @%G is not in a valid format
---------8<-----------
Comment 20 Michael Adam 2016-01-21 10:06:12 UTC
I also just saw another regression where

logon script = %G.bat

does not get resolved on a (NT4) dc.
Comment 21 Michael Adam 2016-01-21 10:53:55 UTC
(In reply to Michael Adam from comment #20)
> I also just saw another regression where
> 
> logon script = %G.bat
> 
> does not get resolved on a (NT4) dc.

Problem seems to be this:
- talloc_sub_basic() for %G calls Get_Pwnam_alloc() on the username
  handed in to get the group from the group struct.
- The fix in this bug report uses domain\username instead
  of just username to call getpwnam, if domain is not NULL.
- ==> For the case of a user of the local SAM, this is not
      a name the will resolve. Hence the regression.

I assume that the fix for this bug has only fixed the case of a
non-local / domain user?

Need to find out what the right thing to do (TM) is here.
Possibly checking whether the domain name is our own sam name
and then omitting it...
Comment 22 Michael Adam 2016-01-21 20:48:54 UTC
Created attachment 11784 [details]
Patchset for master to fix the regression

This is a proposed patchset for master to fix the regression.
I yet have to test it.
And I have a couple of cleanup patches on top.

Should we open a new BZ for this or can we re-use this one?
Comment 23 Michael Adam 2016-01-21 20:49:28 UTC
Comment on attachment 11784 [details]
Patchset for master to fix the regression

what about this patch for master?
Comment 24 Jeremy Allison 2016-01-21 22:04:26 UTC
Comment on attachment 11784 [details]
Patchset for master to fix the regression

LGTM. Pushed to master.