feat: add handshake event #5635

jmayclin · 2025-11-23T01:42:58Z

Goal

Emit a "handshake event", allowing customers to observe multiple pieces of information about the handshake.

Why

Currently customers have to call large numbers of individual "getter" APIs to gather information about the handshake. We expect that with this event structure will eventually be used to aggregate metrics for e.g. emission to CloudWatch.

How

The handshake is emitted to the application through a callback.

Callouts

all of these APIs are currently unstable, and are expected to remain so for the forseeable future. Accordingly everything in this API is a "two way door".
s2n_event_handshake vs s2n_handshake_event? I went with the former because it felt more neatly namespaced.
I wonder whether we want to have the handshake event emitted by value?

This is adding new code in the hotpath, but it has a minimal impact

handshake-secp256r1/s2n-tls
                        time:   [1.1705 ms 1.1711 ms 1.1718 ms]
                        change: [−0.1650% +0.6024% +1.3401%] (p = 0.08 > 0.05)

Testing

Unit tests, and self-talk tests in the rust bindings.

The tests for this cover a large API surface area, which is tedious to cover in C. Therefore I deliberately have most of the testing in Rust.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

maddeleine · 2025-12-08T20:25:43Z

api/unstable/events.h

+/* This is a special value assigned to handshake_start_epoch_ns to indicate that
+ * it has already been sent to the application and should not be sent again.
+ */
+#define HANDSHAKE_EVENT_SENT UINT64_C(1) << 63


Pretty sure you need to include the stdint library in order to use this macro.

maddeleine · 2025-12-08T20:29:56Z

api/unstable/events.h

+ * 
+ * An event is not emitted if the handshake fails.
+ */
+S2N_API extern int s2n_config_set_handshake_event(struct s2n_config *config, s2n_event_on_handshake_cb callback);


Trying to understand, what's the purpose of the subscriber pointer? Is it just the void ctx pointer that we typically have for callbacks? Why wouldn't you just combine these two APIs so that the set_handshake_event takes a subscriber?

maddeleine · 2025-12-08T20:37:14Z

bindings/rust/extended/s2n-tls/src/events.rs

+        Duration::from_nanos(self.0.handshake_end_ns - self.0.handshake_start_ns)
+    }
+
+    /// Handshake time, which just the amount of time synchronously spent in s2n_negotiate.


Suggested change

/// Handshake time, which just the amount of time synchronously spent in s2n_negotiate.

/// Handshake time, which is just the amount of time synchronously spent in s2n_negotiate.

maddeleine · 2025-12-08T20:40:21Z

bindings/rust/extended/s2n-tls/src/events.rs

+                    b"good enough bytes for test",
+                    SystemTime::UNIX_EPOCH,
+                )?
+                .enable_session_tickets(true)?;


Nit: you technically don't need this if you added a session ticket key.

Suggested change

.enable_session_tickets(true)?;

maddeleine · 2025-12-08T20:54:59Z

tls/s2n_client_hello.c

+         * specific error for a null wall clock config just before the client hello callback.
+         * The test's assertions are weakened if this check is moved. 
+         */
+        POSIX_ENSURE(conn->config->wall_clock, S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK);


Why is this change needed?

maddeleine · 2025-12-08T21:10:19Z

tests/unit/s2n_config_test.c

        EXPECT_NOT_NULL(default_config = s2n_fetch_default_config());

        /* s2n_config_new() matches s2n_fetch_default_config() */
+        if (default_config->security_policy != config->security_policy) {


I don't know why you need this code change anymore since you fixed the problem?

maddeleine · 2025-12-08T21:13:26Z

tls/s2n_config.c

 }
+
+int s2n_config_set_subscriber(struct s2n_config *config, void *subscriber)
+{


You're missing null checks on both of these. Purposeful?

CarolYeh910 · 2025-12-06T01:10:36Z

tls/s2n_client_hello.c

+         * specific error for a null wall clock config just before the client hello callback.
+         * The test's assertions are weakened if this check is moved. 
+         */
+        POSIX_ENSURE(conn->config->wall_clock, S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK);


What if conn->config is null? 👀

CarolYeh910 · 2025-12-06T01:27:18Z

utils/s2n_events.c

+{
+    RESULT_ENSURE_REF(conn);
+    RESULT_ENSURE_REF(conn->config);
+    RESULT_ENSURE_REF(conn->config);


Duplicate check: it should be RESULT_ENSURE_REF(event)?

CarolYeh910 · 2025-12-08T18:29:19Z

tests/unit/s2n_events_test.c

+{
+    BEGIN_TEST();
+
+    if (!s2n_is_tls13_fully_supported()) {


Do we expect the event subscriber to be used with TLS 1.2? If so, it might be better to include that in the test?

CarolYeh910 · 2025-12-08T21:37:25Z

utils/s2n_events.c

+
+    event->protocol_version = s2n_connection_get_actual_protocol_version(conn);
+    event->cipher = s2n_connection_get_cipher(conn);
+    s2n_connection_get_key_exchange_group(conn, &event->group);


Maybe add a RESULT_GUARD_POSIX to this call?

Edit: After looking into the RSA rust test, seems like the getter will return INVALID_STATE error in this case. That's why we don't check the error?

feat: add handshake event

1cf8dca

github-actions bot added the s2n-core team label Nov 23, 2025

jmayclin added 10 commits December 3, 2025 22:39

rust format/clippy

95f111c

tls1.3 gate

ba58c5c

clang format and liscense

562968a

cargo fmt all

30e7fd1

adjust connection size for new member

49ede01

add debugging for failing config test

828dfdf

Merge branch 'main' into 2025-11-21-clean-event

b6a81d7

more debuggingg

20ebe4f

fix for apple config issue

a9a83a8

explain failure for future smooth-brained folks like myself

d48ff31

jmayclin marked this pull request as ready for review December 5, 2025 23:18

jmayclin requested review from CarolYeh910 and maddeleine December 5, 2025 23:19

maddeleine reviewed Dec 8, 2025

View reviewed changes

CarolYeh910 reviewed Dec 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add handshake event #5635

feat: add handshake event #5635

Uh oh!

jmayclin commented Nov 23, 2025 •

edited

Loading

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

maddeleine Dec 8, 2025

Uh oh!

CarolYeh910 Dec 6, 2025

Uh oh!

CarolYeh910 Dec 6, 2025

Uh oh!

CarolYeh910 Dec 8, 2025

Uh oh!

CarolYeh910 Dec 8, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// Handshake time, which just the amount of time synchronously spent in s2n_negotiate.
	/// Handshake time, which is just the amount of time synchronously spent in s2n_negotiate.

feat: add handshake event #5635

Are you sure you want to change the base?

feat: add handshake event #5635

Uh oh!

Conversation

jmayclin commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Why

How

Callouts

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolYeh910 Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmayclin commented Nov 23, 2025 •

edited

Loading

CarolYeh910 Dec 8, 2025 •

edited

Loading