-
Notifications
You must be signed in to change notification settings - Fork 281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic TLS Encrypted ClientHello (ECH) support #1044
base: master
Are you sure you want to change the base?
Basic TLS Encrypted ClientHello (ECH) support #1044
Conversation
591113a
to
18baf53
Compare
18baf53
to
9a1a3e2
Compare
This introduces a new Exception so that clients can respond only to the ECH retry request without having to parse SSLHandshakeExceptions in general. This exception should probably be implemented in boringssl or native_crypto.cc.
9a1a3e2
to
3ae5bd7
Compare
God speed, I'd love to see this land. |
Thanks for this! And my apologies for taking so long to get to it! I'm not an ECH expert, but I'm trying to get up to speed plus I'll liaise with the BoringSSL team. From my first reading, the code looks nice but I have a few minor concerns:-
With that in mind, I think it makes sense to encapsulate the ECH parameters into some wrapper class ( I guess we probably need to keep exposing More later! |
We're still very early in ECH's protocol development. Conscrypt ends up in places with restrictly update and stability stories (Android system and Android platform APIs). Every draft protocol is doomed to die, and so uses of ECH, for now, must be limited to deployments that can handle that. Typically, we test features like this in places like Chromium, which are better equipped to handle an experimental protocol. So while I'd definitely encourage thinking about how ECH will look in Conscrypt, we're far too early to actually land anything. Even API-wise, everything around the ECH name override and the recovery flow is all still theoretical and not yet proven. We may well learn from the initial experiment that recovery needs to work very differently. |
There's a definite trade-off. I can see the value of getting something merged and out there so that people can use it and find what works and doesn't. And to some extent we can set expectations and shield people from unstable APIs e.g. mark everything There's clearly some demand to make this work with okhttp, so we need to figure out a way that lets us iterate without constantly breaking them. (@yschimke ?) |
Ah cool, yeah, if there's a way for Conscrypt to expose experimental stuff, that'd be handy! |
Big +1 to experimental. I'm keen to see this move forward, and happy to help in testing it. But OkHttp doesn't need any direct changes, so it's easy for me to push things onto Conscrypt. Ultimately I'm happy it's your call. The interest isn't from any obvious user demand, yet. But at this stage of the draft, I'm trying to support the efforts of @eighthave in proving the viability of the draft and the implementation. We can't magically support ECH in Android or OkHttp without an unbundled Conscrypt, but for the drafts that the industry is pushing forward I'm happy to try to get ahead and find issues that can be fixed, so Android doesn't lag behind other platforms in support and hold this back. If you look at the test I wrote using the patched Conscrypt, it just uses hooks to override Dns and a DelegatingSSLSocketFactory to make the write calls at the right points. If this takes off, and as OkHttp starts handling asynchronous DNS we can work out how to cleanly support this, and more things using SVCB and HTTPS records. |
It would be great to have some basic ECH support merged, even as an experimental API. One potential approach to keep this experimental API simple is to postpone the more elaborate things like name overrides and retry configs, and just fail in that case. That's a setup that I would use now, if it was available. Also, this initial experimental API could only use ECH Configs fed to it via a method call, and then the DNS stuff doesn't need to be included yet (DNS is not in this merge request, but it is in my fork along with more work on a full implementation). I do think it is important to support ECH GREASE as soon as possible. That is the easiest way to start testing ECH compatibility with networks and middleboxes.
|
My 2c - I'd see value in testing out those edge cases like name overrides also. It would be good while testing this against different servers, not to have to exclude certain cases. But defer to you folk with more skin in the game. But I agree about leaving out DNS, particularly as to do securely, we are presumably talking about DoH? which would be hard to do inside of Conscrypt. |
Yes of course, I'm just proposing a priority. Merging something very simple would be better than delaying merging any ECH support.
Encrypted DNS for sure. In Android, there is an API to check whether the system resolver is setup to do encryption, so it'll be possible to use that. |
If I understand what's in the PR so far (big if :) then it seems to me like Regardless of how ECH evolves I suspect there's always going to be an For the return values I'm much less clear about how the protocol works (yet) but again maybe worth a higher level abstraction like an I know it creates a maze of twisty little interface classes but it does shield apps from some of the API and protocol details. |
Also I don't want to just create a lot of busywork for @eighthave ... I'm happy to share some of the encapsulation work. |
My impression from the work so far is that apps won't really need to know about the details, but will have to sometimes shuffle the ECH bytes to Conscrypt. Like with OkHTTP users wanting to do DNS themselves, or other cases where ECH bytes are coming from some other non-DNS channel.
I should mention that our current OTF-funded contract is over but we'll probably go for another round of work. That wouldn't start til May at the very earliest. So I don't really have cycles now to do implementation work, but will happily keep tabs on any work that's happening. |
If it follows the approach from the existing branch, I can put up a refine the existing sample for OkHttp based on this API and Android DNS. See what the interest is in community as well as flush out bugs. |
No, the recovery flow is not an optional part of ECH, even in draft form. This is part of the recovery mechanism to ensure server don't knock themselves offline by deploying ECH, are unable to roll things back in case of issues, or just break on draft version mismatch. It is experimental because we don't yet know how it will behave, but we know that something like it will be needed as critical deployability mechanism. Again, remember that every use of ECH in a draft form is doomed to die. The reason to ship a draft version is to help the protocol develop and validate the ideas we've put into the draft standard. Implementations that aren't doing that should wait until we have something stable and working. |
💯 |
I would of course be happy to see a complete implementation. I think we're all on the same page here. I'm not saying these things are optional, I'm saying there are useful things that can be tested even with an incomplete implementation. And they can be tested by more people and in parallel with other efforts. For example, my dev fork is more complete than this, but nonetheless incomplete. And we have some dev forks of apps that actually work with ECH and Cloudflare. Plus it was enable to explore ECH in OkHTTP. This has also allowed us to start in on interop testing with various implementations: https://defo.ie/ So if there are the resources to fully implement the current draft now, then no need for an incomplete, experimental API. If not, then that's when it would add value. |
On the off chance that any of you will be in Vienna for IETF, I'll be there and would be happy to meet up and discuss. @sftcd and I will be at the Hackathon starting tomorrow. |
Alas, I'm doing IETF remotely this time around. :-( |
At long last, DEfO rides again! We recently started work again based on 2+ years of funding from Open Tech Fund: I plan on diving back in the new year and working to finish ECH support into Conscrypt. For anyone looking for an easy ECH dev setup, we have a HOWTO for quick starts with a new domain name and a VPS: |
This is the first stage of implementing Encrypted ClientHello (ECH) in Conscrypt #730. It provides the APIs required for clients to make TLS connections using ECH. This implements enough of the server-side to provide ECH in the test suite using ECH Key and Configs generated by boringssl. This should be enough to let libs like OkHTTP fully implement ECH square/okhttp#6539
It does not handle fetching the required key material from DNS, but I also have that implemented locally. Since Conscrypt has not yet had to do DNS for itself, this will be breaking new ground.