Skip to content

Commit 1c98447

Browse files
maddeleinemaddeleine
authored
fix: refactor negotiate loop to fix issue with async callback (#5641)
1 parent 2a8313f commit 1c98447

File tree

4 files changed

+293
-113
lines changed

4 files changed

+293
-113
lines changed

‎bindings/rust/standard/integration/src/mtls.rs‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -425,13 +425,8 @@ where
425425
pair
426426
}
427427

428-
// As of 2025-11-24: s2n as client (TLS 1.2, 1.3) and s2n as
429-
// server (TLS 1.3) hang due to a multi-message async cert validation bug.
430-
// s2n incorrectly clears queued handshake messages, causing
431-
// poll_negotiate() to spin forever. Remove #[ignore] once fixed.
432428
// s2n client with async callback, rustls server
433429
#[test]
434-
#[ignore = "Hangs due to multi-message bug in async cert validation"]
435430
fn s2n_client_async_callback() {
436431
// TLS 1.2
437432
let (client, handle, rx) = {
@@ -463,12 +458,9 @@ fn s2n_client_async_callback() {
463458
}
464459

465460
// rustls client, s2n server with async callback
466-
// Rustls TLS 1.2 clients do not send multiple handshake messages in a
467-
// single record, so s2n never hits the multi-message async-callback
468-
// bug that appears in TLS 1.3. These tests are split by protocol
469-
// version until the multi-message bug is fixed.
470461
#[test]
471-
fn s2n_server_async_callback_tls12() {
462+
fn s2n_server_async_callback() {
463+
// TLS 1.2
472464
let client = rustls_mtls_client(SigType::Rsa2048, &rustls::version::TLS12);
473465
let (server, handle, rx) = {
474466
let builder = s2n_mtls_base_builder(SigType::Rsa2048);
@@ -478,11 +470,8 @@ fn s2n_server_async_callback_tls12() {
478470
};
479471
let _pair =
480472
test_async_server_callback::<RustlsConnection, S2NConnection>(&client, &server, handle, rx);
481-
}
482473

483-
#[test]
484-
#[ignore = "Hangs due to multi-message bug in async cert validation"]
485-
fn s2n_server_async_callback_tls13() {
474+
// TLS 1.3
486475
crate::capability_check::required_capability(
487476
&[crate::capability_check::Capability::Tls13],
488477
|| {

‎tests/unit/s2n_multi_message_test.c‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
#include "api/s2n.h"
17+
#include "s2n_test.h"
18+
#include "testlib/s2n_testlib.h"
19+
#include "tls/s2n_tls.h"
20+
/* Just to get access to advance_message */
21+
#include "tls/s2n_handshake_io.c"
22+
23+
struct test_data {
24+
uint8_t message_type;
25+
int (*handler)(struct s2n_connection *conn);
26+
};
27+
28+
struct s2n_cert_cb_ctx {
29+
struct s2n_cert_validation_info *info;
30+
};
31+
32+
static int s2n_test_cert_validation_cb(struct s2n_connection *conn, struct s2n_cert_validation_info *info, void *ctx)
33+
{
34+
struct s2n_cert_cb_ctx *data = (struct s2n_cert_cb_ctx *) ctx;
35+
36+
data->info = info;
37+
38+
/* Returning success since we will accept asynchronously */
39+
return S2N_SUCCESS;
40+
}
41+
42+
struct s2n_sig_cb_ctx {
43+
struct s2n_async_offload_op *op;
44+
};
45+
46+
static int s2n_test_signature_verification_cb(struct s2n_connection *conn, struct s2n_async_offload_op *op, void *ctx)
47+
{
48+
struct s2n_sig_cb_ctx *data = (struct s2n_sig_cb_ctx *) ctx;
49+
50+
data->op = op;
51+
52+
/* Returning success since we will perform the operation asynchronously */
53+
return S2N_SUCCESS;
54+
}
55+
56+
int main(int argc, char **argv)
57+
{
58+
BEGIN_TEST();
59+
60+
if (s2n_is_tls13_fully_supported()) {
61+
DEFER_CLEANUP(struct s2n_cert_chain_and_key *chain_and_key = NULL, s2n_cert_chain_and_key_ptr_free);
62+
EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&chain_and_key,
63+
S2N_DEFAULT_TEST_CERT_CHAIN, S2N_DEFAULT_TEST_PRIVATE_KEY));
64+
65+
DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free);
66+
EXPECT_NOT_NULL(server_config);
67+
/* Policy that will negotiate TLS1.3 */
68+
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, "20251014"));
69+
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key));
70+
EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL));
71+
EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED));
72+
73+
DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
74+
EXPECT_NOT_NULL(client_config);
75+
/* Policy that will negotiate TLS1.3 */
76+
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20251014"));
77+
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key));
78+
EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL));
79+
EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_REQUIRED));
80+
81+
/* This test checks that our async callbacks which interrupt the reading of handshake messages do not
82+
* affect the server's processing of multiple handshake messages in one record.
83+
* This case requires special testing because these async callbacks will exit the s2n_negotiate
84+
* loop with handshake data left unread and we want to ensure that they re-enter and process
85+
* the leftover data successfully.
86+
*/
87+
{
88+
DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
89+
EXPECT_NOT_NULL(client_conn);
90+
EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost"));
91+
EXPECT_SUCCESS(s2n_connection_set_config(client_conn, client_config));
92+
EXPECT_SUCCESS(s2n_connection_set_blinding(client_conn, S2N_SELF_SERVICE_BLINDING));
93+
94+
DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
95+
EXPECT_NOT_NULL(server_conn);
96+
97+
/* Add a cert validation callback. In this case we will store the cert validation info
98+
* when the callback triggers and accept the cert outside of the callback. */
99+
struct s2n_cert_cb_ctx cert_cb_ctx = { 0 };
100+
EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, s2n_test_cert_validation_cb, &cert_cb_ctx));
101+
102+
/* Add a signature verify callback */
103+
struct s2n_sig_cb_ctx sig_cb_ctx = { 0 };
104+
EXPECT_SUCCESS(s2n_config_set_async_offload_callback(server_config, S2N_ASYNC_OFFLOAD_PKEY_VERIFY,
105+
s2n_test_signature_verification_cb, &sig_cb_ctx));
106+
107+
EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING));
108+
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, server_config));
109+
110+
DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
111+
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
112+
EXPECT_SUCCESS(s2n_connections_set_io_pair(client_conn, server_conn, &io_pair));
113+
114+
/* Stop client before sending its last flight of messages */
115+
EXPECT_OK(s2n_negotiate_test_server_and_client_until_message(server_conn, client_conn, CLIENT_CERT));
116+
117+
struct test_data message_arr[] = {
118+
{ .message_type = TLS_CERTIFICATE, .handler = s2n_client_cert_send },
119+
{ .message_type = TLS_CERT_VERIFY, .handler = s2n_tls13_cert_verify_send },
120+
{ .message_type = TLS_FINISHED, .handler = s2n_tls13_client_finished_send }
121+
};
122+
123+
/* We have to manually call the state machine handler and update the handshake hashes
124+
* because s2n-tls is not designed to send multiple handshake messages per record. */
125+
for (size_t i = 0; i < s2n_array_len(message_arr); i++) {
126+
/* Write handshake message */
127+
EXPECT_SUCCESS(s2n_stuffer_write_uint8(&client_conn->handshake.io, message_arr[i].message_type));
128+
struct s2n_stuffer_reservation reservation = { 0 };
129+
EXPECT_SUCCESS(s2n_stuffer_reserve_uint24(&client_conn->handshake.io, &reservation));
130+
EXPECT_SUCCESS(message_arr[i].handler(client_conn));
131+
EXPECT_SUCCESS(s2n_stuffer_write_vector_size(&reservation));
132+
EXPECT_SUCCESS(s2n_advance_message(client_conn));
133+
134+
/* Update transcript hash with handshake message */
135+
struct s2n_blob data = { 0 };
136+
uint32_t len = s2n_stuffer_data_available(&client_conn->handshake.io);
137+
uint8_t *bytes = s2n_stuffer_raw_read(&client_conn->handshake.io, len);
138+
EXPECT_NOT_NULL(bytes);
139+
EXPECT_SUCCESS(s2n_blob_init(&data, bytes, len));
140+
EXPECT_SUCCESS(s2n_conn_update_handshake_hashes(client_conn, &data));
141+
EXPECT_SUCCESS(s2n_stuffer_skip_read(&client_conn->handshake.io,
142+
s2n_stuffer_data_available(&client_conn->handshake.io)));
143+
}
144+
145+
EXPECT_SUCCESS(s2n_stuffer_reread(&client_conn->handshake.io));
146+
147+
/* Writes all messages in the handshake buffer to the same record */
148+
s2n_blocked_status blocked = S2N_NOT_BLOCKED;
149+
EXPECT_OK(s2n_handshake_message_send(client_conn, TLS_HANDSHAKE, &blocked));
150+
151+
/* Handshake will block until the server accepts the client cert */
152+
for (int i = 0; i < 3; i++) {
153+
EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn),
154+
S2N_ERR_ASYNC_BLOCKED);
155+
}
156+
EXPECT_SUCCESS(s2n_cert_validation_accept(cert_cb_ctx.info));
157+
158+
/* Handshake will block until the server verifies client signature */
159+
for (int i = 0; i < 3; i++) {
160+
EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn),
161+
S2N_ERR_ASYNC_BLOCKED);
162+
}
163+
164+
EXPECT_SUCCESS(s2n_async_offload_op_perform(sig_cb_ctx.op));
165+
166+
/* Handshake completes successfully after server accepts client cert */
167+
EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn));
168+
}
169+
}
170+
171+
END_TEST();
172+
}

‎tls/s2n_connection.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,9 +1326,6 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
13261326
return S2N_RESULT_OK;
13271327
}
13281328

1329-
/* Ensure that conn->in doesn't contain any leftover invalid or unauthenticated data. */
1330-
RESULT_GUARD_POSIX(s2n_stuffer_wipe(&(*conn)->in));
1331-
13321329
int error_code = s2n_errno;
13331330
int error_type = s2n_error_get_type(error_code);
13341331

@@ -1343,6 +1340,9 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
13431340
break;
13441341
}
13451342

1343+
/* Ensure that conn->in doesn't contain any leftover invalid or unauthenticated data. */
1344+
RESULT_GUARD_POSIX(s2n_stuffer_wipe(&(*conn)->in));
1345+
13461346
switch (error_code) {
13471347
/* Don't invoke blinding on some of the common errors.
13481348
*

0 commit comments

Comments
 (0)