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

Issue 2753123002: Add --ignore-certificate-errors-spki-list switch and UMA histogram. (Closed)

Created:
2 years, 5 months ago by martinkr
Modified:
2 years, 3 months ago
CC:
chromium-reviews, jam, cbentzel+watch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, net-reviews_chromium.org, agl
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add --ignore-certificate-errors-spki-list switch and UMA histogram. The switch takes a comma-separated list of base64-encoded, SHA-256 subject public key hashes. If any of the certs presented by the server match a hash from the list, certificate verification is skipped, in order to ignore certificate errors. Presence of the flag is counted in a UMA histogram. This should allow us to remove the more blunt --ignore-certificate-errors switch eventually. R=rsleevi@chromium.org CC=agl@chromium.org BUG= Review-Url: https://codereview.chromium.org/2753123002 Cr-Commit-Position: refs/heads/master@{#474021} Committed: https://chromium.googlesource.com/chromium/src/+/8402d3c5bc13e018fa75eba650ed881755e0223b

Patch Set 1 #

Total comments: 16

Patch Set 2 : Add --ignore-certificate-errors-spki-list switch and UMA histogram. #

Total comments: 2

Patch Set 3 : Add IgnoreErrorsCertVerifier. #

Patch Set 4 : Really add IgnoreErrorsCertVerifier. #

Total comments: 14

Patch Set 5 : Move IgnoreErrorsCertVerifier to chrome/browser/ssl. #

Total comments: 19

Patch Set 6 : Add --ignore-certificate-errors-spki-list switch and UMA histogram. #

Total comments: 4

Patch Set 7 : Add --ignore-certificate-errors-spki-list switch and UMA histogram. #

Total comments: 4

Patch Set 8 : Add --ignore-certificate-errors-spki-list switch and UMA histogram. #

Patch Set 9 : Work around command line in ssl_browser_tests exceeding RESTART_MAX_CMD_LINE on Windows. Make tests… #

Patch Set 10 : Rebase. #

Patch Set 11 : Added test for presence of --user-data-dir. #

Patch Set 12 : Move test for --user-data-dir into unittest; add bad flags prompt. #

Total comments: 14

