Proper public keys implementation #23
Conversation
The current webauthn standard is rather ambigious again. So I think there are some mistakes in the implementation that make our public key decoder too crypto-agile. The new draft of the webauthn standard is not ambigious, but also has an issue I'd like to be clarified first: w3c/webauthn#1446 it probably means `alg` should imply `crv` and we should remove support for Ed448 or wait for the spec to be more fleshed out
fido/Crypto/Fido2/PublicKey.hs
Outdated
|
|
||
| data ECDSAKey = ECDSAKey ECDSAIdentifier ECDSA.PublicKey deriving (Eq, Show) | ||
|
|
||
| instance Arbitrary ECDSAKey where |
There was a problem hiding this comment.
Probaly move all these arbitrary instances to the test module?
|
Specifically Section 5.8.5. of the draft forces us to make
|
| 2 -> ECDSAPublicKey <$> decodeECDSAPublicKey | ||
| x -> fail $ "unexpected kty: " ++ show x | ||
|
|
||
| decodeEdDSAKey :: Decoder s EdDSAKey |
There was a problem hiding this comment.
All of the other decoders have PublicKey in the name, but this one just Key. But “key” here does refer to the public key, right? To avoid confusion, I would call this one decodeEdDSAPublicKey.
| [userId] | ||
| pure $ Maybe.catMaybes $ fmap (mkCredential) $ credentialRows | ||
| where | ||
| mkCredential (id, x, y) = do | ||
| mkCredential (id, publicKey) = do | ||
| -- TODO(#22): Convert to the compressed representation so we don't need |
There was a problem hiding this comment.
This comment is now outdated.
Instead of translatign to cryptonite primitives, which is a bit lossy, we use sum types to model our domain! could remove a never reached error case because of this
arianvp
force-pushed
the
proper-public-keys
branch
from
b22ef54
to
af98137
Compare
Attestation and assertion have the same signature verification procedure, so we can reuse this later.
it's not listed as supported for webauthn.
arianvp
force-pushed
the
proper-public-keys
branch
from
81227bc
to
d9fb68d
Compare
Property tests for all the failure cases at least
The current webauthn standard is rather ambigious again. So I think
there are some mistakes in the implementation that make our public
key decoder too crypto-agile.
The new draft of the webauthn standard is not ambigious, but also
has an issue I'd like to be clarified first:
w3c/webauthn#1446
it probably means
algshould implycrvand we should removesupport for Ed448 or wait for the spec to be more fleshed out
Fixes #20