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

Issue 2040513003: Implement Expect-Staple (Closed)

Created:
3 years, 1 month ago by dadrian
Modified:
3 years ago
Reviewers:
svaldez, Ryan Sleevi, estark
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement Expect-Staple This adds the ability to send Expect-Staple reports for hosts in the Expect-Staple preload list. OCSP staples are only parsed for hosts on this list. Reporting is currently disabled, and only enabled in tests. BUG=598021

Patch Set 1 #

Total comments: 3

Patch Set 2 : Move into TransportSecurityState #

Patch Set 3 : Start writing tests #

Total comments: 45

Patch Set 4 : Make everything less ugly. #

Patch Set 5 : Remove call to GetSSLInfo #

Total comments: 20

Patch Set 6 : Address comments from estark@ #

Total comments: 2

Patch Set 7 : Don't report private certificates #

Total comments: 31

Patch Set 8 : Cleanup imports #

Total comments: 30

Patch Set 9 : Address comments from svaldez@ #

Patch Set 10 : Use new der::GeneralizedTime operators #

Total comments: 38

Patch Set 11 : Move OCSP into cert_verify_proc #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+674 lines, -21 lines) Patch
M net/cert/cert_verify_proc.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +75 lines, -0 lines 2 comments Download
M net/cert/cert_verify_result.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M net/cert/cert_verify_result.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M net/cert/internal/parse_ocsp.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -0 lines 1 comment Download
M net/cert/internal/parse_ocsp.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +26 lines, -0 lines 0 comments Download
A net/cert/ocsp_verify_result.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +65 lines, -0 lines 2 comments Download
A net/cert/ocsp_verify_result.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +21 lines, -0 lines 1 comment Download
M net/der/parse_values.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -0 lines 0 comments Download
M net/der/parse_values.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +17 lines, -0 lines 1 comment Download
M net/http/transport_security_state.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +25 lines, -0 lines 2 comments Download
M net/http/transport_security_state.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +86 lines, -0 lines 3 comments Download
M net/http/transport_security_state_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +123 lines, -1 line 0 comments Download
M net/net.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_impl.h View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -1 line 1 comment Download
M net/socket/ssl_client_socket_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +10 lines, -0 lines 0 comments Download
M net/ssl/ssl_info.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M net/ssl/ssl_info.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M net/test/spawned_test_server/base_test_server.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +17 lines, -0 lines 1 comment Download
M net/test/spawned_test_server/base_test_server.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +23 lines, -0 lines 0 comments Download
M net/tools/testserver/minica.py View 1 2 3 4 5 6 7 8 9 10 7 chunks +41 lines, -11 lines 1 comment Download
M net/tools/testserver/testserver.py View 1 2 3 4 5 6 7 8 9 10 2 chunks +17 lines, -0 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +98 lines, -7 lines 2 comments Download

Messages

