Set tight permissions for key files

[Lekensteyn]

Ensure that key files are not world-readable.

temp_umask is based on test.support.temp_umask.

Fixes #29

---

If older Python versions do not have to be supported, then the test code could become something like:

@temp_umask(0o022)
def test_it():
    ...

[Lekensteyn]

Ok, apparently a ChainFile behaves likes its name, it does not store the key even though the FullChainFile implementation suggests otherwise. So this part is not needed.

This becomes different with a change such as #33, should it always set the permission to 0600 for security reasons? If the user requires weaker permissions, the file can be touched before creating (or chmod can be invoked afterwards).

[danmilon]

World readable private key? ouch!
LGTM.

[Lekensteyn]

Updated patchset against latest master, added a rough separate test case as AccountKey got removed.

[kuba]

Please use gen_pkey: I'd like all tests to be self-contained within one script (e.g. so that they can be run from binaries created with PyInstaller), so we would have to inline entire key, which doesn't look great. If at some point that becomes of an performance concern, we will think about it then.

BTW if we were to use testdata, we would use pkg_resources to load such.

[Lekensteyn]

I've tried that, but that seems to require so many indirections (implementation details) that I decided to use this approach instead. I have a small preference to fixed files for reproducibility and faster key generation.

Also, the current IOPlugin.Data thing does not scale, what about setting defaults to None? Something like (out of my head):

IOPlugin.Data.__defaults__ = (None, ) * 4

Then it should become something like key_data = IOPlugin.Data(key=gen_pkey(1024)) (+ some indirections, I could not get this to work).

[kuba]

There are 3 parties potentially involved with the typical usage scenario:

1. user under which the client is run
2. user that runs httpd
3. root

If we assume that those three parties are different users (say nobody, nginx, root), then there are some usability problems, because:

• (1) needs to open and write: account key, chain, cert and key
• (2) needs to read chain, cert and key
• we don't run as root, so we cannot chmod or chown

Restricting key file to 0o600 would mean that httpd is not able to read necessary files :(

I would like those kind of cases to be clearly sorted out (and by that I mean short design doc, possibly included in the repo itself) before merging.

[danmilon]

AFAIK apache runs as root to bind to privilaged ports and/or read the private key, and then spawns other processes as a non-privilaged user (e.g. www-data) to do the actual request handling.

I don't know if other web servers work similarly.

[Lekensteyn]

Comments have been addressed, the code has also been made simpler (both the test and IOPlugin.Data values).

[kuba]

you forgot to remove this file

[kuba]

When I said "design doc" I expected discussion about alternative solutions, giving it's pros and cons. Since you picked one particular implementation, it's rather hard to discuss it. I'm not convinced that the solution presented in this PR is the best one. Notably, that official client has similar problems, and AFAIK there might not be perfect solution to the problem - but at least I would like to know how much we are limited by chosen solution.

Please keep PRs in their original scope. This is not the right place to discuss Data.__new__.__defaults__ magic. Please revert and feel free to submit a separate PR.

[kuba]

For example, why not simply allow --sensitive_files_mode 660?