Consolidate key interface password handling and general overhaul #288

lukpueh · 2020-10-16T13:21:14Z

Fixes: #122
Follows up on: #124 (see for interface discussion) and #148
Partially fixes in-toto/in-toto#80

Description of the changes being introduced by the pull request:

Adds and adopts helper functions to aggregate repetitive code that handles encryption- (in key pair generation), and decryption- (in private key import) passwords, in order to make the behavior consistent for all key types.
Cleans up the code, comments and docstrings and use Google style formatting for the latter to nicely render on readthedocs. A demo is available at: https://in-toto-lukpueh.readthedocs.io/en/latest/api.html#utilities
Adds generic private key import interface functions, to import any of the supported private key types from file (rsa, ecdsa, ed25519).
Changes the behavior of generate_and_write_*_keypair and adds two additional functions (see below).

Behavior changes:

Adds support to optionally not encrypt (on generation) and decrypt (on import) ecdsa private keys to be consistent with rsa and ecdsa key interface functions.
~~Adds optional prompt kwarg to all key generation functions.~~
Adds optional prompt kwarg to ecdsa private key import function (rsa and ed25519 already had it).
Changes handling of None or "" passwords passed to key generation or private key import functions.
- MOST IMPORTANTLY: generation function no longer open a prompt if no password is passed as argument, i.e. keys are written to disk unencrypted by default. This may be revisited in the future (see Consolidate key interface password handling and general overhaul #288 (comment) for rationale/discussion).
- generate_and_write_*_keypair functions now require a positional password argument as first argument, which must be a non-empty string. The functions no longer open a prompt or write unencryted passwords. For that two new interface functions are added per key type, generate_and_write_*_keypair_with_prompt and generate_and_write_unencrypted_*_keypair. This follows the principles of secure defaults and least surprise.

Pain points:

Rsa functions use standardized PEM, and ed25519 and ecdsa functions use a custom json format for keys on disk, as a consequence the signing scheme for rsa keys must be assigned on import, and for ed25519 and ecdsa keys on generation.
Ed25519 and ecdsa key generation functions in the interface don't take a scheme parameter, as a consequence only the the default scheme can be used.
The newly added generic key import function does not take a scheme parameter and thus only the default signing scheme can be used. This is a deliberate decision in this PR due to the existing scheme assignment inconsistency described above.
The newly added generic key import function requires a key_type parameter to dispatch to the appropriate function. It would be nice if the key type could be assessed based on the on-disk format, but this does not seem feasible given the different formats as described above.
Minor: The used sphinx napoleon plugin to render docstrings as html does not handle the custom "Side Effects" section well (see demo above and Custom docstring section sphinx-contrib/napoleon#2).

Note to reviewer
I advise to review commit by commit, or at least to read the commit messages for details

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

Add helper for private key import interface functions to use the, passed decryption password, or to prompt for a password based on a passed 'prompt' boolean, or to treat the key as unencrypted. The helper aggregates and replaces repetitive code in 'interface.import_{rsa, ed25519, ecdsa}_privatekey_from_file' functions, to make the password handling consistent across these functions. This commit only adopts the helper for rsa and ed25519 import functions. The ecdsa function requires more invasive changes (see subsequent commit). **Change of behavior**: Passing an empty string for a password no longer raises a FormatError, instead it is left to the underlying decryption function to fail with something like a "wrong password error", because there shouldn't be a difference between a wrong and a wrong empty password. See test changes for change of behavior.

Add helper for key pair generation interface functions to use the passed encryption password, or to prompt for a password based on a passed 'prompt' boolean, or to not encrypt the key. This helper aggregates and replaces repetitive code in 'interface.generate_and_write_{rsa, ed25519, ecdsa}_keypair' functions, to make the password handling consistent accross these functions and with private key import functions. The commit adopts the helper for rsa and ed25519 generation functions, adding an additional optional 'prompt' parameter. The ecdsa function requires more invasive changes (see subsequent commit). **Change of behavior**: - Passing None as 'password' no longer opens a prompt, but just not encrypts the key. To open a prompt the new boolean kwarg 'prompt' must be used. - Passing an empty password no longer writes the key unencrypted but raises a ValueError instead. The goal of these changes is to require the caller to express the encryption desire more explicitly. Also see test changes for change of behavior.

Use recently added encryption (key generation) and decryption (key import) password helper for ecdsa interface functions for consistency with rsa and ed25519 interface functions. This also updates 'generate_and_write_ecdsa_keypair' and 'import_ecdsa_privatekey_from_file' to accept an additional optional 'prompt' parameter, and to also handle unencrypted keys.

Minor non-behavior changing clean-ups regarding vertical space and grouping, as well as code comments in interface functions. Also removes seemingly random logger statements.

- Change docstring format to Google Style better auto-docs rendering (http://secure-systems-lab/code-style-guidelines#20) - Document recent interface function changes (args, errors) - Thoroughly document which exceptions may be raised - Correct mistakes about used encryption. - Generally overhaul docstrings with the goal to make them more concise, but more helpful for a user too, e.g. by mentioning signing schemes.

Add 'prompt' argument and updates comments in sample code snippets.

Add convenience dispatcher for other private key import interface functions, to import any of the supported private key types from file (rsa, ecdsa, ed25519). This transfers a similar function, currently implemented in in-toto.util, in order to close in-toto/in-toto#80. Caveat: - The key type must be specified via argument (or defaults to RSA). In the future we might want to let the parser infer the key type, as we do in the related in-toto-golang implementation. See https://github.com/in-toto/in-toto-golang/blob/5fba7c22a062a30b6e373f33362d647eabf15822/in_toto/keylib.go#L281-L361 - Currently, the function does not support a signing scheme parameter and thus assigns the default value from import_rsa_privatekey_from_file to the returned key. For the other keep types, the scheme is encoded in the on-disk format. In the future we might want to consolidate this as part of secure-systems-lab#251. For now the primary goal is to have a simple interface that is enough to close in-toto/in-toto#80.

coveralls · 2020-10-16T13:23:55Z

Coverage decreased (-0.08%) to 98.785% when pulling 9a74f30 on lukpueh:misc-interface-enhancements into 3bb4625 on secure-systems-lab:master.

lukpueh · 2020-10-20T10:28:37Z

Minor: The used sphinx napoleon plugin to render docstrings as html does not handle the custom "Side Effects" section well

Just ticketized this in in-toto/in-toto#401, because we currently only generate sphinx docs in in-toto. But the plan is to fix it upstream, so that we can use the custom section in TUF/in-toto/sslib.

joshuagl

This is a lovely set of cleanups, thanks a lot @lukpueh! That we have an overall LOC reduction when adding a new function and tests is incredible. I am very pleased with the new docstring style. It uses much less vertical space. I might even be able to read the method being documented at the same time as the docstring. 😆

My only slight concern is that this PR introduces several behavioural changes to the interface module which arguably break backwards compatibility. I don't think we've made any API stability promises as of yet, but I wanted to at least raise the issue for discussion here.

joshuagl · 2020-10-20T10:02:42Z

securesystemslib/interface.py

  ed25519_key_metadata = securesystemslib.util.load_json_file(filepath)
-  ed25519_key, junk = \
-    securesystemslib.keys.format_metadata_to_key(ed25519_key_metadata)
+  ed25519_key, junk = securesystemslib.keys.format_metadata_to_key(


Nit: I dislike the use of the junk variable name here. I think it's confusing when the accepted Python idiom is to use an _ variable name for unused return values. By having used a name, I would expect to see the variable used somewhere.

Agreed. Will remove.

- Add common CLI argument for 'in-toto-run' and 'in-toto-record' to optionally pass a password ('-P <password>') or open a prompt ('-P') for a password to decrypt a signing key passed with '--key'. - Add '--prompt' CLI argument for 'in-toto-sign' to optionally open a prompt for a password to decrypt signing keys passed with '--key'. Note: Since 'in-toto-sign' allows passing multiple keys at once, we only support the prompt option and not a '--password' option like above, also in order to keep the mostly internally used tool simple. Prior to this commit, the CLI tools always tried to first load the key as if it were unencrypted and, on error, opened a password prompt, assuming (but not knowing!) that the error was due to the key being encrypted, which is not ideal (see in-toto#80, and "explicit is better"). This commit makes the password handling more explicit and was motivated by a recent change of the securesystemslib key interface that gives us better control over the key decryption password. (secure-systems-lab/securesystemslib#288) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

jku

Looks good, the cleanups make the code much more readable.

jku · 2020-10-24T10:32:51Z

securesystemslib/interface.py

+  decrypted, otherwise it is treated as unencrypted.
+
+  NOTE: The default signing scheme 'rsassa-pss-sha256' is assigned to RSA keys.
+  Use 'import_rsa_publickey_from_file' to specify any other than the default


Suggested change

Use 'import_rsa_publickey_from_file' to specify any other than the default

Use 'import_rsa_privatekey_from_file' to specify any other than the default

Good eyes! Thanks for catching. :)

jku · 2020-10-24T11:02:06Z

tests/test_interface.py

+        ({"password": ""},
+          "encryption password must be 1 or more characters long"),
+        # Error on 'password' and 'prompt=True'
+        ({"password": pw, "prompt": True},
+          "passing 'password' and 'prompt=True' is not allowed")]):


I imagine encoding the error messages in multiple tests can make later improvements annoying. Is it really useful?

(I see this is the style already -- so this is just a question, not a suggestion to modify this commit)

I agree that it's annoying and prone to break. As a matter of fact I'm pretty sure that it will break in the next build... I just skimmed through the latest cryptography release and saw that they changed an error message that we check for here.

That said, I think there is merit in more granular error test assertions that go beyond the type. I've seen tests in the past, which meant to check for one error and passed although a different error was raised, because they had the same type.

So I think it's a fair trade-off to update the expected error strings in tests every now and then, if it gives us stronger tests. And once we have a more solid exception taxonomy (see #191), I'm happy to remove the message assertions. What do you think?

FYI: Looks like I was wrong about the cryptography update breaking our build. That particular error message is raised by ourselves.

securesystemslib/securesystemslib/rsa_keys.py

Line 1062 in d05dc6a

raise securesystemslib.exceptions.CryptoError('Decryption failed.')

So no need for action for now... :)

jku · 2020-10-24T12:05:39Z

securesystemslib/interface.py

+def generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,
+    password=None, prompt=False):


