Implement all commands of Ctap2 #154

msirringhaus · 2021-06-16T12:17:39Z

These are now all the commands defined in the spec.
Not all functionality is there yet, and a few testcases are missing, as the spec doesn't give examples for those.
Most notably: All crypto-usage is still missing. For this, we need something like #142 with more functionality and then implement that for different crypto backends (NSS, openssl, ring, ..).

Next step would be to create a new Manager and StateMachine, which uses the new commands.

…mbers)

…COSE-crate is added and able to actually parse it

…g it (which would need a DER-parser)

…d out

…ich has to be calculated before hand

…us-byte

bertvandepoel · 2021-07-01T01:18:11Z

I don't really know much about the spec, so I was wondering whether the crypto-usage stuff, which requires #142 to be finalised and implemented, is a strict requirement or a "nice to have". I assume this PR can't be merged before the manager and statemachine are done and there are no more WIP commits then? Mostly just trying to understand whether webauthn support in firefox is in the near future or not at all.

msirringhaus · 2021-07-01T06:44:37Z

The crypto-stuff is a strict requirement for hardware tokens that support a PIN (which is pretty much most of the FIDO2 compatible ones). In my branch, I already have an openssl-backend going (probably highly insecure) which works, and am currently working on the NSS-backend (need to check for existing bindings or write my own).

The statemachine and manager are mostly functional (although some CTAP1-backwards compatibility is still missing).
Those are all not in the PR yet, as it is pretty huge already and I don't want to make it unreviewable.

Once all of that is done (assuming it passes review at some point), we still need to integrate it into Firefox (Rust<->C glue, new User dialogs, etc.). So yes, its coming, but not tomorrow. I still work more or less full time on this.

Just look at my fork on the ctap2-cont branch for the current state.

In case you (or @theSuess) want to help: More testcases would be nice. Esp. for the GetAssertion and GetNextAssertion commands.

bertvandepoel · 2021-09-13T00:35:49Z

I'm coming back to this after a busy summer (sorry for the delay!), and was wondering what you exactly mean with testcases? Do you mean use our devices to test things, or more literally unit tests. The first one I could probably help with with my Mooltipass miniBLE, but the latter might be a bit out of my league since I don't know any Rust. I see @theSuess wrote msirringhaus#1 but I'm not too sure whether or not that's related.

msirringhaus · 2021-09-27T11:10:19Z

Now its my turn to apologize for the delay :-)
In my original statement, I actually meant unit tests, yes (exactly like the one you mentioned). But trying out as many devices as possible would also be very beneficial!

And thank you for pointing out @theSuess 's PR to me, as I completely missed that one (github somehow didn't notify me about it).

As a quick general update: This project has been on hold for the past two months or so, because of unrelated FF91esr-work (plus some more weeks of vacation), which is still ongoing. But hopefully, this will be done soon, so I can pick this up again.
In theory, I now have a NSS crypto-backend going, and was able to put my code (locally) into Firefox and successfully use CTAP2 on https://demo.yubico.com/ . Most of the Firefox-integration C++ code is still very hacky and it is missing a PIN-dialog for the user, but I personally would consider the Rust part "almost good enough for now". But all of this is needs to be reviewed and accepted by Mozilla's security team, of course. They probably will have a lot to say about "good enough", I'm sure ;-)

bertvandepoel · 2021-09-27T23:22:40Z

No worries, I presumed some vacation was for sure involved. Important to recharge your batteries. If you at some stage would like em to test with my Mooltipass miniBLE, then feel free to let me know!

rmartinho

Ignoring all the TODO stuff, this looks good so far, I just left a few suggestions here and there.

rmartinho · 2021-10-11T11:23:06Z

src/ctap2/client_data.rs

+    pub challenge: Challenge,
+    pub origin: Origin,
+    // Should not be skipped but default to False according to https://www.w3.org/TR/webauthn/#collectedclientdata-hash-of-the-serialized-client-data
+    #[serde(rename = "crossOrigin", skip_serializing_if = "Option::is_none")]


We should make this #[serde(rename = "crossOrigin", serialize_with ="some_fn_that_serializes_none_to_false")]

This is already fixed in my follow-up branch. It is not an Option anymore, and serde is using bool::default now.
Do you want me to backport that, or just look at it with the next PR?

That's ok, we can look at it in the next one, then.

rmartinho · 2021-10-11T11:28:12Z

src/ctap2/client_data.rs

