Add prompt argument to import RSA private key function #124

lukpueh · 2018-03-08T17:00:59Z

Fixes issue #:
Fixes RSA part of #122

Description of the changes being introduced by the pull request:
Add an optional boolean prompt argument to interface.import_rsa_privatekey_from_file and change the behavior of the function like so:

If password is passed use passed password for decryption.
If prompt is True use entered password for decryption.
If no password is passed or entered, or if the entered password is an empty string, omit decryption.
Passing and prompting for a password is not possible.

This PR further:

adds mock as dev/ci requirement for supported Python versions <3.3, i.e. 2.7, to mock password prompts, and
adopts and adds related unit tests.

See commit messages, code comments or #122 for more details.

Please verify and check that the pull request fulfills 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

Add an optional boolean 'prompt' argument to interface.import_rsa_privatekey_from_file and change the behavior of the function like so: If password is passed use passed password for decryption. If prompt is True use entered password for decryption. If no password is passed or entered, or if the entered password is an empty string, omit decryption. Passing and prompting for a passowrd is not possible. See code comments or secure-systems-lab#122 for more details. This commit also adopts the unit tests accordingly.

Add two tests to test failing import of encrypted private key when no password is passed, and failing import of non-encrypted private key when a password is passed.

Add Python mock package as dev and and ci requirement for versions <3.3 (above it's in the standard library) and add interface.import_rsa_privatekey_from_file tests that mock passwords entered on the prompt.

awwad · 2018-03-08T17:42:34Z

I'll review.

It would be good to have a clear sense of what the status quo pre-PR was in the PR description. (My TUF fork in Uptane is still pretty remote, so I think the behavior is somewhat different.)

That might be too demanding. ' dunno. (:

Edit: yeah, way too demanding. It's right in the issue description, basically. Disregard!

awwad · 2018-03-08T20:57:17Z

Consider the following alternative (which I don't say is superior or anything -- just worth considering):

optional argument 'encrypt' instead of optional argument 'prompt'

lukpueh · 2018-03-08T21:38:21Z

Summarizing our conversation on the whiteboard:

I'd like to keep prompt because, it should be explicit whether a function expects user input,
If prompt is True and a password is passed, we could (1) not prompt and use the passed pw, (2) prompt and override the passed pw with the entered pw, (3) fail with an argument error. Any of these make sense, so we can stick with solution (3).

lukpueh · 2018-03-09T18:16:48Z

Maybe we should put it up for vote? @vladimir-v-diaz, @SantiagoTorres what do you guys think?

awwad · 2018-03-13T15:10:02Z

(I think that keeping prompt and doing (3) is fine.)

vladimir-v-diaz · 2018-03-13T15:40:50Z

Approach (3) is fine with me.

If we make prompt optional, how do we deal with guaranteeing consistent behavior across projects? For instance, the TUF project might choose to set prompt=True, whereas in-toto uses prompt=False.

I suppose we can recommend that the CLI tools (that use securesystemslib) handle the option of prompting for a password. In that case, an integrator who uses both in-toto and TUF can expect consistent behavior
by using, for example, $ in-toto --sign --prompt-pw and $ repo.py --sign --prompt-pw.

awwad

LGTM. I left a few questions and comments. We can merge, or I can make the adjustments mentioned.

awwad · 2018-03-13T15:12:07Z

README.rst

+    # Import an existing private key.  If your private key is encrypted,
+    # which it should be, you either have to pass a 'password' or enter one
+    # on the prompt.
+    >>> private_rsa_key1 = import_rsa_privatekey_from_file("rsa_key1", prompt=True)


Consider a second example, since you've outlined two options in the comment above:

>>> private_rsa_key1 = import_rsa_privatekey_from_file("rsa_key1", prompt=True) >>> private_rsa_key1 = import_rsa_privatekey_from_file("rsa_key1", password='some passphrase")

awwad · 2018-03-13T15:13:22Z

securesystemslib/interface.py

+    If prompt is True use entered password for decryption.
+    If no password is passed or entered, or if the entered password is an empty
+    string, omit decryption.
+


awwad · 2018-03-13T15:14:43Z

securesystemslib/interface.py

+    If no password is passed or entered, or if the entered password is an empty
+    string, omit decryption.
+
+    Passing and prompting for a password is not possible.


Would say "is not possible and returns an error." This is made clear in the Exceptions list below, but I think there's no sense not having it here.

awwad · 2018-03-13T18:10:04Z

securesystemslib/interface.py

+
+  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.


Forgive my ignorance: why is it safe to specify the full path of the key in the prompt, but not in exceptions or log files?

AFAIK, Trishank and Vlad made this decision, because in order for the prompt to show the full path, the full path must be known to the caller anyway, whereas an exception/log might reveal that information to someone who normally does not know it.

because in order for the prompt to show the full path, the full path must be known to the caller anyway, whereas an exception/log might reveal that information to someone who normally does not know it.

That's correct. Revealing the full path via an exception or log file should be avoided elsewhere.

We want to avoid vulnerabilities like Information exposure through an error message and full path disclosure.

awwad · 2018-03-13T18:25:35Z

securesystemslib/interface.py

+    # Hence, we treat an empty password here, as if no 'password' was passed.
+    password = get_password('Enter a password for an encrypted RSA'
+        ' file \'' + Fore.RED + filepath + Fore.RESET + '\': ',
+        confirm=False) or None


Cute and succinct, but it took me a moment to process the implication of the or None, even with the comment, FWIW. ("If get_password returns an empty string, this construction stores None" might help, but ... it was probably just my brain hiccuping.)

awwad · 2018-03-13T19:01:15Z

securesystemslib/interface.py

+    # 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)


awwad · 2018-03-13T20:30:50Z

tests/test_interface.py

+
+else:
+  import mock
+


(This question in no way interferes with this PR. As a point of general curiosity:
Is it better practice to test Python version numbers this way when determining what to import, or to try the first import and fail over to the second one? What if, say, Python 4.0 had been released before mock was added to unittest in Python 3.3? Should one just see if the duck quacks first?)

IMHO both are fine. In this case I lean towards the version checking approach, because according to Python docs, mock is "available as unittest.mock in Python 3.3 onwards". So if it fails with e.g. 4.0, it would be a Python (docs) bug.
It is important though that the unittests take all supported Python versions into consideration.

There might be a minimal performance difference between exception handling and tuple comparison, but I think we can disregard that.

awwad · 2018-03-13T20:34:56Z

securesystemslib/interface.py

@@ -237,11 +237,20 @@ def generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,


 def import_rsa_privatekey_from_file(filepath, password=None,
-    scheme='rsassa-pss-sha256'):
+    scheme='rsassa-pss-sha256', prompt=False):


NBD: Has there been some discussion about whether or not False should be the default prompt value used?

TUF tutorial text -- and some code, probably -- will need to change.

(What are the security implications of passing the passphrase in through a function call in the interpreter rather than using the prompt functionality from getpass?)

lukpueh · 2018-03-16T17:38:25Z

@awwad, thanks for the review! It would be great if you could make the changes you proposed.\

@vladimir-v-diaz, --prompt-pw makes sense. An alternative that I've often come across is to provide a -p/--password [<password>] option, which can be used to either pass the password as argument or, if the password is not passed, signals the cli tool to prompt for a password.

vladimir-v-diaz · 2018-03-16T17:53:54Z

@vladimir-v-diaz, --prompt-pw makes sense. An alternative that I've often come across is to provide a -p/--password [] option, which can be used to either pass the password as argument or, if the password is not passed, signals the cli tool to prompt for a password.

That's the approach we used in TUF's CLI tool.
https://github.com/theupdateframework/tuf/blob/develop/docs/CLI.md#cli-examples

Covering what a call looks like if you accept a password rather than leaving prompt on. Signed-off-by: Sebastien Awwad <[email protected]>

Clarify behavior if prompt is True and password is provided anyway (raises error), and in other combinations. Also corrects a typo. Signed-off-by: Sebastien Awwad <[email protected]>

awwad · 2018-04-03T20:38:54Z

Recommendations applied. @vladimir-v-diaz Vlad, does merging this affect you?

vladimir-v-diaz · 2018-04-03T20:45:01Z

Not now, but I'll have to update the TUF code once a new version of securesystemslib is released.

Fixes secure-systems-lab#122 together with secure-systems-lab#124 and secure-systems-lab#148 Prepares for fixing in-toto/in-toto#80 TODO (Maybe in separate PRs): - support prompt arg for import_ecdsa_privatekey_from_file (see this commit) - support prompt arg for interface.generate_and_write_*_keypair - support password=None (no encryption) for generate_and_write_*_keypair (is there a way to check if encrypted?) - add import_key_from_file(filepath, password=None, prompt=False, key_type=RSA), for pub/priv, rsa/ecdsa/ed25519 - add import_public_keys_from_file(filepaths, key_types=RSA), for pub/priv, rsa/ecdsa/ed25519 - add import_public_keys_from_gpg(keyids, gpg_home=None) + fix tests/docs/etc...

lukpueh added 6 commits March 8, 2018 11:41

Add import_rsa_privatekey_from_file unittests

6df9a45

Add two tests to test failing import of encrypted private key when no password is passed, and failing import of non-encrypted private key when a password is passed.

Adopt 'prompt' arg in README import rsa privatekey

b063ace

Add mock requirement and add prompt tests

9596b5f

Add Python mock package as dev and and ci requirement for versions <3.3 (above it's in the standard library) and add interface.import_rsa_privatekey_from_file tests that mock passwords entered on the prompt.

Fix typo in import private rsa key docstring

33886e4

Fix typo in import private rsa key error message

4508a78

awwad reviewed Mar 13, 2018

View reviewed changes

awwad approved these changes Mar 13, 2018

View reviewed changes

awwad added 2 commits April 3, 2018 16:26

DOC: Add another example for key import function

8560add

Covering what a call looks like if you accept a password rather than leaving prompt on. Signed-off-by: Sebastien Awwad <[email protected]>

DOC: Arg interactions for import_rsa_privatekey_from_file

d3021a8

Clarify behavior if prompt is True and password is provided anyway (raises error), and in other combinations. Also corrects a typo. Signed-off-by: Sebastien Awwad <[email protected]>

awwad force-pushed the add-prompt-arg branch from 0c04885 to d3021a8 Compare April 3, 2018 20:38

awwad merged commit f01cfbc into secure-systems-lab:master Apr 3, 2018

vladimir-v-diaz mentioned this pull request Apr 11, 2018

Bump version number (0.10.11 -> 0.11.0) and update CHANGELOG #145

Merged

3 tasks

vladimir-v-diaz mentioned this pull request May 7, 2018

No (en|de)cryption of ed25519 private keys when empty password #148

Merged

3 tasks

lukpueh mentioned this pull request Oct 16, 2020

Consolidate key interface password handling and general overhaul #288

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prompt argument to import RSA private key function #124

Add prompt argument to import RSA private key function #124

lukpueh commented Mar 8, 2018

awwad commented Mar 8, 2018 •

edited

Loading

awwad commented Mar 8, 2018

lukpueh commented Mar 8, 2018

lukpueh commented Mar 9, 2018

awwad commented Mar 13, 2018 •

edited

Loading

vladimir-v-diaz commented Mar 13, 2018

awwad left a comment

awwad Mar 13, 2018

awwad Mar 13, 2018

awwad Mar 13, 2018

awwad Mar 13, 2018

lukpueh Mar 16, 2018

vladimir-v-diaz Mar 16, 2018

awwad Mar 13, 2018

awwad Mar 13, 2018

awwad Mar 13, 2018

lukpueh Mar 16, 2018

awwad Mar 13, 2018

lukpueh commented Mar 16, 2018

vladimir-v-diaz commented Mar 16, 2018

awwad commented Apr 3, 2018 •

edited

Loading

vladimir-v-diaz commented Apr 3, 2018

Add prompt argument to import RSA private key function #124

Add prompt argument to import RSA private key function #124

Conversation

lukpueh commented Mar 8, 2018

awwad commented Mar 8, 2018 • edited Loading

awwad commented Mar 8, 2018

lukpueh commented Mar 8, 2018

lukpueh commented Mar 9, 2018

awwad commented Mar 13, 2018 • edited Loading

vladimir-v-diaz commented Mar 13, 2018

awwad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Mar 16, 2018

vladimir-v-diaz commented Mar 16, 2018

awwad commented Apr 3, 2018 • edited Loading

vladimir-v-diaz commented Apr 3, 2018

awwad commented Mar 8, 2018 •

edited

Loading

awwad commented Mar 13, 2018 •

edited

Loading

awwad commented Apr 3, 2018 •

edited

Loading