Report second-level domains for UMA on pin failure.

Report second-level domains for UMA on pin failure.

Fix some minor "gcl lint" results in these files while I'm at it.

Implementation notes:

1. ScanForHostAndCerts tried to do too much: both search the list for an
HSTS preload and also set one limited criterion (check on required_hashes).
It also returned too little, limiting its usefulness. Replaced it with
GetHSTSPreload which just implements the "return a match; prefer exact
match" semantics and returns the HSTSPreload entry. The caller can then
apply their arbitrary decision criterion (as in IsGooglePinnedProperty) or
use any field from the entry (as in ReportUMAPinFailure). This makes its
interface and implementation simpler, and makes it more useful to more
callers.

2. The IsGooglePinnedProperty unit test still passes, in light of (1).

3. I needed to get from a full hostname to an enum value that I can send in
an UMA_HISTOGRAM_ENUMERATION. Baking the enums into struct HSTSPreload was
the most straightforward and least-bloated way of doing that.

BUG=99782
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107993

[wtc, 2011-10-25 00:49:57]

Review comments on Patch Set 2:

I believe all the issues I found are style issues.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:41: static const char kPinFailure[] =
"SSL.PinFailure";

Same here: please clarify what kind of pin failure this is.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:254: DCHECK(tokenizer.token().length() ==
1);

Nit: it may be better to use DCHECK_EQ.  If you use DCHECK_EQ,
the constant 1 may need to be unsigned 1U.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:813: typedef enum {

Can you document what this enum is for?

I guess the values of the constants in this enum must not
change.  If so, please document that.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:814: DOMAIN_GOOGLE_COM = 0,

Unless our Style Guide requires this, it is not necessary
to initialize the first enum constant to 0 because that is
the default.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:876: DOMAIN_NUM_EVENTS

NUM_EVENTS sounds strange because these aren't events.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:877: } LowResolutionDomainName;
  typedef enum {
    ...
  } LowResolutionDomainName;

is the C way.  In C++, you can simply say:

  enum LowResolutionDomainName {
    ...
  };

-- A C programmer

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:1286: kNumPreloadedSNISTS);
Nit: put curly braces around the "if" body because it is
two lines (even though one statement).

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:1292: DOMAIN_NUM_EVENTS);

I checked with jar, the father of Histogram.

Here you did the right thing: by passing the largest sample + 1
as the third argument, you will be able to add new values
to the *end* of the LowResolutionDomainName enum type in
the future without breaking this histogram.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.h:132: // second-level domain of |host| (e.g.
google.com), and only if |host| is

The example should say something like
"google.com if |host| is mail.google.com".

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.h:133: // an IsPreloadedSTS host (or, if
|sni_available|, an SNI preloaded STS

Nit: IsPreloadedSTS is a private method, so it probably shouldn't
be used in the comment for a public method.

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.h:135: static void ReportUMAPinFailure(const
std::string& host, bool sni_available);

"PinFailure" needs to be more specific, such as "CertPinFailure"
or "PublicKeyPinFailure".

The comment on line 131 ("pin failure") needs a similar change.

Also, "ReportUMAPinFailure" sounds like we want to report
"UMA pin" failure.  I think we need to add "On":
    ReportUMAOnPinFailure
or remove "UMA":
    ReportPinFailure

