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

Encrypted ClientHello (ECH) #730

Open
eighthave opened this issue Oct 1, 2019 · 21 comments
Open

Encrypted ClientHello (ECH) #730

eighthave opened this issue Oct 1, 2019 · 21 comments

Comments

@eighthave
Copy link

It would be great to have support for TLS Encrypted SNI (ESNI) in Conscrypt, so Android apps can include Conscrypt to get TLSv1.3 and ESNI support. Work is already underway in boringssl and openssl to support ESNI, so it should be relatively straightforward

I'm working with others on implementing Encrypted SNI on services used on Android. ESNI is up to draft4 in the IETF process, so now is a good time to start implementing in order to provide feedback to the IETF process. We're wondering how much interest there is here in TLSv1.3 Encrypt SNI extension that is currently an IETF draft implemented by Firefox, Cloudflare, and others. We could potentially submit code to make Conscrypt support ESNI. We are currently working on getting ESNI implemented in openssl, curl, and lighttpd.

@kruton
Copy link
Contributor

kruton commented Oct 1, 2019

This might be able to be done transparently for clients if hooks are defined for each platform. OpenJDK has the DNS/JNDI and it appears you can query with a target record type of "*" if needed. Android would use its netd resolver most likely.

@eighthave
Copy link
Author

eighthave commented Oct 2, 2019

Yeah, making it transparent to clients by default sounds ideal, though I suspect there might be good reason expose somethings. DNS is not a requirement for ESNI, just the suggested way to distribute key material. For example, there are some ESNI modes where the keys do not come from DNS, but "somewhere else". For that to be useful, the client needs to be able to provide that key material in Java.

@kruton
Copy link
Contributor

kruton commented Oct 24, 2019

Some notes for possible future implementors:

  1. For DNS there are several APIs that allow the SSLSocket or SSLEngine to be created. Some are supplied with resolved addresses and some are not. It is critical that during the DNS fetching of ESNI that the ESNI records and A/AAAA records stay in sync (i.e., whatever chooses the IPv4 or IPv6 for multi-record entries also chooses the right ESNI entry). This is dependent on whether HTTPSSVC/SVCB or CNAME style comes out in the end of the RFC process as the winner.

  2. DNS servers in home environments are generally horrible and this might affect performance (e.g., the records may not be able to be fetched at all).

Upstream BoringSSL bug

@davidben
Copy link
Contributor

Small correction to 1, it's not that ESNI and A/AAAA records have to stay in sync (separate record types aren't guaranteed to be in sync). It's that the IP addresses need to come from the ESNI flow, to handle the multi-CDN use case correctly. That means either the ESNI record (which is ESNI keys and addresses stapled in one record) or the HTTPSSVC/SVCB record (which is basically ESNI keys stapled to a funny CNAME), either of which override the usual A/AAAA lookup flow.

Re 2, realistically I expect ESNI will need to depend on DoH or DoT to avoid troubles with buggy router DNS resolvers.

@eighthave
Copy link
Author

So @uniqx and I have been putting together a test harness, and started digging into the DNS resolving question. It looks like the place we'll need to change is in the constructors of AbstractConscryptSocket that include a hostname, or a subclass of that. Specifically, the AbstractConscryptSocket constructors's call to super() end up instantiating an InetAddress instance, which would do the DNS resolution. The method that overrides Socket.connect(SocketAddress, int) would also need to do this.

But it looks like perhaps it should happen at a higher level, in the ConscryptEngineSocket constructors, where there is already SSLParametersImpl, which would then have an instance to manage the ESNI keys, like ESNIKeyManager. We are not entirely clear yet which level makes sense.

Then ESNIKeyManager could be exposed for the use cases that need to provide key material themselves, and skip the built-in DNS lookup.

@davidben
Copy link
Contributor

davidben commented Nov 6, 2019

You need to be very careful here because of the multi-CDN problem. The ESNIKeyManager idea probably won't fly. At least in the direction things are flying, lots of aspects of the DNS lookup are being correlated together. They're also very much in flux right now as we figure out how this should work. (TBH, I think it's trending too much in the complex direction, but it'll be hard to reel it back in. DNS loves deployment flexibility and pushing complexity off to the client.)

@eighthave
Copy link
Author

ESNIKeyManager is a bad name for what we are proposing here. It would be something like what @sftcd did in his openssl API, where there is a list of valid keys stored in SSLParametersImpl. So not a manager at all, but more like ESNIKey[] ValidESNIKeys. Or perhaps a class that provides some kind of query method to get the right key based on provided criteria.

@davidben
Copy link
Contributor

davidben commented Nov 6, 2019

Right, it's the query mechanism that is you need to be careful with.

@eighthave
Copy link
Author

Do you have any concrete examples we can work with? It would be good to have that in a test case as we work on this.

@Fezravien
Copy link

Excuse me.
I wants to know ESNI something.

Can you tell me how to manage the key to encrypt the sni?
Is it managed between web server and dns server?

If yes, what do you think about the key renewal?

