Introduce `codec` module #166

tgeoghegan · 2022-02-02T18:47:30Z

Introduces a new module with traits for encoding and decoding of
messages in TLS syntax into Vec<u8> or from std::io::Cursor<&[u8]>.
This is inspired by rustls's
codec
which uses a custom Reader type instead of Cursor.

We might have defined these traits generically in terms of
std::io::{Read,Write}, but then those generic parameters would also
have to appear all the way up in traits vdaf::{Vdaf, Aggregator}. It
also makes more sense given our use case: the TLS wire encoding isn't
well suited to streaming encoded objects into a W: Write because
variable length objects are encoded as length || object. That means
you need to know the length of an object before you can write it
somewhere, making it more natural to decode from byte slices and encode
into byte vectors. We expect that messages will never be terribly big,
meaning itshouldn't be unreasonable to encode them into a buffer on the
heap before writing them to a network connection or other byte sink. If
it works for a production quality TLS implementation, it should work for
us. Finally, if we only ever encode into in-memory buffers, then we can
make Encode::encode infallible.

We implement these new traits on vdaf::prio3::Prio3InputShare,
Prio3PrepareMessage and vdaf::Share. We also use the new traits to
tighten bounds on associated types on vdaf::Vdaf::InputShare and
vdaf::Aggregator::PrepareMessage. The new associated types
InputShareDecodingParameter and PrepareMessageDecodingParameter
allow generically providing context-specific parameters for message
decoding (i.e., expected sizes of various message portions) without
needing to complicate the Encode or Decode traits by defining them
in terms of Vdaf.

tgeoghegan · 2022-02-02T19:01:01Z

This is far enough along that we can start discussing it.

If we move forward with this strategy, there's a bunch more work that we'd have to do:

implement Encode, Decode on the remaining Prio3 protocol messages
implement Encode, Decode on all the poplar1 protocol messages (currently I just disable the vdaf/poplar1 module
(maybe) implement Encode, Decode on trait FieldElement

To look at how this gets used outside of libprio, see the implementation on hpke::Config in my ppm-prototype. See also that project's codec module which adds functions for encoding variable length byte vectors.

cjpatton

I think this is a pretty good idea. In particular I'm supportive of following the lead of rustls and just defining the trait we need for decoing things. I also really like the idea of making the "decoding context" generic.

Before giving detailed feedback, let me leave just one high level comment: I think the API changes made to Vdaf can be simplified significantly. Namely, there's no need to define an explicit "decoding context" for input shares, in particular because it's necessarily the case that the verification parameter is sufficient context for decoding. This you could just use VerifyParam for the decoding parameter. (Similarly for prepare messages -- see inline comments.)

Finally, one high level question: does ppm-prototype plan to use this trait? If so, where do you plan to define it? Note that I don't expect that the "decoding context" will necessary for PPM: The length of the next chunk to decode will always be provided.

src/codec.rs

src/vdaf.rs

tgeoghegan · 2022-02-05T00:26:47Z

Before giving detailed feedback, let me leave just one high level comment: I think the API changes made to Vdaf can be simplified significantly. Namely, there's no need to define an explicit "decoding context" for input shares, in particular because it's necessarily the case that the verification parameter is sufficient context for decoding. This you could just use VerifyParam for the decoding parameter. (Similarly for prepare messages -- see inline comments.)

Nice, this will make this much simpler!

Finally, one high level question: does ppm-prototype plan to use this trait? If so, where do you plan to define it? Note that I don't expect that the "decoding context" will necessary for PPM: The length of the next chunk to decode will always be provided.

I am using it in ppm-prototype, which is why mod codec is exported from libprio. I also have not yet found that a decoding context is ever needed in PPM messages. As for where the codec lives, I'm not sure what to do. It could stay in crate prio to both be used therein and be exported to clients of prio, but we could also put it into its own crate, which might be useful in the future to other Rust projects that want to use TLS syntax.

tgeoghegan · 2022-02-05T00:29:43Z

Leaving myself some notes for when I pick this up next week

Implement cjpatton's suggestion of using VerifyParam and PrepareStep to figure out how to decode messages.
Add functions to mod codec for encoding vectors of bytes or objects
Implement traits Encoder, Decoder on FieldElement and remaining VDAF messages

Implement latest proposed draft of ppm specification (see ietf-wg-ppm/draft-ietf-ppm-dap#179). Also adopts the new codec traits in libprio for protocol messages (divviup/libprio-rs#166).

tgeoghegan · 2022-02-11T21:17:54Z

This is ready for review! I've broken it up into a series of commits that hopefully make review manageable. To see the new encoding/decoding stuff "in action", see https://github.com/abetterinternet/ppm-prototype at commit a0ed255336762ac1ef1ea08b459cb725996851d5. Besides usage of the Encode and Decode traits for PPM protocol messages, that demonstrates that the trait bounds on Vdaf and traits like Vdaf::PrepareStep make it possible to implement PPM clients, helper, leaders and collectors generically over implementations of vdaf::{Vdaf, Collector, Client, Aggregator}.

tgeoghegan · 2022-02-11T21:33:31Z

We need to check the encoding of Prio3InputShare and Prio3PrepareMessage against the specification in VDAF. I think I have it right, except that I don't treat Prio3InputShare.joint_rand_seed_hint and Prio3InputShare.blind as optional, but then the prio3 implementation doesn't currently treat those as optional, either.

src/codec.rs

branlwyd · 2022-02-11T21:54:36Z

src/codec.rs

+    }
+
+    let len = bytes.len() - len_offset - 1;
+    debug_assert!(len <= 0xff);


IMO, this should probably be more than a debug assertion -- in production releases, we'll silently generate a bad (unparseable) serialization if we ever try to encode data longer than our maximum length.

We probably don't want to panic, since this error might be triggered by data at runtime (i.e. it's not a programmer error), and I don't think we want to update the contract of this function to say it panics if you give it too much data because it would be pretty hard to determine if we were going to hit this without e.g. double-encoding everything, the first time to count the length and the second time to actually encode. I suppose we could return an error; I'm not sure how much complexity dealing with this error will add to the code that calls this function, but I think it's important for correctness.

