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

Remove serde support #53

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Remove serde support #53

merged 4 commits into from
Nov 16, 2023

Conversation

rozbb
Copy link
Owner

@rozbb rozbb commented Nov 9, 2023

This PR removes serde support. This is following a conversation elsewhere about providing serde impls for byte slices. The argument for removing serde impls is:

  1. If you derive the plain [T; N] serialization, or (T, T, ..., T) serialization, then you miss out on optimizations for formats with byteslice representations. This has a horrible slowdown, which I've experience with wasm_bindgen. This is why serde_bytes exists, and it cannot be merged into upstream (see above conversation)
  2. The choice of whether or not to use a length tag on the serialization depends on the format you're serializing to (see above conversation)
  3. The current p256 serde::Serialize implementation is the DER encoding of the public key. This is not something I noticed, and certainly doesn't match the raw byte representation of the other public key serializations. I should not have autoderived without looking. I don't think this is true. It's the bytes given by to_bytes().

In short, people have different needs when it comes to serializing HPKE values with serde, and any choice necessarily excludes other options. So rather than making that choice for the user, this crate should instead just expose its normal Serializable trait (with methods to_bytes and write_exact), and let the user implement what they need.

Looking for feedback on this, since it's removing a feature that might be in use currently.

cc @marmeladema @bwesterb @tarcieri

@tarcieri
Copy link

tarcieri commented Nov 9, 2023

The real kicker is this: serde-rs/serde#2120 (comment)

The serde data model lacks the concept of a homogeneously-typed fixed-size array/tuple. This means there is no universally agreed upon compact representation of a fixed-size byte array. The best you can do is using serialize_bytes on the underlying serializer, but this will add a length prefix even if the length is known in advance.

The only way to avoid a length prefix is to use serialize_tuple, however for some message formats like MessagePack this will include type information with each element, leading to a [u8; N] serialization which is much larger than N bytes.

@rozbb
Copy link
Owner Author

rozbb commented Nov 10, 2023

Ya exactly. I notice you said in the linked thread you were switching to serialize_bytes. In which crate is that happening? I don't think it's necessarily a good option here, but I'm curious to see.

@tarcieri
Copy link

It was in the serdect crate here: RustCrypto/formats#1112

It's designed specifically for constant-time bytes handling. Using serde_bytes was the only way to ensure encoding in MessagePack isn't data-dependent. The additional and often unnecessary length prefix is unfortunate.

@rozbb
Copy link
Owner Author

rozbb commented Nov 16, 2023

It looks like there is one project that is using this, here and here.

I also realized, I don't think the p256 impl is DER-encoded. The serde impls appear to just be a thin wrapper around to_bytes, which is good. So this all drops to the GenericArray serde impls. So I guess the move here is to document everything and offer some sample code for doing the conversion. Shouldn't be hard.

@rozbb rozbb merged commit 362d7bc into main Nov 16, 2023
12 checks passed
@rozbb rozbb deleted the remove-serde branch November 16, 2023 17:06
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.

2 participants