Add -P/--password (and prompt) cli argument

- 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 <[email protected]>
lukpueh · Oct 20, 2020 · de1b2f2 · de1b2f2
1 parent 51e626f
commit de1b2f2
Show file tree

Hide file tree

Showing 7 changed files with 184 additions and 49 deletions.
diff --git a/in_toto/common_args.py b/in_toto/common_args.py
@@ -85,6 +85,28 @@
            rsa=util.KEY_TYPE_RSA, ed25519=util.KEY_TYPE_ED25519))
 }
 
+KEY_PASSWORD_ARGS = ["-P", "--password"]
+KEY_PASSWORD_KWARGS = {
+  "nargs": "?",
+  "const": True,
+  "metavar": "<password>",
+  "help": ("password for encrypted key specified with '--key'. Passing  '-P'"
+           " without <password> opens a prompt. If no password is passed, or"
+           " entered on the prompt, the key is treated as unencrypted. (Do "
+           " not confuse with '-p/--products'!)")
+}
+def parse_password_and_prompt_args(args):
+  """Parse -P/--password optional arg (nargs=?, const=True). """
+   # --P was provided without argument (True)
+  if args.password is True:
+    password = None
+    prompt = True
+  # --P was not provided (None), or provided with argument (<password>)
+  else:
+    password = args.password
+    prompt = False
+
+  return password, prompt
 
 GPG_ARGS = ["-g", "--gpg"]
 GPG_KWARGS = {

diff --git a/in_toto/in_toto_record.py b/in_toto/in_toto_record.py
@@ -25,7 +25,6 @@
 import sys
 import argparse
 import logging
-import in_toto.util
 import in_toto.user_settings
 import in_toto.runlib
 from in_toto import __version__
@@ -35,8 +34,11 @@
     KEY_KWARGS, KEY_TYPE_KWARGS, KEY_TYPE_ARGS, GPG_ARGS, GPG_KWARGS,
     GPG_HOME_ARGS, GPG_HOME_KWARGS, VERBOSE_ARGS, VERBOSE_KWARGS, QUIET_ARGS,
     QUIET_KWARGS, METADATA_DIRECTORY_ARGS, METADATA_DIRECTORY_KWARGS,
+    KEY_PASSWORD_ARGS, KEY_PASSWORD_KWARGS, parse_password_and_prompt_args,
     sort_action_groups, title_case_action_groups)
 
+from securesystemslib import interface
+
 
 # Command line interfaces should use in_toto base logger (c.f. in_toto.log)
 LOG = logging.getLogger("in_toto")
@@ -98,6 +100,7 @@ def create_parser():
 
   parent_named_args.add_argument(*KEY_ARGS, **KEY_KWARGS)
   parent_parser.add_argument(*KEY_TYPE_ARGS, **KEY_TYPE_KWARGS)
+  parent_parser.add_argument(*KEY_PASSWORD_ARGS, **KEY_PASSWORD_KWARGS)
 
   parent_named_args.add_argument(*GPG_ARGS, **GPG_KWARGS)
   parent_parser.add_argument(*GPG_HOME_ARGS, **GPG_HOME_KWARGS)
@@ -170,6 +173,8 @@ def main():
     parser.print_usage()
     parser.error("Specify either '--key <key path>' or '--gpg [<keyid>]'")
 
+  password, prompt = parse_password_and_prompt_args(args)
+
   # If `--gpg` was set without argument it has the value `True` and
   # we will try to sign with the default key
   gpg_use_default = (args.gpg is True)
@@ -184,7 +189,8 @@ def main():
     # case the key is encrypted. Something that should not happen in the lib.
     key = None
     if args.key:
