-
Notifications
You must be signed in to change notification settings - Fork 28
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 SE050 test #423
Add SE050 test #423
Conversation
a1dcd5f
to
e1ef89c
Compare
f8f47c0
to
af78e4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is generally a caller for the in-firmware test, and interpreter of the results gathered there.
- Why the control of the tests is placed in the firmware instead of the test calling code?
Was it easier that way? Or is this a quick POC? - I think
pytest
would be better to use here, but its done already.
What do you mean by "the control of the test"?
The goal here is to get info from beta testers that the SE050 driver works as expected with NK3 that are already deployed. If doing so through pytest we likely wouldn't get feedback since users are unlikely to use it. |
You are essentially calling a fixed test routine in the firmware, and waiting for the results. Instead you could construct test cases on the calling side, and use a proper direct communication channel to se050.
That explains things. I do not think though running pytest itself is that much trouble for a beta tester, but that's my opinion. |
I wanted to test the actual APDU generation from the driver to test it, so all commands to the SE050 needs to be run from rust code. This is meant to test the driver in real conditions. Communicating to the SE050 directly from nitropy would make that much less useful, even if it were to test the T=1 over I2C part of the driver.
I agree, but running pytest is much less discoverable while |
af78e4f
to
015c18b
Compare
this does not interfere with anything else, should we merge this ? |
005fb1a
to
8908094
Compare
We will likely use start using the SE050 in a future release and we don't want the test to keep on being ran
597748b
to
96a41b6
Compare
Running this with the
|
96a41b6
to
6d17acb
Compare
6d17acb
to
41797f1
Compare
Can you try again? This should be fixed I believe. |
Yes, fixed. |
pynitrokey/cli/nk3/test.py
Outdated
try: | ||
q.put(AdminApp(device).se050_tests()) | ||
except CtapError as e: | ||
if e.code == CtapError.ERR.INVALID_LENGTH: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also check for INVALID_COMMAND
instead of whitelisting firmware versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We can also update admin-app
to return it when the command is disabled instead of INVALID_LENGTH
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning UnsupportedCommand
seems to lead to pynitrokey not receiving the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why that is there: https://github.com/Nitrokey/pynitrokey/blob/master/pynitrokey/nk3/admin_app.py#L72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea was to make it easier to handle the case that the firmware does not (yet) support a command. _call
always returns bytes
, so None
always indicates that the command is not supported. And checking for None
is easier than catching the exception and checking the error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I can just skip if None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant PR in the admin app: Nitrokey/admin-app#12
This PR adds a test that is introduced in the admin app with Nitrokey/admin-app#5
Note that it takes around 30seconds to complete, it's not crashing.