Implement a skeleton version of Expect CT reports

Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806
Committed: https://crrev.com/1614475fe5a3366df2c4b73e33cf17e89d8b3a72
Cr-Commit-Position: refs/heads/master@{#380322}

[estark, 2016-01-12 02:43:23]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. The changes in this CL are:

1. Run CertPolicyEnforcer on all requests that pass certificate
validation, not just for EV certs. While a policy violation only
actually affects EV certs by stripping them of their EV status, we want
to run CertPolicyEnforcer so that we can report would-be violations even
for non-EV certs.

2. Introduce a new chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations, as reported by
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. The changes in this CL are:

1. Run CertPolicyEnforcer on all requests that pass certificate
validation, not just for EV certs. While a policy violation only
actually affects EV certs by stripping them of their EV status, we want
to run CertPolicyEnforcer so that we can report would-be violations even
for non-EV certs.

2. Introduce a new chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

BUG=568806
==========

[estark, 2016-01-12 02:54:15]

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

[estark, 2016-01-12 02:54:16]

Hey Ryan -- this is a bit of a rough draft but I wanted to get some feedback on
the overall approach and what code you think should live where. Right now I have
an expect-CT reporter living in chrome/, and learning about CT policy violations
via ChromeNetworkDelegate. The reporter will query TransportSecurityState to
learn what sites are on the expect-CT preload list.

Caveats/to-do list:
- There's a circular dependency between the main URLRequestContext and the
ExpectCTMonitor that I have to clean up but it's a bit tricky; I suspect mmenke@
might be able to help me figure something out.
- Only has one very basic test right now, wanted to get some feedback on the
overall direction before going further.
- I put an ExpectCTMonitor interface in chrome/browser/net with an
implementation in chrome/browser/ssl, but on second thought, maybe that's
over-complicating it and I should just make one class with the whole thing in
chrome/browser/net?

Thanks!

[Ryan Sleevi, 2016-01-12 04:39:12]

Brief high-level suggestion: Let's move the CT policy enforcement into a
separate CL, clean-up the method name, update the documentation, and make a
decision as to whether or not a policy enforcer is necessary.

(Also, I wonder if we should rename it to be CTPolicyEnforcer - especially with
the upcoming CNNIC & Symantec CT policies)

Does that sound like a good starter path?

https://codereview.chromium.org/1579063002/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (left):

https://codereview.chromium.org/1579063002/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:287: if (result == OK &&
policy_enforcer_ &&
BUG: policy_enforcer_ remains optional for embedders

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

https://codereview.chromium.org/1579063002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3131: if
(!policy_enforcer_->DoesConformToCTEVPolicy(
Seems like this method should be renamed?

[Ryan Sleevi, 2016-01-12 04:41:20]

https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:503: }
This feels like a weird place to put this.

Is there a reason you don't see yourself super-classing
CTPolicyEnforcer/CertPolicyEnforcer and applying the checks there? Why do you
track by |request| versus, say, connection?

Just trying to think about the timing and implications, since this may result in
significantly more reports for a single connection (since they may be re-used,
and each and every connection will be invalid), and it's different than how,
say, we handle HPKP (which is enforced at socket connect time)

I'm just curious why this implementation wouldn't follow the same basic skeleton
of HPKP & reporting.

[estark, 2016-01-12 05:05:24]

Thanks, that sounds good about pulling out the policy enforcer stuff into a
separate CL -- in the meantime just wanted to respond to the question about
sending reports at request level rather than connection level (see inline).

https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:503: }
On 2016/01/12 04:41:20, Ryan Sleevi wrote:
> This feels like a weird place to put this.
> 
> Is there a reason you don't see yourself super-classing
> CTPolicyEnforcer/CertPolicyEnforcer and applying the checks there? Why do you
> track by |request| versus, say, connection?
> 
> Just trying to think about the timing and implications, since this may result
in
> significantly more reports for a single connection (since they may be re-used,
> and each and every connection will be invalid), and it's different than how,
> say, we handle HPKP (which is enforced at socket connect time)
> 
> I'm just curious why this implementation wouldn't follow the same basic
skeleton
> of HPKP & reporting.

This feature is more like PKP-Report-Only: we have to send CT reports at the
time that we process HTTP headers rather than socket connect time, because we
want to check for the `Expect-CT: preload` header before sending a report. So I
don't think we can do the whole thing inside
CertPolicyEnforcer::DoesConformToCTEVPolicy() (or whatever its name will change
to) because we don't know the response headers at the time that that's called.

[estark, 2016-01-12 20:53:17]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. The changes in this CL are:

1. Run CertPolicyEnforcer on all requests that pass certificate
validation, not just for EV certs. While a policy violation only
actually affects EV certs by stripping them of their EV status, we want
to run CertPolicyEnforcer so that we can report would-be violations even
for non-EV certs.

2. Introduce a new chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new chrome/browser/net
interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

This is based on https://codereview.chromium.org/1578993003/

BUG=568806
==========

[estark, 2016-01-12 20:53:32]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new chrome/browser/net
interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

This is based on https://codereview.chromium.org/1578993003/

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

This is based on https://codereview.chromium.org/1578993003/

BUG=568806
==========

[estark, 2016-01-12 20:58:44]

Alright, I pulled out some stuff in two other CLs, so that this CL can be just
about how and when we trigger report sending.

#1: https://codereview.chromium.org/1579233002/ (Rename CertPolicyEnforcer to
CTPolicyEnforcer)
#2: https://codereview.chromium.org/1578993003/ (Rename the method that checks
CT policy and call it for all certs, not just EV)
#3: this one

