tls_codec: feature for conditional deserialization derivation

[kkohbrok]

This PR proposes a feature that enables the verifiable attribute macro.

The macro extends the annotated struct with a boolean const generic indicating if the given instance of the struct was verified or not. The macro takes as input either Bytes or Reader and derives the respective flavour of TlsDeserialize for the unverified variant of the struct. The macro also creates two type aliases. Each type alias points to the verified or unverified struct variant and prefixes the struct name with "Verified" or "Unverified" respectively.

This allows a pattern, where a struct is meant to pass a processing step (e.g. verification) after verification. Any functionality that is conditional on that processing step can then be implemented for the verified version of the struct.

TODO:

• Module level documentation
• Create separate PR to bump syn add "full" feature to syn dependency
• rename to make a little more generic

[franziskuskiefer]

Thanks, some thoughts.

• having plain booleans is always a little difficult because you don't know what the value stands for
• having to add the const generic feels a little odd when we have an attribute on it anyway
• what is an unverified/verified struct supposed to (not) offer?

I think we can make this a little nicer.
How about having an attribute for #[verifiable], which adds a const generic to the struct and adds aliases as well as the implementation? That way we could hide all the details.
It could also be generalised a little and use a u8 to allow other states. That might be something we want to have, but not sure if it fits in here.

[franziskuskiefer]

Suggested change

	//!
	//!

[kkohbrok]

Thanks for the review! All very good points.

* having plain booleans is always a little difficult because you don't know what the value stands for

Agreed, but since we can't use enums just yet, I thought it would be a neat option. We can use constants for true and false, but I agree that it's not ideal.

* having to add the const generic feels a little odd when we have an attribute on it anyway

* what is an unverified/verified struct supposed to (not) offer?

We don't necessarily have to have an attribute on it if we can find a way for the deserialize macro to detect the "Verifiable" pattern, although this bears the risk of false positives, of course. We could try to implement it all in one attribute, but we have to make sure that we implementer stays somewhat flexible. The design goals for this whole thing would probably look something like this:

• there should be a single struct that defines the state variables of the verifiable struct
• the verified variant of a verifiable struct shouldn't be deserializable
• there should be distinct types for verified vs. unverified s.t. functions can restrict their input to the verified variant where necessary
• the implementer should be able to create distinct implementations for both variants. For example, depending on the struct the unverified variant has to expose some information to allow for verification (e.g. expose an index to allow the consumer to find the appropriate public key) but hide others to enforce that verification takes place before further processing

I think we can make this a little nicer. How about having an attribute for #[verifiable], which adds a const generic to the struct and adds aliases as well as the implementation? That way we could hide all the details. It could also be generalised a little and use a u8 to allow other states. That might be something we want to have, but not sure if it fits in here.

I like these ideas, although I'll have to step up my macro-foo somewhat. I'm not sure how we would want to generalize this to other states, since the requirements listed above are relatively specific (especially the restriction on deserialization). Is there something you had in mind? In any case, if we don't implement this in the derive macro and if we find that generalization is not something we want, we might want to just implement this on the OpenMLS side for now.

[OtaK]

My 2 cents: I think this feature, while still nice for some uses (i.e. OpenMLS API), should live in another crate that depends on & extends tls_codec.

This seems way out of scope of the crate (which is to allow de/serializing from/to TPL).

[franziskuskiefer]

This seems way out of scope of the crate (which is to allow de/serializing from/to TPL).

I disagree. Safe deserialisation is very much in scope of this crate. I talked through the PR with @kkohbrok and I think it's a useful addition because it allows users to differentiate between incoming and outgoing structs/messages. This facilitates safe implementations.
Also, it will be behind a feature flag and nothing anyone has to use.

[kkohbrok]

I've made the changes discussed OOB. I think we also discussed renaming the functions of the DeserializeBytes variant. Once this is done (I'll create a PR for this shortly), we can probably get rid of the input for the attribute macro and just derive both the reader and the bytes based functions for the deserializable variant.

[franziskuskiefer]

Generally looks good to me. The doc tests fail though.
Looks like this isn't on CI right now. Can you add this at the bottom to the tls_codec.yml action?

- run: cargo hack test -p tls_codec_derive --feature-powerset --doc

[franziskuskiefer]

Do we always need full? If not, let's only enable it for the conditional_deserialization feature.

[franziskuskiefer]

Looks like there's something missing here?

[franziskuskiefer]

Thanks, lgtm now.