-      key = in_toto.util.import_private_key_from_file(args.key, args.key_type)
+      key = interface.import_privatekey_from_file(
+          args.key, key_type=args.key_type, password=password, prompt=prompt)
 
     if args.command == "start":
       in_toto.runlib.in_toto_record_start(args.step_name, args.materials,

diff --git a/in_toto/in_toto_run.py b/in_toto/in_toto_run.py
@@ -26,15 +26,18 @@
 import argparse
 import logging
 import in_toto.user_settings
-from in_toto import (util, runlib, __version__)
+from in_toto import (runlib, __version__)
 
 from in_toto.common_args import (EXCLUDE_ARGS, EXCLUDE_KWARGS, BASE_PATH_ARGS,
     BASE_PATH_KWARGS, LSTRIP_PATHS_ARGS, LSTRIP_PATHS_KWARGS, KEY_ARGS,
     KEY_KWARGS, KEY_TYPE_KWARGS, KEY_TYPE_ARGS, GPG_ARGS, GPG_KWARGS,
     GPG_HOME_ARGS, GPG_HOME_KWARGS, VERBOSE_ARGS, VERBOSE_KWARGS, QUIET_ARGS,
     QUIET_KWARGS, METADATA_DIRECTORY_ARGS, METADATA_DIRECTORY_KWARGS,
+    KEY_PASSWORD_ARGS, KEY_PASSWORD_KWARGS, parse_password_and_prompt_args,
     sort_action_groups, title_case_action_groups)
 
+from securesystemslib import interface
+
 # Command line interfaces should use in_toto base logger (c.f. in_toto.log)
 LOG = logging.getLogger("in_toto")
 
@@ -135,6 +138,7 @@ def create_parser():
 
   named_args.add_argument(*KEY_ARGS, **KEY_KWARGS)
   parser.add_argument(*KEY_TYPE_ARGS, **KEY_TYPE_KWARGS)
+  parser.add_argument(*KEY_PASSWORD_ARGS, **KEY_PASSWORD_KWARGS)
 
   named_args.add_argument(*GPG_ARGS, **GPG_KWARGS)
   parser.add_argument(*GPG_HOME_ARGS, **GPG_HOME_KWARGS)
@@ -181,6 +185,8 @@ def main():
     parser.print_usage()
     parser.error("Specify either '--key <key path>' or '--gpg [<keyid>]'")
 
+  password, prompt = parse_password_and_prompt_args(args)
+
   # If `--gpg` was set without argument it has the value `True` and
   # we will try to sign with the default key
   gpg_use_default = (args.gpg is True)
@@ -204,7 +210,8 @@ def main():
     # case the key is encrypted. Something that should not happen in the lib.
     key = None
     if args.key:
-      key = util.import_private_key_from_file(args.key, args.key_type)
+      key = interface.import_privatekey_from_file(
+          args.key, key_type=args.key_type, password=password, prompt=prompt)
 
     runlib.in_toto_run(
         args.step_name, args.materials, args.products, args.link_cmd,

diff --git a/in_toto/in_toto_sign.py b/in_toto/in_toto_sign.py
@@ -88,7 +88,8 @@ def _sign_and_dump_metadata(metadata, args):
           " of keys specified")
 
       for idx, key_path in enumerate(args.key):
-        key = util.import_private_key_from_file(key_path, args.key_type[idx])
+        key = interface.import_privatekey_from_file(
+            key_path, key_type=args.key_type[idx], prompt=args.prompt)
         signature = metadata.sign(key)
 
     # If `--output` was specified we store the signed link or layout metadata
@@ -268,6 +269,9 @@ def create_parser():
       " omitted, the default of '{rsa}' is used for all keys.".format(
       rsa=util.KEY_TYPE_RSA, ed25519=util.KEY_TYPE_ED25519)))
 
