Stop caching DER-encoded certificates unnecessarily

Stop caching DER-encoded certificates unnecessarily

What was intended as a CPU performance short-circuit
in SSLClientSocket for Open/BoringSSL implementations
ended up introducing rather unfortunate and
significant memory overhead, thus proving yet again
that premature optimization is the root of all evil.

While cleaning the unnecessary DER-conversion/caching
up, a further optimization to reduce the overhead
of adding certificate errors was pointed out, allowing
us to avoid the overhead of storing another copy of the
DER-encoded certificate. As //remoting-in-NaCl can now
use BoringSSL directly, this unwinds
http://crrev.com/93153 and uses the cached certificate
directly.

BUG=642082
R=davidben@chromium.org, sergeyu@chromium.org

Committed: https://crrev.com/74e99744fe3d88a5b4a759818fa813cd2d091bd7
Cr-Commit-Position: refs/heads/master@{#418352}

[Ryan Sleevi, 2016-08-31 03:03:55]

David, Sergey: For your review

Dimitry: When you pointed out the IsAllowedBadCert() DER-encoding, I realized I
could restore the pre-2011 version of the function. Previously, when you added a
cert exception (e.g. clicked through an error at badssl.com), then every socket
for that site would store an additional encoded copy of the certificate (as part
of the SSLConfig). Now, we only store the X509Certificate, which as mentioned on
the bug, is a shared cache under the hood. This should reduce the overhead of a
'bad' certificate by ~1K/socket. Though this only saves ~6-8K in the worst
condition (due to 6-8 socket/host limit), it was low-hanging fruit.

[Ryan Sleevi, 2016-08-31 03:05:05]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 03:05:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 03:24:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 03:24:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Sergey Ulanov, 2016-08-31 18:46:44]

lgtm

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
remoting/protocol/ssl_hmac_channel_authenticator.cc:309:
ssl_config.allowed_bad_certs.emplace_back(cert_and_status);
Why emplace_back() instead of push_back()? Copy constructor is called either
way, so I don't see any reason to use emplace_back() here and push_back() is
more readable.

[davidben, 2016-08-31 19:20:49]

https://codereview.chromium.org/2300533002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2300533002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1486:
server_ssl_config_.allowed_bad_certs.emplace_back(bad_cert);
Same comment as Sergey about why emplace_back vs push_back. I think the
emplace_back version would be emplace_back(ssl_info_.cert,
ssl_info.cert_status), but CertAndStatus has no such constructor.

https://codereview.chromium.org/2300533002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_impl.cc (left):

https://codereview.chromium.org/2300533002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_impl.cc:452: #if defined(USE_OPENSSL_CERTS)
Hrm. This does mean that the SSLClientSocketImpl will no longer share an X509*
with the X509Certificate version, so we lose one deduplication in the good dedup
case but avoid caching things with GetDER in the bad dedup case (where we got an
X509* over the wire into SSLClientSocketImpl that we already had).

Or am I misunderstanding the situation? Seems what we'd really want is for
InsertOrUpdate to not cache stuff on the discarded OSCertHandle. Or maybe it
caches and throws it away?

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
remoting/protocol/ssl_hmac_channel_authenticator.cc:300: if (!cert.get()) {
!cert?

[Ryan Sleevi, 2016-08-31 20:33:13]

https://codereview.chromium.org/2300533002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_impl.cc (left):

https://codereview.chromium.org/2300533002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_impl.cc:452: #if defined(USE_OPENSSL_CERTS)
On 2016/08/31 19:20:49, davidben wrote:
> Or am I misunderstanding the situation? Seems what we'd really want is for
> InsertOrUpdate to not cache stuff on the discarded OSCertHandle. Or maybe it
> caches and throws it away?

We were sharing (de-duping) the first X509* (X509*[1]) from a socket, since
InsertOrUpdate() would add it to the X509CertificateCache. However, because each
SSL* creates a distinct X509*, the second (and subsequent) sockets would end up
calling InsertOrUpdate(), which would:
A) Call GetDER on X509*[2], which caches the DER on X509*[2]
B) See that X509*[2] is the same as X509*[1]
C) Replace X509Certificate's version with X509*[1] and free Chromium's copy of
X509*[2]

However, SSL*[2] still holds X509*[2], so it ended up keeping X509*[2] (and the
DER cache) from A alive until the socket was disconnected. That was the dupe.

So the end result is we lose 1 de-dup if we only had a single socket to the host
with the cert, but if we had >1 socket, we win. It seems safe to assume that, in
aggregate, we'll win here (as reflected in Dimitry's tests), although yes, we
lose out for single-socket H2 connections.

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/2300533002/diff/1/remoting/protocol/ssl_hmac_...
remoting/protocol/ssl_hmac_channel_authenticator.cc:309:
ssl_config.allowed_bad_certs.emplace_back(cert_and_status);
On 2016/08/31 18:46:44, Sergey Ulanov (ooo till 8-31) wrote:
> Why emplace_back() instead of push_back()? Copy constructor is called either
> way, so I don't see any reason to use emplace_back() here and push_back() is
> more readable.

