Check Expect-CT at connection setup

Check Expect-CT at connection setup

This CL adds an Expect-CT check to ShouldRequireCT(), with an option to send
reports if the host is configured with Expect-CT.

This CL is missing a test for ProofVerifierChromium, which I'm omitting because
all the tests for that file are mysteriously disabled and I'm not sure why.

BUG=679012
Review-Url: https://codereview.chromium.org/2850033002
Cr-Commit-Position: refs/heads/master@{#469686}
Committed: https://chromium.googlesource.com/chromium/src/+/bf1b5296ef441a71980bd48279c4442cb53957fd

[estark, 2017-04-29 23:58:50]

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

[commit-bot: I haz the power, 2017-04-29 23:58:55]

Dry run: CQ is trying da patch.

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

[estark, 2017-04-30 00:06:59]

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

[commit-bot: I haz the power, 2017-04-30 00:07:03]

Dry run: CQ is trying da patch.

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

[estark, 2017-04-30 00:09:19]

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

[commit-bot: I haz the power, 2017-04-30 00:09:21]

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, 2017-04-30 00:34:20]

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

[commit-bot: I haz the power, 2017-04-30 00:34:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2017-04-30 00:46:36]

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

[commit-bot: I haz the power, 2017-04-30 00:47:40]

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, 2017-04-30 03:05:07]

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

[commit-bot: I haz the power, 2017-04-30 03:05:08]

Dry run: This issue passed the CQ dry run.

[estark, 2017-05-01 23:14:18]

estark@chromium.org changed reviewers:
+ mattm@chromium.org

[estark, 2017-05-01 23:14:18]

mattm, PTAL?

https://codereview.chromium.org/2850033002/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter.h (left):

https://codereview.chromium.org/2850033002/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter.h:31: const net::SSLInfo& ssl_info)
override;
Two notes:
- I changed this argument from an SSLInfo to take the individual things that are
needed from the SSLInfo, because this method is now called from
ShouldRequireCT() callsites which don't all have a full SSLInfo available yet.
- It's currently odd that this reporting code is implemented in //chrome. (It's
a vestige from the experimental preload-list-only version of Expect-CT, for
which it seemed more reasonable to confine reporting to //chrome because it was
a Chrome-only experiment.) Now that Expect-CT is being standardized in the
platform, I'll move this code to either //content or //net in a follow-up CL.

[mattm, 2017-05-02 23:32:51]

mattm@chromium.org changed reviewers:
+ rch@chromium.org

[mattm, 2017-05-02 23:32:52]

rch: Do you know why all the ProofVerifierChromium tests are disabled?

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:384: // will be sent.
It seems weird that a method called "ShouldRequireCT" has this side effect. Also
whether the connection is compliant isn't checked by ShouldRequireCT, it just
assumes the caller only calls ShouldRequireCT if the connection isn't compliant.

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:392: const X509Certificate*
served_certificate_chain,
confusing that the order is different here than in OnExpectCTFailed

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

https://codereview.chromium.org/2850033002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3527: TEST_F(SSLClientSocketTest,
CTIsRequiredByExpectCT) {
Test other combinations of having a header and different compliance results?

https://codereview.chromium.org/2850033002/diff/60001/net/spdy/chromium/spdy_...
File net/spdy/chromium/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2850033002/diff/60001/net/spdy/chromium/spdy_...
net/spdy/chromium/spdy_session_unittest.cc:5975: "www.example.org"));
use different hostnames so that the test verifies that it is checking the
expect-ct status for the correct hostname

[Ryan Hamilton, 2017-05-02 23:41:27]

On 2017/05/02 23:32:52, mattm wrote:
> rch: Do you know why all the ProofVerifierChromium tests are disabled?

No, but I probably should. It looks related to the changes in
https://codereview.chromium.org/2176323002 where we stopped supporting QUIC
versions earlier than v30 (which supports CT). But I'm not sure why. I'll take a
look.

[estark, 2017-05-03 00:13:20]

(no code changes yet)

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:384: // will be sent.
On 2017/05/02 23:32:52, mattm wrote:
> It seems weird that a method called "ShouldRequireCT" has this side effect.
Also
> whether the connection is compliant isn't checked by ShouldRequireCT, it just
> assumes the caller only calls ShouldRequireCT if the connection isn't
compliant.

Yeah, I thought about that and hoped that the extra documentation + ReportStatus
parameter might mitigate the weirdness. Here are a few other options:
a.) Have the method take a CertPolicyCompliance and immediately return true if
it is compliant. This would remove the issue where we assume the caller only
calls it for noncompliant connections. For the report-sending side effect, we
could rename the method to ShouldRequireCTAndMaybeSendReport or
CheckCTRequirements (mirroring CheckPublicKeyPins which also sends reports)
or... something else?
b.) Make Expect-CT a separate method entirely (CheckExpectCT?), and have callers
call both ShouldRequireCT and CheckExpectCT. I don't like this because it seems
error-prone to ask callers to check two separate methods to determine if a
connection should be closed because of CT.
b) Leave the connection-enforcement logic in ShouldRequireCT but make a separate
method MaybeSendExpectCT report to do the report-sending. I don't like this much
either because it would be easy for a caller to forget to call MaybeSendExpectCT
after calling ShouldRequireCT.

