Bug 2191 - [patch] %G / %g substitution does not correctly work in 'template homedir'
Summary: [patch] %G / %g substitution does not correctly work in 'template homedir'
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 3.6.3
Hardware: All All
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
: 1676 2192 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-12-26 23:21 UTC by takeuchi
Modified: 2020-12-11 08:05 UTC (History)
8 users (show)

See Also:


Attachments
a possible fix (2.64 KB, patch)
2004-12-26 23:22 UTC, takeuchi
no flags Details
Ported first third of the patch (1.36 KB, patch)
2012-03-12 17:03 UTC, Lars Müller
no flags Details
Patch for master (11.90 KB, patch)
2013-11-18 15:51 UTC, Andreas Schneider
gd: review+
jra: review+
Details
v4-1-test patch (12.25 KB, patch)
2013-11-25 07:30 UTC, Andreas Schneider
gd: review+
asn: review? (jra)
Details
v4-0-test patch (12.25 KB, patch)
2013-11-25 07:32 UTC, Andreas Schneider
gd: review+
asn: review? (jra)
Details
additional v4-1-test patch (1.04 KB, patch)
2014-01-17 10:26 UTC, Andreas Schneider
vl: review+
metze: review-
Details
additional v4-0-test patch (1.04 KB, patch)
2014-01-17 10:26 UTC, Andreas Schneider
vl: review+
metze: review-
Details
additional v4-1-test patch (1.22 KB, patch)
2014-02-05 09:00 UTC, Andreas Schneider
metze: review+
Details
additional v4-0-test patch (1.22 KB, patch)
2014-02-05 09:23 UTC, Andreas Schneider
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description takeuchi 2004-12-26 23:21:16 UTC
According to a report of bug id 1676,

Althogh commands below are written in smb.conf, "%G" never changes into a group 
name.
  security = ADS 
  template homedir = /home/%G/%U 
  template shaell = /bin/bash
  winbind use default domain = yes

For exemple:
With this directive, when using "getent", we should get:  
$ getent passwd  
....  
miracle2:x:10006:40003:miracle2:/home/dev/miracle2:/bin/bash
....  
  
Here is what I actually get:  
miracle2:x:10006:40003:miracle2:/home/40003/miracle2:/bin/bash


With investigating file "source/lib/substitute.c", I founded that the return 
value of "gidtoname" function is not a group name, but is still gid.
Comment 1 takeuchi 2004-12-26 23:22:07 UTC
Created attachment 868 [details]
a possible fix
Comment 2 takeuchi 2004-12-27 01:52:53 UTC
*** Bug 2192 has been marked as a duplicate of this bug. ***
Comment 3 Gerald (Jerry) Carter (dead mail address) 2005-02-17 10:04:08 UTC
*** Bug 1676 has been marked as a duplicate of this bug. ***
Comment 4 Seb H 2012-03-12 12:55:31 UTC
Is there any news on when this fox will be integrated? at the moment I have to ln -sf UIDForStaffGroup /home/staff and it looks really messy.

I'm using version 3.6.3
Comment 5 Lars Müller 2012-03-12 17:03:08 UTC
Created attachment 7379 [details]
Ported first third of the patch

@Volker: for the remaining part I'm not sure if we need an additional configuration parameter as we already have with lp_template_homedir() and lp_template_shell()
Comment 6 Lars Müller 2012-03-12 17:12:24 UTC
talloc_sub_specified() needs to be called with the additional argument, grpname
Comment 7 Lars Müller 2012-03-12 22:23:16 UTC
Moving to Samba 3.6.
Comment 8 Christian Ambach 2012-03-13 09:47:30 UTC
%G does not work in the path parameter when Samba is used in an AD environment,

e.g.
path = /homes/D%D/u%u/U%U/g%g/G%G

gets expanded to
/homes/DDOMAIN/uDOMAIN\administrator/Uadministrator/gDOMAIN\domain users/G%G

%G remains untouched

Debugging this shows that alloc_sub_basic gets passed smb_name = administrator and tries to resolve this name via Get_Pwnam_alloc, but this fails:

  Get_Pwnam_internals didn't find user [administrator]!