Because I'm an idiot who forgot to std::move()-it, std::move()-it. :) Fixed it.

(wanted to avoid the extra ref-counting and better reflect the throw-awayness)

[davidben, 2016-09-01 19:44:40]

lgtm modulo further emplace_back vs push_back comments below.

https://codereview.chromium.org/2300533002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2300533002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:1486:
server_ssl_config_.allowed_bad_certs.emplace_back(std::move(bad_cert));
I think push_back(std::move(bad_cert)) also does the right thing? There's an
rvalue reference version of push_back.

http://en.cppreference.com/w/cpp/container/vector/push_back

Don't you also need an rvalue-reference version of the constructor? Honestly I
don't really understand all the C++11 rules.

https://codereview.chromium.org/2300533002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2300533002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:471: chain.emplace_back(std::move(cert));
Ditto

https://codereview.chromium.org/2300533002/diff/40001/net/socket/ssl_server_s...
File net/socket/ssl_server_socket_unittest.cc (right):

https://codereview.chromium.org/2300533002/diff/40001/net/socket/ssl_server_s...
net/socket/ssl_server_socket_unittest.cc:401:
client_ssl_config_.allowed_bad_certs.emplace_back(
Ditto.

https://codereview.chromium.org/2300533002/diff/40001/remoting/protocol/ssl_h...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/2300533002/diff/40001/remoting/protocol/ssl_h...
remoting/protocol/ssl_hmac_channel_authenticator.cc:309:
ssl_config.allowed_bad_certs.emplace_back(std::move(cert_and_status));
Ditto

[davidben, 2016-09-01 21:03:06]

(New emplace_back business lgtm.)

[Ryan Sleevi, 2016-09-01 21:08:42]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-09-01 21:08:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2300533002/#ps60001
(title: "Use the right syntax")

[commit-bot: I haz the power, 2016-09-01 21:09:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 23:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 23:06:25]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Sleevi, 2016-09-02 23:53:22]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:54:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 01:21:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 01:21:59]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Sleevi, 2016-09-03 01:42:33]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-09-03 01:42:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 02:30:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 02:30:25]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Sleevi, 2016-09-03 11:26:58]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-09-03 11:27:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 12:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 12:56:50]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Sleevi, 2016-09-09 18:43:39]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:44:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 20:16:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 20:16:02]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ryan Sleevi, 2016-09-12 17:31:01]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 17:31:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2016-09-13 19:18:22]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-09-13 19:18:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/2300533002/#ps140001
(title: "Remove debug")

[commit-bot: I haz the power, 2016-09-13 19:18:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 20:35:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-13 20:37:22]

Description was changed from

==========
Stop caching DER-encoded certificates unnecessarily

What was intended as a CPU performance short-circuit
in SSLClientSocket for Open/BoringSSL implementations
ended up introducing rather unfortunate and
significant memory overhead, thus proving yet again
that premature optimization is the root of all evil.

While cleaning the unnecessary DER-conversion/caching
up, a further optimization to reduce the overhead
of adding certificate errors was pointed out, allowing
us to avoid the overhead of storing another copy of the
DER-encoded certificate. As //remoting-in-NaCl can now
use BoringSSL directly, this unwinds
http://crrev.com/93153 and uses the cached certificate
directly.

BUG=642082
R=davidben@chromium.org, sergeyu@chromium.org
==========

to

==========
Stop caching DER-encoded certificates unnecessarily

What was intended as a CPU performance short-circuit
in SSLClientSocket for Open/BoringSSL implementations
ended up introducing rather unfortunate and
significant memory overhead, thus proving yet again
that premature optimization is the root of all evil.

While cleaning the unnecessary DER-conversion/caching
up, a further optimization to reduce the overhead
of adding certificate errors was pointed out, allowing
us to avoid the overhead of storing another copy of the
DER-encoded certificate. As //remoting-in-NaCl can now
use BoringSSL directly, this unwinds
http://crrev.com/93153 and uses the cached certificate
directly.

BUG=642082
R=davidben@chromium.org, sergeyu@chromium.org

Committed: https://crrev.com/74e99744fe3d88a5b4a759818fa813cd2d091bd7
Cr-Commit-Position: refs/heads/master@{#418352}
==========

[commit-bot: I haz the power, 2016-09-13 20:37:23]

Patchset 8 (id:??) landed as
https://crrev.com/74e99744fe3d88a5b4a759818fa813cd2d091bd7
Cr-Commit-Position: refs/heads/master@{#418352}