Implement the SSLClientSocket portions of the ECH recovery flow

When ECH is rejected, the server handshakes with ClientHelloOuter, which
will result in it sending a certificate for the public name. ECH uses
this to recover from mismatches between the DNS and server. If the
certificate is good for the public name, we can extract an authenticated
ECH config (or lack thereof) from the connection and retry.

This CL implements the SSLClientSocket APIs needed to support this
fallback flow. The actual retry will need to be implemented a layer
above, in SSLConnectJob, once the fields from the DNS bits are ready.

Some notes:

- I've opted to make certificate errors in the fallback flow
  unbypassable by mapping them to some generic fatal error. This matches
  our behavior with HTTPS proxies and DoH. Otherwise we need to work out
  plumbing and UX to ask the user "as part of connecting to example.com,
  we need to verify a cert for this random unrelated name... is this
  certificate okay for that?". That seems absurd.

- There is a bit of a mess between DNS names vs. IP addresses, layering
  between BoringSSL and Chromium, and the very complex WHATWG URL
  syntax. Due to how our CertVerifier interfaces work, we need to worry
  about misinterpreting a DNS-name-typed string as an IP address.

  This mess is much simplified after
  https://groups.google.com/a/chromium.org/g/blink-dev/c/7QN5nxjwIfM/m/EBWRCH71AQAJ
  but, out of caution, I've added a redundant IP address check, using
  our actual URL parser. (Although, to be honest, the consequences of
  misinterpreting this aren't that bad.)

- I've made the error codes say just "ECH" without "SSL" because the
  QUIC half will need to participate in the same logic. ("SSL" is
  about as inaccurate for TLS/QUIC as it is for TLS/TCP these days, but
  we currently refer to TLS/TCP as "SSL" in code a lot, while we don't
  do so for TLS/QUIC.)

- I've given the Chromium error codes slightly different names from
  the BoringSSL ones, which maybe suggests we should rename them there
  and in the spec. The reasoning is our error codes end up in
  user-visible net error pages. While the spec uses terms like
  "ECH rejected" and "handshaking with ClientHelloOuter", those seem
  slightly confusing. I've used "ECH not negotiated" and "ECH fallback
  certificate". (Much of this was partly my fault in the spec, so I
  only have myself to blame here. :-) )

Bug: 1091403
Change-Id: Iaee9c92752f2559015f68cd34a8d99b92dac66eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216115
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Chris Thompson <cthomp@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/main@{#932609}

net/base/net_error_list.h

@@ -433,6 +433,14 @@ NET_ERROR(SSL_KEY_USAGE_INCOMPATIBLE, -181)
 // The ECHConfigList fetched over DNS cannot be parsed.
 NET_ERROR(INVALID_ECH_CONFIG_LIST, -182)
 
+// ECH was enabled, but the server was unable to decrypt the encrypted
+// ClientHello.
+NET_ERROR(ECH_NOT_NEGOTIATED, -183)
+
+// ECH was enabled, the server was unable to decrypt the encrypted ClientHello,
+// and additionally did not present a certificate valid for the public name.
+NET_ERROR(ECH_FALLBACK_CERTIFICATE_INVALID, -184)
+
 // Certificate error codes
 //
 // The values of certificate error codes must be consecutive.

net/cert/cert_verifier.cc

@@ -30,10 +30,10 @@ CertVerifier::RequestParams::RequestParams() = default;
 
 CertVerifier::RequestParams::RequestParams(
     scoped_refptr<X509Certificate> certificate,
-    const std::string& hostname,
+    base::StringPiece hostname,
     int flags,
-    const std::string& ocsp_response,
-    const std::string& sct_list)
+    base::StringPiece ocsp_response,
+    base::StringPiece sct_list)
     : certificate_(std::move(certificate)),
       hostname_(hostname),
       flags_(flags),
@@ -51,7 +51,7 @@ CertVerifier::RequestParams::RequestParams(
     SHA256_Update(&ctx, CRYPTO_BUFFER_data(cert_handle.get()),
                   CRYPTO_BUFFER_len(cert_handle.get()));
   }
-  SHA256_Update(&ctx, hostname_.data(), hostname.size());
+  SHA256_Update(&ctx, hostname.data(), hostname.size());
   SHA256_Update(&ctx, &flags, sizeof(flags));
   SHA256_Update(&ctx, ocsp_response.data(), ocsp_response.size());
   SHA256_Update(&ctx, sct_list.data(), sct_list.size());

net/cert/cert_verifier.h

