Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 1579063002: Implement a skeleton version of Expect CT reports (Closed)

Created:
2 years, 8 months ago by estark
Modified:
2 years, 6 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, certificate-transparency-chrome_googlegroups.com
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : comments/cleanup #

Patch Set 3 : rebase #

Total comments: 4

Patch Set 4 : rebase on https://codereview.chromium.org/1578993003/ #

Patch Set 5 : rebase, bring up to date with master #

Patch Set 6 : remove log statement #

Patch Set 7 : move expect ct into TransportSecurityState #

Total comments: 14

Patch Set 8 : eranm, mmenke comments #

Total comments: 4

Patch Set 9 : eroman comments, minor unit test build fix #

Patch Set 10 : remove unnecessary (?) NET_EXPORTs #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+516 lines, -34 lines) Patch
M chrome/browser/profiles/profile_io_data.h View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 3 4 5 6 3 chunks +9 lines, -0 lines 0 comments Download
A chrome/browser/ssl/chrome_expect_ct_reporter.h View 1 2 3 4 5 6 7 8 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/browser/ssl/chrome_expect_ct_reporter.cc View 1 2 3 4 5 6 7 8 1 chunk +25 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
A + net/data/url_request_unittest/expect-ct-header.html View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
A + net/data/url_request_unittest/expect-ct-header.html.mock-http-headers View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M net/http/transport_security_state.h View 1 2 3 4 5 6 7 chunks +38 lines, -5 lines 0 comments Download
M net/http/transport_security_state.cc View 1 2 3 4 5 6 7 5 chunks +58 lines, -21 lines 0 comments Download
M net/http/transport_security_state_unittest.cc View 1 2 3 4 5 6 5 chunks +180 lines, -7 lines 0 comments Download
M net/ssl/ssl_info.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M net/url_request/url_request_http_job.h View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M net/url_request/url_request_http_job.cc View 1 2 3 4 5 6 7 2 chunks +23 lines, -0 lines 0 comments Download
M net/url_request/url_request_test_util.h View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 1 comment Download
M net/url_request/url_request_test_util.cc View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +122 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (16 generated)
estark
Hey Ryan -- this is a bit of a rough draft but I wanted to ...
2 years, 8 months ago (2016-01-12 02:54:16 UTC) #3
Ryan Sleevi
Brief high-level suggestion: Let's move the CT policy enforcement into a separate CL, clean-up the ...
2 years, 8 months ago (2016-01-12 04:39:12 UTC) #4
Ryan Sleevi
https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chrome_network_delegate.cc#newcode503 chrome/browser/net/chrome_network_delegate.cc:503: } This feels like a weird place to put ...
2 years, 8 months ago (2016-01-12 04:41:20 UTC) #5
estark
Thanks, that sounds good about pulling out the policy enforcer stuff into a separate CL ...
2 years, 8 months ago (2016-01-12 05:05:24 UTC) #6
estark
Alright, I pulled out some stuff in two other CLs, so that this CL can ...
2 years, 8 months ago (2016-01-12 20:58:44 UTC) #9
estark
I just updated this now that the cert policy compliance stuff is landed. Could you ...
2 years, 6 months ago (2016-02-27 01:35:09 UTC) #11
Ryan Sleevi
Matt should comment on the ChromeNetworkDelegate. IMO, it's an anti-pattern, and reflects the tendency of ...
2 years, 6 months ago (2016-02-27 01:41:03 UTC) #13
mmenke
On 2016/02/27 01:41:03, Ryan Sleevi wrote: > Matt should comment on the ChromeNetworkDelegate. IMO, it's ...
2 years, 6 months ago (2016-02-29 18:40:56 UTC) #14
estark
Ok, thanks Matt and Ryan. Here's what I have now: TransportSecurityState processes the Expect-CT header ...
2 years, 6 months ago (2016-03-01 16:24:41 UTC) #16
estark
friendly ping to Ryan And mmenke maybe you could look at the ProfileIOData parts of ...
2 years, 6 months ago (2016-03-02 20:31:55 UTC) #18
Ryan Sleevi
Sadly, needing to punt to eroman because I'm gonna be OOO until Thurs
2 years, 6 months ago (2016-03-04 01:25:18 UTC) #20
Eran Messeri
Looks good to me (drive-by review), with one minor comment. https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/1579063002/diff/140001/net/http/transport_security_state.cc#newcode1031 ...
2 years, 6 months ago (2016-03-04 11:05:37 UTC) #22
mmenke
profiles/ LGTM https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode11 chrome/browser/ssl/chrome_expect_ct_reporter.cc:11: #include "net/url_request/url_request_context.h" The last two includes are ...
2 years, 6 months ago (2016-03-07 18:28:08 UTC) #23
estark
Hi Matt and Eran, thanks for taking a look. No code changes yet, just a ...
2 years, 6 months ago (2016-03-07 22:20:32 UTC) #24
mmenke
https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode13 chrome/browser/ssl/chrome_expect_ct_reporter.cc:13: ChromeExpectCTReporter::ChromeExpectCTReporter( On 2016/03/07 22:20:32, estark wrote: > On 2016/03/07 ...
2 years, 6 months ago (2016-03-07 23:08:12 UTC) #25
estark
https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/1579063002/diff/140001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode11 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 ...
2 years, 6 months ago (2016-03-08 02:36:06 UTC) #26
estark
friendly ping to eroman, would you be able to take a look at this today? ...
2 years, 6 months ago (2016-03-08 15:54:47 UTC) #27
eroman
Yes looking now!
2 years, 6 months ago (2016-03-08 22:10:17 UTC) #28
eroman
lgtm nice test coverage! https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode1 chrome/browser/ssl/chrome_expect_ct_reporter.cc:1: // Copyright 2015 The Chromium ...
2 years, 6 months ago (2016-03-08 23:40:47 UTC) #29
estark
Thanks Eric! palmer, could you please look at chrome/browser/ssl? https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/1579063002/diff/160001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode1 chrome/browser/ssl/chrome_expect_ct_reporter.cc:1: ...
2 years, 6 months ago (2016-03-08 23:46:32 UTC) #30
estark
Doh, let me try this again and actually add him this time... palmer, could you ...
2 years, 6 months ago (2016-03-08 23:47:05 UTC) #32
palmer
lgtm
2 years, 6 months ago (2016-03-09 20:32:26 UTC) #33
commit-bot: I haz the power
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
2 years, 6 months ago (2016-03-10 02:04:24 UTC) #36
commit-bot: I haz the power
Committed patchset #10 (id:200001)
2 years, 6 months ago (2016-03-10 03:47:03 UTC) #38
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/1614475fe5a3366df2c4b73e33cf17e89d8b3a72 Cr-Commit-Position: refs/heads/master@{#380322}
2 years, 6 months ago (2016-03-10 03:48:34 UTC) #40
Lei Zhang
https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_request_test_util.h File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/1579063002/diff/200001/net/url_request/url_request_test_util.h#newcode105 net/url_request/url_request_test_util.h:105: CTPolicyEnforcer* ct_policy_enforcer_; Umm, you forgot to initialize this, now ...
2 years, 6 months ago (2016-03-10 06:13:16 UTC) #42
Lei Zhang
2 years, 6 months ago (2016-03-10 06:16:58 UTC) #43
Message was sent while issue was closed.
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/

Powered by Google App Engine
This is Rietveld 408576698