search

duo-labs / webauthn

WebAuthn (FIDO2) server library written in Go
https://webauthn.io/
BSD 3-Clause "New" or "Revised" License
1.03k stars 162 forks source link

Use Canonical Encoding for re-marshalling credential unmarshal step #99

Closed arianvp closed 3 years ago

arianvp commented 3 years ago

Marshal seems to be unstable. Instead use CTAP2Encoding as per the Webauthn spec.

cbor.Marshal doesn't seem to be stable across runs. It produces different keyBytes on subsequent calls with the same input which is extremely confusing if you're inspecting the attestation data directly and comparing it to previous attestations.

Swap out for canonical Marshal as per Webauthn spec which uses a stable ordering of map elements.

Overall I'm confused why this step is made in the first place. We're not doing any error handling. what happens if Unmarshal fails? It will try to marshal nil in that case.

Why do we not pass the rawBytes through untouched to the rest of the code?

arianvp commented 3 years ago

Decided to completely remove the function in a second commit a276880fce8d80c46b7aa316fc5b88f186c9bd50 . I really think we shouldn't me marshalling and unmarshaling here.

Let me know if you disagree and i'll drop a276880fce8d80c46b7aa316fc5b88f186c9bd50 again.

arianvp commented 3 years ago

I think https://github.com/duo-labs/webauthn/pull/83 is a more generic fix?