Create private key files with read and write permissions for the user only by sechkova · Pull Request #231 · secure-systems-lab/securesystemslib

sechkova · 2020-04-16T15:26:07Z

Fixes issue #: #222

Description of the changes being introduced by the pull request:

Private key files should be created with read and write permissions for the user only (0o600).

util.persist_temp_file() takes an optional permissions argument
add an optional permissions argument to StorageBackendInterface.put() and FilesystemsBackend.put()
interface.generate_and_write_{rsa, ed25519, ecdsa}_keypair functions pass a permissions value 0o600 when creating the private key file

Note: file permissions are calculated as default file mode (0o777) - umask (0o177)

Please verify and check that the pull request fulfils the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

coveralls · 2020-04-16T15:28:37Z

Coverage increased (+0.005%) to 98.794% when pulling 01431b5 on sechkova:issue-222 into d5aa8e6 on secure-systems-lab:master.

sechkova · 2020-04-16T15:29:28Z

I came across the 'mode' limitation in the Python os module on Windows platforms coming from the different access control implementations in Unix and Windows. It is documented only for the os.chmod function but it applies also for os.open and os.umask:

Note Although Windows supports chmod(), you can only set the file’s read-only flag with it (via the stat.S_IWRITE and stat.S_IREAD constants or a corresponding integer value). All other bits are ignored.

Meaning that the same concepts for restricting Group/Other permissions do not apply on Windows.

I added a workaround in the test case, however, I will be happy to understand the level of Windows support in securesystemslib.

joshuagl · 2020-04-21T15:41:26Z

Thanks for the quality submission @sechkova.

Unfortunately, I think we need to hold this PR for a while until we figure out if/how we can implement this functionality in a system with abstracted filesystems as proposed in PR #232

joshuagl · 2020-05-15T10:39:55Z

