Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PIV commands #506

Merged
merged 8 commits into from
Sep 19, 2024
Merged

Add PIV commands #506

merged 8 commits into from
Sep 19, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

This PR adds PIV commands under nitropy nk3 piv

Checklist

Make sure to run make check and make fix before creating a PR, otherwise the CI will fail.

  • tested with Python3.9
  • [x[ signed commits
  • updated documentation (e.g. parameter description, inline doc, docs.nitrokey)
  • added labels

If we merge this before PIV is stabilised, this will need to be behind a --experimental flag.

@sosthene-nitrokey
Copy link
Contributor Author

Not sure how to deal with the modules that don't have type definitions.

How can I just ignore the errors for pyscard and asn1crypto? I don't think we can get rid of those, or add type checks.

ber-tlv is smaller and might be worth replacing with something homemade, we might want to avoid having a dependency as small as this one.

@robin-nitrokey
Copy link
Member

Generally, there are two solutions to dependencies without type annotations:

  1. manually add the annotations for those parts of the public API that we use in a stub
  2. just ignore the errors

Obviously, the first option is much better, but not always worth the effort, especially for legacy code. See the stubs folder for an example for (1), and the libraries without annotations section in pyproject.toml for (2).

Also make sure to check the typeshed repository. Maybe somebody already wrote a stub.

@tgahlx
Copy link

tgahlx commented Apr 11, 2024

I've found a bug testing this branch with the 1.6.0-test firmware

~/Projekte/nitro-ca-test ❯/home/tga/Projekte/pynitrokey/venv/bin/nitropy nk3 piv write-certificate --format PEM --key 9C --path ./jd-c.crt                                                                                                                                                                          17:05:25
Command line tool to interact with Nitrokey devices 0.4.45
Usage: nitropy nk3 piv write-certificate [OPTIONS] [ADMIN_KEY]
Try 'nitropy nk3 piv write-certificate --help' for help.

Error: Invalid value for '--key': '9C' is not one of '9A', ' 9C', ' 9D', ' 9E', ' 82', ' 83', ' 84', ' 85', ' 86', ' 87', ' 88', ' 89', ' 8A', ' 8B', ' 8C', ' 8D', ' 8E', ' 8F', ' 90', ' 91', ' 92', ' 93', ' 94', ' 95'.
~/Projekte/nitro-ca-test ❯ /home/tga/Projekte/pynitrokey/venv/bin/nitropy nk3 piv write-certificate --format PEM --key ' 9C' --path ./jd-c.crt                                                                                                                                                                       17:05:38
Command line tool to interact with Nitrokey devices 0.4.45
Critical error:
An unhandled exception occurred
	Exception encountered: KeyError(' 9C')

@sosthene-nitrokey sosthene-nitrokey changed the title App PIV commands Add PIV commands Apr 12, 2024
@sosthene-nitrokey
Copy link
Contributor Author

Thank you for the report.

It was a very simple typo, fixed in d5bc2af

@sosthene-nitrokey
Copy link
Contributor Author

I added the --experimental flag to the entire piv group (so this is now nitropy nk3 piv --experimental init (and the same for other piv-related commands).

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing typing checks for smartcard seems unnecessary, otherwise LGTM.

pyproject.toml Show resolved Hide resolved
pynitrokey/tlv.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@sosthene-nitrokey
Copy link
Contributor Author

All review comments are resolved. Let's merge.

@sosthene-nitrokey sosthene-nitrokey merged commit faa3f50 into master Sep 19, 2024
8 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the piv-rebase branch September 19, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants