Update nethsm dependency #469
Conversation
robin-nitrokey
force-pushed
the
nethsm
branch
from
623960e to
9ba2cc3
Compare
This update pulls in multiple relevant changes, mainly: - The auto-generated client is only loaded if required. This improves execution times significantly, especially for simple commands. - Key certificates are now always stored as raw bytes, not as strings, so the get-certificate and set-certificate commands had to be updated. - Changing the backup and unlock passphrases now requires passing the current passphrase. - Using enums instead of strings. - Using stronger types and dataclasses for some methods. - NetHSM API changes that are not visible in the public Python API. On my machine, this reduces the execution time for simple commands like version or --help from 0.9 s to 0.3 s.
robin-nitrokey
force-pushed
the
nethsm
branch
from
9ba2cc3 to
8b5b9e8
Compare
|
This also enables strict typing checks for the nethsm CLI module so that updates will hopefully be easier in the future. |
|
The latest API Spec contains three warnings. Could these or similar warnings be shown when calling the appropriate commands? |
This patch adds warnings to the provision, set-backup-passphrase and set-unlock-passphrase commands that explain that the passphrases cannot be reset without knowing the old value. It also adds confirmation prompts to set-backup-passphrase and set-unlock-passphrase after the warning is shown.
| @@ -835,6 +849,18 @@ def set_backup_passphrase( | |||
|
|
|||
| This command requires authentication as a user with the Administrator | |||
| role.""" | |||
|
|
|||
| print( | |||
There was a problem hiding this comment.
In my opinion this warning should only be printed if the force flag is false. But may this is more a matter of taste.
There was a problem hiding this comment.
I would be conservative here: Always printing the warning doesn’t cost us anything, but it could still help to prevent mistakes.
| ) -> None: | ||
| """Set the unlock passphrase of a NetHSM. | ||
|
|
||
| Changing the unlock passphrase requires the current passphrase. | ||
|
|
||
| This command requires authentication as a user with the Administrator | ||
| role.""" | ||
|
|
||
| print( |
There was a problem hiding this comment.
In my opinion this warning should only be printed if the force flag is false. But may this is more a matter of taste.
| @@ -983,9 +1059,12 @@ def get_certificate(ctx, api, key_id): | |||
| with connect(ctx) as nethsm: | |||
| if key_id: | |||
| cert = nethsm.get_key_certificate(key_id) | |||
| try: | |||
There was a problem hiding this comment.
A user should be able to store arbitrary byte strings for which the .decode() call may not necessarily succeeds.
Using .decode() here limits the byte strings the user can store.
There was a problem hiding this comment.
If decode fails, we print the raw bytes (see the except block). The alternative would be to always print e. g. the hex representation, but I think that would be confusing for most use cases.
This update pulls in multiple relevant changes, mainly:
On my machine, this reduces the execution time for simple commands like version or --help from 0.9 s to 0.3 s.