initial config influences negotiation when using client hello config resolution

[jmayclin]

Problem:

Customers commonly want to configure a config based on the presented SNI. For example, setting different security policies or different certificates based on SNI. Our client-hello-config-resolution example shows how to set this up.

The example includes the following comment

s2n-tls/bindings/rust-examples/client-hello-config-resolution/src/bin/server.rs

Lines 96 to 101 in 4dae2b2

	// this is the initial config that "receives" the connection. Since we error
	// out on an unrecognized SNI, the only important setting on this config is
	// the client hello callback that sets the config used for the rest of the
	// connection.
	let mut initial_config = s2n_tls::config::Builder::new();
	initial_config.set_client_hello_callback(resolver)?;

While this is true in the general case, there are some suprising side effects that we either need to change or document.

We call the client-hello-cb at the end of s2n_client_hello_recv, but s2n_parse_client_hello is called with the initial config. s2n_parse_client_hello relies on the initial config in two places.

SSLv2 Client Hellos

As part of parse_client_hello, we handle SSLv2 client hellos. We reject some of the SSLv2 formatted client hellos based off of the supported version of the security policy on the initial client hello.

s2n-tls/tls/s2n_client_hello.c

Lines 814 to 831 in 4dae2b2

	int s2n_sslv2_client_hello_recv(struct s2n_connection *conn)
	{
	struct s2n_client_hello *client_hello = &conn->client_hello;
	client_hello->sslv2 = true;
	struct s2n_stuffer in_stuffer = { 0 };
	POSIX_GUARD(s2n_stuffer_init(&in_stuffer, &client_hello->raw_message));
	POSIX_GUARD(s2n_stuffer_skip_write(&in_stuffer, client_hello->raw_message.size));
	struct s2n_stuffer *in = &in_stuffer;
	const struct s2n_security_policy *security_policy = NULL;
	POSIX_GUARD(s2n_connection_get_security_policy(conn, &security_policy));
	if (conn->client_protocol_version < security_policy->minimum_protocol_version) {
	POSIX_GUARD(s2n_queue_reader_unsupported_protocol_version_alert(conn));
	POSIX_BAIL(S2N_ERR_PROTOCOL_VERSION_UNSUPPORTED);
	}
	conn->actual_protocol_version = MIN(conn->client_protocol_version, conn->server_protocol_version);

Default Curve

If the client doesn't explicitly specify the supported elliptic curves, s2n-tls will choose the default based on the curves in the initial config.

s2n-tls/tls/s2n_client_hello.c

Lines 474 to 500 in 4dae2b2

	/* Set default key exchange curve.
	* This is going to be our fallback if the client has no preference.
	*
	* P-256 is our preferred fallback option because the TLS1.3 RFC requires
	* all implementations to support it:
	*
	* https://tools.ietf.org/rfc/rfc8446#section-9.1
	* A TLS-compliant application MUST support key exchange with secp256r1 (NIST P-256)
	* and SHOULD support key exchange with X25519 [RFC7748]
	*
	*= https://tools.ietf.org/rfc/rfc4492#section-4
	*# A client that proposes ECC cipher suites may choose not to include these extensions.
	*# In this case, the server is free to choose any one of the elliptic curves or point formats listed in Section 5.
	*
	*/
	const struct s2n_ecc_preferences *ecc_pref = NULL;
	POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
	POSIX_ENSURE_REF(ecc_pref);
	POSIX_ENSURE_GT(ecc_pref->count, 0);
	if (s2n_ecc_preferences_includes_curve(ecc_pref, TLS_EC_CURVE_SECP_256_R1)) {
	conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp256r1;
	} else {
	/* If P-256 isn't allowed by the current security policy, instead choose
	* the first / most preferred curve.
	*/
	conn->kex_params.server_ecc_evp_params.negotiated_curve = ecc_pref->ecc_curves[0];
	}

Solution:

Option 1

Make no mutating actions until after the client hello callback has been called. This is a conceptually cleaner approach that is significantly easier to reason about.

Option 2

Document the tricky things.

[maddeleine]

As of #4676 this issue is partial complete, however, I didn't touch the SSLv2 client hello logic, as that does reference the config before the client hello callback. So closing this issue is dependent on getting an integration test in place that ensures we don't screw up that logic while refactoring it.