From cf89a160b05e5f55165414d27e7312a855797cd6 Mon Sep 17 00:00:00 2001 From: mulhern Date: Tue, 6 Jan 2026 17:37:19 -0500 Subject: [PATCH 01/18] Only match org.freedesktop.zbus.Error for property setting At present, the zbus D-Bus error is the only error that can be returned when setting a property. Signed-off-by: mulhern --- src/stratis_cli/_error_reporting.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/stratis_cli/_error_reporting.py b/src/stratis_cli/_error_reporting.py index f637f47b9..f0158f2a7 100644 --- a/src/stratis_cli/_error_reporting.py +++ b/src/stratis_cli/_error_reporting.py @@ -288,7 +288,7 @@ def _interpret_errors_2( # pylint: disable=too-many-return-statements if dbus_name == "org.freedesktop.zbus.Error" and isinstance( context, DPClientSetPropertyContext - ): # pragma: no cover + ): return ( f"stratisd failed to perform the operation that you " f"requested, because it could not set the D-Bus " @@ -335,15 +335,6 @@ def _interpret_errors_2( # pylint: disable=too-many-return-statements f"service sent the following message: {dbus_message}." ) - if isinstance(context, DPClientSetPropertyContext): # pragma: no cover - return ( - f"stratisd failed to perform the operation that you " - f"requested, because it could not set the D-Bus " - f'property "{context.property_name}" belonging to ' - f'interface "{error.interface_name}" to "{context.value}". ' - f"It returned the following error: {dbus_message}." - ) - if isinstance(context, DPClientGetPropertyContext): # pragma: no cover return ( f"stratisd failed to perform the operation that you " From a07c82cb7afb8e80b12aeabd42413ce51c37396d Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 7 Jan 2026 12:23:59 -0500 Subject: [PATCH 02/18] TEMPORARY: remove manual trigger Signed-off-by: mulhern --- .packit.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.packit.yaml b/.packit.yaml index 76f4e60ad..917ba5d56 100644 --- a/.packit.yaml +++ b/.packit.yaml @@ -27,7 +27,6 @@ jobs: - job: copr_build trigger: pull_request identifier: copr_pull - manual_trigger: true targets: - fedora-all From f04880ea5661d51bd67d227cbd5660a470b4e0d9 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 24 Mar 2025 20:38:32 -0400 Subject: [PATCH 03/18] Update introspection data Signed-off-by: mulhern --- src/stratis_cli/_actions/_introspect.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/stratis_cli/_actions/_introspect.py b/src/stratis_cli/_actions/_introspect.py index b3a742d91..afd151211 100644 --- a/src/stratis_cli/_actions/_introspect.py +++ b/src/stratis_cli/_actions/_introspect.py @@ -177,12 +177,24 @@ + + + + + + + + + + + + @@ -221,6 +233,11 @@ + + + + + @@ -249,13 +266,12 @@ - - - + + From a83665033329d98dd742e85af6bfccb1dcf9f3ba Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 2 Apr 2025 21:29:58 -0400 Subject: [PATCH 04/18] Set up clevis + keyring options for sharing Signed-off-by: mulhern --- src/stratis_cli/_parser/_pool.py | 31 ++---------------------------- src/stratis_cli/_parser/_shared.py | 24 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/stratis_cli/_parser/_pool.py b/src/stratis_cli/_parser/_pool.py index a3026a84f..485ef8afc 100644 --- a/src/stratis_cli/_parser/_pool.py +++ b/src/stratis_cli/_parser/_pool.py @@ -26,7 +26,6 @@ from .._actions import BindActions, PoolActions from .._alerts import PoolAlert from .._constants import ( - Clevis, EncryptionMethod, IntegrityOption, IntegrityTagSpec, @@ -36,6 +35,7 @@ from ._debug import POOL_DEBUG_SUBCMDS from ._encryption import BIND_SUBCMDS, ENCRYPTION_SUBCMDS, REBIND_SUBCMDS from ._shared import ( + CLEVIS_AND_KERNEL, KEYFILE_PATH_OR_STDIN, TRUST_URL_OR_THUMBPRINT, UUID_OR_NAME, @@ -114,34 +114,7 @@ def verify(self, namespace: Namespace, parser: ArgumentParser): "Encryption", { "description": "Arguments controlling creation with encryption", - "args": [ - ( - "--key-desc", - { - "help": ( - "Key description of key in kernel keyring to use " - "for encryption" - ), - }, - ), - ( - "--clevis", - { - "type": Clevis, - "help": "Specification for binding with Clevis.", - "choices": list(Clevis), - }, - ), - ( - "--tang-url", - { - "help": ( - "URL of Clevis tang server " - "(--clevis=[tang|nbde] must be set)" - ), - }, - ), - ], + "args": CLEVIS_AND_KERNEL, }, ), ( diff --git a/src/stratis_cli/_parser/_shared.py b/src/stratis_cli/_parser/_shared.py index 91fddd617..b91fe0d38 100644 --- a/src/stratis_cli/_parser/_shared.py +++ b/src/stratis_cli/_parser/_shared.py @@ -273,3 +273,27 @@ def verify(self, namespace, parser): else ClevisInfo(CLEVIS_PIN_TPM2, {}) ) ) + + +CLEVIS_AND_KERNEL = [ + ( + "--key-desc", + { + "help": ("Key description of key in kernel keyring"), + }, + ), + ( + "--clevis", + { + "type": Clevis, + "help": "Specification for binding with Clevis.", + "choices": list(Clevis), + }, + ), + ( + "--tang-url", + { + "help": "URL of Clevis tang server (--clevis=[tang|nbde] must be set)", + }, + ), +] From 98d9700a3f191e8347dbab7fd63db599d094e7b0 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 24 Mar 2025 21:05:36 -0400 Subject: [PATCH 05/18] Add commands for encrypt, re-encrypt, and unencrypt Signed-off-by: mulhern --- src/stratis_cli/_actions/__init__.py | 1 + src/stratis_cli/_actions/_crypt.py | 165 +++++++++++++++++ src/stratis_cli/_parser/_encryption.py | 82 ++++++++- tests/integration/pool/test_encryption.py | 212 ++++++++++++++++++++++ 4 files changed, 459 insertions(+), 1 deletion(-) create mode 100644 src/stratis_cli/_actions/_crypt.py diff --git a/src/stratis_cli/_actions/__init__.py b/src/stratis_cli/_actions/__init__.py index b95c02e1c..f16cf1953 100644 --- a/src/stratis_cli/_actions/__init__.py +++ b/src/stratis_cli/_actions/__init__.py @@ -22,6 +22,7 @@ MANAGER_0_INTERFACE, POOL_INTERFACE, ) +from ._crypt import CryptActions from ._debug import ( BlockdevDebugActions, FilesystemDebugActions, diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py new file mode 100644 index 000000000..055f3bf9f --- /dev/null +++ b/src/stratis_cli/_actions/_crypt.py @@ -0,0 +1,165 @@ +# Copyright 2025 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Miscellaneous whole pool encryption actions. +""" + +# isort: STDLIB +import json +from argparse import Namespace + +from .._constants import PoolId +from .._errors import ( + StratisCliEngineError, + StratisCliIncoherenceError, + StratisCliNoChangeError, +) +from .._stratisd_constants import StratisdErrors +from ._connection import get_object +from ._constants import TOP_OBJECT + + +class CryptActions: + """ + Whole pool encryption actions. + """ + + @staticmethod + def encrypt(namespace: Namespace): + """ + Encrypt a previously unencrypted pool. + """ + + # pylint: disable=import-outside-toplevel + from ._data import MOPool, ObjectManager, Pool, pools + + pool_id = PoolId.from_parser_namespace(namespace) + assert pool_id is not None + + proxy = get_object(TOP_OBJECT) + + managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {}) + + (pool_object_path, mopool) = next( + pools(props=pool_id.managed_objects_key()) + .require_unique_match(True) + .search(managed_objects) + ) + + if bool(MOPool(mopool).Encrypted()): + raise StratisCliNoChangeError("encryption on", pool_id) + + (changed, return_code, message) = Pool.Methods.EncryptPool( + get_object(pool_object_path), + { + "key_descs": ( + [] + if namespace.key_desc is None + else [((False, 0), namespace.key_desc)] + ), + "clevis_infos": ( + [] + if namespace.clevis is None + else [ + ( + (False, 0), + namespace.clevis.pin, + json.dumps(namespace.clevis.config), + ) + ] + ), + }, + ) + + if return_code != StratisdErrors.OK: + raise StratisCliEngineError(return_code, message) + + if not changed: # pragma: no cover + raise StratisCliIncoherenceError( + f"Expected to change {pool_id} encryption status to " + "encrypted, but stratisd reports that it did not change " + "the encryption status" + ) + + @staticmethod + def unencrypt(namespace: Namespace): + """ + Unencrypt a previously encrypted pool. + """ + # pylint: disable=import-outside-toplevel + from ._data import MOPool, ObjectManager, Pool, pools + + pool_id = PoolId.from_parser_namespace(namespace) + assert pool_id is not None + + proxy = get_object(TOP_OBJECT) + + managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {}) + + (pool_object_path, mopool) = next( + pools(props=pool_id.managed_objects_key()) + .require_unique_match(True) + .search(managed_objects) + ) + + if not bool(MOPool(mopool).Encrypted()): + raise StratisCliNoChangeError("encryption off", pool_id) + + (changed, return_code, message) = Pool.Methods.DecryptPool( + get_object(pool_object_path), {} + ) + + if return_code != StratisdErrors.OK: # pragma: no cover + raise StratisCliEngineError(return_code, message) + + if not changed: # pragma: no cover + raise StratisCliIncoherenceError( + f"Expected to change {pool_id} encryption status to " + "unencrypted, but stratisd reports that it did not change " + "the pool's encryption status" + ) + + @staticmethod + def reencrypt(namespace: Namespace): + """ + Reencrypt an already encrypted pool with a new key. + """ + # pylint: disable=import-outside-toplevel + from ._data import ObjectManager, Pool, pools + + pool_id = PoolId.from_parser_namespace(namespace) + assert pool_id is not None + + proxy = get_object(TOP_OBJECT) + + managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {}) + + (pool_object_path, _) = next( + pools(props=pool_id.managed_objects_key()) + .require_unique_match(True) + .search(managed_objects) + ) + + (changed, return_code, message) = Pool.Methods.ReencryptPool( + get_object(pool_object_path), {} + ) + + if return_code != StratisdErrors.OK: # pragma: no cover + raise StratisCliEngineError(return_code, message) + + if not changed: # pragma: no cover + raise StratisCliIncoherenceError( + f"Expected to reencrypt {pool_id} with a new key but stratisd " + "reports that it did not perform the operation" + ) diff --git a/src/stratis_cli/_parser/_encryption.py b/src/stratis_cli/_parser/_encryption.py index a3a76d3b3..1ca065323 100644 --- a/src/stratis_cli/_parser/_encryption.py +++ b/src/stratis_cli/_parser/_encryption.py @@ -19,9 +19,10 @@ import copy from argparse import SUPPRESS, Namespace -from .._actions import BindActions, RebindActions +from .._actions import BindActions, CryptActions, RebindActions from .._constants import Clevis, EncryptionMethod from ._shared import ( + CLEVIS_AND_KERNEL, TRUST_URL_OR_THUMBPRINT, UUID_OR_NAME, ClevisEncryptionOptions, @@ -352,6 +353,85 @@ def __init__(self, namespace: Namespace): ] ENCRYPTION_SUBCMDS = [ + ( + "on", + { + "help": "Make encrypted a previously unencrypted pool", + "args": [ + ( + "--post-parser", + { + "action": RejectAction, + "default": ClevisEncryptionOptions, + "help": SUPPRESS, + "nargs": "?", + }, + ) + ], + "groups": [ + ( + "Pool Identifier", + { + "description": "Choose one option to specify the pool", + "mut_ex_args": [ + (True, UUID_OR_NAME), + ], + }, + ), + ( + "Encryption", + { + "description": "Arguments controlling encryption", + "args": CLEVIS_AND_KERNEL, + }, + ), + ( + "Tang Server Verification (only if --tang-url option is set)", + { + "description": "Choose one option", + "mut_ex_args": [(False, TRUST_URL_OR_THUMBPRINT)], + }, + ), + ], + "func": CryptActions.encrypt, + }, + ), + ( + "off", + { + "help": "Make unencrypted a previously encrypted pool", + "groups": [ + ( + "Pool Identifier", + { + "description": "Choose one option to specify the pool", + "mut_ex_args": [ + (True, UUID_OR_NAME), + ], + }, + ) + ], + "func": CryptActions.unencrypt, + }, + ), + ( + "reencrypt", + { + "help": "Reencrypt an encrypted pool with a new master key", + "groups": [ + ( + "Pool Identifier", + { + "description": "Choose one option to specify the pool", + "mut_ex_args": [ + (True, UUID_OR_NAME), + ], + }, + ) + ], + "func": CryptActions.reencrypt, + }, + ), ( "bind", { diff --git a/tests/integration/pool/test_encryption.py b/tests/integration/pool/test_encryption.py index 40c76c1fa..ea7af7e49 100644 --- a/tests/integration/pool/test_encryption.py +++ b/tests/integration/pool/test_encryption.py @@ -424,3 +424,215 @@ def test_unbind_with_unused_token_slot(self): "--token-slot=32", ] self.check_error(StratisCliNoChangeError, command_line, _ERROR) + + +class OffTestCase(SimTestCase): + """ + Test turning encryption off when pool is encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "off"] + _POOLNAME = "poolname" + _KEY_DESC = "keydesc" + + def setUp(self): + super().setUp() + with RandomKeyTmpFile() as fname: + command_line = [ + "--propagate", + "key", + "set", + "--keyfile-path", + fname, + self._KEY_DESC, + ] + RUNNER(command_line) + + command_line = [ + "--propagate", + "pool", + "create", + "--key-desc", + self._KEY_DESC, + self._POOLNAME, + ] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_decrypt_with_name(self): + """ + Decrypt an encrypted pool, specifying the pool by name. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + ] + RUNNER(command_line) + + +class OffTestCase2(SimTestCase): + """ + Test turning encryption off when pool is not encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "off"] + _POOLNAME = "poolname" + + def setUp(self): + super().setUp() + command_line = ["pool", "create", self._POOLNAME] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_decrypt_with_name(self): + """ + Decrypting when unencrypted should return an error. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + ] + self.check_error(StratisCliNoChangeError, command_line, _ERROR) + + +class ReencryptTestCase(SimTestCase): + """ + Test re-encrypting when pool is encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "reencrypt"] + _POOLNAME = "poolname" + _KEY_DESC = "keydesc" + + def setUp(self): + super().setUp() + with RandomKeyTmpFile() as fname: + command_line = [ + "--propagate", + "key", + "set", + "--keyfile-path", + fname, + self._KEY_DESC, + ] + RUNNER(command_line) + + command_line = [ + "--propagate", + "pool", + "create", + "--key-desc", + self._KEY_DESC, + self._POOLNAME, + ] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_reencrypt_with_name(self): + """ + Re-encrypt an encrypted pool, specifying the pool by name. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + ] + RUNNER(command_line) + + +class ReencryptTestCase2(SimTestCase): + """ + Test reencryption when pool is not encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "reencrypt"] + _POOLNAME = "poolname" + + def setUp(self): + super().setUp() + command_line = ["pool", "create", self._POOLNAME] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_reencrypt_with_name(self): + """ + Reencrypting when unencrypted should return an error. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + ] + self.check_error(StratisCliEngineError, command_line, _ERROR) + + +class EncryptTestCase(SimTestCase): + """ + Test encrypting when pool is already encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "on"] + _POOLNAME = "poolname" + _KEY_DESC = "keydesc" + + def setUp(self): + super().setUp() + with RandomKeyTmpFile() as fname: + command_line = [ + "--propagate", + "key", + "set", + "--keyfile-path", + fname, + self._KEY_DESC, + ] + RUNNER(command_line) + + command_line = [ + "--propagate", + "pool", + "create", + "--key-desc", + self._KEY_DESC, + self._POOLNAME, + ] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_encrypt_with_name(self): + """ + Encrypting when already encrypted should return an error. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + "--clevis=tpm2", + ] + self.check_error(StratisCliNoChangeError, command_line, _ERROR) + + +class EncryptTestCase2(SimTestCase): + """ + Test encrypting when pool is not already encrypted. + """ + + _MENU = ["--propagate", "pool", "encryption", "on"] + _POOLNAME = "poolname" + _KEY_DESC = "keydesc" + + def setUp(self): + super().setUp() + command_line = [ + "--propagate", + "pool", + "create", + self._POOLNAME, + ] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_encrypt_with_name(self): + """ + Encrypting when not already encrypted should succeed. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + "--clevis=tpm2", + ] + TEST_RUNNER(command_line) + + def test_encryption_with_no_encryption_params(self): + """ + Encrypting without any encryption method fully specified should fail. + """ + command_line = self._MENU + [ + f"--name={self._POOLNAME}", + ] + self.check_error(StratisCliEngineError, command_line, _ERROR) From a3b86ee36dd1ac1b5e1ff8a337e74fe604258135 Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 16 Apr 2025 09:36:02 -0400 Subject: [PATCH 06/18] Test parsing clevis options in encryption on Signed-off-by: mulhern --- tests/integration/test_parser.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_parser.py b/tests/integration/test_parser.py index 3a0125a0d..2953a64c3 100644 --- a/tests/integration/test_parser.py +++ b/tests/integration/test_parser.py @@ -241,14 +241,18 @@ def test_print_all_help(self): class TestClevisOptions(ParserTestCase): """ - Verify that invalid clevis encryption create options are detected. + Verify that invalid clevis encryption options are detected. """ def _do_clevis_test(self, clevis_args): """ Apply clevis args to create command_line and verify parser error. """ - self._do_test(["pool", "create", "pn", "/dev/n"] + clevis_args) + for subcommand in [ + ["pool", "create", "pn", "/dev/n"], + ["pool", "encryption", "on", "--name=pn"], + ]: + self._do_test(subcommand + clevis_args) def test_create_with_clevis_1(self): """ From 550d4bbaff4133c3374282109fb3e41ff904d2fb Mon Sep 17 00:00:00 2001 From: mulhern Date: Tue, 15 Jul 2025 16:18:57 -0400 Subject: [PATCH 07/18] Add an --in-place option Signed-off-by: mulhern --- src/stratis_cli/_actions/_crypt.py | 10 +++ src/stratis_cli/_errors.py | 16 +++++ src/stratis_cli/_parser/_encryption.py | 6 +- src/stratis_cli/_parser/_shared.py | 14 +++++ tests/integration/pool/test_encryption.py | 77 ++++++++++++++++++++--- 5 files changed, 115 insertions(+), 8 deletions(-) diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 055f3bf9f..904cb55a5 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -23,6 +23,7 @@ from .._errors import ( StratisCliEngineError, StratisCliIncoherenceError, + StratisCliInPlaceNotSpecified, StratisCliNoChangeError, ) from .._stratisd_constants import StratisdErrors @@ -41,6 +42,9 @@ def encrypt(namespace: Namespace): Encrypt a previously unencrypted pool. """ + if not namespace.in_place: + raise StratisCliInPlaceNotSpecified() + # pylint: disable=import-outside-toplevel from ._data import MOPool, ObjectManager, Pool, pools @@ -97,6 +101,9 @@ def unencrypt(namespace: Namespace): """ Unencrypt a previously encrypted pool. """ + if not namespace.in_place: + raise StratisCliInPlaceNotSpecified() + # pylint: disable=import-outside-toplevel from ._data import MOPool, ObjectManager, Pool, pools @@ -135,6 +142,9 @@ def reencrypt(namespace: Namespace): """ Reencrypt an already encrypted pool with a new key. """ + if not namespace.in_place: + raise StratisCliInPlaceNotSpecified() + # pylint: disable=import-outside-toplevel from ._data import ObjectManager, Pool, pools diff --git a/src/stratis_cli/_errors.py b/src/stratis_cli/_errors.py index b60731efb..1e6c7fe1a 100644 --- a/src/stratis_cli/_errors.py +++ b/src/stratis_cli/_errors.py @@ -454,3 +454,19 @@ def __init__(self, msg): def __str__(self): return self.msg + + +class StratisCliInPlaceNotSpecified(StratisCliUserError): + """ + Raised if the user requested in-place encryption but did not use the + --in-place option. + """ + + def __str__(self) -> str: + return ( + "Specify the --in-place option to demonstrate that you " + "understand the special nature of the procedure that you are " + "about to initiate. Please refer to the discussion of the " + '"--in-place" option in the man pages for further ' + "information." + ) diff --git a/src/stratis_cli/_parser/_encryption.py b/src/stratis_cli/_parser/_encryption.py index 1ca065323..8ad275540 100644 --- a/src/stratis_cli/_parser/_encryption.py +++ b/src/stratis_cli/_parser/_encryption.py @@ -23,6 +23,7 @@ from .._constants import Clevis, EncryptionMethod from ._shared import ( CLEVIS_AND_KERNEL, + IN_PLACE, TRUST_URL_OR_THUMBPRINT, UUID_OR_NAME, ClevisEncryptionOptions, @@ -367,7 +368,8 @@ def __init__(self, namespace: Namespace): "nargs": "?", }, ) - ], + ] + + IN_PLACE, "groups": [ ( "Pool Identifier", @@ -400,6 +402,7 @@ def __init__(self, namespace: Namespace): "off", { "help": "Make unencrypted a previously encrypted pool", + "args": IN_PLACE, "groups": [ ( "Pool Identifier", @@ -418,6 +421,7 @@ def __init__(self, namespace: Namespace): "reencrypt", { "help": "Reencrypt an encrypted pool with a new master key", + "args": IN_PLACE, "groups": [ ( "Pool Identifier", diff --git a/src/stratis_cli/_parser/_shared.py b/src/stratis_cli/_parser/_shared.py index b91fe0d38..2696e8fdd 100644 --- a/src/stratis_cli/_parser/_shared.py +++ b/src/stratis_cli/_parser/_shared.py @@ -297,3 +297,17 @@ def verify(self, namespace, parser): }, ), ] + +IN_PLACE = [ + ( + "--in-place", + { + "action": "store_true", + "help": ( + "Perform the operation in place; requires no additional " + "devices; see man page entry for this option for more " + "information" + ), + }, + ) +] diff --git a/tests/integration/pool/test_encryption.py b/tests/integration/pool/test_encryption.py index ea7af7e49..f646c5cce 100644 --- a/tests/integration/pool/test_encryption.py +++ b/tests/integration/pool/test_encryption.py @@ -17,7 +17,11 @@ # isort: LOCAL from stratis_cli import StratisCliErrorCodes -from stratis_cli._errors import StratisCliEngineError, StratisCliNoChangeError +from stratis_cli._errors import ( + StratisCliEngineError, + StratisCliInPlaceNotSpecified, + StratisCliNoChangeError, +) from .._keyutils import RandomKeyTmpFile from .._misc import RUNNER, TEST_RUNNER, SimTestCase, device_name_list @@ -431,7 +435,7 @@ class OffTestCase(SimTestCase): Test turning encryption off when pool is encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "off"] + _MENU = ["--propagate", "pool", "encryption", "off", "--in-place"] _POOLNAME = "poolname" _KEY_DESC = "keydesc" @@ -473,7 +477,7 @@ class OffTestCase2(SimTestCase): Test turning encryption off when pool is not encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "off"] + _MENU = ["--propagate", "pool", "encryption", "off", "--in-place"] _POOLNAME = "poolname" def setUp(self): @@ -496,7 +500,7 @@ class ReencryptTestCase(SimTestCase): Test re-encrypting when pool is encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "reencrypt"] + _MENU = ["--propagate", "pool", "encryption", "reencrypt", "--in-place"] _POOLNAME = "poolname" _KEY_DESC = "keydesc" @@ -538,7 +542,7 @@ class ReencryptTestCase2(SimTestCase): Test reencryption when pool is not encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "reencrypt"] + _MENU = ["--propagate", "pool", "encryption", "reencrypt", "--in-place"] _POOLNAME = "poolname" def setUp(self): @@ -561,7 +565,7 @@ class EncryptTestCase(SimTestCase): Test encrypting when pool is already encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "on"] + _MENU = ["--propagate", "pool", "encryption", "on", "--in-place"] _POOLNAME = "poolname" _KEY_DESC = "keydesc" @@ -604,7 +608,7 @@ class EncryptTestCase2(SimTestCase): Test encrypting when pool is not already encrypted. """ - _MENU = ["--propagate", "pool", "encryption", "on"] + _MENU = ["--propagate", "pool", "encryption", "on", "--in-place"] _POOLNAME = "poolname" _KEY_DESC = "keydesc" @@ -636,3 +640,62 @@ def test_encryption_with_no_encryption_params(self): f"--name={self._POOLNAME}", ] self.check_error(StratisCliEngineError, command_line, _ERROR) + + +class NoInPlaceTestCase(SimTestCase): + """ + Test encrypting when pool is not already encrypted. + """ + + _POOLNAME = "poolname" + _KEY_DESC = "keydesc" + + def setUp(self): + super().setUp() + command_line = [ + "--propagate", + "pool", + "create", + self._POOLNAME, + ] + _DEVICE_STRATEGY() + RUNNER(command_line) + + def test_on(self): + """ + In place must be specified for on. + """ + command_line = [ + "--propagate", + "pool", + "encryption", + "on", + f"--name={self._POOLNAME}", + "--clevis=tpm2", + ] + self.check_error(StratisCliInPlaceNotSpecified, command_line, _ERROR) + + def test_off(self): + """ + In place must be specified for off. + """ + command_line = [ + "--propagate", + "pool", + "encryption", + "off", + f"--name={self._POOLNAME}", + ] + self.check_error(StratisCliInPlaceNotSpecified, command_line, _ERROR) + + def test_reencrypt(self): + """ + In place must be specified for reencrypt. + """ + command_line = [ + "--propagate", + "pool", + "encryption", + "reencrypt", + f"--name={self._POOLNAME}", + ] + self.check_error(StratisCliInPlaceNotSpecified, command_line, _ERROR) From bd000b8f5de99c13c274cbcf46b3e7a4502b7332 Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 17 Jul 2025 16:17:33 -0400 Subject: [PATCH 08/18] Half D-Bus timeout We had it as high as two minutes to give a chance of returning on fairly long-running task, like creating encrypted pools, since each device had to be separately encrypted. We do not do that anymore, as the whole pool is encrypted these days, so the create method returns faster. We are about to introduce really long running commands, where 120 minutes will not be enough in almost all cases. So shortening the D-Bus timeout seems like a reasonable thing to do. 60 seconds is a plenty long time to wait for any error messsages that stratisd might return. Signed-off-by: mulhern --- src/stratis_cli/_actions/_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stratis_cli/_actions/_data.py b/src/stratis_cli/_actions/_data.py index b484fb0f6..fc0395bda 100644 --- a/src/stratis_cli/_actions/_data.py +++ b/src/stratis_cli/_actions/_data.py @@ -44,7 +44,7 @@ "deferred until after the stratis_cli module has been fully loaded." ) -DBUS_TIMEOUT_SECONDS = 120 +DBUS_TIMEOUT_SECONDS = 60 # Specification for the lowest manager interface supported by the major # version of stratisd on which this version of the CLI depends. From b689a69a681cafb433a9f1d21c2c91cfec84a9a4 Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 17 Jul 2025 17:35:38 -0400 Subject: [PATCH 09/18] Add a decorator for our long running operations This way we can relatively cheaply avoid printing the timeout error message and return a zero exit code on the timeout. Signed-off-by: mulhern --- src/stratis_cli/_actions/__init__.py | 1 + src/stratis_cli/_actions/_crypt.py | 4 +++ src/stratis_cli/_actions/_utils.py | 43 +++++++++++++++++++++++++++- src/stratis_cli/_error_reporting.py | 13 +-------- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/stratis_cli/_actions/__init__.py b/src/stratis_cli/_actions/__init__.py index f16cf1953..45e079911 100644 --- a/src/stratis_cli/_actions/__init__.py +++ b/src/stratis_cli/_actions/__init__.py @@ -35,3 +35,4 @@ from ._stratis import StratisActions from ._stratisd_version import check_stratisd_version from ._top import TopActions +from ._utils import get_errors diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 904cb55a5..97e19ea6c 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -29,6 +29,7 @@ from .._stratisd_constants import StratisdErrors from ._connection import get_object from ._constants import TOP_OBJECT +from ._utils import long_running_operation class CryptActions: @@ -37,6 +38,7 @@ class CryptActions: """ @staticmethod + @long_running_operation def encrypt(namespace: Namespace): """ Encrypt a previously unencrypted pool. @@ -97,6 +99,7 @@ def encrypt(namespace: Namespace): ) @staticmethod + @long_running_operation def unencrypt(namespace: Namespace): """ Unencrypt a previously encrypted pool. @@ -138,6 +141,7 @@ def unencrypt(namespace: Namespace): ) @staticmethod + @long_running_operation def reencrypt(namespace: Namespace): """ Reencrypt an already encrypted pool with a new key. diff --git a/src/stratis_cli/_actions/_utils.py b/src/stratis_cli/_actions/_utils.py index d2c01e0e5..dcc819948 100644 --- a/src/stratis_cli/_actions/_utils.py +++ b/src/stratis_cli/_actions/_utils.py @@ -22,15 +22,22 @@ import os import sys import termios +from argparse import Namespace +from collections.abc import Iterator from enum import Enum -from typing import Any, Optional, Tuple +from functools import wraps +from typing import Any, Callable, Optional, Tuple from uuid import UUID # isort: THIRDPARTY from dbus import Dictionary, Struct +from dbus.exceptions import DBusException from dbus.proxies import ProxyObject from justbytes import Range +# isort: FIRSTPARTY +from dbus_python_client_gen import DPClientInvocationError + from .._errors import ( StratisCliKeyfileNotFoundError, StratisCliPassphraseEmptyError, @@ -295,3 +302,37 @@ def free(self) -> Optional[Range]: Total - used. """ return None if self._used is None else self._total - self._used + + +def get_errors(exc: BaseException) -> Iterator[BaseException]: + """ + Generates a sequence of exceptions starting with exc and following the chain + of causes. + """ + yield exc + while exc.__cause__ is not None: + yield exc.__cause__ + exc = exc.__cause__ + + +def long_running_operation(func: Callable) -> Callable: + """ + Mark a function as a long running operation and catch and ignore NoReply + D-Bus exception. + """ + + @wraps(func) + def wrapper(namespace: Namespace): + try: + func(namespace) + except DPClientInvocationError as err: + # sim engine completes all operations rapidly + if not any( + isinstance(e, DBusException) + and e.get_dbus_name() == "org.freedesktop.DBus.Error.NoReply" + for e in get_errors(err) + ): # pragma: no cover + raise err + print("Operation initiated", file=sys.stderr) + + return wrapper diff --git a/src/stratis_cli/_error_reporting.py b/src/stratis_cli/_error_reporting.py index f0158f2a7..7a12941fe 100644 --- a/src/stratis_cli/_error_reporting.py +++ b/src/stratis_cli/_error_reporting.py @@ -17,7 +17,6 @@ # isort: STDLIB import os import sys -from collections.abc import Iterator from typing import List, Optional # isort: THIRDPARTY @@ -41,6 +40,7 @@ FILESYSTEM_INTERFACE, MANAGER_0_INTERFACE, POOL_INTERFACE, + get_errors, ) from ._errors import ( StratisCliActionError, @@ -87,17 +87,6 @@ def _interface_name_to_common_name(interface_name: str) -> str: raise StratisCliUnknownInterfaceError(interface_name) # pragma: no cover -def get_errors(exc: BaseException) -> Iterator[BaseException]: - """ - Generates a sequence of exceptions starting with exc and following the chain - of causes. - """ - yield exc - while exc.__cause__ is not None: - yield exc.__cause__ - exc = exc.__cause__ - - def _interpret_errors_0( error: dbus.exceptions.DBusException, ) -> Optional[str]: From 1d0e6d4a19cddf61d6405d4c1379c384cf415a50 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 21 Jul 2025 17:49:20 -0400 Subject: [PATCH 10/18] Display last time re-encrypted in pool detail view Signed-off-by: mulhern --- src/stratis_cli/_actions/_list_pool.py | 9 +++++++++ tests/integration/pool/test_encryption.py | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/src/stratis_cli/_actions/_list_pool.py b/src/stratis_cli/_actions/_list_pool.py index a60f26066..52ddb09b2 100644 --- a/src/stratis_cli/_actions/_list_pool.py +++ b/src/stratis_cli/_actions/_list_pool.py @@ -22,6 +22,7 @@ from typing import List, Optional, Union # isort: THIRDPARTY +from dateutil import parser as date_parser from justbytes import Range from .._alerts import ( @@ -360,6 +361,14 @@ def _print_detail_view( if encrypted: print("Encryption Enabled: Yes") + (valid, timestamp) = mopool.LastReencryptedTimestamp() + reencrypted = ( + date_parser.isoparse(timestamp).astimezone().strftime("%b %d %Y %H:%M") + if valid + else "Never" + ) + print(f" Last Time Reencrypted: {reencrypted}") + if metadata_version is MetadataVersion.V1: # pragma: no cover key_description_str = _non_existent_or_inconsistent_to_str( EncryptionInfoKeyDescription(mopool.KeyDescriptions()) diff --git a/tests/integration/pool/test_encryption.py b/tests/integration/pool/test_encryption.py index f646c5cce..6a93db20a 100644 --- a/tests/integration/pool/test_encryption.py +++ b/tests/integration/pool/test_encryption.py @@ -536,6 +536,10 @@ def test_reencrypt_with_name(self): ] RUNNER(command_line) + # Exercise detail view with last reencryption time set + command_line = ["--propagate", "pool", "list", f"--name={self._POOLNAME}"] + RUNNER(command_line) + class ReencryptTestCase2(SimTestCase): """ From 210608f4f2820fb249cd5d0b298357dabb511f6c Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 14 Aug 2025 11:55:35 -0400 Subject: [PATCH 11/18] Add man page entries for the moved subcommands These are bind, unbind, and rebind. The new commands use the mandatory --name, --uuid option mechanism for specifying the pool to operate on, while the old commands just used name. Signed-off-by: mulhern --- docs/stratis.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/stratis.txt b/docs/stratis.txt index de09713e9..fc7228397 100644 --- a/docs/stratis.txt +++ b/docs/stratis.txt @@ -117,6 +117,25 @@ pool unbind <(clevis|keyring)> [--token-slot ]:: mechanism. MOVE NOTICE: The "unbind" subcommand can also be found under the "pool encryption" subcommand. The "pool unbind" subcommand that you are using now is deprecated and will be removed in stratis 3.10.0. +pool encryption bind <(nbde|tang)> <(--uuid |--name )> <(--thumbprint | --trust-url)> :: + Bind the devices in the specified pool to a supplementary encryption + mechanism that uses NBDE (Network-Bound Disc Encryption). *tang* is + an alias for *nbde*. +pool encryption bind tpm2 <(--uuid |--name )>:: + Bind the devices in the specified pool to a supplementary encryption + mechanism that uses TPM 2.0 (Trusted Platform Module). +pool encryption bind keyring <(--uuid |--name )> :: + Bind the devices in the specified pool to a supplementary encryption + mechanism using a key in the kernel keyring. +pool encryption rebind clevis <(--uuid |--name )> [--token-slot ]:: + Rebind the devices in the specified pool using the Clevis configuration + with which the devices in the pool were previously bound. +pool encryption rebind keyring <(--uuid |--name )> [--token-slot ]:: + Rebind the devices in the specified pool using the specified key + description. +pool encryption unbind <(clevis|keyring)> <(--uuid |--name )> [--token-slot ]:: + Unbind the devices in the specified pool from the specified encryption + mechanism. pool set-fs-limit :: Set the limit on the number of file systems allowed per-pool. This number may only be increased from its current value. From 8e08eec43c7ae28e5f56e8a6bd9d3150b70741ab Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 15 Aug 2025 10:06:14 -0400 Subject: [PATCH 12/18] man: Add entries for encryption on, off, and re-encrypt Signed-off-by: mulhern --- docs/stratis.txt | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/stratis.txt b/docs/stratis.txt index fc7228397..25fe49915 100644 --- a/docs/stratis.txt +++ b/docs/stratis.txt @@ -117,6 +117,18 @@ pool unbind <(clevis|keyring)> [--token-slot ]:: mechanism. MOVE NOTICE: The "unbind" subcommand can also be found under the "pool encryption" subcommand. The "pool unbind" subcommand that you are using now is deprecated and will be removed in stratis 3.10.0. +pool encryption on --in-place <(--uuid |--name )> [--key-desc ] [--clevis <(nbde|tang|tpm2)> [--tang-url ] [<(--thumbprint | --trust-url)>]:: + Turn encryption on for the specified pool. This operation must encrypt + every block in the pool and takes time proportional to the size of the + pool. +pool encryption off --in-place <(--uuid |--name )>:: + Turn encryption off for the specified pool. This operation must write + the plain text of every block in the pool and takes time proportional to + the size of the pool. +pool encryption reencrypt --in-place <(--uuid |--name )>:: + Reencrypt the pool with a new master key. This operation must overwrite + every block in the pool with re-encrypted data and takes time + proportional to the size of the pool. pool encryption bind <(nbde|tang)> <(--uuid |--name )> <(--thumbprint | --trust-url)> :: Bind the devices in the specified pool to a supplementary encryption mechanism that uses NBDE (Network-Bound Disc Encryption). *tang* is @@ -262,6 +274,20 @@ OPTIONS --token-slot :: For V2 pools only. Use the token slot number to select among different bindings that use the same encryption method. +--in-place :: + This is a mandatory option that must be set when requesting + a long-running in-place encryption operation. These operations are + a kind that must read and write every block in the pool. Hence these + operations take a time that is linear in the size of the pool. + Additionally, these operations are run in place, that is, the + pool's data blocks are directly modified while it is in use. While + the operation is taking place, automatic administrative actions, + for example, extending filesystems, can not be taken on the pool. + Furthermore, user-initiated actions, such as adding a new device to + a pool are also disabled. The pool administrator should therefore + ensure that no administrative operations will become urgently + necessary while the encryption operation is running. Consider + backing up your data before initiating this operation. SIZE SPECIFICATION FORMAT FOR INPUT From 2c02ad8e8bb54db858810629b0324ec27afa6192 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 15 Aug 2025 13:34:12 -0400 Subject: [PATCH 13/18] man: Add entry for the "Last Time Reencrypted" field Signed-off-by: mulhern --- docs/stratis.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/stratis.txt b/docs/stratis.txt index 25fe49915..018fad2bb 100644 --- a/docs/stratis.txt +++ b/docs/stratis.txt @@ -383,6 +383,11 @@ Encryption Enabled: Clevis Configuration:: The Clevis configuration, if the pool is encrypted via Clevis. Only displayed if metadata version is 1 and encryption is enabled. +Encryption Enabled: Last Time Reencrypted:: + The last time the pool was re-encrypted. If the pool has never been + re-encrypted, the value is "Never". Only displayed if metadata + version is 2 and encryption is enabled. + Encryption Enabled: Free Token Slots Remaining:: The number of token slots remaining that can accommodate new bindings, followed by a list of binding descriptions, ordered by From b3e85bf079c440c3bb49b0b9ae4f7283428a0c8b Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 21 Aug 2025 14:51:32 -0400 Subject: [PATCH 14/18] Test exception handling paths of long_running_operation result Signed-off-by: mulhern --- src/stratis_cli/_actions/_utils.py | 3 +- tests/unit/test_running.py | 61 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_running.py diff --git a/src/stratis_cli/_actions/_utils.py b/src/stratis_cli/_actions/_utils.py index dcc819948..10a34a637 100644 --- a/src/stratis_cli/_actions/_utils.py +++ b/src/stratis_cli/_actions/_utils.py @@ -326,12 +326,11 @@ def wrapper(namespace: Namespace): try: func(namespace) except DPClientInvocationError as err: - # sim engine completes all operations rapidly if not any( isinstance(e, DBusException) and e.get_dbus_name() == "org.freedesktop.DBus.Error.NoReply" for e in get_errors(err) - ): # pragma: no cover + ): raise err print("Operation initiated", file=sys.stderr) diff --git a/tests/unit/test_running.py b/tests/unit/test_running.py new file mode 100644 index 000000000..2f1e74493 --- /dev/null +++ b/tests/unit/test_running.py @@ -0,0 +1,61 @@ +# Copyright 2025 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Test 'long_running_operation'. +""" + +# isort: STDLIB +import unittest + +# isort: THIRDPARTY +import dbus + +# isort: FIRSTPARTY +from dbus_python_client_gen import DPClientInvocationError + +# isort: LOCAL +from stratis_cli._actions._utils import long_running_operation + + +class LongRunningOperationTestCase(unittest.TestCase): + """ + Test long_running_operation error paths that don't show up in the sim + engine. + """ + + def test_raise_dbus_exception(self): + """ + Should succeed because it catches the distinguishing NoReply D-Bus + error. + """ + + def raises_error(_): + raise DPClientInvocationError( + "fake", "intf", None + ) from dbus.exceptions.DBusException( + name="org.freedesktop.DBus.Error.NoReply" + ) + + self.assertIsNone(long_running_operation(raises_error)(None)) + + def test_no_dbus_exception(self): + """ + Should raise an exception that was previously raised. + """ + + def raises_error(_): + raise DPClientInvocationError("fake", "intf", None) + + with self.assertRaises(DPClientInvocationError): + long_running_operation(raises_error)(None) From 04dfaf9b7d2c4798662a3babb73152187b29e1aa Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 15 Jan 2026 12:58:12 -0500 Subject: [PATCH 15/18] Remove unnecessary no cover pragma Signed-off-by: mulhern --- src/stratis_cli/_actions/_crypt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 97e19ea6c..62efc942b 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -169,7 +169,7 @@ def reencrypt(namespace: Namespace): get_object(pool_object_path), {} ) - if return_code != StratisdErrors.OK: # pragma: no cover + if return_code != StratisdErrors.OK: raise StratisCliEngineError(return_code, message) if not changed: # pragma: no cover From 6a529ccc9b6d9decc1aa4eb9e87ef1c164d255f8 Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 15 Jan 2026 15:00:36 -0500 Subject: [PATCH 16/18] Time out after ten seconds for long-running methods Signed-off-by: mulhern --- src/stratis_cli/_actions/_crypt.py | 41 +++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 62efc942b..687ca8222 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -17,8 +17,12 @@ # isort: STDLIB import json +import xml.etree.ElementTree as ET # nosec B405 from argparse import Namespace +# isort: FIRSTPARTY +from dbus_python_client_gen import make_class + from .._constants import PoolId from .._errors import ( StratisCliEngineError, @@ -48,7 +52,17 @@ def encrypt(namespace: Namespace): raise StratisCliInPlaceNotSpecified() # pylint: disable=import-outside-toplevel - from ._data import MOPool, ObjectManager, Pool, pools + from ._data import MOPool, ObjectManager, pool_spec, pools + + spec = ET.Element(pool_spec.tag, pool_spec.attrib) + spec.extend( + [ + child + for child in pool_spec + if child.tag == "method" and child.get("name") == "EncryptPool" + ] + ) + Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None @@ -108,8 +122,17 @@ def unencrypt(namespace: Namespace): raise StratisCliInPlaceNotSpecified() # pylint: disable=import-outside-toplevel - from ._data import MOPool, ObjectManager, Pool, pools - + from ._data import MOPool, ObjectManager, pool_spec, pools + + spec = ET.Element(pool_spec.tag, pool_spec.attrib) + spec.extend( + [ + child + for child in pool_spec + if child.tag == "method" and child.get("name") == "DecryptPool" + ] + ) + Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None @@ -150,7 +173,17 @@ def reencrypt(namespace: Namespace): raise StratisCliInPlaceNotSpecified() # pylint: disable=import-outside-toplevel - from ._data import ObjectManager, Pool, pools + from ._data import ObjectManager, pool_spec, pools + + spec = ET.Element(pool_spec.tag, pool_spec.attrib) + spec.extend( + [ + child + for child in pool_spec + if child.tag == "method" and child.get("name") == "ReencryptPool" + ] + ) + Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None From 10b24d387509c30d27f6b7c85c1123c69d699ee1 Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 15 Jan 2026 19:16:16 -0500 Subject: [PATCH 17/18] Abstract that class creation action Signed-off-by: mulhern --- src/stratis_cli/_actions/_crypt.py | 47 +++++++++++++++--------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 687ca8222..7fdc9a6ee 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -36,6 +36,22 @@ from ._utils import long_running_operation +def _make_class(spec, class_name, *, methods=None, timeout=-1): + """ + Make a class from spec, with only requested methods. + Interpret None as all methods. + """ + new_spec = ET.Element(spec.tag, spec.attrib) + new_spec.extend( + [ + child + for child in spec + if child.tag == "method" and methods is None or child.get("name") in methods + ] + ) + return make_class(class_name, new_spec, timeout=timeout) + + class CryptActions: """ Whole pool encryption actions. @@ -54,15 +70,9 @@ def encrypt(namespace: Namespace): # pylint: disable=import-outside-toplevel from ._data import MOPool, ObjectManager, pool_spec, pools - spec = ET.Element(pool_spec.tag, pool_spec.attrib) - spec.extend( - [ - child - for child in pool_spec - if child.tag == "method" and child.get("name") == "EncryptPool" - ] + Pool = _make_class( # pylint: disable=invalid-name + pool_spec, "Pool", methods=["EncryptPool"], timeout=10 ) - Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None @@ -124,15 +134,10 @@ def unencrypt(namespace: Namespace): # pylint: disable=import-outside-toplevel from ._data import MOPool, ObjectManager, pool_spec, pools - spec = ET.Element(pool_spec.tag, pool_spec.attrib) - spec.extend( - [ - child - for child in pool_spec - if child.tag == "method" and child.get("name") == "DecryptPool" - ] + Pool = _make_class( # pylint: disable=invalid-name + pool_spec, "Pool", methods=["DecryptPool"], timeout=10 ) - Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name + pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None @@ -175,15 +180,9 @@ def reencrypt(namespace: Namespace): # pylint: disable=import-outside-toplevel from ._data import ObjectManager, pool_spec, pools - spec = ET.Element(pool_spec.tag, pool_spec.attrib) - spec.extend( - [ - child - for child in pool_spec - if child.tag == "method" and child.get("name") == "ReencryptPool" - ] + Pool = _make_class( # pylint: disable=invalid-name + pool_spec, "Pool", methods=["ReencryptPool"], timeout=10 ) - Pool = make_class("Pool", spec, timeout=10) # pylint: disable=invalid-name pool_id = PoolId.from_parser_namespace(namespace) assert pool_id is not None From 44a7c56e05becda2061dbbaf93010e7067a153c1 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 16 Jan 2026 10:59:20 -0500 Subject: [PATCH 18/18] Make long_running_operation decorator more specific It should be specific to the particular D-Bus method that launches the long-running operation. Signed-off-by: mulhern --- src/stratis_cli/_actions/_crypt.py | 6 ++-- src/stratis_cli/_actions/_utils.py | 58 ++++++++++++++++++++---------- tests/unit/test_running.py | 30 ++++++++++++---- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/stratis_cli/_actions/_crypt.py b/src/stratis_cli/_actions/_crypt.py index 7fdc9a6ee..1955bf1da 100644 --- a/src/stratis_cli/_actions/_crypt.py +++ b/src/stratis_cli/_actions/_crypt.py @@ -58,7 +58,7 @@ class CryptActions: """ @staticmethod - @long_running_operation + @long_running_operation(method_names=["EncryptPool"]) def encrypt(namespace: Namespace): """ Encrypt a previously unencrypted pool. @@ -123,7 +123,7 @@ def encrypt(namespace: Namespace): ) @staticmethod - @long_running_operation + @long_running_operation(method_names=["DecryptPool"]) def unencrypt(namespace: Namespace): """ Unencrypt a previously encrypted pool. @@ -169,7 +169,7 @@ def unencrypt(namespace: Namespace): ) @staticmethod - @long_running_operation + @long_running_operation(method_names=["ReencryptPool"]) def reencrypt(namespace: Namespace): """ Reencrypt an already encrypted pool with a new key. diff --git a/src/stratis_cli/_actions/_utils.py b/src/stratis_cli/_actions/_utils.py index 10a34a637..9bd7ce326 100644 --- a/src/stratis_cli/_actions/_utils.py +++ b/src/stratis_cli/_actions/_utils.py @@ -26,7 +26,7 @@ from collections.abc import Iterator from enum import Enum from functools import wraps -from typing import Any, Callable, Optional, Tuple +from typing import Any, Callable, List, Optional, Tuple from uuid import UUID # isort: THIRDPARTY @@ -36,7 +36,7 @@ from justbytes import Range # isort: FIRSTPARTY -from dbus_python_client_gen import DPClientInvocationError +from dbus_python_client_gen import DPClientInvocationError, DPClientMethodCallContext from .._errors import ( StratisCliKeyfileNotFoundError, @@ -315,23 +315,45 @@ def get_errors(exc: BaseException) -> Iterator[BaseException]: exc = exc.__cause__ -def long_running_operation(func: Callable) -> Callable: +def long_running_operation( + *, method_names: List[str] +) -> Callable[[Callable], Callable]: """ Mark a function as a long running operation and catch and ignore NoReply - D-Bus exception. + D-Bus exception so long as the method raising the exception is one + of the methods specified in method_names. """ - @wraps(func) - def wrapper(namespace: Namespace): - try: - func(namespace) - except DPClientInvocationError as err: - if not any( - isinstance(e, DBusException) - and e.get_dbus_name() == "org.freedesktop.DBus.Error.NoReply" - for e in get_errors(err) - ): - raise err - print("Operation initiated", file=sys.stderr) - - return wrapper + def decorator(func: Callable) -> Callable: + """ + Decorator + """ + + @wraps(func) + def wrapper(namespace: Namespace): + """ + Wrapper + """ + try: + func(namespace) + except DPClientInvocationError as err: + if not any( + isinstance(e, DBusException) + and e.get_dbus_name() == "org.freedesktop.DBus.Error.NoReply" + for e in get_errors(err) + ): + raise err + + context = err.context + if ( + isinstance(context, DPClientMethodCallContext) + and context.method_name in method_names + ): + print("Operation initiated", file=sys.stderr) + + else: + raise err + + return wrapper + + return decorator diff --git a/tests/unit/test_running.py b/tests/unit/test_running.py index 2f1e74493..4e28a2930 100644 --- a/tests/unit/test_running.py +++ b/tests/unit/test_running.py @@ -22,7 +22,7 @@ import dbus # isort: FIRSTPARTY -from dbus_python_client_gen import DPClientInvocationError +from dbus_python_client_gen import DPClientInvocationError, DPClientMethodCallContext # isort: LOCAL from stratis_cli._actions._utils import long_running_operation @@ -34,20 +34,38 @@ class LongRunningOperationTestCase(unittest.TestCase): engine. """ - def test_raise_dbus_exception(self): + def test_catch_dbus_exception(self): """ Should succeed because it catches the distinguishing NoReply D-Bus - error. + error from the identified method. """ def raises_error(_): raise DPClientInvocationError( - "fake", "intf", None + "fake", "intf", DPClientMethodCallContext("MethodName", []) ) from dbus.exceptions.DBusException( name="org.freedesktop.DBus.Error.NoReply" ) - self.assertIsNone(long_running_operation(raises_error)(None)) + self.assertIsNone( + long_running_operation(method_names=["MethodName"])(raises_error)(None) + ) + + def test_raise_dbus_exception_no_name_match(self): + """ + Should succeed because it catches the distinguishing NoReply D-Bus + error from the identified method. + """ + + def raises_error(_): + raise DPClientInvocationError( + "fake", "intf", DPClientMethodCallContext("MethodName", []) + ) from dbus.exceptions.DBusException( + name="org.freedesktop.DBus.Error.NoReply" + ) + + with self.assertRaises(DPClientInvocationError): + long_running_operation(method_names=["OtherMethodName"])(raises_error)(None) def test_no_dbus_exception(self): """ @@ -58,4 +76,4 @@ def raises_error(_): raise DPClientInvocationError("fake", "intf", None) with self.assertRaises(DPClientInvocationError): - long_running_operation(raises_error)(None) + long_running_operation(method_names=["OtherMethodName"])(raises_error)(None)