Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(112)

Issue 2777383002: Update SSL error handling code to account for Subject CN deprecation (Closed)

Created:
3 years, 9 months ago by elawrence
Modified:
3 years, 8 months ago
Reviewers:
Mark P, Ryan Sleevi, estark
CC:
Ryan Sleevi, chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Update SSL error handling code to account for Subject CN deprecation In Issue 308330, Chrome deprecated the use of the Subject CN field in certificate hostname validation. However, the certificate error interstitial and error classification logic were left unchanged, leading to misleading error messages and doomed error recovery attempts in the event that a certificate lacked SubjectAltNames. In this change, Chrome's Certificate Error interstitial and error recovery will no longer fallback to the certificate's Subject CN field when evaluating the certificate's valid dns names. BUG=703614 Review-Url: https://codereview.chromium.org/2777383002 Cr-Commit-Position: refs/heads/master@{#462230} Committed: https://chromium.googlesource.com/chromium/src/+/c7484f52b8ceb68e4334cad505e894aeef6cba83

Patch Set 1 : First try #

Patch Set 2 : Add and Fix tests #

Patch Set 3 : Generate certificate via script #

Patch Set 4 : Update build script #

Total comments: 19

Patch Set 5 : Address nits #

Total comments: 11

Patch Set 6 : Address Emily's feedback, add new histogram values. #

Total comments: 12

Patch Set 7 : Update histogram text #

Total comments: 14

Patch Set 8 : Privatize GetCertificate in subclass #

Patch Set 9 : Address Mark Feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+406 lines, -196 lines) Patch
M chrome/browser/ssl/ssl_error_handler.h View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -9 lines 0 comments Download
M chrome/browser/ssl/ssl_error_handler.cc View 1 2 3 4 5 1 chunk +28 lines, -27 lines 0 comments Download
M chrome/browser/ssl/ssl_error_handler_unittest.cc View 1 2 3 4 5 6 7 7 chunks +53 lines, -6 lines 0 comments Download
M components/ssl_errors/error_classification.h View 1 2 3 4 5 6 7 8 2 chunks +30 lines, -8 lines 0 comments Download
M components/ssl_errors/error_classification.cc View 1 2 3 4 5 6 7 8 6 chunks +22 lines, -35 lines 0 comments Download
M components/ssl_errors/error_classification_unittest.cc View 1 2 3 4 5 6 2 chunks +82 lines, -58 lines 0 comments Download
M components/ssl_errors/error_info.cc View 1 2 3 4 5 6 1 chunk +21 lines, -13 lines 0 comments Download
M net/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M net/cert/x509_certificate.h View 1 2 1 chunk +4 lines, -1 line 0 comments Download
A net/data/ssl/certificates/subjectAltName_www_example_com.pem View 1 2 1 chunk +75 lines, -0 lines 0 comments Download
M net/data/ssl/scripts/ee.cnf View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M net/data/ssl/scripts/generate-test-certs.sh View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M net/test/test_certificate_data.h View 1 3 chunks +7 lines, -3 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 3 chunks +61 lines, -36 lines 0 comments Download

Messages

Total messages: 53 (35 generated)
elawrence
PTAL? This removes the SubjectCN from the list used to populate the Cert Name Mismatch ...
3 years, 8 months ago (2017-03-28 23:24:47 UTC) #18
elawrence
On 2017/03/28 23:24:47, elawrence wrote: > PTAL? This removes the SubjectCN from the list used ...
3 years, 8 months ago (2017-03-29 13:16:05 UTC) #21
elawrence
On 2017/03/29 13:16:05, elawrence wrote: > On 2017/03/28 23:24:47, elawrence wrote: > > PTAL? This ...
3 years, 8 months ago (2017-03-29 15:45:19 UTC) #24
elawrence
PTAL?
3 years, 8 months ago (2017-03-31 14:36:01 UTC) #26
Ryan Sleevi
Apologies for the review delay - I thought I was CC'd. This LGTM (a few ...
3 years, 8 months ago (2017-03-31 14:51:34 UTC) #28
elawrence
https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl_error_handler.cc File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl_error_handler.cc#newcode576 chrome/browser/ssl/ssl_error_handler.cc:576: ssl_info_.cert->GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:33, Ryan Sleevi wrote: > ...
3 years, 8 months ago (2017-03-31 16:09:42 UTC) #29
estark
https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl_error_handler.cc File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl_error_handler.cc#newcode580 chrome/browser/ssl/ssl_error_handler.cc:580: RecordUMA(WWW_MISMATCH_FOUND); nit: technically, since we're changing the conditions under ...
3 years, 8 months ago (2017-04-03 02:01:43 UTC) #30
elawrence
mpearson@: PTAL at histograms.xml? estark@: PTAL at my changes based on your comments? https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl_error_handler.cc File ...
3 years, 8 months ago (2017-04-04 15:52:28 UTC) #32
estark
Thanks, just some nits and a string question https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl_error_handler_unittest.cc File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl_error_handler_unittest.cc#newcode243 chrome/browser/ssl/ssl_error_handler_unittest.cc:243: class ...
3 years, 8 months ago (2017-04-04 17:22:08 UTC) #33
elawrence
estark@: PTAL? I've updated based on feedback, added a TODO for one case, and cleaned ...
3 years, 8 months ago (2017-04-04 19:53:24 UTC) #35
estark
lgtm https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler_unittest.cc File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler_unittest.cc#newcode256 chrome/browser/ssl/ssl_error_handler_unittest.cc:256: scoped_refptr<net::X509Certificate> GetCertificate() override { this should be private ...
3 years, 8 months ago (2017-04-05 03:37:41 UTC) #40
elawrence
https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler_unittest.cc File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler_unittest.cc#newcode256 chrome/browser/ssl/ssl_error_handler_unittest.cc:256: scoped_refptr<net::X509Certificate> GetCertificate() override { On 2017/04/05 03:37:41, estark wrote: ...
3 years, 8 months ago (2017-04-05 14:51:40 UTC) #41
Mark P
histograms.xml and metrics definitions lgtm modulo a nice pile of nits --mark https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler.h File chrome/browser/ssl/ssl_error_handler.h ...
3 years, 8 months ago (2017-04-05 18:18:25 UTC) #42
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/2777383002/240001
3 years, 8 months ago (2017-04-05 20:19:25 UTC) #46
commit-bot: I haz the power
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/c7484f52b8ceb68e4334cad505e894aeef6cba83
3 years, 8 months ago (2017-04-05 21:52:11 UTC) #50
Mark P
Can you please publish your replies to my comments? thanks, mark
3 years, 8 months ago (2017-04-05 22:20:30 UTC) #51
elawrence
https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler.h File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl_error_handler.h#newcode69 chrome/browser/ssl/ssl_error_handler.h:69: SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE, On 2017/04/05 18:18:25, Mark P wrote: > optional ...
3 years, 8 months ago (2017-04-05 23:05:17 UTC) #52
Mark P
3 years, 8 months ago (2017-04-05 23:12:00 UTC) #53
Message was sent while issue was closed.
thanks.  still lgtm

Powered by Google App Engine
This is Rietveld 408576698