-
Notifications
You must be signed in to change notification settings - Fork 124
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
base: main
Are you sure you want to change the base?
Conversation
Hi WeiChao. Are you still planning to work on this? |
sure, this is initial implementation, Is it right way fix this issue? |
There was a problem hiding this 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.
quic/s2n-quic-core/src/crypto/tls.rs
Outdated
@@ -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>>) {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>> = |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any already has 'static https://doc.rust-lang.org/std/any/trait.Any.html
Does you add |
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 :) |
try fix #2437