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

Fork or replace postcard #63

Open
robin-nitrokey opened this issue Jan 17, 2023 · 5 comments
Open

Fork or replace postcard #63

robin-nitrokey opened this issue Jan 17, 2023 · 5 comments

Comments

@robin-nitrokey
Copy link
Member

robin-nitrokey commented Jan 17, 2023

Upgrading postcard from 0.7 to 1.0 would be a breaking change and invalidate existing FIDO credentials generated with fido-authenticator. To avoid outdated dependencies, we should consider forking or replacing postcard.

@nickray
Copy link
Member

nickray commented Jan 25, 2023

If we replace, a possible alternative would be bincode v2 (unreleased), given its widespread use outside embedded.

@daringer
Copy link
Member

daringer commented Jan 25, 2023

This is less a technical question than a migration/user question first. I don't think it is an option to invalidate all FIDO credentials with a firmware update. So we first need a migration strategy, right? My take on this would be:

  • existing credentials should still use postcard 0.7
  • new credentials should use <insert-new-fancy-serializer>
  • during firmware update (or some other comm way), tell the user that she/should re-register u2f credentials
  • remove old postcard after a reasonable migration time

So far I understand, forking postcard would be the only option if we would not like to do this, right?

From the technical point of view, I don't really have an opinion, but benchmarks seem to swing in favor of bincode.

@robin-nitrokey
Copy link
Member Author

As far as I see, postcard deserialization is only used once: to deserialize reply::Encrypt. My first approach would be see if we can just manually deserialize this struct.

let reply::Encrypt {
ciphertext,
nonce,
tag,
} = crate::postcard_deserialize(&request.wrapped_key).map_err(|_| Error::CborError)?;

trussed/src/api.rs

Lines 382 to 385 in 8e347ab

Encrypt:
- ciphertext: Message
- nonce: ShortData
- tag: ShortData

@robin-nitrokey
Copy link
Member Author

Here is a quick draft of how a manual implementation could look like: robin-nitrokey@11429b7 It’s not trivial, but not too bad either – 50 lines of code that are rather easy to review and to test, no external dependencies.

@nickray
Copy link
Member

nickray commented Jan 30, 2023

Nice - this amount of code seems reasonable to keep around for a rather long "reasonable migration time".

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

No branches or pull requests

3 participants