From e8fe4f956b5383f7d5fa7ae1a7550c69a5b90d0b Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Mon, 18 Dec 2023 23:11:13 +0100 Subject: [PATCH] nk3 set-config: Add support for opcard.use_se050_backend This patch refactors the set-config command: 1. It checks whether the config option is supported by the device using the GET_CONFIG command before any other steps are performed. 2. For known keys that trigger a reset, currently only opcard.use_se050_backend, a warning and a confirmation prompt are shown. 3. Unknown keys are rejected unless --force is set, trigger a warning and require a confirmation prompt. 4. It adds a --dry-run option to check the infos and prompts that are printed for a config change. 5. It adds an automatic reboot if required. Potential improvements: - The metadata for config values should be put into data structures that can also be used by nitrokey-app2. - Information on whether a command triggers a reset and requires touch confirmation could be queried from the device using a new command. - We could validate the configuration values before sending them to the device. - We could compare the current and the new value to detect no-op calls before printing the sermon about side effects. --- pynitrokey/cli/nk3/__init__.py | 109 +++++++++++++++++++++++++++------ pynitrokey/nk3/admin_app.py | 6 ++ 2 files changed, 95 insertions(+), 20 deletions(-) diff --git a/pynitrokey/cli/nk3/__init__.py b/pynitrokey/cli/nk3/__init__.py index 90fd75f1..19090afc 100644 --- a/pynitrokey/cli/nk3/__init__.py +++ b/pynitrokey/cli/nk3/__init__.py @@ -489,35 +489,104 @@ def get_config(ctx: Context, key: str) -> None: @click.pass_obj @click.argument("key") @click.argument("value") -def set_config(ctx: Context, key: str, value: str) -> None: +@click.option( + "-f", + "--force", + is_flag=True, + default=False, + help="Set the config value even if it is not known to pynitrokey", +) +@click.option( + "--dry-run", + is_flag=True, + default=False, + help="Perform all checks but don’t execute the configuration change", +) +def set_config(ctx: Context, key: str, value: str, force: bool, dry_run: bool) -> None: """ Set a config value. - This command should not be used directly as it may have unexpected - side effects, for example resetting an application. It is only intended - for development and testing. - """ - - # config fields that don’t have side effects - whitelist = [ - "fido.disable_skip_up_timeout", - ] + Per default, this command can only be used with configuration values that + are known to pynitrokey. Changing some configuration values can have side + effects. For these values, a summary of the effects of the change and a + confirmation prompt will be printed. - if key not in whitelist: - print( - "Changing configuration values can have unexpected side effects, including data loss.", - file=sys.stderr, - ) - print( - "This command should only be used for development and testing.", - file=sys.stderr, - ) + If you use the --force/-f flag, you can also set configuration values that + are not known to pynitrokey. This may have unexpected side effects, for + example resetting an application. It is only intended for development and + testing. - click.confirm("Do you want to continue anyway?", abort=True) + To see the information about a config value without actually performing the + change, use the --dry-run flag. + """ with ctx.connect_device() as device: admin = AdminApp(device) + + # before the confirmation prompt, check if the config value is supported + if not admin.has_config(key): + raise CliException( + f"The configuration option '{key}' is not supported by the device.", + support_hint=False, + ) + + # config fields that don’t have side effects + whitelist = [ + "fido.disable_skip_up_timeout", + ] + requires_touch = False + requires_reboot = False + + if key == "opcard.use_se050_backend": + requires_touch = True + requires_reboot = True + print( + "This configuration values determines whether the OpenPGP Card " + "application uses a software implementation or the secure element.", + file=sys.stderr, + ) + print( + "Changing this configuration value will cause a factory reset of " + "the OpenPGP card application and destroy all OpenPGP keys and " + "user data currently stored on the device.", + file=sys.stderr, + ) + elif key not in whitelist: + pass + print( + "Changing configuration values can have unexpected side effects, including data loss.", + file=sys.stderr, + ) + print( + "This should only be used for development and testing.", + file=sys.stderr, + ) + + if not force: + raise CliException( + "Unknown config values can only be set if the --force/-f flag is set. Aborting.", + support_hint=False, + ) + + if key not in whitelist: + click.confirm("Do you want to continue anyway?", abort=True) + + if dry_run: + print("Stopping dry run.", file=sys.stderr) + raise click.Abort() + + if requires_touch: + print( + "Press the touch button to confirm the configuration change.", + file=sys.stderr, + ) + admin.set_config(key, value) + + if requires_reboot: + print("Rebooting device to apply config change.") + device.reboot() + print(f"Updated configuration {key}.") diff --git a/pynitrokey/nk3/admin_app.py b/pynitrokey/nk3/admin_app.py index a9ba4baf..d584d414 100644 --- a/pynitrokey/nk3/admin_app.py +++ b/pynitrokey/nk3/admin_app.py @@ -169,6 +169,12 @@ def version(self) -> Version: def se050_tests(self) -> Optional[bytes]: return self._call(AdminCommand.TEST_SE050) + def has_config(self, key: str) -> bool: + reply = self._call(AdminCommand.GET_CONFIG, data=key.encode()) + if not reply or len(reply) < 1: + return False + return ConfigStatus.from_int(reply[0]) == ConfigStatus.SUCCESS + def get_config(self, key: str) -> str: reply = self._call(AdminCommand.GET_CONFIG, data=key.encode()) if not reply or len(reply) < 1: