New SSL metric added: Likely From Same Domain

New SSL metric added: Likely From Same Domain

This metric records the CERT_COMMON_NAME_INVALID errors when the hostname entered and one of the dns names of the certificate have same effective domain name (eTLD+1).This helps us see if the certificate is completely random or if it is related to the current hostname.

BUG=507715
Committed: https://crrev.com/a250ccfb581fb9353c0efe18789df49a91ddb0a5
Cr-Commit-Position: refs/heads/master@{#339699}

[Bhanu Dev, 2015-07-09 17:34:35]

bhanudev@google.com changed reviewers:
+ meacer@chromium.org, palmer@chromium.org

[palmer, 2015-07-09 19:33:14]

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:472: std::vector<std::string>
dns_names_domain;
Declare this closer to where it is used: Just above the for loop that starts on
line 479.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:475: // Add current host name to
dns_names vector.
This comment is superfluous.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:489: for (size_t i = 0; i <
dns_names_domain_size - 1; ++i) {
You might be able to use std::find here instead of an explicit for loop?

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:495: 
No blank line here.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.h (right):

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.h:120: // Returns true if the
hostname entered and atleast one of the dns names have
Be more concise and more precise:

Returns true if the hostname in |request_url_| has the same domain (effective
TLD + 1 label) as at least one of the subject alternative names in |cert_|.

https://codereview.chromium.org/1227173006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67876: +    This cause is recorded only
for CERT_COMMON_NAME_INVALID errors when the
cause -> case

tld -> TLD

Use the same concise and precise wording as in the documentation for
|IsCertLikelyFromSameDomain|.

[Bhanu Dev, 2015-07-09 22:49:52]

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:472: std::vector<std::string>
dns_names_domain;
On 2015/07/09 19:33:13, palmer wrote:
> Declare this closer to where it is used: Just above the for loop that starts
on
> line 479.

Done.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:475: // Add current host name to
dns_names vector.
On 2015/07/09 19:33:13, palmer wrote:
> This comment is superfluous.

Done.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:489: for (size_t i = 0; i <
dns_names_domain_size - 1; ++i) {
On 2015/07/09 19:33:13, palmer wrote:
> You might be able to use std::find here instead of an explicit for loop?

Done.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:495: 
On 2015/07/09 19:33:13, palmer wrote:
> No blank line here.

Done.

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.h (right):

https://codereview.chromium.org/1227173006/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.h:120: // Returns true if the
hostname entered and atleast one of the dns names have
On 2015/07/09 19:33:13, palmer wrote:
> Be more concise and more precise:
> 
> Returns true if the hostname in |request_url_| has the same domain (effective
> TLD + 1 label) as at least one of the subject alternative names in |cert_|.

Done.

https://codereview.chromium.org/1227173006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67876: +    This cause is recorded only
for CERT_COMMON_NAME_INVALID errors when the
On 2015/07/09 19:33:13, palmer wrote:
> cause -> case
> 
> tld -> TLD
> 
> Use the same concise and precise wording as in the documentation for
> |IsCertLikelyFromSameDomain|.

Done.

[meacer, 2015-07-11 00:26:07]

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:478: for (size_t i = 0; i <
dns_names_size; ++i) {
Use C++11 style loop:

for (const string& dns_name: dns_names)

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:486: size_t dns_names_domain_size
= dns_names_domain.size();
This is just used once, no need to create a separate variable.

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:487: std::string host_name_domain
= dns_names_domain[dns_names_domain_size - 1];
Need to check if dns_names_domain_size==0. In fact, just do an early exit after
cert_.GetDNSNames in that case.

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:489: return
std::find(dns_names_domain.begin(), dns_names_domain.end() - 1,
You might want to add a small comment here saying why you are excluding the last
item (that it's the original domain pushed above).

https://codereview.chromium.org/1227173006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67877: +    the hostname in request url
has the same domain (effective TLD + 1 label) as
url -> URL

[Bhanu Dev, 2015-07-13 19:22:43]

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:478: for (size_t i = 0; i <
dns_names_size; ++i) {
On 2015/07/11 00:26:07, Mustafa Emre Acer wrote:
> Use C++11 style loop:
> 
> for (const string& dns_name: dns_names)

Done.

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:486: size_t dns_names_domain_size
= dns_names_domain.size();
On 2015/07/11 00:26:07, Mustafa Emre Acer wrote:
> This is just used once, no need to create a separate variable.

Done.

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:487: std::string host_name_domain
= dns_names_domain[dns_names_domain_size - 1];
On 2015/07/11 00:26:07, Mustafa Emre Acer wrote:
> Need to check if dns_names_domain_size==0. In fact, just do an early exit
after
> cert_.GetDNSNames in that case.

I think dns_names_domain_size cannot be 0, since host_name is appended to it. I
added a DCHECK.

https://codereview.chromium.org/1227173006/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:489: return
std::find(dns_names_domain.begin(), dns_names_domain.end() - 1,
On 2015/07/11 00:26:07, Mustafa Emre Acer wrote:
> You might want to add a small comment here saying why you are excluding the
last
> item (that it's the original domain pushed above).

Done.

https://codereview.chromium.org/1227173006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67877: +    the hostname in request url
has the same domain (effective TLD + 1 label) as
On 2015/07/11 00:26:07, Mustafa Emre Acer wrote:
> url -> URL

Done.

[meacer, 2015-07-15 20:40:36]

Lgtm

https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68281: +    This case is recorded if the
ssl error is CERT_COMMON_NAME_INVALID error and
nit: ssl -> SSL

[Bhanu Dev, 2015-07-15 22:51:07]

https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68281: +    This case is recorded if the
ssl error is CERT_COMMON_NAME_INVALID error and
On 2015/07/15 20:40:36, Mustafa Emre Acer wrote:
> nit: ssl -> SSL

Done.

[Bhanu Dev, 2015-07-16 18:38:28]

On 2015/07/15 22:51:07, Bhanu Dev wrote:
>
https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1227173006/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:68281: +    This case is recorded if
the
> ssl error is CERT_COMMON_NAME_INVALID error and
> On 2015/07/15 20:40:36, Mustafa Emre Acer wrote:
> > nit: ssl -> SSL
> 
> Done.

+palmer

Chris, Do you have any comments? Thanks.

[palmer, 2015-07-16 19:08:21]

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:485: std::string host_name_domain
= dns_names_domain[dns_names_domain.size() - 1];
const std::string& host_name_domain = dns_names_domain.back();

Take a const & for efficiency and const-correctness, and |back| is more concise.

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:488: return
std::find(dns_names_domain.begin(), dns_names_domain.end() - 1,
I'm not sure if that kind of pointer arithmetic is valid on iterators? It
probably is, but... ask someone who knows C++ STL better than I do. :)

[Bhanu Dev, 2015-07-20 21:59:42]

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:485: std::string host_name_domain
= dns_names_domain[dns_names_domain.size() - 1];
On 2015/07/16 19:08:21, palmer wrote:
> const std::string& host_name_domain = dns_names_domain.back();
> 
> Take a const & for efficiency and const-correctness, and |back| is more
concise.

Done.

https://codereview.chromium.org/1227173006/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.cc:488: return
std::find(dns_names_domain.begin(), dns_names_domain.end() - 1,
On 2015/07/16 19:08:21, palmer wrote:
> I'm not sure if that kind of pointer arithmetic is valid on iterators? It
> probably is, but... ask someone who knows C++ STL better than I do. :)

http://www.cplusplus.com/reference/iterator/ says that iterator supports
arithmetic operators. Similar iterator arithmetic is used previously, an example
at
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_resp....
Thanks

[palmer, 2015-07-20 22:40:42]

lgtm

[Bhanu Dev, 2015-07-20 22:59:37]

bhanudev@google.com changed reviewers:
+ asvitkine@chromium.org

[Bhanu Dev, 2015-07-20 22:59:38]

+asvitkine, Owners review for histograms.xml file. PTAL? Thanks.

[Alexei Svitkine (slow), 2015-07-21 15:42:49]

lgtm

[Bhanu Dev, 2015-07-21 17:55:54]

The CQ bit was checked by bhanudev@google.com

[Bhanu Dev, 2015-07-21 17:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1227173006/#ps100001
(title: "resolving comments")

[commit-bot: I haz the power, 2015-07-21 17:57:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227173006/100001

[commit-bot: I haz the power, 2015-07-21 18:16:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-21 18:18:28]

Patchset 6 (id:??) landed as
https://crrev.com/a250ccfb581fb9353c0efe18789df49a91ddb0a5
Cr-Commit-Position: refs/heads/master@{#339699}