Enable Expect-Staple in SSLClientSocket.

Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_|
to true by default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around
extracting the OCSP response and setting the
UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See
https://crbug.com/631101

BUG=598021
Committed: https://crrev.com/3c0e49240847ea54265e17c7a8a1ecf73daeaeee
Cr-Commit-Position: refs/heads/master@{#407575}

[dadrian, 2016-07-20 00:54:34]

Description was changed from

==========
Respect Expect-Staple in SSLClientSocket.

SSLClientSocket calls TransportSecurityState::ProcessExpectStaple. However,
Expect-Staple is currently not enabled in TransportSecurityState, so the net
effect is nothing happens.

BUG=598021
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

BUG=598021
==========

[dadrian, 2016-07-20 23:37:19]

dadrian@google.com changed reviewers:
+ estark@chromium.org, rsleevi@chromium.org, svaldez@chromium.org

[dadrian, 2016-07-20 23:37:19]

Sending this out for review now that the depends-on are in the CQ / committed.

This boldly goes forward and enables Expect-Staple for preloaded sites (which,
as of now, consists of a nonexistent test site).

[Ryan Sleevi, 2016-07-20 23:46:35]

Small question: Why not QUIC too?

[dadrian, 2016-07-20 23:48:59]

On 2016/07/20 23:46:35, Ryan Sleevi (extremely slow) wrote:
> Small question: Why not QUIC too?

AFAIK QUIC has no way of stapling OCSP information.

[svaldez, 2016-07-25 14:57:39]

You'll probably want to add a bug or TODO for supporting Expect-Staple with
QUIC, at least when QUIC/TLS1.3 happens that information will be exposed, don't
know if its currently exposed in the APIs.

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

https://codereview.chromium.org/2155753002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1205:
UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled", ocsp_response_len != 0);
Possibly keep this under the same sct_enabled check, otherwise update the CL to
note that the histogram is changing.

[chromium-reviews, 2016-07-25 15:10:33]

On Jul 25, 2016 7:57 AM, <svaldez@chromium.org> wrote:
>
>
>
>
>
https://codereview.chromium.org/2155753002/diff/20001/net/socket/ssl_client_s...
> File net/socket/ssl_client_socket_impl.cc (right):
>
>
https://codereview.chromium.org/2155753002/diff/20001/net/socket/ssl_client_s...
> net/socket/ssl_client_socket_impl.cc:1205:
> UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled", ocsp_response_len !=
> 0);
> Possibly keep this under the same sct_enabled check, otherwise update
> the CL to note that the histogram is changing.
>
> https://codereview.chromium.org/2155753002/

It's always true, so no need (to change). The CL description could say that
though, but it's precisely because we never set it differently that the
check is unnecessary.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dadrian, 2016-07-25 17:38:54]

Description was changed from

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

BUG=598021
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

BUG=598021
==========

[dadrian, 2016-07-25 17:39:47]

TODO added, description updated.

[Ryan Sleevi, 2016-07-25 18:31:42]

LGTM but wouldn't mind a secondary spot-check from estark or svaldez

[svaldez, 2016-07-25 19:11:23]

Consider adding the QUIC bug to the CL description, though maybe not necessary.
LGTM otherwise.

[dadrian, 2016-07-25 20:07:19]

Description was changed from

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

BUG=598021
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021
==========

[dadrian, 2016-07-25 20:07:32]

The CQ bit was checked by dadrian@google.com

[commit-bot: I haz the power, 2016-07-25 20:08:20]

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-07-25 21:17:20]

Description was changed from

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021
==========

[commit-bot: I haz the power, 2016-07-25 21:17:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-25 21:19:58]

Description was changed from

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021

