tls: Implement session resumption for client connections#4790
tls: Implement session resumption for client connections#4790julia-stripe wants to merge 1 commit into
Conversation
|
@PiotrSikora / @htuch Could you take a pass and comment on approach? Thank you |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Modulo the BoringSSL details, which I will defer to @PiotrSikora for a review on, this looks good. @julia-stripe can you add some tests?
| ClientContextImpl* client_context_impl = dynamic_cast<ClientContextImpl*>(context_impl); | ||
| int len = i2d_SSL_SESSION(session, NULL); | ||
| if (len <= ENVOY_MAX_SESSION_SIZE) { | ||
| client_context_impl->session_.resize(len); |
There was a problem hiding this comment.
Do we only have one resumption per client context? What if the same context is used to speak to multiple endpoints, each of which who negotiates their own resumption ticket?
| #error Envoy requires BoringSSL | ||
| #endif | ||
|
|
||
| #define ENVOY_MAX_SESSION_SIZE 4096 |
There was a problem hiding this comment.
make this a constexpr int inside unnamed namespace in .cc file?
|
|
||
| if (allow_session_resumption_ && !session_.empty()) { | ||
| const unsigned char* temp = session_.data(); | ||
| SSL_SESSION *session = d2i_SSL_SESSION(NULL, &temp, session_.size()); |
There was a problem hiding this comment.
Isn't session leaked? d2i_SSL_SESSION returns with ref count 1, SSL_set_session will increment 1 and decrement once it is no longer used. You need to use bssl::UniquePtr here.
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
Hi Julia,
thanks for working on this! I've started adding comments, but since your code driven by NGINX implementation, which uses shared memory for sharing session across workers, it ended up being quite unproductive, so I've pushed a version, which I wrote a while ago, but failed to add tests to it. See #4791.
|
Thanks so much @PiotrSikora! Closing in favour of #4791. |
6 participants
Description:
This adds a new
allow_session_resumptionthat makes the Envoy client attempt to use session tickets to resume TLS connections.This is in a pretty early state right now (this PR is basically just copying some code out of nginx's session resumption), but it does resume sessions in my testing and I'd love some feedback on the approach.
If this approach seems reasonable to folks I'll work on adding tests / docs.
Risk Level: medium
Testing: TODO
Docs Changes: TODO
Release Notes: TODO
Fixes #3817