Skip to content

Conversation

@kaukabrizvi
Copy link
Contributor

Goal

Add an mTLS “stress” integration test that exercises the full async path: async certificate validation + async pkey verify offload, using rustls as client and s2n-tls as server (TLS 1.3).

Why

This combination adds additional complexity to the mTLS handshake path and ensures that async callback ordering, offload behavior, and multi-message handshake handling remain correct. In addition to the async cert verify callback tests introduced in #5638, applications may configure mTLS with both asynchronous certificate validation and asynchronous public-key verification, so it’s valuable to have integration coverage for that full path.

How

  • Adds a new integration test that drives a full TLS 1.3 mTLS handshake using rustls → s2n-tls.
  • Registers an async certificate-validation callback and defers acceptance until the test supplies the decision.
  • Registers an async pkey-verify offload callback (s2n_config_set_async_offload_callback) so that CertificateVerify signature checking is suspended and later resumed.
  • Steps the handshake in phases, receiving the async operations, performing them, and ensuring the connection resumes correctly.

Callouts

  • Introduces a small restructuring of the integration/mtls tests: the stress test is in mtls/async_verify_and_offload.rs, while mod.rs holds the shared helpers and the basic, sync, and async cert-verify tests. This keeps the stress test isolated while still reusing common helpers. Future public-key offload coverage (across client/server, TLS 1.2/1.3, sync/async) can also live in this folder and follow the same structure.
  • The rustls→s2n TLS 1.3 variant is temporarily #[ignore] due to an existing s2n-tls bug where multi-message async cert validation clears queued handshake messages. The #[ignore] should be removed once fix: refactor negotiate loop to fix issue with async callback #5641 is merged.

Testing

I validated the test as follows:

  • Rebased on the fixes from fix: refactor negotiate loop to fix issue with async callback #5641, then verified (via debug prints) that the TLS 1.3 handshake progresses correctly and that both async callbacks fire at the expected points, so the test passes.
  • Confirmed the expected failure mode without those fixes: the handshake hangs, and only the cert-verify callback counter increments, while the pkey-offload callback is never caleld, because the server never receives the CertificateVerify message due to the bug.

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

@github-actions github-actions bot added the s2n-core team label Dec 5, 2025
@kaukabrizvi kaukabrizvi marked this pull request as ready for review December 5, 2025 20:22
@kaukabrizvi kaukabrizvi changed the title test(integration): add mTLS 'stress' test test(integration): add asyn cert verify and offload 'stress' test Dec 5, 2025
@kaukabrizvi kaukabrizvi changed the title test(integration): add asyn cert verify and offload 'stress' test test(integration): add async cert verify and offload 'stress' test Dec 5, 2025
@kaukabrizvi kaukabrizvi requested a review from dougch as a code owner December 8, 2025 21:25
@kaukabrizvi kaukabrizvi force-pushed the async_verify_and_offload_test branch from f3e37e4 to 9095a05 Compare December 8, 2025 21:36
@kaukabrizvi kaukabrizvi requested review from maddeleine and removed request for dougch December 8, 2025 21:36
@kaukabrizvi kaukabrizvi requested a review from jmayclin December 8, 2025 22:15
@kaukabrizvi kaukabrizvi requested a review from jmayclin December 11, 2025 21:34
invoked: Arc::clone(&invoked),
sender: tx,
});
let ctx_ptr = Box::into_raw(ctx) as *mut c_void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is technically leaking memory.

Not a huge deal because it's just a test, but that's a slippery slope, and it's generally a good idea to get in the habit of writing leak-free code 😆

Option 1: use Box::from_raw in the callback. This will take back ownership from the pointer, and it will be freed after the callback has been used. This is probably the neatest option. It does mean that the callback can only be invoked once, but I think that's fine for the test.

Option 2: "store" the context for the duration of the program.

fn my_function() {
    // first, put the object on the heap (not technically necessary, but easier)
    let thing_ffi_needs = Box::new(5);
    // DANGER: The application is responsible for ensuring that ptr_for_ffi is never 
    // references after `thing_ffi_needs` goes out of scope.
    let ptr_for_ffi = &this_ffi_needs as *mut c_void;

    // pass ptr_for_ffi into a function, or whatever

    // at the end of the function, thing_ffi_needs goes out of scope, and the memory is cleaned up.
}

Copy link
Contributor Author

@kaukabrizvi kaukabrizvi Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right, this is leaking memory (noted in the comment below). I will change this to keep it leak-free by reclaiming ownership with Box::from_raw in the callback, and apply the same change in the mTLS test as well:

let ctx_ptr = Box::into_raw(ctx) as *mut c_void;
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants