'bzr push' does not preserve sgid bit on newly created directories over sftp

Bug #50568 reported by John A Meinel
44
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Bazaar
Confirmed
Medium
Unassigned
Breezy
Triaged
Low
Unassigned

Bug Description

It seems that the chmod through sftp strips sgid and suid bits. So even if a directory is created with sgid, trying to 'chmod 2775' sets the mode to 775 on the remote side.

Steps to reproduce:

$ bzr init a
$ echo a > a/a
$ bzr add a/a
$ bzr commit -m a a/a
$ bzr branch a b
$ chown -R .users b/.bzr
$ find b/.bzr -type d -print0 | xargs -0 chmod 2770
$ find b/.bzr -type f -print0 | xargs -0 chmod 660

# Branch 'b' should now be set up properly as a shared repository.

$ echo b > a/b
$ bzr add a/b
$ bzr commit -m b a/b
$ cd a
$ bzr push sftp://localhost/path/to/b
$ cd ../b/.bzr/repository/knits/
$ ll

If you look closely, you will see that the directory that holds 'a-*.knit' will have the sticky group bit set, but the directory that holds 'b-*.knit' does not. It has permissions 770, but not 2770.

Simon Ekstrand has this comment:

Unless this has been fixed recently, it's broken, atleast for
open-ssh(d). Openssh's serverside sftp implementation masks out any suid/sgid bits on chmod operations for regular users. The sftp transport mkdir method does a mode-less mkdir then a chmod, which doesn't work. Including the directory mode with the mkdir call would work fine (tested), but there's a comment in the mkdir method about this breaking with the sftp server used for tests.

So it seems simply updating SftpTransport to use mode with mkdir, and then fix the stub server.
We still have to worry a little bit about if the remote umask prevents us from creating the bits we want.

Tags: sftp
Revision history for this message
Robey Pointer (robey) wrote :

shouldn't os.mkdir() honor the umask anyway? it seems like we should just be able to use sftp.mkdir(path, mode) and stub_sftp will "just work".

Revision history for this message
John A Meinel (jameinel) wrote :

The problem is that frequently we need to step outside the umask. Or at least I was hoping to be able to do that.

My use case is this: the default umask on a machine is likely to be 0022. Which is nice that the files in my home directory don't default to being world writeable.
But I'm also committing to a shared branch in say /home/shared/bzr/foo
We set the sgid bit on 'foo' and all the subdirs, (g+Srw). All files are g+rw.
The problem is that when we create a new directory that is a subdir (say a new entry like .bzr/repository/knits/aa) that directory should also be g+Srw (02770 for example).
However, because the default umask is 0022, sftp will create that as 02750, and if we sftp.chmod(02770), the sftp server will strip the gid bits, and create it as 00770. (g+rw).

We're pretty screwed either way. We can't set the sticky bit over sftp, and we can't set the write bit without resetting the sticky bit.

Revision history for this message
Robey Pointer (robey) wrote :

yeah, i get it now. you're right, we're screwed. :)

i think the comments you made on the mailing list about changing sftp-server's umask will probably end up being a FAQ for people trying to use the sgid configuration.

Revision history for this message
John A Meinel (jameinel) wrote :

Not much we can do, but I don't want to close it yet. We should think about alternatives and make a Wiki entry that this bug can reference for workarounds.

Changed in bzr:
importance: Untriaged → Medium
status: Unconfirmed → Confirmed
Revision history for this message
Claudius (schnoerr) wrote :

I advocate classifying this bug as being more severe than just medium. It prevents us from collaborating correctly by bzr and ssh.

Revision history for this message
Martin Pool (mbp) wrote :

Claudius, you may be able to work around this by using bzr+ssh rather than sftp, if you can install bzr on the server.

Revision history for this message
Martin Pool (mbp) wrote :

John/Robey: Presumably the problem with losing the sgid bits is that new directories will have the wrong ownership. We could try chgrping newly created directories and files by hand. Unfortunately that would add a new roundtrip for each one we create...

Revision history for this message
Robey Pointer (robey) wrote :

One feature of Linux's sgid-folders is that creating a nested folder automatically makes that folder setgid too. Since we can't set the sgid bit after doing a chmod, we lose that marker. For example, creating a deeply nested tree, we wouldn't have any sgid folders except the very top one, and we'd have to walk up toward the root, looking for a sgid folder, to see if we should change the group. Anyone not using bazaar to access the subtree would probably mess it up.

Interestingly, while trying this out, I found out that OSX and, apparently, all BSDs don't have a meaning for sgid on folders because they *always* make a folder's group equal to the parent folder's group. If bazaar followed that logic, we could ignore the sgid problem.

