Skip to content

Create private key files with read and write permissions for the user only #231

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 11, 2022
Merged

Create private key files with read and write permissions for the user only #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6264e70
Add an optional mode argument to FilesystemBackend.put()
sechkova Apr 14, 2020
eb40113
Create private keys with mode 0o600
sechkova Apr 14, 2020
9574d92
Update test_B7_persist_temp_file
sechkova Apr 14, 2020
4d026be
Update test_generate_keypair_wrappers
sechkova Dec 7, 2020
0fffe11
Update _generate_and_write_*_keypair side effects
sechkova Dec 7, 2020
c58d65d
Preserve filesystem abstraction when restricting file permissions
joshuagl Sep 21, 2022
6250c79
Tighten docstring language on overwriting files
joshuagl Sep 22, 2022
9432937
Apply suggestions from code review
joshuagl Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions securesystemslib/interface.py
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@
DEFAULT_RSA_KEY_BITS = 3072





def get_password(prompt='Password: ', confirm=False):
"""Prompts user to enter a password.

Expand Down Expand Up @@ -204,6 +201,7 @@ def _generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,
Side Effects:
Prompts user for a password if 'prompt' is True.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -239,7 +237,7 @@ def _generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,
# Write PEM-encoded private key to <filepath>
file_object = tempfile.TemporaryFile()
file_object.write(private.encode('utf-8'))
util.persist_temp_file(file_object, filepath)
util.persist_temp_file(file_object, filepath, restrict=True)

return filepath

Expand Down Expand Up @@ -270,6 +268,7 @@ def generate_and_write_rsa_keypair(password, filepath=None,

Side Effects:
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -306,6 +305,7 @@ def generate_and_write_rsa_keypair_with_prompt(filepath=None,
Side Effects:
Prompts user for a password.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -338,6 +338,7 @@ def generate_and_write_unencrypted_rsa_keypair(filepath=None,

Side Effects:
Writes unencrypted key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -469,6 +470,7 @@ def _generate_and_write_ed25519_keypair(filepath=None, password=None,
Side Effects:
Prompts user for a password if 'prompt' is True.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -508,7 +510,7 @@ def _generate_and_write_ed25519_keypair(filepath=None, password=None,
# Write private key to <filepath>
file_object = tempfile.TemporaryFile()
file_object.write(ed25519_key.encode('utf-8'))
util.persist_temp_file(file_object, filepath)
util.persist_temp_file(file_object, filepath, restrict=True)

return filepath

Expand Down Expand Up @@ -536,6 +538,7 @@ def generate_and_write_ed25519_keypair(password, filepath=None):

Side Effects:
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -568,6 +571,7 @@ def generate_and_write_ed25519_keypair_with_prompt(filepath=None):
Side Effects:
Prompts user for a password.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -595,6 +599,7 @@ def generate_and_write_unencrypted_ed25519_keypair(filepath=None):

Side Effects:
Writes unencrypted key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -713,6 +718,7 @@ def _generate_and_write_ecdsa_keypair(filepath=None, password=None,
Side Effects:
Prompts user for a password if 'prompt' is True.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -752,7 +758,7 @@ def _generate_and_write_ecdsa_keypair(filepath=None, password=None,
# Write private key to <filepath>
file_object = tempfile.TemporaryFile()
file_object.write(ecdsa_key.encode('utf-8'))
util.persist_temp_file(file_object, filepath)
util.persist_temp_file(file_object, filepath, restrict=True)

return filepath

Expand Down Expand Up @@ -780,6 +786,7 @@ def generate_and_write_ecdsa_keypair(password, filepath=None):

Side Effects:
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -812,6 +819,7 @@ def generate_and_write_ecdsa_keypair_with_prompt(filepath=None):
Side Effects:
Prompts user for a password.
Writes key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down Expand Up @@ -839,6 +847,7 @@ def generate_and_write_unencrypted_ecdsa_keypair(filepath=None):

Side Effects:
Writes unencrypted key files to disk.
Overwrites files if they already exist.

Returns:
The private key filepath.
Expand Down
38 changes: 34 additions & 4 deletions securesystemslib/storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@
import logging
import os
import shutil
import stat
from contextlib import contextmanager
from securesystemslib import exceptions
from typing import BinaryIO, IO, Iterator, List
from typing import BinaryIO, IO, Iterator, List, Optional

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -65,7 +66,8 @@ def get(self, filepath: str) -> Iterator[BinaryIO]:


@abc.abstractmethod
def put(self, fileobj: IO, filepath: str) -> None:
def put(self, fileobj: IO, filepath: str, restrict: Optional[bool] = False
) -> None:
"""
<Purpose>
Store a file-like object in the storage backend.
Expand All @@ -79,6 +81,13 @@ def put(self, fileobj: IO, filepath: str) -> None:
filepath:
The full path to the location where 'fileobj' will be stored.

restrict:
Whether the file should be created with restricted permissions.
What counts as restricted is backend-specific. For a filesystem on a
UNIX-like operating system, that may mean read/write permissions only
for the user (octal mode 0o600). For a cloud storage system, that
likely means Cloud provider specific ACL restrictions.

<Exceptions>
securesystemslib.exceptions.StorageError, if the file can not be stored.

Expand Down Expand Up @@ -208,14 +217,35 @@ def get(self, filepath:str) -> Iterator[BinaryIO]:
file_object.close()


def put(self, fileobj: IO, filepath: str) -> None:
def put(self, fileobj: IO, filepath: str, restrict: Optional[bool] = False
) -> None:
# If we are passed an open file, seek to the beginning such that we are
# copying the entire contents
if not fileobj.closed:
fileobj.seek(0)

# If a file with the same name already exists, the new permissions
# may not be applied.
try:
os.remove(filepath)
except OSError:
pass

try:
with open(filepath, 'wb') as destination_file:
if restrict:
# On UNIX-based systems restricted files are created with read and
# write permissions for the user only (octal value 0o600).
fd = os.open(filepath, os.O_WRONLY|os.O_CREAT,
stat.S_IRUSR|stat.S_IWUSR)
else:
# Non-restricted files use the default 'mode' argument of os.open()
# granting read, write, and execute for all users (octal mode 0o777).
# NOTE: mode may be modified by the user's file mode creation mask
# (umask) or on Windows limited to the smaller set of OS supported
# permisssions.
fd = os.open(filepath, os.O_WRONLY|os.O_CREAT)

with os.fdopen(fd, "wb") as destination_file:
shutil.copyfileobj(fileobj, destination_file)
# Force the destination file to be written to disk from Python's internal
# and the operating system's buffers. os.fsync() should follow flush().
Expand Down
10 changes: 8 additions & 2 deletions securesystemslib/util.py
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,8 @@ def persist_temp_file(
temp_file: IO,
persist_path: str,
storage_backend: Optional[StorageBackendInterface] = None,
should_close: bool = True
should_close: bool = True,
restrict: bool = False
) -> None:
"""
<Purpose>
Expand All @@ -203,6 +204,11 @@ def persist_temp_file(
A boolean indicating whether the file should be closed after it has been
persisted. Default is True, the file is closed.

restrict:
A boolean indicating whether the file should have restricted privileges.
What evactly counts as restricted privileges is an implementation detail
of the backing StorageBackendInterface implementation.

<Exceptions>
securesystemslib.exceptions.StorageError: If file cannot be written.

Expand All @@ -213,7 +219,7 @@ def persist_temp_file(
if storage_backend is None:
storage_backend = FilesystemBackend()

storage_backend.put(temp_file, persist_path)
storage_backend.put(temp_file, persist_path, restrict=restrict)

if should_close:
temp_file.close()
Expand Down
8 changes: 8 additions & 0 deletions tests/test_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ def test_generate_keypair_wrappers(self):

"""
key_pw = "pw"
expected_priv_mode = stat.S_IFREG|stat.S_IRUSR|stat.S_IWUSR
for idx, (gen, gen_prompt, gen_plain, import_priv, schema) in enumerate([
(
generate_and_write_rsa_keypair,
Expand Down Expand Up @@ -661,6 +662,10 @@ def test_generate_keypair_wrappers(self):
priv = import_priv(fn_encrypted, key_pw)
self.assertTrue(schema.matches(priv), assert_msg)

# Test that encrypted private key is generated with read and write
# permissions for user only
self.assertEqual(os.stat(fn_encrypted).st_mode, expected_priv_mode)

# Test generate_and_write_*_keypair errors if password is None or empty
with self.assertRaises(FormatError, msg=assert_msg):
fn_encrypted = gen(None)
Expand Down Expand Up @@ -688,6 +693,9 @@ def test_generate_keypair_wrappers(self):
priv = import_priv(fn_unencrypted)
self.assertTrue(schema.matches(priv), assert_msg)

# Test that unencrypted private key is generated with read and write
# permissions for user only
self.assertEqual(os.stat(fn_unencrypted).st_mode, expected_priv_mode)


def test_import_publickeys_from_file(self):
Expand Down
13 changes: 12 additions & 1 deletion tests/test_util.py
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import tempfile
import unittest
import timeit
import stat

import securesystemslib.settings
import securesystemslib.hash
Expand Down Expand Up @@ -240,8 +241,18 @@ def test_B9_persist_temp_file(self):
dest_path = os.path.join(dest_temp_dir, self.random_string())
tmpfile = tempfile.TemporaryFile()
tmpfile.write(self.random_string().encode('utf-8'))
securesystemslib.util.persist_temp_file(tmpfile, dest_path)

# Write a file with restricted permissions
securesystemslib.util.persist_temp_file(tmpfile, dest_path, restrict=True)
self.assertTrue(dest_path)

# Need to set also the stat.S_IFREG bit to match the st_mode output
# stat.S_IFREG - Regular file
expected_mode = stat.S_IFREG | stat.S_IRUSR | stat.S_IWUSR
if os.name == 'nt':
# Windows only supports setting the read-only attribute.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@joshuagl joshuagl Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expected_mode = stat.S_IFREG | stat.S_IWUSR | stat.S_IRUSR | stat.S_IWGRP | stat.S_IRGRP | stat.S_IWOTH | stat.S_IROTH
self.assertEqual(os.stat(dest_path).st_mode, expected_mode)
self.assertTrue(tmpfile.closed)

# Test persisting a file without automatically closing the tmpfile
Expand Down