+            {
+                let mut out = [0u8; 32];
+                if out.len() != v.len() {
+                    return Err(E::custom("unexpected byte len"));


I think we should use E::invalid_length here instead of a custom error.

rmartinho · 2021-10-11T11:32:11Z

Cargo.toml

-base64 = { version = "^0.10", optional = true }
+base64 = "^0.10"
+nom = "^5.1"
+sha2 = "^0.8.0"


if we use sha2 0.9, it uses digest 0.9, which in turn uses generic-array 0.13, which supports has impl Into<[T; N]> for GenericArrays.

I have tried to use the versions that are already vendored in Firefox, to avoid update procedures there that might cause problems. Currently, Firefox has sha2 in version 0.8.2.
If you think it's not a problem to bump that dependency in Firefox as well, I can go with 0.9.

rmartinho · 2021-10-11T11:33:34Z

src/ctap2/client_data.rs

+        hasher.input(&data);
+
+        let mut output = [0u8; 32];
+        output.copy_from_slice(hasher.result().as_slice());


It's nicer to just use hasher.result().into(); see comment on Cargo.toml.

rmartinho · 2021-10-11T11:35:18Z

src/ctap2/client_data.rs

+            token_binding: Some(TokenBinding::Present(vec![0x00, 0x01, 0x02, 0x03])),
+        };
+
+        assert_eq!(


Is this expected hash correct? AFAICS the implementation will not serialize properly when cross_origin is None, so the test shouldn't pass right now.

rmartinho · 2021-11-04T15:11:22Z

src/ctap2/commands/mod.rs

@@ -314,7 +324,14 @@ impl Into<u8> for StatusCode {
 pub enum CommandError {
    InputTooSmall,
    MissingRequiredField(&'static str),
-    Deserializing(SerdeError),
+    Deserializing(CborError),
+    Serialization(CborError),


Can we name these two consistently? Either Deerializing/Serializing, or Deserialization/Serialization

rmartinho · 2021-11-04T15:23:29Z

src/ctap2/commands/get_assertion.rs

+        Dev: FidoDevice + io::Read + io::Write + fmt::Debug,
+    {
+        if input.is_empty() {
+            return Err(CommandError::InputTooSmall).map_err(HIDError::Command);


I guess this is a matter of style, and I see it in many places, but it looks weird to me to just call map_err like this when we could just use HIDError::Command(...); or alternatively we can just implement From as appropriate and rely on ?

…'Parsing'-variant

…bit weird)

…ls, by using the shorter .into()

msirringhaus · 2021-11-08T10:24:33Z

That would be it from my side for now. Remaining questions are sha2-version bump and backporting of the crossOrigin-changes. I hope the follow-up commits resolve the other issues.

rmartinho

This looks fine as is. The sha2 version bump we can handle later, and there's no need to spend time backporting stuff onto this if they're coming in the follow-up PR anyway.

Martin Sirringhaus added 12 commits June 16, 2021 13:24

WIP: Add part MakeCredentials

bfe6fb4

Add test for (and do some fixes) for MakeCredential CTAP1 mapping

0e746fb

Adjust tests with RelyingParty and name-field

c35619e

Fix serialization of MakeCredentialOptions (individually serialize me…

26f231a

…mbers)

Temporarily parse credential_public_key just as a byte-vector, until …

f7dd900

…COSE-crate is added and able to actually parse it

Add Option Unparsed to AttestationStatement, to currently skip parsin…

bf12cdb

…g it (which would need a DER-parser)

Add commands GetAssertion and GetAssertionNext. Still missing tests.

4345004

WIP: Add clientpin command with all the crypto functionality commente…

d62a52a

…d out

Remove duplicate Pin-definition

15eb9ed

MakeCredentials and GetAssertion now do not store Pin but PinAuth, wh…

2980fb8

…ich has to be calculated before hand

MakeCredentials: Remove pointless alias for HIDError

5f3930c

GetInfo: Fix wrong parsing of response. Forgot to strip away the stat…

1b085b2

…us-byte

rmartinho reviewed Nov 4, 2021

View reviewed changes

Martin Sirringhaus added 3 commits November 5, 2021 12:58

Make naming of CommandError variants consistent and remove redundant …

e206255

…'Parsing'-variant

Replace custom error with invalid_length (even though the usage is a …

a147e50

…bit weird)

Use From<CommandError>-trait of HIDError to avoid verbose map_err-cal…

386d616

…ls, by using the shorter .into()

rmartinho approved these changes Nov 15, 2021

View reviewed changes

dveditz merged commit 33c26aa into mozilla:ctap2-2021 Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement all commands of Ctap2 #154

Implement all commands of Ctap2 #154

msirringhaus commented Jun 16, 2021

bertvandepoel commented Jul 1, 2021

msirringhaus commented Jul 1, 2021

bertvandepoel commented Sep 13, 2021

msirringhaus commented Sep 27, 2021

bertvandepoel commented Sep 27, 2021

rmartinho left a comment

rmartinho Oct 11, 2021

msirringhaus Nov 5, 2021

rmartinho Nov 15, 2021

rmartinho Oct 11, 2021

rmartinho Oct 11, 2021

msirringhaus Nov 5, 2021

rmartinho Oct 11, 2021

rmartinho Oct 11, 2021

rmartinho Nov 4, 2021

rmartinho Nov 4, 2021 •

edited

Loading

msirringhaus commented Nov 8, 2021

rmartinho left a comment

Implement all commands of Ctap2 #154

Implement all commands of Ctap2 #154

Conversation

msirringhaus commented Jun 16, 2021

bertvandepoel commented Jul 1, 2021

msirringhaus commented Jul 1, 2021

bertvandepoel commented Sep 13, 2021

msirringhaus commented Sep 27, 2021

bertvandepoel commented Sep 27, 2021

rmartinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartinho Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

msirringhaus commented Nov 8, 2021

rmartinho left a comment

Choose a reason for hiding this comment

rmartinho Nov 4, 2021 •

edited

Loading