Comment 9 smb 2013-02-12 04:25:49 UTC
i got last release(3.6.12) from samba.org and compile it to debian,but samba can NOT resolve %G or %g in "template homedir". i checked it by "wbinfo -i <i>username</i>" ,and i have another problem ,i configed nsswitch.conf but i cant see my DC users by "getend passwd" command.
i want to know ,it`s still a bug or i configed samba incorrect.
Comment 10 smb 2013-02-12 05:18:07 UTC
i got last release(3.6.12) from samba.org and compile it to debian,but samba can NOT resolve %G or %g in "template homedir". i checked it by "wbinfo -i <i>username</i>" ,and i have another problem ,i configed nsswitch.conf but i cant see my DC users by "getend passwd" command.
i want to know ,it`s still a bug or i configed samba incorrect.
Comment 11 Andreas Schneider 2013-11-18 09:59:21 UTC
*** Bug 9652 has been marked as a duplicate of this bug. ***
Comment 12 Andreas Schneider 2013-11-18 15:51:01 UTC
Created attachment 9432 [details]
Patch for master

If you're fine with the two patches, please push them to master. I will backport them once they hit the repo.
Comment 13 Christian Ambach 2013-11-18 20:03:58 UTC
Do they fix the problem described in comment #8?
Comment 14 Andreas Schneider 2013-11-19 07:48:52 UTC
I don't really understand what you want to say in comment #8. But I developed this with winbind joined to AD.

samba:~ # getent passwd LEVEL1+bob1
LEVEL1+bob1:*:100001106:100000513:Bob One:/home/LEVEL1/Domain Users/bob1:/bin/bash
Comment 15 Andreas Schneider 2013-11-19 09:31:55 UTC
samba:~ # getent passwd LEVEL1+Administrator
LEVEL1+administrator:*:100000500:100000513:Administrator:/home/LEVEL1/Domain Users/administrator:/bin/bash
samba:~ # getent passwd DISCWORLD+Administrator
DISCWORLD+administrator:*:1000000:1000012:Administrator:/home/DISCWORLD/Domain Users/administrator:/bin/bash
Comment 16 Andreas Schneider 2013-11-19 09:55:12 UTC
Ambi I'm not sure which codepath you are talking about but in this case 'security = ADS' we are inside winbind and call gidtoname(gid) which calls getgrgid(). As we are in winbind we have an environment variable set which prevents winbind talking to itself to not end up in an infinite loop.

What I do is to call directly the winbind function in this case and pass the group name down.
Comment 17 Andreas Schneider 2013-11-21 08:03:50 UTC
Ambi?
Comment 18 Jeremy Allison 2013-11-21 17:20:55 UTC
I will try and get to this today or tomorrow. Just FYI.

