Remove the Channel ID callback.

The remaining remnants of Channel ID all configure the private key ahead
of time. Unwind the callback machinery, which cuts down on async points
and the cases we need to test.

This also unwinds some odd interaction between the callback and
SSL_set_tls_channel_id_enabled: If a client uses
SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake
would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now
removed, so SSL_set_tls_channel_id_enabled only affects the server and
SSL_CTX_set1_tls_channel_id only affects the client.

Update-Note: SSL_CTX_set_channel_id_cb is removed.
SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client,
only as a server.

Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907
Reviewed-by: Adam Langley <agl@google.com>

include/openssl/ssl.h

@@ -508,12 +508,10 @@ OPENSSL_EXPORT int SSL_get_error(const SSL *ssl, int ret_code);
 // TODO(davidben): Remove this. It's used by accept BIOs which are bizarre.
 #define SSL_ERROR_WANT_ACCEPT 8
 
-// SSL_ERROR_WANT_CHANNEL_ID_LOOKUP indicates the operation failed looking up
-// the Channel ID key. The caller may retry the operation when |channel_id_cb|
-// is ready to return a key or one has been configured with
-// |SSL_set1_tls_channel_id|.
+// SSL_ERROR_WANT_CHANNEL_ID_LOOKUP is never used.
 //
-// See also |SSL_CTX_set_channel_id_cb|.
+// TODO(davidben): Remove this. Some callers reference it when stringifying
+// errors. They should use |SSL_error_description| instead.
 #define SSL_ERROR_WANT_CHANNEL_ID_LOOKUP 9
 
 // SSL_ERROR_PENDING_SESSION indicates the operation failed because the session
