Upstream merge #46

daringer · 2022-04-18T10:26:45Z

merge notes/todos:

dispatch-fido is not needed anymore, thus remove (included in fido-auth with "dispatch" in features)
fido-authenticator has been taken from upstream/main for now -> @todo ?
- or does version 0.1 already contain all things we need ?
- so do we merge fido-authenticator upstream, or stay on version = 0.1
ctaphid.set_version(usbd_ctaphid::Version {
- don't we want that?
- in initilizer.rs
- added for now here
inside initializer.rs get_serial_number()
- afaik this should not be set for fido2-devices
- did not take the upstream implementation, just returning "00....00"
various RGB related changes and adaptations inside board/trussed.rs
- to be discussed if we really want a math-lib as dep to calc a LED-amplitude
- anyways RGB should be heavily tested
- added Intensities::scale_by() for easy scaling (currently misused by scaling with the amplitude...)
src/types.rs UsbClasses with or without serial ?
- maybe #cfg[] ? based on feature?
- serial pulled in for now...

- put NDEF app back in - set CTAPHID_INIT versions - blink in green when processing

szszszsz · 2022-04-19T10:49:33Z

components/usbd-ctaphid/src/constants.rs

-pub const MESSAGE_SIZE: usize = PACKET_SIZE - 7 + 128 * (PACKET_SIZE - 5);
+// 1200
+// pub const MESSAGE_SIZE: usize = ctap_types::sizes::REALISTIC_MAX_MESSAGE_SIZE;
+pub const MESSAGE_SIZE: usize = 3072;


This one looks arbitrary. To confirm its origin.

szszszsz · 2022-04-19T10:52:44Z

runners/lpc55/.cargo/config.toml

-  # "-Dwarnings",
+  "-Dwarnings",


What's this one doing? -D is of course defining a macro, but what's its purpose?

-D, --deny LINT Set lint denied – so this should make the compilation fail if there are compiler warnings. Makes sense for the CI, less so for development.

szszszsz · 2022-04-19T11:24:01Z

runners/lpc55/src/initializer.rs

+fn get_serial_number() -> &'static str {
+    /*static mut SERIAL_NUMBER: heapless::String<heapless::consts::U36> = heapless::String(heapless::i::String::new());
+    use core::fmt::Write;
+    unsafe {
+        let uuid = crate::hal::uuid();
+        SERIAL_NUMBER.write_fmt(format_args!("{}", hexstr!(&uuid))).unwrap();
+        &SERIAL_NUMBER
+    }*/
+    "00000000-0000-0000-00000000"
+}
+
+


IMO showing serial number should be configurable, with the default being the SN exposed to allow managing devices in the enterprise setting. Let's discuss this.

We had this discussion before the release and the conclusion was that we disable it by default and add an option to enable it later.

szszszsz · 2022-04-19T11:28:21Z

runners/lpc55/src/main.rs

+        // let remaining = msp() - 0x2000_0000;
+        // if remaining < 100_000 {
+        //     debug_now!("USB interrupt: remaining stack size: {} bytes", remaining);
+        // }


This debug code should be active in the develop builds, or removed completely.
Similarly other statements like that.

szszszsz · 2022-04-19T11:46:15Z

dispatch-fido is not needed anymore, thus remove (included in fido-auth with "dispatch" in features)
fido-authenticator has been taken from upstream/main for now -> @todo ?
or does version 0.1 already contain all things we need ?
so do we merge fido-authenticator upstream, or stay on version = 0.1

AFAIK fido-authenticator is updated with our changes, but potentially modified, and with that breaking user data compatibility. Let's use the released version. To do: check data retention for the FIDO2 key handles; verify changes against our last used.

ctaphid.set_version(usbd_ctaphid::Version {
don't we want that?
in initilizer.rs
added for now here

Yes, we do. See spec below. Let's make sure the proper version is used.

https://fidoalliance.org/specs/fido-v2.0-rd-20161004/fido-client-to-authenticator-protocol-v2.0-rd-20161004.html
- search for 6.1.9.1.2 CTAPHID_INIT

various RGB related changes and adaptations inside board/trussed.rs
to be discussed if we really want a math-lib as dep to calc a LED-amplitude
anyways RGB should be heavily tested
added Intensities::scale_by() for easy scaling (currently misused by scaling with the amplitude...)

Yes, math lib for that looks like overkill to me as well. Why the final values were not tabularized / calculated during the build via the const func? Do you know what was wrong with the initial sin approximation func?
See how this was done for the Nitrokey FIDO2:

https://github.com/Nitrokey/nitrokey-fido2-firmware/blob/master/targets/stm32l432/src/led.c#L17-L50

src/types.rs UsbClasses with or without serial ?
maybe #cfg[] ? based on feature?
serial pulled in for now...

AFAIK it got removed in the upstream, as this was mainly debug feature. Or am I wrong?
I do not see the need to leave it enabled for the release. Potentially worth to make it a feature for the fast debug purposes.

szszszsz · 2022-04-19T11:51:52Z

Final question: have you used the stable upstream release for the merge? Or an arbitrary commit?
And where is the CI build backlink? :|

robin-nitrokey · 2022-04-19T13:01:39Z

re. RGB: We want to change the LED patterns anyway and remove the pulsing. Maybe just change it to constant colors in this PR and then change it to the colors we actually want before the next release?

szszszsz · 2022-04-27T10:01:19Z

Bumping. ETA?

robin-nitrokey

Not tested yet, just based on the code change:

I’d also like some more context for the constant changes regarding CTAPHID packet size, as mentioned by Szczepan
I think we should change the LED code to keep the status quo for the moment – this should also remove the need for the math dependency.
Instead of setting an all-zero serial number, can’t we just remove the .serial_number(serial_number) call?

szszszsz · 2022-04-28T08:35:52Z

Is this fixing below by any chance?

SSH: Unable to download resident keys trussed-dev/fido-authenticator#10 (edit)

robin-nitrokey · 2022-04-28T09:01:48Z

Is this fixing below by any chance?

* [SSH: Unable to download resident keys solokeys/fido-authenticator#10](https://github.com/solokeys/fido-authenticator/issues/10) (edit)

That should already be fixed with our v1.0.1 release. The issue we are having with resident keys is querying them, not creating them.

Edit: I replied to an old version of your comment. The download issue is not fixed yet AFAIK, but I haven’t tested it yet with the current fido-authenticator.

runners/lpc55/Makefile

This patch removes solo2-specific targets from the Makefile.

This patch indicates the LED pulsing. This does not change the current behavior for production devices as LED pulsing is not working due to a missing peripheral in locked mode in the current firmware version.

Instead of setting an all-zero serial number for the USB device, we remove the serial number entirely to maintain the status quo.

This patch removes the -Dwarnings flag from the cargo configuration that fails the build if there is a warning. While this setting makes sense in the CI, it is annoying during development.

Due to recent changes in Trussed, the LED is blinking during a user confirmation request if it is processing data, for example on webauthn.bin.coffee. This patch restores the old behavior by using a custom Trussed version that does not change the LED when processing data and by fixing the refresh method in board/src/trussed.rs.

robin-nitrokey · 2022-05-24T08:16:56Z

I’ve added some commits to finalize this PR – in my opinion, it is ready now. @szszszsz @daringer Please review.

The only non-trivial change I made is the last commit. It is necessary to maintain the current LED behavior, but it has to use a patched Trussed version because of the upstream changes. Here is the required patch.

robin-nitrokey · 2022-05-24T08:17:59Z

Also, can someone please test NFC with these changes as there seem to be new issues for solo2?

runners/lpc55/Cargo.toml

runners/lpc55/board/src/trussed.rs

runners/lpc55/Cargo.toml

szszszsz · 2022-05-24T10:00:17Z

@daringer Engage!

daringer

+1

robin-nitrokey · 2022-06-02T11:45:25Z

To test:

Check whether fido-authenticator: Support downloading resident keys #48 is fixed
Re-test FIDO2 over NFC to rule out NFC issues as with latest solo2 beta firmware: https://gist.github.com/nickray/7de5d40d5d93b7a1b6d0983062c9339e

szszszsz · 2022-07-04T14:17:00Z

Looks good. I do not see any problems while using under the latest Windows 10. #48 is fixed.
Using Resident Keys works well, including OpenSSH.
As discussed, the following will be retested on the production hardware:

NFC,
User data retention.

nickray and others added 14 commits March 17, 2022 23:23

Inline never in usbd-{ccid,ctaphid}

78c673b

Bump dependencies to published versions

1b9dd4d

Bump versions, remove serial USB + ndef app

1a3da24

Implement winking, slight board code cleanup

082be57

Configurably larger CTAP pipe; winking

7302d5c

PC build fixes

d776a6d

Allow setting versions in CTAPHID_INIT

b43e3f2

Tweaks

223fe45

- put NDEF app back in - set CTAPHID_INIT versions - blink in green when processing

Big bug: No DWT.CYCCNT in locked devices - quick fix

8c1e81f

Move Monotonic and Reboot zero-sized types into board

483c0a3

Bump version, use merged fido-authenticator

a9727ca

Use published Trussed and apps

48b0637

Some more dependency fixes

330e93e

Merge upstream branch 'solo2/main'

37338c2

szszszsz reviewed Apr 19, 2022

View reviewed changes

robin-nitrokey mentioned this pull request Apr 27, 2022

CTAPHID_CANCEL doesn't seem to get handled correctly #47

Closed

robin-nitrokey requested changes Apr 27, 2022

View reviewed changes

robin-nitrokey reviewed May 17, 2022

View reviewed changes

runners/lpc55/Makefile Outdated Show resolved Hide resolved

robin-nitrokey added 5 commits May 19, 2022 20:23

Remove unnecessary targets from Makefile

567d142

This patch removes solo2-specific targets from the Makefile.

Disable LED pulsing

ebbd01e

This patch indicates the LED pulsing. This does not change the current behavior for production devices as LED pulsing is not working due to a missing peripheral in locked mode in the current firmware version.

Remove dummy serial number from USB device

3408e6c

Instead of setting an all-zero serial number for the USB device, we remove the serial number entirely to maintain the status quo.

Allow cargo warnings

4f75fb6

This patch removes the -Dwarnings flag from the cargo configuration that fails the build if there is a warning. While this setting makes sense in the CI, it is annoying during development.

robin-nitrokey marked this pull request as ready for review May 24, 2022 08:15

robin-nitrokey approved these changes May 24, 2022

View reviewed changes

robin-nitrokey mentioned this pull request May 24, 2022

fido-authenticator: Support downloading resident keys #48

Closed

szszszsz approved these changes May 24, 2022

View reviewed changes

runners/lpc55/Cargo.toml Show resolved Hide resolved

runners/lpc55/board/src/trussed.rs Show resolved Hide resolved

runners/lpc55/Cargo.toml Show resolved Hide resolved

daringer commented May 24, 2022

View reviewed changes

daringer merged commit e0b4461 into main May 24, 2022

szszszsz deleted the upstream-merge branch June 23, 2022 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream merge #46

Upstream merge #46

daringer commented Apr 18, 2022

szszszsz Apr 19, 2022

szszszsz Apr 19, 2022

robin-nitrokey Apr 19, 2022

szszszsz Apr 19, 2022

robin-nitrokey Apr 19, 2022

szszszsz Apr 19, 2022

szszszsz commented Apr 19, 2022

szszszsz commented Apr 19, 2022

robin-nitrokey commented Apr 19, 2022

szszszsz commented Apr 27, 2022

robin-nitrokey left a comment

szszszsz commented Apr 28, 2022 •

edited

Loading

robin-nitrokey commented Apr 28, 2022 •

edited

Loading

robin-nitrokey commented May 24, 2022

robin-nitrokey commented May 24, 2022

szszszsz commented May 24, 2022

daringer left a comment

robin-nitrokey commented Jun 2, 2022 •

edited by szszszsz

Loading

szszszsz commented Jul 4, 2022

Upstream merge #46

Upstream merge #46

Conversation

daringer commented Apr 18, 2022

szszszsz Apr 19, 2022

Choose a reason for hiding this comment

szszszsz Apr 19, 2022

Choose a reason for hiding this comment

robin-nitrokey Apr 19, 2022

Choose a reason for hiding this comment

szszszsz Apr 19, 2022

Choose a reason for hiding this comment

robin-nitrokey Apr 19, 2022

Choose a reason for hiding this comment

szszszsz Apr 19, 2022

Choose a reason for hiding this comment

szszszsz commented Apr 19, 2022

szszszsz commented Apr 19, 2022

robin-nitrokey commented Apr 19, 2022

szszszsz commented Apr 27, 2022

robin-nitrokey left a comment

Choose a reason for hiding this comment

szszszsz commented Apr 28, 2022 • edited Loading

robin-nitrokey commented Apr 28, 2022 • edited Loading

robin-nitrokey commented May 24, 2022

robin-nitrokey commented May 24, 2022

szszszsz commented May 24, 2022

daringer left a comment

Choose a reason for hiding this comment

robin-nitrokey commented Jun 2, 2022 • edited by szszszsz Loading

szszszsz commented Jul 4, 2022

szszszsz commented Apr 28, 2022 •

edited

Loading

robin-nitrokey commented Apr 28, 2022 •

edited

Loading

robin-nitrokey commented Jun 2, 2022 •

edited by szszszsz

Loading