Total messages: 36 (6 generated)
dadrian
This isn't intended to be merged, just looking for some thoughts. Also, I might have ...
3 years, 1 month ago (2016-06-03 19:05:18 UTC) #3
estark
https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc#newcode33 chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); On 2016/06/03 19:05:18, dadrian wrote: > So, a ...
3 years, 1 month ago (2016-06-04 00:39:17 UTC) #4
estark
https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc#newcode33 chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); On 2016/06/04 00:39:16, estark wrote: > On 2016/06/03 ...
3 years, 1 month ago (2016-06-04 00:40:43 UTC) #5
dadrian
On 2016/06/04 00:40:43, estark wrote: > https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc > File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): > > https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_expect_staple_reporter.cc#newcode33 > ...
3 years, 1 month ago (2016-06-07 20:59:37 UTC) #6
dadrian
Looking for a first round of feedback. This definitely needs more tests, and will have ...
3 years, 1 month ago (2016-06-08 22:42:43 UTC) #9
estark
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode45 net/http/transport_security_state.cc:45: struct OCSPStaple { could probably use some comments (in ...
3 years, 1 month ago (2016-06-09 21:24:15 UTC) #10
dadrian
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode45 net/http/transport_security_state.cc:45: struct OCSPStaple { On 2016/06/09 21:24:14, estark wrote: > ...
3 years, 1 month ago (2016-06-10 01:05:53 UTC) #11
svaldez
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode77 net/http/transport_security_state.cc:77: bool CheckOCSPDateValid(bool has_next_update, Add a comment to the RFC ...
3 years, 1 month ago (2016-06-13 14:03:04 UTC) #12
dadrian
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode758 net/http/transport_security_state.cc:758: expect_ct_reporter_(nullptr), On 2016/06/13 14:03:03, svaldez wrote: > Extra change? ...
3 years, 1 month ago (2016-06-13 23:03:32 UTC) #13
estark
Mostly nits, but I'm still a tad confused by the missing-nextUpdate/max-age thingy https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc ...
3 years, 1 month ago (2016-06-14 02:10:28 UTC) #14
dadrian
https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc File net/cert/ocsp_staple.cc (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc#newcode25 net/cert/ocsp_staple.cc:25: const base::Time verify_time, On 2016/06/14 02:10:27, estark wrote: > ...
3 years, 1 month ago (2016-06-14 18:40:01 UTC) #15
svaldez
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode87 net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < next_update); On 2016/06/14 02:10:27, estark ...
3 years, 1 month ago (2016-06-14 18:56:13 UTC) #16
dadrian
On 2016/06/14 18:56:13, svaldez wrote: > https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc > File net/http/transport_security_state.cc (right): > > https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_security_state.cc#newcode87 > ...
3 years, 1 month ago (2016-06-14 19:52:23 UTC) #17
estark
https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_security_state.cc#newcode1217 net/http/transport_security_state.cc:1217: report_dict.Set("served-certificate-chain", important note: only include this if |is_issued_by_known_root| is ...
3 years, 1 month ago (2016-06-15 02:38:37 UTC) #18
dadrian
https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_security_state.cc File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_security_state.cc#newcode1217 net/http/transport_security_state.cc:1217: report_dict.Set("served-certificate-chain", On 2016/06/15 02:38:37, estark wrote: > important note: ...
3 years, 1 month ago (2016-06-15 20:59:03 UTC) #19
estark
(note: I updated the issue description to obey the weird rule that the first line ...
3 years, 1 month ago (2016-06-15 23:51:47 UTC) #21
dadrian
https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple_report.cc File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple_report.cc#newcode6 net/cert/expect_staple_report.cc:6: On 2016/06/15 23:51:46, estark wrote: > missing a bunch ...
3 years, 1 month ago (2016-06-16 03:27:25 UTC) #22
svaldez
Missed that there was a new patchset (why can't we have Gerrits notifications.) https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple_report.cc File ...
3 years, 1 month ago (2016-06-16 11:14:38 UTC) #23
dadrian
https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple_report.cc File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple_report.cc#newcode34 net/cert/expect_staple_report.cc:34: !(response.this_update < response.next_update)) On 2016/06/16 11:14:38, svaldez wrote: > ...
3 years ago (2016-06-16 19:20:18 UTC) #24
svaldez
Probably reasonable to ask for a net OWNER to look over/stamp. I'm still a bit ...
3 years ago (2016-06-16 19:56:23 UTC) #25
dadrian
Now with new der::GeneralizedTime operators! Adding rsleevi@ as a reviewer, per conversation in person while ...
3 years ago (2016-06-16 21:36:18 UTC) #27
Ryan Sleevi
Kicking over to Emily because I think there's a lot of design concerns here that ...
3 years ago (2016-06-16 21:49:30 UTC) #28
svaldez
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h#newcode46 net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. ...
3 years ago (2016-06-17 13:33:51 UTC) #29
Ryan Sleevi
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h#newcode26 net/cert/expect_staple_report.h:26: // Stores the validity of a single stapled response. ...
3 years ago (2016-06-17 16:19:58 UTC) #30
dadrian
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h#newcode46 net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. ...
3 years ago (2016-06-17 17:26:55 UTC) #31
estark
https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_socket_impl.cc File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_socket_impl.cc#newcode1445 net/socket/ssl_client_socket_impl.cc:1445: transport_security_state_->CheckExpectStaple( On 2016/06/17 17:26:55, dadrian wrote: > On 2016/06/16 ...
3 years ago (2016-06-17 18:33:02 UTC) #32
Ryan Sleevi
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h#newcode46 net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. ...
3 years ago (2016-06-17 18:54:25 UTC) #33
dadrian
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple_report.h#newcode27 net/cert/expect_staple_report.h:27: struct SingleResult { On 2016/06/17 16:19:54, Ryan Sleevi wrote: ...
3 years ago (2016-06-17 23:17:36 UTC) #34
svaldez
https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_proc.cc#newcode254 net/cert/cert_verify_proc.cc:254: return; nit: omit. https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_proc.cc#newcode334 net/cert/cert_verify_proc.cc:334: CheckOCSP(ocsp_response, verify_result); Should this ...
3 years ago (2016-06-23 14:03:16 UTC) #35
Ryan Sleevi
3 years ago (2016-06-23 22:11:53 UTC) #36
Next suggested split: Split the "move OCSP to CertVerifyProc" into a separate
CL. That's something that can be easily bitten in to.

https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:57: base::Optional<OCSPCertStatus::Status>
cert_status;
Reviewer Feels time: I'd really love to avoid introducing base::Optional into
net/. I think it's a mistake (in boost and C++17) and one of those things that I
think is really gross that we did in //base anyways but that doesn't mean it's
all that nice.

https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:59: // Any stapled responses.
From the doc and discussion with Emily, I think this bears clarifying a bit more
what you're talking about (e.g. is this the BasicResponse data, etc).

When in doubt, cite RFC :)

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

https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec...
net/http/transport_security_state.cc:160: std::string
OCSPCertStatusToString(OCSPCertStatus::Status status) {
On 2016/06/23 14:03:15, svaldez wrote:
> Possibly attach this to ocsp_verify_result?

I'm actually supportive of leaving it here, since these strings are meaningful
in the report-API sense, not just the C++ API sense

https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec...
net/http/transport_security_state.cc:160: std::string
OCSPCertStatusToString(OCSPCertStatus::Status status) {
return "const char*" - don't force a string coercion unless the caller wants it
:)

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

https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec...
net/http/transport_security_state.h:22: #include "net/cert/x509_certificate.h"
Don't introduce this headers

https://codereview.chromium.org/2040513003/diff/190001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_impl.h (right):

https://codereview.chromium.org/2040513003/diff/190001/net/socket/ssl_client_...
net/socket/ssl_client_socket_impl.h:137: void ReportOCSP();
I'd like to avoid adding a dedicated state here; it's actually on the agenda of
"cleanups Sleevi wants to hack on" to clean up how the CT/cert verification
stuff works, but let's not add an extra method for it

https://codereview.chromium.org/2040513003/diff/190001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2040513003/diff/190001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:96: };
Document

Powered by Google App Engine
This is Rietveld 408576698