Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access application context from tls connection #2448

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

taikulawo
Copy link

try fix #2437

@WesleyRosenblum
Copy link
Contributor

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

@taikulawo
Copy link
Author

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

Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

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

@@ -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>>) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leave out the default impl here

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -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>> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a 'static as well?

Copy link
Author

@taikulawo taikulawo Feb 21, 2025

Choose a reason for hiding this comment

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

@taikulawo
Copy link
Author

taikulawo commented Feb 21, 2025

Does you add take_application_context on s2n-tls or me?

@taikulawo taikulawo requested a review from camshaft February 23, 2025 10:34
@camshaft
Copy link
Contributor

camshaft commented Mar 3, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection allow get/set context data to control ClientHelloCallback
3 participants