Handle ECH Retry

[yschimke]

A few assorted changes

• grab the ech retry config and use it to retry when we get a mismatch
• avoid ech retry more than once
• avoid downgrading from ech to non ech

[yschimke]

@swankjesse I don't think Conscrypt ECH PR supported this

https://github.com/google/conscrypt/pull/1406/changes#diff-808df7e6867f40b6d68b137bb7e1be92b26fb0b99f20df4ed016e7f4015a42e3

[yschimke]

Quick search for evidence

https://chromium.googlesource.com/chromium/src/+/refs/tags/143.0.7499.47/net/url_request/http_with_dns_over_https_unittest.cc#449

https://github.com/mozilla/gecko-dev/blob/master/security/nss/gtests/ssl_gtest/tls_ech_unittest.cc#L2137-L2160

https://go.dev/src/crypto/tls/handshake_client_test.go#L2808

https://github.com/google/boringssl/blob/22a0079b189c391b95689813a41982ce11876f0a/ssl/test/runner/ech_tests.go#L2192-L2265

[yschimke]

I'll ask at work tomorrow.

[swankjesse]

I’d like to be particularly careful with landing this as it introduces new attack vectors and is not yet possible to test with MockWebServer + JVM Conscrypt.

I think it’s definitely good to have special recovery code for ECH handshake failures. But my first instinct is that special recovery code should mostly be about reducing the opportunity for downgrade attacks.

This is attempting to do something else, and I would like to understand the problem it solves before we land this.

[bjiang7]

Just chiming in here to state that the ECH recovery flow is a pretty important part of the spec, and so should be implemented if OkHttp is to say it supports ECH.

The reason why is because there's no other way to recover from a mismatch in ECH config between the client and server (i.e. synchronization issues). It's a performance hit to have to fall into this codepath (and shouldn't be happening in most cases), but it is needed so the consequences of things being out of sync is latency rather than outages.

I also asked BoringSSL a long time ago about this when doing the Android implementation, because I had the same question of how important the recovery flow was. Their stance was "This behavior is pretty crucial to allow servers to safely deploy ECH because DNS records can be stale and we need to make it possible for servers to recover from mismatches. The existence of any client that gets this wrong means a server now cannot deploy ECH in a rollback-safe way."

[swankjesse]

Making it safer for server operators to confidently deploy ECH is good.

[swankjesse]

This is excellent. Some feedback and I’d like another look before merge. I promise to not make you wait so long on followups

[swankjesse]

I’m walking through this, and trying to cover all our bases.

This sets up a mechanism to attempt the next connection spec in sequence, and it’s replaced with code to attempt the next ECH config.

If the ECH config fails, do we attempt the next connection spec? I suppose we don’t per the ECH doc, which is also weird!

If the server rejects ECH, the client proceeds with the handshake, authenticating for ECHConfig.contents.public_name as described in Section 6.1.7. If authentication or the handshake fails, the client MUST return a failure to the calling application.

(We shouldn’t fall back using our normal fallback mechanism, and that’s difficult to test)

[yschimke]

I think we should clarify, one badly configured server/region is more likely than poorly attempted hacking, so I don't like reducing one of the "free" wins of distributed computing, retries.

But the change was really intended to just be to include the possible mismatch failure in the next choice, so it can't happen before connectTls.

[yschimke]

Does this cover the fallback concern for you?

    // If this was already in response to an ech retry, we are done for this
    // connection
    if (echRetryConfig != null || !retryTlsHandshake(sslException)) return null

[swankjesse]

Yeah, I am mostly eager to write a bunch of test cases for this because the logic is too difficult for me to follow statically.

[swankjesse]

Note to self... this behavior isn’t removed, we just compute a null retryTlsConnection elsewhere

[swankjesse]

This condition surprises me. The logic looks incorrect for the case where the server securely disables ECH. In particular I would expect the offeredEchRetryConfig to be null in the case where the server rejects ECH (distinct from offeredEchRetryConfig.configList == null)

[yschimke]

From the validation comments here #9611 (comment)

Which one do you think is which?

[swankjesse]

I think it might be a problem with documentation and naming, and not a problem with logic.

If the server doesn’t include an ECH extension, there’s no ‘offered ECH retry config’. In particular there shouldn’t be anything offered there, perhaps derived.

But we do need an object to extract the public name from the EchConfigList object we passed in.

[swankjesse]

(The class EchRetryConfig is documented as sent by a server but I don’t think the publicName can be sent by a server; that needs to come from our passed-in EchConfigList)

[swankjesse]

This doc says ‘Sent by a server’ but we need a mechanism to lookup the public name when the server replies without this extension.

Does this class represent the encrypted_client_hello extension? Or is it a higher-level object that describes the TLS client’s state?

(I think it’s probably something describing the client state, because I don’t think the publicHostname is included in the encrypted_client_hello extension)

[yschimke]

https://cs.android.com/android/platform/superproject/+/android-latest-release:external/boringssl/src/include/openssl/ssl.h;l=4352?q=SSL_get0_ech_name_override&ss=android%2Fplatform%2Fsuperproject:external%2F&start=11

// SSL_get0_ech_name_override, if |ssl| is a client and the server rejected ECH,
// sets |*out_name| and |*out_name_len| to point to a buffer containing the ECH
// public name. Otherwise, the buffer will be empty.
//
// When offering ECH as a client, this function should be called during the
// certificate verification callback (see |SSL_CTX_set_custom_verify|). If
// |*out_name_len| is non-zero, the caller should verify the certificate against
// the result, interpreted as a DNS name, rather than the true server name. In
// this case, the handshake will never succeed and is only used to authenticate
// retry configs. See also |SSL_get0_ech_retry_configs|.
OPENSSL_EXPORT void SSL_get0_ech_name_override(const SSL *ssl,
                                               const char **out_name,
                                               size_t *out_name_len);

  std::string_view ech_name_override = GetECHNameOverride();
  if (!ech_name_override.empty()) {
    // If ECH was offered but not negotiated, BoringSSL will ask to verify a
    // different name than the origin. If verification succeeds, we continue the
    // handshake, but BoringSSL will not report success from SSL_do_handshake().
    // If all else succeeds, BoringSSL will report |SSL_R_ECH_REJECTED|, mapped
    // to |ERR_R_ECH_NOT_NEGOTIATED|. |ech_name_override| is only used to
    // authenticate GetECHRetryConfigs().
    DCHECK(!ssl_config_.ech_config_list.empty());
    used_ech_name_override_ = true;