Serde implementation

[daxpedda]

Implemented serde::Deserialize and serde::Serialize for all types. This might be a bit overkill, implementing it for UInt might have been enough.

As serde currently doesn't support const generics (serde-rs/serde#1937), I used serde-big-array for the UInt implementation. The dependency is quiet minimal, this PR also spawned est31/serde-big-array#14 to make sure that the dependency is as minimal as possible.

I hope the tests are appropriate enough.

[tarcieri]

I've been meaning to extract a crypto-serde crate where we can implement serialization in a common place and test it across multiple formats. Serialization can be implemented in terms of helper types, which would reduce the overall amount of serde boilerplate we have across the project. Relevant: RustCrypto/elliptic-curves#536

I would definitely prefer not having to pull in crates like serde-big-array. Instead the data can be serialized as slices, which avoids the need for serde to have const generic support.

[daxpedda]

The reason why I was trying to avoid using slices is because when using DeserializeOwned it doesn't work. The correct way to do this is to use Visitor, which is exactly what serde-big-array does.

Alternatively, I could just copy the code of serde-big-array over, but it uses unsafe, which is maybe where a separate crate like crypto-serde could help. serde-big-array is really small, this is why I didn't worry about it, the maintainer is est31 (I want to avoid pinging him), he also hosts rcgen.

The main reason I made this PR is because currently ScalarCore is using slices to de/serialize and is failing when used with DeserializeOwned as pointed out above. My thinking was that if we implement the Serde traits on UInt directly we can do it correctly here instead of having to maintain things all over the place.

Preferably I would like to have this merged until we have crypto-serde. Alternatively we can scratch this and I can make PRs to fix this in elliptic-curve and other crates directly. I could also help jump-start the work on crypto-serde, but I have no idea about the vision, so more information would be helpful, you can also ping me on Zulip for that.

[tarcieri]

The main reason I made this PR is because currently ScalarCore is using slices to de/serialize and is failing when used with DeserializeOwned as pointed out above.

This has been bothering me enough I'll go ahead and spike out crypto-serde.

There are a lot of conflicting concerns that go into serde serializers which require tradeoffs, especially between different formats, so it'd be nice to have those all captured in one place.

I'm a bit surprised to hear that DeserializeOwned bounds aren't working when using borrowed values which are immediately consumed and turned into owned values. That should work? There's nothing magic about DeserializeOwned: it's just a marker trait that uses HRTBs.

I think we can get away with not copying and pasting code from serde-big-array, but I really need something concrete to work with in order to see the problems.

[daxpedda]

I'm a bit surprised to hear that DeserializeOwned bounds aren't working when using borrowed values which are immediately consumed and turned into owned values. That should work? There's nothing magic about DeserializeOwned: it's just a marker trait that uses HRTBs.

I honestly don't know enough about Serde to argue with you, but just to clarify, compilation isn't the problem, it just throws an error during runtime. I will make a small test to exemplify this.

[tarcieri]

@daxpedda are you sure the issue wasn't fixed by RustCrypto/elliptic-curves#536 ?

I think the most likely issue, especially if it's occurring at runtime, is the input format decoder doesn't allow borrowing from the input. That's the sort of concern it'd be really nice to solve in a single place like crypto-serde.

[daxpedda]

[dependencies]
bincode = "1"
# Can't compile without `pem`, this is a bug.
elliptic-curve = { version = "0.12.0-pre.1", features = ["pem"] }
p256 = { version = "0.11.0-pre.0", features = ["serde"] }
serde = "1"

[patch.crates-io]
elliptic-curve = { git = "https://github.com/RustCrypto/traits" }

use p256::Scalar;

fn main() {
    let scalar = Scalar::ZERO;

    let serialized = bincode::serialize(&scalar).unwrap();
    let deserialized: Scalar = bincode::deserialize_from(serialized.as_slice()).unwrap();

    assert_eq!(scalar, deserialized);
}

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom("invalid type: byte array, expected a borrowed byte array")', src/main.rs:7:81

Same error on the latest version on crates.io and master.

[tarcieri]

The issue there is most likely that the bincode crate doesn't allow borrowing from the input document, which is kind of surprising.

Again, this is why it'd be great to have the deserializer/serializer types in one place so we can test them across multiple serde-based format implementations so we can solve these concerns in a single place that's well tested against multiple serde format implementations.

Right now all serde support is spread out across several repos which is a maintenance nightmare.

[daxpedda]

I think the most likely issue, especially if it's occurring at runtime, is the input format decoder doesn't allow borrowing from the input.

<&[u8]>::deserialize(deserializer)?;

I think so too, this here just doesn't work. As far as I'm aware there is no way to avoid implementing a Visitor by hand to solve this problem.

That's the sort of concern it'd be really nice to solve in a single place like crypto-serde.

I'm happy to contribute 😃

[tarcieri]

Initial implementation of crypto-serde: RustCrypto/formats#531

[tarcieri]

@daxpedda you can use this now: https://crates.io/crates/serdect/0.1.0

[daxpedda]

Bug I found on the way. We need to test Encoding.

[daxpedda]

Couldn't come up with a cleaner way.

[tarcieri]

Yeah, it's kind of tough. I think GenericArray::default() would work if you use Self::from_le_byte_array and the ArrayEncoding trait.

Otherwise there's UInt::<Limbs>::Repr::default() but unfortunately those impls only go up to 32-bytes, so not only would you need a Default bound to use them but also it would limit the serializable size to U256 unnecessarily.