Jeremy.
Comment 19 Christian Ambach 2013-11-21 18:42:49 UTC
(In reply to comment #14)
> I don't really understand what you want to say in comment #8. But I developed
> this with winbind joined to AD.
> 
> samba:~ # getent passwd LEVEL1+bob1
> LEVEL1+bob1:*:100001106:100000513:Bob One:/home/LEVEL1/Domain
> Users/bob1:/bin/bash


My example was made using 
path = /homes/D%D/u%u/U%U/g%g/G%G

for a share in smb.conf.

In that case, make_connection_snum() calls talloc_sub_advanced() which does not treat %G (only %g) and then calls talloc_sub_basic() which fails to perform the proper resolution. 

As I currently do not have an environment handy to test this out, it would be good if you could give this a quick test on your own by using such a path definition in smb.conf and see what it gets resolved to.
Comment 20 Andreas Schneider 2013-11-21 19:29:58 UTC
Yes, this doesn't work and I would say is unrelated to this bug report. These are different codepath. The bug I fixed is a problem in winbind and you describe a bug in smbd. So you should open a bug for %G not treated by smbd.

I've opened bug #10286 for that.
Comment 21 Christian Ambach 2013-11-21 19:37:30 UTC
(In reply to comment #20)
> Yes, this doesn't work and I would say is unrelated to this bug report. These
> are different codepath. The bug I fixed is a problem in winbind and you
> describe a bug in smbd. So you should open a bug for %G not treated by smbd.

Agreed.. looks like I did not carefully enough at the scope of this bug.
 
> I've opened bug #10286 for that.

Thanks
Comment 22 Jeremy Allison 2013-11-21 23:07:32 UTC
Comment on attachment 9432 [details]
Patch for master

LGTM. Will push to master.
Comment 23 Andreas Schneider 2013-11-25 07:30:41 UTC
Created attachment 9477 [details]
v4-1-test patch
Comment 24 Andreas Schneider 2013-11-25 07:32:09 UTC
Created attachment 9478 [details]
v4-0-test patch
Comment 25 Guenther Deschner 2013-11-25 16:36:35 UTC
Comment on attachment 9432 [details]
Patch for master

looks good
Comment 26 Guenther Deschner 2013-11-25 16:36:43 UTC
Comment on attachment 9477 [details]
v4-1-test patch

looks good
Comment 27 Guenther Deschner 2013-11-25 16:36:54 UTC
Comment on attachment 9478 [details]
v4-0-test patch

looks good
Comment 28 Andreas Schneider 2013-11-25 16:41:25 UTC
Karolin, please add the patches to 4.0 and 4.1. If someone is interested I have patches for 3.6 too. Thanks!
Comment 29 Karolin Seeger 2013-11-26 19:35:45 UTC
Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 30 Karolin Seeger 2013-11-27 10:06:34 UTC
Pushed to v4-1-test and v4-0-test.
Closing out bug report.

Thanks!
Comment 31 Volker Lendecke 2014-01-08 11:49:46 UTC
I think we need something like

--- a/source3/winbindd/wb_fill_pwent.c
+++ b/source3/winbindd/wb_fill_pwent.c
@@ -91,7 +91,7 @@ static void wb_fill_pwent_sid2uid_done(struct tevent_req *subreq)
 
        state->pw->pw_uid = (uid_t)xid.id;
 
-       subreq = wb_getgrsid_send(state, state->ev, &state->info->group_sid, 1);
+       subreq = wb_getgrsid_send(state, state->ev, &state->info->group_sid, 0);
        if (tevent_req_nomem(subreq, req)) {
                return;
        }

on top of this. Enumerating group members can be expensive or even impossible here.
Comment 32 Karolin Seeger 2014-01-10 08:09:12 UTC
(In reply to comment #31)
> I think we need something like
> 
> --- a/source3/winbindd/wb_fill_pwent.c
> +++ b/source3/winbindd/wb_fill_pwent.c
> @@ -91,7 +91,7 @@ static void wb_fill_pwent_sid2uid_done(struct tevent_req
> *subreq)
> 
>         state->pw->pw_uid = (uid_t)xid.id;
> 
> -       subreq = wb_getgrsid_send(state, state->ev, &state->info->group_sid,
> 1);
> +       subreq = wb_getgrsid_send(state, state->ev, &state->info->group_sid,
> 0);
>         if (tevent_req_nomem(subreq, req)) {
>                 return;
>         }
> 
> on top of this. Enumerating group members can be expensive or even impossible
> here.

Re-assigning to Andreas for commenting.
Comment 33 Andreas Schneider 2014-01-15 14:26:35 UTC
Volker: I've tested the patch and it looks fine. Should I push it to master?
Comment 34 Volker Lendecke 2014-01-16 15:04:31 UTC
(In reply to comment #33)
> Volker: I've tested the patch and it looks fine. Should I push it to master?

Sure. If it works, it certainly improves things significantly.
Comment 35 Andreas Schneider 2014-01-17 10:26:24 UTC
Created attachment 9601 [details]
additional v4-1-test patch
Comment 36 Andreas Schneider 2014-01-17 10:26:57 UTC
Created attachment 9602 [details]
additional v4-0-test patch
Comment 37 Volker Lendecke 2014-01-17 11:03:15 UTC
Karo, please push to 4.0 and 4.1.

Thanks,

Volker
Comment 38 Stefan Metzmacher 2014-01-20 20:39:30 UTC
Comment on attachment 9602 [details]
additional v4-0-test patch

Please upload a cherry-pick from master
commit 1a43778433934530d77791edd1af538de8b1d8a3, d145b21f3eb42de9b3bd2b813d4a07d46d9eb775 is not a public commit!
Comment 39 Stefan Metzmacher 2014-01-20 20:39:57 UTC
Comment on attachment 9601 [details]
additional v4-1-test patch

Please upload a cherry-pick from master
commit 1a43778433934530d77791edd1af538de8b1d8a3, d145b21f3eb42de9b3bd2b813d4a07d46d9eb775 is not a public commit!
Comment 40 Andreas Schneider 2014-02-05 09:00:14 UTC
Created attachment 9639 [details]
additional v4-1-test patch
Comment 41 Andreas Schneider 2014-02-05 09:23:31 UTC
Created attachment 9641 [details]
additional v4-0-test patch
Comment 42 Karolin Seeger 2014-02-06 10:46:43 UTC
Pushed additional patches to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 43 Karolin Seeger 2014-02-14 19:00:33 UTC
(In reply to comment #42)
> Pushed additional patches to autobuild-v4-1-test and autobuild-v4-0-test.

Pushed to v4-1-test and v4-0-test.
Closing out bug report.

Thanks!