New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syncthing doesn't honor umask #1339

Closed
dr4Ke opened this Issue Feb 9, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@dr4Ke

dr4Ke commented Feb 9, 2015

I'm using syncthing 0.10.21 on archlinux and debian (3 machines). I'm syncing some folders, shared between local users. For those folders, I want to:

  • ignore permissions as the files have different owners and only the owner can change permissions (so syncthing is not able to do it and generates lots of errors if permissions are not ignored)
  • have syncthing create files with rw-rw-r-- permissions.

So I tried to set the syncthing umask to 0002 as I did for the local users, but it continues to create the synced files with rw-r--r--. I did that by overloading the syncthing systemd unit with:

[Service]
UMask=0002

I checked that this umask is actually applied to the syncthing process with gdb and call umask(0) as suggested by http://stackoverflow.com/a/165718/335197

I didn't look at the code to investigate more yet. Do you have any idea?

@calmh

This comment has been minimized.

Member

calmh commented Feb 9, 2015

Correct as described, we don't currently honor the umask.

@calmh calmh added the enhancement label Feb 9, 2015

@calmh calmh added the help-wanted label Apr 5, 2015

@mneiger

This comment has been minimized.

mneiger commented Apr 5, 2015

I'd gladly participate in testing this using the the following platforms
Linux Mint
Raspbian
with non permission enabled platforms:
Windows 7
Android

@mogwa1

This comment has been minimized.

Contributor

mogwa1 commented May 18, 2015

I have a very similar use case where I'm syncing windows and linux machines, and (1) permissions should be ignored, but (2) the umask should be respected on the linux systems (files should be group-writeable, i.e. umask 0002).

Anyway, I've had a look at the syncthing code, and I've identified the main parts of the code that are involved. As a matter of fact, syncthing currently does respect umask settings. Any golang system call to create new files or directories will respect the umask on unix-like systems. The actual problem is that new files are created with 0644 permissions and directories with 0755. If you set 'ignore permissions' on a share, these default permissions are not altered anymore after the initial file creation. Since the umask by definition only takes away permissions, the group-write bit will therefore never be set.

I've managed to compile a syncthing version that produces the correct behavior (at least for my current setup) by setting default permissions to 0666 and 0777 for new files and directories, respectively. The unix umask then automatically takes care of taking away the inappropriate permission bits (i.e. making the files and directories not world-writeable etc). If you want, I can share this commit? (Still need to do some cleaning up.) So far I've only tested this on windows and linux (arch) on shares where the permissions are ignored.

@dr4Ke

This comment has been minimized.

dr4Ke commented May 18, 2015

Cool. Yes, sharing your commit would be nice. Thank you for your work on this.

@calmh

This comment has been minimized.

Member

calmh commented May 19, 2015

This sounds like it would be a reasonable behavior when ignorePetmissions is set. Please do contribute a pull request to close this issue.

@mogwa1

This comment has been minimized.

Contributor

mogwa1 commented May 22, 2015

I'll do a pull request as soon as I have some more time to test whether these changes impact the behaviour when ignorePermissions is not set (false).

@calmh

This comment has been minimized.

Member

calmh commented May 22, 2015

The current integration tests cover that fairly well, so we'll make sure of that as well :)

@mogwa1

This comment has been minimized.

Contributor

mogwa1 commented May 22, 2015

Ok, that's good to know. I'll try to finish it this weekend.

@mogwa1

This comment has been minimized.

Contributor

mogwa1 commented May 24, 2015

It turned out to be more tricky than I originally thought. When I tested my original fix with servers having different combinations of ignorePermisions, I ran into problems. I've got it fixed now; or at least for the combinations of OSes and ignorePermissions settings that I tried.

I'm by no means an expert on the syncthing code (or Go as a matter of fact), but I think the problem is a bug in the current ignorePermissions implementation.

  • According to https://forum.syncthing.net/t/v0-8-10-ignore-permissions/263/3, the ignorePermissions flag should only be picked up on the server where the new file is created. If ignorePermissions is set on that server, the FlagNoPermBits is added to the flags.
  • On the receiving side, the file flags should be checked for FlagNoPermBits. If this bit is set, the default permissions for that OS should be used.

However, what is happening in the code (at least to my limited knowledge) is:

  • The FlagNoPermBits is set correctly on the server where the new file is created.
  • On the receiving end, the ignorePermissions setting (belonging to the share) is used instead of checking whether the FlagNoPermBits are set on the files which are being received.
  • This will give the same results as the intended behaviour except when you have a mixture of ignorePermissions settings on the servers. In that case, you will get very unpredictable permission settings.

I will do 2 pull requests, if that is ok:

  • one to fix this (probable?) ignorePermisions/FlagNoPermBits bug; (I will also open a bug report)
  • another one directly related to this issue, where the permissions of newly created files are changed such that the umask is respected on systems that support it.

Note, however, that the two pull requests are needed to completely close/fix this issue.

@calmh calmh closed this in badfc77 May 25, 2015

calmh added a commit that referenced this issue May 25, 2015

Merge pull request #1868 from mogwa1/umask
Change permissions of newly created files and directories (fixes #1339)

@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.