Merge pull request #178 from peopledoc/crash-176

Author: Joachim Jablon

docs/howto/environment.rst

@@ -114,3 +114,19 @@ Your call would look like:
 .. code:: console
 
     $ vault-cli env --omit-single-key --path myapp -- myapp
+
+Ignoring errors
+---------------
+
+By default, ``vault-cli`` will not launch you program if an error happens during secrets
+collection. You can pass ``--force`` to ensure that your program will be launched,
+even if it will be missing some secrets.
+
+.. code:: console
+
+    $ vault-cli env --path myapp --force -- myapp
+
+.. warning::
+
+    Even if just a single key for a secret produces an error (e.g. a template rendering
+    error), the whole secret will be missing.

tests/unit/test_cli.py

@@ -304,6 +304,36 @@ def test_env(cli_runner, vault_with_token, mocker):
     assert kwargs["environment"]["FOO_BAZ_VALUE"] == "yo"
 
 
+def test_env_error(cli_runner, vault_with_token, mocker):
+    exec_command = mocker.patch("vault_cli.environment.exec_command")
+
+    vault_with_token.forbidden_get_paths.add("foo")
+
+    cli_runner.invoke(cli.cli, ["env", "--path", "foo", "--", "echo", "yay"])
+
+    exec_command.assert_not_called()
+
+
+def test_env_error_force_sub_error(cli_runner, vault_with_token, mocker):
+    exec_command = mocker.patch("vault_cli.environment.exec_command")
+
+    vault_with_token.forbidden_get_paths.add("foo")
+
+    cli_runner.invoke(cli.cli, ["env", "--path", "foo", "--force", "--", "echo", "yay"])
+
+    exec_command.assert_called()
+
+
+def test_env_error_force_main_error(cli_runner, vault_with_token, mocker):
+    exec_command = mocker.patch("vault_cli.environment.exec_command")
+
+    vault_with_token.forbidden_list_paths.add("foo")
+
+    cli_runner.invoke(cli.cli, ["env", "--path", "foo", "--force", "--", "echo", "yay"])
+
+    exec_command.assert_called()
+
+
 def test_env_prefix(cli_runner, vault_with_token, mocker):
     exec_command = mocker.patch("vault_cli.environment.exec_command")
 
@@ -714,3 +744,12 @@ def test_parse_octal(input, output):
 )
 def test_repr_octal(input, output):
     assert cli.repr_octal(input) == output
+
+
+def test_ensure_str():
+    assert cli.ensure_str("foo", "bar") == "foo"
+
+
+def test_ensure_str_wrong():
+    with pytest.raises(exceptions.VaultWrongType):
+        cli.ensure_str(1, "bar")

tests/unit/test_client_base.py

@@ -399,9 +399,10 @@ def test_vault_client_base_render_template(vault):
     assert vault.render_template("Hello {{ vault('a/b').value }}") == "Hello c"
 
 
-def test_vault_client_base_render_template_path_not_found(vault):
+@pytest.mark.parametrize("template", ["Hello {{ vault('a/b') }}", "Hello {{"])
+def test_vault_client_base_render_template_path_not_found(vault, template):
     with pytest.raises(exceptions.VaultRenderTemplateError):
