Align behavior with vault concerning key and phrase in config
Marco Ricci

Marco Ricci commited on 2024-10-05 23:30:07
Zeige 5 geänderte Dateien mit 36 Einfügungen und 44 Löschungen.


When both a key and a passphrase are specified in the vault
configuration, vault(1) would unconditionally use the key, *unless* the
command-line overrides this choice.  `derivepassphrase` however always
gave preference to the most "specific" configuration, and would error
out if both key and passphrase were specified at the same specificity.
While arguably more intuitive, this behavior is a visible deviation from
vault(1), and shall thus be removed.

Besides two instances of the `test_200_is_vault_config` in
`tests.test_derivepassphrase_types`, this also flips the result of
`test_205_service_phrase_if_key_in_global_config` in
`tests.test_derivepassphrase_cli`.  Because that flipped version needs
extra mocking infrastructure – the `sign` function – and because that
mock function already exists in another test (but local to that test),
promote that mock function to global and shift it into the top-level
`tests` module.

Since we had to update the imports in `tests` anyway, we also purged
`dpp.vault...` references in `tests.test_derivepassphrase_cli` in favor
of `vault...`.
... ...
@@ -182,10 +182,6 @@ def validate_vault_config(  # noqa: C901,PLR0912,PLR0915
182 182
         json_path_str = as_json_path_string(json_path)
183 183
         return f'vault config entry {json_path_str} is not an integer'
184 184
 
185
-    err_key_and_phrase = (
186
-        '"key" and "phrase" specified on the same vault config level'
187
-    )
188
-
189 185
     def err_derivepassphrase_extension(
190 186
         key: str, json_path: Sequence[str], /
191 187
     ) -> str:
... ...
@@ -234,8 +230,6 @@ def validate_vault_config(  # noqa: C901,PLR0912,PLR0915
234 230
                     )
235 231
             elif not allow_unknown_settings:
236 232
                 raise ValueError(err_unknown_setting(key, ('global',)))
237
-        if 'key' in o_global and 'phrase' in o_global:
238
-            raise ValueError(err_key_and_phrase)
239 233
     if not isinstance(obj.get('services'), dict):
240 234
         raise TypeError(err_not_a_dict(['services']))
241 235
     for sv_name, service in obj['services'].items():
... ...
@@ -284,8 +278,6 @@ def validate_vault_config(  # noqa: C901,PLR0912,PLR0915
284 278
                 raise ValueError(
285 279
                     err_unknown_setting(key, ['services', sv_name])
286 280
                 )
287
-        if 'key' in service and 'phrase' in service:
288
-            raise ValueError(err_key_and_phrase)
289 281
 
290 282
 
291 283
 def is_vault_config(obj: Any) -> TypeIs[VaultConfig]:  # noqa: ANN401
... ...
@@ -1574,17 +1574,12 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1574 1574
 
1575 1575
             # If either --key or --phrase are given, use that setting.
1576 1576
             # Otherwise, if both key and phrase are set in the config,
1577
-            # one must be global (ignore it) and one must be
1578
-            # service-specific (use that one). Otherwise, if only one of
1579
-            # key and phrase is set in the config, use that one.  In all
1580
-            # these above cases, set the phrase via
1581
-            # derivepassphrase.vault.Vault.phrase_from_key if a key is
1582
-            # given. Finally, if nothing is set, error out.
1577
+            # use the key.  Otherwise, if only one of key and phrase is
1578
+            # set in the config, use that one.  In all these above
1579
+            # cases, set the phrase via vault.Vault.phrase_from_key if
1580
+            # a key is given.  Finally, if nothing is set, error out.
1583 1581
             if use_key or use_phrase:
1584 1582
                 kwargs['phrase'] = key_to_phrase(key) if use_key else phrase
1585
-            elif kwargs.get('phrase') and kwargs.get('key'):
1586
-                if any('key' in m for m in settings.maps[:2]):
1587
-                    kwargs['phrase'] = key_to_phrase(kwargs['key'])
1588 1583
             elif kwargs.get('key'):
1589 1584
                 kwargs['phrase'] = key_to_phrase(kwargs['key'])
1590 1585
             elif kwargs.get('phrase'):
... ...
@@ -19,7 +19,7 @@ from typing import TYPE_CHECKING
19 19
 import pytest
20 20
 from typing_extensions import NamedTuple, Self, assert_never
21 21
 
22
-from derivepassphrase import _types, cli, ssh_agent
22
+from derivepassphrase import _types, cli, ssh_agent, vault
23 23
 
24 24
 __all__ = ()
25 25
 
... ...
@@ -721,6 +721,18 @@ def list_keys(self: Any = None) -> list[_types.KeyCommentPair]:
721 721
     return list1 + list2
722 722
 
723 723
 
724
+def sign(
725
+    self: Any, key: bytes | bytearray, message: bytes | bytearray
726
+) -> bytes:
727
+    del self  # Unused.
728
+    assert message == vault.Vault._UUID
729
+    for value in SUPPORTED_KEYS.values():
730
+        if value['public_key_data'] == key:  # pragma: no branch
731
+            assert value['expected_signature'] is not None
732
+            return value['expected_signature']
733
+    raise AssertionError
734
+
735
+
724 736
 def list_keys_singleton(self: Any = None) -> list[_types.KeyCommentPair]:
725 737
     del self  # Unused.
726 738
     Pair = _types.KeyCommentPair  # noqa: N806
... ...
@@ -16,9 +16,8 @@ import click.testing
16 16
 import pytest
17 17
 from typing_extensions import NamedTuple
18 18
 
19
-import derivepassphrase as dpp
20 19
 import tests
21
-from derivepassphrase import _types, cli, ssh_agent
20
+from derivepassphrase import _types, cli, ssh_agent, vault
22 21
 
23 22
 if TYPE_CHECKING:
24 23
     from collections.abc import Callable
... ...
@@ -225,7 +224,7 @@ class TestCLI:
225 224
     ) -> None:
226 225
         monkeypatch.setattr(cli, '_prompt_for_passphrase', tests.auto_prompt)
227 226
         option = f'--{charset_name}'
228
-        charset = dpp.vault.Vault._CHARSETS[charset_name].decode('ascii')
227
+        charset = vault.Vault._CHARSETS[charset_name].decode('ascii')
229 228
         runner = click.testing.CliRunner(mix_stderr=False)
230 229
         with tests.isolated_config(
231 230
             monkeypatch=monkeypatch,
... ...
@@ -304,7 +303,7 @@ class TestCLI:
304 303
             monkeypatch=monkeypatch, runner=runner, config=config
305 304
         ):
306 305
             monkeypatch.setattr(
307
-                dpp.vault.Vault, 'phrase_from_key', tests.phrase_from_key
306
+                vault.Vault, 'phrase_from_key', tests.phrase_from_key
308 307
             )
309 308
             _result = runner.invoke(
310 309
                 cli.derivepassphrase_vault,
... ...
@@ -336,7 +335,7 @@ class TestCLI:
336 335
                 cli, '_get_suitable_ssh_keys', tests.suitable_ssh_keys
337 336
             )
338 337
             monkeypatch.setattr(
339
-                dpp.vault.Vault, 'phrase_from_key', tests.phrase_from_key
338
+                vault.Vault, 'phrase_from_key', tests.phrase_from_key
340 339
             )
341 340
             _result = runner.invoke(
342 341
                 cli.derivepassphrase_vault,
... ...
@@ -386,22 +385,12 @@ class TestCLI:
386 385
         config: dict[str, Any],
387 386
         key_index: int,
388 387
     ) -> None:
389
-        def sign(
390
-            _: Any, key: bytes | bytearray, message: bytes | bytearray
391
-        ) -> bytes:
392
-            del message  # Unused.
393
-            for value in tests.SUPPORTED_KEYS.values():
394
-                if value['public_key_data'] == key:
395
-                    assert value['expected_signature'] is not None
396
-                    return value['expected_signature']
397
-            raise AssertionError
398
-
399 388
         with monkeypatch.context():
400 389
             monkeypatch.setenv('SSH_AUTH_SOCK', running_ssh_agent)
401 390
             monkeypatch.setattr(
402 391
                 ssh_agent.SSHAgentClient, 'list_keys', tests.list_keys
403 392
             )
404
-            monkeypatch.setattr(ssh_agent.SSHAgentClient, 'sign', sign)
393
+            monkeypatch.setattr(ssh_agent.SSHAgentClient, 'sign', tests.sign)
405 394
             runner = click.testing.CliRunner(mix_stderr=False)
406 395
             with tests.isolated_vault_config(
407 396
                 monkeypatch=monkeypatch, runner=runner, config=config
... ...
@@ -422,7 +411,14 @@ class TestCLI:
422 411
     def test_205_service_phrase_if_key_in_global_config(
423 412
         self,
424 413
         monkeypatch: pytest.MonkeyPatch,
414
+        running_ssh_agent: str,
425 415
     ) -> None:
416
+        with monkeypatch.context():
417
+            monkeypatch.setenv('SSH_AUTH_SOCK', running_ssh_agent)
418
+            monkeypatch.setattr(
419
+                ssh_agent.SSHAgentClient, 'list_keys', tests.list_keys
420
+            )
421
+            monkeypatch.setattr(ssh_agent.SSHAgentClient, 'sign', tests.sign)
426 422
             runner = click.testing.CliRunner(mix_stderr=False)
427 423
             with tests.isolated_vault_config(
428 424
                 monkeypatch=monkeypatch,
... ...
@@ -447,10 +443,10 @@ class TestCLI:
447 443
         assert _result.stdout_bytes, 'expected program output'
448 444
         last_line = _result.stdout_bytes.splitlines(True)[-1]
449 445
         assert (
450
-            last_line.rstrip(b'\n') != DUMMY_RESULT_KEY1
451
-        ), 'known false output: key-based instead of phrase-based'
446
+            last_line.rstrip(b'\n') != DUMMY_RESULT_PASSPHRASE
447
+        ), 'known false output: phrase-based instead of key-based'
452 448
         assert (
453
-            last_line.rstrip(b'\n') == DUMMY_RESULT_PASSPHRASE
449
+            last_line.rstrip(b'\n') == DUMMY_RESULT_KEY1
454 450
         ), 'expected known output'
455 451
 
456 452
     @pytest.mark.parametrize(
... ...
@@ -1559,7 +1555,7 @@ Boo.
1559 1555
 
1560 1556
     def test_204_phrase_from_key_manually(self) -> None:
1561 1557
         assert (
1562
-            dpp.vault.Vault(
1558
+            vault.Vault(
1563 1559
                 phrase=DUMMY_PHRASE_FROM_KEY1, **DUMMY_CONFIG_SETTINGS
1564 1560
             ).generate(DUMMY_SERVICE)
1565 1561
             == DUMMY_RESULT_KEY1
... ...
@@ -1844,7 +1840,7 @@ class TestCLITransition:
1844 1840
     ) -> None:
1845 1841
         monkeypatch.setattr(cli, '_prompt_for_passphrase', tests.auto_prompt)
1846 1842
         option = f'--{charset_name}'
1847
-        charset = dpp.vault.Vault._CHARSETS[charset_name].decode('ascii')
1843
+        charset = vault.Vault._CHARSETS[charset_name].decode('ascii')
1848 1844
         runner = click.testing.CliRunner(mix_stderr=False)
1849 1845
         with tests.isolated_config(
1850 1846
             monkeypatch=monkeypatch,
... ...
@@ -22,7 +22,7 @@ from derivepassphrase import _types
22 22
         ),
23 23
         (
24 24
             {'global': {'phrase': 'abc', 'key': '...'}, 'services': {}},
25
-            'incompatible config values: global.key and global.phrase',
25
+            '',
26 26
         ),
27 27
         ({'services': None}, 'bad config value: services'),
28 28
         ({'services': {2: {}}}, 'bad config value: services."2"'),
... ...
@@ -62,10 +62,7 @@ from derivepassphrase import _types
62 62
         ({'services': {'sv': {'length': 10, 'phrase': '...'}}}, ''),
63 63
         ({'services': {'sv': {'length': 10, 'key': '...'}}}, ''),
64 64
         ({'services': {'sv': {'upper': 10, 'key': '...'}}}, ''),
65
-        (
66
-            {'services': {'sv': {'phrase': 'abc', 'key': '...'}}},
67
-            'incompatible config values: services.sv.key and services.sv.phrase',  # noqa: E501
68
-        ),
65
+        ({'services': {'sv': {'phrase': 'abc', 'key': '...'}}}, ''),
69 66
         (
70 67
             {
71 68
                 'global': {'phrase': 'abc'},
72 69