[agl, 2011-10-25 15:00:00]

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
net/base/transport_security_state.cc:813: typedef enum {
This typeful is significantly larger than the set of hosts for which we have
pinning. Since we wouldn't accept most of these hosts for pinning in any case,
is there any point in including them now?

[palmer, 2011-10-25 21:41:04]

>
http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
> net/base/transport_security_state.cc:41: static const char kPinFailure[] =
> "SSL.PinFailure";
> 
> Same here: please clarify what kind of pin failure this is.

Done; renamed to kPublicKeyPinFailure[] = "SSL.PublicKeyPinFailure";.

> net/base/transport_security_state.cc:254: DCHECK(tokenizer.token().length() ==
> 1);
> 
> Nit: it may be better to use DCHECK_EQ.  If you use DCHECK_EQ,
> the constant 1 may need to be unsigned 1U.

Done.

> Can you document what this enum is for?
> 
> I guess the values of the constants in this enum must not
> change.  If so, please document that.

Done.

> net/base/transport_security_state.cc:814: DOMAIN_GOOGLE_COM = 0,
> 
> Unless our Style Guide requires this, it is not necessary
> to initialize the first enum constant to 0 because that is
> the default.

Done.

> net/base/transport_security_state.cc:876: DOMAIN_NUM_EVENTS
> 
> NUM_EVENTS sounds strange because these aren't events.

Well, they are events that are UMA-logged. I followed the convention in
chrome_render_view_observer.cc.

> is the C way.  In C++, you can simply say:
> 
>   enum LowResolutionDomainName {

Done.

> Nit: put curly braces around the "if" body because it is
> two lines (even though one statement).

Done.

> net/base/transport_security_state.h:132: // second-level domain of |host|
(e.g.
> http://google.com), and only if |host| is
> 
> The example should say something like
> "google.com if |host| is mail.google.com".
> 
>
http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
> net/base/transport_security_state.h:133: // an IsPreloadedSTS host (or, if
> |sni_available|, an SNI preloaded STS
> 
> Nit: IsPreloadedSTS is a private method, so it probably shouldn't
> be used in the comment for a public method.

Both done.

> net/base/transport_security_state.h:135: static void ReportUMAPinFailure(const
> std::string& host, bool sni_available);
> 
> "PinFailure" needs to be more specific, such as "CertPinFailure"
> or "PublicKeyPinFailure".
> 
> The comment on line 131 ("pin failure") needs a similar change.
> 
> Also, "ReportUMAPinFailure" sounds like we want to report
> "UMA pin" failure.  I think we need to add "On":
>     ReportUMAOnPinFailure
> or remove "UMA":
>     ReportPinFailure

Done. Commented updated and name changed to ReportUMAOnPinFailure.

[palmer, 2011-10-25 21:43:57]

>
http://codereview.chromium.org/8364023/diff/5001/net/base/transport_security_...
> net/base/transport_security_state.cc:813: typedef enum {
> This typeful is significantly larger than the set of hosts for which we have
> pinning. Since we wouldn't accept most of these hosts for pinning in any case,
> is there any point in including them now?

I'm not sure what you mean. That enum is full of domains that are all pulled
from the already-existing HSTSPreload arrays. The reason they are "low
resolution" (i.e. "conceptually-second-level") is to purposefully reduce the
specificity of the UMA report. cevans called for this in bug 99782.

[agl, 2011-10-25 21:45:39]

On Tue, Oct 25, 2011 at 5:43 PM,  <palmer@chromium.org> wrote:
> I'm not sure what you mean. That enum is full of domains that are all pulled
> from the already-existing HSTSPreload arrays. The reason they are "low
> resolution" (i.e. "conceptually-second-level") is to purposefully reduce the
> specificity of the UMA report. cevans called for this in bug 99782.

They may be HSTS hosts, but we don't have certificate pins for most of them.


Cheers

AGL

[palmer, 2011-10-25 21:57:17]

> They may be HSTS hosts, but we don't have certificate pins for most of them.

Oh, I see. Because it is better to change the enum rarely than often, I figured
it was safest to just go all-out in this first rev in case we do add pins for
some of those domains. If they wanted HSTS, they might soon want pinning...?

Should I leave it as-is, or remove the pin-less domains from the enum?

So what are the conditions under which we would or would not accept pins?

[agl, 2011-10-25 21:59:21]

On Tue, Oct 25, 2011 at 5:57 PM,  <palmer@chromium.org> wrote:
> So what are the conditions under which we would or would not accept pins?

I think it's pretty clear that we wouldn't accept pins from most of
them given the overhead of adding pins. I'd tend to leave them out so
far.


Cheers

AGL

[palmer, 2011-10-26 00:37:28]

> I think it's pretty clear that we wouldn't accept pins from most of
> them given the overhead of adding pins. I'd tend to leave them out so
> far.

Ok, got rid of those enums.

[Chris Evans, 2011-10-26 22:49:46]

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:254: DCHECK_EQ(tokenizer.token().length(),
1U);
Style nit: the expectation usually comes first in the DCHECK_EQ macro.

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:817: // domains at the END of the listing.
Nit: clarify "END"? i.e. before and not after DOMAIN_NUM_EVENTS :)

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1007: DOMAIN_GOOGLE_COM },
Style nit: DOMAIN_GOOGLE_COM should probably be aligned with the "D" under ther
"1" ?

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1253: if (!entry && sni_available) {
I think we can not pass in sni_available to make things simpler. If we don't get
the hit in the non-SNI list then can we just look in the SNI list
unconditionally?

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1259: return;
I guess this can't actually happen until we expose cert pinning to the web at
large? If it's a "can't happen", it should probably start life as a DCHECK?

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1261:
UMA_HISTOGRAM_ENUMERATION(kPublicKeyPinFailure, entry->low_res_name,
Bug -- there's a script that this might confuse (extract_histograms.py). The
"Blah.ConfunculatorStatus" string needs to be inline with this macro.

For that matter, I think you might still need to update the histograms list so
that the dashboard knows the "pretty" name for your histogram. See
tools/histograms/pretty-print.py. Can be done in a separate CL but best not to
forget.

http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request...
net/url_request/url_request_http_job.cc:689:
TransportSecurityState::ReportUMAOnPinFailure(host, sni_available);
I don't think we need to pass sni_available; I think it's redundant? Assuming
so, passing less things is infinitely better. See comment in other file.

[palmer, 2011-10-27 01:15:45]

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:817: // domains at the END of the listing.
On 2011/10/26 22:49:46, Chris Evans wrote:
> Nit: clarify "END"? i.e. before and not after DOMAIN_NUM_EVENTS :)

Done.

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1007: DOMAIN_GOOGLE_COM },
On 2011/10/26 22:49:46, Chris Evans wrote:
> Style nit: DOMAIN_GOOGLE_COM should probably be aligned with the "D" under
ther
> "1" ?

No, the style guide calls for 4 spaces of indentation on continuation lines.

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1253: if (!entry && sni_available) {
On 2011/10/26 22:49:46, Chris Evans wrote:
> I think we can not pass in sni_available to make things simpler. If we don't
get
> the hit in the non-SNI list then can we just look in the SNI list
> unconditionally?

I prefer that, but I was emulating the other functions in this class when I did
that.

Done for this function.

cevans and other reviewers: Should I generate another CL to de-sni_available-ize
the other functions, too? It would simplify our calling code nicely.

http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security...
net/base/transport_security_state.cc:1261:
UMA_HISTOGRAM_ENUMERATION(kPublicKeyPinFailure, entry->low_res_name,
On 2011/10/26 22:49:46, Chris Evans wrote:
> Bug -- there's a script that this might confuse (extract_histograms.py). The
> "Blah.ConfunculatorStatus" string needs to be inline with this macro.

Done.

The code I copied, 

./chrome/renderer/chrome_render_view_observer.cc:   
UMA_HISTOGRAM_ENUMERATION(kSSLInsecureContent,

has this bug also. Looks like you're the author of that code. :)

> For that matter, I think you might still need to update the histograms list so
> that the dashboard knows the "pretty" name for your histogram. See
> tools/histograms/pretty-print.py. Can be done in a separate CL but best not to
> forget.

Ok, I updated histograms.xml and am creating a CL for it.

http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request...
net/url_request/url_request_http_job.cc:689:
TransportSecurityState::ReportUMAOnPinFailure(host, sni_available);
On 2011/10/26 22:49:46, Chris Evans wrote:
> I don't think we need to pass sni_available; I think it's redundant? Assuming
> so, passing less things is infinitely better. See comment in other file.

Done.

[palmer, 2011-10-27 01:29:17]

> Ok, I updated histograms.xml and am creating a CL for it.

I get a Python stack trace when I try to gcl upload it, so that might be a
while, but it's coming.

[cevans, 2011-10-27 04:51:04]

On Wed, Oct 26, 2011 at 6:15 PM, <palmer@chromium.org> wrote:

>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
base/transport_security_state.**cc<http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security_state.cc>
> File net/base/transport_security_**state.cc (right):
>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
base/transport_security_state.**cc#newcode817<http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security_state.cc#newcode817>
> net/base/transport_security_**state.cc:817: // domains at the END of the
> listing.
> On 2011/10/26 22:49:46, Chris Evans wrote:
>
>> Nit: clarify "END"? i.e. before and not after DOMAIN_NUM_EVENTS :)
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
base/transport_security_state.**cc#newcode1007<http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security_state.cc#newcode1007>
> net/base/transport_security_**state.cc:1007: DOMAIN_GOOGLE_COM },
> On 2011/10/26 22:49:46, Chris Evans wrote:
>
>> Style nit: DOMAIN_GOOGLE_COM should probably be aligned with the "D"
>>
> under ther
>
>> "1" ?
>>
>
> No, the style guide calls for 4 spaces of indentation on continuation
> lines.
>
>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
base/transport_security_state.**cc#newcode1253<http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security_state.cc#newcode1253>
> net/base/transport_security_**state.cc:1253: if (!entry && sni_available)
> {
> On 2011/10/26 22:49:46, Chris Evans wrote:
>
>> I think we can not pass in sni_available to make things simpler. If we
>>
> don't get
>
>> the hit in the non-SNI list then can we just look in the SNI list
>> unconditionally?
>>
>
> I prefer that, but I was emulating the other functions in this class
> when I did that.
>
> Done for this function.
>
> cevans and other reviewers: Should I generate another CL to
> de-sni_available-ize the other functions, too? It would simplify our
> calling code nicely.


Yeah, that can be done as a follow-up. If there's information that really
isn't needed, we shouldn't pass it around. <3 simplicity where possible :)


>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
base/transport_security_state.**cc#newcode1261<http://codereview.chromium.org/8364023/diff/11006/net/base/transport_security_state.cc#newcode1261>
> net/base/transport_security_**state.cc:1261:
> UMA_HISTOGRAM_ENUMERATION(**kPublicKeyPinFailure, entry->low_res_name,
> On 2011/10/26 22:49:46, Chris Evans wrote:
>
>> Bug -- there's a script that this might confuse
>>
> (extract_histograms.py). The
>
>> "Blah.ConfunculatorStatus" string needs to be inline with this macro.
>>
>
> Done.
>
> The code I copied,
>
> ./chrome/renderer/chrome_**render_view_observer.cc:
> UMA_HISTOGRAM_ENUMERATION(**kSSLInsecureContent,
>
> has this bug also. Looks like you're the author of that code. :)


Never, ever, ever copy code from cevans :)
More commonly it's done like this:
http://www.google.com/codesearch#search/&exact_package=chromium&q=UMA_HISTOGR...
If you wanted to be a superstar, you could apply the 2-liner fix to my dodgy
code.


>
>  For that matter, I think you might still need to update the histograms
>>
> list so
>
>> that the dashboard knows the "pretty" name for your histogram. See
>> tools/histograms/pretty-print.**py. Can be done in a separate CL but
>>
> best not to
>
>> forget.
>>
>
> Ok, I updated histograms.xml and am creating a CL for it.
>
>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
url_request/url_request_http_**job.cc<http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request_http_job.cc>
> File net/url_request/url_request_**http_job.cc (right):
>
> http://codereview.chromium.**org/8364023/diff/11006/net/**
>
url_request/url_request_http_**job.cc#newcode689<http://codereview.chromium.org/8364023/diff/11006/net/url_request/url_request_http_job.cc#newcode689>
> net/url_request/url_request_**http_job.cc:689:
> TransportSecurityState::**ReportUMAOnPinFailure(host, sni_available);
> On 2011/10/26 22:49:46, Chris Evans wrote:
>
>> I don't think we need to pass sni_available; I think it's redundant?
>>
> Assuming
>
>> so, passing less things is infinitely better. See comment in other
>>
> file.
>
> Done.
>
>
http://codereview.chromium.**org/8364023/<http://codereview.chromium.org/8364...
>

[palmer, 2011-10-27 18:55:19]

> > cevans and other reviewers: Should I generate another CL to
> > de-sni_available-ize the other functions, too? It would simplify our
> > calling code nicely.
> 
> Yeah, that can be done as a follow-up. If there's information that really
> isn't needed, we shouldn't pass it around. <3 simplicity where possible :)

I will enjoy generating the CL that you will see soon.

> Never, ever, ever copy code from cevans :)
> More commonly it's done like this:
>
http://www.google.com/codesearch#search/&exact_package=chromium&q=UMA_HISTOGR...
> If you wanted to be a superstar, you could apply the 2-liner fix to my dodgy
> code.

Done in https://codereview.chromium.org/8341074/, which you've already LGTM'd.
I'll commit it.

This CL LGTY?

[wtc, 2011-10-27 22:21:41]

Review comments on Patch Set 5.

The most important comment is the very last one, about
naming the new histogram to suggest its close relation to
the existing histogram Net.CertificatePinSuccess.

All the other comments are about minor issues.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:816: enum LowResolutionDomainName {

What does "low resolution" mean here?  Perhaps you should
call it "second level", the term you used in the bug report
and the CL's commit message.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:841: DOMAIN_AKAMAIHD_NET,

You seem to group the enum constants by company affiliation.
So DOMAIN_AKAMAIHD_NET should be its own group, separate
from the two Twitter domains.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1075: DOMAIN_DOUBLECLICK_NET },

This should be DOMAIN_NOT_PINNED because its required_hashes
field is 0.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1092: DOMAIN_APPSPOT_COM },

This should be DOMAIN_NOT_PINNED for the same reason.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1215: hit = entry;

It seems that we should always return entry here, for any i.
I don't see why we should continue either the inner or outer
loop if i != 0.

1. If we should continue the inner loop, that means there
may be duplicate entries in the HSTSPreload array.

2. If we should continue the outer loop, that means a
less exact match of a lower resolution domain is preferrable.

Neither of these seems right.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1255: DCHECK_NE(static_cast<const struct
HSTSPreload*>(NULL), entry);

You may want to assert these two:
  DCHECK(entry->required_hashes);
  DCHECK(entry->low_res_name != DOMAIN_NOT_PINNED);

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.h:131: // Report UMA statistics upon public
key pin failure. Reports only down to

Nit: Report => Reports

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:688:
UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", false);

I recommend that you move this UMA_HISTOGRAM_BOOLEAN call
into TransportSecurityState::ReportUMAOnPinFailure().

The new histogram you added is named SSL.PublicKeyPinFailure.
Compare that name with this name:    Net.CertificatePinSuccess

These two histograms are closely related, so their names
should suggest the relation:
- Probably should put both under Net.*
- Probably should use "CertificatePin" in the new histogram's
  name.

[palmer, 2011-10-28 00:24:39]

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:816: enum LowResolutionDomainName {
On 2011/10/27 22:21:42, wtc wrote:
> 
> What does "low resolution" mean here?  Perhaps you should
> call it "second level", the term you used in the bug report
> and the CL's commit message.

Done.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:841: DOMAIN_AKAMAIHD_NET,
On 2011/10/27 22:21:42, wtc wrote:
> 
> You seem to group the enum constants by company affiliation.
> So DOMAIN_AKAMAIHD_NET should be its own group, separate
> from the two Twitter domains.

Done.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1075: DOMAIN_DOUBLECLICK_NET },
On 2011/10/27 22:21:42, wtc wrote:
> 
> This should be DOMAIN_NOT_PINNED because its required_hashes
> field is 0.

Done.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1092: DOMAIN_APPSPOT_COM },
On 2011/10/27 22:21:42, wtc wrote:
> 
> This should be DOMAIN_NOT_PINNED for the same reason.

