Skip to content
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

Consolidate key interface password handling and general overhaul #288

Merged
merged 13 commits into from
Nov 6, 2020
Merged

Consolidate key interface password handling and general overhaul #288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9ee6998
Add and adopt decryption password interface helper
lukpueh Oct 9, 2020
a8508ee
Add and adopt encryption password interface helper
lukpueh Oct 9, 2020
bc62907
Adopt password helper in ecdsa interface functions
lukpueh Oct 9, 2020
7d00ecb
Clean up interface function bodies
lukpueh Oct 16, 2020
2379b7f
Reformat, update and clean-up interface docstrings
lukpueh Oct 16, 2020
7d37d55
Adopt recent interface changes in README.rst
lukpueh Oct 12, 2020
698f54f
Add generic import_privatekey_from_file function
lukpueh Oct 14, 2020
1db7f21
Fix docstring and rename junk vars in interface
lukpueh Oct 28, 2020
fae4ebc
Add blank lines and minor doc fixes in interface
lukpueh Nov 4, 2020
dd2c5fc
Make interface.generate_*_keypair funcs protected
lukpueh Nov 4, 2020
edcf74e
Add thin interface._generate_*_keypair wrappers
lukpueh Nov 4, 2020
1cb5bcd
Fix interface encryption password helper call time
lukpueh Nov 5, 2020
9a74f30
Refine interface helper docstrings
lukpueh Nov 6, 2020
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
101 changes: 34 additions & 67 deletions securesystemslib/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,38 @@ def get_password(prompt='Password: ', confirm=False):



def _get_key_file_decryption_password(password, prompt, path):
"""Decryption password helper.

- Fail if 'password' is passed and 'prompt' is True (precedence unclear)
- Return None on empty pw on prompt (suggests desire to not decrypt)

"""
securesystemslib.formats.BOOLEAN_SCHEMA.check_match(prompt)

# We don't want to decide which takes precedence so we fail
if password is not None and prompt:
raise ValueError("passing 'password' and 'prompt=True' is not allowed")

# Prompt user for password
if prompt:
password = get_password("enter password to decrypt private key file "
"'" + TERM_RED + str(path) + TERM_RESET + "' "
"(leave empty if key not encrypted): '", confirm=False)

# Treat empty password as no password. A user on the prompt can only
# indicate the desire to not decrypt by entering no password.
if not len(password):
return None

if password is not None:
securesystemslib.formats.PASSWORD_SCHEMA.check_match(password)
# No additional vetting needed. Decryption will show if it was correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

So we allow empty string decryption passwords here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly.


return password



def generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,
password=None):
"""
Expand Down Expand Up @@ -302,42 +334,7 @@ def import_rsa_privatekey_from_file(filepath, password=None,

# Is 'scheme' properly formatted?
securesystemslib.formats.RSA_SCHEME_SCHEMA.check_match(scheme)

if password and prompt:
raise ValueError("Passing 'password' and 'prompt' True is not allowed.")

# If 'password' was passed check format and that it is not empty.
if password is not None:
securesystemslib.formats.PASSWORD_SCHEMA.check_match(password)

# TODO: PASSWORD_SCHEMA should be securesystemslib.schema.AnyString(min=1)
if not len(password):
raise ValueError('Password must be 1 or more characters')

elif prompt:
# Password confirmation disabled here, which should ideally happen only
# when creating encrypted key files (i.e., improve usability).
# It is safe to specify the full path of 'filepath' in the prompt and not
# worry about leaking sensitive information about the key's location.
# However, care should be taken when including the full path in exceptions
# and log files.
# NOTE: A user who gets prompted for a password, can only signal that the
# key is not encrypted by entering no password in the prompt, as opposed
# to a programmer who can call the function with or without a 'password'.
# Hence, we treat an empty password here, as if no 'password' was passed.
password = get_password('Enter a password for an encrypted RSA'
' file \'' + TERM_RED + filepath + TERM_RESET + '\': ',
confirm=False) or None

if password is not None:
# This check will not fail, because a mal-formatted passed password fails
# above and an entered password will always be a string (see get_password)
# However, we include it in case PASSWORD_SCHEMA or get_password changes.
securesystemslib.formats.PASSWORD_SCHEMA.check_match(password)

else:
logger.debug('No password was given. Attempting to import an'
' unencrypted file.')
password = _get_key_file_decryption_password(password, prompt, filepath)
Copy link
Contributor

Choose a reason for hiding this comment

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

This call would fail if the user does not set either the password or prompt, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it will return None. Because callers may use import_*_privatekey_from_file with neither password nor prompt=True to import an unencrypted private key.

See import_*_privatekey_from_file docstrings:
"If a password is passed or entered on the prompt, the private key is decrypted, otherwise it is treated as unencrypted."


if storage_backend is None:
storage_backend = securesystemslib.storage.FilesystemBackend()
Copy link
Contributor

Choose a reason for hiding this comment

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

Off-topic, but: now that I see how we keep using this, we should make a singleton out of FilesystemBackend() rather than instantiating it over and over again wherever we need to use it. @joshuagl

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Mind submitting a feature request?

Expand Down Expand Up @@ -643,41 +640,11 @@ def import_ed25519_privatekey_from_file(filepath, password=None, prompt=False,
# types, and that all dict keys are properly named.
# Raise 'securesystemslib.exceptions.FormatError' if there is a mismatch.
securesystemslib.formats.PATH_SCHEMA.check_match(filepath)

if password and prompt:
raise ValueError("Passing 'password' and 'prompt' True is not allowed.")
password = _get_key_file_decryption_password(password, prompt, filepath)

if storage_backend is None:
storage_backend = securesystemslib.storage.FilesystemBackend()

# If 'password' was passed check format and that it is not empty.
if password is not None:
securesystemslib.formats.PASSWORD_SCHEMA.check_match(password)

# TODO: PASSWORD_SCHEMA should be securesystemslib.schema.AnyString(min=1)
if not len(password):
raise ValueError('Password must be 1 or more characters')

elif prompt:
# Password confirmation disabled here, which should ideally happen only
# when creating encrypted key files (i.e., improve usability).
# It is safe to specify the full path of 'filepath' in the prompt and not
# worry about leaking sensitive information about the key's location.
# However, care should be taken when including the full path in exceptions
# and log files.
# NOTE: A user who gets prompted for a password, can only signal that the
# key is not encrypted by entering no password in the prompt, as opposed
# to a programmer who can call the function with or without a 'password'.
# Hence, we treat an empty password here, as if no 'password' was passed.
password = get_password('Enter a password for an encrypted RSA'
' file \'' + TERM_RED + filepath + TERM_RESET + '\': ',
confirm=False)

# If user sets an empty string for the password, explicitly set the
# password to None, because some functions may expect this later.
if len(password) == 0:
password = None

# Finally, regardless of password, try decrypting the key, if necessary.
# Otherwise, load it straight from storage.
with storage_backend.get(filepath) as file_object:
Expand Down
21 changes: 14 additions & 7 deletions tests/test_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,14 @@ def test_rsa(self):
([fn_encrypted], {}, CryptoError,
"Password was not given but private key is encrypted"),
# Error on encrypted but empty pw passed
([fn_encrypted], {"password": ""}, ValueError,
"Password must be 1 or more character"),
([fn_encrypted], {"password": ""}, CryptoError,
"Password was not given but private key is encrypted"),
# Error on encrypted but bad pw passed
([fn_encrypted], {"password": "bad pw"}, CryptoError,
"Bad decrypt. Incorrect password?"),
# Error on pw and prompt
([fn_default], {"password": pw, "prompt": True}, ValueError,
"Passing 'password' and 'prompt' True is not allowed.")]):
"passing 'password' and 'prompt=True' is not allowed")]):

with self.assertRaises(err, msg="(row {})".format(idx)) as ctx:
import_rsa_privatekey_from_file(*args, **kwargs)
Expand Down Expand Up @@ -258,6 +258,10 @@ def test_rsa(self):
with self.assertRaises(FormatError):
import_rsa_privatekey_from_file(fn_default, password=123456)

# bad prompt
with self.assertRaises(FormatError):
import_rsa_privatekey_from_file(fn_default, prompt="not-a-bool")



def test_ed25519(self):
Expand Down Expand Up @@ -381,15 +385,15 @@ def test_ed25519(self):
([fn_encrypted], {}, CryptoError,
"Malformed Ed25519 key JSON, possibly due to encryption, "
"but no password provided?"),
# Error on encrypted but empty pw passed
([fn_encrypted], {"password": ""}, ValueError,
"Password must be 1 or more character"),
# Error on encrypted but empty pw
([fn_encrypted], {"password": ""}, CryptoError,
"Decryption failed."),
# Error on encrypted but bad pw passed
([fn_encrypted], {"password": "bad pw"}, CryptoError,
"Decryption failed."),
# Error on pw and prompt
([fn_default], {"password": pw, "prompt": True}, ValueError,
"Passing 'password' and 'prompt' True is not allowed.")]):
"passing 'password' and 'prompt=True' is not allowed")]):

with self.assertRaises(err, msg="(row {})".format(idx)) as ctx:
import_ed25519_privatekey_from_file(*args, **kwargs)
Expand Down Expand Up @@ -420,6 +424,9 @@ def test_ed25519(self):
with self.assertRaises(FormatError):
import_ed25519_privatekey_from_file(fn_default, password=123456)

# Error on bad prompt format
with self.assertRaises(FormatError):
import_ed25519_privatekey_from_file(fn_default, prompt="not-a-bool")


def test_ecdsa(self):
Expand Down