Implement ECH #427
Implement ECH #427
Conversation
8e229d0 to
ba04d58
Compare
|
Hello, |
|
Hello, I had time to review the PR, and I have two concerns regarding the implementation. First, ECH is enabled for all connections, at least during the DNS resolution. ECH only really make senses between wstunnel client and the server. All others connections should not pay the overhead of trying to handle ECH. Second, there should be an option to disable/enable ech. The main reason is that most firewall reject TLS connection that has ECH enabled because they can't inspect the SNI and do their job. The goal of wstunnel is to bypass firewall, so if the connection is rejected due to ECH being enabled it is bad ;x Hope it helps, I am ok to introduce ECH as long as it is an option to toggle and it does not impact the performance of non ECH connections. |
827d6c8 to
be82d10
Compare
|
Sorry for the delay, I updated the PR with your remarks, and added a flag
The newer ECH implementation consists of using an "outer" and "inner" SNI, so the client hello still uses an SNI but masks the real SNI that we want to reach using a generic one like There's still the problem of the freebsd's and Android armhf builds. Cheers |
src/protocols/tls/server.rs
Outdated
| @@ -197,7 +199,37 @@ pub async fn connect(client_cfg: &WsClientConfig, tcp_stream: TcpStream) -> anyh | |||
| ); | |||
| } | |||
|
|
|||
| let tls_stream = tls_connector.connect(sni, tcp_stream).await.with_context(|| { | |||
| let mut connector = tls_connector.clone(); | |||
There was a problem hiding this comment.
If you can make a dedicated function to handle ech case.
Ideally, the connector created should be cached to avoid re-creating it each time at new connection. (i.e: you lose the TLS context, resume tickets, ...)
But the ech config can expire/be invalid if the DNS entry change, so there is no way to tell when or if this happens.
Let's say for now that's ok to re-do it each time when ech is enabled.
Only extract this code into a dedicated function
let tls_stream = if ech_config.is_some() && tls_ech_enabled {
tls_connect_with_ech().await.xxxx
} else {
tls_connector.connect(sni, tcp_stream).await.xxx
}|
Hello, Thanks for letting me know about the new design for ECH, wasn't aware of it. For the other archs, I think you can make it works, if you construct the builder differently, instead of using should let it default to the correct backend, when aws_lc_rs is not used, the backend is provided by |
9deebfb to
cf206a9
Compare
|
Ok it seems that rustls support hke only for aws-lc crypto provider. Going to merge the PR, and make a feature flag to aws-lc crypto provider |
|
Thank you for the PR, I am going to make a release when I have a bit of time to introduce the feature flag. I have made some cleanup 99cde68 |
|
It is released in v10.4.0 P.s: For now the ech config does not reload if the dns entry change. If it is a real requirement, let me know. |
This works, but i'm not really fond of how i've implemented it, I would prefer not to use tuples to propagate the
EchConfig.Unfortunately considering the dns resolution is done during the
connectand the ech configuration must be done during tls configuration I did not see an other way to propagate the config without reworking the whole connect stack.I'd love to have your feedback on this and hope this'll be merged !