Fix missing consideration of key and phrase both being specified
Marco Ricci

Marco Ricci commited on 2024-10-14 23:14:09
Zeige 3 geänderte Dateien mit 106 Einfügungen und 18 Löschungen.


In 798ddc103c6c03835394733aeca128b970aacd06, we permitted both key and
phrase to be specified when importing a configuration.  However, the
code for updating or setting a stored global or service configuration
was still purging the key or the phrase setting if the other was being
set.  Additionally, the currently effective settings and the new
settings object were being incorrectly calculated, and none of the tests
were actually specifically testing this.  So fix all of these.

When setting the new settings object, we issue a warning if we are newly
configuring a passphrase but a key is effectively set; the key will
override the passphrase unconditionally.

Since there are now a couple of (harmless) warning messages that will be
emitted for weird or problematic inputs, and which are therefore likely
and sometimes necessary to trigger during tests, add a new function to
classify warnings as "harmless and expected" or not.  This keeps the
test code for successfully calling `derivepassphrase vault` still
reasonably short and readable.  It also keeps this test code generic, so
that we needn't defensively code around situations where the input is
being programmatically generated and may or may not trigger these kinds
of warnings.
... ...
@@ -1511,7 +1511,6 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1511 1511
                 dict[str, Any],
1512 1512
                 configuration['services'].get(service or '', {}),
1513 1513
             ),
1514
-            {},
1515 1514
             cast(dict[str, Any], configuration.get('global', {})),
1516 1515
         )
1517 1516
         if use_key:
... ...
@@ -1550,20 +1549,24 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1550 1549
             view = (
1551 1550
                 collections.ChainMap(*settings.maps[:2])
1552 1551
                 if service
1553
-                else settings.parents.parents
1552
+                else collections.ChainMap(settings.maps[0], settings.maps[2])
1554 1553
             )
1555 1554
             if use_key:
1556 1555
                 view['key'] = key
1557
-                for m in view.maps:
1558
-                    m.pop('phrase', '')
1559 1556
             elif use_phrase:
1557
+                view['phrase'] = phrase
1558
+                settings_type = 'service' if service else 'global'
1560 1559
                 _check_for_misleading_passphrase(
1561 1560
                     ('services', service) if service else ('global',),
1562 1561
                     {'phrase': phrase},
1563 1562
                 )
1564
-                view['phrase'] = phrase
1565
-                for m in view.maps:
1566
-                    m.pop('key', '')
1563
+                if 'key' in settings:
1564
+                    err_msg = (
1565
+                        f'{PROG_NAME}: Warning: Setting a {settings_type} '
1566
+                        f'passphrase is ineffective because a key is also '
1567
+                        f'set.'
1568
+                    )
1569
+                    click.echo(err_msg, err=True)
1567 1570
             if not view.maps[0]:
1568 1571
                 settings_type = 'service' if service else 'global'
