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

TLS sessions resumption #1054

Closed
6 tasks done
krizhanovsky opened this issue Aug 11, 2018 · 5 comments · Fixed by #1430
Closed
6 tasks done

TLS sessions resumption #1054

krizhanovsky opened this issue Aug 11, 2018 · 5 comments · Fixed by #1430
Assignees
Labels
crucial enhancement TLS Tempesta TLS module and related issues
Milestone

Comments

@krizhanovsky
Copy link
Contributor

krizhanovsky commented Aug 11, 2018

Scope

Session tickets defined by RFC 5077 must be enabled. TLS cache is considered as a bad practice due to unnecessary server memory usage, so I completely removed TTLS_CACHE_C option.

A new config option with 2 attributes must be introduced:

  1. random or secret - define how a ticket key must be generated - using random generator or the static data for tickets sharing among cluster nodes (a hash function must be used to not to send the secret as is).
  2. the ticket lifetime in seconds

In case of using shared secret, the perfect forward secrecy must be kept and the secret must be regenerated in pseudo-random (to make sure that all cluster nodes have the same state) fashion.

It makes sense to use the same secret as for sticky_secret, so I propose to rename the option to secret and use it for the TLS tickets as well as for the session tickets. (Please update Wiki).

Notice (probably good to add to Wiki) it's attractive to reuse TLS Session ID instead of sticky cookie or vice versa, but Session ID is generated (and controlled) by a client, so we can not rely on it and can not force a client to use our cookie.

AES_128_GCM should be used (bedtls/programs/ssl/ssl_server2.c) for encryption.

Note and fix TODO #1054 comments around the code.

Handshakes limits

It's supposed that normal clients use TLS sessions instead of establish new TLS connections, so as part of the issue, a rate limit for the new handshakes (not only RSA as in #1038) must be introduced. This is the right way to limit TLS.

The limit must be per-vhost in sense of #715.

Code reworking

Use ttls_ticket_write() and ttls_ticket_parse() directly instead of indirect calls off_ticket_write() and f_ticket_parse() from tls->conf. This allows to fix record size estimation in ttls_write_new_session_ticket().

ssl_load_session() must not parse X.509 each time, instead it must cache only the required information from the certificate.

I believe ttls_ticket_context->mutex in ttls_ticket_parse() should experience a serious contention, so the lock must be acquired only to update the key in ssl_ticket_update_keys() - all the other operations with the context are read only.

In general, please review the code and move all computations which can be precomputed to configuration phase, reduce copies, extra memset()s, and other inneficiencies.

I didn't finish code cleanups in /tls, so please fix coding style in ssl_ticket.[ch].

Known problems

It seems we broke TLS handshakes FSM for session resumption: in ttls_write_server_hello() we set next state TTLS_SERVER_CHANGE_CIPHER_SPEC, but the FSM in ttls_handshake_server_hello() doesn't expect the state. Need to fix this and send all the server messages created on the consequent server states in one shot. See comment and the new goto in ttls_handshake_server_hello() in branch ak-bn-mem-profiles.

Testing

As part of the feature development and debugging, please develop appropriate functional test in tls/:

  • Client sends valid session ticket and bypasses the full handshake
  • Client sends invalid session tickets (try different magling), Tempesta falls back to full handshake, but issues a new ticket after that.
  • Client doesn't support session tickets
  • Client supports session tickets and establishes a TLS connection, next it reestablishes an abbreviated handshake
  • Scenario from Virtual Host Confusion : establish TLS session with one vhost and then use the same ticket to establish session with another vhost (the test should use vhost/tls configuration which (1) doesn't allow the behavior and (2) does allow this). This should be checked against two different vhosts and a vhost and default. Please also document the recommended Tempesta FW configuration to not to allow this type of attacks in https://github.com/tempesta-tech/tempesta/wiki/Tempesta-TLS . While the paper says that SPDY obeyed more relaxed rules on connection reuse, RFC 7540 9.1 seems provide the same requirements as for HTTPS/1.
  • tests for tls_connection_rate, tls_connection_burst, and tls_uncomplete_rate limits. Probably it makes sense to move these 3 tests into a separate test which will be developed further in Functional test for Frang #673.

Besides the functional test, please make sure that a regular browser (at least Firefox and Chrome) normally uses a ticket to restore a session.

Performance evaluation

Need to benchmark TLS sessions resumption on HTTP/1.1 and HTTP/2 (#309) cases and compare with OpenSSL's TLS 1.2 and 1.3 in zero rtt handshake coupled with either HAProxy or Nginx (one of them is enough). Analyze perf report and create issue for 0.7 if some anomalies are found and/or Tempesta shows worse results than 30% better than OpenSSL TLS 1.2 with Nginx for both HTTP/1.1 and HTTP/2 (we're basically fine if TLS 1.3 0-rtt bets TLS 1.2 session resumption - it's left for #1031).

tls-perf extension tempesta-tech/tls-perf#3 is required to perform the measurements.

h2load seems a good tools. Check that we're not worse than H2O results.

@krizhanovsky krizhanovsky added this to the 1.0 Beta milestone Aug 11, 2018
@krizhanovsky krizhanovsky modified the milestones: 1.0 Beta, 0.7 HTTP/2 Sep 4, 2018
krizhanovsky added a commit that referenced this issue Oct 17, 2018
@krizhanovsky krizhanovsky mentioned this issue Dec 3, 2018
7 tasks
krizhanovsky added a commit that referenced this issue Dec 25, 2018
krizhanovsky added a commit that referenced this issue Dec 25, 2018
krizhanovsky added a commit that referenced this issue Dec 31, 2018
@krizhanovsky krizhanovsky assigned i-rinat and unassigned vankoven and i-rinat Jun 8, 2019
@krizhanovsky krizhanovsky assigned vankoven and unassigned i-rinat Oct 22, 2019
@krizhanovsky krizhanovsky added the TLS Tempesta TLS module and related issues label Apr 27, 2020
@krizhanovsky
Copy link
Contributor Author

Update as discussed in the chat: there should be to 2 handshakes limits - the 1st one for successful handshakes and the 2nd for protocol errors on handshakes. Due to TLS tickets asymmetry, it makes sense for a bot just to send us garbage instead of valid crypto data, so the server struggles on meaningless crypto operations.

@vankoven
Copy link
Contributor

vankoven commented Aug 7, 2020

The #1430 fixed the issue partly, there are still a few todos:

  • handshake limits
  • NewTicketMessage is sent separately from ChangeCipherSpec and Finished. The same apply to ServerHello and ChangeCipherSpec and Finished durin abbreviated handshake. Need to send them in one tcp frame.
  • fast sertificate parsing
  • funxtional tests

@vankoven vankoven reopened this Aug 7, 2020
@vankoven
Copy link
Contributor

vankoven commented Aug 7, 2020

Probably need to add client IP address into ticket. At least OpenSSL allows to serialise session in one application and deserialise it in an another application. Attacker may use that to establish TLS session once and then propagate the session to all the bots and resume the session there without any need to pass through full handshake.

@krizhanovsky
Copy link
Contributor Author

Clients are likely changing their IP addresses thanks to DHCP and maybe mobile cells....

@vankoven
Copy link
Contributor

closed by #1458

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crucial enhancement TLS Tempesta TLS module and related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants