Optimize CT & OCSP handling code

Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org

Committed: https://crrev.com/22cae167148a1f2b632ff4c20f975810d4edb188
Cr-Commit-Position: refs/heads/master@{#440818}

[Ryan Sleevi, 2016-12-23 01:21:59]

Description was changed from

==========
Clean up CTVerifier API

Change CTVerifier::Verify() to return void, as the response code is
never used. The "Does this conform to a policy I expect it to" is
handled by the CTPolicyEnforcer, and the only two response codes
it ever returned can simply be inferred from scts.empty().

Also change CTVerifier::Verify() to use a base::StringPiece, rather
than const std::string&. This allows for a reduction in copies,
notably around the OCSP response, as a slight optimization.

In updating this interface, also update clients that were overriding
a net::CTVerifier to skip CT validation with the new/recommended
way of doing this, a net::DoNothingCTVerifier, which skips processing
the CT response when the client will ignore it in the CTPolicyEnforcer.

BUG=571203
==========

to

==========
Optimize CT & OCSP handling code

This change makes a few small improvements to the handling of the
Certificate Transparency and OCSP code to be more memory friendly
and to eliminate some API oddities.

The main change is to allow OCSP responses and the SCT TLS
extension to be zero-copy, remaining owned by the underlying SSL*,
by updating the APIs using these responses to use
base::StringPiece rather than const std::string&

Since this required updating every CTVerifier implementation,
an API wart that had emerged was excised. Originally, it was
expected that the CTVerifier contract would also handle any
relevant CT policies, and thus was given the full flexibility to
return a net error code (or net::OK). However, this flexibility
only makes sense in a hard-fail CT world, which isn't the world
today nor anytime soon, which is why the CTPolicyEnforcer was
introduced (to handle CT enforcement on a more nuanced level,
without mucking up the TLS sockets). As CTVerifier's response
codes were then just effectively a boolean as to whether the
output SCT list was .empty(), the whole return code API was
stripped and it was made a void API, which contractually makes it
clearer that SCT evaluation is always synchronous.

While a fundamentally small change, as this touched all the
CTVerifier-derived classes, it also made sense to clean up the
derived classes, by either migrating to the DoNothingCTVerifier,
which indicates no interest in CT (thus avoiding any parsing
overhead) or by eliminating unnecessary GMock implementations.

The result of this change is that for connections which have an
OCSP stapled response, we should save an extra 2K-12K per socket,
and allow BoringSSL the opportunity to more efficiently string
pool in its underlying implementation in the future.

BUG=571203
==========

[Ryan Sleevi, 2016-12-23 01:23:07]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2016-12-23 01:23:08]

David: I'll send this to you. As you can see from the description, I grouped a
few cleanups into a single API, but it seemed to logically fit with what the
task was and involved. If you would prefer the void return vs base::StringPiece
changes split, I can, but mostly will involve touching the same files.

[Eran Messeri, 2016-12-27 20:58:04]

eranm@chromium.org changed reviewers:
+ eranm@chromium.org

[Eran Messeri, 2016-12-27 20:58:05]

drive-by lgtm (all seems to make sense from an API perspective, thanks Ryan for
removing the return value from CTVerifier::Verify).

[eroman, 2016-12-27 22:00:39]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2016-12-27 22:00:41]

lgtm after addressing these comments