@@ -11,6 +11,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/strings/string_piece.h"
 #include "net/base/completion_once_callback.h"
 #include "net/base/hash_value.h"
 #include "net/base/net_export.h"
@@ -123,10 +124,10 @@ class NET_EXPORT CertVerifier {
    public:
     RequestParams();
     RequestParams(scoped_refptr<X509Certificate> certificate,
-                  const std::string& hostname,
+                  base::StringPiece hostname,
                   int flags,
-                  const std::string& ocsp_response,
-                  const std::string& sct_list);
+                  base::StringPiece ocsp_response,
+                  base::StringPiece sct_list);
     RequestParams(const RequestParams& other);
     ~RequestParams();
 

net/socket/fuzzed_socket_factory.cc

@@ -102,6 +102,12 @@ class FailingSSLClientSocket : public SSLClientSocket {
     return 0;
   }
 
+  // SSLClientSocket implementation:
+  std::vector<uint8_t> GetECHRetryConfigs() override {
+    NOTREACHED();
+    return {};
+  }
+
  private:
   NetLogWithSource net_log_;
 };

net/socket/socket_test_util.cc

@@ -1683,6 +1683,13 @@ int MockSSLClientSocket::ExportKeyingMaterial(const base::StringPiece& label,
   return OK;
 }
 