-        vault.render_template("Hello {{ vault('a/b') }}")
+        vault.render_template(template)
 
 
 @pytest.mark.parametrize(
@@ -499,18 +500,38 @@ def test_vault_client_base_get_secret_missing_key(vault):
         vault.get_secret("a", key="username")
 
 
+def test_vault_client_base_get_secret_template_error(vault, caplog):
+    vault.db = {"a": {"key": "!template!{{"}}
+
+    with pytest.raises(exceptions.VaultRenderTemplateError) as exc_info:
+        vault.get_secret("a")
+
+    assert str(exc_info.value) == 'Error while rendering secret at path "a"'
+    assert (
+        str(exc_info.value.__cause__)
+        == 'Error while rendering secret value for key "key"'
+    )
+    assert str(exc_info.value.__cause__.__cause__) == "Jinja2 template syntax error"
+
+
 def test_vault_client_base_lookup_token(vault):
     assert vault.lookup_token() == {"data": {"expire_time": "2100-01-01T00:00:00"}}
 
 
-def test_vault_client_base_get_secrets_error(vault):
+def test_vault_client_base_get_secrets_error(vault, caplog):
     vault.db = {"a": {"value": "b"}, "c": {"value": "d"}}
     vault.forbidden_get_paths = {"c"}
 
     assert vault.get_secrets("") == {
         "a": {"value": "b"},
-        "c": {"error": "<error while retrieving secret>"},
+        "c": {},
     }
+    assert caplog.record_tuples[0] == (
+        "vault_cli.client",
+        40,
+        "VaultForbidden: Insufficient access for interacting with the requested "
+        "secret",
+    )
 
 
 def test_vault_client_base_get_secrets_list_forbidden(vault):
@@ -571,12 +592,12 @@ def test_vault_client_base_base_path(vault, path, expected):
     assert vault.base_path == expected
 
 
-def test_vault_client_base_get_secret_implicit_cache_ends(vault):
+def test_vault_client_base_get_secret_implicit_cache(vault):
     vault.db = {"a": {"value": "b"}}
     assert vault.get_secret("a") == {"value": "b"}
     vault.db = {"a": {"value": "c"}}
-    # Value updated. Cache was just for the duration of the call
-    assert vault.get_secret("a") == {"value": "c"}
+    # Value was cached
+    assert vault.get_secret("a") == {"value": "b"}
 
 
 class RaceConditionTestVaultClient(testing.TestVaultClient):
@@ -600,13 +621,16 @@ def test_vault_client_base_get_secret_implicit_cache_no_race_condition():
 
     vault = RaceConditionTestVaultClient()
 
-    assert vault.get_secret("a") == {"b": "b0", "c": "c0"}
-    assert vault.get_secret("a") == {"b": "b1", "c": "c1"}
+    with vault:
+        assert vault.get_secret("a") == {"b": "b0", "c": "c0"}
+    with vault:
+        assert vault.get_secret("a") == {"b": "b1", "c": "c1"}
 
     vault.db = {"d": {"value": """!template!{{ vault("a").b }}-{{ vault("a").c }}"""}}
 
     # b2-c3 would be the value if caching didn't work.
-    assert vault.get_secret("d") == {"value": "b2-c2"}
+    with vault:
+        assert vault.get_secret("d") == {"value": "b2-c2"}
 
 
 def test_vault_client_base_get_secrets_implicit_cache_no_race_condition():
@@ -628,8 +652,9 @@ def test_vault_client_base_get_secrets_implicit_cache_no_race_condition():
 
 def test_vault_client_base_get_secret_explicit_cache(vault):
     vault.db = {"a": {"value": "b"}}
-    with vault.caching():
+    with vault:
         assert vault.get_secret("a") == {"value": "b"}
         vault.db = {"a": {"value": "c"}}
         # Value not updated
         assert vault.get_secret("a") == {"value": "b"}
+    assert vault.get_secret("a") == {"value": "c"}

tests/unit/test_client_hvac.py

@@ -2,6 +2,7 @@
 
 import hvac
 import pytest
+import requests
 
 from vault_cli import client, exceptions
 
@@ -176,6 +177,7 @@ def test_set_context_manager(mocker):
         (hvac.exceptions.InternalServerError, exceptions.VaultInternalServerError),
         (hvac.exceptions.VaultDown, exceptions.VaultSealed),
         (hvac.exceptions.UnexpectedError, exceptions.VaultAPIException),
+        (requests.exceptions.ConnectionError, exceptions.VaultConnectionError),
     ],
 )
 def test_handle_errors(hvac_exc, vault_cli_exc):

tests/unit/test_exceptions.py

@@ -11,10 +11,7 @@ def test_vault_overwrite_secret_error():
 
 
 def test_vault_render_template_error():
