Allow pass TLS context from third-party TLS provider to application layer #2448

taikulawo · 2025-01-15T08:08:12Z

close #2437

WesleyRosenblum · 2025-02-20T18:12:22Z

Hi WeiChao. Are you still planning to work on this?

taikulawo · 2025-02-21T02:35:36Z

sure, this is initial implementation, Is it right way fix this issue?

camshaft

Overall looks good! We'll need to get the s2n-tls method merged and released before merging this though.

camshaft · 2025-02-21T03:22:16Z

quic/s2n-quic-core/src/crypto/tls.rs

@@ -130,6 +130,8 @@ pub trait Context<Crypto: crate::crypto::CryptoSuite> {
    //# peer's Finished message.
    fn on_handshake_complete(&mut self) -> Result<(), crate::transport::Error>;

+    /// Transfer application context from TLS connection to quic connection
+    fn on_application_context(&mut self, _context: Option<Box<dyn Any + Send + Sync>>) {}


I think we can leave out the default impl here

camshaft · 2025-02-21T03:22:53Z

quic/s2n-quic-tls/src/session.rs

@@ -155,6 +156,10 @@ impl tls::Session for Session {
                    context.on_tls_exporter_ready(self)?;
                    self.handshake_complete = true;
                }
+                // TODO Add new s2n-tls new api, take and put in quic::connection
+                let ctx: Option<Box<dyn Any + Send + Sync>> =


Does this need a 'static as well?

Any already has 'static https://doc.rust-lang.org/std/any/trait.Any.html

taikulawo · 2025-02-21T12:25:32Z

Does you add take_application_context on s2n-tls or me?

camshaft · 2025-03-03T17:33:03Z

Does you add take_application_context on s2n-tls or me?

I won't have time for the next few weeks, unfortunately. If you want to take a stab at it, that would definitely help get this through :)

taikulawo · 2025-03-06T12:08:52Z

Sure, I will do. but let me try #2446 (comment) first :)

taikulawo · 2025-03-19T13:16:32Z

Hi, I want to merge this PR now.
We don't need to add API on s2n-tls. We have write a new TLS provider, replace in with_tls(new_server).

It's good to see s2n-quic has so flexible API. In new provider, we pre-parse client hello before do real TLS handshake.

After this PR get merged, following issue can be closed.

#2446
#2437

taikulawo · 2025-03-22T03:40:55Z

@camshaft Hi, I have fixed CI issue, could you re-run it?

taikulawo · 2025-03-22T12:34:01Z

fixed

camshaft

If you're up for it, I think we also need a test for this feature just to make sure it works right now and to prevent regressions in the future. I think the simplest approach is to extend the null TLS provider to take a context as part of its construction and show that everything gets threaded through all the way. You can add your test in

https://github.com/aws/s2n-quic/blob/main/quic/s2n-quic/src/tests/no_tls.rs

quic/s2n-quic/src/connection/handle.rs

camshaft · 2025-03-25T04:24:30Z

quic/s2n-quic-core/src/crypto/tls/null_tls_context.rs

+use core::{mem::size_of, task::Poll};
+
+#[derive(Debug)]
+pub struct Endpoint(pub UserProvidedTlsContext);


I don't think we need a new endpoint just for this. I think it would be better to have the existing null TLS provider take an Option<Box<dyn Any>>

We need clone it(new_server_session take &mut self), But Clone isn't dyn compatible, Box<dyn Any + Clone> don't works

I rewrite to use generic over T, you can decide accept generic or keep original

camshaft

Getting close! Just a few more changes :)

camshaft · 2025-03-25T15:50:21Z

quic/s2n-quic-core/src/crypto/tls/null.rs


 #[derive(Debug)]
-pub struct Endpoint(());
+pub struct Endpoint<T = ()>(pub Option<T>);


I'd prefer not to make this public but have it be passed in a Endpoint::new function. That will allow us to add new fields, if needed, in the future.

quic/s2n-quic-core/src/crypto/tls/null.rs

camshaft · 2025-03-25T15:54:04Z

quic/s2n-quic-core/src/crypto/tls.rs

