Make COSE deserialization more robust #8
Conversation
|
lgtm, tests ran w/o issues |
Previously, COSE keys could only be deserialized if the map entries were
in the correct order. With this patch, the deserialization is made more
robust (and also more concise) by introducing a helper struct,
RawPublicKey, that is used as an intermediate step during the
deserialization. Also, we make the algorithm field optional to conform
with the spec.
This is a port from ctap-types:
trussed-dev/ctap-types#8
| trait PublicKeyConstants { | ||
| const KTY: Kty; | ||
| const ALG: Alg; | ||
| const CRV: Crv; | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| #[derive(Clone, Debug, Eq, PartialEq, Serialize)] | ||
| #[serde(into = "RawPublicKey")] |
There was a problem hiding this comment.
Why is there no with = "RawPublicKey" here?
There was a problem hiding this comment.
There is no with argument for containers. Do you mean from/try_from? I chose to manually implement Deserialize to make error handling easier, but we can also add our own error type and use TryFrom.
|
@robin-nitrokey can you add more context to this? Did you run into a specific issue? The reason we're currently not doing this is related to CTAP2 canonical CBOR encoding form. Now I agree that, while we need to be sure we emit "canonical" CBOR, it might be helpful to be less strict when receiving. Is there a kind of "non-exhaustive" issue? Generally, I thought that encoding/decoding these CTAP types was a matter of getting it right "once and for all". |
| } | ||
|
|
||
| #[derive(Clone, Debug, Eq, PartialEq, Serialize)] | ||
| #[serde(into = "RawPublicKey")] |
There was a problem hiding this comment.
Same comment as above, wouldn't we want Deserialize via with = "RawPublicKey" too?
|
@nickray A user reported an issue with |
|
I wasn’t aware that the standard also discourages accepting standard but non-canonical CBOR. If you want to keep this behavior, the approach in this PR could still be used to simplify the serde trait implementations. |
|
When reviewing this PR i checked the COSE spec and it says that canonical encoding rules applies do not apply to public keys. https://datatracker.ietf.org/doc/html/rfc8152#section-14
But the FIDO2 spec appears to be more constrained than that. |
|
Yes, you need to distinguish upstream COSE canonical encoding and "CTAP2 canonical CBOR encoding form" unfortunately. I think it's better to stick with the strict (applicable) standard and consider issues arising from this a bug - unless you are strongly of a different opinion. |
robin-nitrokey
force-pushed
the
cose
branch
2 times, most recently
from
ccd7363
to
c448bb9
Compare
|
I’ve rebased this PR and rewrote it based on the previous discussion. This means we still require canonical encoding (i. e. correct order), but also keep the simplified de-/serialization with |
|
I found one final problem during the implementation: The CTAP spec requires us to ignore unknown map keys. The documentation for Yet it does not work – So I think we have to stick with the current implementation here, failing for unknown keys until we have found all keys we want. |
|
So we'd need to implement support for |
|
There is already a pull request for it: trussed-dev/cbor-smol#5 |
|
Yes, with that we should be able to avoid the problem. But we could also do that separately as this implementation does not change the existing (working) behavior AFAIS. |
|
A PR that works on top of the latest Nitrokey/ctap-types main: trussed-dev/cbor-smol#6 |
|
Nice! Here is an improved implementation using |
This patch introduces the RawPublicKey helper type that takes care of serialization and deserialization of the key type. The *PublicKey structs now only need to check if all required fields are present and have the correct value. It also adds extensive tests to make sure that serialization and deserialization work correctly.
The algorithm field is optional, see RFC 8152 § 7:
COSE_Key = {
1 => tstr / int, ; kty
? 2 => bstr, ; kid
? 3 => tstr / int, ; alg
? 4 => [+ (tstr / int) ], ; key_ops
? 5 => bstr, ; Base IV
* label => values
}
alg: This parameter is used to restrict the algorithm that is used
with the key. If this parameter is present in the key structure,
the application MUST verify that this algorithm matches the
algorithm for which the key is being used.
This patch introduces the RawPublicKey helper type that takes care of serialization and deserialization of the key type. The *PublicKey structs now only need to check if all required fields are present and have the correct value. It also adds extensive tests to make sure that serialization and deserialization work correctly. This patch is ported from: trussed-dev/ctap-types#8
This patch introduces the RawPublicKey helper type that takes care of serialization and deserialization of the key type. The *PublicKey structs now only need to check if all required fields are present and have the correct value. It also adds extensive tests to make sure that serialization and deserialization work correctly. This patch is ported from: trussed-dev/ctap-types#8
Previously, COSE keys could only be deserialized if the map entries were in the correct order.With this patch, the deserialization is made more robust (and also more concise) by introducing a helper struct, RawPublicKey, that is used as an intermediate step during the deserialization. It also adds support for parsing keys without an algorithm field.