I don't have a strong opinion here, but FWIW, it's typical for a TLS stack to panic in this situation:

Here's the analogous pattern in rustls: https://github.com/rustls/rustls/blob/main/rustls/src/msgs/codec.rs#L206

Here's ClientHello extension serialization in golang: https://github.com/golang/go/blob/master/src/crypto/tls/handshake_messages.go#L299

I think people are generally happy to panic in this situation because it's usually a sign of programmer error. In particular, any configuration of a TLS client that would result in it trying to produce a ClientHello that can't be represented should be prevented by the program.

I agree with both of you! With @branlwyd in that these should be hard asserts if they assert at all, and with @cjpatton in that panicking is OK in the face of such an error.

We should definitely detect bogus lengths and return errors on the decode side, where we are handling messages from some unknown peer, but on the encode side, I agree with Chris that this just indicates an error, and we should panic in order to (1) noisily indicate to the programmer that they messed up and (2) not introduce error conditions to the encode side that would require changing the function signatures.

Like I said, I don't object to taking @branlwyd's suggestion. I just wanted to point out that the current pattern is not atypical. (On the other hand, I've also seen libraries that return an error rather than panic. Doing so is not very ergonomic in Go or C++, but it's definitely ergonomic in Rust :) )

branlwyd · 2022-02-11T22:08:11Z

src/codec.rs

+
+    // Create cursor over specified portion of provided cursor to ensure we
+    // can't read past len
+    let mut sub = Cursor::new(&bytes.get_ref()[initial_position..initial_position + length]);


This will panic with a slice-out-of-bounds error if length specifies more bytes than are available in the cursor; length comes from the message data itself, i.e. it should be considered untrusted.

While we're touching this, I think this logic could be specified slightly more simply -- here's two suggestions:

