Fix empty key handling in `vault` config, and harmonize warning text (c9e143e) - derivepassphrase.git

Fix empty key handling in `vault` config, and harmonize warning text

Marco Ricci commited on 2024-12-20 11:39:45
Zeige 3 geänderte Dateien mit 73 Einfügungen und 8 Löschungen.

Correctly handle empty `key` settings in the `vault` configuration: if
at the global level, it is equivalent to eliding the setting, but at the
service level, it explicitly disables key usage (even if a global key is
set).  This implies that both cases need different cleanup steps.

Harmonize the warning message for ineffective passphrases for services:
always include the affected service name.

If during configuration import we find a key shadowing a passphrase,
issue the same ineffective passphrase warning as would happen during
interactive configuration or interactive use.

(The tests need minor modifications and reparametrization to continue to
cover all warning cases.)

src/derivepassphrase/_types.py 6359641..2efb9e7
src/derivepassphrase/cli.py bc254d1..cb15d8e
tests/test_derivepassphrase_cli.py e2b5cfd..6f72f1e

src/derivepassphrase/_types.py

Zeige Datei @ c9e143e

@@ -517,7 +517,7 @@ def clean_up_falsy_vault_config_values(  # noqa: C901,PLR0912
                         )
                     )
                     service_obj[key] = ''
-            elif key in {'notes', 'key'}:
+            elif key == 'notes':
                 if not js_truthiness(value):
                     cleanup_completed.append(
                         CleanupStep(
@@ -525,6 +525,22 @@ def clean_up_falsy_vault_config_values(  # noqa: C901,PLR0912
                         )
                     )
                     service_obj.pop(key)
+            elif key == 'key':
+                if not js_truthiness(value):
+                    if path == ['global']:
+                        cleanup_completed.append(
+                            CleanupStep(
+                                (*path, key), service_obj[key], 'remove', None
+                            )
+                        )
+                        service_obj.pop(key)
+                    else:
+                        cleanup_completed.append(
+                            CleanupStep(
+                                (*path, key), service_obj[key], 'replace', ''
+                            )
+                        )
+                        service_obj[key] = ''
             elif key == 'length':
                 if not js_truthiness(value):
                     cleanup_completed.append(

src/derivepassphrase/cli.py

Zeige Datei @ c9e143e

@@ -2025,6 +2025,29 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
                 )
         except AssertionError as e:
             err('The configuration file is invalid.  ' + str(e))
+        global_obj = maybe_config.get('global', {})
+        has_key = _types.js_truthiness(global_obj.get('key'))
+        has_phrase = _types.js_truthiness(global_obj.get('phrase'))
+        if has_key and has_phrase:
+            logger.warning(
+                'Setting a global passphrase is ineffective '
+                'because a key is also set.'
+            )
+        for service_name, service_obj in maybe_config['services'].items():
+            has_key = _types.js_truthiness(
+                service_obj.get('key')
+            ) or _types.js_truthiness(global_obj.get('key'))
+            has_phrase = _types.js_truthiness(
+                service_obj.get('phrase')
+            ) or _types.js_truthiness(global_obj.get('phrase'))
+            if has_key and has_phrase:
+                logger.warning(
+                    (
+                        'Setting a service passphrase is ineffective '
+                        'because a key is also set: %s'
+                    ),
+                    json.dumps(service_name),
+                )
         if overwrite_config:
             put_config(maybe_config)
         else:
@@ -2153,12 +2176,18 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
                 except AssertionError as e:
                     err('The configuration file is invalid.  ' + str(e))
                 if 'key' in settings:
+                    if service:
                         logger.warning(
                             (
-                            'Setting a %s passphrase is ineffective '
-                            'because a key is also set.'
+                                'Setting a service passphrase is ineffective '
+                                'because a key is also set: %s'
                             ),
-                        settings_type,
+                            json.dumps(service),
+                        )
+                    else:
+                        logger.warning(
+                            'Setting a global passphrase is ineffective '
+                            'because a key is also set.'
                         )
             if not view.maps[0]:
                 settings_type = 'service' if service else 'global'

tests/test_derivepassphrase_cli.py

Zeige Datei @ c9e143e

@@ -221,7 +221,7 @@ def is_harmless_config_import_warning(record: tuple[str, int, str]) -> bool:
         ),
         (
             'Setting a service passphrase is ineffective '
-            'because a key is also set.'
+            'because a key is also set:'
         ),
     ]
     return any(tests.warning_emitted(w, [record]) for w in possible_warnings)
