Skip to content

Commit

Permalink
nk3 set-config: Add support for opcard.use_se050_backend
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
robin-nitrokey committed Dec 18, 2023
1 parent c88030e commit e8fe4f9
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 20 deletions.
109 changes: 89 additions & 20 deletions pynitrokey/cli/nk3/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}.")


Expand Down
6 changes: 6 additions & 0 deletions pynitrokey/nk3/admin_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit e8fe4f9

Please sign in to comment.