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.

api/unstable/events.h

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?

In rust world, we expect to have

Struct MetricAggregator; impl MetricAggregator { fn emit_some_handshake_metrics(...); }

The void ctx pointer is what will actually point to the MetricAggregator struct.

Why wouldn't you just combine these two APIs so that the set_handshake_event takes a subscriber?

Because that would make it more difficult to add events in the future. My current expectation for how we'd add a new event is to just add a new s2n_config_set_foo_event(...) function. If we passed the subscriber in along with the callback, I think it would be more difficult to add new events in the future.

Curious why adding new events may affect the subscriber/MetricAggregator 🤔 Could you elaborate why it makes adding events difficult in the future?

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

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?

This change is needed because now we are access in the config before the client hello callback. This is because the monotonic clock is stored on the config, and we need the clock as soon as we hit s2n_negotiate.

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?

My thought is that someone might make the same mistake again in the future, and this would be really helpful to help them debug it.

E.g. something like this

s2n-tls/.github/workflows/ci_rust.yml

Lines 138 to 144 in 60cfca0

- name: Test external build

# if this test is failing, make sure that api headers are appropriately

# included. For a symbol to be visible in a shared lib, the

# __attribute__((visibility("default"))) label must be on a declaration

# in the same unit of compilation as the definition. Generally this just

# means that if the linker can't resolve foo_method in tls/foo.c, you

# forgot to include api/unstable/foo.h in tls/foo.c

tls/s2n_config.c

tls/s2n_client_hello.c

utils/s2n_events.c

tests/unit/s2n_events_test.c

utils/s2n_events.c

Co-authored-by: maddeleine <[email protected]>

CarolYeh910 · 2025-12-08T23:13:59Z

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

+            cipher: "ECDHE-RSA-AES128-GCM-SHA256",
+            // TODO: This is unexpected, and should be None, because TLS 1.2 session
+            // resumption does not include any ECDHE.
+            group: Some("secp256r1"),


Curious why the group is not overridden. Maybe session resumption doesn't change the original negotiated group? So we get the same value as the full handshake

CarolYeh910 · 2025-12-11T23:51:02Z

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);


Curious why adding new events may affect the subscriber/MetricAggregator 🤔 Could you elaborate why it makes adding events difficult in the future?

CarolYeh910 · 2025-12-11T23:57:11Z

tests/unit/s2n_config_test.c

            DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),
                    s2n_connection_ptr_free);
            EXPECT_NOT_NULL(server_conn);
-            server_conn->config = NULL;


This test doesn't call s2n_negotiate(), so technically we can set server_conn->config to NULL. However, it will hit S2N_ERR_NULL with the new changes, so 🤷‍♀️

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

tls/s2n_client_hello.c Show resolved Hide resolved

utils/s2n_events.c Outdated Show resolved Hide resolved

tests/unit/s2n_events_test.c Outdated Show resolved Hide resolved

utils/s2n_events.c Show resolved Hide resolved

jmayclin and others added 3 commits December 11, 2025 10:50

Apply suggestions from code review

d24e00f

Co-authored-by: maddeleine <[email protected]>

address pr feedback

4c701c9

address pr feedback

4c8aefb

jmayclin requested review from CarolYeh910 and maddeleine December 11, 2025 19:03

CarolYeh910 reviewed Dec 11, 2025

View reviewed changes

	- name: Test external build
	# if this test is failing, make sure that api headers are appropriately
	# included. For a symbol to be visible in a shared lib, the
	# __attribute__((visibility("default"))) label must be on a declaration
	# in the same unit of compilation as the definition. Generally this just
	# means that if the linker can't resolve foo_method in tls/foo.c, you
	# forgot to include api/unstable/foo.h in tls/foo.c

feat: add handshake event #5635

Are you sure you want to change the base?

feat: add handshake event #5635

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

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