@@ -479,8 +479,17 @@ class TestCLI:
         ), 'expected known output'
 
     @pytest.mark.parametrize(
-        'config',
+        ['config', 'command_line'],
         [
+            pytest.param(
+                {
+                    'global': {'key': DUMMY_KEY1_B64},
+                    'services': {},
+                },
+                ['--config', '-p'],
+                id='global',
+            ),
+            pytest.param(
                 {
                     'services': {
                         DUMMY_SERVICE: {
@@ -489,18 +498,26 @@ class TestCLI:
                         },
                     },
                 },
+                ['--config', '-p', '--', DUMMY_SERVICE],
+                id='service',
+            ),
+            pytest.param(
                 {
                     'global': {'key': DUMMY_KEY1_B64},
                     'services': {DUMMY_SERVICE: DUMMY_CONFIG_SETTINGS.copy()},
                 },
+                ['--config', '-p', '--', DUMMY_SERVICE],
+                id='service-over-global',
+            ),
         ],
     )
-    def test_206_setting_service_phrase_thus_overriding_key_in_config(
+    def test_206_setting_phrase_thus_overriding_key_in_config(
         self,
         monkeypatch: pytest.MonkeyPatch,
         running_ssh_agent: tests.RunningSSHAgentInfo,
         caplog: pytest.LogCaptureFixture,
         config: _types.VaultConfig,
+        command_line: list[str],
     ) -> None:
         with monkeypatch.context():
             monkeypatch.setenv('SSH_AUTH_SOCK', running_ssh_agent.socket)
@@ -516,7 +533,7 @@ class TestCLI:
             ):
                 _result = runner.invoke(
                     cli.derivepassphrase_vault,
-                    ['--config', '-p', '--', DUMMY_SERVICE],
+                    command_line,
                     input=DUMMY_PASSPHRASE,
                     catch_exceptions=False,
                 )
@@ -529,6 +546,9 @@ class TestCLI:
         assert tests.warning_emitted(
             'Setting a service passphrase is ineffective ',
             caplog.record_tuples,
+        ) or tests.warning_emitted(
+            'Setting a global passphrase is ineffective ',
+            caplog.record_tuples,
         ), 'expected known warning message'
         assert all(map(is_warning_line, result.stderr.splitlines(True)))
         assert all(


...	...	@@ -517,7 +517,7 @@ def clean_up_falsy_vault_config_values( # noqa: C901,PLR0912
517	517	)
518	518	)
519	519	service_obj[key] = ''
520		- elif key in {'notes', 'key'}:
	520	+ elif key == 'notes':
