Introduce the ability to require CT for specific hosts

Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org

Committed: https://crrev.com/4a6ca8c5929a170798ad87339fb070361c2a3777
Cr-Commit-Position: refs/heads/master@{#401801}

[Ryan Sleevi, 2016-06-18 00:45:34]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine if a
host/certificate/public key hashes is required to supply valid
Certificate Transparency information. If so, cause the connection
to fail with ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic, this adds
the ability to set a RequireCTDelegate on the TSS, which allows
hosts to be opted-in or opted-out of the CT requirement.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures that both public key
pins and CT information are checked in parallel, but that the PKP error is
treated as more serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
==========

[Ryan Sleevi, 2016-06-18 00:46:43]

David: Since you got to deal with my rambling yesterday, PTAL
Emily: Sanity checking based on what we discussed

I'd like to get all of this landed as soon as reasonable, so please let me know
if you're time pressured.

[mmenke, 2016-06-18 00:59:28]

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

[mmenke, 2016-06-18 00:59:29]

https://codereview.chromium.org/2076363002/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/2076363002/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:378: NET_ERROR(SSL_CERTIFICATE_TRANSPARENCY_REQUIRED,
-172)
Should there be error page text for this, reusing the text from one of the other
cert errors maybe, rather than showing a generic ERR_FAILED page?

[estark, 2016-06-20 20:02:35]

lgtm for implementing what we talked about (checking both CT and HPKP, treating
HPKP as more serious). Though, perhaps there should be a unit test for that
behavior?

[mmenke, 2016-06-20 20:18:31]

One other question:  Is there a reason we're showing a net error page instead of
an interstitial on this error?

[Ryan Sleevi, 2016-06-20 20:33:49]

On 2016/06/20 20:18:31, mmenke wrote:
> One other question:  Is there a reason we're showing a net error page instead
of
> an interstitial on this error?

So, nothing actually triggers this error, which is why I wasn't touching the UI
aspect yet, but apologies for not being clearer on that.

Knowing that UI reviews can be a pain, my plan had been to touch on several
aspects of this concurrently, to try to streamline CLs

1) This CL, which introduces the //net error code, but nothing in the codebase
actually triggers it (that's why the TSS default code returns false, and there's
no delegate wired up)
2) Introduce the error handling page. In particular, this is being done as a net
error code rather than a cert status, so it will require a little bit more care
to surface it up to the interstitial code, which right now does a weird thing of
trying to recover the net error code via the CertStatus (which it's passed). It
was something that dadrian@, estark@, davidben@ and I had been discussing, but I
didn't share that context on this CL, apologies
3) Introduce the enterprise policy glue that wires up a RequireCTDelegate in the
IOThread for the profile, so that enterprises can whitelist some hosts from the
Symantec CT policy
4) Introduce the CL that actually enforces the Symantec policy

5) A bunch of various cleanups found while working on the parent patchset making
CT required (things like CT logs not being added, making sure the CT policies
are correct for non-Chrome, making sure mobile-CT is working, etc)

The ordering of the CLs is [1], [2, 3], [4] - meaning that 2/3 are parallel, but
block 4 from landing - but not blocking it from submitting.

[mmenke, 2016-06-20 21:07:52]

On 2016/06/20 20:33:49, Ryan Sleevi wrote:
> On 2016/06/20 20:18:31, mmenke wrote:
> > One other question:  Is there a reason we're showing a net error page
instead
> of
> > an interstitial on this error?
> 
> So, nothing actually triggers this error, which is why I wasn't touching the
UI
> aspect yet, but apologies for not being clearer on that.
> 
> Knowing that UI reviews can be a pain, my plan had been to touch on several
> aspects of this concurrently, to try to streamline CLs
> 
> 1) This CL, which introduces the //net error code, but nothing in the codebase
> actually triggers it (that's why the TSS default code returns false, and
there's
> no delegate wired up)
> 2) Introduce the error handling page. In particular, this is being done as a
net
> error code rather than a cert status, so it will require a little bit more
care
> to surface it up to the interstitial code, which right now does a weird thing
of
> trying to recover the net error code via the CertStatus (which it's passed).
It
> was something that dadrian@, estark@, davidben@ and I had been discussing, but
I
> didn't share that context on this CL, apologies
> 3) Introduce the enterprise policy glue that wires up a RequireCTDelegate in
the
> IOThread for the profile, so that enterprises can whitelist some hosts from
the
> Symantec CT policy
> 4) Introduce the CL that actually enforces the Symantec policy
> 
> 5) A bunch of various cleanups found while working on the parent patchset
making
> CT required (things like CT logs not being added, making sure the CT policies
> are correct for non-Chrome, making sure mobile-CT is working, etc)
> 
> The ordering of the CLs is [1], [2, 3], [4] - meaning that 2/3 are parallel,
but
> block 4 from landing - but not blocking it from submitting.