Committed: https://crrev.com/3c0e49240847ea54265e17c7a8a1ecf73daeaeee
Cr-Commit-Position: refs/heads/master@{#407575}
==========

[commit-bot: I haz the power, 2016-07-25 21:19:59]

Patchset 3 (id:??) landed as
https://crrev.com/3c0e49240847ea54265e17c7a8a1ecf73daeaeee
Cr-Commit-Position: refs/heads/master@{#407575}

[Ryan Sleevi, 2016-07-25 21:29:46]

Description was changed from

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_| to true by
default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around extracting the OCSP
response and setting the UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See https://crbug.com/631101

BUG=598021

Committed: https://crrev.com/3c0e49240847ea54265e17c7a8a1ecf73daeaeee
Cr-Commit-Position: refs/heads/master@{#407575}
==========

to

==========
Enable Expect-Staple in SSLClientSocket.

In TransportSecurityState, set |enable_static_expect_staple_|
to true by default. Update SSLClientSocket to call
TransportSecurityState::ProcessExpectStaple.

In ssl_client_socket_impl.cc, this also removes the if
(|signed_certificate_timestamps_enabled_) check around
extracting the OCSP response and setting the
UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled"). Since
SCTs are always enabled, this if statement was a noop.

This does not enable Expect-Staple for QUIC. See
https://crbug.com/631101

BUG=598021

Committed: https://crrev.com/3c0e49240847ea54265e17c7a8a1ecf73daeaeee
Cr-Commit-Position: refs/heads/master@{#407575}
==========

[xunjieli, 2016-07-25 21:48:35]

net_unittests is failing on a Cronet bot. This is the ssl-related change in the
range. Could you take a look?
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM...

I  164.993s Main  1 failed tests remain.
C  165.045s Main 
********************************************************************************
C  165.045s Main  Detailed Logs
C  165.045s Main 
********************************************************************************
C  165.063s Main  [FAIL] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid:
C  165.063s Main  [ RUN      ] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid
C  165.063s Main  [WARNING:embedded_test_server.cc(193)] Request not handled.
Returning 404: /
C  165.063s Main  ../../net/url_request/url_request_unittest.cc:9342: Failure
C  165.063s Main  Value of: mock_report_sender.latest_report().empty()
C  165.063s Main    Actual: false
C  165.064s Main  Expected: true
C  165.064s Main  ../../net/url_request/url_request_unittest.cc:9343: Failure
C  165.064s Main  Value of: mock_report_sender.latest_report_uri()
C  165.064s Main    Actual: https://report.badssl.com/expect-staple
C  165.064s Main  Expected: GURL()
C  165.064s Main  Which is: 
C  165.064s Main  [  FAILED  ] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid
(65 ms)
C  165.064s Main 
********************************************************************************
C  165.064s Main  Summary
C  165.064s Main 
********************************************************************************
C  165.116s Main  [==========] 18158 tests ran.
C  165.116s Main  [  PASSED  ] 18157 tests.
C  165.116s Main  [  FAILED  ] 1 test, listed below:
C  165.116s Main  [  FAILED  ] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid

[Mark P, 2016-07-25 21:54:32]

The tree is closed.  To reopen the tree, I will revert this change.

[Mark P, 2016-07-25 21:56:34]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2176183003/ by mpearson@chromium.org.

The reason for reverting is: The newly added test
HTTPSOCSPTest.ExpectStapleReportNotSentOnValid fails on the Android bot:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM...


C  165.045s Main  Detailed Logs
C  165.045s Main 
********************************************************************************
C  165.063s Main  [FAIL] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid:
C  165.063s Main  [ RUN      ] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid
C  165.063s Main  [WARNING:embedded_test_server.cc(193)] Request not handled.
Returning 404: /
C  165.063s Main  ../../net/url_request/url_request_unittest.cc:9342: Failure
C  165.063s Main  Value of: mock_report_sender.latest_report().empty()
C  165.063s Main    Actual: false
C  165.064s Main  Expected: true
C  165.064s Main  ../../net/url_request/url_request_unittest.cc:9343: Failure
C  165.064s Main  Value of: mock_report_sender.latest_report_uri()
C  165.064s Main    Actual: https://report.badssl.com/expect-staple
C  165.064s Main  Expected: GURL()
C  165.064s Main  Which is: 
C  165.064s Main  [  FAILED  ] HTTPSOCSPTest.ExpectStapleReportNotSentOnValid
(65 ms).

[findit-for-me, 2016-07-25 22:19:02]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 407575 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...