general comment on generate_*() functions: If I've understood correctly calling them with default values now leads to creating unencrypted keys. It's not horrible but I think the expected default would be prompting...

Instead of prompt these functions could maybe have encrypted=True? By default this would lead to a prompt if password was not given, but you could set encrypted=False to create unencrypted keys.

Good points, @jku!

We discussed using an encrypt instead of a prompt parameter in #124 (comment) pp. but opted for prompt to be more explicit about whether user interaction is required. However, I do see how one could equally well argue for being more explicit about encryption.

Regarding default, I agree that from a security point of view prompt=True makes more sense (i.e. secure by default). From a usability point of view I don't like that an API function opens a prompt by default. But the former is probably more important. What do others think? (cc @joshuagl, @mnm678, @adityasaky, @SantiagoTorres, @trishankatdatadog).

I don't like that an API function opens a prompt by default

Fair point, understood.

I guess you could decide that user has to choose something: leaving both password and prompt unset would be an error. Then calling the function with default argument values would fail with ValueError instead of prompting unexpectedly.

To create unencrypted keys you would have to generate_and_write_rsa_keypair(filepath, prompt=False): it's not ideal because nothing in that explicitly shows that the result is going to be unencrypted... so it's still not great.

I guess a combo is possible as well: user needs to choose exactly one of password=<value>, prompt=True or encrypted=False -- then each call would be explicit about what it wants. In another language this would be a SecurityType enum with three possible choices but that seems a bit clunky in Python

