Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep old credential ID for existing credentials #112

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Conversation

robin-nitrokey
Copy link
Member

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).

Fixes: #111


Notes:

  • This fix means that older credentials require more size. I think this is a reasonable trade-off.
  • To simplify the code, we could also remove the Local* types entirely and also implement Serialize with a helper struct.

This patch adds a test case that ensures that the calculated credential
ID for a credential that was created using the old (unstripped) format
is the same as the one generated originally.  Otherwise, the platform or
the RP could reject assertions because of a changed credential ID.
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
Copy link
Member Author

Successfully tested with my NK3CN dev device.

To simplify the Rp and User types introduced in the last commit and to
reduce code duplication, this patch removes the
LocalPublicKeyCredential*Entity types.  Instead the Rp and User types
always store a PublicKeyCredential*Entity together with the
serialization format.
@robin-nitrokey robin-nitrokey merged commit 63a1479 into main Dec 3, 2024
3 checks passed
@robin-nitrokey robin-nitrokey deleted the credential-id branch December 3, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Credential ID changed for old credentials
2 participants