SGTM.  Thanks for the details!

[mmenke, 2016-06-20 21:08:03]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[davidben, 2016-06-20 22:04:34]

On 2016/06/20 20:33:49, Ryan Sleevi wrote:
> On 2016/06/20 20:18:31, mmenke wrote:
> > One other question:  Is there a reason we're showing a net error page
instead
> of
> > an interstitial on this error?
> 
> So, nothing actually triggers this error, which is why I wasn't touching the
UI
> aspect yet, but apologies for not being clearer on that.
> 
> Knowing that UI reviews can be a pain, my plan had been to touch on several
> aspects of this concurrently, to try to streamline CLs
> 
> 1) This CL, which introduces the //net error code, but nothing in the codebase
> actually triggers it (that's why the TSS default code returns false, and
there's
> no delegate wired up)
> 2) Introduce the error handling page. In particular, this is being done as a
net
> error code rather than a cert status, so it will require a little bit more
care
> to surface it up to the interstitial code, which right now does a weird thing
of
> trying to recover the net error code via the CertStatus (which it's passed).
It
> was something that dadrian@, estark@, davidben@ and I had been discussing, but
I
> didn't share that context on this CL, apologies

By interstitial, did you mean for this to be a bypassable error? We don't
actually have any means right now to make random net errors bypassable. Only
stuff that flows through OnCertificateError and friends.

> 3) Introduce the enterprise policy glue that wires up a RequireCTDelegate in
the
> IOThread for the profile, so that enterprises can whitelist some hosts from
the
> Symantec CT policy
> 4) Introduce the CL that actually enforces the Symantec policy
> 
> 5) A bunch of various cleanups found while working on the parent patchset
making
> CT required (things like CT logs not being added, making sure the CT policies
> are correct for non-Chrome, making sure mobile-CT is working, etc)
> 
> The ordering of the CLs is [1], [2, 3], [4] - meaning that 2/3 are parallel,
but
> block 4 from landing - but not blocking it from submitting.

[mmenke, 2016-06-20 22:07:13]

On 2016/06/20 22:04:34, davidben wrote:
> On 2016/06/20 20:33:49, Ryan Sleevi wrote:
> > On 2016/06/20 20:18:31, mmenke wrote:
> > > One other question:  Is there a reason we're showing a net error page
> instead
> > of
> > > an interstitial on this error?
> > 
> > So, nothing actually triggers this error, which is why I wasn't touching the
> UI
> > aspect yet, but apologies for not being clearer on that.
> > 
> > Knowing that UI reviews can be a pain, my plan had been to touch on several
> > aspects of this concurrently, to try to streamline CLs
> > 
> > 1) This CL, which introduces the //net error code, but nothing in the
codebase
> > actually triggers it (that's why the TSS default code returns false, and
> there's
> > no delegate wired up)
> > 2) Introduce the error handling page. In particular, this is being done as a
> net
> > error code rather than a cert status, so it will require a little bit more
> care
> > to surface it up to the interstitial code, which right now does a weird
thing
> of
> > trying to recover the net error code via the CertStatus (which it's passed).
> It
> > was something that dadrian@, estark@, davidben@ and I had been discussing,
but
> I
> > didn't share that context on this CL, apologies
> 
> By interstitial, did you mean for this to be a bypassable error? We don't
> actually have any means right now to make random net errors bypassable. Only
> stuff that flows through OnCertificateError and friends.

Yea, this seems like a cert error, so an error that LOAD_BYPASS_CERT_ERRORS or
whatever should bypass, or does bypassing an interstitial follow a different
path for bypassing the error?

> > 3) Introduce the enterprise policy glue that wires up a RequireCTDelegate in
> the
> > IOThread for the profile, so that enterprises can whitelist some hosts from
> the
> > Symantec CT policy
> > 4) Introduce the CL that actually enforces the Symantec policy
> > 
> > 5) A bunch of various cleanups found while working on the parent patchset
> making
> > CT required (things like CT logs not being added, making sure the CT
policies
> > are correct for non-Chrome, making sure mobile-CT is working, etc)
> > 
> > The ordering of the CLs is [1], [2, 3], [4] - meaning that 2/3 are parallel,
> but
> > block 4 from landing - but not blocking it from submitting.