[estark, 2016-02-27 01:30:41]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

This is based on https://codereview.chromium.org/1578993003/

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

BUG=568806
==========

[estark, 2016-02-27 01:35:09]

I just updated this now that the cert policy compliance stuff is landed. Could
you please take another look, Ryan?

I'm sort of following the DomainReliabilityMonitor pattern by making an
ExpectCTMonitor that gets called by ChromeNetworkDelegate. Another option might
be to do something like what FraudulentCertReporter used to do when it was alive
and well... that is, have a net::ExpectCTReporter interface, with an
implementation in chrome/browser/ssl, where URLRequestHTTPJob has a pointer to
an ExpectCTReporter and notifies the ExpectCTReporter whenever CT cert policy is
violated for a request.

[Ryan Sleevi, 2016-02-27 01:41:02]

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

[Ryan Sleevi, 2016-02-27 01:41:03]

Matt should comment on the ChromeNetworkDelegate. IMO, it's an anti-pattern, and
reflects the tendency of Chrome code to lump dependencies into giant objects
rather than thread em through.

My gut is that this sort of checking / reporting probably belongs lower, in
//net, at the URLRequest layer - I'm not a big fan of adding logic to the
NetworkDelegate, especially if it's behaviour other ports would want (and given
that the CT compliance checking is now in //net, this seems to be OK)

[mmenke, 2016-02-29 18:40:56]

On 2016/02/27 01:41:03, Ryan Sleevi wrote:
> Matt should comment on the ChromeNetworkDelegate. IMO, it's an anti-pattern,
and
> reflects the tendency of Chrome code to lump dependencies into giant objects
> rather than thread em through.
> 
> My gut is that this sort of checking / reporting probably belongs lower, in
> //net, at the URLRequest layer - I'm not a big fan of adding logic to the
> NetworkDelegate, especially if it's behaviour other ports would want (and
given
> that the CT compliance checking is now in //net, this seems to be OK)

Hrm...  Both the URLRequestContext and ChromeNetworkDelegate are pretty ugly and
bloaty, hooking up a bunch of random things that do random stuff.  In this case,
I think the extra checks make a lot of sense in net/, since all the other CT
stuff is there, and having to check in two different places is crying out for a
call from URLRequestHttpJob::OnStartCompleted or 
URLRequestJob::NotifyHeadersComplete.  In URLRequestHttpJob::OnStartCompleted we
catch all cases, including those that are cancelled or fail for whatever reason
(like due to cert errors) before they make it to the other NetworkDelegate
methods, so that may be the best way to go.  I assume we don't care about FTP
through explicitly configured SSL proxies.  Not sure we get the SSL info in that
case, anyways.

I think either we should move this into the URLRequestContext, or we should add
yet another method to NetworkDelegate for this.  I don't have strong feelings
about which one we do -- I view them both as aggregations of cruft.  Since Ryan
has a preference here, happy to defer to him.

