Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
376333003:
Find reasons for the SSL common name invalid error. (Closed)
|
Created:
6 years, 5 months ago by radhikabhar Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFind reasons for the SSL common name invalid error.
BUG=392310
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287942
Patch Set 1 : Changes #Patch Set 2 : Rebase update #
Total comments: 23
Patch Set 3 : Addressed comments #
Total comments: 49
Patch Set 4 : Addressed Comments #
Total comments: 24
Patch Set 5 : Addressed comments #
Total comments: 18
Patch Set 6 : Comments #
Total comments: 6
Patch Set 7 : Addressed Comments #
Total comments: 10
Patch Set 8 : Rebase Update #Patch Set 9 : Addressed Comments #
Total comments: 1
Patch Set 10 : Rebase-Update #Patch Set 11 : Removed the Self -Signed function #
Total comments: 16
Patch Set 12 : Addressed comments #
Total comments: 16
Patch Set 13 : Addressed Comments #Patch Set 14 : Rebase-Update #
Total comments: 8
Patch Set 15 : Addressed comments #Patch Set 16 : Changed the name of the histogram back #Patch Set 17 : Added comments to the histogram #
Total comments: 10
Patch Set 18 : Changed the comments in the histogram.xml #
Total comments: 9
Patch Set 19 : Addressed Comments #
Messages
Total messages: 45 (0 generated)
On 2014/07/14 21:37:16, radhikabhar wrote: hey, i don't think you need the "patch from issue 376663002 " line anymore because 376663002 landed a day ago
https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:29: // Scores/weights which will be constant through all the SSL error types. As we discussed earlier today, I'm not sure whether this is a good idea moving forwards. It seems like some checks (e.g., the clock) ought to weigh a lot more than a fraction of 0.5. You don't necessarily need to change it here, but please be thinking about it. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:44: // "abc" then this function returns "a" otherwise it returns an empty string. Is this supposed to be checking whether str2 = prefix + str1, and then returning prefix? It so, it might make sense to name it explain it like: // Utility function: Check to see whether |longer_str| is equivalent to |smaller_str| plus a prefix. If so, // return the prefix. If not, returns an empty string. For example: if |longer_str| is "abc" and |shorter_str| // is "bc", it will return "a". std::string FindStringPrefix(std::string longer_str, std::string smaller_str) { You don't have to use this exact wording or anything -- but I think the current explanation is confusing and could be improved by explaining that you are looking for a string prefix. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:47: std::size_t found = str1.find(str2); nit: a stray space before the = sign https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:51: return std::string(); to make sure I understand correctly: the reason for the result_last.compare(str2) != 0 comparison is to make sure that there are no trailing strings? e.g., this will reject "abcd" and "bc" as a pair. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:53: return result_first; why is this indented? https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:92: float SSLErrorClassification::InvalidDateSeverityScore() const{ It looks like a rebase went wrong -- InvalidDateSeverityScore is repeated here twice. I can't tell what is supposed to be new here https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:199: || dns_names[i].length() == host_name.length()) since this is multi-line, I'd recommend putting { } https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:209: } It might be interesting to do another version of this that checks for subdomains in general (not just www) https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // float InvalidAuthoritySeverityScore(); Please don't include commented-out methods here, just save them for a future CL https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:50: bool IsWWWDifference() const; Can you either give these more descriptive names or add comments w/ explanations of what they do? "IsRegisteredDomainInverseMatch" isn't an intuitive enough name to stand on its own. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:71: GURL origin("https://foo.www.google.com"); can you also test https://www.google.com.foo to make sure it is right aligning?
https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:29: // Scores/weights which will be constant through all the SSL error types. On 2014/07/15 00:52:44, felt wrote: > As we discussed earlier today, I'm not sure whether this is a good idea moving > forwards. It seems like some checks (e.g., the clock) ought to weigh a lot more > than a fraction of 0.5. You don't necessarily need to change it here, but please > be thinking about it. Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:44: // "abc" then this function returns "a" otherwise it returns an empty string. On 2014/07/15 00:52:44, felt wrote: > Is this supposed to be checking whether str2 = prefix + str1, and then returning > prefix? It so, it might make sense to name it explain it like: > > // Utility function: Check to see whether |longer_str| is equivalent to > |smaller_str| plus a prefix. If so, > // return the prefix. If not, returns an empty string. For example: if > |longer_str| is "abc" and |shorter_str| > // is "bc", it will return "a". > std::string FindStringPrefix(std::string longer_str, std::string smaller_str) { > > You don't have to use this exact wording or anything -- but I think the current > explanation is confusing and could be improved by explaining that you are > looking for a string prefix. Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:47: std::size_t found = str1.find(str2); On 2014/07/15 00:52:44, felt wrote: > nit: a stray space before the = sign Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:51: return std::string(); On 2014/07/15 00:52:44, felt wrote: > to make sure I understand correctly: the reason for the > result_last.compare(str2) != 0 comparison is to make sure that there are no > trailing strings? e.g., this will reject "abcd" and "bc" as a pair. Yes, because we don't want o be accepting foo.google.com.foo when we are presented with a certificate for google.com https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:53: return result_first; On 2014/07/15 00:52:44, felt wrote: > why is this indented? Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:92: float SSLErrorClassification::InvalidDateSeverityScore() const{ On 2014/07/15 00:52:44, felt wrote: > It looks like a rebase went wrong -- InvalidDateSeverityScore is repeated here > twice. I can't tell what is supposed to be new here Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:199: || dns_names[i].length() == host_name.length()) On 2014/07/15 00:52:44, felt wrote: > since this is multi-line, I'd recommend putting { } Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:209: } On 2014/07/15 00:52:44, felt wrote: > It might be interesting to do another version of this that checks for subdomains > in general (not just www) The function IsRegisteredDomainMatch() checks whether the url is a subdomain of the dns name given in the server. Is that what you meant or something else? https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // float InvalidAuthoritySeverityScore(); On 2014/07/15 00:52:44, felt wrote: > Please don't include commented-out methods here, just save them for a future CL Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:50: bool IsWWWDifference() const; On 2014/07/15 00:52:44, felt wrote: > Can you either give these more descriptive names or add comments w/ explanations > of what they do? "IsRegisteredDomainInverseMatch" isn't an intuitive enough name > to stand on its own. Done. https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:71: GURL origin("https://foo.www.google.com"); On 2014/07/15 00:52:44, felt wrote: > can you also test https://www.google.com.foo to make sure it is right aligning? The expected behaviour is false right now. Wouldn't www.google.com.foo be an attack for www.google.com and shouldn't it be rejected? Or should we consider it as a false positive?
I'm a little worried about the fact that you're essentially running regexes over URLs. That often leads to pain. At the very least, please make sure to have numerous test cases for every method that does any string matching on URLs. Have you looked over the GURL class to see if there's any support for tokenizing URLs to identify subdomains? https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:71: GURL origin("https://foo.www.google.com"); On 2014/07/15 17:34:09, radhikabhar wrote: > On 2014/07/15 00:52:44, felt wrote: > > can you also test https://www.google.com.foo to make sure it is right > aligning? > > The expected behaviour is false right now. Wouldn't http://www.google.com.foo be an > attack for http://www.google.com and shouldn't it be rejected? Or should we consider it > as a false positive? yes that is what I mean, can you check that IsWWWDifference will return false for www.google.com.foo when compared to google.com? https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Pretty sure this was correct before, they removed the "(c)" in 2013 or 2014. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:70: // CLient-side characteristics. Check whether or not the system's clock is nit: Client- https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:71: // worng and whether or not the user has already encountered this error nit: wrong https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:121: void SSLErrorClassification::RecordUMAStatistics(bool overridable) { Can you add UMA stats for all the other ones too? IsWWWDifference, IsSubDomainMatch, etc. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:163: bool SSLErrorClassification::IsWWWDifference() const { suggestion: rename this IsWWWSubDomainMatch to be consistent with the other methods https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:169: cert_.GetDNSNames(&dns_names); spurious indentation https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:199: for (size_t i = 0; i < dns_names.size(); ++i) { indentation https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:204: if (string_prefix.empty() || (string_prefix.compare("www.") == 0)) { Why is this false if the string prefix is www.? Shouldn't IsSubDomainMatch be a superset of IsWWWDifference? https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:207: size_t total_count = std::count(string_prefix.begin(), What about the pair: a.foogoogle.com, google.com? It should return false, but I think it will return true. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:217: // attack. Can you explain why, for posterity? https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:237: string_prefix.end(),'.'); nit: space between arguments https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:245: // This method is valid for wildcard certificates only. you should make sure that it returns false if it runs on a non-wildcard certificate (add a test, I don't see one) https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:246: bool SSLErrorClassification::IsHostNameTooBroad() const { You might want to consider renaming this something like: IsSubDomainOutsideWildcard, or something like that. I didn't understand what "too broad" meant here. Also, might make sense to document with the good example you gave me: it should return true for cases like a.b.foo.com, when the certificate is for *.foo.com. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:258: if (dns_names[i].find("*") == std::string::npos) { More specifically, you are expecting the first two characters to be "*.". You should probably check for that directly, instead of checking to see if there is a * anywhere. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:268: extracted_common_name); ^ indentation https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:19: const::GURL& url, I'm surprised this compiles, I don't think you want those extra :: https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:45: bool IsWWWDifference() const; can you add a comment to this one too?
https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:43: // the |longer_str| = prefix + |smaller_str|. If so, then it returns the prefix Nit: Antonyms are longer/shorter, larger/smaller. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:179: string_prefix = GetStringPrefix(dns_names[i], host_name); BUG: You really do need to tokenize the hostnames. That is, split on "." into a vector of DNS labels: "www.google.com" --> ("www", "google", "com") Otherwise, you run the risk of treating "totallyfakegoogle.com" as a subdomain of "google.com"! I think what you want is something more like: bool IsSubdomain(const vector<string>& tokenized_parent, const vector<string>& tokenized_potential_subdomain) { // ... } But I suspect that some similar code — possibly code that takes the Public Suffix List into account — already exists somewhere in Chromium. And you probably will want to take the PSL into account. Probably even as simple as, "never apply this heuristic to any hostname that ends in a domain name in the PSL that is not a true TLD; require exact matches for such names." https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:47: // This method checks whether the given url is a subdomain of the dns name Nit: Capitalize "DNS" and "URL" (throughout this file, and generally). Also, I would be more precise: "...whether or not the hostname in the given URL is a subdomain of any DNS name in the CN or SAN fields of the certificate." https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:52: // subdomain of the given url or not. Same precision concern as above. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:55: // This method check whether the host name is too broad for the scope of a Typo: "checks", "hostname" https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:65: const GURL request_url_; Can this be const GURL&, or does it need to be a copy?
There might be some useful methods in net/base/net_util.h. For example, for the www matching case you could do something like: if (a.host() == StripWWW(b.host()) return true; (pardon the pseudocode)
There are several places in Chrome which check whether the URL is a subdomain or not but I am not using that as I want to check more than that (e.g. what is the difference in between the two strings). https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/07/15 20:44:34, felt wrote: > Pretty sure this was correct before, they removed the "(c)" in 2013 or 2014. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:43: // the |longer_str| = prefix + |smaller_str|. If so, then it returns the prefix On 2014/07/15 21:23:23, Chromium Palmer wrote: > Nit: Antonyms are longer/shorter, larger/smaller. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:70: // CLient-side characteristics. Check whether or not the system's clock is On 2014/07/15 20:44:34, felt wrote: > nit: Client- Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:71: // worng and whether or not the user has already encountered this error On 2014/07/15 20:44:34, felt wrote: > nit: wrong Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:121: void SSLErrorClassification::RecordUMAStatistics(bool overridable) { On 2014/07/15 20:44:34, felt wrote: > Can you add UMA stats for all the other ones too? IsWWWDifference, > IsSubDomainMatch, etc. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:163: bool SSLErrorClassification::IsWWWDifference() const { On 2014/07/15 20:44:33, felt wrote: > suggestion: rename this IsWWWSubDomainMatch to be consistent with the other > methods Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:169: cert_.GetDNSNames(&dns_names); On 2014/07/15 20:44:34, felt wrote: > spurious indentation Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:179: string_prefix = GetStringPrefix(dns_names[i], host_name); On 2014/07/15 21:23:23, Chromium Palmer wrote: > BUG: You really do need to tokenize the hostnames. That is, split on "." into a > vector of DNS labels: > > "www.google.com" --> ("www", "google", "com") > > Otherwise, you run the risk of treating "totallyfakegoogle.com" as a subdomain > of "google.com"! > > I think what you want is something more like: > > bool IsSubdomain(const vector<string>& tokenized_parent, > const vector<string>& tokenized_potential_subdomain) { > // ... > } > > But I suspect that some similar code — possibly code that takes the Public > Suffix List into account — already exists somewhere in Chromium. > > And you probably will want to take the PSL into account. Probably even as simple > as, "never apply this heuristic to any hostname that ends in a domain name in > the PSL that is not a true TLD; require exact matches for such names." Done https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:204: if (string_prefix.empty() || (string_prefix.compare("www.") == 0)) { On 2014/07/15 20:44:34, felt wrote: > Why is this false if the string prefix is http://www. Shouldn't IsSubDomainMatch be a > superset of IsWWWDifference? Yes but then I wanted to differentiate between the cases for www and the rest of them because www is more likely to be benign as compared to any other case. Also, if I return true out here for www then it would be recorded twice in the histogram once in the bucket for IsWWWDifference and IsSubDomain or so we want to record them in both the places? If I am calculating the score for Common_name_invalid, I will be calling both the functions and this will again return twice which might mess with the calculation. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:207: size_t total_count = std::count(string_prefix.begin(), On 2014/07/15 20:44:34, felt wrote: > What about the pair: http://a.foogoogle.com, google.com? It should return false, but I > think it will return true. You are right. Fixed it by tokenizing the string and then checking whether it is a sub domain or not. Added a test case too. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:217: // attack. On 2014/07/15 20:44:33, felt wrote: > Can you explain why, for posterity? Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:237: string_prefix.end(),'.'); On 2014/07/15 20:44:34, felt wrote: > nit: space between arguments Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:245: // This method is valid for wildcard certificates only. On 2014/07/15 20:44:34, felt wrote: > you should make sure that it returns false if it runs on a non-wildcard > certificate (add a test, I don't see one) I actually do check it for the google cert on line 58, Should I add more test cases? https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:246: bool SSLErrorClassification::IsHostNameTooBroad() const { On 2014/07/15 20:44:34, felt wrote: > You might want to consider renaming this something like: > IsSubDomainOutsideWildcard, or something like that. I didn't understand what > "too broad" meant here. Also, might make sense to document with the good example > you gave me: it should return true for cases like http://a.b.foo.com, when the > certificate is for *.foo.com. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:258: if (dns_names[i].find("*") == std::string::npos) { On 2014/07/15 20:44:34, felt wrote: > More specifically, you are expecting the first two characters to be "*.". You > should probably check for that directly, instead of checking to see if there is > a * anywhere. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:268: extracted_common_name); On 2014/07/15 20:44:33, felt wrote: > ^ indentation Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:19: const::GURL& url, On 2014/07/15 20:44:35, felt wrote: > I'm surprised this compiles, I don't think you want those extra :: Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:45: bool IsWWWDifference() const; On 2014/07/15 20:44:35, felt wrote: > can you add a comment to this one too? Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:47: // This method checks whether the given url is a subdomain of the dns name On 2014/07/15 21:23:23, Chromium Palmer wrote: > Nit: Capitalize "DNS" and "URL" (throughout this file, and generally). > > Also, I would be more precise: "...whether or not the hostname in the given URL > is a subdomain of any DNS name in the CN or SAN fields of the certificate." Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:52: // subdomain of the given url or not. On 2014/07/15 21:23:23, Chromium Palmer wrote: > Same precision concern as above. Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:55: // This method check whether the host name is too broad for the scope of a On 2014/07/15 21:23:23, Chromium Palmer wrote: > Typo: "checks", "hostname" Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:65: const GURL request_url_; On 2014/07/15 21:23:23, Chromium Palmer wrote: > Can this be const GURL&, or does it need to be a copy? Done. https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:108: EXPECT_TRUE(ssl_error.IsSubDomainInverseMatch()); This check will be false for everything because the function GetRegistryList that I am using to check whether it is a known top level domain returns 0 if the host name is the tld only.
https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:267: SSLErrorClassification ssl_error_classification( you should create a single SSLErrorClassification object and call its methods repeatedly https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:275: if (SSLErrorInfo::NetErrorToErrorType(cert_error_) == you should probably save the error type instead of calling NetErrorToErrorType every time
https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:204: if (string_prefix.empty() || (string_prefix.compare("www.") == 0)) { On 2014/07/16 22:35:14, radhikabhar wrote: > On 2014/07/15 20:44:34, felt wrote: > > Why is this false if the string prefix is http://www. Shouldn't > IsSubDomainMatch be a > > superset of IsWWWDifference? > > Yes but then I wanted to differentiate between the cases for www and the rest of > them because www is more likely to be benign as compared to any other case. > Also, if I return true out here for www then it would be recorded twice in the > histogram once in the bucket for IsWWWDifference and IsSubDomain or so we want > to record them in both the places? > If I am calculating the score for Common_name_invalid, I will be calling both > the functions and this will again return twice which might mess with the > calculation. ok sounds good https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:245: // This method is valid for wildcard certificates only. On 2014/07/16 22:35:14, radhikabhar wrote: > On 2014/07/15 20:44:34, felt wrote: > > you should make sure that it returns false if it runs on a non-wildcard > > certificate (add a test, I don't see one) > > I actually do check it for the google cert on line 58, Should I add more test > cases? missed that, OK https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:108: EXPECT_TRUE(ssl_error.IsSubDomainInverseMatch()); On 2014/07/16 22:35:16, radhikabhar wrote: > This check will be false for everything because the function GetRegistryList > that I am using to check whether it is a known top level domain returns 0 if the > host name is the tld only. can you add a comment that says that this part of the test is essentially checking that everything abides by the GetRegistryList TLD restriction? https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:271: ssl_error_classification.RecordUMAStatisticsDateInvalid( Actually, wouldn't it make more sense to just have one public method RecordUMAStatistics, then have a switch statement inside of it based on the error type that does the appropriate UMA stats? that way SSLBlockingPage doesn't need to handle any of this logic. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:55: // method checks to see whether |tokenized_potential_sundomain| is a subdomain typo: sundomain -> subdomain https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:59: int IsSubDomainDifference( since this returns an int now, you should rename it FindSubDomainDifference https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:65: return 0; what's the anticipated behavior if they are equivalent? should it be >= ? (I know, that can't happen right now because this should only be invoked if there's a mismatch. But you should still keep it in mind, because the calling logic for this method might change.) https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:152: nit: no empty line https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:214: bool SSLErrorClassification::IsWWWSubDomainMatch() const { can you document that you *do* accept the "inverse" in this case, and explain why? https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:227: || dns_names[i].length() == host_name.length()) { do you want to check IsNotValidURL for the dns names too? (same question for the others) https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:245: IsNotValidURL(request_url_)) { do you need to do the HostIsIPAddress and host_name.empty() checks, or are they redundant with what GetRegistryLength already checks? if you do need them, can you move the checks into IsNotValidUrl? https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:32: static bool IsNotValidURL(const GURL& url); does this need to be public? does it have a caller? https://codereview.chromium.org/376333003/diff/260001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/260001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46937: + <int value="6" label="SELF_SIGNED: The certificate is self_signed"/> have you run the script to format histograms.xml?
https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:267: SSLErrorClassification ssl_error_classification( On 2014/07/16 22:47:33, felt wrote: > you should create a single SSLErrorClassification object and call its methods > repeatedly Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:271: ssl_error_classification.RecordUMAStatisticsDateInvalid( On 2014/07/16 23:31:47, felt wrote: > Actually, wouldn't it make more sense to just have one public method > RecordUMAStatistics, then have a switch statement inside of it based on the > error type that does the appropriate UMA stats? that way SSLBlockingPage doesn't > need to handle any of this logic. Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:275: if (SSLErrorInfo::NetErrorToErrorType(cert_error_) == On 2014/07/16 22:47:33, felt wrote: > you should probably save the error type instead of calling NetErrorToErrorType > every time Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:55: // method checks to see whether |tokenized_potential_sundomain| is a subdomain On 2014/07/16 23:31:47, felt wrote: > typo: sundomain -> subdomain Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:59: int IsSubDomainDifference( On 2014/07/16 23:31:47, felt wrote: > since this returns an int now, you should rename it FindSubDomainDifference Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:65: return 0; On 2014/07/16 23:31:47, felt wrote: > what's the anticipated behavior if they are equivalent? should it be >= ? > > (I know, that can't happen right now because this should only be invoked if > there's a mismatch. But you should still keep it in mind, because the calling > logic for this method might change.) Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:152: On 2014/07/16 23:31:47, felt wrote: > nit: no empty line Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:214: bool SSLErrorClassification::IsWWWSubDomainMatch() const { On 2014/07/16 23:31:47, felt wrote: > can you document that you *do* accept the "inverse" in this case, and explain > why? Done. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:227: || dns_names[i].length() == host_name.length()) { On 2014/07/16 23:31:47, felt wrote: > do you want to check IsNotValidURL for the dns names too? (same question for the > others) Added a check. https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:245: IsNotValidURL(request_url_)) { On 2014/07/16 23:31:47, felt wrote: > do you need to do the HostIsIPAddress and host_name.empty() checks, or are they > redundant with what GetRegistryLength already checks? if you do need them, can > you move the checks into IsNotValidUrl? Yes they are redundant. Removed them https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:32: static bool IsNotValidURL(const GURL& url); On 2014/07/16 23:31:47, felt wrote: > does this need to be public? does it have a caller? Done. https://codereview.chromium.org/376333003/diff/260001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/260001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46937: + <int value="6" label="SELF_SIGNED: The certificate is self_signed"/> On 2014/07/16 23:31:47, felt wrote: > have you run the script to format histograms.xml? Yes I did run the pretty_print.py script.
https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:61: const std::vector<base::string16>& tokenized_potential_subdomian, Typo: "domian" should be "domain". https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:61: const std::vector<base::string16>& tokenized_potential_subdomian, Why base::string16? GURL::domain returns std::string. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:76: return diff_size; |diff_size| is a size_t, but you've declared this function to return int. Instead, declare the function to return size_t. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:297: std::vector<base::string16> dns_name_tokens; Also, this tokenizing code is repeated in all these functions. Don't do that; instead, tokenize once and redefine these functions to take the tokenized form as an argument. Faster at runtime, less code. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:299: base::SplitStringDontTrim(base::ASCIIToUTF16(dns_names[i]), Don't convert std::string to string16; it's unnecessarily expensive. There does exist a version of SplitStringDontTrim(const std::string&, ...). https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:305: int domain_diff = FindSubDomainDifference(dns_name_tokens, size_t https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:339: base::SplitStringDontTrim(base::ASCIIToUTF16(extracted_dns_name), same thing here, don't convert to string16. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:47: // This method checks whether the URL has a known Top Level Domain or not. Nit: This function checks hostname strings, not entire URLs. Make sure the comment is precise. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:48: static bool IsNotValidURL(const std::string& host_name); Naming: The comment does not exactly match the name of the function. Maybe call the function |URLHasKnownTLD| or something.
https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:61: const std::vector<base::string16>& tokenized_potential_subdomian, On 2014/07/17 20:11:19, Chromium Palmer wrote: > Why base::string16? GURL::domain returns std::string. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:61: const std::vector<base::string16>& tokenized_potential_subdomian, On 2014/07/17 20:11:19, Chromium Palmer wrote: > Why base::string16? GURL::domain returns std::string. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:76: return diff_size; On 2014/07/17 20:11:19, Chromium Palmer wrote: > |diff_size| is a size_t, but you've declared this function to return int. > Instead, declare the function to return size_t. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:297: std::vector<base::string16> dns_name_tokens; On 2014/07/17 20:11:19, Chromium Palmer wrote: > Also, this tokenizing code is repeated in all these functions. Don't do that; > instead, tokenize once and redefine these functions to take the tokenized form > as an argument. Faster at runtime, less code. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:299: base::SplitStringDontTrim(base::ASCIIToUTF16(dns_names[i]), On 2014/07/17 20:11:19, Chromium Palmer wrote: > Don't convert std::string to string16; it's unnecessarily expensive. There does > exist a version of SplitStringDontTrim(const std::string&, ...). Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:305: int domain_diff = FindSubDomainDifference(dns_name_tokens, On 2014/07/17 20:11:20, Chromium Palmer wrote: > size_t Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:339: base::SplitStringDontTrim(base::ASCIIToUTF16(extracted_dns_name), On 2014/07/17 20:11:19, Chromium Palmer wrote: > same thing here, don't convert to string16. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:47: // This method checks whether the URL has a known Top Level Domain or not. On 2014/07/17 20:11:20, Chromium Palmer wrote: > Nit: This function checks hostname strings, not entire URLs. Make sure the > comment is precise. Done. https://codereview.chromium.org/376333003/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:48: static bool IsNotValidURL(const std::string& host_name); On 2014/07/17 20:11:20, Chromium Palmer wrote: > Naming: The comment does not exactly match the name of the function. Maybe call > the function |URLHasKnownTLD| or something. Done.
looking good, only some very minor comments this time https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:171: if (IsHostNameKnownTLD(host_name)) { Suggestion: you might want to record how often the IsHostNameKnownTLD check fails here, with an UMA stat. https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:54: // certificate is "www.". My brain still can't parse this comment. Can you take another stab at it please? https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:24: std::vector<std::vector<std::string>> GetTokenizedDNSNames( I'm worried about having logic like this in a test function. Could you actually extract the DNS names and create a std::vector of known strings in the test, instead of doing this computation every time?
https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:171: if (IsHostNameKnownTLD(host_name)) { On 2014/07/21 21:34:13, felt wrote: > Suggestion: you might want to record how often the IsHostNameKnownTLD check > fails here, with an UMA stat. Done. https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:54: // certificate is "www.". On 2014/07/21 21:34:13, felt wrote: > My brain still can't parse this comment. Can you take another stab at it please? Done. https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/376333003/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:24: std::vector<std::vector<std::string>> GetTokenizedDNSNames( On 2014/07/21 21:34:13, felt wrote: > I'm worried about having logic like this in a test function. Could you actually > extract the DNS names and create a std::vector of known strings in the test, > instead of doing this computation every time? Done.
lgtm
https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:334: bool SSLErrorClassification::IsSubDomainOutsideWildcard( It just seems like there has got to already be a function in net/ or elsewhere in Chrome that does this check. https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:52: // A certificate for bank.com does not work for www.bank.com and vice versa. Looks like some kind of weird non-ASCII "i" in "certificate"? Maybe my browser is just glitching. Yeah, it looks like the fi ligature "fi". https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:62: const std::vector<std::string>& host_name_tokens, I don't like the names |host_name_tokens| and |dns_name_tokens| much, because — to me, at least — "hostname" and "DNS name" are nearly synonyms. Additionally, "_tokens" is a bit redundant, given the type of the argument (a vector of strings). The function names |IsSubDomainInverseMatch| and (to a lesser extent) |IsSubDomainMatch| bother me a bit; they aren't perfectly clear. How about: typedef std::vector<std::string> Tokens; // Returns true if |child| is a subdomain of any of the |potential_parents|. bool NameUnderAnyNames(const Tokens& child, const std::vector<Tokens>& potential_parents); // Returns true if any of the |potential_children| is a subdomain of any of the |parent|. bool AnyNamesUnderName(const std::vector<Tokens>& potential_children, const Tokens& parent); https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:74: // SSL certificate is "*.example.com". But, it retuns false for the hostname Typo: "retuns" --> "returns"
https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:334: bool SSLErrorClassification::IsSubDomainOutsideWildcard( Yeah, see if you can call X509Certificate::VerifyNameMatch or X509Certificate::VerifyHostname. In fact, there may be other functions of X509Certificate that you can re-use in this CL. https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:368: bool SSLErrorClassification::IsSelfSigned() const { Comparing CertPrincipals is not a real way to check for self-signedness; we actually need to check signatures. (See the comment for |Matches| in net/cert/x509_cert_types.h.) rsleevi suggests putting something in x509_util.h to check if OSCertHandle A was issued by OSCertHandle B. (OSCertHandle has signature-checking functions, unlike X509Certificate.)
https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:334: bool SSLErrorClassification::IsSubDomainOutsideWildcard( On 2014/07/23 01:53:22, Chromium Palmer wrote: > Yeah, see if you can call X509Certificate::VerifyNameMatch or > X509Certificate::VerifyHostname. > > In fact, there may be other functions of X509Certificate that you can re-use in > this CL. I actually did check both of those functions before implementing this function. As far as I understood, VerifyHostNameMatch will return true if a certificate matches a wildcard certificate but I need to check that when the wild card certificate does not match then is it of the form a.b.foo.com when the wildcard certificate is *.foo.com. VerifyNameMatch just calls VerifyHostNameMatch. That's the reason I did not use these functions. https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:368: bool SSLErrorClassification::IsSelfSigned() const { On 2014/07/23 01:53:22, Chromium Palmer wrote: > Comparing CertPrincipals is not a real way to check for self-signedness; we > actually need to check signatures. (See the comment for |Matches| in > net/cert/x509_cert_types.h.) > > rsleevi suggests putting something in x509_util.h to check if OSCertHandle A was > issued by OSCertHandle B. (OSCertHandle has signature-checking functions, unlike > X509Certificate.) I think so the correct way to check whether a certificate is self signed or not is by comparing in the OSCertHandle struct fields 1. Subject name and the issuer name are the same. 2. Subject Id and the issuer Id are the same. (I don't understand what we have to do with the signature field) Should I create this function in the x509_util.h file? https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:52: // A certificate for bank.com does not work for www.bank.com and vice versa. On 2014/07/23 01:22:06, Chromium Palmer wrote: > Looks like some kind of weird non-ASCII "i" in "certificate"? Maybe my browser > is just glitching. > > Yeah, it looks like the fi ligature "fi". Done. https://codereview.chromium.org/376333003/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:74: // SSL certificate is "*.example.com". But, it retuns false for the hostname On 2014/07/23 01:22:06, Chromium Palmer wrote: > Typo: "retuns" --> "returns" Done. https://codereview.chromium.org/376333003/diff/460001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/460001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:80: std::vector<std::string>& dns_names) const; I did not use Tokens out here because these are the dns_names of the certificate in the string field. they are not tokenized yet.
> > Yeah, see if you can call X509Certificate::VerifyNameMatch or > > X509Certificate::VerifyHostname. > > I actually did check both of those functions before implementing this function. > As far as I understood, VerifyHostNameMatch will return true if a certificate > matches a wildcard certificate but I need to check that when the wild card > certificate does not match then is it of the form http://a.b.foo.com when the wildcard > certificate is *.foo.com. > VerifyNameMatch just calls VerifyHostNameMatch. That's the reason I did not use > these functions. I see. > I think so the correct way to check whether a certificate is self signed or not > is by comparing in the OSCertHandle struct fields > 1. Subject name and the issuer name are the same. > 2. Subject Id and the issuer Id are the same. > (I don't understand what we have to do with the signature field) We need to check the chain of signatures as verified by the underlying platform crypto library. I've started an email thread to pinpoint the best way to do that.
On 2014/07/25 19:15:14, Chromium Palmer wrote: > > > Yeah, see if you can call X509Certificate::VerifyNameMatch or > > > X509Certificate::VerifyHostname. > > > > I actually did check both of those functions before implementing this > function. > > As far as I understood, VerifyHostNameMatch will return true if a certificate > > matches a wildcard certificate but I need to check that when the wild card > > certificate does not match then is it of the form http://a.b.foo.com when the > wildcard > > certificate is *.foo.com. > > VerifyNameMatch just calls VerifyHostNameMatch. That's the reason I did not > use > > these functions. > > I see. > > > I think so the correct way to check whether a certificate is self signed or > not > > is by comparing in the OSCertHandle struct fields > > 1. Subject name and the issuer name are the same. > > 2. Subject Id and the issuer Id are the same. > > (I don't understand what we have to do with the signature field) > > We need to check the chain of signatures as verified by the underlying platform > crypto library. I've started an email thread to pinpoint the best way to do > that. I have removed the IsSelfSigned() function from this CL and implementing it in a new one.
how's this going? palmer@, are there more changes needed?
Getting close. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:340: // This method is valid for wildcard certificates only. All documentation for functions should be in the .h file. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:362: base::SplitStringDontTrim(extracted_dns_name, As I think I said before, this repeated code block needs to be a separate function. Something like: Tokens Tokenize(const std::string& hostname); https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:37: // A method which calculates the severity score when the ssl error is // When the SSL error is |CERT_COMMON_NAME_INVALID|, returns // a severity score between 0.0 and 1.0 (higher values being // more severe). (Does the implementation handle the case when the SSL error is not |CERT_COMMON_NAME_INVALID|? Maybe just a DCHECK?) https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:54: static bool IsHostNameKnownTLD(const std::string& host_name); // Returns true if |hostname| has a known top-level domain. static bool HasKnownTLD(const std::string& hostname); (In technical English, "hostname" tends to be 1 word — like "nodename", but unlike "domain name". English is weird.) https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:60: bool IsWWWSubDomainMatch() const; This should be more concise, and maybe illustrate it with examples. // Returns true if the site's hostname differs from one of the DNS // names in the certificate (CN or SANs) only by the presence or // absence of the single-label prefix "www". E.g.: // // www.example.com ~ example.com -> true // example.com ~ www.example.com -> true // www.food.example.com ~ example.com -> false // mail.example.com ~ example.com -> false https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:66: // Returns true if any of the |potential_children| is a subdomain of any of "...is a subdomain of the |parent|." (I.e. remove "any of".) https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:76: bool IsSubDomainOutsideWildcard(const Tokens& host_name_tokens) const; // Returns true if |hostname| is too broad for the scope of a wildcard // certificate. E.g.: // // a.b.example.com ~ *.example.com --> true // b.example.com ~ *.example.com --> false bool IsSubDomainOutsideWildcard(const Tokens& hostname); https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:81: std::vector<std::string>& dns_names) const; |dns_names| can be a const &? Can this function be static? (Does it need to access any internal state of |this|?)
https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:340: // This method is valid for wildcard certificates only. On 2014/07/31 22:40:30, Chromium Palmer wrote: > All documentation for functions should be in the .h file. Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:362: base::SplitStringDontTrim(extracted_dns_name, On 2014/07/31 22:40:30, Chromium Palmer wrote: > As I think I said before, this repeated code block needs to be a separate > function. Something like: > > Tokens Tokenize(const std::string& hostname); It was in another CL. Forgot to merge it with this one. Done https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:37: // A method which calculates the severity score when the ssl error is On 2014/07/31 22:40:30, Chromium Palmer wrote: > // When the SSL error is |CERT_COMMON_NAME_INVALID|, returns > // a severity score between 0.0 and 1.0 (higher values being > // more severe). > > (Does the implementation handle the case when the SSL error is not > |CERT_COMMON_NAME_INVALID|? Maybe just a DCHECK?) Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:54: static bool IsHostNameKnownTLD(const std::string& host_name); On 2014/07/31 22:40:30, Chromium Palmer wrote: > // Returns true if |hostname| has a known top-level domain. > static bool HasKnownTLD(const std::string& hostname); > > (In technical English, "hostname" tends to be 1 word — like "nodename", but > unlike "domain name". English is weird.) Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:60: bool IsWWWSubDomainMatch() const; On 2014/07/31 22:40:30, Chromium Palmer wrote: > This should be more concise, and maybe illustrate it with examples. > > // Returns true if the site's hostname differs from one of the DNS > // names in the certificate (CN or SANs) only by the presence or > // absence of the single-label prefix "www". E.g.: > // > // http://www.example.com ~ http://example.com -> true > // http://example.com ~ http://www.example.com -> true > // http://www.food.example.com ~ http://example.com -> false > // http://mail.example.com ~ http://example.com -> false Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:66: // Returns true if any of the |potential_children| is a subdomain of any of On 2014/07/31 22:40:30, Chromium Palmer wrote: > "...is a subdomain of the |parent|." (I.e. remove "any of".) Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:76: bool IsSubDomainOutsideWildcard(const Tokens& host_name_tokens) const; On 2014/07/31 22:40:30, Chromium Palmer wrote: > // Returns true if |hostname| is too broad for the scope of a wildcard > // certificate. E.g.: > // > // http://a.b.example.com ~ *.example.com --> true > // http://b.example.com ~ *.example.com --> false > bool IsSubDomainOutsideWildcard(const Tokens& hostname); Done. https://codereview.chromium.org/376333003/diff/530001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:81: std::vector<std::string>& dns_names) const; On 2014/07/31 22:40:30, Chromium Palmer wrote: > |dns_names| can be a const &? > > Can this function be static? (Does it need to access any internal state of > |this|?) Done.
https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:146: if (type == SSLErrorInfo::CERT_DATE_INVALID) { Nit: A switch/case might be cleaner. switch (type) { case SSLErrorInfo::CERT_DATE_INVALID: { } // ... } https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:231: std::vector<std::vector<std::string>> SSLErrorClassification:: Nit: std::vector<Tokens> is easier to read. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:265: std::vector<std::string> SSLErrorClassification:: Nit: Use the Tokens typedef here too https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:320: // attack. We don't want foo.appspot.com to be able to MITM for appspot.com. This documentation should be in the header file. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:350: if (!(dns_names[i][0] == '*' && dns_names[i][1] == '.')) { I think (?) this is equivalent, and easier to read: const std::string& name = dns_names[i]; if (name.length() < 2 || name.length() >= host_name.length() || name.find('\0') != std::string::npos || !IsHostNameKnownTLD(name) || name[0] != '*" || name[1] != '.') { continue; } // Move past the "*.". std::string extracted_dns_name = name.substr(2); if (FindSubDomainDifference( host_name_tokens, Tokenize(extracted_dns_name)) == 2) { return true; } https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:25: static bool IsUserClockInThePast(base::Time time_now); Nit: Searching through the codebase shows a LOT of "const base::Time&" passed by reference instead of passing by copied value. However, the only data member of a |base::Time| is a single int64, so as a practical matter it doesn't make a difference, performance-wise. But you might want to stick to standard practice. Up to you. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:34: // CERT_DATE_INVALID, returns a severity score between 0.0. and 1.0 (higher Nit: Be more concise: // Returns a score between 0.0 and 1.0, higher values being more // severe, indicating how severe the certificate's invalid date error // is. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // more severe). Nit: Prefer the more concise style, as above.
https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:146: if (type == SSLErrorInfo::CERT_DATE_INVALID) { On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: A switch/case might be cleaner. > > switch (type) { > case SSLErrorInfo::CERT_DATE_INVALID: { > > } > > // ... > } Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:231: std::vector<std::vector<std::string>> SSLErrorClassification:: On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: std::vector<Tokens> is easier to read. Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:265: std::vector<std::string> SSLErrorClassification:: On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: Use the Tokens typedef here too Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:320: // attack. We don't want foo.appspot.com to be able to MITM for appspot.com. On 2014/08/04 23:48:29, Chromium Palmer wrote: > This documentation should be in the header file. Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:350: if (!(dns_names[i][0] == '*' && dns_names[i][1] == '.')) { On 2014/08/04 23:48:29, Chromium Palmer wrote: > I think (?) this is equivalent, and easier to read: > > const std::string& name = dns_names[i]; > if (name.length() < 2 || name.length() >= host_name.length() || > name.find('\0') != std::string::npos || > !IsHostNameKnownTLD(name) || > name[0] != '*" || name[1] != '.') { > continue; > } > > // Move past the "*.". > std::string extracted_dns_name = name.substr(2); > if (FindSubDomainDifference( > host_name_tokens, Tokenize(extracted_dns_name)) == 2) { > return true; > } Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:25: static bool IsUserClockInThePast(base::Time time_now); On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: Searching through the codebase shows a LOT of "const base::Time&" passed by > reference instead of passing by copied value. However, the only data member of a > |base::Time| is a single int64, so as a practical matter it doesn't make a > difference, performance-wise. But you might want to stick to standard practice. > Up to you. Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:34: // CERT_DATE_INVALID, returns a severity score between 0.0. and 1.0 (higher On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: Be more concise: > > // Returns a score between 0.0 and 1.0, higher values being more > // severe, indicating how severe the certificate's invalid date error > // is. Done. https://codereview.chromium.org/376333003/diff/590001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // more severe). On 2014/08/04 23:48:29, Chromium Palmer wrote: > Nit: Prefer the more concise style, as above. Done.
LGTM
On 2014/08/05 17:38:18, Chromium Palmer wrote: > LGTM @mpearson - Could you PTAL @ histograms.xml file and give owners approval.
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48048: + <int value="2" How where these buckets evaluated in the past? Were these conditions simply not emitted previously? Also, I'd mildly encourage you to rename the histogram, lest four months down the line you wonder why these types of failure modes are suddenly increasing and why clock errors are a decreasing percentage of total errors. https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48056: + wildcard certificate"/> Am I correct in assuming this will only be emitted for certificates that use wildcards? What are the priority of these errors? I can imagine multiple of these errors occur on the same certificate. Hopefully you can put something here or in the description that clarifies which errors get counted when.
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48048: + <int value="2" On 2014/08/05 22:07:27, Mark P wrote: > How where these buckets evaluated in the past? Were these conditions simply not > emitted previously? > > > Also, I'd mildly encourage you to rename the histogram, lest four months down > the line you wonder why these types of failure modes are suddenly increasing and > why clock errors are a decreasing percentage of total errors. These buckets weren't evaluated in the past but these conditions were emitted previously. Done - renamed the histogram https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48056: + wildcard certificate"/> On 2014/08/05 22:07:27, Mark P wrote: > Am I correct in assuming this will only be emitted for certificates that use > wildcards? > > What are the priority of these errors? I can imagine multiple of these errors > occur on the same certificate. Hopefully you can put something here or in the > description that clarifies which errors get counted when. We haven't prioritized the errors and the SUBDOMIAN_OUTSIDE_WILDCARD is the only error which is for wildcards and all these errors have been implemented such that they are mutually exclusive. Before making changes, I just want to be sure - should I just add an example of what each error is?
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48048: + <int value="2" On 2014/08/06 00:10:18, radhikabhar wrote: > On 2014/08/05 22:07:27, Mark P wrote: > > How where these buckets evaluated in the past? Were these conditions simply > not > > emitted previously? > > > > > > Also, I'd mildly encourage you to rename the histogram, lest four months down > > the line you wonder why these types of failure modes are suddenly increasing > and > > why clock errors are a decreasing percentage of total errors. > > These buckets weren't evaluated in the past but these conditions were emitted > previously. > > Done - renamed the histogram Wait, please don't rename my histogram. Mark, why do you say Radhika needs to rename the histogram?
On 2014/08/06 01:00:24, felt wrote: > https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:48048: + <int value="2" > On 2014/08/06 00:10:18, radhikabhar wrote: > > On 2014/08/05 22:07:27, Mark P wrote: > > > How where these buckets evaluated in the past? Were these conditions simply > > not > > > emitted previously? > > > > > > > > > Also, I'd mildly encourage you to rename the histogram, lest four months > down > > > the line you wonder why these types of failure modes are suddenly increasing > > and > > > why clock errors are a decreasing percentage of total errors. > > > > These buckets weren't evaluated in the past but these conditions were emitted > > previously. > > > > Done - renamed the histogram > > Wait, please don't rename my histogram. Mark, why do you say Radhika needs to > rename the histogram? Please read earlier on this comment thread.
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48056: + wildcard certificate"/> On 2014/08/06 00:10:18, radhikabhar wrote: > On 2014/08/05 22:07:27, Mark P wrote: > > Am I correct in assuming this will only be emitted for certificates that use > > wildcards? > > > > What are the priority of these errors? I can imagine multiple of these errors > > occur on the same certificate. Hopefully you can put something here or in the > > description that clarifies which errors get counted when. > > We haven't prioritized the errors I think you have implicitly. For instance, the difference between the URL and the DNS is www is also a case when URL is a subdomain of the DNS. I assume you're emitting to to the former bucket and not (also) the latter. Likewise, I can imagine, say, self-signed certificates that also provide DNS that's a subdomain of the URL. You say "all these errors have been implemented such that they are mutually exclusive" so clearly you have a rule that says in that particular situation, only omit to, say, the latter bucket and not the former. This histogram should explain these rules about how you record errors that could be categorized into multiple buckets. > and the SUBDOMIAN_OUTSIDE_WILDCARD is the only > error which is for wildcards and all these errors have been implemented such > that they are mutually exclusive. > Before making changes, I just want to be sure - should I just add an example of > what each error is?
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48048: + <int value="2" HiOn 2014/08/06 01:00:24, felt wrote: > On 2014/08/06 00:10:18, radhikabhar wrote: > > On 2014/08/05 22:07:27, Mark P wrote: > > > How where these buckets evaluated in the past? Were these conditions simply > > not > > > emitted previously? > > > > > > > > > Also, I'd mildly encourage you to rename the histogram, lest four months > down > > > the line you wonder why these types of failure modes are suddenly increasing > > and > > > why clock errors are a decreasing percentage of total errors. > > > > These buckets weren't evaluated in the past but these conditions were emitted > > previously. > > > > Done - renamed the histogram > > Wait, please don't rename my histogram. Mark, why do you say Radhika needs to > rename the histogram? I don't understand the first part of your comment. Maybe this will clear it up. Two of the conditions were emitted previously (the ones I added) and the others were not (this CL adds them). The relative frequency in the histogram doesn't matter because the math is done (condition)/(all SSL errors), and all SSL errors is recorded in a different counter. This histogram was intended to expand.
https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48048: + <int value="2" On 2014/08/06 19:32:36, felt wrote: > HiOn 2014/08/06 01:00:24, felt wrote: > > On 2014/08/06 00:10:18, radhikabhar wrote: > > > On 2014/08/05 22:07:27, Mark P wrote: > > > > How where these buckets evaluated in the past? Were these conditions > simply > > > not > > > > emitted previously? > > > > > > > > > > > > Also, I'd mildly encourage you to rename the histogram, lest four months > > down > > > > the line you wonder why these types of failure modes are suddenly > increasing > > > and > > > > why clock errors are a decreasing percentage of total errors. > > > > > > These buckets weren't evaluated in the past but these conditions were > emitted > > > previously. > > > > > > Done - renamed the histogram > > > > Wait, please don't rename my histogram. Mark, why do you say Radhika needs to > > rename the histogram? > > I don't understand the first part of your comment. Maybe this will clear it up. > Two of the conditions were emitted previously (the ones I added) and the others > were not (this CL adds them). The relative frequency in the histogram doesn't > matter because the math is done (condition)/(all SSL errors), and all SSL errors > is recorded in a different counter. This histogram was intended to expand. Then this should be explained in the (two) histogram descriptions. The description should clearly point to the "all SSL errors" histogram and remark that many errors are not reported in this particular histogram, and that new errors have been added to this histogram over time so one should not look the breakdown of this histogram (one bucket divided by the sum) because that would be misleading and inaccurate.
On 2014/08/06 20:19:29, Mark P wrote: > https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/376333003/diff/650001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:48048: + <int value="2" > On 2014/08/06 19:32:36, felt wrote: > > HiOn 2014/08/06 01:00:24, felt wrote: > > > On 2014/08/06 00:10:18, radhikabhar wrote: > > > > On 2014/08/05 22:07:27, Mark P wrote: > > > > > How where these buckets evaluated in the past? Were these conditions > > simply > > > > not > > > > > emitted previously? > > > > > > > > > > > > > > > Also, I'd mildly encourage you to rename the histogram, lest four months > > > down > > > > > the line you wonder why these types of failure modes are suddenly > > increasing > > > > and > > > > > why clock errors are a decreasing percentage of total errors. > > > > > > > > These buckets weren't evaluated in the past but these conditions were > > emitted > > > > previously. > > > > > > > > Done - renamed the histogram > > > > > > Wait, please don't rename my histogram. Mark, why do you say Radhika needs > to > > > rename the histogram? > > > > I don't understand the first part of your comment. Maybe this will clear it > up. > > Two of the conditions were emitted previously (the ones I added) and the > others > > were not (this CL adds them). The relative frequency in the histogram doesn't > > matter because the math is done (condition)/(all SSL errors), and all SSL > errors > > is recorded in a different counter. This histogram was intended to expand. > > Then this should be explained in the (two) histogram descriptions. The > description should clearly point to the "all SSL errors" histogram and remark > that many errors are not reported in this particular histogram, and that new > errors have been added to this histogram over time so one should not look the > breakdown of this histogram (one bucket divided by the sum) because that would > be misleading and inaccurate. Added description to the histogram.xml file
https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9716: + of the SSL error that exists when it is recorded. "the total count of the type of the SSL error that exists when it is recorded" where is this recorded? I hope in rewriting that fragment with a pointer you'll end up explaining better what it means because I don't understand the phrase right now either. https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48076: + have received a wildcard certificate and the hostname is too broad for the This seems like an odd use of the term "broad". Perhaps a better phrasing would be that "the scope of the wildcard certificate is too narrow for the hostname." https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48077: + scope of a wildcard certificate. What happens in a situation like: wildcard certificate *.www.google.com hostname: google.com This feels like bucket 2 and bucket 5. https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48078: + </int> What happened to 6? https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48081: + This cause is recorded only for CERT_COMMON_NAME_INVALID errors. What happens with DNS = www.asdf URL = asdf Is this bucket 2 or bucket 7?
https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9716: + of the SSL error that exists when it is recorded. On 2014/08/06 21:35:50, Mark P wrote: > "the total count of the type of the SSL error that exists when it is recorded" > where is this recorded? > > I hope in rewriting that fragment with a pointer you'll end up explaining better > what it means because I don't understand the phrase right now either. Done. https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48076: + have received a wildcard certificate and the hostname is too broad for the On 2014/08/06 21:35:50, Mark P wrote: > This seems like an odd use of the term "broad". > > Perhaps a better phrasing would be that "the scope of the wildcard certificate > is too narrow for the hostname." https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48077: + scope of a wildcard certificate. On 2014/08/06 21:35:50, Mark P wrote: > What happens in a situation like: > wildcard certificate *.www.google.com > hostname: http://google.com > > This feels like bucket 2 and bucket 5. This case will actually never be recorded. https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48078: + </int> On 2014/08/06 21:35:50, Mark P wrote: > What happened to 6? Done. Removed Self-Signed because that is not implemented in this CL. Forgor to change the number. https://codereview.chromium.org/376333003/diff/750001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48081: + This cause is recorded only for CERT_COMMON_NAME_INVALID errors. On 2014/08/06 21:35:50, Mark P wrote: > What happens with > DNS = http://www.asdf > URL = asdf > > Is this bucket 2 or bucket 7? This is bucket 7 (now bucket 6) only because if the host name is not a known TLD then none of the above buckets are recorded. Added this fact to the description.
Almost there. :-) https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9713: + overtime, therefore one should not look at the breakdown of this histogram nit: overtime -> over time https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9715: + one should look at each bucket count divided by the ssl error. E.g. WWW nit: the ssl error -> the count of ssl errors of that type https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9719: + interstitial.ssl_error_type. This is much nicer; thank you for your patience. https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48079: + difference between the DNS name and the DNS name is not "www". one of these DNS names should be URL https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48081: + = false. This case is not recorded if the host name is not a known TLD. The example doesn't help (me) at all. I suggest you remove it. If you want to keep it, you should make it clear that the first element in those ~ operations is the DNS and the second is the URL (wildcard), and by true/false you mean whether it's an error and would be recorded in this bucket. You don't have to keep the example.
https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9713: + overtime, therefore one should not look at the breakdown of this histogram On 2014/08/06 22:25:56, Mark P wrote: > nit: overtime -> over time Done. https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9715: + one should look at each bucket count divided by the ssl error. E.g. WWW On 2014/08/06 22:25:56, Mark P wrote: > nit: > the ssl error > -> > the count of ssl errors of that type Done. https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48079: + difference between the DNS name and the DNS name is not "www". On 2014/08/06 22:25:56, Mark P wrote: > one of these DNS names should be URL Done. https://codereview.chromium.org/376333003/diff/790001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48081: + = false. This case is not recorded if the host name is not a known TLD. On 2014/08/06 22:25:56, Mark P wrote: > The example doesn't help (me) at all. I suggest you remove it. > > If you want to keep it, you should make it clear that the first element in those > ~ operations is the DNS and the second is the URL (wildcard), and by true/false > you mean whether it's an error and would be recorded in this bucket. > > You don't have to keep the example. Removed the example.
histograms.xml lgtm
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/376333003/770002
Message was sent while issue was closed.
Change committed as 287942 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