[Ryan Sleevi, 2016-06-20 23:00:44]

On 2016/06/20 22:07:13, mmenke wrote:
> Yea, this seems like a cert error, so an error that LOAD_BYPASS_CERT_ERRORS or
> whatever should bypass, or does bypassing an interstitial follow a different
> path for bypassing the error?

Yeah, discussed more with David via GVC after realizing how much code was
hinging on IsCertStatusMinorError. The IsCertificateError code (which is where
HPKP errors are stuffed right now) triggers a different special snowflake path
that just 'happens' to work for PKP because of incidental side-effects rather
than intentional design, and those side-effects would not trigger for here.

That said, CertStatus and NetErrors are intrinsically coupled right now, so the
NetError still remains, but a new CertStatus major error will be introduced to
map between the two and keep the interstitial code happy (to avoid needing to
refactor that whole can of worms)

And the last bit of "Blah" is that we're out of the reserved bits for major
errors, and they're persisted, so I'll need to resurrect one of the CLs I had
thrown Wan-Teh back in the day.

The impact to *this* CL is a few more tweaks to CertStatus in this CL, but
otherwise the approach outlined remains mostly the same.

[Ryan Sleevi, 2016-06-21 00:06:30]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures that both public key
pins and CT information are checked in parallel, but that the PKP error is
treated as more serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
==========

[Ryan Sleevi, 2016-06-21 00:09:34]