Patch Set 13 : Add --ignore-certificate-errors-spki-list switch and UMA histogram. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+553 lines, -2 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -1 line 0 comments Download
A chrome/browser/ssl/ignore_errors_cert_verifier.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +68 lines, -0 lines 0 comments Download
A chrome/browser/ssl/ignore_errors_cert_verifier.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +132 lines, -0 lines 0 comments Download
A chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +200 lines, -0 lines 0 comments Download
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +120 lines, -1 line 0 comments Download
M chrome/browser/ui/startup/bad_flags_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 70 (35 generated)
martinkr
I finally got around to adding this flag that we chatted about a while ago. ...
2 years, 5 months ago (2017-03-16 22:49:41 UTC) #1
Ryan Sleevi
note x509_certificate_nss is only used on Linux/ChromeOS, so it'd have to be updated for all ...
2 years, 5 months ago (2017-03-16 23:19:14 UTC) #2
martinkr
Apologies for the delayed response, PTAL. https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc#newcode1032 chrome/browser/io_thread.cc:1032: params->ignore_certificate_error_spki_list.insert(spki); On 2017/03/16 ...
2 years, 5 months ago (2017-03-28 23:16:03 UTC) #3
Ryan Sleevi
A much larger design comment - soliciting for feedback before you do anything, to get ...
2 years, 4 months ago (2017-03-30 15:12:09 UTC) #4
martinkr
https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_thread.cc#newcode1043 chrome/browser/io_thread.cc:1043: } On 2017/03/30 15:12:09, Ryan Sleevi wrote: > So ...
2 years, 4 months ago (2017-03-30 17:33:38 UTC) #5
Ryan Sleevi
On 2017/03/30 17:33:38, martinkr wrote: > Good question. I lean towards yes because it doesn't ...
2 years, 4 months ago (2017-03-30 18:12:27 UTC) #6
martinkr
On 2017/03/30 18:12:27, Ryan Sleevi wrote: > On 2017/03/30 17:33:38, martinkr wrote: > > Good ...
2 years, 4 months ago (2017-03-30 18:13:14 UTC) #7
martinkr
PTAL
2 years, 4 months ago (2017-04-06 22:31:07 UTC) #8
Ryan Sleevi
Thanks for continuing to work on this! Some thoughts below, but feel free to reach ...
2 years, 4 months ago (2017-04-07 16:08:05 UTC) #9
martinkr
https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_thread.cc#newcode585 chrome/browser/io_thread.cc:585: if (command_line.HasSwitch(switches::kIgnoreCertificateErrorsSPKIList)) { On 2017/04/07 16:08:05, Ryan Sleevi wrote: ...
2 years, 4 months ago (2017-04-07 21:40:57 UTC) #10
Ryan Sleevi
https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/ignore_errors_cert_verifier.cc File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right): https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/ignore_errors_cert_verifier.cc#newcode30 chrome/browser/ssl/ignore_errors_cert_verifier.cc:30: HashValue hv; style: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules a name like "hash" or ...
2 years, 4 months ago (2017-04-12 21:53:18 UTC) #11
martinkr
https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/ignore_errors_cert_verifier.cc File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right): https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/ignore_errors_cert_verifier.cc#newcode30 chrome/browser/ssl/ignore_errors_cert_verifier.cc:30: HashValue hv; On 2017/04/12 21:53:17, Ryan Sleevi wrote: > ...
2 years, 4 months ago (2017-04-24 23:54:26 UTC) #12
Ryan Sleevi
lgtm https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_verifier.cc File net/cert/mock_cert_verifier.cc (right): https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_verifier.cc#newcode69 net/cert/mock_cert_verifier.cc:69: callback.Run(result); On 2017/04/24 23:54:26, martinkr wrote: > On ...
2 years, 4 months ago (2017-04-25 18:07:10 UTC) #14
martinkr
https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ignore_errors_cert_verifier.cc File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right): https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ignore_errors_cert_verifier.cc#newcode102 chrome/browser/ssl/ignore_errors_cert_verifier.cc:102: [](const SHA256HashValue v) { return HashValue(v); }); On 2017/04/25 ...
2 years, 4 months ago (2017-04-25 19:48:37 UTC) #15
Ryan Sleevi
Adding a metrics reviewer :) Hopefully CL description has sufficient context for "why" :)
2 years, 4 months ago (2017-04-25 19:49:40 UTC) #18
martinkr
On 2017/04/25 19:49:40, Ryan Sleevi wrote: > Adding a metrics reviewer :) Hopefully CL description ...
2 years, 4 months ago (2017-04-25 19:50:16 UTC) #21
Ilya Sherman
Metrics LGTM % comments: https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histograms/histograms.xml#newcode31683 tools/metrics/histograms/histograms.xml:31683: + enum="BooleanEnabled"> This enum has ...
2 years, 4 months ago (2017-04-26 00:14:33 UTC) #24
martinkr
https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histograms/histograms.xml#newcode31683 tools/metrics/histograms/histograms.xml:31683: + enum="BooleanEnabled"> On 2017/04/26 00:14:33, Ilya Sherman wrote: > ...
2 years, 4 months ago (2017-04-26 00:57:04 UTC) #25
Ilya Sherman
(Metrics still LGTM)
2 years, 4 months ago (2017-04-26 07:14:43 UTC) #30
martinkr
Looks like the ssl_browser_tests still fail on trybot: There seems to be an issue around ...
2 years, 4 months ago (2017-04-26 18:47:06 UTC) #31
martinkr
I added a test for --user-data-dir as suggested, although I'm not entirely sure if this ...
2 years, 3 months ago (2017-05-10 00:16:02 UTC) #44
Ilya Sherman
metrics lgtm
2 years, 3 months ago (2017-05-10 00:24:00 UTC) #47
estark
Hmm, I'm also not sure if it's okay to unset --user-data-dir in a browser test. ...
2 years, 3 months ago (2017-05-11 01:07:08 UTC) #50
Ryan Sleevi
On 2017/05/11 01:07:08, estark wrote: > Hmm, I'm also not sure if it's okay to ...
2 years, 3 months ago (2017-05-11 19:22:05 UTC) #51
martinkr
On 2017/05/11 19:22:05, Ryan Sleevi wrote: > On 2017/05/11 01:07:08, estark wrote: > > Hmm, ...
2 years, 3 months ago (2017-05-11 19:25:07 UTC) #52
martinkr
On 2017/05/11 19:25:07, martinkr wrote: > On 2017/05/11 19:22:05, Ryan Sleevi wrote: > > On ...
2 years, 3 months ago (2017-05-12 01:13:02 UTC) #53
Ryan Sleevi
lgtm
2 years, 3 months ago (2017-05-15 14:45:03 UTC) #54
estark
lgtm with nits https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ignore_errors_cert_verifier.h File chrome/browser/ssl/ignore_errors_cert_verifier.h (right): https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ignore_errors_cert_verifier.h#newcode1 chrome/browser/ssl/ignore_errors_cert_verifier.h:1: // Copyright (c) 2017 The Chromium ...
2 years, 3 months ago (2017-05-17 18:04:19 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2753123002/220001
2 years, 3 months ago (2017-05-18 22:36:54 UTC) #58
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/441740)
2 years, 3 months ago (2017-05-18 22:50:40 UTC) #60
martinkr
R: +pkasting for chrome/browser/ui/startup/bad_flags_prompt.cc
2 years, 3 months ago (2017-05-18 22:56:46 UTC) #62
Peter Kasting
LGTM
2 years, 3 months ago (2017-05-19 02:03:35 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2753123002/240001
2 years, 3 months ago (2017-05-23 18:30:26 UTC) #66
martinkr
https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ignore_errors_cert_verifier.h File chrome/browser/ssl/ignore_errors_cert_verifier.h (right): https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ignore_errors_cert_verifier.h#newcode1 chrome/browser/ssl/ignore_errors_cert_verifier.h:1: // Copyright (c) 2017 The Chromium Authors. All rights ...
2 years, 3 months ago (2017-05-23 18:30:43 UTC) #67
commit-bot: I haz the power
2 years, 3 months ago (2017-05-23 20:11:25 UTC) #70
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/8402d3c5bc13e018fa75eba650ed...

Powered by Google App Engine
This is Rietveld 408576698