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>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index cad9750..828938d 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -508,12 +508,10 @@
// 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 @@
// 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 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.
//
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index ca2238f..ebd5df0 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -684,10 +684,6 @@
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;
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index b7432ec..fbf0ef5 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1480,18 +1480,6 @@
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;
diff --git a/ssl/internal.h b/ssl/internal.h
index 7be766d..ad31e54 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1557,7 +1557,6 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
// 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
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index a05b9b1..65dcfae 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -1369,7 +1369,6 @@
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 @@
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 @@
}
ctx->channel_id_private = UpRef(private_key);
- ctx->channel_id_enabled = true;
-
return 1;
}
@@ -2452,8 +2447,6 @@
}
ssl->config->channel_id_private = UpRef(private_key);
- ssl->config->channel_id_enabled = true;
-
return 1;
}
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index a52ec3d..b0f0892 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -1284,12 +1284,3 @@
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;
-}
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index a63ca5d..44c96e8 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1643,7 +1643,7 @@
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 @@
}
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 @@
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
diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc
index 34cee47..b999831 100644
--- a/ssl/test/handshake_util.cc
+++ b/ssl/test/handshake_util.cc
@@ -86,14 +86,6 @@
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;
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 47fac65..c68df9f 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -695,10 +695,6 @@
}
}
-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 @@
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 @@
}
}
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() &&
diff --git a/ssl/test/test_state.h b/ssl/test/test_state.h
index d9fe945..4199d4a 100644
--- a/ssl/test/test_state.h
+++ b/ssl/test/test_state.h
@@ -41,7 +41,6 @@
// 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;
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index bac37ff..37ca4b2 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -821,15 +821,6 @@
// 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) ||