+std::vector<uint8_t> MockSSLClientSocket::GetECHRetryConfigs() {
+  // TODO(crbug.com/1091403): Add a mechanism to specify this, when testing the
+  // retry portions of the recovery flow.
+  NOTIMPLEMENTED();
+  return {};
+}
+
 void MockSSLClientSocket::RunCallbackAsync(CompletionOnceCallback callback,
                                            int result) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(

net/socket/socket_test_util.h

@@ -1018,6 +1018,9 @@ class MockSSLClientSocket : public AsyncSocket, public SSLClientSocket {
                            unsigned char* out,
                            unsigned int outlen) override;
 
+  // SSLClientSocket implementation.
+  std::vector<uint8_t> GetECHRetryConfigs() override;
+
   // This MockSocket does not implement the manual async IO feature.
   void OnReadComplete(const MockRead& data) override;
   void OnWriteComplete(int rv) override;

net/socket/ssl_client_socket.h

@@ -41,6 +41,14 @@ class NET_EXPORT SSLClientSocket : public SSLSocket {
  public:
   SSLClientSocket();
 
+  // Called in response to |ERR_ECH_NOT_NEGOTIATED| in Connect(), to determine
+  // how to retry the connection, up to some limit. If this method returns a
+  // non-empty string, it is the serialized updated ECHConfigList provided by
+  // the server. The connection can be retried with the new value. If it returns
+  // an empty string, the server has indicated ECH has been disabled. The
+  // connection can be retried with ECH disabled.
+  virtual std::vector<uint8_t> GetECHRetryConfigs() = 0;
+
   // Log SSL key material to |logger|. Must be called before any
   // SSLClientSockets are created.
   //

net/socket/ssl_client_socket_impl.cc

@@ -26,6 +26,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/notreached.h"
 #include "base/rand_util.h"
 #include "base/strings/string_piece.h"
 #include "base/synchronization/lock.h"
@@ -250,6 +251,14 @@ bool IsCECPQ2Host(const std::string& host) {
              .find(features::kPostQuantumCECPQ2Prefix.Get()) == 0;
 }
 
+bool HostIsIPAddressNoBrackets(base::StringPiece host) {
+  // Note this cannot directly call url::HostIsIPAddress, because that function
+  // expects bracketed IPv6 literals. By the time hosts reach SSLClientSocket,
+  // brackets have been removed.
+  IPAddress unused;
+  return unused.AssignFromIPLiteral(host);
+}
+
 }  // namespace
 
 class SSLClientSocketImpl::SSLContext {
@@ -404,6 +413,13 @@ void SSLClientSocketImpl::SetSSLKeyLogger(
   SSLContext::GetInstance()->SetSSLKeyLogger(std::move(logger));
 }
 
+std::vector<uint8_t> SSLClientSocketImpl::GetECHRetryConfigs() {
+  const uint8_t* retry_configs;
+  size_t retry_configs_len;
+  SSL_get0_ech_retry_configs(ssl_.get(), &retry_configs, &retry_configs_len);
+  return std::vector<uint8_t>(retry_configs, retry_configs + retry_configs_len);
+}
+
 int SSLClientSocketImpl::ExportKeyingMaterial(const base::StringPiece& label,
                                               bool has_context,
                                               const base::StringPiece& context,
@@ -746,9 +762,8 @@ int SSLClientSocketImpl::Init() {
   if (!ssl_ || !context->SetClientSocketForSSL(ssl_.get(), this))
     return ERR_UNEXPECTED;
 
-  IPAddress unused;
   const bool host_is_ip_address =
-      unused.AssignFromIPLiteral(host_and_port_.host());
+      HostIsIPAddressNoBrackets(host_and_port_.host());
 
   // SNI should only contain valid DNS hostnames, not IP addresses (see RFC
   // 6066, Section 3).
@@ -997,6 +1012,12 @@ int SSLClientSocketImpl::DoHandshakeComplete(int result) {
     return OK;
   }
 
+  // If ECH overrode certificate verification to authenticate a fallback, using
+  // the socket for application data would bypass server authentication.
+  // BoringSSL will never complete the handshake in this case, so this should
+  // not happen.
+  CHECK(!used_ech_name_override_);
+
   const uint8_t* alpn_proto = nullptr;
   unsigned alpn_len = 0;
   SSL_get0_alpn_selected(ssl_.get(), &alpn_proto, &alpn_len);
@@ -1115,18 +1136,6 @@ ssl_verify_result_t SSLClientSocketImpl::VerifyCert() {
     return HandleVerifyResult();
   }
 
-  // TODO(crbug.com/1091403): Implement the recovery flow.
-  const char* ech_name_override;
-  size_t ech_name_override_len;
-  SSL_get0_ech_name_override(ssl_.get(), &ech_name_override,
-                             &ech_name_override_len);
-  if (ech_name_override_len > 0) {
-    DCHECK(!ssl_config_.ech_config_list.empty());
-    NOTIMPLEMENTED();
-    OpenSSLPutNetError(FROM_HERE, ERR_FAILED);
-    return ssl_verify_invalid;
-  }
-
   // In this configuration, BoringSSL will perform exactly one certificate
   // verification, so there cannot be state from a previous verification.
   CHECK(!server_cert_);
@@ -1150,7 +1159,7 @@ ssl_verify_result_t SSLClientSocketImpl::VerifyCert() {
   // If the certificate is bad and has been previously accepted, use
   // the previous status and bypass the error.
   CertStatus cert_status;
-  if (ssl_config_.IsAllowedBadCert(server_cert_.get(), &cert_status)) {
+  if (IsAllowedBadCert(server_cert_.get(), &cert_status)) {
     server_cert_verify_result_.Reset();
     server_cert_verify_result_.cert_status = cert_status;
     server_cert_verify_result_.verified_cert = server_cert_;
@@ -1160,6 +1169,34 @@ ssl_verify_result_t SSLClientSocketImpl::VerifyCert() {
 
   start_cert_verification_time_ = base::TimeTicks::Now();
 
+  base::StringPiece 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;
+
+    // CertVerifier::Verify takes a string host and internally interprets it as
+    // either a DNS name or IP address. However, the ECH public name is only
+    // defined to be an DNS name. Thus, reject all public names that would not
+    // be interpreted as IP addresses. Distinguishing IPv4 literals from DNS
+    // names varies by spec, however. BoringSSL internally checks for an LDH
+    // string, and that the last component is non-numeric. This should be
+    // sufficient for the web, but check with Chromium's parser, in case they
+    // diverge.
+    //
+    // See section 6.1.7 of draft-ietf-tls-esni-13.
+    if (HostIsIPAddressNoBrackets(ech_name_override)) {
+      NOTREACHED();
+      OpenSSLPutNetError(FROM_HERE, ERR_INVALID_ECH_CONFIG_LIST);
+      return ssl_verify_invalid;
+    }
+  }
+
   const uint8_t* ocsp_response_raw;
   size_t ocsp_response_len;
   SSL_get0_ocsp_response(ssl_.get(), &ocsp_response_raw, &ocsp_response_len);
@@ -1174,8 +1211,10 @@ ssl_verify_result_t SSLClientSocketImpl::VerifyCert() {
 
   cert_verification_result_ = context_->cert_verifier()->Verify(
       CertVerifier::RequestParams(
-          server_cert_, host_and_port_.host(), ssl_config_.GetCertVerifyFlags(),
-          std::string(ocsp_response), std::string(sct_list)),
+          server_cert_,
+          ech_name_override.empty() ? host_and_port_.host() : ech_name_override,
+          ssl_config_.GetCertVerifyFlags(), std::string(ocsp_response),
+          std::string(sct_list)),
       &server_cert_verify_result_,
       base::BindOnce(&SSLClientSocketImpl::OnVerifyComplete,
                      base::Unretained(this)),
@@ -1260,7 +1299,7 @@ ssl_verify_result_t SSLClientSocketImpl::HandleVerifyResult() {
     server_cert_verify_result_.cert_status |= CERT_STATUS_LEGACY_TLS;
 
     // Only set the resulting net error if it hasn't been previously bypassed.
-    if (!ssl_config_.IsAllowedBadCert(server_cert_.get(), nullptr))
+    if (!IsAllowedBadCert(server_cert_.get(), nullptr))
       result = ERR_SSL_OBSOLETE_VERSION;
   }
 
@@ -1271,8 +1310,18 @@ ssl_verify_result_t SSLClientSocketImpl::HandleVerifyResult() {
       context_->transport_security_state()->ShouldSSLErrorsBeFatal(
           host_and_port_.host());
 
-  if (IsCertificateError(result) && ssl_config_.ignore_certificate_errors) {
-    result = OK;
+  if (IsCertificateError(result)) {
+    if (!GetECHNameOverride().empty()) {
+      // Certificate exceptions are only applicable for the origin name. For
+      // simplicity, we do not allow certificate exceptions for the public name
+      // and map all bypassable errors to fatal ones.
+      result = result == ERR_SSL_OBSOLETE_VERSION
+                   ? ERR_SSL_VERSION_OR_CIPHER_MISMATCH
+                   : ERR_ECH_FALLBACK_CERTIFICATE_INVALID;
+    }
+    if (ssl_config_.ignore_certificate_errors) {
+      result = OK;
+    }
   }
 
   if (result == OK) {
@@ -1897,4 +1946,21 @@ int SSLClientSocketImpl::MapLastOpenSSLError(
   return net_error;
 }
 
+base::StringPiece SSLClientSocketImpl::GetECHNameOverride() const {
+  const char* data;
+  size_t len;
+  SSL_get0_ech_name_override(ssl_.get(), &data, &len);
+  return base::StringPiece(data, len);
+}
+
+bool SSLClientSocketImpl::IsAllowedBadCert(X509Certificate* cert,
+                                           CertStatus* cert_status) const {
+  if (!GetECHNameOverride().empty()) {
+    // Certificate exceptions are only applicable for the origin name. For
+    // simplicity, we do not allow certificate exceptions for the public name.
+    return false;
+  }
+  return ssl_config_.IsAllowedBadCert(cert, cert_status);
+}
+
 }  // namespace net

net/socket/ssl_client_socket_impl.h

@@ -71,6 +71,9 @@ class SSLClientSocketImpl : public SSLClientSocket,
   // SSLClientSockets are created.
   static void SetSSLKeyLogger(std::unique_ptr<SSLKeyLogger> logger);
 
+  // SSLClientSocket implementation.
+  std::vector<uint8_t> GetECHRetryConfigs() override;
+
   // SSLSocket implementation.
   int ExportKeyingMaterial(const base::StringPiece& label,
                            bool has_context,
@@ -203,6 +206,15 @@ class SSLClientSocketImpl : public SSLClientSocket,
                           const crypto::OpenSSLErrStackTracer& tracer,
                           OpenSSLErrorInfo* info);
 
+  // Wraps SSL_get0_ech_name_override. See documentation for that function.
+  base::StringPiece GetECHNameOverride() const;
+
+  // Returns true if |cert| is one of the certs in |allowed_bad_certs|.
+  // The expected cert status is written to |cert_status|. |*cert_status| can
+  // be nullptr if user doesn't care about the cert status. This method checks
+  // handshake state, so it may only be called during certificate verification.
+  bool IsAllowedBadCert(X509Certificate* cert, CertStatus* cert_status) const;
+
   CompletionOnceCallback user_connect_callback_;
   CompletionOnceCallback user_read_callback_;
   CompletionOnceCallback user_write_callback_;
@@ -275,6 +287,9 @@ class SSLClientSocketImpl : public SSLClientSocket,
   // True if the socket has been disconnected.
   bool disconnected_;
 
+  // True if certificate verification used an ECH name override.
+  bool used_ech_name_override_ = false;
+
   NextProto negotiated_protocol_;
 
   // Set to true if a CertificateRequest was received.

net/socket/ssl_client_socket_unittest.cc

@@ -75,6 +75,7 @@
 #include "net/socket/tcp_server_socket.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_client_session_cache.h"
+#include "net/ssl/ssl_config.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/ssl/ssl_handshake_details.h"
@@ -5580,16 +5581,19 @@ TEST_F(SSLClientSocketTest, ECH) {
   EXPECT_FALSE(server_ssl_info->encrypted_client_hello);
 }
 
+// Test that, on key mismatch, the public name can be used to authenticate
+// replacement keys.
 TEST_F(SSLClientSocketTest, ECHWrongKeys) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(features::kEncryptedClientHello);
 
+  static const char kPublicName[] = "public.example";
   std::vector<uint8_t> ech_config_list1, ech_config_list2;
   bssl::UniquePtr<SSL_ECH_KEYS> keys1 =
-      MakeTestECHKeys("public.example", /*max_name_len=*/64, &ech_config_list1);
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list1);
   ASSERT_TRUE(keys1);
   bssl::UniquePtr<SSL_ECH_KEYS> keys2 =
-      MakeTestECHKeys("public.example", /*max_name_len=*/64, &ech_config_list2);
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list2);
   ASSERT_TRUE(keys2);
 
   // Configure the client and server with different keys.
@@ -5601,12 +5605,165 @@ TEST_F(SSLClientSocketTest, ECHWrongKeys) {
   ASSERT_TRUE(
       StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
 
-  // Connecting with the client should use ECH.
+  // Verify the fallback handshake verifies the certificate against the public
+  // name.
+  cert_verifier_->set_default_result(ERR_CERT_INVALID);
+  scoped_refptr<X509Certificate> server_cert =
+      embedded_test_server()->GetCertificate();
+  CertVerifyResult verify_result;
+  verify_result.verified_cert = server_cert;
+  cert_verifier_->AddResultForCertAndHost(server_cert, kPublicName,
+                                          verify_result, OK);
+
+  // Connecting with the client should report ECH was not negotiated.
   int rv;
   ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
-  // TODO(crbug.com/1091403): Implement the recovery flow. Test that this both
-  // verifies against the public name and exposes retry configs.
-  EXPECT_THAT(rv, IsError(ERR_FAILED));
+  EXPECT_THAT(rv, IsError(ERR_ECH_NOT_NEGOTIATED));
+
+  // The server's keys are available as retry keys.
+  EXPECT_EQ(ech_config_list1, sock_->GetECHRetryConfigs());
+}
+
+// Test that, if the server does not support ECH, it can securely report this
+// via the public name. This allows recovery if the server needed to
+// rollback ECH support.
+TEST_F(SSLClientSocketTest, ECHSecurelyDisabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kEncryptedClientHello);
+
+  static const char kPublicName[] = "public.example";
+  std::vector<uint8_t> ech_config_list;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys =
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list);
+  ASSERT_TRUE(keys);
+
+  // The server does not have keys configured.
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, SSLServerConfig()));
+
+  // However it can authenticate for kPublicName.
+  cert_verifier_->set_default_result(ERR_CERT_INVALID);
+  scoped_refptr<X509Certificate> server_cert =
+      embedded_test_server()->GetCertificate();
+  CertVerifyResult verify_result;
+  verify_result.verified_cert = server_cert;
+  cert_verifier_->AddResultForCertAndHost(server_cert, kPublicName,
+                                          verify_result, OK);
+
+  // Connecting with the client should report ECH was not negotiated.
+  SSLConfig client_config;
+  client_config.ech_config_list = std::move(ech_config_list);
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsError(ERR_ECH_NOT_NEGOTIATED));
+
+  // The retry config is empty, meaning the server has securely reported that
+  // ECH is disabled
+  EXPECT_TRUE(sock_->GetECHRetryConfigs().empty());
+}
+
+// The same as the above, but testing that it also works in TLS 1.2, which
+// otherwise does not support ECH.
+TEST_F(SSLClientSocketTest, ECHSecurelyDisabledTLS12) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kEncryptedClientHello);
+
+  static const char kPublicName[] = "public.example";
+  std::vector<uint8_t> ech_config_list;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys =
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list);
+  ASSERT_TRUE(keys);
+
+  // The server does not have keys configured.
+  SSLServerConfig server_config;
+  server_config.version_max = SSL_PROTOCOL_VERSION_TLS1_2;
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
+
+  // However it can authenticate for kPublicName.
+  cert_verifier_->set_default_result(ERR_CERT_INVALID);
+  scoped_refptr<X509Certificate> server_cert =
+      embedded_test_server()->GetCertificate();
+  CertVerifyResult verify_result;
+  verify_result.verified_cert = server_cert;
+  cert_verifier_->AddResultForCertAndHost(server_cert, kPublicName,
+                                          verify_result, OK);
+
+  // Connecting with the client should report ECH was not negotiated.
+  SSLConfig client_config;
+  client_config.ech_config_list = std::move(ech_config_list);
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsError(ERR_ECH_NOT_NEGOTIATED));
+
+  // The retry config is empty, meaning the server has securely reported that
+  // ECH is disabled
+  EXPECT_TRUE(sock_->GetECHRetryConfigs().empty());
+}
+
+// Test that the ECH fallback handshake rejects bad certificates.
+TEST_F(SSLClientSocketTest, ECHFallbackBadCert) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kEncryptedClientHello);
+
+  static const char kPublicName[] = "public.example";
+  std::vector<uint8_t> ech_config_list1, ech_config_list2;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys1 =
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list1);
+  ASSERT_TRUE(keys1);
+  bssl::UniquePtr<SSL_ECH_KEYS> keys2 =
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list2);
+  ASSERT_TRUE(keys2);
+
+  // Configure the client and server with different keys.
+  SSLServerConfig server_config;
+  server_config.ech_keys = std::move(keys1);
+  SSLConfig client_config;
+  client_config.ech_config_list = std::move(ech_config_list2);
+
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
+
+  // Configure the client to reject the certificate for the public name (or any
+  // other name).
+  cert_verifier_->set_default_result(ERR_CERT_INVALID);
+
+  // Connecting with the client will fail with a fatal error.
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsError(ERR_ECH_FALLBACK_CERTIFICATE_INVALID));
+}
+
+// Test that the ECH fallback handshake rejects legacy TLS versions.
+TEST_F(SSLClientSocketTest, ECHFallbackLegacyVersion) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kEncryptedClientHello);
+
+  static const char kPublicName[] = "public.example";
+  std::vector<uint8_t> ech_config_list;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys =
+      MakeTestECHKeys(kPublicName, /*max_name_len=*/64, &ech_config_list);
+  ASSERT_TRUE(keys);
+
+  // Configure TLS 1.1 as allowed, but with a bypassable error.
+  SSLContextConfig context_config;
+  context_config.version_min = SSL_PROTOCOL_VERSION_TLS1;
+  context_config.version_min_warn = SSL_PROTOCOL_VERSION_TLS1_2;
+  ssl_config_service_->UpdateSSLConfigAndNotify(context_config);
+
+  SSLServerConfig server_config;
+  server_config.version_max = SSL_PROTOCOL_VERSION_TLS1_1;
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
+
+  // We do not implement bypassable errors for the fallback handshake, so this
+  // should report a fatal ERR_SSL_VERSION_OR_CIPHER_MISMATCH, not
+  // ERR_SSL_OBSOLETE_VERSION.
+  SSLConfig client_config;
+  client_config.ech_config_list = std::move(ech_config_list);
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsError(ERR_SSL_VERSION_OR_CIPHER_MISMATCH));
 }
 
 TEST_F(SSLClientSocketTest, InvalidECHConfigList) {

net/ssl/openssl_ssl_util.cc

@@ -108,6 +108,8 @@ int MapOpenSSLErrorSSL(uint32_t error_code) {
       return ERR_WRONG_VERSION_ON_EARLY_DATA;
     case SSL_R_TLS13_DOWNGRADE:
       return ERR_TLS13_DOWNGRADE_DETECTED;
+    case SSL_R_ECH_REJECTED:
+      return ERR_ECH_NOT_NEGOTIATED;
     // SSL_R_SSLV3_ALERT_HANDSHAKE_FAILURE may be returned from the server after
     // receiving ClientHello if there's no common supported cipher. Map that
     // specific case to ERR_SSL_VERSION_OR_CIPHER_MISMATCH to match the NSS

net/ssl/ssl_config.h

@@ -138,9 +138,9 @@ struct NET_EXPORT SSLConfig {
   NetworkIsolationKey network_isolation_key;
 
   // If non-empty, a serialized ECHConfigList to use to encrypt the ClientHello.
-  //
-  // TODO(crbug.com/1091403): Support is currently incomplete. Implement the
-  // recovery flow and document what this does to the socket behavior.
+  // If this field is non-empty, callers should handle |ERR_ECH_NOT_NEGOTIATED|
+  // errors from Connect() by calling GetECHRetryConfigs() to determine how to
+  // retry the connection.
   std::vector<uint8_t> ech_config_list;
 
   // An additional boolean to partition the session cache by.