@@ -130,6 +130,9 @@ pub trait Context<Crypto: crate::crypto::CryptoSuite> {
    //# peer's Finished message.
    fn on_handshake_complete(&mut self) -> Result<(), crate::transport::Error>;

+    /// Set TLS context and transfer from TLS provider to application layer.
+    #[cfg(feature = "alloc")]
+    fn on_tls_context(&mut self, _context: Option<alloc::boxed::Box<dyn Any + Send>>);


Looking through everything, is there a reason this isn't just Box<dyn Any + send> instead of Option<...>. I think it would be better to only call this method if there is a context available.

quic/s2n-quic-transport/src/connection/connection_trait.rs

camshaft · 2025-03-25T15:59:17Z

quic/s2n-quic/src/connection/handle.rs

+        /// Take TLS context came from TLS layer to application layer.
+        ///
+        /// Calling it a second time will always return None so applications should store it somewhere.
+        ///
+        /// # Example
+        /// It's useful when you implement your own TLS provider. Some HTTP/3 server need more
+        /// information from TLS layer to decide which configuration to apply, So you can
+        ///
+        /// 1. Create own TLS provider, in that, parse TLS manually, set tls context by
+        /// `context.on_tls_context(Box::new(MyContext{}))`
+        /// 2. After `let connection = server.accept().await` returned, take
+        /// context by `connection.take_tls_context()`.
+        ///
+        /// And then do your rest works. For more usage, see `tests/tls_context.rs`


Suggested change

/// Take TLS context came from TLS layer to application layer.

///

/// Calling it a second time will always return None so applications should store it somewhere.

///

/// # Example

/// It's useful when you implement your own TLS provider. Some HTTP/3 server need more

/// information from TLS layer to decide which configuration to apply, So you can

///

/// 1. Create own TLS provider, in that, parse TLS manually, set tls context by

/// `context.on_tls_context(Box::new(MyContext{}))`

/// 2. After `let connection = server.accept().await` returned, take

/// context by `connection.take_tls_context()`.

///

/// And then do your rest works. For more usage, see `tests/tls_context.rs`

/// Takes the context provided by the TLS provider.

///

/// This functionality is useful when you need to pass information from the TLS provider to the

/// application. This could include things like certificate information or application-specific data.

///

/// Calling this function a second time will always return `None` so applications should

/// store the context elsewhere if it is needed in multiple locations.

taikulawo added 2 commits January 15, 2025 16:06

access application context from tls

cf58083

update

3a77111

taikulawo mentioned this pull request Jan 15, 2025

Connection allow get/set context data to control ClientHelloCallback #2437

Closed

boquan-fang requested a review from camshaft January 23, 2025 18:13

camshaft reviewed Feb 21, 2025

View reviewed changes

update

5650df3

taikulawo requested a review from camshaft February 23, 2025 10:34

taikulawo added 3 commits March 19, 2025 17:35

Merge remote-tracking branch 'origin/main' into app-context

eb59fd4

Merge branch 'aws:main' into app-context

4a1459d

1

ce671dd

taikulawo added 4 commits March 22, 2025 11:00

cargo fmt

0ca5e3c

fix no_std

9b6692f

fix clippy

002d559

1

50f3870

taikulawo added 2 commits March 22, 2025 20:27

update

2d3dc77

fix

9f3ae46

taikulawo added 3 commits March 24, 2025 17:09

update

d400a9c

update

775139e

update

35a24dc

camshaft reviewed Mar 24, 2025

View reviewed changes

quic/s2n-quic/src/connection/handle.rs Outdated Show resolved Hide resolved

quic/s2n-quic/src/connection/handle.rs Outdated Show resolved Hide resolved

taikulawo added 2 commits March 25, 2025 11:24

update

5e1a546

update

c8f04b5

1

a8a4852

taikulawo requested a review from camshaft March 25, 2025 03:58

1

be46ff2

camshaft reviewed Mar 25, 2025

View reviewed changes

update

0919636

taikulawo requested a review from camshaft March 25, 2025 05:04

taikulawo added 4 commits March 25, 2025 15:47

remove sync

d9cdeef

update

f496bce

endpoint use generic instead of concrete struct

efeaecc

remove TlsServerSession

13b54ff

taikulawo changed the title ~~Access application context from tls connection~~ Allow pass TLS context from third-party TLS provider to application layer Mar 25, 2025

camshaft reviewed Mar 25, 2025

View reviewed changes

fix

ad1f8e8

taikulawo requested a review from camshaft March 25, 2025 16:15

camshaft approved these changes Mar 25, 2025

View reviewed changes

camshaft merged commit f091f44 into aws:main Mar 25, 2025
116 checks passed

taikulawo deleted the app-context branch April 1, 2025 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow pass TLS context from third-party TLS provider to application layer #2448

Allow pass TLS context from third-party TLS provider to application layer #2448

taikulawo commented Jan 15, 2025 •

edited

Loading

WesleyRosenblum commented Feb 20, 2025

taikulawo commented Feb 21, 2025

camshaft left a comment

camshaft Feb 21, 2025

taikulawo Feb 21, 2025

camshaft Feb 21, 2025

taikulawo Feb 21, 2025 •

edited

Loading

taikulawo commented Feb 21, 2025 •

edited

Loading

camshaft commented Mar 3, 2025

taikulawo commented Mar 6, 2025

taikulawo commented Mar 19, 2025

taikulawo commented Mar 22, 2025

taikulawo commented Mar 22, 2025

camshaft left a comment

camshaft Mar 25, 2025

taikulawo Mar 25, 2025

taikulawo Mar 25, 2025

camshaft left a comment

camshaft Mar 25, 2025

camshaft Mar 25, 2025

camshaft Mar 25, 2025

Allow pass TLS context from third-party TLS provider to application layer #2448

Allow pass TLS context from third-party TLS provider to application layer #2448

Conversation

taikulawo commented Jan 15, 2025 • edited Loading

WesleyRosenblum commented Feb 20, 2025

taikulawo commented Feb 21, 2025

camshaft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taikulawo Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

taikulawo commented Feb 21, 2025 • edited Loading

camshaft commented Mar 3, 2025

taikulawo commented Mar 6, 2025

taikulawo commented Mar 19, 2025

taikulawo commented Mar 22, 2025

taikulawo commented Mar 22, 2025

camshaft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camshaft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taikulawo commented Jan 15, 2025 •

edited

Loading

taikulawo Feb 21, 2025 •

edited

Loading

taikulawo commented Feb 21, 2025 •

edited

Loading