https://codereview.chromium.org/2604513002/diff/40001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:46: // empty string. |result| will be filled with the
SCTs present, divided into
Can you fix this comment too?

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.cc (left):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.cc:166: base::StringPiece temp(encoded_sct_list);
I suggest keeping a temp string piece for clarity, to make it clear that
modifications by DecodeSCTList aren't  are discarded.

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.cc:170: // XXX(rsleevi): Should we really just
skip over bad SCTs?
Can you fix this to TODO ?

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.h:48: // placing the verification results in
|result|. The SCTs in the list
Same here, can you fix this comment?

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:114: bool
VerifySinglePrecertificateChain(scoped_refptr<X509Certificate> chain) {
Can you document what the return value of this function represents? (The new
implementation doesn't match what my expectation would be).

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:118: return !scts.empty();
Not sure what the goal of the function is, but I would have expected this to be
returning true only if there is a success:

return !SCTsMatchingStatus(scts, SCT_STATUS_OK).empty()

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1571: const uint8_t* ocsp_response_raw;
Extract to a helper?

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1573: SSL_get0_ocsp_response(ssl_.get(),
&ocsp_response_raw, &ocsp_response_len);
Not very familiar with the interactions here, but I am assuming the OCSP
response is accessible here and has the same value as where it was snapshotted
in the old code.

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2318: EXPECT_CALL(ct_verifier,
Verify(_, _, sct_ext, _, _));
Shouldn't this also clear the sct list?

I am not clear based on some of the test implementations whether CTVerifier's
contract is an assign model, or an append model.

 * DoNothingCTVerifier assumes an assign model
 * This callsite and some other tests imply the contract is an append model
 * The documentation (which refers to defunct argument) describes it as "fill",
and doesn't mention any constraint that the list is already empty.

Either the documentation needs to guarantee that the input list is empty (and
hence implementations need only append)

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_server_s...
net/socket/ssl_server_socket_unittest.cc:99: return net::OK;
(This now deleted implementation didn't clear output_scts)

https://codereview.chromium.org/2604513002/diff/40001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.cc (left):

https://codereview.chromium.org/2604513002/diff/40001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.cc:440: return net::OK;
(same here)

[Ryan Sleevi, 2016-12-27 22:17:19]

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.cc (left):

https://codereview.chromium.org/2604513002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.cc:166: base::StringPiece temp(encoded_sct_list);
On 2016/12/27 22:00:40, eroman (slow) wrote:
> I suggest keeping a temp string piece for clarity, to make it clear that
> modifications by DecodeSCTList aren't  are discarded.

DecodeSCTList doesn't need to modify the input. I've instead corrected that
signature to make it more obvious / fix more API warts.

That said, I'm slightly inclined to believe it should be 'obvious' by the fact
that it's a base::StringPiece by-val - we shouldn't need to make temporaries to
make it obvious it's a temporary.

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1571: const uint8_t* ocsp_response_raw;
On 2016/12/27 22:00:41, eroman (slow) wrote:
> Extract to a helper?

I don't believe the helper here helps readability. The previous attempt to
extract added unnecessary memory overheads; this code makes it clearer.

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1573: SSL_get0_ocsp_response(ssl_.get(),
&ocsp_response_raw, &ocsp_response_len);
On 2016/12/27 22:00:41, eroman (slow) wrote:
> Not very familiar with the interactions here, but I am assuming the OCSP
> response is accessible here and has the same value as where it was snapshotted
> in the old code.

Correct.

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2318: EXPECT_CALL(ct_verifier,
Verify(_, _, sct_ext, _, _));
On 2016/12/27 22:00:41, eroman (slow) wrote:
> Shouldn't this also clear the sct list?
> 
> I am not clear based on some of the test implementations whether CTVerifier's
> contract is an assign model, or an append model.

It's supposed to be exclusively an assign-on-success model.

>  * This callsite and some other tests imply the contract is an append model

I'm not sure why this line implies an append model to you. Could you explain
further?

> Either the documentation needs to guarantee that the input list is empty (and
> hence implementations need only append)

Incomplete comment, but I'm not sure why you feel that one contract or the other
is implied from this file.

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

https://codereview.chromium.org/2604513002/diff/40001/net/socket/ssl_server_s...
net/socket/ssl_server_socket_unittest.cc:99: return net::OK;
On 2016/12/27 22:00:41, eroman (slow) wrote:
> (This now deleted implementation didn't clear output_scts)

Yeah, was improper :)

[Ryan Sleevi, 2016-12-28 00:36:58]

Description was changed from

==========
Optimize CT & OCSP handling code

This change makes a few small improvements to the handling of the
Certificate Transparency and OCSP code to be more memory friendly
and to eliminate some API oddities.

The main change is to allow OCSP responses and the SCT TLS
extension to be zero-copy, remaining owned by the underlying SSL*,
by updating the APIs using these responses to use
base::StringPiece rather than const std::string&

Since this required updating every CTVerifier implementation,
an API wart that had emerged was excised. Originally, it was
expected that the CTVerifier contract would also handle any
relevant CT policies, and thus was given the full flexibility to
return a net error code (or net::OK). However, this flexibility
only makes sense in a hard-fail CT world, which isn't the world
today nor anytime soon, which is why the CTPolicyEnforcer was
introduced (to handle CT enforcement on a more nuanced level,
without mucking up the TLS sockets). As CTVerifier's response
codes were then just effectively a boolean as to whether the
output SCT list was .empty(), the whole return code API was
stripped and it was made a void API, which contractually makes it
clearer that SCT evaluation is always synchronous.

While a fundamentally small change, as this touched all the
CTVerifier-derived classes, it also made sense to clean up the
derived classes, by either migrating to the DoNothingCTVerifier,
which indicates no interest in CT (thus avoiding any parsing
overhead) or by eliminating unnecessary GMock implementations.

The result of this change is that for connections which have an
OCSP stapled response, we should save an extra 2K-12K per socket,
and allow BoringSSL the opportunity to more efficiently string
pool in its underlying implementation in the future.

BUG=571203
==========

to

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
==========

[Ryan Sleevi, 2016-12-28 00:38:35]

rsleevi@chromium.org changed reviewers:
+ sergeyu@chromium.org
- davidben@chromium.org

[Ryan Sleevi, 2016-12-28 00:38:36]

Sergey: Going to TBR the mechanical change to you

[Ryan Sleevi, 2016-12-28 00:38:48]

Description was changed from

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
==========

to

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org
==========

[Ryan Sleevi, 2016-12-28 00:38:57]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-12-28 00:38:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
eranm@chromium.org
Link to the patchset: https://codereview.chromium.org/2604513002/#ps100001
(title: "Review feedback round two")

[commit-bot: I haz the power, 2016-12-28 00:39:14]

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

[eroman, 2016-12-28 01:33:50]

lgtm

https://codereview.chromium.org/2604513002/diff/100001/net/cert/multi_log_ct_...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/2604513002/diff/100001/net/cert/multi_log_ct_...
net/cert/multi_log_ct_verifier.h:47: // Verify a list of SCTs from
|encoded_sct_list| over |expected_entry|,
[optional] Verifies

[Sergey Ulanov, 2016-12-28 01:42:55]

lgtm

[commit-bot: I haz the power, 2016-12-28 01:53:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482885537354840,
"parent_rev": "b57b67d8e43f8eeb3091d73473a0f599378c0659", "commit_rev":
"a7eefe92f467e1019590b62830037691e6e59259"}

[commit-bot: I haz the power, 2016-12-28 01:54:06]

Description was changed from

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org
==========

to

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org

Review-Url: https://codereview.chromium.org/2604513002
==========

[commit-bot: I haz the power, 2016-12-28 01:54:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2017-01-02 15:48:09]

Description was changed from

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org

Review-Url: https://codereview.chromium.org/2604513002
==========

to

==========
Optimize CT & OCSP handling code

Update the interface for CTVerifier to use a base::StringPiece
for the OCSP and TLS SCT extension, rather than an std::string.
By allowing this to be zero-copy from the underlying SSL* that
stores this information, this allows pruning the ocsp_response_
member from SSLSocketImpl, which can save an extra 2K-12K per
socket for those that staple OCSP responses, and eliminate an
extra copy for the CT information.

As this required touching every CTVerifier-derived class, this
also fixes the API wart that crept in regarding
CTVerifier::Verify() having a return code. As the actual
verification of policy compliance is handled not by CTVerifier
but by CTPolicyEnforcer, there's no actual status code to return
from CTVerifier. To make future cleanups easier, and to align the
API and implementations for consistency, this also replaced the
various stub verifiers that grew with explicit
DoNothingCTVerifiers that skip all parsing entirely.

BUG=571203
TBR=sergeyu@chromium.org

Committed: https://crrev.com/22cae167148a1f2b632ff4c20f975810d4edb188
Cr-Commit-Position: refs/heads/master@{#440818}
==========

[commit-bot: I haz the power, 2017-01-02 15:48:10]

Patchset 6 (id:??) landed as
https://crrev.com/22cae167148a1f2b632ff4c20f975810d4edb188
Cr-Commit-Position: refs/heads/master@{#440818}