Yes, we should encrypt by default, no question about it, and principle of least surprise. Could the API be designed better? Sure, but let's leave that for the future.

I think we should only create unencrypted keys if the user explicitly sets this, for example encrypted=False, or at the very least we can set prompt=True by default.

Yes, you understood my ideas as I meant them. I agree with your comments (raising an exception as default is not ideal, enum/class style works but doesn't fit API well)

@trishankatdatadog and @joshuagl, you both approved the PR (with no default encryption) but also signaled that you would like to see default encryption. I am unsure how to proceed.

My preferred course of action would be to merge as is, i.e. without default encryption, for the the following (partially interdependent) reasons:

I'd like to land this asap, because it blocks in-toto 1.0.0.

I have already adopted the changes from this PR on in-toto (Remove util module and use securesystemslib key interfaces instead in-toto/in-toto#402) and tuf (Adopt sslib keygen interface encryption changes theupdateframework/python-tuf#1191) sides.

I am against default prompts (see UX).

I am against more sophisticated parameters (see UX/consistency and asap-argument).

We can reconsider this for securesystemslib v1.0.0. Who knows, we might even remove key generation functionality and limit securesystemslib to parsing standard, out-of-band generated key formats, translating them into tuf/in-toto formats, and using them to sign/verify.

I agree that default encryption is desirable but does not outweigh all these arguments

Can I get 👍 /👎 if others are (not) okay with this?

I'm OK with merging as-is and planning to improve in the future by either: 1) removing key generation functionality 2) implementing encryption by default.

Should we file an issue in the 1.0 milestone to capture the discussion here?

I wanted to add some comments I shared privately. It would worry me that an unintended side-effect is for the function to block (waiting for stdin), as I assume this would catch many people by surprise, may be difficult to debug (e.g., a CI build hanging, or tox randomy hanging with no stdout/in redirected anywhere).

I'm not entirely sure what's the user story we have in mind, but for a library that is generating keys in a program, it also feels like this is a feature that many people may not want. This is reminiscent of that time we added colorama, and all the consequences that ensured (i.e., very little benefit, and a lot of maintenance burden).

Although I agree that secure by default is a thing, I also wonder if this is adding meaningful security for most of the usecases. I'd expect the following:

The API function to be noninteractive (hell, we don't even know if this is going to be attached to a terminal)

A CLI tool that uses this function to set up a prompt (or use this batteries-included setting).

I don't think at the time of writing this function we have enough context to know how users will use this regularly, so I err on the side of non-interactivity by default.

@trishankatdatadog and @joshuagl, you both approved the PR (with no default encryption) but also signaled that you would like to see default encryption. I am unsure how to proceed.

My bad. I would rather that we encrypt keys on disk rather than our users shoot themselves in the foot. Having said that, TUF/in-toto can take care of that rather than SSLlib. Does this answer your question?

lukpueh · 2020-10-26T09:54:02Z

My only slight concern is that this PR introduces several behavioural changes to the interface module which arguably break backwards compatibility. I don't think we've made any API stability promises as of yet, but I wanted to at least raise the issue for discussion here.

@joshuagl, I'm not so worried about that. securesystemslib is mostly for internal (TUF/in-toto) use and we are still in pre-1.X.X-phase. What's IMO more important is that we get the interface "right" (see comment/discussion about defaults above).

secure-systems-lab/securesystemslib#288 changes the key generation interface functions to no longer auto-prompt for an encryption password if no password is passed, in order to not suprise the caller with a blocking prompt. The downside of this change is that the keys are stored in plain text per default, which may be mitigated by recommending encryption in the docs. This commit updates related TUF documentation, which always passes an encryption password or shows a prompt. NOTE: The securesystemslib private key import functions do not auto-prompt for decryption passwords either, however TUF only exposes custom wrappers (see repository_lib) that do auto-prompt. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

SantiagoTorres

I have a small comment, yet I don't know if it's a blocker (I may have well missed the original rationale).

SantiagoTorres · 2020-10-28T16:07:47Z

securesystemslib/interface.py

-      scheme, password)
+  # Optionally decrypt and convert PEM-encoded key to 'RSAKEY_SCHEMA' format
+  rsa_key = securesystemslib.keys.import_rsakey_from_private_pem(
+      pem_key, scheme, password)


I have a small gripe with the way we set up these two functions. It feels to me that the pattern I most of the time do is:

generate a key use the key right away

It is rather confusing to me that the filepath is returned, rather than the generated key itself, specially given that there is no default filepath (so we don't have to provide users any disambiguation). So with this in mind, what's the added value of returning the filename rather than the key itself?

Agreed. This was just not a focus of this PR.

Also, it is already possible to do that one level deeper in the abstraction hierarchy (see securesystemslib.keys.generate_*_key).

Fair, I still wonder if we'd like to change the return value for the key instead of the filepath?

trishankatdatadog

I would rather that we encrypt keys on disk rather than our users shoot themselves in the foot. Having said that, TUF/in-toto can take care of that rather than SSLlib. Does this work?

lukpueh · 2020-10-29T12:04:02Z

Having said that, TUF/in-toto can take care of that rather than SSLlib. Does this work?

I'd rather try to get this right right away, because I want to expose this interface directly to in-toto users and not create an additional layer that I need to then maintain in in-toto (and TUF).

lukpueh · 2020-10-29T12:08:21Z

Let me explore alternatives...

trishankatdatadog · 2020-10-29T13:58:34Z

Let me explore alternatives...

What about theupdateframework/python-tuf#1191 (comment)?

SantiagoTorres · 2020-10-29T15:16:09Z

I would rather that we encrypt keys on disk rather than our users shoot themselves in the foot. Having said that, TUF/in-toto can take care of that rather than SSLlib. Does this work?

I think that'd be ideal in my opinion. In the same way the openssl library doesn't encrypt by default on keygen, but the openssl binary requires you to pass -nopasswd (or passwd '' I forget) to avoid encryption.

lukpueh · 2020-10-29T15:31:08Z

Let me recap...

Original behavior:

Before this PR the interface.generate_and_write_*_keypair had only an optional password argument to control encryption (and no prompt argument) and behaved like so:

password=None (default): prompt for password
- if empty: no encryption
- else: encryption
password="<non-empty-string>": encryption
password="": no prompt and no encryption

(NOTE: generate_and_write_ecdsa_keypair behaved slightly different, i.e. it would encrypt anyway, even with an empty passed or entered password)

New behavior:

All interface.generate_and_write_*_keypair now have an optional prompt argument and consistently behave like so:

password=None, prompt=False (default): no encryption
password="<non-empty-string>", prompt=False: encryption
password="<any-string>", prompt=True: error
password="", prompt=False: error
password=None, prompt=True: prompt for password
- if empty: no encryption
- else: encryption

Motivation:

My main motivation was to make encryption control and prompt behavior more explicit and surprise-free, so that I could remove the in_toto.util-facade, which had thin wrappers around generate functions, i.e.

in_toto.util.generate_*_keypair(.., password=""): no auto-prompt, but also no encryption by default
in_toto.util.prompt_generate_*_keypair(): always prompts for a password

I admit that I did not prioritize the security by default principle.

I also tried to withstand the temptation to address other related key interface issues, like

module architecture / choice of abstraction layers (see Revise module architecture and define clear API modules and functions #270 and Consolidate key interface password handling and general overhaul #288 (comment))
non-standard on-disk key formats for ed25519, ecdsa and other key format issues ... (see key format inconsistency #251)

Alternatives to promote default encryption

Revert to original behavior described above
Cons:
- Non-surprise-free encryption control
- default prompt if no password is passed
Set default prompt=True
Cons:
- Default prompt if no password is passed
- Unclear behavior if password is passed (error, or prioritize one of prompt=True or password?)
Set default prompt=None and require either a password or explicitly setting prompt to True or False
Cons:
- Default error
- Three cases for a boolean argument (None, True, False)
Use a complex mandatory argument, e.g. encryption_method, that allows to pass exactly one of password, prompt=True, or no-encrypt (akin to what pyca/securesystemslib does)
Cons:
- Unexpected API style give the reset of securesystemslib
Provide different functions to create unencrypted and encrypted keys (@trishankatdatadog, would you mind describing the exact behavior you have in mind?)
Cons:
- Might require more complicated case handling in calling code
Remove key generation functionality
I think this is what we should aim for in the long run, but it requires us to support common standard on-disk formats for all our key types.

joshuagl

Apart from some over-eager find/replace the additional changes look reasonable to me.

I'd like to hear from others who contributed to the discussion before we merge them, though. cc @jku @trishankatdatadog @SantiagoTorres @mnm678

tests/test_interface.py

lukpueh · 2020-11-05T11:29:14Z

Many thanks for the quick review, @joshuagl!

Apart from some over-eager find/replace the additional changes look reasonable to me.

I added these on purpose because test_{rsa, ed25519, ecdsa} do only test the _generate_* and not the generate_* functions. The latter are tested in the newly added test_generate_keypair_wrappers function.

lukpueh · 2020-11-05T11:42:30Z

I'd like to hear from others who contributed to the discussion before we merge them, though. cc @jku @trishankatdatadog @SantiagoTorres @mnm678

So would I!

FYI I implemented @jku's approach as sketched in #288 (comment).

See the most recent commits for these changes, and https://in-toto-lukpueh.readthedocs.io/en/latest/api.html#generate-key-pairs, for what the docs look like on RTD.

lukpueh · 2020-11-05T11:43:17Z

If we decide to merge this, I'll update the PR description.

PR in-toto#402 adopted key interface changes from the pending secure-systems-lab/securesystemslib#288 PR and was merged prematurely. The sslib PR now has further evolved, in order to follow the principle of secure defaults in regards to private key encryption, which requires the following adoptions in in-toto: - The not secure by default generate_and_write_*_keypair function is now protected (_generate_and_write_*_keypair), and only used for the keygen cli utility, where it is really convenient. - In other cases we use either generate_and_write_*_keypair (for encrypted keys only) or generate_and_write_unencrypted_*_keypair. Furthermore, the newly added sslib key generation interface functions are added to the in-toto API docs. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

The _get_key_file_encryption_password helper needs to be called after the passed or keyid-based filepath has been determined, i.e. after key creation in the latter case, because it might be displayed on the password prompt. Plus remove obsolete quotes.

jku

LGTM 👍

secure-systems-lab/securesystemslib#288 changes the key generation interface functions in such a way that it is clear if a call opens a blocking prompt, or writes the key unencrypted. To do this two functions are added per key type: - `generate_and_write_*_keypair_with_prompt` - `generate_and_write_unencrypted_*_keypair` The default generate_and_write_*_keypair function now only allows encrypted keys and only using a passed password. This respects the principle of secure defaults and least surprise. sslib#288 also adds a protected _generate_and_write_*_keypair function per keytype which may be used NOTE: The securesystemslib private key import functions do not auto-prompt for decryption passwords either, TUF, however, only exposes custom wrappers (see repository_lib) that do auto-prompt. The sslib#288 does change prompt texts for encryption and also decryption keys, which is reflected in this commit. TODO: - Adopt changes in TUTORIAL.md in test_tutorial.py - Proof read ('repo.py' in particular) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

trishankatdatadog

Looks great, fantastic job, thanks for seeing this through, Lukas! Just a few comments...

trishankatdatadog · 2020-11-05T19:38:15Z

tests/check_public_interfaces.py

@@ -64,7 +68,26 @@ def test_interface(self):

    with self.assertRaises(
          securesystemslib.exceptions.UnsupportedLibraryError):


Sorry, I'm confused... why are we expecting to see UnsupportedLibraryError in this series of tests?

To check that all interface functions return the expected Exception, if the optional dependencies pyca/cryptography, pyca/pynacl and gpg are not available. See tox.ini and travis.yml how these tests are called with required dependencies only.

trishankatdatadog · 2020-11-05T19:55:48Z

securesystemslib/interface.py

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


So we allow empty string decryption passwords here?

securesystemslib/interface.py

trishankatdatadog · 2020-11-05T19:58:30Z

securesystemslib/interface.py

+        the key is written to CWD using the keyid as filename. The public key
+        is written to the same path as the private key using the suffix '.pub'.
+    bits (optional): The number of bits of the generated RSA key.
+    password (optional): An encryption password.


Suggested change

password (optional): An encryption password.

password (optional): An encryption password, which may not be an empty string.

My goal was to remove most redundancy in the docstrings to keep them as dense as possible. The information you suggest is already available in the Raises section, i.e. ValueError: An empty string is passed as 'password'...

trishankatdatadog · 2020-11-05T20:02:13Z

securesystemslib/interface.py

-  # Encrypt the private key if 'password' is set.
-  if len(password):
+  # Encrypt the private key if a 'password' was passed or entered on the prompt
+  if password is not None:


Maybe we can do the password schema check here instead of forcing it on the direct/indirect caller?

We check in the _get_key_file_encryption_password helper, which we call right before.

trishankatdatadog · 2020-11-05T20:03:32Z

securesystemslib/interface.py

  """
-  <Purpose>
-    Import the PEM file in 'filepath' containing the private key.
+  securesystemslib.formats.PASSWORD_SCHEMA.check_match(password)


See my comment above about moving the password schema check to _get_key_file_encryption_password().

That's already the case. However, it only checks if the password is not None. That's because _generate_and_write_*_keypair (which uses _get_key_file_encryption_password) accept None for a password so that callers can indicate the desire to not encrypt. (see docstrings)

generate_and_write_*_keypair OTOH do not accept None as password, that's why we unconditionally check the schema here right away.

trishankatdatadog · 2020-11-05T20:06:14Z

securesystemslib/interface.py

+  securesystemslib.formats.PATH_SCHEMA.check_match(filepath)
+  securesystemslib.formats.RSA_SCHEME_SCHEMA.check_match(scheme)
+
+  password = _get_key_file_decryption_password(password, prompt, filepath)

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


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

Good idea! Mind submitting a feature request?

trishankatdatadog · 2020-11-05T20:06:40Z

securesystemslib/interface.py

+  securesystemslib.formats.PATH_SCHEMA.check_match(filepath)
+  securesystemslib.formats.RSA_SCHEME_SCHEMA.check_match(scheme)
+
+  password = _get_key_file_decryption_password(password, prompt, filepath)


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

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

trishankatdatadog · 2020-11-05T20:30:02Z

tests/test_interface.py


    # TEST: Generate default keys and import
    # Assert location and format
    fn_default = "default"
-    fn_default_ret = generate_and_write_rsa_keypair(
-        filepath=fn_default, password="")
+    fn_default_ret = _generate_and_write_rsa_keypair(filepath=fn_default)


Unencrypted, correct?

But note that this is the protected internal _generate_and_write_rsa_keypair (see _-prefix) function for flexible but hazardous use. The public generate_and_write_rsa_keypair does not accept an empty password, and thus can not write unencrypted keys.

Clarify how 'password' and 'prompt' arguments affect encryption and decryption of private keys. Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>

lukpueh · 2020-11-06T09:55:57Z

Thanks for the comments, @trishankatdatadog. I integrated one of your suggestions and tried clarifying your other questions, both here inline and also by updating the helper function docstrings.

secure-systems-lab/securesystemslib#288 changes the key generation interface functions in such a way that it is clear if a call opens a blocking prompt, or writes the key unencrypted. To do this two functions are added per key type: - `generate_and_write_*_keypair_with_prompt` - `generate_and_write_unencrypted_*_keypair` The default `generate_and_write_*_keypair` function now only allows encrypted keys and only using a passed password. This respects the principle of secure defaults and least surprise. sslib#288 furthermore adds a protected `_generate_and_write_*_keypair`, which is not exposed publicly because it does not encrypt by default, but is more flexible and thus convenient e.g. to consume all arguments from a key generation command line tool such as 'repo.py'. This commit adds the new public functions to the tuf namespace and adopts their usage accordingly. NOTE regarding repo.py: This commit does not fix any problematic password behavior of 'repo.py' like default passwords, etc. (see theupdateframework#881). It only adopts the sslib#288 changes to maintain the current behvior, plus removing one glaringly obsolete password prompt. NOTE regarding key import: The securesystemslib private key import functions were also changed to no longer auto-prompt for decryption passwords , TUF, however, only exposes custom wrappers (see repository_lib) that do auto-prompt. sslib#288 changes to the prompt texts are nevertheless propagated to tuf and reflected in this commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh · 2020-11-06T12:20:13Z

Thanks for all the reviews and comments! The required adoptions for tuf and in-toto are pending in theupdateframework/python-tuf#1191 and in-toto/in-toto#408. And I updated the PR description here to reflect the recent changes. Merging ...

SantiagoTorres · 2020-11-06T16:14:48Z

Hi, this seems like it's been approved enough and it also LGTM. I'm going to merge it.

lukpueh · 2020-11-09T12:52:08Z

Thanks for merging, @SantiagoTorres! Just realized that I forgot to update the snippets in README.rst a second time after the most recent commits. 🤦 Will follow up with a fix.

secure-systems-lab/securesystemslib#288 changes the key generation interface functions in such a way that it is clear if a call opens a blocking prompt, or writes the key unencrypted. To do this two functions are added per key type: - `generate_and_write_*_keypair_with_prompt` - `generate_and_write_unencrypted_*_keypair` The default `generate_and_write_*_keypair` function now only allows encrypted keys and only using a passed password. This respects the principle of secure defaults and least surprise. sslib#288 furthermore adds a protected `_generate_and_write_*_keypair`, which is not exposed publicly because it does not encrypt by default, but is more flexible and thus convenient e.g. to consume all arguments from a key generation command line tool such as 'repo.py'. This commit adds the new public functions to the tuf namespace and adopts their usage accordingly. NOTE regarding repo.py: This commit does not fix any problematic password behavior of 'repo.py' like default passwords, etc. (see theupdateframework#881). It only adopts the sslib#288 changes to maintain the current behvior, plus removing one glaringly obsolete password prompt. NOTE regarding key import: The securesystemslib private key import functions were also changed to no longer auto-prompt for decryption passwords , TUF, however, only exposes custom wrappers (see repository_lib) that do auto-prompt. sslib#288 changes to the prompt texts are nevertheless propagated to tuf and reflected in this commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Adopt the following changes: - generate_and_write_*_keypair no longer auto-prompts for a password generate_and_write_*_keypair_with_prompt should be used to present a prompt. - import_*_privatekey_from_file has a new prompt message.

Adopt interface changes (#288) in README snippets

PR in-toto#402 adopted key interface changes from the pending secure-systems-lab/securesystemslib#288 PR and was merged prematurely. The sslib PR now has further evolved, in order to follow the principle of secure defaults in regards to private key encryption, which requires the following adoptions in in-toto: - The not secure by default generate_and_write_*_keypair function is now protected (_generate_and_write_*_keypair), and only used for the keygen cli utility, where it is really convenient. - In other cases we use either generate_and_write_*_keypair (for encrypted keys only) or generate_and_write_unencrypted_*_keypair. Furthermore, the newly added sslib key generation interface functions are added to the in-toto API docs. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

secure-systems-lab/securesystemslib#288 changes the key generation interface functions in such a way that it is clear if a call opens a blocking prompt, or writes the key unencrypted. To do this two functions are added per key type: - `generate_and_write_*_keypair_with_prompt` - `generate_and_write_unencrypted_*_keypair` The default `generate_and_write_*_keypair` function now only allows encrypted keys and only using a passed password. This respects the principle of secure defaults and least surprise. sslib#288 furthermore adds a protected `_generate_and_write_*_keypair`, which is not exposed publicly because it does not encrypt by default, but is more flexible and thus convenient e.g. to consume all arguments from a key generation command line tool such as 'repo.py'. This commit adds the new public functions to the tuf namespace and adopts their usage accordingly. NOTE regarding repo.py: This commit does not fix any problematic password behavior of 'repo.py' like default passwords, etc. (see theupdateframework#881). It only adopts the sslib#288 changes to maintain the current behvior, plus removing one glaringly obsolete password prompt. NOTE regarding key import: The securesystemslib private key import functions were also changed to no longer auto-prompt for decryption passwords , TUF, however, only exposes custom wrappers (see repository_lib) that do auto-prompt. sslib#288 changes to the prompt texts are nevertheless propagated to tuf and reflected in this commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh added 7 commits October 16, 2020 13:17

Clean up interface function bodies

7d00ecb

Minor non-behavior changing clean-ups regarding vertical space and grouping, as well as code comments in interface functions. Also removes seemingly random logger statements.

Adopt recent interface changes in README.rst

7d37d55

Add 'prompt' argument and updates comments in sample code snippets.

lukpueh requested review from joshuagl and SantiagoTorres October 16, 2020 14:06

joshuagl approved these changes Oct 20, 2020

View reviewed changes

lukpueh mentioned this pull request Oct 21, 2020

Remove util module and use securesystemslib key interfaces instead in-toto/in-toto#402

Merged

3 tasks

trishankatdatadog approved these changes Oct 23, 2020

View reviewed changes

jku reviewed Oct 24, 2020

View reviewed changes

lukpueh mentioned this pull request Oct 26, 2020

Revise exception taxonomy #191

Closed

Fix docstring and rename junk vars in interface

1db7f21

This was referenced Oct 28, 2020

Adopt sslib keygen interface encryption changes theupdateframework/python-tuf#1191

Merged

Bump securesystemslib dependency to 0.18.0 in-toto/in-toto#406

Merged

Update TUF, in-toto and securesystemslib DataDog/integrations-core#7844

Merged

SantiagoTorres approved these changes Oct 28, 2020

View reviewed changes

trishankatdatadog suggested changes Oct 29, 2020

View reviewed changes

joshuagl reviewed Nov 5, 2020

View reviewed changes

tests/test_interface.py Show resolved Hide resolved

tests/test_interface.py Show resolved Hide resolved

tests/test_interface.py Show resolved Hide resolved

lukpueh mentioned this pull request Nov 5, 2020

Adopt recent sslib key interface changes in-toto/in-toto#408

Merged

3 tasks

lukpueh force-pushed the misc-interface-enhancements branch from 2c32b1f to 1cb5bcd Compare November 5, 2020 12:51

jku approved these changes Nov 5, 2020

View reviewed changes

trishankatdatadog reviewed Nov 5, 2020

View reviewed changes

Refine interface helper docstrings

9a74f30

Clarify how 'password' and 'prompt' arguments affect encryption and decryption of private keys. Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>

lukpueh force-pushed the misc-interface-enhancements branch from 6064d2d to 9a74f30 Compare November 6, 2020 09:54

trishankatdatadog approved these changes Nov 6, 2020

View reviewed changes

SantiagoTorres merged commit fee1cc8 into secure-systems-lab:master Nov 6, 2020

lukpueh mentioned this pull request Nov 9, 2020

Prepare 0.18.0 release #301

Merged

lukpueh added a commit that referenced this pull request Nov 9, 2020

Merge pull request #300 from lukpueh/adopt-288-in-readme

deb50ad

Adopt interface changes (#288) in README snippets

lukpueh mentioned this pull request Feb 15, 2021

Add the Signer interface #319

Merged

3 tasks

	Use 'import_rsa_publickey_from_file' to specify any other than the default
	Use 'import_rsa_privatekey_from_file' to specify any other than the default

		def generate_and_write_rsa_keypair(filepath=None, bits=DEFAULT_RSA_KEY_BITS,
		password=None, prompt=False):

		@@ -64,7 +68,26 @@ def test_interface(self):

		with self.assertRaises(
		securesystemslib.exceptions.UnsupportedLibraryError):

	password (optional): An encryption password.
	password (optional): An encryption password, which may not be an empty string.

Consolidate key interface password handling and general overhaul #288

Consolidate key interface password handling and general overhaul #288

Conversation

lukpueh commented Oct 16, 2020 • edited Loading

Description of the changes being introduced by the pull request:

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

coveralls commented Oct 16, 2020 • edited Loading

lukpueh commented Oct 20, 2020

joshuagl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku 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

jku Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

lukpueh Oct 28, 2020 • edited Loading

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 Oct 26, 2020

SantiagoTorres 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

trishankatdatadog left a comment

Choose a reason for hiding this comment

lukpueh commented Oct 29, 2020

lukpueh commented Oct 29, 2020

trishankatdatadog commented Oct 29, 2020

SantiagoTorres commented Oct 29, 2020

lukpueh commented Oct 29, 2020 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

lukpueh commented Nov 5, 2020

lukpueh commented Nov 5, 2020 • edited Loading

lukpueh commented Nov 5, 2020

jku left a comment

Choose a reason for hiding this comment

trishankatdatadog 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

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 Nov 6, 2020

lukpueh commented Nov 6, 2020

SantiagoTorres commented Nov 6, 2020

lukpueh commented Nov 9, 2020

lukpueh commented Oct 16, 2020 •

edited

Loading

coveralls commented Oct 16, 2020 •

edited

Loading

jku Oct 26, 2020 •

edited

Loading

jku Oct 28, 2020 •

edited

Loading

lukpueh Oct 28, 2020 •

edited

Loading

lukpueh commented Oct 29, 2020 •

edited

Loading

lukpueh commented Nov 5, 2020 •

edited

Loading