Done.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1215: hit = entry;
On 2011/10/27 22:21:42, wtc wrote:
> 
> It seems that we should always return entry here, for any i.
> I don't see why we should continue either the inner or outer
> loop if i != 0.
> 
> 1. If we should continue the inner loop, that means there
> may be duplicate entries in the HSTSPreload array.
> 
> 2. If we should continue the outer loop, that means a
> less exact match of a lower resolution domain is preferrable.
> 
> Neither of these seems right.

Yep, you are right! Thanks. Tests all still pass, as expected.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:1255: DCHECK_NE(static_cast<const struct
HSTSPreload*>(NULL), entry);
On 2011/10/27 22:21:42, wtc wrote:
> 
> You may want to assert these two:
>   DCHECK(entry->required_hashes);
>   DCHECK(entry->low_res_name != DOMAIN_NOT_PINNED);

Done.

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/8364023/diff/14002/net/base/transport_security...
net/base/transport_security_state.h:131: // Report UMA statistics upon public
key pin failure. Reports only down to
On 2011/10/27 22:21:42, wtc wrote:
> 
> Nit: Report => Reports

Done.

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:688:
UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", false);
> I recommend that you move this UMA_HISTOGRAM_BOOLEAN call
> into TransportSecurityState::ReportUMAOnPinFailure().