[estark, 2016-03-01 16:17:24]

Patchset #7 (id:120001) has been deleted

[estark, 2016-03-01 16:24:41]

Ok, thanks Matt and Ryan. Here's what I have now:

TransportSecurityState processes the Expect-CT header (when URLRequestHTTPJob
tells it to) and defines an interface for handling violations. I used
TransportSecurityState because it already knows about Expect CT (handles the
entries on the preload list), and because it roughly matches how HPKP
report-only mode works, except that for HPKP the report-building logic is also
in TransportSecurityState. Maybe we'll eventually want to have most of the
Expect CT reporting logic in //net as well, but I assume for now that Chrome is
the only embedder that actually wants to build and send these reports, so I'm
planning to implement the actual report building and sending in
ChromeExpectCTReporter (it's just a stub at the moment).

[estark, 2016-03-01 18:18:19]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
chrome/browser/net interface called ExpectCTMonitor
for observing CT policy violations that pass through
ChromeNetworkDelegate. There is a skeleton implementation called
ExpectCTReporter in chrome/browser/ssl which will eventually use a
net::CertificateReportSender to send reports.

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806
==========

[estark, 2016-03-02 20:31:55]

friendly ping to Ryan

And mmenke maybe you could look at the ProfileIOData parts of the latest
patchset?

Thanks!

[Ryan Sleevi, 2016-03-04 01:25:17]

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

[Ryan Sleevi, 2016-03-04 01:25:18]

Sadly, needing to punt to eroman because I'm gonna be OOO until Thurs

[Eran Messeri, 2016-03-04 11:05:36]

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

[Eran Messeri, 2016-03-04 11:05:37]

Looks good to me (drive-by review), with one minor comment.

https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_sec...
net/http/transport_security_state.cc:1031: if (!expect_ct_reporter_ || value !=
"preload" || !IsBuildTimely() ||
Nit: very long condition, I suggest breaking it down to individual if
statements.

[mmenke, 2016-03-07 18:28:08]

profiles/ LGTM

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:11: #include
"net/url_request/url_request_context.h"
The last two includes are not currently used in this file, and I don't think the
url_request or the url_request_context is likely to be used by this class (The
certificate_report_sender will presumably be the thing using them).

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:13:
ChromeExpectCTReporter::ChromeExpectCTReporter(
Is there going to be anything Chrome specific about this class?  It's currently
a chrome/browser class that links the
net::TransportSecurityState::ExpectCTReporter API with a
net::CertificateReportSender.

Given that those are both in net/, unless there's going to be some
Chrome-specific magic, seems like it belongs in there as well.  Unless it also
talks to Google server or something, though then I'd suggest putting the core
logic in net/, and adding an observer of some sort or something.

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.h (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:18: // about failures for sites
that have opted in.
Mention it must be deleted before the URLRequestContext, so it can cancel its
requests?

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:32:
DISALLOW_COPY_AND_ASSIGN(ChromeExpectCTReporter);
nit:  Blank line before DISALLOW_COPY_AND_ASSIGN(ChromeExpectCTReporter);

https://codereview.chromium.org/1579063002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:864: return;
nit:  Use braces if an if condition takes up multiple lines.

[estark, 2016-03-07 22:20:32]

Hi Matt and Eran, thanks for taking a look. No code changes yet, just a reply to
Matt's question inline.

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:13:
ChromeExpectCTReporter::ChromeExpectCTReporter(
On 2016/03/07 18:28:08, mmenke wrote:
> Is there going to be anything Chrome specific about this class?  It's
currently
> a chrome/browser class that links the
> net::TransportSecurityState::ExpectCTReporter API with a
> net::CertificateReportSender.
> 
> Given that those are both in net/, unless there's going to be some
> Chrome-specific magic, seems like it belongs in there as well.  Unless it also
> talks to Google server or something, though then I'd suggest putting the core
> logic in net/, and adding an observer of some sort or something.

I did it this way (ChromeExpectCTReporter instead of all the logic in //net)
because, initially, Expect CT is going to be a pretty Googley feature while it's
in a sort of prototype phase. We're not yet specifying or standardizing it any
remotely formal way, and initially, we're only going to allow reports to be sent
to Google servers (so that we can, for example, tweak the policies and reporting
logic without worrying about DoSing other site owners). I was thinking that
other embedders would probably not want this feature in its current immature
state, and the more report-building and -sending logic we include in //net, the
more wasteful this feature will be for them. So I was imagining that all the
logic would end up in //net as the feature "grows up" a little bit, but not
while it's in this rather Google-specific prototype phase. What do you think?

[mmenke, 2016-03-07 23:08:12]

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:13:
ChromeExpectCTReporter::ChromeExpectCTReporter(
On 2016/03/07 22:20:32, estark wrote:
> On 2016/03/07 18:28:08, mmenke wrote:
> > Is there going to be anything Chrome specific about this class?  It's
> currently
> > a chrome/browser class that links the
> > net::TransportSecurityState::ExpectCTReporter API with a
> > net::CertificateReportSender.
> > 
> > Given that those are both in net/, unless there's going to be some
> > Chrome-specific magic, seems like it belongs in there as well.  Unless it
also
> > talks to Google server or something, though then I'd suggest putting the
core
> > logic in net/, and adding an observer of some sort or something.
> 
> I did it this way (ChromeExpectCTReporter instead of all the logic in //net)
> because, initially, Expect CT is going to be a pretty Googley feature while
it's
> in a sort of prototype phase. We're not yet specifying or standardizing it any
> remotely formal way, and initially, we're only going to allow reports to be
sent
> to Google servers (so that we can, for example, tweak the policies and
reporting
> logic without worrying about DoSing other site owners). I was thinking that
> other embedders would probably not want this feature in its current immature
> state, and the more report-building and -sending logic we include in //net,
the
> more wasteful this feature will be for them. So I was imagining that all the
> logic would end up in //net as the feature "grows up" a little bit, but not
> while it's in this rather Google-specific prototype phase. What do you think?

Hrm...Then it seems a little weird that CertificateReportSender is in net/. 
Anyhow, if this is just to talk to Google, having it here makes sense.

[estark, 2016-03-08 02:36:06]

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:11: #include
"net/url_request/url_request_context.h"
On 2016/03/07 18:28:08, mmenke wrote:
> The last two includes are not currently used in this file, and I don't think
the
> url_request or the url_request_context is likely to be used by this class (The
> certificate_report_sender will presumably be the thing using them).

Done.

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:11: #include
"net/url_request/url_request_context.h"
On 2016/03/07 18:28:08, mmenke wrote:
> The last two includes are not currently used in this file, and I don't think
the
> url_request or the url_request_context is likely to be used by this class (The
> certificate_report_sender will presumably be the thing using them).

Done.

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:13:
ChromeExpectCTReporter::ChromeExpectCTReporter(
On 2016/03/07 23:08:12, mmenke wrote:
> On 2016/03/07 22:20:32, estark wrote:
> > On 2016/03/07 18:28:08, mmenke wrote:
> > > Is there going to be anything Chrome specific about this class?  It's
> > currently
> > > a chrome/browser class that links the
> > > net::TransportSecurityState::ExpectCTReporter API with a
> > > net::CertificateReportSender.
> > > 
> > > Given that those are both in net/, unless there's going to be some
> > > Chrome-specific magic, seems like it belongs in there as well.  Unless it
> also
> > > talks to Google server or something, though then I'd suggest putting the
> core
> > > logic in net/, and adding an observer of some sort or something.
> > 
> > I did it this way (ChromeExpectCTReporter instead of all the logic in //net)
> > because, initially, Expect CT is going to be a pretty Googley feature while
> it's
> > in a sort of prototype phase. We're not yet specifying or standardizing it
any
> > remotely formal way, and initially, we're only going to allow reports to be
> sent
> > to Google servers (so that we can, for example, tweak the policies and
> reporting
> > logic without worrying about DoSing other site owners). I was thinking that
> > other embedders would probably not want this feature in its current immature
> > state, and the more report-building and -sending logic we include in //net,
> the
> > more wasteful this feature will be for them. So I was imagining that all the
> > logic would end up in //net as the feature "grows up" a little bit, but not
> > while it's in this rather Google-specific prototype phase. What do you
think?
> 
> Hrm...Then it seems a little weird that CertificateReportSender is in net/. 
> Anyhow, if this is just to talk to Google, having it here makes sense.

net::CertificateReportSender is used by other things as well, including HPKP
reporting (implemented entirely in //net) and Safe Browsing Extended Reporting
certificate reports (implemented in a component).

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.h (right):

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:18: // about failures for sites
that have opted in.
On 2016/03/07 18:28:08, mmenke wrote:
> Mention it must be deleted before the URLRequestContext, so it can cancel its
> requests?

Done.

https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:32:
DISALLOW_COPY_AND_ASSIGN(ChromeExpectCTReporter);
On 2016/03/07 18:28:08, mmenke wrote:
> nit:  Blank line before DISALLOW_COPY_AND_ASSIGN(ChromeExpectCTReporter);

Done.

https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_sec...
net/http/transport_security_state.cc:1031: if (!expect_ct_reporter_ || value !=
"preload" || !IsBuildTimely() ||
On 2016/03/04 11:05:37, Eran Messeri wrote:
> Nit: very long condition, I suggest breaking it down to individual if
> statements.

Done.

[estark, 2016-03-08 15:54:47]

friendly ping to eroman, would you be able to take a look at this today? (No
fault of yours but I've been waiting for a week to get a full review for this
and I'm eager to keep things moving along)

[eroman, 2016-03-08 22:10:17]

Yes looking now!

[eroman, 2016-03-08 23:40:47]

lgtm

nice test coverage!

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.h (right):

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
nit: 2016

[estark, 2016-03-08 23:46:32]

Thanks Eric!

palmer, could you please look at chrome/browser/ssl?

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/03/08 23:40:47, eroman wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter.h (right):

https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/03/08 23:40:47, eroman wrote:
> nit: 2016

Done.

[estark, 2016-03-08 23:47:05]

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

[estark, 2016-03-08 23:47:05]

Doh, let me try this again and actually add him this time... palmer, could you
please look at chrome/browser/ssl?

[palmer, 2016-03-09 20:32:26]

lgtm

[estark, 2016-03-10 02:03:51]

The CQ bit was checked by estark@chromium.org

[estark, 2016-03-10 02:03:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
eroman@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1579063002/#ps200001
(title: "remove unnecessary (?) NET_EXPORTs")

[commit-bot: I haz the power, 2016-03-10 02:04:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1579063002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1579063002/200001

[commit-bot: I haz the power, 2016-03-10 03:47:02]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806
==========

[commit-bot: I haz the power, 2016-03-10 03:47:03]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-03-10 03:48:32]

Description was changed from

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806
==========

to

==========
Implement a skeleton version of Expect CT reports

This CL implements the skeleton of Expect CT reporting, which will
eventually send reports when an opted-in site fails to conform to CT
policy as determined by CertPolicyEnforcer. Introduces a new
TransportSecurityState interface called ExpectCTReporter
for observing CT policy violations that pass through
URLRequestHTTPJob. There is a skeleton implementation called
ChromeExpectCTReporter in chrome/browser/ssl which will eventually
use a net::CertificateReportSender to send reports.

BUG=568806

Committed: https://crrev.com/1614475fe5a3366df2c4b73e33cf17e89d8b3a72
Cr-Commit-Position: refs/heads/master@{#380322}
==========

[commit-bot: I haz the power, 2016-03-10 03:48:34]

Patchset 10 (id:??) landed as
https://crrev.com/1614475fe5a3366df2c4b73e33cf17e89d8b3a72
Cr-Commit-Position: refs/heads/master@{#380322}

[Lei Zhang, 2016-03-10 06:13:15]

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

[Lei Zhang, 2016-03-10 06:13:16]

https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_re...
net/url_request/url_request_test_util.h:105: CTPolicyEnforcer*
ct_policy_enforcer_;
Umm, you forgot to initialize this, now lots of memory bots are red. I'll send a
CL...

[Lei Zhang, 2016-03-10 06:16:58]

On 2016/03/10 06:13:16, Lei Zhang (OOO) wrote:
>
https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_re...
> File net/url_request/url_request_test_util.h (right):
> 
>
https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_re...
> net/url_request/url_request_test_util.h:105: CTPolicyEnforcer*
> ct_policy_enforcer_;
> Umm, you forgot to initialize this, now lots of memory bots are red. I'll send
a
> CL...

https://codereview.chromium.org/1781663005/