We could call bytes.take(length). This is (locally) simple and would let us drop the update-the-outer-cursor logic on L184, but we'd need to refactor the rest of this codec code to work against a Read implementation (which probably sinks this suggestion, since I suspect this would be a fairly large refactor).

If that's undesirable, I think a check that length is not too large, followed by &bytes.remaining_slice()[..length], is the best I can think of. Using remaining_slice rather than get_ref simplifies the slicing math a bit.

At a minimum I will eliminate the chance of a panic.

Cursor::remaining_slice is experimental so I don't want to use it yet. I added some logic to check whether the provided length goes past the end of the outer bytes, and also checks for overflow on usize.

branlwyd · 2022-02-11T22:24:39Z

src/codec.rs

+/// Encode the `items` into `bytes` as a vector of `items.len()` items, up to
+/// `0xff`.
+pub fn encode_items_u8<E: Encode>(bytes: &mut Vec<u8>, items: &[E]) {
+    assert!(items.len() <= 0xff);


I think the logic here is incorrect: in TLS structures, the prefixed length is the number of bytes, rather than the number of items.

See https://datatracker.ietf.org/doc/html/rfc8446#section-3.4 for the nitty-gritty details: "In either case, the length declares the number of bytes, not the number of elements, in the vector. [...] When [variable-length vectors] are encoded, the actual length precedes the vector's contents in the byte stream."

For practical evidence, see https://tls13.ulfheim.net: if you click through to the Client Hello message and open its annotations, and look to the "Cipher Suites" section, notice that the list contains three elements of two bytes each, so the length is listed as 6.

Yes, you are right, and now that I think of it, it has to be this way: the point of the length prefix is so that TLS implementations that don't know about a particular message type can skip past it in a byte stream, and getting the number of elements doesn't help in that case. Thanks for catching that! This being the case, I think I can delete some of the encode/decode_items_* variants.

We now have only encode_*_items and decode_*_items, renamed from {encode,decode}_items_opaque_*. I also got rid of the {encode,decode}_opaque_* variants. I added those to optimize working with &[u8] or Vec<u8> without having to invoke u8::{decode,encode] many many times, but for now, I want to trust rustc to optimize that case well, and we can bring back those variants in the future if we need them. In the meantime, they're a bit confusing.

branlwyd · 2022-02-11T22:59:47Z

src/codec.rs

+/// Encode the `items` into `bytes` as a vector of `items.len()` items, up to
+/// `0xff`.
+pub fn encode_items_u8<E: Encode>(bytes: &mut Vec<u8>, items: &[E]) {
+    assert!(items.len() <= 0xff);


I'm a little wary of the length-checking asserts in the encoding methods -- I think these functions should return an error instead of panicking if handed too much data. My reasoning is this:

The application code that eventually ends up using this library is probably working with "higher-level" objects, whose encoding sizes they are very much unaware of (and we would not like to make them write code to check, both to avoid double-encoding and to avoid repetition of length-checking code).

The encoding implementation code could check the length and return an error before calling e.g. encode_items_u8 -- but then we have to make sure every encoding implementation remembers to write this length-checking code.

Therefore, it's simplest to check the length here and return an error. Rust itself will stop us from forgetting to handle the result.

I really like that encode is infallible, but I think you're right about checking the item count and returning an error here.

This particular case is obsolete because the function encode_items_u8 was based on my misreading of RFC 8446. However encode() does not return an error per your suggestion.

branlwyd · 2022-02-11T23:07:29Z

src/codec.rs

+    }
+}
+
+impl Decode<()> for u8 {


For the Encode/Decode implementations on primitive numeric types, consider use of the byteorder crate -- I think all of the implementations would be reduced to a one-liner.

Yes, functions like read_u16 and friends look like what we want here.

Done! byteorder even provides read_u24 and write_u24. There's one tricky aspect here: on the encode side, the one liners are implemented in terms of trait Write, which means that they can fail with std::io::Error. But in fact these writes are always infallible, because we are only ever writing to an in-memory buffer. Now, if we make the Encode side fallible anyway because we upgrade the debug_assert! statements to returning errors, that's OK. But if we don't go that way, then I don't think it's worth foisting error handling on callers of encode() just to save 1-2 lines of code.

Note that byteorder does provide methods like ByteOrder::write_u16 that infallibly write to a &mut [u8], but that's not qutie what we want. We are writing into Vec<u8>, so you'd think we could just do BigEndian.write_u16(&the_vec, 15u16), but that won't work, because the Vec isn't necessarily big enough yet to receive the write, and so it will panic (playground). That means we're on the hook for growing the vector, at which point we're back to the complexity we had before we tried using ByteOrder.

cjpatton

My first pass includes comments on codec.rs and vdaf.rs. I'll take a close look at prio3 and poplar1 in the next round.

src/codec.rs

src/vdaf.rs

src/vdaf/prio3.rs

tgeoghegan

Thanks for the thorough and insightful reviews -- I will address these early next week.

src/codec.rs

tgeoghegan · 2022-02-12T00:39:27Z

src/codec.rs

+    }
+}
+
+impl Decode<()> for u8 {


Yes, functions like read_u16 and friends look like what we want here.

src/codec.rs

src/vdaf.rs

tgeoghegan · 2022-02-12T00:54:23Z

src/codec.rs

+
+    // Create cursor over specified portion of provided cursor to ensure we
+    // can't read past len
+    let mut sub = Cursor::new(&bytes.get_ref()[initial_position..initial_position + length]);


At a minimum I will eliminate the chance of a panic.

tgeoghegan · 2022-02-12T00:56:39Z

src/codec.rs

+/// Encode the `items` into `bytes` as a vector of `items.len()` items, up to
+/// `0xff`.
+pub fn encode_items_u8<E: Encode>(bytes: &mut Vec<u8>, items: &[E]) {
+    assert!(items.len() <= 0xff);


I really like that encode is infallible, but I think you're right about checking the item count and returning an error here.

src/codec.rs

cjpatton

Coming together! Two high level comments:

The documentation for the codec module could be improved. I think this module accomplishes two, separable tasks:
- First, it defines traits for encoding/decoding objects that require additional context for decoding (in particular, VDAF messages). Notably, these traits are more general than required for TLS messages, since TLS messages don't require context for decoding.
- Second, it provides implementations of these traits for primitives encoded in TLS wire format. It also provides helper functions for serializing TLS structures.
I'm weary of making too many changes to poplar1 right now, in particular implementing serialization. The spec for poplar1 is far less mature than for prio3. The internals are going to change significantly, at which time we'll also have to re-do all of the serialization code anyway. We'll need implementation of Encode and Decode in order to satisfy trait bounds, however I think the implementation of each of the methods should be panic("not implemented") instead of functioning code.

src/codec.rs

src/vdaf/poplar1.rs

cjpatton · 2022-02-16T20:44:57Z

src/vdaf/poplar1.rs

+    }
+}
+
+/// An input share for the heavy hitters VDAF.


