Magic Folder: permissions of downloaded files should be set in a more failure-safe way

[dawuud]

During the demo with Zooko, Daira, Warner and Meejah last night we found yet another bug: the magic-folder downloader writes files with the original uploader's permissions intact as if the umask were 0077, but it should probably use the downloader's user's umask instead. If we do it like that then the user downloading files will have consistent permissions for all downloaded files.

[daira]

I don't think it uses the original uploader's permissions. It uses fileutil.write (​here), which uses whatever the default is for open(path, "wb") (​here). But it does this as the node process, which might not have the same umask as the user's shell (ISTR that twistd changes the umask).

[daira]

twistd sets umask to 0077 by default, unless overridden by ​--umask.

The correct approach here seems to be to save the original umask for later use by _write_downloaded_file, before passing control to twistd in tahoe start/run. (We don't want to change the umask of the node from 0077 using --umask because that would have difficult-to-review security consequences.)

[daira]

This bug is Unix-specific.

[daira]

Alternatively, or as well, we can have a [magic_folder]download.umask entry in tahoe.cfg. (For good reason, daemons don't normally just use the default umask.)

[daira]

Note that the umask (or [magic_folder]download.umask entry) should only be used when writing a new file or directory. When replacing a file, docs/proposed/magic-folder/remote-to-local-sync.rst describes what should happen:

The implementation of file replacement differs between Unix and Windows. On Unix, it can be implemented as follows:

• 4a. Set the permissions of the replacement file to be the same as the replaced file, bitwise-or'd with octal 600 (rw-------).

[daira]

This does not block merging Magic Folder to trunk, but should preferably be fixed before a subsequent release.

[dawuud]

fixed here: ​https://github.com/david415/tahoe-lafs/tree/2535.fix-file-perms.0

submitted pull request: ​https://github.com/tahoe-lafs/tahoe-lafs/pull/212

[daira]

Merged, but this also needs documentation in docs/configuration.rst​ and ​docs/frontends/magic-folder.rst.

[dawuud]

i added a note about the umask to magic-folder.rst

configuration.rst doesn't have a magic-folder section.

[dawuud]

what's next? review?

[daira]

Pushed to 2438.magic-folder-stable.5 .

[daira]

The current approach is undesirable because the os.umask in the finally block could fail, leaving an unsafe umask for the whole node process.

Using os.open with a mode argument wouldn't work because the umask specified by twistd (0077) would still be masked off, both when creating the file and when creating any ancestor directories that do not already exist. It appears that the only safe way for a Unix process to atomically set an arbitrary umask when creating a file, without setting its own process-wide umask, is to create a subprocess that will have the desired umask.

Here, we don't need atomicity for setting the umask of the replacement file, so we could use chmod to relax the permissions from the ones that result from the twistd umask. However we would also need to chmod any created directories. Reopening as a reminder to change it to use this method.

[dawuud]

ok i got rid of the call to umask and instead use chmod as daira suggested; in my dev branch here: ​https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.0

please review

[meejah]

Looks good to me. Two questions and a comment:

• where does self._local_path_u come from (i.e. should that be local_path_u instead?)
• i would maybe call the local_path_u arg to _write_downloaded_file workdir_u or similar to match the arg name at the callsite (and also to avoid confusion to self._local_path_u if that isn't a typo)
• while loops make me nervous ;) but that one looks fine

[dawuud]

The callsite passes self._local_path_u which is the Magic-Folder directory... and _write_downloaded_file needs it to ensure that we can chmod all created directories without chmod'ing outside the Magic-Folder.

I guess I should change the argument name to magicfolder_dir_u...

[dawuud]

the current plan is to break out the chmod while loop into a separate function in fileutils.py and write unit tests for it.

[dawuud]

corrections made and unit test added: ​https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.0

please review

[daira]

Review comments need addressing.

[dawuud]

i've fixed the things daira said to fix in her review comments. i also noticed this branch doesn't merge cleanly onto stable.12 so i'll have to make another branch using stable.12 and then perhaps cherry-pick several times.

[dawuud]

new dev branch here... cherry-picked and awaiting review ​https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.1