Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777383002:
Update SSL error handling code to account for Subject CN deprecation (Closed)
DescriptionUpdate 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 #Messages
Total messages: 53 (35 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
elawrence@chromium.org changed reviewers: + rsleevi@chromium.org
PTAL? This removes the SubjectCN from the list used to populate the Cert Name Mismatch error page. The change also ignores the SubjectCN when evaluating whether adding/removing "www." from the URL would avoid the error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/03/28 23:24:47, elawrence wrote: > PTAL? This removes the SubjectCN from the list used to populate the Cert Name > Mismatch error page. > > The change also ignores the SubjectCN when evaluating whether adding/removing > "www." from the URL would avoid the error. Also, I don't suppose you know why the iOS-simulator can't load my .PEM file? The .DER didn't work either. :-/
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/29 13:16:05, elawrence wrote: > On 2017/03/28 23:24:47, elawrence wrote: > > PTAL? This removes the SubjectCN from the list used to populate the Cert Name > > Mismatch error page. > > > > The change also ignores the SubjectCN when evaluating whether adding/removing > > "www." from the URL would avoid the error. > > Also, I don't suppose you know why the iOS-simulator can't load my .PEM file? > The .DER didn't work either. :-/ Ah. Build.gn update fixed that. Okay, now I'm really ready for you to PTAL. :)
elawrence@chromium.org changed reviewers: + estark@chromium.org - rsleevi@chromium.org
PTAL?
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Apologies for the review delay - I thought I was CC'd. This LGTM (a few nits), but I would feel better for Emily having a look at it related to the ErrorInfo approach, since -enamel tends to own that UI :) https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (left): https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:574: DCHECK(!dns_names.empty()); Wow, this was never correct from an API standpoint (because of IP address certs) https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:576: ssl_info_.cert->GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:150: cert.GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:289: cert.GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:386: cert.GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:414: cert.GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:461: cert.GetSubjectAltName(&dns_names, NULL); nit: nullptr https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_classification_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:61: ASSERT_EQ(1u, dns_names_example.size()); // ["www.example.com"] JUDGEMENT CALL NIT: ASSERT_THAT(dns_names_example, ElementsAre("www.example.com")); (If you want the GMock version, which apparently is the internal recommendation. I'm torn on that recommendation myself (not a fan), but I wouldn't be a good reviewer if I didn't highlight the changing winds of style :) https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_info.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_info.cc:36: cert->GetSubjectAltName(&dns_names, NULL); nullptr ;) https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_info.cc:54: dns_names.push_back("\"missing_subjectAltName\""); readability suggestion: It's usually easier to put the 'error' handling first int he condition, and then jump to the bulk of the logic, as it helps group together the flow That is if (dns_names.empty()) { dns_names.push_back(...) } else { ...
https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/100001/chrome/browser/ssl/ssl... 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: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:150: cert.GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:33, Ryan Sleevi wrote: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:289: cert.GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:33, Ryan Sleevi wrote: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:386: cert.GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:33, Ryan Sleevi wrote: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:414: cert.GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:33, Ryan Sleevi wrote: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification.cc:461: cert.GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:34, Ryan Sleevi wrote: > nit: nullptr Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_classification_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:61: ASSERT_EQ(1u, dns_names_example.size()); // ["www.example.com"] On 2017/03/31 14:51:34, Ryan Sleevi wrote: > JUDGEMENT CALL NIT: > ASSERT_THAT(dns_names_example, ElementsAre("www.example.com")); > > (If you want the GMock version, which apparently is the internal recommendation. > I'm torn on that recommendation myself (not a fan), but I wouldn't be a good > reviewer if I didn't highlight the changing winds of style :) Neat. I'd rather the code do the testing than leave the comments that explained what they hoped the test found. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... File components/ssl_errors/error_info.cc (right): https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_info.cc:36: cert->GetSubjectAltName(&dns_names, NULL); On 2017/03/31 14:51:34, Ryan Sleevi wrote: > nullptr ;) Done. https://codereview.chromium.org/2777383002/diff/100001/components/ssl_errors/... components/ssl_errors/error_info.cc:54: dns_names.push_back("\"missing_subjectAltName\""); On 2017/03/31 14:51:34, Ryan Sleevi wrote: > readability suggestion: It's usually easier to put the 'error' handling first > int he condition, and then jump to the bulk of the logic, as it helps group > together the flow > > That is > > if (dns_names.empty()) { > dns_names.push_back(...) > } else { > ... Swapped. (Heh. That's how I had it originally, then I flipped it because I felt like it was better to be optimistic.)
https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:580: RecordUMA(WWW_MISMATCH_FOUND); nit: technically, since we're changing the conditions under which this value gets recorded, best practice would be to mark this value as deprecated and introduce a new one WWW_MISMATCH_FOUND2 https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:243: class SSLErrorHandlerNameMismatchNoSANTest This test fixture is the same as the one above except for the certificate, right? If so, perhaps you could stick with one test fixture, and pull out SetUp() lines 242-265 into a SetUpErrorHandler() method that takes a certificate as an argument. https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:154: RecordSSLInterstitialCause(overridable, SUBDOMAIN_MATCH); same nit here about using a new UMA value https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:386: cert.GetSubjectAltName(&dns_names, nullptr); Blegh, I suppose all these will affect existing histograms too... https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.h:121: bool IsWWWSubDomainMatch(const GURL& request_url, Does this need to live in the public interface? Looks like it's only used by unit tests, in which case we could instead unit test the interface we already have, i.e. test that RecordUMAStatistics does not record WWW_SUBDOMAIN_MATCH when there's no SubjectAltName. That would involve moving SSLInterstitialCause into the public header to unit test the histogram, but IMO that's a better change because it would allow us to write more unit tests in future for RecordUMAStatistics, which we really should have.
elawrence@chromium.org changed reviewers: + mpearson@chromium.org
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... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:580: RecordUMA(WWW_MISMATCH_FOUND); On 2017/04/03 02:01:42, estark wrote: > nit: technically, since we're changing the conditions under which this value > gets recorded, best practice would be to mark this value as deprecated and > introduce a new one WWW_MISMATCH_FOUND2 Done. https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:243: class SSLErrorHandlerNameMismatchNoSANTest On 2017/04/03 02:01:42, estark wrote: > This test fixture is the same as the one above except for the certificate, > right? If so, perhaps you could stick with one test fixture, and pull out > SetUp() lines 242-265 into a SetUpErrorHandler() method that takes a certificate > as an argument. I didn't know how to do that in a straightforward way because I don't control the GoogleTest calls to SetUp(). So instead I've created a simple derived fixture, a pattern I've seen elsewhere. Reasonable? https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:154: RecordSSLInterstitialCause(overridable, SUBDOMAIN_MATCH); On 2017/04/03 02:01:42, estark wrote: > same nit here about using a new UMA value Done. https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:386: cert.GetSubjectAltName(&dns_names, nullptr); On 2017/04/03 02:01:42, estark wrote: > Blegh, I suppose all these will affect existing histograms too... yes. https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/2777383002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.h:121: bool IsWWWSubDomainMatch(const GURL& request_url, On 2017/04/03 02:01:42, estark wrote: > Does this need to live in the public interface? Looks like it's only used by > unit tests, in which case we could instead unit test the interface we already > have, i.e. test that RecordUMAStatistics does not record WWW_SUBDOMAIN_MATCH > when there's no SubjectAltName. That would involve moving SSLInterstitialCause > into the public header to unit test the histogram, but IMO that's a better > change because it would allow us to write more unit tests in future for > RecordUMAStatistics, which we really should have. Done.
Thanks, just some nits and a string question https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:243: class SSLErrorHandlerNameMismatchNoSANTest On 2017/04/04 15:52:28, elawrence wrote: > On 2017/04/03 02:01:42, estark wrote: > > This test fixture is the same as the one above except for the certificate, > > right? If so, perhaps you could stick with one test fixture, and pull out > > SetUp() lines 242-265 into a SetUpErrorHandler() method that takes a > certificate > > as an argument. > > I didn't know how to do that in a straightforward way because I don't control > the GoogleTest calls to SetUp(). So instead I've created a simple derived > fixture, a pattern I've seen elsewhere. Reasonable? Oh, I was thinking you could just call SetUpErrorHandler() in the test itself, but I like what you did better. https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:221: virtual scoped_refptr<net::X509Certificate> GetCertificate() { nit: make protected https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:233: ~SSLErrorHandlerNameMismatchTest() override {} nit: should go at line 199 https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... File components/ssl_errors/error_classification_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:62: ASSERT_TRUE(example_cert.get()); nit: I believe the .get() isn't necessary https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:164: histograms.ExpectTotalCount(kSslErrorCauseHistogram, 1); optional nit: instead of ExpectTotalCount and ExpectBucketCount, you can use ExpectUniqueSample https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... File components/ssl_errors/error_info.cc (right): https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_info.cc:41: dns_names.push_back("[missing_subjectAltName]"); Can we use a more human-readable |details| string for this case? WDYT of "This server could not prove that it is example.com; its security certificate is missing a Subject Alternative Name."? https://codereview.chromium.org/2777383002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2777383002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:111596: + hostname differs from one of the DNS names in the certificate (CN or SANs) remove "CN or", right?
Patchset #7 (id:160001) has been deleted
estark@: PTAL? I've updated based on feedback, added a TODO for one case, and cleaned up the outdated remarks in histograms.xml. https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:221: virtual scoped_refptr<net::X509Certificate> GetCertificate() { On 2017/04/04 17:22:07, estark wrote: > nit: make protected I made it private based on usage. https://codereview.chromium.org/2777383002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:233: ~SSLErrorHandlerNameMismatchTest() override {} On 2017/04/04 17:22:07, estark wrote: > nit: should go at line 199 Done. https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... File components/ssl_errors/error_classification_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:62: ASSERT_TRUE(example_cert.get()); On 2017/04/04 17:22:07, estark wrote: > nit: I believe the .get() isn't necessary Done. https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_classification_unittest.cc:164: histograms.ExpectTotalCount(kSslErrorCauseHistogram, 1); On 2017/04/04 17:22:07, estark wrote: > optional nit: instead of ExpectTotalCount and ExpectBucketCount, you can use > ExpectUniqueSample Done. https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... File components/ssl_errors/error_info.cc (right): https://codereview.chromium.org/2777383002/diff/140001/components/ssl_errors/... components/ssl_errors/error_info.cc:41: dns_names.push_back("[missing_subjectAltName]"); On 2017/04/04 17:22:07, estark wrote: > Can we use a more human-readable |details| string for this case? > WDYT of "This server could not prove that it is http://example.com; its security > certificate is missing a Subject Alternative Name."? Filed crbug.com/708268 https://codereview.chromium.org/2777383002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2777383002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:111596: + hostname differs from one of the DNS names in the certificate (CN or SANs) On 2017/04/04 17:22:08, estark wrote: > remove "CN or", right? Done.
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:256: scoped_refptr<net::X509Certificate> GetCertificate() override { this should be private too
https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:256: scoped_refptr<net::X509Certificate> GetCertificate() override { On 2017/04/05 03:37:41, estark wrote: > this should be private too Done.
histograms.xml and metrics definitions lgtm modulo a nice pile of nits --mark https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:69: SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE, optional nit: prefer labeling = 1, ... as recommended here https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:31: // Events for UMA. Do not reorder or change! nit: Please follow the comment and value setting recommendations here. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:35: WWW_SUBDOMAIN_MATCH, // Deprecated nit: in M__ here and below https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:47: // In Chrome 58, SubjectCN matching was deprecated, deprecating original ^58^59, right? https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:47: // In Chrome 58, SubjectCN matching was deprecated, deprecating original optional nit: This comment seems out of place to me. (Also, I don't know what SubjectCN means, but maybe that's okay.) Perhaps the comment better goes right above the 2-suffixed variants? Or perhaps you don't need it because the 2's make things obvious? Or perhaps you can saw in the deprecated lines above, "; replace with #2 version"? Just some thoughts. https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:56: UNUSED_INTERSTITIAL_CAUSE_ENTRY, Please use a name like SLL_INTERSTITIAL_CAUSE_MAX. The current name is confusing.
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, estark@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2777383002/#ps240001 (title: "Address Mark Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491423521568550,
"parent_rev": "f7c47573b7686c59723f3b7da6d69f1ec494f23b", "commit_rev":
"c7484f52b8ceb68e4334cad505e894aeef6cba83"}
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491423521568550,
"parent_rev": "f7c47573b7686c59723f3b7da6d69f1ec494f23b", "commit_rev":
"c7484f52b8ceb68e4334cad505e894aeef6cba83"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c7484f52b8ceb68e4334cad505e8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/c7484f52b8ceb68e4334cad505e8...
Message was sent while issue was closed.
Can you please publish your replies to my comments? thanks, mark
Message was sent while issue was closed.
https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2777383002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:69: SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE, On 2017/04/05 18:18:25, Mark P wrote: > optional nit: prefer labeling = 1, ... as recommended here > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:31: // Events for UMA. Do not reorder or change! On 2017/04/05 18:18:25, Mark P wrote: > nit: Please follow the comment and value setting recommendations here. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > Done. https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:35: WWW_SUBDOMAIN_MATCH, // Deprecated On 2017/04/05 18:18:25, Mark P wrote: > nit: in M__ > here and below Done. https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:47: // In Chrome 58, SubjectCN matching was deprecated, deprecating original On 2017/04/05 18:18:25, Mark P wrote: > ^58^59, right? SubjectCN matching was deprecated in M58. This change is going to M59 now, and might possibly merge to M58 (but given the late date, probably will be rejected). https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:47: // In Chrome 58, SubjectCN matching was deprecated, deprecating original On 2017/04/05 18:18:25, Mark P wrote: > optional nit: This comment seems out of place to me. (Also, I don't know what > SubjectCN means, but maybe that's okay.) Perhaps the comment better goes right > above the 2-suffixed variants? Or perhaps you don't need it because the 2's > make things obvious? Or perhaps you can saw in the deprecated lines above, "; > replace with #2 version"? > Just some thoughts. I've just removed this. https://codereview.chromium.org/2777383002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:56: UNUSED_INTERSTITIAL_CAUSE_ENTRY, On 2017/04/05 18:18:25, Mark P wrote: > Please use a name like SLL_INTERSTITIAL_CAUSE_MAX. The current name is > confusing. Done.
Message was sent while issue was closed.
thanks. still lgtm |