Might as well

Suggested change

/// An input share for the heavy hitters VDAF.

/// An input share for the poplar1 VDAF.

src/vdaf/suite.rs

src/vdaf/prio3.rs

src/codec.rs

branlwyd

Approving with a few comments -- I took a close look at codec.rs and a cursory look at the remainder of the files in the PR.

src/codec.rs

cjpatton

Really nice work. My only objection is that we are likely going to re-do serialization for poplar1, but I'm happy to ship it so long as folks are aware of this. (In any case, prio3 may change a little as I work towards interop with the reference implementation.)

tgeoghegan · 2022-02-18T00:30:15Z

Yes, I anticipate a lot of churn in poplar1 as we bring it to readiness for production so I'm not too worried about how "good" the current serializations are.

I had a thought for another enhancement. With this PR, we have functions:

pub fn encode_u16_items<E: Encode>(bytes: &mut Vec<u8>, items: &[E]);

pub fn decode_u16_items<P, D: Decode<P>>(
    decoding_parameter: &P,
    bytes: &mut Cursor<&[u8]>,
) -> Result<Vec<D>, CodecError>;

These are meant to encode things like Structure seq<1..2^16-1>. But RFC 8446 has examples like:

opaque mandatory<300..400>;
            /* length field is two bytes, cannot be empty */
      uint16 longer<0..800>;
            /* zero to 400 16-bit unsigned integers */

We currently have no way of enforcing bounds like 300..400 or 0..800. So maybe the encode_*_items and decode_*_items should take an std::ops::Range argument so that the encode side can check that the length of the encoding of &[E] is within bounds and the decode side could do the same for the length prefix encoded into bytes.

However I don't want to tack that onto this already large PR so I'll revisit this idea later.

Introduces a new module with traits for encoding and decoding of messages in TLS syntax into `Vec<u8>` or from `std::io::Cursor<&[u8]>`. This is inspired by `rustls`'s [`codec`](https:////github.com/rustls/rustls/blob/main/rustls/src/msgs/codec.rs) which uses a custom `Reader` type instead of `Cursor`. We might have defined these traits generically in terms of `std::io::{Read,Write}`, but then those generic parameters would also have to appear all the way up in traits `vdaf::{Vdaf, Aggregator}`. It also wouldn't help much given our use case: the TLS wire encoding isn't well suited to streaming encoded objects into a `W: Write` because variable length objects are encoded as `length || object`. That means you need to know the length of an object before you can write it somewhere, making it more natural to decode from byte slices and encode into byte vectors. We expect that messages will never be terribly big, meaning it shouldn't be unreasonable to encode them into a buffer on the heap before writing them to a network connection or other byte sink. If it works for a production quality TLS implementation, it should work for us. Finally, if we only ever encode into in-memory buffers, then we can make `Encode::encode` infallible. `mod codec` also provides some utility routines for encoding vectors of objects, opaque byte vectors, or vectors of objects as opaque byte vectors. See the module-level comment for details.

Provides implementation of codec traits on `vdaf::suite::Key` and all implementations of trait `FieldElement`. These types appear in several message types that will need to implement `Encode` and `Decode`. We also remove the `serde::{Serialize,Deserialize}` traits from those types as we never expect to need to encode or decode crate `prio` types to anything but TLS syntax wire messages.

Adds `Encode` and `Decode` implementations for the `prio3` VDAF protocol messages, as well as the `Share` and `AggregateShare` types defined in module `vdaf`. We use the existing `Prio3VerifyParam` and `Prio3PrepareStep` as the decoding parameters for `Prio3InputShare` and `Prio3PrepareMessage`, respectively, adding fields where necessary to provide the context required to decode messages from the wire. Since we now do all encoding and decoding of `FieldElement` implementations via the new traits, we can delete `FieldElement::try_from_reader`.

Adds `Encode` and `Decode` implementations for the `poplar1` VDAF protocol messages. The wire encoding of `poplar1` is not yet specified by VDAF, so I have made some guesses here. In particular, I'm not certain whether the length of some message members can be known statically based on a VDAF instantiation and so have encoded them with variable length in a few places.

Adds trait bounds into various associated types on trait `Vdaf` so that we can (1) ensure that any VDAF implementation provides the necessary `Encode` and `Decode` implementations on its associated types and (2) ensure that clients of crate `prio` can work generically with traits `Vdaf`, `Aggregator`, `Client` and `Collector` to encode and decode messages. We also require `Clone + Debug` on a number of associated types to save clients the trouble of specifying lots of trait bounds when working with trait `Vdaf`.

tgeoghegan force-pushed the timg/trait-codec branch from 4bee7ee to c3efed2 Compare February 2, 2022 18:56

cjpatton reviewed Feb 4, 2022

View reviewed changes

src/codec.rs Outdated Show resolved Hide resolved

src/vdaf.rs Outdated Show resolved Hide resolved

src/vdaf.rs Outdated Show resolved Hide resolved

src/vdaf.rs Outdated Show resolved Hide resolved

src/vdaf.rs Outdated Show resolved Hide resolved

cjpatton mentioned this pull request Feb 7, 2022

WIP vdaf: Handle messages as opaque strings #162

Closed