OK, this should be ready for David's proper review - it introduces the extra
CertStatus (I opted for the lazy-path of using the high-high bits of CertStatus,
although extending it to 64-bit || coming up with a better HTTP-cache-expiration
strategy was also something I'd like to explore independently in the future)

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

https://codereview.chromium.org/2076363002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1823: if
(ct_verify_result_.cert_policy_compliance !=
I know moving around can be a pain, sorry about that. This is the only new
code/change in this section.

[davidben, 2016-06-22 21:44:59]

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

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:75: DEFAULT,
To confirm, the reason DEFAULT is a notion is because TSS will have internal
logic? Is the intent that Expect-CT and Symantec bits be implemented in TSS and
the admin policy bits be in the delegate?

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:81: virtual CTRequirementLevel
IsCTRequiredForHost(
Nit: A little weird to have an IsFoo function return something other than a
boolean. Although GetRequirementLevelForHost() is kinda wordy, so maybe this is
better?

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:536: bool enable_static_pins_;
Why make the pointers = nullptr while the bools are set in the constructor? (I'm
unclear on when we prefer to do which.)

https://codereview.chromium.org/2076363002/diff/60001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/2076363002/diff/60001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:306: int
ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
[Wow, I hadn't realized there was this much duplicated code. :-) Now I'm very
convinced that all this stuff needs to get bundled behind some abstraction, be
it CertVerifier or something else.]

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

https://codereview.chromium.org/2076363002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1351: result = ct_result;
SSLClientSocket-level test for all this? Particularly the part where HPKP
continues to be treated as more serious. (Maybe also on the QUIC side since the
logic is, sadly, currently duplicated. Though if we un-duplicate it...)

https://codereview.chromium.org/2076363002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1824:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS &&
To confirm: it is intentional that CERT_POLICY_COMPLIES_VIA_WHITELIST is not
sufficient here.

[Ryan Sleevi, 2016-06-22 22:07:39]

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

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:75: DEFAULT,
On 2016/06/22 21:44:59, davidben wrote:
> To confirm, the reason DEFAULT is a notion is because TSS will have internal
> logic? Is the intent that Expect-CT and Symantec bits be implemented in TSS
and
> the admin policy bits be in the delegate?

Yes (or eventually the CertVerifier)

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:81: virtual CTRequirementLevel
IsCTRequiredForHost(
On 2016/06/22 21:44:59, davidben wrote:
> Nit: A little weird to have an IsFoo function return something other than a
> boolean. Although GetRequirementLevelForHost() is kinda wordy, so maybe this
is
> better?

It's just a tri-state bool ;)

( http://thedailywtf.com/articles/tri-state-boolean )

It's "Yes", "No", "#Yolo"

https://codereview.chromium.org/2076363002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:536: bool enable_static_pins_;
On 2016/06/22 21:44:59, davidben wrote:
> Why make the pointers = nullptr while the bools are set in the constructor?
(I'm
> unclear on when we prefer to do which.)

I haven't seen any clear guidance in any of the style guides, but the style I
was adopting was trying to be reflective of the policies regarding forward
declarations or sticking implementation details in unnamed namespaces:

What's in the header is, effectively, a contract - and a reason for your
dependencies to change. In this case, I didn't think that dependents should need
to be recompiled if we change how enable_static_* behaves (and, to be fair, it
changes in the ctor depending on compile flags that I don't think we want
leaking into the .h), but the assumption is we'd never "not" null-initialize the
pointers, so we won't end up churning these.

https://codereview.chromium.org/2076363002/diff/60001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/2076363002/diff/60001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:306: int
ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
On 2016/06/22 21:44:59, davidben wrote:
> [Wow, I hadn't realized there was this much duplicated code. :-) Now I'm very
> convinced that all this stuff needs to get bundled behind some abstraction, be
> it CertVerifier or something else.]

Yup. I agree :)

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

https://codereview.chromium.org/2076363002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1351: result = ct_result;
On 2016/06/22 21:44:59, davidben wrote:
> SSLClientSocket-level test for all this? Particularly the part where HPKP
> continues to be treated as more serious. (Maybe also on the QUIC side since
the
> logic is, sadly, currently duplicated. Though if we un-duplicate it...)

Sure

https://codereview.chromium.org/2076363002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1824:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS &&
On 2016/06/22 21:44:59, davidben wrote:
> To confirm: it is intentional that CERT_POLICY_COMPLIES_VIA_WHITELIST is not
> sufficient here.

There is no CERT_POLICY_COMPLIES_VIA_WHITELIST - there's only a whitelist for
the EV policy, not the base cert policy. That's part of the CTPolicyEnforcer /
CTPolicyStatus documentation.

[Ryan Sleevi, 2016-06-23 00:02:40]

David: Added more tests :) Always happy for pushback like that ;)

[davidben, 2016-06-23 19:51:43]

lgtm

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
File net/quic/crypto/proof_verifier_chromium_test.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium_test.cc:312:
Return(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS));
Just to confirm, clobbering the EXPECT_CALL in SetUp like this is how GMock is
intended to work? (Other option I can see is to use a different
MockCTPolicyEnforcer as the old version did.)

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium_test.cc:505:
CTRequirementLevel::NOT_REQUIRED));
Does this line do anything?

https://codereview.chromium.org/2076363002/diff/120001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3382:
CTRequirementLevel::NOT_REQUIRED));
Is this line needed?

https://codereview.chromium.org/2076363002/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3433:
CTRequirementLevel::NOT_REQUIRED));
Ditto.

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:5763:
.WillRepeatedly(Return(CTRequirementLevel::NOT_REQUIRED));
Only mail.example.org is queried, right?

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:5790:
.WillRepeatedly(Return(CTRequirementLevel::NOT_REQUIRED));
Ditto.

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:5817:
.WillRepeatedly(Return(CTRequirementLevel::NOT_REQUIRED));
Ditto.

[Ryan Sleevi, 2016-06-23 21:38:31]

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
File net/quic/crypto/proof_verifier_chromium_test.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium_test.cc:312:
Return(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS));
On 2016/06/23 19:51:43, davidben wrote:
> Just to confirm, clobbering the EXPECT_CALL in SetUp like this is how GMock is
> intended to work? (Other option I can see is to use a different
> MockCTPolicyEnforcer as the old version did.)

Yup.
https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m...
covers this in the intro to GMock

https://codereview.chromium.org/2076363002/diff/120001/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium_test.cc:505:
CTRequirementLevel::NOT_REQUIRED));
On 2016/06/23 19:51:43, davidben wrote:
> Does this line do anything?

Objectively, no. It just means that if the hostname should ever change from
kTestHostname, the test will fail. That is, it guards against any implicit or
default mode by making explicit what happens for any hostname other than
kTestHostname (the line 506 expectation).

So it's really documentation more than anything - and guaranteeing the
preconditions.

