Hi! I have this config: [myshare] path = /mnt/myshare create mode = 0644 directory mode = 0775 force create mode = 0644 force directory mode = 0775 force user = http valid users = user1 user2 write list = user1 which is not working. Samba server is launching, but on the client side I get "Permission denied error", when trying to authenticate either as user1 or as user2. If I comment out 'force user' line, I authenticate just fine, but then, obviously, files and directories created are owned by user1 or user2, but not http. If I add http user to 'valid users' and to 'write list', and do not comment 'force user = http' line, I also am able to log in as user1 and user2, however, not only user1 can create and change files, but also a user2, which now happens to be treated as http user. In previous versions of samba (in 3.<something> at least) behaviour was a) forced user (http) was not required to be present in 'valid users' list b) even when overrided to forced UNIX user, permissions to write to a share we still somehow controlled by the permissions of original Samba user (permission was bitwise-and of those permissions) N.B. http user does not exist in by tdbsam database of Sambe users - it is purely UNIX user, while user1 and user2 both and UNIX and Samba users. I wonder, if this change in behaviour was intentional and if so, how can I get the previous behaviour?
Figured out, how to do what I want - do not need to add http user to write list. If I do this way, user1 gets read/write permissions and files are created as http user and user2 gets read-only permissions. Still, necessity to add http user to valid users is counterintuitive - was it intentional?
Confirmed. And also broken for 4.1. This is definitely a regression and needs to be fixed. In a member setup, I even needed to specify "Unix User\user" in the valid users setting for unix user "user" in "force user = user".
So the code path looks like this. Inside source3/smbd/service.c we have: conn->read_only = lp_readonly(SNUM(conn)); status = set_conn_force_user_group(conn, snum); if (!NT_STATUS_IS_OK(status)) { goto err_root_exit; } conn->vuid = vuser->vuid; ... /* * Set up the share security descriptor */ status = check_user_share_access(conn, conn->session_info, &conn->share_access, &conn->read_only); The "set_conn_force_user_group()" sets the token to be that of the forced user. The check_user_share_access() then compares the token against the 'valid users' (or 'invalid users') list. If http (the forced user) isn't in the valid users list, then status is returned as ACCESS_DENIED and we exit. This *does* seem like a reasonable check to make, even if it was different in 3.6.x (so is strictly a regression). We might want to address this by updating the 'force user' documentation to explain the user must be valid for this share connection. I'll look into how 3.6.x did it before making a final evaluation. Jeremy.
Hmmm. The interaction between "force user", "valid users", "read list" and "write list" is horribly undefined.. If force user is set does this mean : a). No access unless the forced user is in the valid users list ? (currently true in master). b). Read-only access unless the forced user is in the write list ? (currently false in master). c). Read-write access unless the forced user is in the read list ? (not tested by me yet). We need to define once and for all what the interactions between these parameters *should* be, then add tests to ensure it doesn't break. Jeremy.
Hi Jeremy, yeah this is definitely a regression. My understanding of forced user vs. valid users (and friends) is this: - "valid users" (and read list and write list) are checks that are done at the smb level, i.e. against the windows token of the connecting user. This is so to speak a poor man's share ACL. - "force user" and "force group" are a means to change the unix user / group accessing the file system on behalf of the connected Windows user. I.e. this at the posix level, between smbd and the file system. This is impersonation is done after the SMB level access checks and is completely unrelated. It has been a very long practice to use "force user" with a user that is only a unix user unknown to the smb level of samba (pdb). (In reply to comment #4) > Hmmm. The interaction between "force user", "valid users", "read list" and > "write list" is horribly undefined.. > > If force user is set does this mean : > > a). No access unless the forced user is in the valid users list ? > (currently true in master). > > b). Read-only access unless the forced user is in the write list ? > (currently false in master). > > c). Read-write access unless the forced user is in the read list ? > (not tested by me yet). > > We need to define once and for all what the interactions between these > parameters *should* be, then add tests to ensure it doesn't break. So according to my (and many setups I have seen and done myself), none of the above should be true. access/read/write checking based on the "valid users"/"write list" is done at the smb level only, and the completely unrelated forced user will perform any file system operation that the SMB layer of Samba commands it to do, based on these SMB checks (given that the forced user is allowed to perform them). So the wrong thing is master is that a windows token mapped from the forced unix user is checked against the lists at all. Here is what the smb.conf manpage says regarding the topic: >>> This user name only gets used once a connection is established. >>> Thus clients still need to connect as a valid user and supply >>> a valid password. Once connected, all file operations will be >>> performed as the "forced user", no matter what username the >>> client connected as. Cheers - Michael
(In reply to comment #5) > Hi Jeremy, > > yeah this is definitely a regression. > > My understanding of forced user vs. valid users (and friends) is this: > > - "valid users" (and read list and write list) are checks that are done > at the smb level, i.e. against the windows token of the connecting > user. This is so to speak a poor man's share ACL. > > - "force user" and "force group" are a means to change the unix > user / group accessing the file system on behalf of the connected > Windows user. I.e. this at the posix level, between smbd and > the file system. This is impersonation is done after the SMB level > access checks and is completely unrelated. > > It has been a very long practice to use "force user" with a user > that is only a unix user unknown to the smb level of samba (pdb). > > So according to my (and many setups I have seen and done myself), > none of the above should be true. > > access/read/write checking based on the "valid users"/"write list" > is done at the smb level only, and the completely unrelated > forced user will perform any file system operation that the SMB layer > of Samba commands it to do, based on these SMB checks (given that the > forced user is allowed to perform them). > > So the wrong thing is master is that a windows token mapped from > the forced unix user is checked against the lists at all. "So it's once more into the breach then, dear friends" :-). This keeps getting broken because we don't have a test that stops us regressing here, and also because this expected behavior has never been written down and has never been codified as "expected". Trust me, I'd know if it had - and I can never remember the interactions between these parameters. I can work on the code to change it back to have the "correct" semantics, but I really need some help getting the tests written to make sure we don't regress on this.
(In reply to comment #6) > (In reply to comment #5) > > So the wrong thing is master is that a windows token mapped from > > the forced unix user is checked against the lists at all. > > "So it's once more into the breach then, dear friends" :-). > > This keeps getting broken because we don't have a test > that stops us regressing here, and also because this > expected behavior has never been written down and has > never been codified as "expected". I always understood the above quote from the manpage to describe exactly this, but at second reading, I consent that it can be made much more clear. > I can work on the code to change it back to have the > "correct" semantics, but I really need some help getting > the tests written to make sure we don't regress on this. Ok, great! I'll also happily work/collaborate on this. Cheers - Michael
Btw, if I got this right, the regression was introduced with 86d1e1db8e2747e30c89627cda123fde1e84f579: https://git.samba.org/?p=samba.git;a=commitdiff;h=86d1e1db8e2747e30c89627cda123fde1e84f579
Oh, good catch. That fix was definitely required, as it fixed an issue with calculating the share permissions correctly for the user. Looks like the call to check_user_share_access() is doing more than is required, as the call inside check_user_share_access() to: if (!user_ok_token(session_info->unix_info->unix_name, session_info->info->domain_name, session_info->security_token, snum)) { return NT_STATUS_ACCESS_DENIED; } is causing the access denied if the forced user isn't in the write list. The previous code just calculated the share access mask directly.
Hmmm. Although thinking about it, the conn->session_info->security_token at this point is already the forced user token. Should we be calculating the share access mask based off the forced user, or the original valid user ? Again, this is a little trickier than it looks at first glance :-). Michael, what are your thoughts on who we should use to calculate the share access mask and readonly parameter on the connection ?
(In reply to comment #10) > Hmmm. Although thinking about it, the conn->session_info->security_token at > this point is already the forced user token. > > Should we be calculating the share access mask based off the forced user, or > the original valid user ? > > Again, this is a little trickier than it looks at first glance :-). Michael, > what are your thoughts on who we should use to calculate the share access mask > and readonly parameter on the connection ? Hmmm, can't we keep the original connecting user's token for access checks instead of overwriting it with the forced user's token which can be a unix token mapped up with the "Unix User\user" style fake sid-level token? The share access check should be done with the original windows-level token and not the forced token, so we somehow need to keep both, right? So do we need to keep two tokens? Or do we just overwrite the one token later? ...
And here's one more thing. Right now the logic flow in make_connection_snum() calculates the session_info of the 'forced user' and then uses it in the : char *s = talloc_sub_advanced(talloc_tos(), lp_servicename(talloc_tos(), SNUM(conn)), conn->session_info->unix_info->unix_name, conn->connectpath, conn->session_info->unix_token->gid, conn->session_info->unix_info->sanitized_username, conn->session_info->info->domain_name, lp_pathname(talloc_tos(), snum)); call which can change the details of the connect path etc. (due to the %u and other substitutions). Should these substitutions be done as the original, or the forced user ? For example, think of a share with path = /home/%u Should this be: /home/'forced user' or /home/'logged in user' ? We need to decide upon these details. If I make the simple change to move the forced user calculation after the share access mask changes, this will have the side effect of changing the substitutions done.
With Samba 4.1.2 this is /home/'forced user'. With 3.6.22 this is /home/'forced user'. And I think, this is the right thing: Once authenticated, all operations should be done with the forced user - and thats why also the substitutions.
I ran into this same issue on 4.1.3 related to sharing the root filesystem where force user and force group are set to root. (standalone server) See the thread "What in samba 4.1 prevents a '/' share?" In summary, using the share definition of: [config] comment = Phoinix Config (Archlinux) path = / valid users = david force user = root force group = root browseable = no writeable = Yes Caused mount failure returning the following error: [14:45 providence:~/cnf] # mount.cifs //nemesis/config /mnt/nm-cfg/ -o uid=1000,credentials=/root/cnf/mountcfile,noperm mount error(13): Permission denied Refer to the mount.cifs(8) manual page (e.g. man mount.cifs) After searching through a level 10 debug with 'log file = /var/log/samba/level10-%S.log' I ran across the following user_ok_token issues: [2014/01/07 20:32:58.157111, 5, pid=5405, effective(0, 0), real(0, 0)] ../source3/lib/username.c:181(Get_Pwnam_alloc) Finding user david <snip> [2014/01/07 20:32:58.158932, 10, pid=5405, effective(0, 0), real(0, 0)] ../source3/smbd/share_access.c:237(user_ok_token) user_ok_token: share config is ok for unix user david [2014/01/07 20:32:58.159036, 5, pid=5405, effective(0, 0), real(0, 0)] ../source3/lib/username.c:181(Get_Pwnam_alloc) Finding user root <big snip> [2014/01/07 20:32:58.176304, 10, pid=5405, effective(0, 0), real(0, 0)] ../source3/smbd/share_access.c:215(user_ok_token) User root not in 'valid users' <snip> [2014/01/07 20:32:58.176620, 3, pid=5405, effective(0, 0), real(0, 0)] ../source3/smbd/error.c:82(error_packet_set) NT error packet at ../source3/smbd/reply.c(952) cmd=117 (SMBtconX) NT_STATUS_ACCESS_DENIED Removing the 'force user' and 'force group' options from the [config] share allowed the share to mount without error. I have used the 'force user' and 'force group' options in the [config] share since at least 2.0.7 (maybe 1.8.x), and 4.x is the first time I ran into a mount failure. If it will help, I have the level 10 for the failure.
Ok, I might have a quick and easy fix for this... Testing now. Jeremy.
Created attachment 9781 [details] git-am fix for 4.1.next
Created attachment 9782 [details] git-am fix for 4.0.next
Comment on attachment 9781 [details] git-am fix for 4.1.next Either I am very confused, or one of these statements is not true: conn->session_info represents the token to use when actually accessing the file system, and so is modified by force user and force group. conn->session_info represents the "pristine" token of the user logging in, and is never modified by force user and force group. I think you mean vuser in the second instance. Either way, I think we should move check_user_share_access() up to just below create_connection_session_info() to more closely match check_user_ok() in uid.c (or better still, find a way to merge more aspects of both into a single fucntion). Finally, this needs a test-suite. We chickened out of this last time we worked here, and this is our reward, so we must do it this time, including not just valid users but also setting a per-share security descriptor.
(In reply to comment #18) > Comment on attachment 9781 [details] > git-am fix for 4.1.next > > Either I am very confused, or one of these statements is not true: > > conn->session_info represents the token to use > when actually accessing the file system, and so > is modified by force user and force group. > > conn->session_info represents the "pristine" > token of the user logging in, and is never modified > by force user and force group. > > I think you mean vuser in the second instance. Indeed I do, thanks for noticing that. I'll upload a changed version with the correct comment :-). > Either way, I think we should move check_user_share_access() up to just below > create_connection_session_info() to more closely match check_user_ok() in uid.c > (or better still, find a way to merge more aspects of both into a single > fucntion). Nope. Don't want to make arbitrary changes right now. It's best to do the "minimum necessary fix" for the bug. Let's not experiment with making the code elegant until it's fixed. > Finally, this needs a test-suite. We chickened out of this last time we worked > here, and this is our reward, so we must do it this time, including not just > valid users but also setting a per-share security descriptor. Now this I agree with wholeheartedly :-). Luckily for me Michael has volunteered above to help with this :-). Jeremy.
Created attachment 9783 [details] git-am fix for 4.1.next Fixed commit message as noticed by Andrew.
Created attachment 9784 [details] git-am fix for 4.0.next Fixed commit message as noticed by Andrew.
(In reply to comment #19) > Now this I agree with wholeheartedly :-). Luckily for me Michael has > volunteered above to help with this :-). But I won't object if you want to add tests too :-). Cheers, Jeremy.
We don't have any 'force user' tests at all. We also need a test for 'force user' with 'security = ads'. So I would say review+, but it would be great to have tests in master so we avoid regressions in future.
Comment on attachment 9783 [details] git-am fix for 4.1.next Adding Andreas so he can formally +1
Comment on attachment 9784 [details] git-am fix for 4.0.next Adding Andreas so he can formally +1
Here is the feedback from the list: Date: Tue, 18 Mar 2014 07:32:35 +0100 (CET) From: Gerhard Wiesinger <lists@wiesinger.com> To: Jeremy Allison <jra@samba.org> cc: samba-technical@samba.org, obnox@samba.org Subject: Re: [PATCH] Fix bug #9878 - force user does not work as expected. User-Agent: Alpine 2.02 (LFD 1266 2009-07-14) Reviewed-by: Gerhard Wiesinger <lists@wiesinger.com> Tested-by: Gerhard Wiesinger <lists@wiesinger.com> Ack-by: Gerhard Wiesinger <lists@wiesinger.com> Thank you for the fast fix. Ciao, Gerhard
Karolin, please add the patches to the relevant branches. Thanks!
The fix isn't working correctly. See samba-technical.
Issue turns out not to be with these fixes, but to do with user login not getting the correct group list (missing an added group from /etc/group). Andreas will re-review (and hopefully +1 :-).
(In reply to comment #30) > Karolin, please add the patches to the relevant branches. Thanks! Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Pushed to v4-0-test and v4-1-test. Closing out bug report. Thanks!