Serialize credential with fields names using only 1 bytes #59
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
from
9672662 to
9c82819
Compare
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
2 times, most recently
from
c4149f7 to
6bf26c2
Compare
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
from
6bf26c2 to
21c8325
Compare
robin-nitrokey
requested changes
Jul 26, 2024
Cargo.toml
Outdated
| @@ -76,9 +77,11 @@ ctap-types = { git = "https://github.com/trussed-dev/ctap-types.git", rev = "72e | |||
| ctaphid-dispatch = { git = "https://github.com/trussed-dev/ctaphid-dispatch.git", rev = "57cb3317878a8593847595319aa03ef17c29ec5b" } | |||
| apdu-dispatch = { git = "https://github.com/trussed-dev/apdu-dispatch.git", rev = "915fc237103fcecc29d0f0b73391f19abf6576de" } | |||
| littlefs2 = { git = "https://github.com/trussed-dev/littlefs2.git", rev = "2b45a7559ff44260c6dd693e4cb61f54ae5efc53" } | |||
| serde-indexed = { git = "https://github.com/sosthene-nitrokey/serde-indexed.git", rev = "5005d23cb4ee8622e62188ea0f9466146f851f0d" } | |||
There was a problem hiding this comment.
Looks like this PR does not use serde-indexed (anymore). Can we remove the patch?
src/credential.rs
Outdated
| @@ -205,15 +205,105 @@ impl Credential { | |||
| } | |||
| } | |||
|
|
|||
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with `serde_indexed` serialization | |||
There was a problem hiding this comment.
Suggested change
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with `serde_indexed` serialization | |
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with shorter field names for serialization |
src/credential.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with `serde_indexed` serialization |
There was a problem hiding this comment.
Suggested change
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with `serde_indexed` serialization | |
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with shorter field names for serialization |
src/credential.rs
Outdated
|
|
||
| /// Copy of [`ctap_types::webauthn::PublicKeyCredentialUserEntity`] but with `serde_indexed` serialization | ||
| #[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq, Eq)] | ||
| pub struct LocalPublicKeyCredentialUserEntity { |
There was a problem hiding this comment.
nit: move struct definition above the impls
src/credential.rs
Outdated
| @@ -724,6 +810,308 @@ mod test { | |||
| }); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn indexed_derive_rp_name_none() { | |||
There was a problem hiding this comment.
nit: s/indexed_derive/local/, also for the other tests
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
from
21c8325 to
7c2a3e5
Compare
robin-nitrokey
approved these changes
Jul 26, 2024
CHANGELOG.md
Outdated
| @@ -37,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| [#61]: https://github.com/Nitrokey/fido-authenticator/pull/61 | |||
| [#62]: https://github.com/Nitrokey/fido-authenticator/pull/62 | |||
| [#63]: https://github.com/Nitrokey/fido-authenticator/pull/63 | |||
| [#59]: ttps://github.com/Nitrokey/fido-authenticator/issues/59 | |||
daringer
approved these changes
Jul 26, 2024
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
from
7c2a3e5 to
5c77c23
Compare
This saves space when serializing credentials
sosthene-nitrokey
force-pushed
the
serde-name-one-char
branch
from
5c77c23 to
44f0299
Compare
robin-nitrokey
added a commit
that referenced
this pull request
Dec 2, 2024
In #59, we changed the format for serialized credentials to use shorter field names for the RP and user entities. This has an unintended side effect: For non-discoverable credentials that were generated with older crate versions, the stripped data embedded into the credential ID includes the RP and user. If we change their serialization format, we also change these credential IDs. We already supported deserializing both formats using a serde alias. This patch introduces helper enums that deserialize both formats using a custom Deserialize implementation and keep track of the used format. This format is then also used for serialization (using serde’s untagged mechanism that is not available for deserialization in no-std contexts). #59 Fixes: #111
robin-nitrokey
added a commit
that referenced
this pull request
Dec 2, 2024
In #59, we changed the format for serialized credentials to use shorter field names for the RP and user entities. This has an unintended side effect: For non-discoverable credentials that were generated with older crate versions, the stripped data embedded into the credential ID includes the RP and user. If we change their serialization format, we also change these credential IDs. We already supported deserializing both formats using a serde alias. This patch introduces helper enums that deserialize both formats using a custom Deserialize implementation and keep track of the used format. This format is then also used for serialization (using serde’s untagged mechanism that is not available for deserialization in no-std contexts). #59 Fixes: #111
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This saves space when serializing credentials.
We cannot use
#[serde(remote)]because that would require supporting#[serde(with)]inserde_indexed, already used forCredentialDataThis replaces #58 it should be 1 bytes larger per serialized fields, but is better than serde-indexed as it has les risks to break accidentally when adding/removing a field.