-
Notifications
You must be signed in to change notification settings - Fork 712
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
example(bindings): Add session resumption example #4573
base: main
Are you sure you want to change the base?
Conversation
aa3db54
to
991e533
Compare
# immediately bail if any command fails | ||
set -e | ||
|
||
echo "generating self-signed certificate" |
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.
Instead of generating new certificates, should we move the "certs" folder from the client-hello-resolver
example into the rust-examples
directory, and then have other examples depend on that?
net::TcpStream, | ||
}; | ||
|
||
struct ApplicationContext { |
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.
Nit, in this case, we are the application so this naming feels a bit odd. ConnectionContext
, perhaps?
let key = std::fs::read(key_path).unwrap(); | ||
|
||
let mut config = s2n_tls::config::Builder::new(); | ||
config.set_security_policy(&DEFAULT_TLS13).unwrap(); |
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.
Since we're already returning Box<dyn Error>
from the main function, prefer ?
over unwrap
.
builder.build()? | ||
}; | ||
|
||
for connection_idx in 0..3 { |
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.
Nit: what does x
mean?
let stream = TcpStream::connect("127.0.0.1:9000").await?; | ||
let ip = stream.peer_addr().unwrap().ip(); | ||
|
||
let builder = ModifiedBuilder::new(config.clone(), |conn| { |
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'd vote for a comment explaining this.
this Modified builder allows us to automatically set an available session ticket whenever a new connection is created ..., we do this because ...
@@ -0,0 +1,13 @@ | |||
This example demonstrates the s2n-tls [session resumption](https://aws.github.io/s2n-tls/usage-guide/ch11-resumption.html) feature. First, the client connects to the server and retrieves a session ticket. Then, the client connects to the server again, but uses the retrieved session ticket to resume the previous session. |
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.
Yay for more examples 🕺 !
Two broad points
- The "ApplicationContext" demonstration feels a bit shoe-horned into this example. There is no need in this scenario to lock session tickets to a specific IP address, and generally client implementers should not do that unless they have specific information about distributed session resumption not being supported. I worry that someone is going to look at this and say "oh, session resumption should only happen with the same ip address". I think we should either
- only focus on session resumption in this example
- expand the example to use two servers with different STEKs, and demonstrate the success (ip specific resumption) and failure (attempt distributed resumption) modes.
- If we choose to go the latter route, I'd vote for using turmoil in the example. This allows all of the clients/servers to be simulated in the single process (no need for 3 terminal windows) and also allows the example to be explicitly testable. You can see an example of the turmoil stuff here
Note that if your goal is to just have an example of using the set_application_context
method, a doc comment might be sufficient?
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description of changes:
Adds an example that demonstrates how the application context added in #4563 can be used to associate application data with received session tickets.
Call-outs:
None
Testing:
I ran the example server and client locally.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.