Add UMA entry for intranet SSL warnings

Adds new UMA entries to give us SSL clickthrough rates specific to intranet sites. Relies on value of IsHostnameNonUnique.

This CL also moves CertVerifyProc::IsHostnameNonUnique into net_util.

BUG=225570
R=rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215428

[felt, 2013-08-01 16:04:30]

Hi Ryan,

Can you please review this CL?  This adds UMA entries for internal networks,
using IsHostnameNonUnique.

There will be two follow-up CLs:
-- One to extend IsHostnameNonUnique to handle IPs
-- Another to modify the internal histograms.xml to have the new enum values

Adrienne

[felt, 2013-08-01 16:10:57]

On 2013/08/01 16:04:30, felt wrote:
> Hi Ryan,
> 
> Can you please review this CL?  This adds UMA entries for internal networks,
> using IsHostnameNonUnique.
> 
> There will be two follow-up CLs:
> -- One to extend IsHostnameNonUnique to handle IPs
> -- Another to modify the internal histograms.xml to have the new enum values
> 
> Adrienne

P.S. Do you know of any good way to end-to-end test this?

[Ryan Sleevi, 2013-08-01 17:36:15]

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
chrome/browser/ssl/ssl_blocking_page.cc:117: if (internal)
RecordSSLBlockingPageEventStats(PROCEED_INTERNAL_HOSTNAME);
Not recording reject for internals?

On a style nit side, though
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
permits it, it seems uncommon in this file and section of code to use one-line
conditionals, compared to the

if (internal)
  RecordSSL...

form

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
chrome/browser/ssl/ssl_blocking_page.cc:212: base::TimeTicks::Now());
style: Can you clang-format / style guide format this? The current style doesn't
seem to match
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Same for the later calls.

[Ryan Sleevi, 2013-08-01 17:39:01]

Oh, and for the E2E tests, look at StatisticsRecorder. You can see that some
unittests use it to make sure that histogram data is appropriately logged.

[felt, 2013-08-01 19:00:28]

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
chrome/browser/ssl/ssl_blocking_page.cc:117: if (internal)
RecordSSLBlockingPageEventStats(PROCEED_INTERNAL_HOSTNAME);
> Not recording reject for internals?

We don't need to; SHOW - PROCEED = REJECT. We can record any two of the three.

> On a style nit side, though
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
> permits it, it seems uncommon in this file and section of code to use one-line
> conditionals, compared to the
> 
> if (internal)
>   RecordSSL...
> 
> form

OK, split into two lines.

https://codereview.chromium.org/21572002/diff/1/chrome/browser/ssl/ssl_blocki...
chrome/browser/ssl/ssl_blocking_page.cc:212: base::TimeTicks::Now());
On 2013/08/01 17:36:15, Ryan Sleevi wrote:
> style: Can you clang-format / style guide format this? The current style
doesn't
> seem to match
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Same for the later calls.

Done, substantially less ugly now.

[Ryan Sleevi, 2013-08-01 19:03:55]

LGTM, mod some shuffling of deck-chairs :)

https://codereview.chromium.org/21572002/diff/18001/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/21572002/diff/18001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc.h:78: static bool IsHostnameNonUnique(const
std::string& hostname);
I don't know what happened, but my original comment was lost and I missed it.

I suggest that this be moved from CVP into net_util. CVP is more of an
implementation of net/. It also fits more in net_util, since that's where the
existing IP address functionality (localhost, reserved, cidr mapping) lives, and
this will logically fit with http://crbug.com/119212

[felt, 2013-08-01 20:32:22]

https://codereview.chromium.org/21572002/diff/18001/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/21572002/diff/18001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc.h:78: static bool IsHostnameNonUnique(const
std::string& hostname);
On 2013/08/01 19:03:55, Ryan Sleevi wrote:
> I don't know what happened, but my original comment was lost and I missed it.
> 
> I suggest that this be moved from CVP into net_util. CVP is more of an
> implementation of net/. It also fits more in net_util, since that's where the
> existing IP address functionality (localhost, reserved, cidr mapping) lives,
and
> this will logically fit with http://crbug.com/119212

Done, moved it over.

[Ryan Sleevi, 2013-08-01 21:37:54]

LGTM, mod comment nits! Sorry about that.

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.cc#newco...
net/base/net_util.cc:1410: // trusted CAs.
nit: comment update re: "trusted CAs"

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.h#newcod...
net/base/net_util.h:115: // certificates (eg: an "internal server name").
nit: Probably need to update this comment to remove cert references


// Returns true if |hostname| contains a non-registerable or
// non-assignable domain name (eg: a gTLD that has not been
// assigned by IANA)
//
// TODO(rsleevi): http://crbug.com/119212 - Also match internal IP
// address ranges.


The notes about CA/B Forum should remain in CertVerifyProc in some form
(probably just the implementation file)

https://codereview.chromium.org/21572002/diff/8002/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/21572002/diff/8002/net/cert/cert_verify_proc....
net/cert/cert_verify_proc.h:75: friend class CertVerifyProcNonUniqueNameTest;
can remove this line.

[felt, 2013-08-01 22:00:24]

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.cc#newco...
net/base/net_util.cc:1410: // trusted CAs.
On 2013/08/01 21:37:55, Ryan Sleevi wrote:
> nit: comment update re: "trusted CAs"

Not entirely sure what you want here -- I removed the part of the TODO.

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/21572002/diff/8002/net/base/net_util.h#newcod...
net/base/net_util.h:115: // certificates (eg: an "internal server name").
On 2013/08/01 21:37:55, Ryan Sleevi wrote:
> nit: Probably need to update this comment to remove cert references
> 
> 
> // Returns true if |hostname| contains a non-registerable or
> // non-assignable domain name (eg: a gTLD that has not been
> // assigned by IANA)
> //
> // TODO(rsleevi): http://crbug.com/119212 - Also match internal IP
> // address ranges.
> 
> 
> The notes about CA/B Forum should remain in CertVerifyProc in some form
> (probably just the implementation file)

Most of the comment is in the implementation file at the use site of
IsHostnameNonUnique; I added the TODO, which was really the only thing missing
there.

https://codereview.chromium.org/21572002/diff/8002/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/21572002/diff/8002/net/cert/cert_verify_proc....
net/cert/cert_verify_proc.h:75: friend class CertVerifyProcNonUniqueNameTest;
On 2013/08/01 21:37:55, Ryan Sleevi wrote:
> can remove this line.

Done.

[felt, 2013-08-01 22:02:13]

Adam, despite your valiant quest to remove yourself from OWNERS of
chrome/browser/ssl/... your OWNER stamp is still needed here for
chrome/browser/ssl/ssl_blocking_page.cc|h. :) Can you please review?

[abarth-chromium, 2013-08-01 22:10:59]

ssl <-- LGTM

[commit-bot: I haz the power, 2013-08-02 19:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/21572002/44001

[commit-bot: I haz the power, 2013-08-03 00:45:03]

Change committed as 215428