Of these, I like (a) best; WDYT?

[mattm, 2017-05-03 00:37:07]

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:384: // will be sent.
On 2017/05/03 00:13:20, estark wrote:
> On 2017/05/02 23:32:52, mattm wrote:
> > It seems weird that a method called "ShouldRequireCT" has this side effect.
> Also
> > whether the connection is compliant isn't checked by ShouldRequireCT, it
just
> > assumes the caller only calls ShouldRequireCT if the connection isn't
> compliant.
> 
> Yeah, I thought about that and hoped that the extra documentation +
ReportStatus
> parameter might mitigate the weirdness. Here are a few other options:
> a.) Have the method take a CertPolicyCompliance and immediately return true if
> it is compliant. This would remove the issue where we assume the caller only
> calls it for noncompliant connections. For the report-sending side effect, we
> could rename the method to ShouldRequireCTAndMaybeSendReport or
> CheckCTRequirements (mirroring CheckPublicKeyPins which also sends reports)
> or... something else?
> b.) Make Expect-CT a separate method entirely (CheckExpectCT?), and have
callers
> call both ShouldRequireCT and CheckExpectCT. I don't like this because it
seems
> error-prone to ask callers to check two separate methods to determine if a
> connection should be closed because of CT.
> b) Leave the connection-enforcement logic in ShouldRequireCT but make a
separate
> method MaybeSendExpectCT report to do the report-sending. I don't like this
much
> either because it would be easy for a caller to forget to call
MaybeSendExpectCT
> after calling ShouldRequireCT.
> 
> Of these, I like (a) best; WDYT?

(a) sgtm too. CheckCTRequirements seems reasonable. (I think the
ShouldRequireCT... name would then be a bit weird if it does the compliance
check too.)

[estark, 2017-05-04 01:12:08]

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

[commit-bot: I haz the power, 2017-05-04 01:12:39]

Dry run: CQ is trying da patch.

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

[estark, 2017-05-04 01:18:30]

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:384: // will be sent.
On 2017/05/03 00:37:07, mattm wrote:
> On 2017/05/03 00:13:20, estark wrote:
> > On 2017/05/02 23:32:52, mattm wrote:
> > > It seems weird that a method called "ShouldRequireCT" has this side
effect.
> > Also
> > > whether the connection is compliant isn't checked by ShouldRequireCT, it
> just
> > > assumes the caller only calls ShouldRequireCT if the connection isn't
> > compliant.
> > 
> > Yeah, I thought about that and hoped that the extra documentation +
> ReportStatus
> > parameter might mitigate the weirdness. Here are a few other options:
> > a.) Have the method take a CertPolicyCompliance and immediately return true
if
> > it is compliant. This would remove the issue where we assume the caller only
> > calls it for noncompliant connections. For the report-sending side effect,
we
> > could rename the method to ShouldRequireCTAndMaybeSendReport or
> > CheckCTRequirements (mirroring CheckPublicKeyPins which also sends reports)
> > or... something else?
> > b.) Make Expect-CT a separate method entirely (CheckExpectCT?), and have
> callers
> > call both ShouldRequireCT and CheckExpectCT. I don't like this because it
> seems
> > error-prone to ask callers to check two separate methods to determine if a
> > connection should be closed because of CT.
> > b) Leave the connection-enforcement logic in ShouldRequireCT but make a
> separate
> > method MaybeSendExpectCT report to do the report-sending. I don't like this
> much
> > either because it would be easy for a caller to forget to call
> MaybeSendExpectCT
> > after calling ShouldRequireCT.
> > 
> > Of these, I like (a) best; WDYT?
> 
> (a) sgtm too. CheckCTRequirements seems reasonable. (I think the
> ShouldRequireCT... name would then be a bit weird if it does the compliance
> check too.)

Done.

https://codereview.chromium.org/2850033002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:392: const X509Certificate*
served_certificate_chain,
On 2017/05/02 23:32:52, mattm wrote:
> confusing that the order is different here than in OnExpectCTFailed

Done (flipped the order in OnExpectCTFailed)

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

https://codereview.chromium.org/2850033002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3527: TEST_F(SSLClientSocketTest,
CTIsRequiredByExpectCT) {
On 2017/05/02 23:32:52, mattm wrote:
> Test other combinations of having a header and different compliance results?

Done. The logic for checking compliance results is now tested in
TransportSecurityState unit tests, but I added tests here for good measure.
(Happy to remove them if you feel they're superfluous now.)

https://codereview.chromium.org/2850033002/diff/60001/net/spdy/chromium/spdy_...
File net/spdy/chromium/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2850033002/diff/60001/net/spdy/chromium/spdy_...
net/spdy/chromium/spdy_session_unittest.cc:5975: "www.example.org"));
On 2017/05/02 23:32:52, mattm wrote:
> use different hostnames so that the test verifies that it is checking the
> expect-ct status for the correct hostname

