Chromium Code Reviews
Help | Chromium Project | Sign in
(150)

Issue 11274032: Separate http_security_headers from transport_security_state (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by unsafe
Modified:
1 year, 3 months ago
CC:
chromium-reviews_chromium.org, cbentzel+watch_chromium.org, eroman, darin-cc_chromium.org
Base URL:
https://src.chromium.org/chrome/trunk/src/
Visibility:
Public.

Description

This is the first in an intended sequence of CLs to refactor
TransportSecurityState, fix some book-keeping bugs, and hopefully add TACK.
This sequence of CLs will be derived from the original, overly-large CL
#11191005.

This CL does a few things:
- Adds a high-level API for processing HSTS/HPKP
- Move the code for handling HSTS/HPKP headers out of transport_security_state
- Move HashValue out of x509_cert_types
- Addresses several HSTS/HPKP parsing bugs identified during review of the cleanup
- Ignore unknown HSTS/HPKP directives
- Ignore unknown hash algorithms
- Handle overly-large (> int64) expirations without parsing issues
- Reject invalid pins entered by users


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175595

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 46

Patch Set 6 : #

Patch Set 7 : #

Total comments: 12

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Total comments: 51

Patch Set 14 : #

Total comments: 2

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : #

Total comments: 13

Patch Set 19 : #

Total comments: 32

Patch Set 20 : #

Total comments: 21

Patch Set 21 : #

Patch Set 22 : #

Total comments: 20

Patch Set 23 : #

Total comments: 10

Patch Set 24 : #

Total comments: 4

Patch Set 25 : #

Patch Set 26 : #

Patch Set 27 : #

Total comments: 2

Patch Set 28 : #

Total comments: 1

Patch Set 29 : #

Patch Set 30 : #

Patch Set 31 : #

Patch Set 32 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1327 lines, -1137 lines) Lint Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/browser/net/transport_security_persister.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +3 lines, -11 lines 0 comments 0 errors Download
M chrome/browser/ui/webui/net_internals/net_internals_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 4 chunks +41 lines, -35 lines 0 comments 0 errors Download
A net/base/hash_value.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +125 lines, -0 lines 0 comments 1 errors Download
A net/base/hash_value.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +123 lines, -0 lines 0 comments 1 errors Download
M net/base/transport_security_state.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 3 chunks +10 lines, -19 lines 0 comments 0 errors Download
M net/base/transport_security_state.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 8 chunks +78 lines, -408 lines 0 comments 0 errors Download
M net/base/transport_security_state_static.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +105 lines, -53 lines 0 comments 0 errors Download
M net/base/transport_security_state_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +1 line, -401 lines 0 comments 0 errors Download
M net/base/x509_cert_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +1 line, -82 lines 0 comments 0 errors Download
M net/base/x509_cert_types.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 3 chunks +0 lines, -65 lines 0 comments 0 errors Download
A net/http/http_security_headers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +60 lines, -0 lines 0 comments 1 errors Download
A net/http/http_security_headers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +330 lines, -0 lines 0 comments 2 errors Download
A net/http/http_security_headers_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +424 lines, -0 lines 0 comments 1 errors Download
M net/net.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 3 chunks +5 lines, -0 lines 0 comments 0 errors Download
M net/url_request/url_request_http_job.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +20 lines, -63 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 61
Ryan Sleevi
Hi Trevor - first run through on this, highlighting the low-hanging fruits and just getting ...
1 year, 6 months ago #1
unsafe
Hi Ryan, I responded to your comments and uploaded a new version. My initial goal ...
1 year, 6 months ago #2
unsafe
Fixed a couple try errors I think (added NET_EXPORT_PRIVATE) to http_security_headers. Not sure what's going ...
1 year, 6 months ago #3
Ryan Sleevi
You're failing on Android for the reasons in the unit test that I point out ...
1 year, 5 months ago #4
unsafe
See below - the nits are fixed but I'm still not sure why verifier->Verify() failed ...
1 year, 5 months ago #5
Ryan Sleevi
I'll take another pass tomorrow, but I'm running out of nits, and that's a great ...
1 year, 5 months ago #6
unsafe
On 2012/10/29 23:46:25, Ryan Sleevi wrote: > Hence why I said to rip it all ...
1 year, 5 months ago #7
unsafe
Recovered from IETF, back in action... Per conversation w/rsleevi did a bit more refactoring: - ...
1 year, 5 months ago #8
Chromium Palmer
> * Rsleevi may have mentioned that HPKP header processing in > url_request_http_job should parse ...
1 year, 5 months ago #9
Ryan Sleevi
will follow up with palmer@ re: coalescing or not. You can choose to interpret that ...
1 year, 5 months ago #10
Chromium Palmer
https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#newcode117 net/base/hash_value.cc:117: return NULL != bsearch(hash.data, array, arraylen, base::kSHA1Length, Perhaps it's ...
1 year, 5 months ago #11
Ryan Sleevi
https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_request_http_job.cc#newcode708 net/url_request/url_request_http_job.cc:708: security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info); On 2012/11/13 19:35:04, Chris P. wrote: ...
1 year, 5 months ago #12
Chris Palmer
> So yeah, it seems like only one PKP header may be present. I'm not ...
1 year, 5 months ago #13
unsafe
OK, I think I addressed all comments. Open questions: - do you really want 1-space ...
1 year, 5 months ago #14
Ryan Sleevi
https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode1196 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1196: Base64StringToHashes(hashes_str, &state.dynamic_spki_hashes); On 2012/11/13 23:20:18, unsafe wrote: > On ...
1 year, 5 months ago #15
unsafe
On 2012/11/13 23:32:05, Ryan Sleevi wrote: > > > In the original code, if a ...
1 year, 5 months ago #16
unsafe
Regarding unknown hash algos: I just changed HPKP header parsing to skip them, but left ...
1 year, 5 months ago #17
Chromium Palmer
> My assumption - for both this and unknown-hash-algs - is that a "bad"/incomplete > ...
1 year, 5 months ago #18
Chromium Palmer
> Regarding unknown hash algos: I just changed HPKP header parsing to skip them, > ...
1 year, 5 months ago #19
unsafe
On 2012/11/14 21:03:04, Chris P. wrote: > > To summarize: this CL changes HPKP header ...
1 year, 5 months ago #20
Chris Palmer
On Wed, Nov 14, 2012 at 1:56 PM, <unsafe@trevp.net> wrote: > So, this CL changes ...
1 year, 5 months ago #21
unsafe
On 2012/11/16 00:30:35, Chris Palmer wrote: > > That sounds good, thanks! Cool, let me ...
1 year, 4 months ago #22
Chromium Palmer
Some nits, but that's it. I think we are getting close; what do rsleevi and ...
1 year, 4 months ago #23
unsafe
Addressed Chris's nits, I believe. https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode1103 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1103: std::string HashesToBase64String(const net::HashValueVector& hashes) ...
1 year, 4 months ago #24
Chromium Palmer
> Here's my case for changing it to binary: > > On a connection, any ...
1 year, 4 months ago #25
agl
Thoughts will be delayed until Monday I'm afraid. Too much happening.
1 year, 4 months ago #26
Ryan Sleevi
Sorry for the delays. I first focused on the correctness of the code in terms ...
1 year, 4 months ago #27
unsafe
https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode1165 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1165: result->SetString("static_spki_hashes", hashes_str); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > ...
1 year, 4 months ago #28
agl
https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode130 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:130: if (hash_str.substr(0, 4) != "sha1" && hash_str.substr(0, 6) != ...
1 year, 4 months ago #29
unsafe
I'll send out Go diff separately; removed the extra file-ending LF from new files, but ...
1 year, 4 months ago #30
agl
On Mon, Dec 10, 2012 at 3:59 PM, <unsafe@trevp.net> wrote: > I'll send out Go ...
1 year, 4 months ago #31
unsafe
Diff for transport_security_state_static_generate.go: func writeCertsOutput(out *bufio.Writer, pins []pin) { out.WriteString(`// These are SubjectPublicKeyInfo hashes for ...
1 year, 4 months ago #32
unsafe
On 2012/12/10 21:06:18, unsafe wrote: > Diff for transport_security_state_static_generate.go: Oops, copy/paste error, this is correct: ...
1 year, 4 months ago #33
agl
go change LGTM. (Note, that code lives here now: https://github.com/agl/transport-security-state-generate) Cheers AGL
1 year, 4 months ago #34
unsafe
I updated the CL description to a better synopsis. Also, made one last fix: On ...
1 year, 4 months ago #35
Ryan Sleevi
Sorry for the delays! I have a few nits, mostly around the comments then the ...
1 year, 4 months ago #36
unsafe
https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode131 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:131: if (hash_str.empty()) /* pair of commas with no content ...
1 year, 4 months ago #37
Ryan Sleevi
Still a few nits, but we're in the home stretch. Note the comments about the ...
1 year, 4 months ago #38
unsafe
https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode134 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:134: hash_str.compare(0, 7, "sha256/") != 0) On 2012/12/22 00:56:30, Ryan ...
1 year, 4 months ago #39
Ryan Sleevi
Oof. I feel your pain for long reviews - especially after I had vowed not ...
1 year, 3 months ago #40
unsafe
On 2013/01/03 01:11:32, Ryan Sleevi wrote: > I've submitted tryjobs based on Patch Set 24 ...
1 year, 3 months ago #41
unsafe
https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_headers.cc File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_headers.cc#newcode23 net/http/http_security_headers.cc:23: // is returned on any parse error. On 2013/01/03 ...
1 year, 3 months ago #42
Ryan Sleevi
https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_headers_unittest.cc File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_headers_unittest.cc#newcode290 net/http/http_security_headers_unittest.cc:290: std::min(kMaxHSTSAgeSecs, (int64)GG_INT64_C(39408299))); NACK: We don't use C-style casts It ...
1 year, 3 months ago #43
unsafe
Fix for try failures, hopefully (flying bind, I can't reproduce and don't really understand issue...) ...
1 year, 3 months ago #44
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/136001
1 year, 3 months ago #45
I haz the power (commit-bot)
Presubmit check for 11274032-136001 failed and returned exit status 1. Running presubmit commit checks ...
1 year, 3 months ago #46
Ryan Sleevi
Missed that the long lines need wrapping. Luckily C strings can be continued without special ...
1 year, 3 months ago #47
eroman
LGTM for net_internals/* https://chromiumcodereview.appspot.com/11274032/diff/136001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://chromiumcodereview.appspot.com/11274032/diff/136001/chrome/browser/ui/webui/net_internals/net_internals_ui.cc#newcode137 chrome/browser/ui/webui/net_internals/net_internals_ui.cc:137: if (hash_str.compare(0, 5, "sha1/") != 0 ...
1 year, 3 months ago #48
unsafe
On 2013/01/07 23:55:59, Ryan Sleevi wrote: > Missed that the long lines need wrapping. Luckily ...
1 year, 3 months ago #49
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/134003
1 year, 3 months ago #50
I haz the power (commit-bot)
Presubmit check for 11274032-134003 failed and returned exit status 1. Running presubmit commit checks ...
1 year, 3 months ago #51
unsafe
On 2013/01/08 02:03:06, I haz the power (commit-bot) wrote: > mailto:unsafe@trevp.net is not in AUTHORS ...
1 year, 3 months ago #52
Ryan Sleevi
Editing the CL description to be clearer Original description as follows: This is the first ...
1 year, 3 months ago #53
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/147002
1 year, 3 months ago #54
I haz the power (commit-bot)
Failed to apply patch for AUTHORS: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file ...
1 year, 3 months ago #55
Ryan Sleevi
On 2013/01/08 05:36:00, I haz the power (commit-bot) wrote: > Failed to apply patch for ...
1 year, 3 months ago #56
unsafe
On 2013/01/08 05:41:33, Ryan Sleevi wrote: > > +Trevor Perrin <mailto:unsafe@trevp.net> > > Heh, you ...
1 year, 3 months ago #57
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/133003
1 year, 3 months ago #58
I haz the power (commit-bot)
Retried try job too often on win_aura for step(s) content_browsertests
1 year, 3 months ago #59
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/133003
1 year, 3 months ago #60
I haz the power (commit-bot)
1 year, 3 months ago #61
Message was sent while issue was closed.
Change committed as 175595
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6