Revision history for this message
Joerg Bornschein (j.bornschein) wrote :

I solved this whole problem using (Posix1.e) filesystem ACLs:

1) Activate ACLs on your filesystem: edit /etc/fstab and add "acl" where appropriate. Run "mount / -o remount,acl" for immediate effect.

2) Allow developer group to rwx the base directory an make sure all newly created files inherit that ACL:

  $ setfacl -m group:developer:rwx /bzr
  $ setfacl -m default:group:developer:rwx /bzr

3) Proceed as descibed in the shared repository tutorial: run "bzr init-repo /bzr" and upload projects to that directory via sftp.

All newly created files will inherit hat ACL. That even solves umask problems on linux, because the umask is not used t mask out ACL permissions.

Seems to work fine for me...

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 50568] Re: 'bzr push' does not preserve sgid bit on newly created directories

On Mon, 2007-06-04 at 22:14 +0000, j.bornschein wrote:
> I solved this whole problem using (Posix1.e) filesystem ACLs:
>
> 1) Activate ACLs on your filesystem: edit /etc/fstab and add "acl" where
> appropriate. Run "mount / -o remount,acl" for immediate effect.
>
> 2) Allow developer group to rwx the base directory an make sure all
> newly created files inherit that ACL:
>
> $ setfacl -m group:developer:rwx /bzr
> $ setfacl -m default:group:developer:rwx /bzr
>
> 3) Proceed as descibed in the shared repository tutorial: run "bzr
> init-repo /bzr" and upload projects to that directory via sftp.
>
> All newly created files will inherit hat ACL. That even solves umask
> problems on linux, because the umask is not used t mask out ACL
> permissions.
>
> Seems to work fine for me...

Cool. Perhaps we should put this in the documentation?

-Rob

--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

Revision history for this message
Claudius (schnoerr) wrote : Re: 'bzr push' does not preserve sgid bit on newly created directories

Setting ACLs may not be possible on virtual servers of some providers.
I tried it on STRATO's V-Powerservers, where it is not possible.
This was acknowledged by a service administrator of STRATO.

Revision history for this message
ChriS (christophe-troestler) wrote :

I was hit by this problem too but in a different flavor. My problem was not that the sgid bit was not propagated but that we were pushing into a directory with the sgid bit set on the server and that this sgid bit was erased with bzr executing chmod to set the group bits. Since I am using rich-root-pack — which implies as a side effect that no directory will be created after the initial push —, a workaround is to set again the sgid bit by hand...

Revision history for this message
ChriS (christophe-troestler) wrote :

For the problem I reported above, a patch is in the current bzr.dev and should be part of bzr 1.6.

Revision history for this message
ChriS (christophe-troestler) wrote :

Ref. http://bundlebuggy.aaronbentley.com/project/bzr/request/<20080709.115028.572522674361770034.Christophe.Troestler%40umh.ac.be>

Revision history for this message
Wouter van Heyst (larstiq) wrote :

This seems to be the case still for bzr+ssh as well.

Revision history for this message
John A Meinel (jameinel) wrote :

Can you elaborate with a test script?

In my testing with a Pack repository, I see:

drwxrwsr-x 4 jameinel bzrusers 168 Oct 22 14:10 .bzr/
drwxrwsr-x 3 jameinel jameinel 96 Oct 22 14:10 test/
drwxr-sr-x 3 jameinel jameinel 72 Oct 22 14:14 test-bzr+ssh/
drwxrwsr-x 3 jameinel jameinel 72 Oct 22 14:13 test-sftp/

And then for new files:
.bzr/repository/indices:
total 48
-rw-rw-r-- 1 jameinel bzrusers 192 Oct 22 14:13 8f5d35c66e891b556673b032c52d5fd4.iix
-rw-rw-r-- 1 jameinel bzrusers 187 Oct 22 14:13 8f5d35c66e891b556673b032c52d5fd4.rix
-rw-rw-r-- 1 jameinel bzrusers 60 Oct 22 14:13 8f5d35c66e891b556673b032c52d5fd4.six
-rw-rw-r-- 1 jameinel bzrusers 60 Oct 22 14:13 8f5d35c66e891b556673b032c52d5fd4.tix
-rw-rw-r-- 1 jameinel bzrusers 192 Oct 22 14:14 916267eb715bcaadf4b026cbe061ac8b.iix
-rw-rw-r-- 1 jameinel bzrusers 187 Oct 22 14:14 916267eb715bcaadf4b026cbe061ac8b.rix
-rw-rw-r-- 1 jameinel bzrusers 60 Oct 22 14:14 916267eb715bcaadf4b026cbe061ac8b.six
-rw-rw-r-- 1 jameinel bzrusers 60 Oct 22 14:14 916267eb715bcaadf4b026cbe061ac8b.tix
-rw-rw-r-- 1 jameinel bzrusers 127 Oct 22 14:10 c4ca1ce789afb4c8701fba569200647c.iix
-rw-rw-r-- 1 jameinel bzrusers 126 Oct 22 14:10 c4ca1ce789afb4c8701fba569200647c.rix
-rw-rw-r-- 1 jameinel bzrusers 60 Oct 22 14:10 c4ca1ce789afb4c8701fba569200647c.six
-rw-rw-r-- 1 jameinel bzrusers 164 Oct 22 14:10 c4ca1ce789afb4c8701fba569200647c.tix

