Panicky, non-validating conversions involving protobuf types and/or domain types

[mzabaluev]

After #1022 has landed, there is a number of conversions between protobuf types and so-called domain types that do not validate their input, are prone to panic when the input does not conform to expectations on the destination value, or produce a protobuf message struct value that falls afoul of the specification on well-known protobuf types.

Here's why this is problematic for protobuf ⭢ domain type:
As the most likely origin of protobuf DTO values is by having been parsed from an incoming message by prost without any validation as to its contents, this is an easy DoS bomb under application developers who would make use of these conversions. Please consider using fallible conversions when dealing with untrusted values originating from the network.

Here's why this is problematic for domain type ⭢ protobuf:
Aside from the low-level tendermint-proto crate, tendermint-rs should never produce protocol messages that do not comply with the specification. The alternative of panicking makes for poor API usability. The tendermint API should provide true domain types where the From-type conversion to their protobuf DTOs is a total function always resulting in valid messages.

List of problematic conversions

The list of identified conversions that exhibit the problems described above, along with the PR addressing them:

• From<chrono::DateTime<chrono::Utc>> for pb::Timestamp (removed in Replace chrono with time 0.3 #1030, replaced by Time conversions)
• From<pb::Timestamp> for chrono::DateTime<chrono::Utc> (removed in Replace chrono with time 0.3 #1030, replaced by Time conversions)
• From<chrono::Duration> for pb::Duration (removed in Replace chrono with time 0.3 #1030, replaced by TBA)
• From<pb::Duration> for chrono::Duration (removed in Replace chrono with time 0.3 #1030, replaced by TBA)
• TryFrom<pb::Timestamp> for Time (fixed in Replace chrono with time 0.3 #1030)
• Time::from_unix_timestamp (fixed in Replace chrono with time 0.3 #1030)

Homespun domain types

Domain types to be provided in tendermint for lack of a suitable alternative:

• Time, to be domain-hardened in Replace chrono with time 0.3 #1030
• Duration.

Originally posted by @mzabaluev in #1030 (comment)

[tony-iqlusion]

FWIW, in cosmrs I used a trait with an associated type to enforce conversion bounds between "domain types" and their associated prost-generated protobufs:

https://docs.rs/cosmrs/0.3.0/cosmrs/tx/trait.Msg.html

pub trait Msg: Clone + Sized + TryFrom<Self::Proto, Error = ErrorReport> + Into<Self::Proto> {
    type Proto: MsgProto;
    [...]
}

This is somewhat specialized to the tx.Msg use case, but with the elided portion aside it works for any "domain type" with an associated protobuf type.

The bounds ensure that there's always a TryFrom conversion available from the associated Protobuf type, as well as an Into conversion to convert to the domain type.

I think it'd be great if there were a trait in tendermint-rs that wrapped up this pattern which we could build on in cosmrs.

[thanethomson]

Would the approach outlined in #661 be of use here?

[mzabaluev]

Would the approach outlined in #661 be of use here?

Yes, essentially this issue is tracking transgressions against that approach.