@@ -2974,15 +2972,16 @@ OPENSSL_EXPORT int SSL_select_next_proto(uint8_t **out, uint8_t *out_len,
 
 // Channel ID.
 //
-// See draft-balfanz-tls-channelid-01.
+// See draft-balfanz-tls-channelid-01. This is an old, experimental mechanism
+// and should not be used in new code.
 
 // SSL_CTX_set_tls_channel_id_enabled configures whether connections associated
-// with |ctx| should enable Channel ID.
+// with |ctx| should enable Channel ID as a server.
 OPENSSL_EXPORT void SSL_CTX_set_tls_channel_id_enabled(SSL_CTX *ctx,
                                                        int enabled);
 
 // SSL_set_tls_channel_id_enabled configures whether |ssl| should enable Channel
-// ID.
+// ID as a server.
 OPENSSL_EXPORT void SSL_set_tls_channel_id_enabled(SSL *ssl, int enabled);
 
 // SSL_CTX_set1_tls_channel_id configures a TLS client to send a TLS Channel ID
@@ -3005,20 +3004,6 @@ OPENSSL_EXPORT int SSL_set1_tls_channel_id(SSL *ssl, EVP_PKEY *private_key);
 OPENSSL_EXPORT size_t SSL_get_tls_channel_id(SSL *ssl, uint8_t *out,
                                              size_t max_out);
 
-// SSL_CTX_set_channel_id_cb sets a callback to be called when a TLS Channel ID
-// is requested. The callback may set |*out_pkey| to a key, passing a reference
-// to the caller. If none is returned, the handshake will pause and
-// |SSL_get_error| will return |SSL_ERROR_WANT_CHANNEL_ID_LOOKUP|.
-//
-// See also |SSL_ERROR_WANT_CHANNEL_ID_LOOKUP|.
-OPENSSL_EXPORT void SSL_CTX_set_channel_id_cb(
-    SSL_CTX *ctx, void (*channel_id_cb)(SSL *ssl, EVP_PKEY **out_pkey));
-
-// SSL_CTX_get_channel_id_cb returns the callback set by
-// |SSL_CTX_set_channel_id_cb|.
-OPENSSL_EXPORT void (*SSL_CTX_get_channel_id_cb(SSL_CTX *ctx))(
-    SSL *ssl, EVP_PKEY **out_pkey);
-
 
 // DTLS-SRTP.
 //

ssl/handshake.cc

@@ -684,10 +684,6 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
         ssl->s3->rwstate = SSL_ERROR_WANT_X509_LOOKUP;
         hs->wait = ssl_hs_ok;
         return -1;
-      case ssl_hs_channel_id_lookup:
-        ssl->s3->rwstate = SSL_ERROR_WANT_CHANNEL_ID_LOOKUP;
-        hs->wait = ssl_hs_ok;
-        return -1;
       case ssl_hs_private_key_operation:
         ssl->s3->rwstate = SSL_ERROR_WANT_PRIVATE_KEY_OPERATION;
         hs->wait = ssl_hs_ok;

ssl/handshake_client.cc

@@ -1480,18 +1480,6 @@ static enum ssl_hs_wait_t do_send_client_certificate_verify(SSL_HANDSHAKE *hs) {
 static enum ssl_hs_wait_t do_send_client_finished(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
   hs->can_release_private_key = true;
-  // Resolve Channel ID first, before any non-idempotent operations.
-  if (hs->channel_id_negotiated) {
-    if (!ssl_do_channel_id_callback(hs)) {
-      return ssl_hs_error;
-    }
-
-    if (hs->config->channel_id_private == NULL) {
-      hs->state = state_send_client_finished;
-      return ssl_hs_channel_id_lookup;
-    }
-  }
-
   if (!ssl->method->add_change_cipher_spec(ssl) ||
       !tls1_change_cipher_state(hs, evp_aead_seal)) {
     return ssl_hs_error;

ssl/internal.h

@@ -1557,7 +1557,6 @@ enum ssl_hs_wait_t {
   ssl_hs_handoff,
   ssl_hs_handback,
   ssl_hs_x509_lookup,
-  ssl_hs_channel_id_lookup,
   ssl_hs_private_key_operation,
   ssl_hs_pending_session,
   ssl_hs_pending_ticket,
@@ -2862,7 +2861,8 @@ struct SSL_CONFIG {
 
   Array<uint16_t> supported_group_list;  // our list
 
-  // The client's Channel ID private key.
+  // channel_id_private is the client's Channel ID private key, or null if
+  // Channel ID should not be offered on this connection.
   UniquePtr<EVP_PKEY> channel_id_private;
 
   // For a client, this contains the list of supported protocols in wire
@@ -2901,9 +2901,8 @@ struct SSL_CONFIG {
   // whether OCSP stapling will be requested.
   bool ocsp_stapling_enabled : 1;
 
-  // channel_id_enabled is copied from the |SSL_CTX|. For a server, means that
-  // we'll accept Channel IDs from clients. For a client, means that we'll
-  // advertise support.
+  // channel_id_enabled is copied from the |SSL_CTX|. For a server, it means
+  // that we'll accept Channel IDs from clients. It is ignored on the client.
   bool channel_id_enabled : 1;
 
   // If enforce_rsa_key_usage is true, the handshake will fail if the
@@ -3195,12 +3194,6 @@ bool tls1_channel_id_hash(SSL_HANDSHAKE *hs, uint8_t *out, size_t *out_len);
 // data.
 bool tls1_record_handshake_hashes_for_channel_id(SSL_HANDSHAKE *hs);
 
-// ssl_do_channel_id_callback checks runs |hs->ssl->ctx->channel_id_cb| if
-// necessary. It returns true on success and false on fatal error. Note that, on
-// success, |hs->ssl->channel_id_private| may be unset, in which case the
-// operation should be retried later.
-bool ssl_do_channel_id_callback(SSL_HANDSHAKE *hs);
-
 // ssl_can_write returns whether |ssl| is allowed to write.
 bool ssl_can_write(const SSL *ssl);
 
@@ -3324,9 +3317,6 @@ struct ssl_ctx_st {
   int (*client_cert_cb)(SSL *ssl, X509 **out_x509,
                         EVP_PKEY **out_pkey) = nullptr;
 
-  // get channel id callback
-  void (*channel_id_cb)(SSL *ssl, EVP_PKEY **out_pkey) = nullptr;
-
   CRYPTO_EX_DATA ex_data;
 
   // Default values used when no per-SSL value is defined follow
@@ -3454,7 +3444,8 @@ struct ssl_ctx_st {
   // Supported group values inherited by SSL structure
   bssl::Array<uint16_t> supported_group_list;
 
-  // The client's Channel ID private key.
+  // channel_id_private is the client's Channel ID private key, or null if
+  // Channel ID should not be offered on this connection.
   bssl::UniquePtr<EVP_PKEY> channel_id_private;
 
   // ech_server_config_list contains the server's list of ECHConfig values and

ssl/ssl_lib.cc

@@ -1369,7 +1369,6 @@ int SSL_get_error(const SSL *ssl, int ret_code) {
     case SSL_ERROR_HANDOFF:
     case SSL_ERROR_HANDBACK:
     case SSL_ERROR_WANT_X509_LOOKUP:
-    case SSL_ERROR_WANT_CHANNEL_ID_LOOKUP:
     case SSL_ERROR_WANT_PRIVATE_KEY_OPERATION:
     case SSL_ERROR_PENDING_TICKET:
     case SSL_ERROR_EARLY_DATA_REJECTED:
@@ -1443,8 +1442,6 @@ const char *SSL_error_description(int err) {
       return "WANT_CONNECT";
     case SSL_ERROR_WANT_ACCEPT:
       return "WANT_ACCEPT";
-    case SSL_ERROR_WANT_CHANNEL_ID_LOOKUP:
-      return "WANT_CHANNEL_ID_LOOKUP";
     case SSL_ERROR_PENDING_SESSION:
       return "PENDING_SESSION";
     case SSL_ERROR_PENDING_CERTIFICATE:
@@ -2437,8 +2434,6 @@ int SSL_CTX_set1_tls_channel_id(SSL_CTX *ctx, EVP_PKEY *private_key) {
   }
 
   ctx->channel_id_private = UpRef(private_key);
-  ctx->channel_id_enabled = true;
-
   return 1;
 }
 
@@ -2452,8 +2447,6 @@ int SSL_set1_tls_channel_id(SSL *ssl, EVP_PKEY *private_key) {
   }
 
   ssl->config->channel_id_private = UpRef(private_key);
-  ssl->config->channel_id_enabled = true;
-
   return 1;
 }
 

ssl/ssl_session.cc

@@ -1284,12 +1284,3 @@ void (*SSL_CTX_get_info_callback(SSL_CTX *ctx))(const SSL *ssl, int type,
                                                 int value) {
   return ctx->info_callback;
 }
-
-void SSL_CTX_set_channel_id_cb(SSL_CTX *ctx,
-                               void (*cb)(SSL *ssl, EVP_PKEY **pkey)) {
-  ctx->channel_id_cb = cb;
-}
-
-void (*SSL_CTX_get_channel_id_cb(SSL_CTX *ctx))(SSL *ssl, EVP_PKEY **pkey) {
-  return ctx->channel_id_cb;
-}

ssl/t1_lib.cc

@@ -1643,7 +1643,7 @@ static bool ext_alpn_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
 
 static bool ext_channel_id_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
   SSL *const ssl = hs->ssl;
-  if (!hs->config->channel_id_enabled || SSL_is_dtls(ssl)) {
+  if (!hs->config->channel_id_private || SSL_is_dtls(ssl)) {
     return true;
   }
 
@@ -1663,7 +1663,7 @@ static bool ext_channel_id_parse_serverhello(SSL_HANDSHAKE *hs,
   }
 
   assert(!SSL_is_dtls(hs->ssl));
-  assert(hs->config->channel_id_enabled);
+  assert(hs->config->channel_id_private);
 
   if (CBS_len(contents) != 0) {
     return false;
@@ -4189,23 +4189,6 @@ bool tls1_record_handshake_hashes_for_channel_id(SSL_HANDSHAKE *hs) {
   return true;
 }
 
-bool ssl_do_channel_id_callback(SSL_HANDSHAKE *hs) {
-  if (hs->config->channel_id_private != NULL ||
-      hs->ssl->ctx->channel_id_cb == NULL) {
-    return true;
-  }
-
-  EVP_PKEY *key = NULL;
-  hs->ssl->ctx->channel_id_cb(hs->ssl, &key);
-  if (key == NULL) {
-    // The caller should try again later.
-    return true;
-  }
-
-  UniquePtr<EVP_PKEY> free_key(key);
-  return SSL_set1_tls_channel_id(hs->ssl, key);
-}
-
 bool ssl_is_sct_list_valid(const CBS *contents) {
   // Shallow parse the SCT list for sanity. By the RFC
   // (https://tools.ietf.org/html/rfc6962#section-3.3) neither the list nor any

ssl/test/handshake_util.cc

@@ -86,14 +86,6 @@ bool RetryAsync(SSL *ssl, int ret) {
     case SSL_ERROR_WANT_WRITE:
       AsyncBioAllowWrite(test_state->async_bio, 1);
       return true;
-    case SSL_ERROR_WANT_CHANNEL_ID_LOOKUP: {
-      UniquePtr<EVP_PKEY> pkey = LoadPrivateKey(config->send_channel_id);
-      if (!pkey) {
-        return false;
-      }
-      test_state->channel_id = std::move(pkey);
-      return true;
-    }
     case SSL_ERROR_WANT_X509_LOOKUP:
       test_state->cert_ready = true;
       return true;

ssl/test/test_config.cc

@@ -695,10 +695,6 @@ static void InfoCallback(const SSL *ssl, int type, int val) {
   }
 }
 
-static void ChannelIdCallback(SSL *ssl, EVP_PKEY **out_pkey) {
-  *out_pkey = GetTestState(ssl)->channel_id.release();
-}
-
 static SSL_SESSION *GetSessionCallback(SSL *ssl, const uint8_t *data, int len,
                                        int *copy) {
   TestState *async_state = GetTestState(ssl);
@@ -1376,8 +1372,6 @@ bssl::UniquePtr<SSL_CTX> TestConfig::SetupCtx(SSL_CTX *old_ctx) const {
     SSL_CTX_set_alpn_select_cb(ssl_ctx.get(), AlpnSelectCallback, NULL);
   }
 
-  SSL_CTX_set_channel_id_cb(ssl_ctx.get(), ChannelIdCallback);
-
   SSL_CTX_set_current_time_cb(ssl_ctx.get(), CurrentTimeCallback);
 
   SSL_CTX_set_info_callback(ssl_ctx.get(), InfoCallback);
@@ -1727,13 +1721,9 @@ bssl::UniquePtr<SSL> TestConfig::NewSSL(
     }
   }
   if (!send_channel_id.empty()) {
-    SSL_set_tls_channel_id_enabled(ssl.get(), 1);
-    if (!async) {
-      // The async case will be supplied by |ChannelIdCallback|.
-      bssl::UniquePtr<EVP_PKEY> pkey = LoadPrivateKey(send_channel_id);
-      if (!pkey || !SSL_set1_tls_channel_id(ssl.get(), pkey.get())) {
-        return nullptr;
-      }
+    bssl::UniquePtr<EVP_PKEY> pkey = LoadPrivateKey(send_channel_id);
+    if (!pkey || !SSL_set1_tls_channel_id(ssl.get(), pkey.get())) {
+      return nullptr;
     }
   }
   if (!host_name.empty() &&

ssl/test/test_state.h

@@ -41,7 +41,6 @@ struct TestState {
   // packeted_bio is the packeted BIO which simulates read timeouts.
   BIO *packeted_bio = nullptr;
   std::unique_ptr<MockQuicTransport> quic_transport;
-  bssl::UniquePtr<EVP_PKEY> channel_id;
   bool cert_ready = false;
   bssl::UniquePtr<SSL_SESSION> session;
   bssl::UniquePtr<SSL_SESSION> pending_session;

ssl/tls13_client.cc

@@ -821,15 +821,6 @@ static enum ssl_hs_wait_t do_complete_second_flight(SSL_HANDSHAKE *hs) {
 
   // Send a Channel ID assertion if necessary.
   if (hs->channel_id_negotiated) {
-    if (!ssl_do_channel_id_callback(hs)) {
-      hs->tls13_state = state_complete_second_flight;
-      return ssl_hs_error;
-    }
-
-    if (hs->config->channel_id_private == NULL) {
-      return ssl_hs_channel_id_lookup;
-    }
-
     ScopedCBB cbb;
     CBB body;
     if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_CHANNEL_ID) ||