.bzr/repository/packs:
total 12
-rw-rw-r-- 1 jameinel bzrusers 562 Oct 22 14:13 8f5d35c66e891b556673b032c52d5fd4.pack
-rw-rw-r-- 1 jameinel bzrusers 561 Oct 22 14:14 916267eb715bcaadf4b026cbe061ac8b.pack
-rw-rw-r-- 1 jameinel bzrusers 735 Oct 22 14:10 c4ca1ce789afb4c8701fba569200647c.pack

So new branches were created with the default group (but I forgot to set the repo to g+s, and only set .bzr/ that way).

bzr+ssh used my default umask to create the branch, which caused it to be g-w, while sftp created it with g+w. Though I have an sftp-wrapper script installed which does:
#!/bin/sh
# Just run sftp-server with the correct umask
umask 0002
exec /usr/libexec/openssh/sftp-server

Anyway, both sftp and bzr+ssh used the right permissions for creating new pack and index files, and the group bits were preserved.

I guess Pack repos don't continually create new directories like Knit repos do, but I'd like a bit more detail as to what is actually failing via bzr+ssh.

Revision history for this message
Wouter van Heyst (larstiq) wrote :

Sure, the goal in question is to get group writable branches, but that doesn't happen by virtue of the repository being mode 2770.

The format was default, pack-0.92

Jelmer Vernooij (jelmer)
tags: added: sftp
Revision history for this message
Deni Bertovic (denibertovic) wrote :

Has there been any breakthrough with this bug? bzr+ssh still does not respect the default umask of the system when doing a initial push onto a remote repo.

Jelmer Vernooij (jelmer)
summary: - 'bzr push' does not preserve sgid bit on newly created directories
+ 'bzr push' does not preserve sgid bit on newly created directories over
+ sftp
Revision history for this message
Deni Bertovic (denibertovic) wrote :

I've found out some new Information and possible work arounds.

bzr over sftp and bzr+ssh have the same fault as far as I have tested. Meaning that they both disregard the default umask set up in /etc/profile during the *INITIAL* push to a remote repo. Just so there is no confusion and that we are all clear on this, this only happens during the initial push, meaning the folder is not on the the remote host yet. Bzr creates it....with the wrong permissions :). After bzr created the remote folder and I ssh to the remote machine, do a manual chmod g+w everything works fine after that and all leading pushes (from OTHER users) to that repo work just fine.

Now, the reason bzr ssh and sftp ignore the /etc/profile default umask setting is because AIUI no bash shell is executed and therefore the /etc/profile file is not even read.

There is a way to correct this for sftp. Edit sshd_config change the part with sftp to: 'Subsystem sftp /usr/lib/openssh/sftp-server -u 007' (or the desired umask..in my case it's 007 so the dev group have write permissions to the repos).

This does NOT work for bzr+ssh.....or scp for that matter. To make ssh affected you have to change the pam settings in /etc/pam.d/sshd and add the line "session optional pam_umask.so umask=007". So this affects sftp, scp and ssh. Note that for ssh to use pam it needs to have the UsePam directive set to yes in /etc/ssh/ssd_config file.

Now everything works fine. Or does it?!?! We worked around the issue of bzr messging up the permissions on an initial push but now we are stuck with the umask 007 set for all users on the system. Meaning if you create a file in your home directory it will be readable and writeable by the group your username belongs to. Not the best solution in the world.

Also, it helps to know that the user specific settings override all this. Meaning if a user sets the umask to 022 in his ~/.profile or .bashrc it will override the global settings. Again, these script will be called only if a bash shell is executed.

And that's it. Hope this information helps someone. Atleast until this issue is fixed.

Jelmer Vernooij (jelmer)
tags: added: check-for-breezy
Jelmer Vernooij (jelmer)
Changed in brz:
status: New → Triaged
importance: Undecided → Medium
tags: removed: check-for-breezy
Changed in brz:
importance: Medium → Low
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.