Note, similarly to @julia-stripe's PR, this assumes a single session store per cluster, not per endpoint.

@PiotrSikora can you talk a bit about the rationalization for that assumption?

The assumption is wrong most of the time (i.e. we should save session per endpoint, not per cluster), which is one of the reasons why this was sitting in my local tree until now.

However, the per cluster store works perfectly fine for (a) single-endpoint clusters, (b) deployments using shared cache and/or session tickets, where the same session can be resumed across all endpoints, so it's a stepping stone in the right direction, and per endpoint store can be added a bit later.

@PiotrSikora can you clarify what the next stones are towards per-endpoint stores? I think @julia-stripe and I were under the impression there was a single ClientContextImpl per endpoint, rather than per-cluster, which it sounds like isn't true. As you point out, this PR as stands won't work well for multi-endpoint clusters without shared session ticket keys, which is our big use-case.

@bobby-stripe it's either:

1. Storing sessions in the endpoint object, so that session's lifetime is the same as endpoint's.
2. Adding ability to configure "session cache key", so that sessions stored in ClientContextImpl can be retrieved by a specific key(s) (e.g. %UPSTREAM_CLUSTER% - which is effectively what we have right now, %UPSTREAM_HOST% or %REQ(:AUTHORITY)%).

I'm leaning towards the second option, since it's much more flexible, but I didn't have time to work on this yet.

cc @julia-stripe @ggreenway

@PiotrSikora how do you want to move forward with this? Merge this PR and then do 1/2 above? I'm guessing you get most of your wins by just doing (1) above.

I'd leaning towards (2), since it's much more flexible solution.

But yeah, let's definitely merge this PR as-is (feature-wise).

Can you open up a ticket for the per-endpoint continuation to track?

Would it make sense (in the future) to have a corresponding server-side setting for how many session tickets a TLS 1.3 server will issue at a time? the RFC says:

Servers that issue tickets SHOULD offer at least as many tickets as the number of connections that a client might use; for example, a web browser using HTTP/1.1 [RFC7230] might open six connections to a server.

Yes, but it's currently hardcoded to static const int kNumTickets = 2 in BoringSSL (see: https://boringssl.googlesource.com/boringssl/+/c0c9001440db8121bdc1ff1307b3a9aedf26fcd8/ssl/tls13_server.cc#165). cc @davidben

Q: Should this feature be disabled or enabled by default?

Are there any security implications from enabling by default? E.g. are we materially increasing the amount of code that might be subject to compromise in BoringSSL etc. I have zero clue on this, but my inclination would be if there was a tradeoff to sacrifice performance (i.e. the resumption) for improved default security posture.

There is a case of a "privacy leak" (passive observer being able to correlate connections from the same user by looking at the session that's being resumed, which is sent unencrypted in ClientHello) in TLS versions older than 1.3, but that's mostly a threat to end-users (so also a single-user client-side proxy) and not middle/edge proxies and/or service mesh, so I don't think that it justifies having this off by default in Envoy.

Note: TLSv1.3 sends single-use sessions, so the default of 1 is probably too small. Perhaps we could make it vary by default, i.e. 1 if tls_maximum_protocol_version is smaller than TLSv1.3, and 4(?) otherwise.

I think the common case is service mesh and middle/edge proxies, so we should optimize for that. It's probably not great to be optimizing for TLS 1.3 quiet yet, I don't think this is universal by far.

Sorry, I somehow missed this comment earlier.

What I meant regarding TLS v1.3 is basically:

if upstream_tls_context.tls_params.tls_maximum_protocol_version == TLSv1_3:
   max_session_keys = 4;
else
   max_session_keys = 1;

But I'm fine leaving the default at 1 for the time being, and we can revisit it later, when we enable TLSv1.3 by default.

Make this mutable doesn't looks correct, should we just make newSsl non const? SslSocket holds non-const shared pointer so it should be OK.

Fixed, thanks!

Should we check or ASSERT errors for these BoringSSL calls?

Not for those. SSL_CTX_set_session_cache_mode() cannot fail and returns previously configured mode, SSL_CTX_sess_set_new_cb() cannot fail and returns void.

Can you open up a ticket for the per-endpoint continuation to track?

Can you add some comments here on why picking the front of the queue is the right thing to do? I.e. why not the third item in the Q?

Done.

I think we are safe for multi cert work, given that the client contexts will continue to have a single cert, but could you comment here on whether in the future, if we support multiple client certs, whether anything needs to change?

I don't think that we'll ever support multiple client certificates that can affect sessions, since client certificates are not revalidated during session resumption by the server.

In any case, this would be covered by #5073.

Sad that we need to take a writer mutex on a data path operation here. I assume that we're not that concerned because we expect connections to be relatively long lived. Is there a case for being able to take a reader mutex on the common path?

Well, we don't really need to take it. The alternative is to use per-worker lock-less session cache, but that would result in bigger memory usage and much lower hit rate, so I think that using shared cache is a good trade-off.

In theory, we only need write/write locks when we store single-use session keys (TLS 1.3), so I've added an optimization to use read/write locks for the other cases.

Thanks!

Thanks for the explanation and switching to reader mutex. I think this is fine for scalability, since this only impacts initial connection latency, not per request and also we'll eventually move to a shared connection pool model for scalability.

Do we need this? Can we use the simulated time_system above?

No, it just shows how old the code really is. Thanks!

Nit: slight preference for explicit capture list here; I don't mind & in tests if it really makes them a lot less verbose (as opposed to regular code, where we should avoid wildcard), but here it doesn't help much.

Done. I updated those functions, as well as other lambdas in this file, but the list is a bit ridiculous, to be honest...

Nit: this should be connect_second_time, as it's still a variable, albeit a lambda.

It's also a function, and connectSecondTime() looks nicer than connect_second_time(), IMHO.

But I changed it anyway.

Do you think we could InSequence these?

Done, but due to the ordering of events that depends on the version of the TLS protocol and whether or not the session was successfully resumed, this resulted in quite a lot of extra code.

See: 4aeefe4

Nit: prefer a // one liner explaining in plain text what all these tests do.

Done.

Actually, I agree it's a bit ridiculous to have explicit capture lists in tests when it gets this long. My rule of thumb is in production code, make it explicit (avoid unintended mistakes that can creep in) and in test code, make the capture list explicit when it's short, otherwise you can wildcard it. I had though in the example I pointed at it would only be two items long, but I guess that's not the case across this file.

Should I revert it or leave it as-is, then? I'm fine with either.

Yeah, if you could revert back the ones that are actually ridiculous (and leave the shorter ones as is) that would be great.

Done.