-    assert (
-        str(exceptions.VaultRenderTemplateError("yay"))
-        == "VaultRenderTemplateError: Error while rendering template: yay"
-    )
+    assert str(exceptions.VaultRenderTemplateError("yay")) == "yay"
 
 
 @pytest.mark.parametrize(

vault_cli/cli.py

@@ -19,7 +19,7 @@
 import yaml
 
 import vault_cli
-from vault_cli import client, environment, exceptions, settings, ssh, types
+from vault_cli import client, environment, exceptions, settings, ssh, types, utils
 
 logger = logging.getLogger(__name__)
 
@@ -59,14 +59,7 @@ def handle_errors():
     try:
         yield
     except exceptions.VaultException as exc:
-        messages = []
-        while True:
-            exc_str = str(exc).strip()
-            messages.append(f"{type(exc).__name__}: {exc_str}")
-            exc = exc.__cause__ or exc.__context__
-            if not exc:
-                break
-        raise click.ClickException("\n".join(messages))
+        raise click.ClickException("\n".join(utils.extract_error_messages(exc)))
 
 
 def print_version(ctx, __, value):
@@ -417,17 +410,23 @@ def delete(client_obj: client.VaultClientBase, name: str, key: Optional[str]) ->
 @click.option(
     "-o",
     "--omit-single-key/--no-omit-single-key",
-    is_flag=True,
     default=False,
     help="When the secret has only one key, don't use that key to build the name of the environment variable",
 )
+@click.option(
+    "-f",
+    "--force/--no-force",
+    default=False,
+    help="Run the command even if there is a problem while reading secrets",
+)
 @click.argument("command", nargs=-1, required=True)
 @click.pass_obj
 @handle_errors()
 def env(
     client_obj: client.VaultClientBase,
     path: Sequence[str],
     omit_single_key: bool,
+    force: bool,
     command: Sequence[str],
 ) -> NoReturn:
     """
@@ -462,15 +461,19 @@ def env(
         path_with_key, _, prefix = path.partition("=")
         path, _, filter_key = path_with_key.partition(":")
 
+        env_updates = {}
         env_updates = environment.get_envvars(
             vault_client=client_obj,
             path=path,
             prefix=prefix,
             omit_single_key=omit_single_key,
             filter_key=filter_key,
         )
+
         env_secrets.update(env_updates)
 
+    if bool(client_obj.errors) and not force:
+        raise click.ClickException("There was an error while reading the secrets.")
     environment.exec_command(command=command, environment=env_secrets)
 
 
@@ -639,20 +642,30 @@ def ssh_(
     if ":" not in key:
         raise click.UsageError("Format for private_key: `path/to/private_key:key`")
     private_key_name, private_key_key = key.rsplit(":", 1)
-    private_key_secret = client_obj.get_secret(private_key_name, private_key_key)
+    private_key_secret_obj = client_obj.get_secret(private_key_name, private_key_key)
+    private_key_secret = ensure_str(secret=private_key_secret_obj, path=key)
 
     passphrase_secret = None
     if passphrase:
         if ":" not in passphrase:
             raise click.UsageError("Format for passphrase: `path/to/passphrase:key`")
         passphrase_name, passphrase_key = passphrase.rsplit(":", 1)
-        passphrase_secret = client_obj.get_secret(passphrase_name, passphrase_key)
+        passphrase_secret_obj = client_obj.get_secret(passphrase_name, passphrase_key)
+        passphrase_secret = ensure_str(secret=passphrase_secret_obj, path=passphrase)
 
     ssh.add_key(key=private_key_secret, passphrase=passphrase_secret)
 
     environment.exec_command(command=command)
 
 
+def ensure_str(secret, path) -> str:
+    if not isinstance(secret, str):
+        raise exceptions.VaultWrongType(
+            f"secret at {path} is not a string but {type(secret)}"
+        )
+    return secret
+
+
 # This is purposedly not called as a click subcommand, and we cannot take any argument
 def askpass():
     print(os.environ[ssh.SSH_PASSPHRASE_ENVVAR])