521	521	if not js_truthiness(value):
522	522	cleanup_completed.append(
523	523	CleanupStep(
...	...	@@ -525,6 +525,22 @@ def clean_up_falsy_vault_config_values( # noqa: C901,PLR0912
525	525	)
526	526	)
527	527	service_obj.pop(key)
	528	+ elif key == 'key':
	529	+ if not js_truthiness(value):
	530	+ if path == ['global']:
	531	+ cleanup_completed.append(
	532	+ CleanupStep(
	533	+ (*path, key), service_obj[key], 'remove', None
	534	+ )
	535	+ )
	536	+ service_obj.pop(key)
	537	+ else:
	538	+ cleanup_completed.append(
	539	+ CleanupStep(
	540	+ (*path, key), service_obj[key], 'replace', ''
	541	+ )
	542	+ )
	543	+ service_obj[key] = ''
528	544	elif key == 'length':
529	545	if not js_truthiness(value):
530	546	cleanup_completed.append(

...	...	@@ -2025,6 +2025,29 @@ def derivepassphrase_vault( # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
2025	2025	)
2026	2026	except AssertionError as e:
2027	2027	err('The configuration file is invalid. ' + str(e))
	2028	+ global_obj = maybe_config.get('global', {})
	2029	+ has_key = _types.js_truthiness(global_obj.get('key'))
	2030	+ has_phrase = _types.js_truthiness(global_obj.get('phrase'))
	2031	+ if has_key and has_phrase:
	2032	+ logger.warning(
	2033	+ 'Setting a global passphrase is ineffective '
	2034	+ 'because a key is also set.'
	2035	+ )
	2036	+ for service_name, service_obj in maybe_config['services'].items():
	2037	+ has_key = _types.js_truthiness(
	2038	+ service_obj.get('key')
	2039	+ ) or _types.js_truthiness(global_obj.get('key'))
	2040	+ has_phrase = _types.js_truthiness(
	2041	+ service_obj.get('phrase')
	2042	+ ) or _types.js_truthiness(global_obj.get('phrase'))
	2043	+ if has_key and has_phrase:
	2044	+ logger.warning(
	2045	+ (
	2046	+ 'Setting a service passphrase is ineffective '
	2047	+ 'because a key is also set: %s'
	2048	+ ),
	2049	+ json.dumps(service_name),
	2050	+ )
2028	2051	if overwrite_config:
2029	2052	put_config(maybe_config)
2030	2053	else:
...	...	@@ -2153,12 +2176,18 @@ def derivepassphrase_vault( # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
2153	2176	except AssertionError as e:
2154	2177	err('The configuration file is invalid. ' + str(e))
2155	2178	if 'key' in settings:
	2179	+ if service:
2156	2180	logger.warning(
2157	2181	(
2158		- 'Setting a %s passphrase is ineffective '
2159		- 'because a key is also set.'
	2182	+ 'Setting a service passphrase is ineffective '
	2183	+ 'because a key is also set: %s'
2160	2184	),
2161		- settings_type,
	2185	+ json.dumps(service),
	2186	+ )
	2187	+ else:
	2188	+ logger.warning(
	2189	+ 'Setting a global passphrase is ineffective '
	2190	+ 'because a key is also set.'
2162	2191	)
2163	2192	if not view.maps[0]:
2164	2193	settings_type = 'service' if service else 'global'

...	...	@@ -221,7 +221,7 @@ def is_harmless_config_import_warning(record: tuple[str, int, str]) -> bool:
221	221	),
222	222	(
223	223	'Setting a service passphrase is ineffective '
224		- 'because a key is also set.'
	224	+ 'because a key is also set:'
225	225	),
226	226	]
227	227	return any(tests.warning_emitted(w, [record]) for w in possible_warnings)
...	...	@@ -479,8 +479,17 @@ class TestCLI:
479	479	), 'expected known output'
480	480
481	481	@pytest.mark.parametrize(
482		- 'config',
	482	+ ['config', 'command_line'],
483	483	[
	484	+ pytest.param(
	485	+ {
	486	+ 'global': {'key': DUMMY_KEY1_B64},
	487	+ 'services': {},
	488	+ },
	489	+ ['--config', '-p'],
	490	+ id='global',
	491	+ ),
	492	+ pytest.param(
484	493	{
485	494	'services': {
486	495	DUMMY_SERVICE: {
...	...	@@ -489,18 +498,26 @@ class TestCLI:
489	498	},
490	499	},
491	500	},
	501	+ ['--config', '-p', '--', DUMMY_SERVICE],
	502	+ id='service',
	503	+ ),
	504	+ pytest.param(
492	505	{
493	506	'global': {'key': DUMMY_KEY1_B64},
494	507	'services': {DUMMY_SERVICE: DUMMY_CONFIG_SETTINGS.copy()},
495	508	},
	509	+ ['--config', '-p', '--', DUMMY_SERVICE],
	510	+ id='service-over-global',
	511	+ ),
496	512	],
497	513	)
498		- def test_206_setting_service_phrase_thus_overriding_key_in_config(
	514	+ def test_206_setting_phrase_thus_overriding_key_in_config(
499	515	self,
500	516	monkeypatch: pytest.MonkeyPatch,
501	517	running_ssh_agent: tests.RunningSSHAgentInfo,
502	518	caplog: pytest.LogCaptureFixture,
503	519	config: _types.VaultConfig,
	520	+ command_line: list[str],
504	521	) -> None:
505	522	with monkeypatch.context():
506	523	monkeypatch.setenv('SSH_AUTH_SOCK', running_ssh_agent.socket)
...	...	@@ -516,7 +533,7 @@ class TestCLI:
516	533	):
517	534	_result = runner.invoke(
518	535	cli.derivepassphrase_vault,
519		- ['--config', '-p', '--', DUMMY_SERVICE],
	536	+ command_line,
520	537	input=DUMMY_PASSPHRASE,
521	538	catch_exceptions=False,
522	539	)
...	...	@@ -529,6 +546,9 @@ class TestCLI:
529	546	assert tests.warning_emitted(
530	547	'Setting a service passphrase is ineffective ',
531	548	caplog.record_tuples,
	549	+ ) or tests.warning_emitted(
	550	+ 'Setting a global passphrase is ineffective ',
	551	+ caplog.record_tuples,
532	552	), 'expected known warning message'
533	553	assert all(map(is_warning_line, result.stderr.splitlines(True)))
534	554	assert all(
535	555