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

openssl: support session resume with TLS 1.3 #3271

Closed
wants to merge 1 commit into from

Conversation

mkauf
Copy link
Contributor

@mkauf mkauf commented Nov 14, 2018

Session resumption information is not available immediately after a TLS 1.3
handshake. The client must wait until the server has sent a session ticket.

Use OpenSSL's "new session" callback to get the session information and put it
into curl's session cache. For TLS 1.3 sessions, this callback will be invoked
after the server has sent a session ticket.

The "new session" callback is invoked only if OpenSSL's session cache is
enabled, so enable it and use the "external storage" mode which lets curl manage
the contents of the session cache.

A pointer to the connection data and the sockindex are now saved as "SSL extra
data" to make them available to the callback.

This approach also works for old SSL/TLS versions and old OpenSSL versions.

Fixes #3202

Session resumption information is not available immediately after a TLS 1.3
handshake. The client must wait until the server has sent a session ticket.

Use OpenSSL's "new session" callback to get the session information and put it
into curl's session cache. For TLS 1.3 sessions, this callback will be invoked
after the server has sent a session ticket.

The "new session" callback is invoked only if OpenSSL's session cache is
enabled, so enable it and use the "external storage" mode which lets curl manage
the contents of the session cache.

A pointer to the connection data and the sockindex are now saved as "SSL extra
data" to make them available to the callback.

This approach also works for old SSL/TLS versions and old OpenSSL versions.

Fixes curl#3202
@mkauf mkauf added the TLS label Nov 14, 2018
@mkauf
Copy link
Contributor Author

mkauf commented Nov 14, 2018

I have tested the TLS 1.3 session resumption with OpenSSL 1.1.1 and BoringSSL (current master). I have also tested that the session resumption for older TLS versions still works with OpenSSL 0.9.7 (after applying #3266) and with LibreSSL 2.8.2.

@bagder
Copy link
Member

bagder commented Nov 14, 2018

That's just great @mkauf!

*/
static int ossl_get_ssl_conn_index(void)
{
static int ssl_ex_data_conn_index = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Will the use of static like this not make this function (possibly) fail when used threaded ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called during the initialization by Curl_ossl_init(), which is called by curl_global_init(). It's documented that curl_global_init() is not thread-safe.

After initialization, the static variable is read-only, so it should work when used threaded.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, thanks!

int sockindex_idx = ossl_get_ssl_sockindex_index();

if(connectdata_idx < 0 || sockindex_idx < 0)
return res;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just return 0; here so that we don't need to check what 'res' contains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK


conn = (struct connectdata*) SSL_get_ex_data(ssl, connectdata_idx);
if(!conn)
return res;
Copy link
Member

Choose a reason for hiding this comment

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

likewise

@mkauf mkauf closed this in 549310e Nov 21, 2018
@mkauf mkauf deleted the openssl_resume branch November 21, 2018 10:20
@mkauf
Copy link
Contributor Author

mkauf commented Nov 22, 2018

@bagder The BoringSSL autobuild https://curl.haxx.se/dev/log.cgi?id=20181122015322-7075 fails:

 ../src/curl: symbol lookup error: ../src/curl: undefined symbol: SSL_get_ex_new_index

This happens when curl is compiled with BoringSSL, but uses libssl.so from OpenSSL at runtime. SSL_get_ex_new_index is a function in BoringSSL, but a macro in OpenSSL.

So I think the autobuild does not use BoringSSL correctly... this can probably be fixed by setting LD_LIBRARY_PATH.

@danielgustafsson
Copy link
Member

I was just about to write an issue for this, only finding this here in the closed PR as I was about to click save. Can you open an issue for this instead to keep us from not forgetting this?

@mkauf
Copy link
Contributor Author

mkauf commented Nov 26, 2018

@danielgustafsson The configurations of the autobuilds are managed by individual developrs, they are not part of curl's source code. So creating an issue doesn't help.

@bagder can you please (try to) fix the BoringSSL autobuild? The autobuild compiles with BoringSSL but uses OpenSSL at runtime.

@bagder
Copy link
Member

bagder commented Nov 26, 2018

Thanks @mkauf, I've now updated my BoringSSL build script to set the LD_LIBRARY_PATH to point out the BoringSSL dir, which should do it...

@c-taylor
Copy link

c-taylor commented Dec 11, 2018

I've just bumped into this; Is there an s_client test for it?

@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants