tweag / webauthn

A library for parsing and validating webauthn/fido2 credentials
Apache License 2.0
34 stars 11 forks source link

demo server fails to start due to a parsing error #156

Closed eahlberg closed 1 year ago

eahlberg commented 1 year ago

When running the demo server with ./run.sh, the following error is thrown. (I omitted the subject key identifier, if it's of relevance I'll bring it back.)

...
Fetching Metadata
server: A attestationCertificateKeyIdentifier failed to parse because it has the wrong length for a SHA1 hash: <subjectKeyIdentifierText>
CallStack (from HasCallStack):
  error, called at src/MetadataFetch.hs:76:17 in main:MetadataFetch

The tests pass with the metadata from the repo (in tests/golden-metadata) but after updating the blobs using /bin/update-test-blob, the following error is thrown:

...
tests: ProcessingValidationErrors (InFuture :| [])
CallStack (from HasCallStack):
  error, called at tests/Main.hs:63:17 in main:Main

I'm on NixOS and use the recommended nix setup described in the README.

ErinvanderVeen commented 1 year ago

My first impression is that this is a bug in the metadata. Could you verify @infinisil

The relevant part of the metadata is:

        "attestationCertificateKeyIdentifiers": [
          "413486e96dbd403f9936dd65352a8dd5",
          "0a426ee17afd16533b1cdfa95de1e920a6aedf3a"
        ],

Which is defined in the metadata specification as:

This value MUST be calculated according to method 1 for computing the keyIdentifier as defined in [RFC5280] section 4.2.1.2.

Referring to this section:

The keyIdentifier is composed of the 160-bit SHA-1 hash of the value of the BIT STRING subjectPublicKey (excluding the tag, length, and number of unused bits).

Which is it obviously is not.

ErinvanderVeen commented 1 year ago

@eahlberg We have reported the issue to the fido alliance, and are considering adding a (temporary) workaround, but that will be on @infinisil's plate.

Thank you for reporting the issue!

We'll keep this issue open until the metadata is well-formed or we merge a workaround.

eahlberg commented 1 year ago

I was just testing out the repo so no rush on implementing a fix or workaround from my part, but thanks for the quick response!

infinisil commented 1 year ago

Alright, thanks for the issue though! I'd say let's wait for a response by the FIDO alliance before proceeding further. Wouldn't want to implement a workaround for somebody else's bug if we can avoid it.

infinisil commented 1 year ago

We got a reply, the mistaken record has been corrected, and I can confirm that the demo server runs again :)

eahlberg commented 1 year ago

Cool, thanks!