Done.

> The new histogram you added is named SSL.PublicKeyPinFailure.
> Compare that name with this name:    Net.CertificatePinSuccess
> 
> These two histograms are closely related, so their names
> should suggest the relation:
> - Probably should put both under Net.*

OK.

> - Probably should use "CertificatePin" in the new histogram's
>   name.

Actually (as part of the HTTP-based public key pinning effort in IETF) we have
decided to consistently call it "public key pinning" and not "certificate
pinning" since it is the PK that is pinned, not the cert.

So I'll change the Net.CertificatePin* names to Net.PublicKeyPin*, and update
histograms.xml in the separate CL I have pending for that purpose.

[palmer, 2011-10-28 19:38:40]

LGTY? I hate to nag but I want to do additional work on
transport_security_state* (de-sni_available-izing, and then HTTP-based PK
pinning). Thanks! :)

[wtc, 2011-10-28 23:36:10]

Patch Set 6 LGTM.  I suggest two changes related to histograms
before you commit this CL.  Sorry about that.

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8364023/diff/14002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:695:
UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", true);

Argh, I'm sorry that I didn't see this line here.
My previous suggestion doesn't make sense.  Please
move the
  UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", false);
call back to this file.  It's bad to have them spread in two
files.

Are you sure you want to rename an existing histogram?
I can live with "CertificatePin" even though it is not
100% accurate.

[palmer, 2011-10-28 23:57:00]

> net/url_request/url_request_http_job.cc:695:
> UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", true);
> 
> Argh, I'm sorry that I didn't see this line here.
> My previous suggestion doesn't make sense.  Please
> move the
>   UMA_HISTOGRAM_BOOLEAN("Net.CertificatePinSuccess", false);
> call back to this file.  It's bad to have them spread in two
> files.

Done.

> Are you sure you want to rename an existing histogram?
> I can live with "CertificatePin" even though it is not
> 100% accurate.

On jar's advice, I Put "CertificatePin" back in but marked it
<obsolete>...</obsolete>. So now both names for the signal are in, with the
obsolete one marked. I think that's best.