Done.

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state_unittest.cc:2583:
ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS));
Now that the CertPolicyCompliance testing is in TransportSecurityState instead
of its callers, I tried to include test coverage that both NOT_ENOUGH_SCTS and
NOT_DIVERSE_SCTS are treated as non-compliant. I considered parameterizing some
of these tests to test all combinations of compliant and non-compliant statuses,
but that seemed a little overkill to me.

[commit-bot: I haz the power, 2017-05-04 01:45:23]

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

[commit-bot: I haz the power, 2017-05-04 01:45:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mattm, 2017-05-04 01:57:11]

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:863: bool
TransportSecurityState::CheckCTRequirements(
do you think it's worth returning an enum value so there's no confusion over
what true / false mean?

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:908: return g_ct_required_for_testing == 1;
should that be !=  now?

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:913: bool default_response = true;
since you're touching this anyway, make this const?

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:964: return true;
should that be false now? (Is there a test for this?)

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.h:559: // For unit tests only; causes
CheckCTRequirements() to return |*required|
comment seems backwards now. (should require = true implies CheckCTRequirements
should return false)

[estark, 2017-05-04 02:33:11]

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

[commit-bot: I haz the power, 2017-05-04 02:33:47]

Dry run: CQ is trying da patch.

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

[estark, 2017-05-04 04:03:25]

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:863: bool
TransportSecurityState::CheckCTRequirements(
On 2017/05/04 01:57:11, mattm wrote:
> do you think it's worth returning an enum value so there's no confusion over
> what true / false mean?

Yes, I think that makes sense. Done.

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:908: return g_ct_required_for_testing == 1;
On 2017/05/04 01:57:11, mattm wrote:
> should that be !=  now?

Fixed with the new enum return value.

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:913: bool default_response = true;
On 2017/05/04 01:57:11, mattm wrote:
> since you're touching this anyway, make this const?

Done.

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:964: return true;
On 2017/05/04 01:57:11, mattm wrote:
> should that be false now? (Is there a test for this?)

Oops. There was a test; I just forgot to run it. Sorry about that. Fixed,
trybots are green now.

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/80001/net/http/transport_secu...
net/http/transport_security_state.h:559: // For unit tests only; causes
CheckCTRequirements() to return |*required|
On 2017/05/04 01:57:11, mattm wrote:
> comment seems backwards now. (should require = true implies
CheckCTRequirements
> should return false)

Fixed with new enum return value.

[commit-bot: I haz the power, 2017-05-04 04:11:12]

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

[commit-bot: I haz the power, 2017-05-04 04:11:13]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-05 06:59:02]

lgtm

https://codereview.chromium.org/2850033002/diff/100001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/100001/net/http/transport_sec...
net/http/transport_security_state.h:389: // Returns CT_REQUIREMENTS_NOT)MET if a
connection violates CT policy
CT_REQUIREMENTS_NOT_MET

[estark, 2017-05-05 15:15:12]

Thanks, Matt!

Filed crbug.com/718895 for following up on the QUIC test.

https://codereview.chromium.org/2850033002/diff/100001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2850033002/diff/100001/net/http/transport_sec...
net/http/transport_security_state.h:389: // Returns CT_REQUIREMENTS_NOT)MET if a
connection violates CT policy
On 2017/05/05 06:59:02, mattm wrote:
> CT_REQUIREMENTS_NOT_MET

Done.

[estark, 2017-05-05 15:15:57]

The CQ bit was checked by estark@chromium.org

[estark, 2017-05-05 15:15:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2850033002/#ps120001
(title: "fix comment typo")

[commit-bot: I haz the power, 2017-05-05 15:16:30]

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, 2017-05-05 17:05:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493997357153170,
"parent_rev": "df175df214a222e144783ec7cf776266fa8c974e", "commit_rev":
"bf1b5296ef441a71980bd48279c4442cb53957fd"}

[commit-bot: I haz the power, 2017-05-05 17:05:46]

Description was changed from

==========
Check Expect-CT at connection setup

This CL adds an Expect-CT check to ShouldRequireCT(), with an option to send
reports if the host is configured with Expect-CT.

This CL is missing a test for ProofVerifierChromium, which I'm omitting because
all the tests for that file are mysteriously disabled and I'm not sure why.

BUG=679012
==========

to

==========
Check Expect-CT at connection setup

This CL adds an Expect-CT check to ShouldRequireCT(), with an option to send
reports if the host is configured with Expect-CT.

This CL is missing a test for ProofVerifierChromium, which I'm omitting because
all the tests for that file are mysteriously disabled and I'm not sure why.

BUG=679012

Review-Url: https://codereview.chromium.org/2850033002
Cr-Commit-Position: refs/heads/master@{#469686}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf1b5296ef441a71980bd48279c4...
==========

[commit-bot: I haz the power, 2017-05-05 17:05:48]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bf1b5296ef441a71980bd48279c4...