1569 1572
                 msg = (
... ...
@@ -155,6 +155,14 @@ TEST_CONFIGS: list[VaultTestConfig] = [
155 155
         '',
156 156
         None,
157 157
     ),
158
+    VaultTestConfig(
159
+        {
160
+            'global': {'key': '...'},
161
+            'services': {'sv': {'phrase': 'abc', 'key': '...', 'length': 10}},
162
+        },
163
+        '',
164
+        None,
165
+    ),
158 166
     VaultTestConfig(
159 167
         {
160 168
             'global': {'key': '...'},
... ...
@@ -203,6 +203,25 @@ for opt, config in SINGLES.items():
203 203
     )
204 204
 
205 205
 
206
+def is_harmless_config_import_warning_line(line: str) -> bool:
207
+    """Return true if the warning line is harmless, during config import."""
208
+    possible_warnings = [
209
+        'Replacing invalid value ',
210
+        'Removing ineffective setting ',
211
+        (
212
+            'Setting a global passphrase is ineffective '
213
+            'because a key is also set.'
214
+        ),
215
+        (
216
+            'Setting a service passphrase is ineffective '
217
+            'because a key is also set.'
218
+        ),
219
+    ]
220
+    return any(  # pragma: no branch
221
+        (' Warning: ' + w) in line for w in possible_warnings
222
+    )
223
+
224
+
206 225
 class TestCLI:
207 226
     def test_200_help_output(self, monkeypatch: pytest.MonkeyPatch) -> None:
208 227
         runner = click.testing.CliRunner(mix_stderr=False)
... ...
@@ -454,6 +473,64 @@ class TestCLI:
454 473
             last_line.rstrip(b'\n') == DUMMY_RESULT_KEY1
455 474
         ), 'expected known output'
456 475
 
476
+    @pytest.mark.parametrize(
477
+        'config',
478
+        [
479
+            {
480
+                'services': {
481
+                    DUMMY_SERVICE: {
482
+                        'key': DUMMY_KEY1_B64,
483
+                        **DUMMY_CONFIG_SETTINGS,
484
+                    },
485
+                },
486
+            },
487
+            {
488
+                'global': {'key': DUMMY_KEY1_B64},
489
+                'services': {
490
+                    DUMMY_SERVICE: DUMMY_CONFIG_SETTINGS.copy()
491
+                },
492
+            },
493
+        ],
494
+    )
495
+    def test_206_setting_service_phrase_thus_overriding_key_in_config(
496
+        self,
497
+        monkeypatch: pytest.MonkeyPatch,
498
+        running_ssh_agent: str,
499
+        config: _types.VaultConfig,
500
+    ) -> None:
501
+        with monkeypatch.context():
502
+            monkeypatch.setenv('SSH_AUTH_SOCK', running_ssh_agent)
503
+            monkeypatch.setattr(
504
+                ssh_agent.SSHAgentClient, 'list_keys', tests.list_keys
505
+            )
506
+            monkeypatch.setattr(ssh_agent.SSHAgentClient, 'sign', tests.sign)
507
+            runner = click.testing.CliRunner(mix_stderr=False)
508
+            with tests.isolated_vault_config(
509
+                monkeypatch=monkeypatch,
510
+                runner=runner,
511
+                config=config,
512
+            ):
513
+                _result = runner.invoke(
514
+                    cli.derivepassphrase_vault,
515
+                    ['--config', '-p', '--', DUMMY_SERVICE],
516
+                    input=DUMMY_PASSPHRASE,
517
+                    catch_exceptions=False,
518
+                )
519
+        result = tests.ReadableResult.parse(_result)
520
+        assert result.clean_exit(), 'expected clean exit'
521
+        assert not result.output.strip(), 'expected no program output'
522
+        assert result.stderr, 'expected known error output'
523
+        err_lines = result.stderr.splitlines(False)
524
+        assert err_lines[0].startswith('Passphrase:')
525
+        assert any(  # pragma: no branch
526
+            ' Warning: Setting a service passphrase is ineffective ' in line
527
+            for line in err_lines
528
+        ), 'expected known warning message'
529
+        assert all(  # pragma: no branch
530
+            is_harmless_config_import_warning_line(line)
531
+            for line in result.stderr.splitlines(True)
532
+        ), 'unexpected error output'
533
+
457 534
     @pytest.mark.parametrize(
458 535
         'option',
459 536
         [
... ...
@@ -582,7 +659,7 @@ class TestCLI:
582 659
         'config',
583 660
         [
584 661
             conf.config
585
-            for conf in tests.TEST_CONFIGS
662
+            for conf in TEST_CONFIGS
586 663
             if tests.is_valid_test_config(conf)
587 664
         ],
588 665
     )
... ...
@@ -608,8 +685,12 @@ class TestCLI:
608 685
             ) as infile:
609 686
                 config2 = json.load(infile)
610 687
         result = tests.ReadableResult.parse(_result)
611
-        assert result.clean_exit(empty_stderr=True), 'expected clean exit'
688
+        assert result.clean_exit(empty_stderr=False), 'expected clean exit'
612 689
         assert config2 == config, 'config not imported correctly'
690
+        assert not result.stderr or all(  # pragma: no branch
691
+            is_harmless_config_import_warning_line(line)
692
+            for line in result.stderr.splitlines(True)
693
+        ), 'unexpected error output'
613 694
 
614 695
     @hypothesis.given(
615 696
         conf=tests.smudged_vault_test_config(
... ...
@@ -642,17 +723,10 @@ class TestCLI:
642 723
             ) as infile:
643 724
                 config3 = json.load(infile)
644 725
         result = tests.ReadableResult.parse(_result)
645
-
646
-        def expected_warning_line(line: str) -> bool:
647
-            return (
648
-                ' Warning: Replacing invalid value ' in line
649
-                or ' Warning: Removing ineffective setting ' in line
650
-            )
651
-
652 726
         assert result.clean_exit(empty_stderr=False), 'expected clean exit'
653 727
         assert config3 == config2, 'config not imported correctly'
654 728
         assert not result.stderr or all(
655
-            expected_warning_line(line)
729
+            is_harmless_config_import_warning_line(line)
656 730
             for line in result.stderr.splitlines(True)
657 731
         ), 'unexpected error output'
658 732
 
... ...
@@ -925,7 +999,10 @@ contents go here
925 999
             (
926 1000
                 ['--key'],
927 1001
                 '1\n',
928
-                {'global': {'key': DUMMY_KEY1_B64}, 'services': {}},
1002
+                {
1003
+                    'global': {'key': DUMMY_KEY1_B64, 'phrase': 'abc'},
1004
+                    'services': {},
1005
+                },
929 1006
             ),
930 1007
             (
931 1008
                 ['--phrase', 'sv'],
932 1009