+  parser.add_argument("-p", "--prompt", action="store_true",
+      help="prompt for signing key decryption password")
+
   parser.add_argument("-g", "--gpg", nargs="*", metavar="<id>", help=(
       "GPG keyids used to sign the passed link or layout metadata or to verify"
       " its signatures. If passed without keyids, the default GPG key is"

diff --git a/tests/test_in_toto_record.py b/tests/test_in_toto_record.py
@@ -28,15 +28,16 @@
 else:
   import mock # pylint: disable=import-error
 
-import in_toto.util
 from in_toto.models.link import UNFINISHED_FILENAME_FORMAT
 from in_toto.in_toto_record import main as in_toto_record_main
 
-from tests.common import CliTestCase, TmpDirMixin, GPGKeysMixin
+from tests.common import CliTestCase, TmpDirMixin, GPGKeysMixin, GenKeysMixin
 
+import securesystemslib.interface # pylint: disable=unused-import
 
 
-class TestInTotoRecordTool(CliTestCase, TmpDirMixin, GPGKeysMixin):
+
+class TestInTotoRecordTool(CliTestCase, TmpDirMixin, GPGKeysMixin, GenKeysMixin):
   """Test in_toto_record's main() - requires sys.argv patching; and
   in_toto_record_start/in_toto_record_stop - calls runlib and error logs/exits
   on Exception. """
@@ -48,12 +49,7 @@ def setUpClass(self):
     generate key pair, dummy artifact and base arguments. """
     self.set_up_test_dir()
     self.set_up_gpg_keys()
-
-    self.rsa_key_path = "test_key_rsa"
-    in_toto.util.generate_and_write_rsa_keypair(self.rsa_key_path)
-
-    self.ed25519_key_path = "test_key_ed25519"
-    in_toto.util.generate_and_write_ed25519_keypair(self.ed25519_key_path)
+    self.set_up_keys()
 
     self.test_artifact1 = "test_artifact1"
     self.test_artifact2 = "test_artifact2"
@@ -74,6 +70,21 @@ def test_start_stop(self):
     self.assert_cli_sys_exit(["start"] + args, 0)
     self.assert_cli_sys_exit(["stop"] + args, 0)
 
+    # Start/stop recording using encrypted rsa key with password on prompt
+    args = ["--step-name", "test1.1", "--key", self.rsa_key_enc_path,
+        "--password"]
+    with mock.patch('securesystemslib.interface.get_password',
+        return_value=self.key_pw):
+      self.assert_cli_sys_exit(["start"] + args, 0)
+      self.assert_cli_sys_exit(["stop"] + args, 0)
+
+    # Start/stop recording using encrypted rsa key passing the pw
+    args = ["--step-name", "test1.2", "--key", self.rsa_key_enc_path,
+        "--password", self.key_pw]
+    self.assert_cli_sys_exit(["start"] + args, 0)
+    self.assert_cli_sys_exit(["stop"] + args, 0)
+
+
     # Start/stop with recording one artifact using rsa key
     args = ["--step-name", "test2", "--key", self.rsa_key_path]
     self.assert_cli_sys_exit(["start"] + args + ["--materials",
@@ -106,6 +117,20 @@ def test_start_stop(self):
     self.assert_cli_sys_exit(["start"] + args, 0)
     self.assert_cli_sys_exit(["stop"] + args, 0)
 
+    # Start/stop with encrypted ed25519 key entering password on the prompt
+    args = ["--step-name", "test4.1", "--key", self.ed25519_key_enc_path,
+        "--key-type", "ed25519", "--password"]
+    with mock.patch('securesystemslib.interface.get_password',
+        return_value=self.key_pw):
+      self.assert_cli_sys_exit(["start"] + args, 0)
+      self.assert_cli_sys_exit(["stop"] + args, 0)
+
+    # Start/stop with encrypted ed25519 key passing the password
+    args = ["--step-name", "test4.2", "--key", self.ed25519_key_enc_path,
+        "--key-type", "ed25519", "--password", self.key_pw]
+    self.assert_cli_sys_exit(["start"] + args, 0)
+    self.assert_cli_sys_exit(["stop"] + args, 0)
+
     # Start/stop with recording one artifact using ed25519 key
     args = ["--step-name", "test5", "--key", self.ed25519_key_path, "--key-type", "ed25519"]
     self.assert_cli_sys_exit(["start"] + args + ["--materials",
@@ -169,6 +194,17 @@ def test_glob_to_many_unfinished_files(self):
         "--gpg-home", self.gnupg_home]
     self.assert_cli_sys_exit(["stop"] + args, 1)
 
+  def test_encrypted_key_but_no_pw(self):
+    args = ["--step-name", "enc-key", "--key", self.rsa_key_enc_path]
+    self.assert_cli_sys_exit(["start"] + args, 1)
+    self.assert_cli_sys_exit(["stop"] + args, 1)
+
+    args = ["--step-name", "enc-key", "--key", self.ed25519_key_enc_path,
+        "--key-type", "ed25519"]
+    self.assert_cli_sys_exit(["start"] + args, 1)
+    self.assert_cli_sys_exit(["stop"] + args, 1)
+
+
   def test_wrong_key(self):
     """Test CLI command record with wrong key exits 1 """
     args = ["--step-name", "wrong-key", "--key", "non-existing-key"]
@@ -181,6 +217,7 @@ def test_no_key(self):
     self.assert_cli_sys_exit(["start"] + args, 2)
     self.assert_cli_sys_exit(["stop"] + args, 2)
 
+
   def test_missing_unfinished_link(self):
     """Error exit with missing unfinished link file. """
     args = ["--step-name", "no-link", "--key", self.rsa_key_path]

diff --git a/tests/test_in_toto_run.py b/tests/test_in_toto_run.py
@@ -30,18 +30,16 @@
 else:
   import mock # pylint: disable=import-error
 
-from in_toto.util import (generate_and_write_rsa_keypair,
-    generate_and_write_ed25519_keypair, import_private_key_from_file,
-    KEY_TYPE_RSA, KEY_TYPE_ED25519)
-
 from in_toto.models.metadata import Metablock
 from in_toto.in_toto_run import main as in_toto_run_main
 from in_toto.models.link import FILENAME_FORMAT
 
-from tests.common import CliTestCase, TmpDirMixin, GPGKeysMixin
+from tests.common import CliTestCase, TmpDirMixin, GPGKeysMixin, GenKeysMixin
+
+import securesystemslib.interface # pylint: disable=unused-import
 
 
-class TestInTotoRunTool(CliTestCase, TmpDirMixin, GPGKeysMixin):
+class TestInTotoRunTool(CliTestCase, TmpDirMixin, GPGKeysMixin, GenKeysMixin):
   """Test in_toto_run's main() - requires sys.argv patching; and
   in_toto_run- calls runlib and error logs/exits on Exception. """
   cli_main_func = staticmethod(in_toto_run_main)
@@ -52,20 +50,18 @@ def setUpClass(self):
     generate key pair, dummy artifact and base arguments. """
     self.set_up_test_dir()
     self.set_up_gpg_keys()
-
-    self.rsa_key_path = "test_key_rsa"
-    generate_and_write_rsa_keypair(self.rsa_key_path)
-    self.rsa_key = import_private_key_from_file(self.rsa_key_path,
-        KEY_TYPE_RSA)
-
-    self.ed25519_key_path = "test_key_ed25519"
-    generate_and_write_ed25519_keypair(self.ed25519_key_path)
-    self.ed25519_key = import_private_key_from_file(self.ed25519_key_path,
-        KEY_TYPE_ED25519)
+    self.set_up_keys()
 
     self.test_step = "test_step"
-    self.test_link_rsa = FILENAME_FORMAT.format(step_name=self.test_step, keyid=self.rsa_key["keyid"])
-    self.test_link_ed25519 = FILENAME_FORMAT.format(step_name=self.test_step, keyid=self.ed25519_key["keyid"])
+    self.test_link_rsa = FILENAME_FORMAT.format(
+        step_name=self.test_step, keyid=self.rsa_key_id)
+    self.test_link_ed25519 = FILENAME_FORMAT.format(
+        step_name=self.test_step, keyid=self.ed25519_key_id)
+    self.test_link_rsa_enc = FILENAME_FORMAT.format(
+        step_name=self.test_step, keyid=self.rsa_key_enc_id)
+    self.test_link_ed25519_enc = FILENAME_FORMAT.format(
+        step_name=self.test_step, keyid=self.ed25519_key_enc_id)
+
     self.test_artifact = "test_artifact"
     open(self.test_artifact, "w").close()
 
@@ -84,7 +80,6 @@ def test_main_required_args(self):
         "python", "--version"]
 
     self.assert_cli_sys_exit(args, 0)
-
     self.assertTrue(os.path.exists(self.test_link_rsa))
 
 
@@ -159,21 +154,36 @@ def test_main_with_unencrypted_ed25519_key(self):
     self.assertTrue(os.path.exists(self.test_link_ed25519))
 
 
-  def test_main_with_encrypted_ed25519_key(self):
+  def test_main_with_encrypted_keys(self):
     """Test CLI command with encrypted ed25519 key. """
-    key_path = "test_key_ed25519_enc"
-    password = "123456"
-    generate_and_write_ed25519_keypair(key_path, password)
-    args = ["-n", self.test_step,
-        "--key", key_path,
-        "--key-type", "ed25519", "--", "ls"]
 
-    with mock.patch('in_toto.util.prompt_password', return_value=password):
-      key = import_private_key_from_file(key_path, KEY_TYPE_ED25519)
-      linkpath = FILENAME_FORMAT.format(step_name=self.test_step, keyid=key["keyid"])
+    for key_type, key_path, link_path in [
+        ("rsa", self.rsa_key_enc_path, self.test_link_rsa_enc),
+        ("ed25519", self.ed25519_key_enc_path, self.test_link_ed25519_enc)]:
+
 
-      self.assert_cli_sys_exit(args, 0)
-      self.assertTrue(os.path.exists(linkpath))
+      # Define common arguments passed to in in-toto-run below
+      args = [
+          "-n", self.test_step,
+          "--key", key_path,
+          "--key-type", key_type]
+      cmd = ["--", "python", "--version"]
+
+      # Make sure the link file to be generated doesn't already exist
+      self.assertFalse(os.path.exists(link_path))
+
+      # Test 1: Call in-toto-run entering signing key password on prompt
+      with mock.patch('securesystemslib.interface.get_password',
+          return_value=self.key_pw):
+        self.assert_cli_sys_exit(args + ["--password"] + cmd, 0)
+
+      self.assertTrue(os.path.exists(link_path))
+      os.remove(link_path)
+
+      # Test 2: Call in-toto-run passing signing key password
+      self.assert_cli_sys_exit(args + ["--password", self.key_pw] + cmd, 0)
+      self.assertTrue(os.path.exists(link_path))
+      os.remove(link_path)
 
 
   def test_main_with_specified_gpg_key(self):
@@ -242,5 +252,15 @@ def test_main_wrong_key_exits(self):
     self.assertFalse(os.path.exists(self.test_link_rsa))
 
 
+  def test_main_encrypted_key_but_no_pw(self):
+    """Test CLI command exits 1 with encrypted key but no pw. """
+    args = ["-n", self.test_step, "--key", self.rsa_key_enc_path, "-x"]
+    self.assert_cli_sys_exit(args, 1)
+    self.assertFalse(os.path.exists(self.test_link_rsa_enc))
+
+    args = ["-n", self.test_step, "--key", self.ed25519_key_enc_path, "-x"]
+    self.assert_cli_sys_exit(args, 1)
+    self.assertFalse(os.path.exists(self.test_link_ed25519_enc))
+
 if __name__ == "__main__":
   unittest.main()