Will this have a significant impact on the size of SSLClientSessionCache?

I expect we'd want to keep the same global limit as before, but yeah this is definitely something to watch for. I don't think we've ever put much effort into tuning our session cache for resumption rates and memory usage. We really should.

By far the largest part of the saved session is the certificate, which should be ref-counted and shared between all the sessions from a connection. Though we might get sessions from two different connections in which case they will likely be the same, but we won't notice it...

Yea, the cert refcounting is what I was really interested in, since those seem to be the monsters here.  Though I maybe this could also increase the chance of getting different cert chains for the same host?

If we get multiple sessions from one connection, I believe they'll naturally get ref-counted. If we resume a session, the server doesn't re-assert authentication, and it renews the ticket, that'll still happen.

If we have two connections open at once or the server re-asserts authentication, then there's no guarantee that the two certificates are the same. If they are the same, we won't currently notice.

I'm a little uneasy about introducing any kind of pooling dedup thingy for certificates while we're still using the legacy X.509 code from OpenSSL. Those objects are distressingly mutable.

Once we got off of that, we could certainly build in hooks to dedup things. Preferably in  a way that dedups between net::X509Certificate and whatever BoringSSL internally stores. (We currently hold on to each cert twice because there's two different representations depending on what code is doing what.)

Though exactly how this works will be rather wonky since BoringSSL will only be able to talk to its consumer's certificate library via function pointers and such...


(I am not expecting we'll store more than 2-4 sessions per host. It's probably greater than 1, but I'm happy assuming that people will be getting to the happen HTTP/2 world with fewer connections.)

I feel like this discussion of memory constraints may be orthogonal, especially considering we already de-dup net::X509Certificates.

SSL_SESSIONs do not contain net::X509Certificate. They contain whatever BoringSSL's internal representation of certificates is, which is currently X509*.

Depending on how we get rid of crypto/x509, we may or may not be able to make them actually hold foreign handles, but I'd have to think about if that could work. That an SSL_SESSION is free-standing and not tied to an SSL_CTX makes this somewhat wonky.

But, yes, it's relatively orthogonal. It's relevant in so far as this would put more pressure on the session cache, so our memory vs. performance tradeoff may need additional musing. It needs musing anyway as we really haven't put much effort into tuning this.

rsleevi:  You're right that it's orthogonal.

I'm not suggesting that the behavior change incorporate any improvements in terms of memory use, or be delayed based on having a story on it.  I just want to know what will change in terms of memory when this happens, and this seems the best place to ask.

Fair enough. Of course, we could significantly reduce memory pressure by making some of our caches LowMemory observers, which would probably be good anyways. That might by sufficient breathing room to allow for more experiments like the proposed.

Yup, the session cache is a LowMemory observer as of recently.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00d06ff78fc0705706dd0c7db1170072072e8225

commit 00d06ff78fc0705706dd0c7db1170072072e8225
Author: nharper <nharper@chromium.org>
Date: Fri Jan 20 00:55:56 2017

SSLClientSessionCache: Log number of times Lookup is called per Session.

Session tickets in TLS 1.3 will be single-use, so to be able to resume
multiple sessions to the same host, we will need to store multiple
session tickets per host. By counting how many times Lookup is called
per session in an SSLClientSessionCache, we can tune how many session
tickets per host we need to store for TLS 1.3.

BUG= 631988 

Review-Url: https://codereview.chromium.org/2625883002
Cr-Commit-Position: refs/heads/master@{#444902}

[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/net/socket/ssl_client_socket_impl.h
[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/net/ssl/ssl_client_session_cache.cc
[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/net/ssl/ssl_client_session_cache.h
[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/net/ssl/ssl_client_session_cache_unittest.cc
[modify] https://crrev.com/00d06ff78fc0705706dd0c7db1170072072e8225/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5f8823dd3ba11cc8696bfe926654ed7db1f7ed9

commit f5f8823dd3ba11cc8696bfe926654ed7db1f7ed9
Author: nharper <nharper@chromium.org>
Date: Wed Mar 01 23:52:40 2017

Fix Net.SSLSessionConcurrentLookupCount metric

BUG= 631988 

Review-Url: https://codereview.chromium.org/2723883003
Cr-Commit-Position: refs/heads/master@{#454103}

[modify] https://crrev.com/f5f8823dd3ba11cc8696bfe926654ed7db1f7ed9/net/socket/ssl_client_socket_impl.cc

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/e9c7b1c8ae85e5625e9d24d9e20ccedeaaeb0d0a

commit e9c7b1c8ae85e5625e9d24d9e20ccedeaaeb0d0a
Author: David Benjamin <davidben@google.com>
Date: Thu Sep 28 18:38:17 2017

Add SSL_SESSION_is_single_use.

Querying versions is a bit of a mess between DTLS and TLS and variants
and friends. Add SSL_SESSION_is_single_use which informs the caller
whether the session should be single-use.

Bug:  chromium:631988 
Change-Id: I745d8a5dd5dc52008fe99930d81fed7651b92e4e
Reviewed-on: https://boringssl-review.googlesource.com/20844
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

[modify] https://crrev.com/e9c7b1c8ae85e5625e9d24d9e20ccedeaaeb0d0a/ssl/ssl_session.cc
[modify] https://crrev.com/e9c7b1c8ae85e5625e9d24d9e20ccedeaaeb0d0a/include/openssl/ssl.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6112f4e8695cd1a49c6094144cd80f92775e248

commit e6112f4e8695cd1a49c6094144cd80f92775e248
Author: Steven Valdez <svaldez@chromium.org>
Date: Thu Oct 05 22:20:12 2017

Enforce single-use tickets and remove session counting histogram.

Bug:  631988 
Change-Id: I16e2aaa227e36c3d025c6663b10f6a15e4a32428
Reviewed-on: https://chromium-review.googlesource.com/692946
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506882}
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/net/socket/ssl_client_socket_impl.h
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/net/ssl/ssl_client_session_cache.cc
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/net/ssl/ssl_client_session_cache.h
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/net/ssl/ssl_client_session_cache_unittest.cc
[modify] https://crrev.com/e6112f4e8695cd1a49c6094144cd80f92775e248/tools/metrics/histograms/histograms.xml