Add an option to retrieve TLS errors

[matszczygiel]

Hello,

I'm using quinn create together with TLS Encrypted Client Hello (ECH) extension.
However, I encountered a problem where the client uses an invalid server ECH config. There is a mechanism described in the ECH RFC that the server can respond with the correct config, and the client can reconnect using it instead.

And theoretically, the rustls crate offers a way to retrieve the server ECH config from the TLS error, as discussed in rustls/rustls#2572.

Sadly, when using the quinn crate, there is no way to retrieve the underlying TLS error and extract the ECH config.

This PR aims to provide a way to get the underlying rustls error and effectively allow for ECH client retry.

[djc]

This seems mostly reasonable.

[matszczygiel]

Oh, I didn't notice that rustls is an optional dependency. Then perhaps we should store it under #[cfg(any(rustls-ring, rustls-aws-lc-rs, rustls-aws-lc-rs-fips))]?
Alternatively, we can use io::Error as an error representation, as downstream apps can always cast it to a concrete error type.

I'm not sure which option would be better in this case.

[djc]

Oh, I didn't notice that rustls is an optional dependency. Then perhaps we should store it under #[cfg(any(rustls-ring, rustls-aws-lc-rs, rustls-aws-lc-rs-fips))]? Alternatively, we can use io::Error as an error representation, as downstream apps can always cast it to a concrete error type.

Definitely don't think cfg guards are the right solution here, since it becomes confusing for non-rustls crypto::Session implementers.

[matszczygiel]

Oh, I didn't notice that rustls is an optional dependency. Then perhaps we should store it under #[cfg(any(rustls-ring, rustls-aws-lc-rs, rustls-aws-lc-rs-fips))]? Alternatively, we can use io::Error as an error representation, as downstream apps can always cast it to a concrete error type.

Definitely don't think cfg guards are the right solution here, since it becomes confusing for non-rustls crypto::Session implementers.

Yeah, I was thinking of using Option<Arc<io::Error>> because of the Clone requirement, which io::Error does not provide, but it feels a bit off. Maybe Option<Box<dyn (Any + Clone)>> would work in this case?

[matszczygiel]

I've ended up using Option<Arc<dyn std::error::Error + Send + Sync>> to not tie it to any particular error type. And still provide the downcasting method https://doc.rust-lang.org/stable/std/error/trait.Error.html#method.downcast_ref

[Ralith]

Maybe we should ditch this impl outright? Users should probably only be performing comparisons on code anyway.

[matszczygiel]

That would mean dropping PartialEq, Eq from the ConnectionError here https://github.com/quinn-rs/quinn/blob/main/quinn-proto/src/connection/mod.rs#L3917, or converting it to an ugly manual impl. Perhaps we should keep the comparison here, but only for the code field?

[djc]

This comment seems to be referencing Eq but not PartialEq. I think it'd be okay to drop Eq from ConnectionError, but we should probably try to preserve PartialEq.

[matszczygiel]

Oh, I assumed differently, as in that case, there's little difference between Eq and PartialEq. @Ralith can you clarify what you meant?

Users should probably only be performing comparisons on code anyway.

Regarding this, I rewrote the PartialEq comparison to include only code. Is that OK?

[Ralith]

Should we name/document this in a more generic way? While QUIC requires TLS, Quinn technically doesn't, and we have some non-TLS downstream users. We refer to the "crypto" layer elsewhere...

[matszczygiel]

I've renamed the field to crypto and updated the doc comment.

[matszczygiel]

I've updated the PartialEq manual implementation so that it compares only the code field. I hope this covers all the use cases.