Admin app se050 test #335

sosthene-nitrokey · 2023-08-16T09:44:54Z

This PR adds a test for the admin app in the SE050.

Linked nitropy PR: Nitrokey/pynitrokey#423
Linked admin-app PR: Nitrokey/admin-app#5

utils/nrf-bootloader/.built

robin-nitrokey

Does it still make sense to include the random bytes backend if we can’t use it together with the tests anyway?

sosthene-nitrokey · 2023-08-22T12:59:27Z

I left the random bytes backend since the boilerplate to get the se050 passed to a backend is useful (as it will be part of a backend in the end, not an app).

sosthene-nitrokey · 2023-08-22T13:02:01Z

I left the random bytes backend because the boilerplate to get the se050 passed to a backend is useful (as it will be part of a backend in the end, not an app).

sosthene-nitrokey · 2023-09-15T08:21:22Z

This PR has evolved a bit.

Now it adds the admin app tests and a barebones SE050 backend that:

tests the SE050 driver (through the admin-app)
Is used for random number generation.

It also feeds some entropy from the SE050 in the initial Rng seed for the nrf52 runner.

sosthene-nitrokey · 2023-09-19T15:52:36Z

@robin-nitrokey if you can test this on LPC55 (including that nitropy nk3 status doesn't show an error), we can then merge.

sosthene-nitrokey · 2023-09-25T08:38:40Z

@robin-nitrokey have you had time to test this on NFC?

robin-nitrokey · 2023-09-25T17:11:56Z

Not yet, I’ll try to do so today or tomorrow morning.

robin-nitrokey · 2023-09-26T10:09:00Z

The boot process over USB works fine, but the se050 test causes a CTAP error: 0x03 - INVALID_LENGTH.

$ nitropy nk3 status
Command line tool to interact with Nitrokey devices 0.4.39
UUID:               223FE5E2AE287150AD9DAD9E34B7F989
Firmware version:   v1.5.0-test.admin.app.se050
Init status:        ok
Free blocks (int):  51
Free blocks (ext):  453
Variant:            LPC55

$ nitropy nk3 test
Command line tool to interact with Nitrokey devices 0.4.39
Found 1 Nitrokey 3 device(s):
- Nitrokey 3 at /dev/hidraw0

Running tests for Nitrokey 3 at /dev/hidraw0

[1/5]	uuid     	UUID query              	SUCCESS  	223FE5E2AE287150AD9DAD9E34B7F989
[2/5]	version  	Firmware version query  	SUCCESS  	v1.5.0-test.admin.app.se050
[3/5]	status   	Device status           	SUCCESS  	Status(init_status=<InitStatus.0: 0>, ifs_blocks=51, efs_blocks=453, variant=<Variant.LPC55: 1>)
Running SE050 test:   0%|                                                                                                                                                                                                                                                                 Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.9/threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "/home/robin/reps/pynitrokey/venv/lib/python3.9/site-packages/pynitrokey/cli/nk3/test.py", line 373, in internal_se050_run
    q.put(AdminApp(device).se050_tests())
  File "/home/robin/reps/pynitrokey/venv/lib/python3.9/site-packages/pynitrokey/nk3/admin_app.py", line 104, in se050_tests
    return self._call(AdminCommand.TEST_SE050)
  File "/home/robin/reps/pynitrokey/venv/lib/python3.9/site-packages/pynitrokey/nk3/admin_app.py", line 67, in _call
    return self.device._call(
  File "/home/robin/reps/pynitrokey/venv/lib/python3.9/site-packages/pynitrokey/nk3/device.py", line 133, in _call
    response = self.device.call(command.value, data=data)
  File "/home/robin/reps/pynitrokey/venv/lib/python3.9/site-packages/fido2/hid/__init__.py", line 216, in call
    raise CtapError(struct.unpack_from(">B", recv)[0])
fido2.ctap.CtapError: CTAP error: 0x03 - INVALID_LENGTH

sosthene-nitrokey · 2023-09-28T12:55:40Z

I see where this error comes from. You need to enable the se050-test-app feature when compiling.

There are 2 features added with this PR:

se050: enables the SE050 initialization, including getting some RNG from the SE050 on boot. It also initializes the backend.
se050-test-app: enables the SE050 test for the admin app and provides it with the backend.

Sorry I should have documented that in the PR.

sosthene-nitrokey · 2023-09-28T13:31:52Z

The INVALID_LENGTH error comes from the NotAvailable error being converted to INVALID_LENGTH here

robin-nitrokey · 2023-09-28T18:39:34Z

Argh, I just assumed those would be activated by the test feature. I should have checked that.

Unfortunately, even SE050 initialization does not work. If I enable just the se050 feature, the LED does not turn off after boot and the USB device does not show up.

When running with gdb, it hangs at:

        self.t1.resync()?;

https://github.com/Nitrokey/se05x/blob/0b77eb6b152d214897696aadf87767fd84ffcb0e/src/se05x.rs#L133

        let data = self.receive_data(&mut [])?;

https://github.com/Nitrokey/se05x/blob/0b77eb6b152d214897696aadf87767fd84ffcb0e/src/t1.rs#L570

           let read = self.read(&mut header_buffer);

https://github.com/Nitrokey/se05x/blob/0b77eb6b152d214897696aadf87767fd84ffcb0e/src/t1.rs#L458

        match self.twi.read(self.se_address, buffer) {

https://github.com/Nitrokey/se05x/blob/0b77eb6b152d214897696aadf87767fd84ffcb0e/src/t1.rs#L422

            while self.i2c.stat.read().mstpending().is_in_progress() {}

https://github.com/Nitrokey/lpc55-hal/blob/781c97372d07e98d48828ae345412cc311bd4113/src/drivers/i2c.rs#L255

Though this has to be taken with a grain of salt as I previously experienced timing issues when debugging I2C communication, so the actual problem could be somewhere else. Do you have any ideas what could be the problem or should I try to debug it?

robin-nitrokey · 2023-09-28T18:56:59Z

As a benchmark, I executed the hardware tests from the main branch. I did not run into this issue, even when debugging with gdb and enabling log messages with semihosting.

robin-nitrokey · 2023-09-28T19:21:29Z

I did some more tests. Removing the RESYNC command in get_se050_i2c fixes the initialization problem and makes the device boot properly (init status 0).

Running the SE050 tests with gdb times out. It seems to succeed shortly after the timeout, so maybe we should bump it to 2 min? Without gdb, it seems to work fine:

Running SE050 test:  61%|███████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████▋                                                                                                     
[4/5]	se050    	SE050                   	SUCCESS  	SE050 firmware version: 3.1.1 - 1.11, (persistent: (32767,), transient_deselect: (607,), transient_reset: (592,))

Edit: Looks like this is not related to gdb. I tried again without gdb and it also timed out.

robin-nitrokey · 2023-09-28T19:47:06Z

But FIDO operations no longer work if test and se050 is enabled:

Program received signal SIGTRAP, Trace/breakpoint trap.
0xeffffffe in ?? ()
(gdb)

Without test, it works. Stack overflow?

sosthene-nitrokey · 2023-10-03T09:03:29Z

If this can't be done quickly, I think we might want to merge and add a increase timeout for the NFC version, or reduce the total number of tests. Some tests are probably a bit overkill for in-the-field testing, though they were necessary for development of the driver. For example reducing RSA tests should be significant.

robin-nitrokey · 2023-10-04T09:30:15Z

That’s fine with me. For fixing the problem, the next steps could be:

trying to identify what exactly is slowing us down – is everything becoming slower, or is it only a subset of operations?
bringing up the issue with SE050 support
trying to verify the I2C rate on the LPC55 – can we somehow measure it?
double-checking the I2C configuration on the LPC55
trying to use the 96 MHz input clock (although the docs suggest it won’t work)
asking LPC55 support about the I2C settings

daringer · 2023-10-04T11:54:51Z

I would also prefer to not have this PR be blocked by this issue entirely. To fix that I would suggest to:

reduce the se050 tests to an reasonable amount so that we don't see any timeouts anymore (and then merge as is)
investigate the issue more thoroughly before addressing 3rd-parties, especially a comparison between nRF52 and lpc55 on a (mostly) deterministic operation on the se050 would be interesting - so that we can be sure that the i2c frequency is the root of this issue
the latter would also be then a good metric to improve se050/i2c communication speed on the lpc55

sosthene-nitrokey · 2023-10-04T12:44:58Z

I updated the tests with reduced testing. It should still test most important features and detect unstable behaviour.
Can you retest @robin-nitrokey ?

I also bumped the timeout on the nitropy side.

robin-nitrokey · 2023-10-04T12:50:37Z

Just to see whether changing the configured data rate does make a difference, I tried to time the RESYNC operation writes in init.rs:

let command = [0x5a, 0xc0, 0x00, 0xff, 0xfc];
i2c.write(0x48, &command)
    .expect("failed to send RESYNC command");

I consistently get an execution time of ca. 570 µs with the 100 kHz setting and 155 µs with 400 kHz. Of course this is not really a reliable measurement, but the settings do seem to have an effect and even the proportions of 1:4 roughly match.

sosthene-nitrokey · 2023-10-04T12:56:52Z

And what happens with the read that follows? Is it also faster?

robin-nitrokey · 2023-10-04T13:05:10Z

And what happens with the read that follows? Is it also faster?

Basically the same. 570 µs vs 150 µs.

robin-nitrokey · 2023-10-04T15:02:20Z

With 7021fd3 and Nitrokey/pynitrokey@f7f507e I get execution times of 8 s, 13 s, 14 s and 21 s. So still quite a big variation but overall much faster and not close to the timeout.

sosthene-nitrokey · 2023-10-04T15:52:18Z

Great then. I suppose we can merge this and Nitrokey/pynitrokey#423

robin-nitrokey

Can you add se050 and se050-test-app to the test feature so that we can tick that box?

robin-nitrokey · 2023-10-09T06:53:41Z

runners/usbip/volatile.bin                 | Bin 0 -> 8192 bytes

to be removed

robin-nitrokey · 2023-10-09T06:56:52Z

runners/embedded/src/bin/app-nrf.rs

+        use chacha20::ChaCha8Rng;
+        use rand::Rng as _;
+        let mut dev_rng = Rng::new(ctx.device.RNG);
+        let seed: [u8; 32] = dev_rng.gen();
+        #[cfg(feature = "se050")]
+        let mut se050 = se05x::se05x::Se05X::new(twim, 0x48, se050_timer);
+
+        #[cfg(feature = "se050")]
+        let seed = (|| {
+            use se05x::se05x::commands::GetRandom;
+            se050.enable()?;
+            let buf = &mut [0; 100];
+            let se050_rand = se050.run_command(&GetRandom { length: 32.into() }, buf)?;
+            let mut s: [u8; 32] = se050_rand
+                .data
+                .try_into()
+                .or(Err(se05x::se05x::Error::Unknown))?;
+            for (se050, orig) in s.iter_mut().zip(seed) {
+                *se050 ^= orig;


Could be moved to a helper method in lib.rs to be shared between both socs.

robin-nitrokey · 2023-10-10T17:49:37Z

Some thoughts during debugging:

For the SoC-independent code, we should use a consistent term – either I2C or TWI. I think I2C is more precise here, right?
When running from NFC, we should not even setup the I2C interface and the SE050 to save power and cycles.

sosthene-nitrokey · 2023-10-16T08:38:48Z

For the SoC-independent code, we should use a consistent term – either I2C or TWI. I think I2C is more precise here, right?

TWI is the term used by the NRF peripheral that drives its I2C bus. So yes I2C would be more general.

When running from NFC, we should not even setup the I2C interface and the SE050 to save power and cycles.

Right, that would be great. But for everything to type check the the backend needs to be always be created. Creating it without the se05x would be tricky. Maybe I can implement the backend and extension traits for Option<RealBackend> that returns an error every time it receives a request.

This would mean that it is impossible to perform any operation for clients that use the SE050 backend. We could be more granular and return RequestNotAvailable by having the main backend use Option<Se05x> but that would be a larger refactor and may require some annoying workaround the borrow checker.
I would go with the first.

robin-nitrokey · 2023-10-16T08:50:02Z

But for everything to type check the the backend needs to be always be created

Couldn’t we store an Option<Se050Backend> in Dispatch and return RequestNotAvailable if it is None?

sosthene-nitrokey · 2023-10-16T08:52:42Z

Dispatch seems like a good place indeed.

I would rather not return RequestNotAvailable as the risk would be having a request that should be handled by the SE050 be passed through to another backend when using NFC rather failing. This could lead to state inconsistency. This would first require a bug making an SE050-enabled app available over NFC, but that has already happened.

robin-nitrokey · 2023-10-19T16:26:10Z

Can you please rebase onto main for a final round of tests?

robin-nitrokey · 2023-10-19T16:43:14Z

Also, the CI cannot find the webcrypt dependency:

Caused by:
  Unable to update https://github.com/sosthene-nitrokey/nitrokey-webcrypt-rust?rev=32240f8551c34fea1c9feb1c6b90a138ee81a51f#32240f85
Caused by:
  object not found - no match for id (32240f8551c34fea1c9feb1c6b90a138ee81a51f); class=Odb (9); code=NotFound (-3)

Although I’m not sure why because the commit seems to exist: sosthene-nitrokey/nitrokey-webcrypt-rust@32240f8 Maybe just merge and tag Nitrokey/nitrokey-websmartcard-rust#9?

sosthene-nitrokey · 2023-10-20T12:18:13Z

Tests appear fine on the nk3am

robin-nitrokey · 2023-10-20T19:26:21Z

Works for me:

$ nitropy nk3 test --pin 123456
Command line tool to interact with Nitrokey devices 0.4.40
Found 1 Nitrokey 3 device(s):
- Nitrokey 3 at /dev/hidraw4

Running tests for Nitrokey 3 at /dev/hidraw4

[1/5]	uuid     	UUID query              	SUCCESS  	223FE5E2AE287150AD9DAD9E34B7F989
[2/5]	version  	Firmware version query  	SUCCESS  	v1.5.0-test.20230704
[3/5]	status   	Device status           	SUCCESS  	Status(init_status=<InitStatus.0: 0>, ifs_blocks=59, efs_blocks=451, variant=<Variant.LPC55: 1>)
Running SE050 test: |                                                                                                                                                                                                                                                                     
[4/5]	se050    	SE050                   	SUCCESS  	SE050 firmware version: 3.1.1 - 1.11, (persistent: (32767,), transient_deselect: (607,), transient_reset: (592,))
Please press the touch button on the device ...
[5/5]	fido2    	FIDO2                   	FAILURE  	Unexpected FIDO2 cert hash for version v1.5.0-test.20230704: c7fbd9ee89f3a32408ce6cc4adb23da940cc1515c741237ef4b8718e24515ac6

5 tests, 4 successful, 0 skipped, 1 failed

RSA-2048 key generation with OpenPGP also works for me with this branch.

cargo fmt reports some issues though.

components/apps/src/lib.rs

robin-nitrokey · 2023-10-24T10:23:55Z

Thanks! So from my POV, the remaining blockers are:

CI failure / tagged dependencies
cargo fmt

This PR adds an SE050 driver and its tests and uses SE050 entropy to bootstrap the random number generator. Fixes: #342 #343

sosthene-nitrokey requested review from szszszsz and robin-nitrokey August 16, 2023 09:44

szszszsz reviewed Aug 16, 2023

View reviewed changes

utils/nrf-bootloader/.built Outdated Show resolved Hide resolved

sosthene-nitrokey force-pushed the admin-app-se050-test branch 2 times, most recently from d42fcad to 38cef33 Compare August 16, 2023 14:38

robin-nitrokey reviewed Aug 16, 2023

View reviewed changes

daringer approved these changes Sep 1, 2023

View reviewed changes

sosthene-nitrokey force-pushed the admin-app-se050-test branch 2 times, most recently from 87e8fa7 to 12481bf Compare September 5, 2023 09:38

sosthene-nitrokey force-pushed the admin-app-se050-test branch from 5c24adf to 1ac519c Compare September 15, 2023 08:02

sosthene-nitrokey requested review from daringer and robin-nitrokey September 15, 2023 08:45

sosthene-nitrokey force-pushed the admin-app-se050-test branch from fb7e70c to d0d124e Compare September 15, 2023 09:26

This was linked to issues Sep 19, 2023

Admin-app se050 tests #342

Closed

Introduce SE050 for entropy initialization #343

Closed

This was referenced Sep 19, 2023

Use updated CTAP dependencies with less stack usage #332

Closed

Add custom se050 backend for the RandomBytes syscalls #317

Closed

robin-nitrokey reviewed Oct 4, 2023

View reviewed changes

sosthene-nitrokey mentioned this pull request Oct 6, 2023

Update fido-authenticator #351

Closed

robin-nitrokey reviewed Oct 9, 2023

View reviewed changes

robin-nitrokey mentioned this pull request Oct 9, 2023

Minimal SE050 RNG implementation #352

Closed

robin-nitrokey mentioned this pull request Oct 19, 2023

se050/lpc55: Investigate execution times and I2C data rate #357

Open

robin-nitrokey reviewed Oct 20, 2023

View reviewed changes

components/apps/src/lib.rs Outdated Show resolved Hide resolved

Add tests for the SE050 and bootstrap RNG with it

7935733

This PR adds an SE050 driver and its tests and uses SE050 entropy to bootstrap the random number generator. Fixes: #342 #343

robin-nitrokey force-pushed the admin-app-se050-test branch from 3f2d929 to 7935733 Compare October 25, 2023 14:42

robin-nitrokey merged commit bf2051b into main Oct 25, 2023

robin-nitrokey deleted the admin-app-se050-test branch October 25, 2023 14:50

robin-nitrokey mentioned this pull request Oct 25, 2023

Update Nitrokey WebSmartCard to v0.8-rc1 #329

Closed

Admin app se050 test #335

Admin app se050 test #335

Conversation

sosthene-nitrokey commented Aug 16, 2023

robin-nitrokey left a comment

Choose a reason for hiding this comment

sosthene-nitrokey commented Aug 22, 2023

sosthene-nitrokey commented Aug 22, 2023 • edited Loading

sosthene-nitrokey commented Sep 15, 2023 • edited Loading

sosthene-nitrokey commented Sep 19, 2023

sosthene-nitrokey commented Sep 25, 2023

robin-nitrokey commented Sep 25, 2023

robin-nitrokey commented Sep 26, 2023

sosthene-nitrokey commented Sep 28, 2023

sosthene-nitrokey commented Sep 28, 2023

robin-nitrokey commented Sep 28, 2023 • edited Loading

robin-nitrokey commented Sep 28, 2023

robin-nitrokey commented Sep 28, 2023 • edited Loading

robin-nitrokey commented Sep 28, 2023

sosthene-nitrokey commented Oct 3, 2023 • edited Loading

robin-nitrokey commented Oct 4, 2023

daringer commented Oct 4, 2023 • edited Loading

sosthene-nitrokey commented Oct 4, 2023

robin-nitrokey commented Oct 4, 2023

sosthene-nitrokey commented Oct 4, 2023

robin-nitrokey commented Oct 4, 2023

robin-nitrokey commented Oct 4, 2023

sosthene-nitrokey commented Oct 4, 2023

robin-nitrokey left a comment

Choose a reason for hiding this comment

robin-nitrokey commented Oct 9, 2023

robin-nitrokey Oct 9, 2023

Choose a reason for hiding this comment

robin-nitrokey commented Oct 10, 2023

sosthene-nitrokey commented Oct 16, 2023

robin-nitrokey commented Oct 16, 2023

sosthene-nitrokey commented Oct 16, 2023

robin-nitrokey commented Oct 19, 2023

robin-nitrokey commented Oct 19, 2023

sosthene-nitrokey commented Oct 20, 2023

robin-nitrokey commented Oct 20, 2023

robin-nitrokey commented Oct 24, 2023 • edited Loading

sosthene-nitrokey commented Aug 22, 2023 •

edited

Loading

sosthene-nitrokey commented Sep 15, 2023 •

edited

Loading

robin-nitrokey commented Sep 28, 2023 •

edited

Loading

robin-nitrokey commented Sep 28, 2023 •

edited

Loading

sosthene-nitrokey commented Oct 3, 2023 •

edited

Loading

daringer commented Oct 4, 2023 •

edited

Loading

robin-nitrokey commented Oct 24, 2023 •

edited

Loading