@eighthave
Copy link
Author

Right now, we're starting with HTTPSSVC as the DNS method for getting the ESNI key material. @davidben @kruton do either of you have specific thoughts or requirements on which DNS records Conscrypt should support by default? Like whether Conscrypt should only do HTTPSSVC by default or also handle generic SVCB requests?

Also, I guess the idea is that Conscrypt would handle the DoH/DoT detection. Or will Android provide some method for that?

@eighthave
Copy link
Author

Android provides LinkProperties.isPrivateDnsActive() and LinkProperties.getPrivateDnsServerName(), for detecting DoH/DoT. OpenJDK does not seem to have anything similar. So we would need to abstract that piece in order to use this detection in ConscryptEngineSocket

@eighthave eighthave changed the title Encrypted SNI Encrypted ClientHello (ECH) May 4, 2021
@eighthave
Copy link
Author

FYI I've started work on this. I have the latest BoringSSL building with the latest Conscrypt. Now I'm wiring up the new BoringSSL methods to be available in Java.

@eighthave
Copy link
Author

I have the first test setup with JNDI working. JNDI provides querying by type number, e.g. "65", so I think that's the preferred approach. Querying using "*" won't work well because many setups do not include HTTPS and SVCB results in the "*" results, for example, crypto.cloudflare.com.

@eighthave
Copy link
Author

So I have ECH working in Conscrypt #1044. The big open question is how to handle the new DNS requirements in Conscrypt. In summary, the hard part about adding ECH support is that it requires more from DNS than what is currently available when connecting to a socket. These requirements are mapped out in HTTPS / SVCB. Currently, the hostname is provided, then InetAddress.getByName() returns an IP address, and that is all that is needed to negotiate a TLSv1.3 connection. With ECH, there must be a public key first so that the client can encrypt the ClientHello before sending it. That comes from either DNS or another out-of-band channel. On top of that, the public key must match the IP address so that multi-CDN setups still work, so the IP address and public key must be used together. If the A/AAAA lookup is done separately from the HTTPS/SVCB lookup, they might not match since the results could come from different caches, etc.

As far as I understand it, the InetSocketAddress instance is currently where DNS is most likely happening, but I don't have a deep understanding of that part. Resolving the IP address along with the HTTPS lookup and providing it to SSLSocket/InetSocketAddress might work if the hostname verification is not happening in boringssl. If the hostname is given, then InetSocketAddress's lookup might give an IP address that does not match the public key used to encrypt the ClientHello. Then the server would not be able to decrypt the outer ClientHello. SSLSocket and Socket seem to handle DNS in the same way, but that won't always be possible when using ECH or HTTPS/SVCB. So perhaps there needs to be EchSSLSocket to ensure that the hostname is never passed to the super classes or InetSocketAddress instances when HTTPS/SVCB/ECH are in use.

Another thing that needs working out here is when Conscrypt should do the DNS, and when it should not. A pure library that provides ECH should not handle the DNS, e.g. openssl. A pure client must handle the DNS, e.g a browser. Conscrypt can sometimes act like the client, like in the case when an Android app is just calling URL.openConnection(), so it seems like Conscrypt should handle the DNS there. But when OkHTTP makes calls, it treats Conscrypt like a library, and Conscrypt should let OkHTTP handle the DNS.

@eighthave
Copy link
Author

We've started publishing builds of our Conscrypt fork to make testing easy:

implementation 'info.guardianproject.conscrypt:conscrypt-android:2.6.alpha1638179154.job1828169525'

The source matching those tags is here:
https://github.com/guardianproject/conscrypt

@yschimke
Copy link
Contributor

yschimke commented Feb 6, 2022

Quick update. I think we have a POC for OkHttp that shows we can connect a custom Dns implementation that also fetches HTTPS records instead of or in addition to A/AAAA records. Then a custom SSLSocketFactory that configures using the changes by @eighthave square/okhttp#6539

However if it isn't likely to land in Conscrypt, then it doesn't make much sense to continue efforts on an OkHttp supporting library (not likely part of the core).

So mainly curious if the existing PR #1044 has legs?

@eighthave
Copy link
Author

eighthave commented Mar 8, 2022 via email

@ag2s20150909
Copy link

@yoshimo
Copy link

yoshimo commented Dec 31, 2024

Android provides LinkProperties.isPrivateDnsActive() and LinkProperties.getPrivateDnsServerName(), for detecting DoH/DoT.

You shouldn't limit the use of ECH when there is no DOT/DOH Resolver in use.
If my networks resolver is run by a trusted entity like myself using ECH to protect the metadata outside my network is still something i would like to happen. This check is not necessary.

@yschimke
Copy link
Contributor

The standard DNS implementation we use wouldn't be sufficient, so I'm not clear how this can work.

You might want broader support, but okhttp is not the place to request.

We can only enable this once the platform or conscrypt supports AND exposes it.

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

No branches or pull requests

8 participants
@yoshimo @kruton @davidben @eighthave @yschimke @ag2s20150909 @Fezravien and others