tgeoghegan force-pushed the timg/trait-codec branch from c3efed2 to b252826 Compare February 11, 2022 20:50

tgeoghegan marked this pull request as ready for review February 11, 2022 21:15

tgeoghegan requested a review from a team February 11, 2022 21:15

branlwyd requested changes Feb 11, 2022

View reviewed changes

cjpatton requested changes Feb 12, 2022

View reviewed changes

tgeoghegan commented Feb 12, 2022

View reviewed changes

tgeoghegan force-pushed the timg/trait-codec branch from b252826 to 52cb957 Compare February 16, 2022 02:09

tgeoghegan requested review from branlwyd and cjpatton February 16, 2022 02:09

cjpatton requested changes Feb 16, 2022

View reviewed changes

cjpatton self-requested a review February 17, 2022 00:06

cjpatton requested changes Feb 17, 2022

View reviewed changes

src/codec.rs Outdated Show resolved Hide resolved

branlwyd approved these changes Feb 17, 2022

View reviewed changes

src/codec.rs Outdated Show resolved Hide resolved

src/codec.rs Show resolved Hide resolved

tgeoghegan requested a review from cjpatton February 17, 2022 00:31

cjpatton approved these changes Feb 17, 2022

View reviewed changes

tgeoghegan added 5 commits February 17, 2022 17:27

tgeoghegan force-pushed the timg/trait-codec branch from 9e5b2ef to 8e966fc Compare February 18, 2022 01:32

tgeoghegan merged commit 1dccadc into main Feb 18, 2022

tgeoghegan deleted the timg/trait-codec branch October 12, 2022 19:24

tgeoghegan mentioned this pull request Dec 4, 2023

Change Encode/Decode to use Read/Write instead of Cursor/Vec #860

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `codec` module #166

Introduce `codec` module #166

tgeoghegan commented Feb 2, 2022

tgeoghegan commented Feb 2, 2022

cjpatton left a comment

tgeoghegan commented Feb 5, 2022

tgeoghegan commented Feb 5, 2022 •

edited

Loading

tgeoghegan commented Feb 11, 2022

tgeoghegan commented Feb 11, 2022 •

edited

Loading

branlwyd Feb 11, 2022

cjpatton Feb 12, 2022

tgeoghegan Feb 15, 2022

cjpatton Feb 16, 2022

branlwyd Feb 11, 2022

tgeoghegan Feb 12, 2022

tgeoghegan Feb 15, 2022

branlwyd Feb 11, 2022

tgeoghegan Feb 12, 2022

tgeoghegan Feb 15, 2022

branlwyd Feb 11, 2022

tgeoghegan Feb 12, 2022

tgeoghegan Feb 15, 2022

branlwyd Feb 11, 2022

tgeoghegan Feb 12, 2022

tgeoghegan Feb 15, 2022

cjpatton left a comment

tgeoghegan left a comment

tgeoghegan Feb 12, 2022

tgeoghegan Feb 12, 2022

tgeoghegan Feb 12, 2022

cjpatton left a comment

cjpatton Feb 16, 2022

branlwyd left a comment

cjpatton left a comment

tgeoghegan commented Feb 18, 2022 •

edited

Loading

	/// An input share for the heavy hitters VDAF.
	/// An input share for the poplar1 VDAF.

Introduce codec module #166

Introduce codec module #166

Conversation

tgeoghegan commented Feb 2, 2022

tgeoghegan commented Feb 2, 2022

cjpatton left a comment

Choose a reason for hiding this comment

tgeoghegan commented Feb 5, 2022

tgeoghegan commented Feb 5, 2022 • edited Loading

tgeoghegan commented Feb 11, 2022

tgeoghegan commented Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

tgeoghegan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branlwyd left a comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

tgeoghegan commented Feb 18, 2022 • edited Loading

Introduce `codec` module #166

Introduce `codec` module #166

tgeoghegan commented Feb 5, 2022 •

edited

Loading

tgeoghegan commented Feb 11, 2022 •

edited

Loading

tgeoghegan commented Feb 18, 2022 •

edited

Loading