https://codereview.chromium.org/2076363002/diff/120001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3382:
CTRequirementLevel::NOT_REQUIRED));
On 2016/06/23 19:51:43, davidben wrote:
> Is this line needed?

Same comment as from other; sets up a default expectation for any other
unmatched calls (line 3385). This is documenting preconditions/behaviours more
than anything.

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2076363002/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:5763:
.WillRepeatedly(Return(CTRequirementLevel::NOT_REQUIRED));
On 2016/06/23 19:51:43, davidben wrote:
> Only http://mail.example.org is queried, right?

Correct, but nothing in the interface contract of SpdySession::CanPool states
that, so I instead am treating it as a blackbox and testing the observables.

[Ryan Sleevi, 2016-06-23 21:39:01]

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

[Ryan Sleevi, 2016-06-23 21:39:01]

Going to TBR Eugene because this is a no-op change to iOS

[Ryan Sleevi, 2016-06-23 21:39:15]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
TBR=eugenebut@chromium.org
==========

[Ryan Sleevi, 2016-06-23 21:39:29]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-23 21:39:29]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2076363002/#ps120001
(title: "CanPool tests")

[commit-bot: I haz the power, 2016-06-23 21:39:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076363002/120001

[Eugene But (OOO till 7-30), 2016-06-23 21:46:10]

ios lgtm

[commit-bot: I haz the power, 2016-06-23 22:02:59]

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

[commit-bot: I haz the power, 2016-06-23 22:03:00]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ryan Sleevi, 2016-06-23 22:40:15]

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

[commit-bot: I haz the power, 2016-06-23 22:40:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076363002/140001

[commit-bot: I haz the power, 2016-06-23 23:03:26]

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

[commit-bot: I haz the power, 2016-06-23 23:03:27]

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

[Ryan Sleevi, 2016-06-23 23:12:20]

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

[commit-bot: I haz the power, 2016-06-23 23:13:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076363002/160001

[Ryan Sleevi, 2016-06-24 01:03:13]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org
TBR=eugenebut@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org
==========

[commit-bot: I haz the power, 2016-06-24 02:29:33]

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

[commit-bot: I haz the power, 2016-06-24 02:29:35]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-06-24 02:40:12]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-24 02:40:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, davidben@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2076363002/#ps160001
(title: "Android is weird")

[commit-bot: I haz the power, 2016-06-24 02:40:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076363002/160001

[commit-bot: I haz the power, 2016-06-24 03:05:35]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org
==========

[commit-bot: I haz the power, 2016-06-24 03:05:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-24 03:07:11]

Description was changed from

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org
==========

to

==========
Introduce the ability to require CT for specific hosts

Add the ability for TransportSecurityState to determine
if a host/certificate/public key hashes is required to
supply valid Certificate Transparency information. If so,
cause the connection to fail with
ERR_SSL_CERTIFICATE_REQUIRED (even when using QUIC).

To override the TSS policy decisions with custom logic,
this adds the ability to set a RequireCTDelegate on the
TSS, which allows hosts to be opted-in or opted-out of
the CT requirement.

To support this change in enforcement, this also ensures
that both public key pins and CT information are checked
in parallel, but that the PKP error is treated as more
serious than the CT error.

BUG=621252
R=davidben@chromium.org, estark@chromium.org, eugenebut@chromium.org

Committed: https://crrev.com/4a6ca8c5929a170798ad87339fb070361c2a3777
Cr-Commit-Position: refs/heads/master@{#401801}
==========

[commit-bot: I haz the power, 2016-06-24 03:07:13]

Patchset 9 (id:??) landed as
https://crrev.com/4a6ca8c5929a170798ad87339fb070361c2a3777
Cr-Commit-Position: refs/heads/master@{#401801}

[eroman, 2016-07-14 18:37:44]

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

[eroman, 2016-07-14 18:37:45]

FYI, run tools/metrics/histograms/update_net_error_codes.py after adding a code
to net_error_list.h

Perhaps that should be added as a presubmit check...

[davidben, 2016-07-25 19:08:02]

On 2016/07/14 18:37:45, eroman wrote:
> FYI, run tools/metrics/histograms/update_net_error_codes.py after adding a
code
> to net_error_list.h
> 
> Perhaps that should be added as a presubmit check...

I suggested this at some point (forget where), but people objected. Though maybe
people won't object this time around? I'd be for it.