Now that we have abstract files and directories (#232) landed, let's figure out how to implement this change in the new world.

I think it probably makes sense to have an optional argument to securesystemslib.storage. StorageBackendInterface.put() that modifies the on-disk permissions. Implementers of StorageBackendInterface can choose what to do with this optional parameter, for example they might choose to store the permissions in their storage system's metadata and apply them at get() time? Or they might choose to ignore them.

What do you think @sechkova ?

sechkova · 2020-05-27T13:47:11Z

An optional argument to StorageBackendInterface.put() makes sense to me too + applying the changes from this PR on the FilesystemBackend implementation.

sechkova · 2020-06-05T16:25:58Z

Let me know what do you think.
And my Windows concerns from the comment above remain unchanged 🙂

jku

4 months later I get to this :|

On Windows: I think it's ok to not support Windows ACLs as long as it's documented -- we're doing what we can with the file permissions available.

My only request for change is umask(), assuming it doesn't get much more complex with os.open(). The others are just comments.

jku · 2020-10-24T09:35:32Z

+    # When calculating file permissions, the current OS umask value is first
+    # masked out. If custom permissions are set, we calculate a new umask
+    # followed by a call to open() with default permissions (0o777).
+    # This trick is done since open() does not support mode(permissions) as
+    # input parameter, alternatively os.open() can be used.


I think the os.open() method is preferable if there are no other complications using it: os.umask() changes umask for the whole process. This creates a race condition and could lead to bugs in a multithreaded application so should be avoided in a library.

I had a look again. If we use os.open(), its documentation says "When computing mode, the current umask value is first masked out." Meaning if we want to avoid race conditions caused by os.umask, we can pass the desired permissions to os.open but the resulting file permissions maybe restricted even further due to the applied mask.

This is not necessarily a bad thing for this concrete case where we want to restrict keys permissions. We can put the same line as a disclaimer in the docs.

the comment seems to refer to the earlier version of the PR?

jku · 2020-10-24T09:52:01Z

+        The permissions of the file-like object. If None, the
+        implementation-specific default permissions apply.


The actual type of permissions (int) should be specified, maybe with mention to constants in stat module

jku · 2020-10-24T09:55:09Z

+    permissions:
+      Custom file permissions for the newly created file. If None, the default
+      OS permissions apply.


The actual type of permissions (int) should be specified, maybe with mention to constants in stat module

jku · 2020-10-24T10:00:42Z

 SUPPORTED_KEY_TYPES = ['rsa', 'ed25519']

+# Private keys are created with read and write permissions for the user only
+PRIVATE_KEY_MODE = 0o600


I know some people prefer the octal form so I don't make any demands, just note that this could be
PRIVATE_KEY_MODE = stat.S_IRUSR | stat.S_IWUSR

Yes, let's use the constants.

sechkova · 2020-11-17T11:17:52Z

A loong time has passed since opening this pull request ... let me resolve the conflicts and I will revive the thread.

sechkova · 2020-11-27T18:57:22Z

I changed a bit the original idea.
As pointed in #231 (comment) os.unmask is not recommended for a library so I decided to wrap os.open. The desired permissions are set but umask is masked out first.
This makes the user responsible of calling os.umask if needed.

jku

Left some comments.

Also, can you add a test (or modify an existing one) for the actual beef of the PR: that private keys are written with the correct permissions?

jku · 2020-11-30T13:34:51Z

+    # When calculating file permissions, the current OS umask value is first
+    # masked out. If custom permissions are set, we calculate a new umask
+    # followed by a call to open() with default permissions (0o777).
+    # This trick is done since open() does not support mode(permissions) as
+    # input parameter, alternatively os.open() can be used.


the comment seems to refer to the earlier version of the PR?

sechkova · 2020-12-07T15:07:57Z

Pushed a new bunch of commits:

adding test for creating private keys with restricted permissions
fixing wrong comments and pointing to os.open mode documentation in the docstrings

Related to overwriting keys, since this is the current behaviour:

added a side effect in the docstrings of _generate_keypair wrappers that keys with the same file path will be overwritten
@joshuagl recommended addressing the problem in a separate issue and it turned out it already exists: filepath arg to generate_and_write_*_keypair may already exist #172

sechkova · 2020-12-07T15:11:07Z

Reworded the last commit 01431b5

jku

Code looks fine to me. I have two high level comments though (I'm sorry for not coming up with them earlier already).

StorageBackendInterface is supposedly abstract (I assume possibly not backed by a filesystem at all) but now we expect it to work with POSIX file permission -- should this have been a more abstract: "private/not private"? Or have a I misunderstood what the ABC is about?
The fact that the ABC method signature changes is a bit annoying: I think linters pick it up though so any out-of-tree implementations of the ABC should notice when they update SSLib (can anyone confirm from memory if linters check for the ABC method signature?)

Still, might be worth notifying the known implementors or even asking their opinion on the change. Does warehouse implement their own StorageBackendInterface?

Add an optional argument to StorageBackendInterface and FilesystemsBackend put() method for creating a file-object with custom mode (permissions). If custom permissions are not set, the implementation-specific default permissions apply. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Update generate_and_write_{rsa, ed25519, ecdsa}_keypair functions to create private keys with read and write permissions for the user only (0o600). Update util.persist_temp_file() with an optional permissions parameter. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Add a test case for persisting a file with custom permissions Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Add test case checking that private key is generated with read and write permissions for user only. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Document as a side effect the overwriting of existing keys with the same filepath. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Rather than leak filesystem details to the user by exposing the file mode we add an abstract "restrict" boolean parameter. How the created object on the storage is restricted is an implementation detail of the StorageBackend. Suggested-by: Jussi Kukkonen <jkukkonen@google.com> Signed-off-by: Joshua Lock <jlock@vmware.com>

Signed-off-by: Joshua Lock <jlock@vmware.com>

joshuagl · 2022-09-22T09:14:24Z

I just pushed updates to rebase this PR on master and address Jussi's first comment in #231 (review)

StorageBackendInterface is supposedly abstract

I changed the permissions parameter to a boolean restrict which lets implementing StorageBackendInterface's determine how best to interpret what restricted means. For FileSystemBackend the implementation sets the permissions of the file to 0o600 when restrict=True.

lukpueh

Thanks for the PR, @sechkova and @joshuagl! Code looks good. I have two minor suggestions and one more important question inline...

lukpueh · 2022-10-04T12:00:12Z

+        fd = os.open(filepath, os.O_WRONLY|os.O_CREAT,
+          stat.S_IRUSR|stat.S_IWUSR)
+      else:
+        # Use the default value (0o777) of the 'mode' argument of os.open()


Minor comment nits:
Is it worth to mention that passed and default permissions may be silently masked out by umask, or limited to 666 by windows? And is it worth to mention what 666 and 777 means?

I'll take a stab at including that info.

lukpueh · 2022-10-04T12:06:19Z

+    # stat.S_IFREG - Regular file
+    expected_mode = stat.S_IFREG | stat.S_IRUSR | stat.S_IWUSR
+    if os.name != 'posix':
+      # Windows only supports setting the read-only attribute.


Is this comment accurate? Doesn't 666 mean rw-rw-rw?

It's really hard to locate and parse the docs around this.

Python's os.open() points to the Windows docs for _open() in their C Runtime which say that possible modes on Windows are:

pmode Meaning

_S_IREAD Only reading permitted.

_S_IWRITE Writing permitted. (In effect, permits reading and writing.)

_S_IREAD | _S_IWRITE Reading and writing permitted.

which I read as being effectively 0o666 on Windows. Some additional confusion creeps in because Python's os.chmod() docs state:

Note Although Windows supports chmod(), you can only set the file’s read-only flag with it (via the stat.S_IWRITE and stat.S_IREAD constants or a corresponding integer value). All other bits are ignored.

but I assume this means the mode cannot be changed from stat.S_IREAD to stat.S_IWRITE. However, I don't have access to a Windows box to verify. Nor do we have CI set up for Windows, so perhaps that (having Windows testing) is a blocker for this PR?

Thanks for the detailed research. Just ran the test on a windows box and it works. Having Windows CI does not seem to be a blocker for this PR.

OK, great. I think I will push an extra commit to change the os.name != 'posix' to os.name == 'nt' for additional clarity (I don't expect anyone is using sslib on ~~QNX~~ 'java', but 🤷).

Added that change as a suggestion https://github.com/secure-systems-lab/securesystemslib/pull/231/files#r988760854

lukpueh · 2022-10-11T09:56:28Z

@joshuagl, are you going to commit these changes, or are you waiting for me to press the button? :D

Co-authored-by: Lukas Pühringer <luk.puehringer@gmail.com>

joshuagl · 2022-10-11T10:01:48Z

@joshuagl, are you going to commit these changes, or are you waiting for me to press the button? :D

Oh, thanks for the nudge. Batched and committed.

The incompatibility with newer securesystemslib versions was caused because of a new breaking change introduced in: secure-systems-lab/securesystemslib#231 Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Fix sslib new versions incompatibility bug The incompatibility with newer securesystemslib versions was caused because of a new breaking change introduced in: secure-systems-lab/securesystemslib#231 Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * LocalStorage.put use restrict by default We want to use the "restrict" by default as we are the ones who will save metadata and target files through LocalStorage.put and we want to have as least privileges as possible because of security. Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Isort lint fix Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Update securesystemslib to 0.26.0 Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Make "restrict" true by default for IStorage.put() Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Remove StorageBackendInterface dependency Simplify IStorage by not inheriting StorageBackendInterface anymore. In the last securesystemslib version, the contributors there added a lot of functionality inside the StorageBackendInterface which we don't use and is irrelevant to us. Additionally, we had at least two cases where our tests failed because of StorageBackendInterface API changes and we were blocked to use an older securesystemslib version or update IStorage accordingly. Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Make IStorage an interface again By mistake I have made IStorage a regular class as opposed to an interface which is its purpose. Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Move write file logic of persist into IStorage.put Move the logic where we actually save the file content into the IStorage.put API call as this will help us easily support more storages within persist() call. Signed-off-by: Martin Vrachev <mvrachev@vmware.com> * Remove unused imports Signed-off-by: Martin Vrachev <mvrachev@vmware.com> Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

sechkova changed the title ~~Issue 222~~ Create private key files with read and write permissions for the user only Apr 16, 2020

sechkova force-pushed the issue-222 branch from 33486ba to 0b9fe95 Compare June 5, 2020 16:18

jku requested changes Oct 24, 2020

View reviewed changes

sechkova force-pushed the issue-222 branch 2 times, most recently from 9929300 to d66764e Compare November 27, 2020 18:48

sechkova commented Nov 27, 2020

View reviewed changes

Comment thread securesystemslib/storage.py

jku reviewed Nov 30, 2020

View reviewed changes

sechkova force-pushed the issue-222 branch 2 times, most recently from c4a8770 to d0f880f Compare December 7, 2020 14:56

sechkova force-pushed the issue-222 branch from d0f880f to 01431b5 Compare December 7, 2020 15:10

jku reviewed Dec 18, 2020

View reviewed changes

Comment thread securesystemslib/interface.py Outdated

Comment thread tests/test_util.py Outdated

sechkova and others added 6 commits September 21, 2022 17:47

Update test_B7_persist_temp_file

9574d92

Add a test case for persisting a file with custom permissions Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Update test_generate_keypair_wrappers

4d026be

Add test case checking that private key is generated with read and write permissions for user only. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

Update _generate_and_write_*_keypair side effects

0fffe11

Document as a side effect the overwriting of existing keys with the same filepath. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

joshuagl force-pushed the issue-222 branch from 01431b5 to c58d65d Compare September 22, 2022 09:07

Tighten docstring language on overwriting files

6250c79

Signed-off-by: Joshua Lock <jlock@vmware.com>

joshuagl requested review from jku and lukpueh October 3, 2022 11:05

lukpueh requested changes Oct 4, 2022

View reviewed changes

lukpueh mentioned this pull request Oct 5, 2022

Create backwards-compatible RSA PSS sigs by default #431

Closed

3 tasks

joshuagl reviewed Oct 6, 2022

View reviewed changes

Comment thread tests/test_util.py Outdated

joshuagl reviewed Oct 6, 2022

View reviewed changes

Comment thread securesystemslib/storage.py Outdated

Apply suggestions from code review

9432937

Co-authored-by: Lukas Pühringer <luk.puehringer@gmail.com>

lukpueh approved these changes Oct 11, 2022

View reviewed changes

lukpueh merged commit 57fbcdd into secure-systems-lab:master Oct 11, 2022

joshuagl mentioned this pull request Oct 11, 2022

Create private key files with umask 177 #222

Closed

MVrachev mentioned this pull request Jan 5, 2023

Fix sslib new versions incompatibility bug repository-service-tuf/repository-service-tuf-worker#159

Merged

		The permissions of the file-like object. If None, the
		implementation-specific default permissions apply.

pmode	Meaning
_S_IREAD	Only reading permitted.
_S_IWRITE	Writing permitted. (In effect, permits reading and writing.)
_S_IREAD \| _S_IWRITE	Reading and writing permitted.

Uh oh!

Conversation

sechkova commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshuagl commented Apr 21, 2020

Uh oh!

joshuagl commented May 15, 2020

Uh oh!

sechkova commented May 27, 2020

Uh oh!

sechkova commented Jun 5, 2020

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sechkova Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sechkova commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sechkova commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sechkova commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova commented Dec 7, 2020

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joshuagl commented Sep 22, 2022

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sechkova commented Apr 16, 2020 •

edited

Loading

coveralls commented Apr 16, 2020 •

edited

Loading

sechkova commented Apr 16, 2020 •

edited

Loading

sechkova Nov 25, 2020 •

edited

Loading

sechkova commented Nov 17, 2020 •

edited

Loading

sechkova commented Nov 27, 2020 •

edited

Loading

jku left a comment •

edited

Loading

sechkova commented Dec 7, 2020 •

edited

Loading

joshuagl Oct 6, 2022 •

edited

Loading