Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eighthave
Copy link

@eighthave eighthave commented Nov 11, 2021

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.

@eighthave eighthave force-pushed the basic-encrypted-clienthello-support branch from 591113a to 18baf53 Compare November 11, 2021 15:53
@eighthave eighthave changed the title WIP: Basic TLS Encrypted ClientHello (ECH) support Basic TLS Encrypted ClientHello (ECH) support Nov 11, 2021
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.
@eighthave eighthave force-pushed the basic-encrypted-clienthello-support branch from 9a1a3e2 to 3ae5bd7 Compare November 17, 2021 16:07
@yschimke
Copy link
Contributor

God speed, I'd love to see this land.

@prbprbprb
Copy link
Collaborator

prbprbprb commented Mar 9, 2022

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:-

  • We need to be able to insulate application code from any changes to the BoringSSL APIs (which this PR mostly does)
  • Once we commit any public API it's always going to be problematic to change it (even more so once we expose it to Android apps)
  • Ultimately, ideally we would want to be compatible with whatever the SunJSSE provider does, but currently this doesn't even appear to be on their JEP roadmap

With that in mind, I think it makes sense to encapsulate the ECH parameters into some wrapper class (EchParameters maybe?) and store that in the SSLParametersImpl. That slightly reduces the number of new APIs we have to add to sockets and engines as well, plus we can (later) add helper functionality for creating configs rather than relying on application code to do the serialisation.

I guess we probably need to keep exposing getEchNameOverride() and echAccepted().

More later!

@davidben
Copy link
Contributor

davidben commented Mar 9, 2022

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.

@prbprbprb
Copy link
Collaborator

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 @ExperimentalApi and expose nothing via the Android platform public APIs until it's stable.

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 ?)

@davidben
Copy link
Contributor

Ah cool, yeah, if there's a way for Conscrypt to expose experimental stuff, that'd be handy!

@yschimke
Copy link
Contributor

yschimke commented Mar 10, 2022

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.

@eighthave
Copy link
Author

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.

With that in mind, I think it makes sense to encapsulate the ECH parameters into some wrapper class (EchParameters maybe?) and store that in the SSLParametersImpl.

EchParameters sounds workable to me as an API, but maybe named EchConfigParameters or EchConfigs since the drafts call these "ECH Config" or "ECH Config Lists".

@yschimke
Copy link
Contributor

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.

@eighthave
Copy link
Author

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.

Yes of course, I'm just proposing a priority. Merging something very simple would be better than delaying merging any ECH support.

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.

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.

@prbprbprb
Copy link
Collaborator

EchParameters sounds workable to me as an API, but maybe named EchConfigParameters or EchConfigs since the drafts call these "ECH Config" or "ECH Config Lists".

If I understand what's in the PR so far (big if :) then it seems to me like EchParameters (or EchConfigParameters, I'm easy) would contain a List<EchConfig> and a boolean for useGrease.

Regardless of how ECH evolves I suspect there's always going to be an EchConfig abstraction which could start off as we have now (just a byte[] that comes from elsewhere) and evolve to contain better constructors, DNS logic etc.

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 EchStatus which can return the connection status, name overides and maybe a List<EchRetryConfig>.

I know it creates a maze of twisty little interface classes but it does shield apps from some of the API and protocol details.

@prbprbprb
Copy link
Collaborator

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.

@eighthave
Copy link
Author

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.

Also I don't want to just create a lot of busywork for @eighthave ... I'm happy to share some of the encapsulation work.

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.

@yschimke
Copy link
Contributor

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.

@davidben
Copy link
Contributor

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.

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.

@yschimke
Copy link
Contributor

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.

💯

@eighthave
Copy link
Author

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.

@eighthave
Copy link
Author

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.

@davidben
Copy link
Contributor

Alas, I'm doing IETF remotely this time around. :-(

@eighthave
Copy link
Author

At long last, DEfO rides again! We recently started work again based on 2+ years of funding from Open Tech Fund:
https://guardianproject.info/2023/11/09/defo-developing-ech-for-openssl-round-two/

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:
https://guardianproject.info/2023/11/10/quick-set-up-guide-for-encrypted-client-hello-ech/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants