Make HSTS headers not clobber preloaded pins.

Make HSTS headers not clobber preloaded pins.

BUG=329386
TEST=With a fresh profile, ensure that accounts.google.com and twitter.com
have public keys pinned. (Check chrome://net-internals/#hsts.) Then visit
those sites. Close the browser, re-launch it, and check that the pins are
still present in a query in chrome://net-internals/#hsts.

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

[palmer, 2013-12-18 01:44:08]

PTAL.

[agl, 2013-12-18 15:49:19]

This seems like it might be a bit of a partial solution. For example, if
example.com is currently sending HSTS headers and we add preloaded pins, would
the dynamic state still shadow them?

https://codereview.chromium.org/103803012/diff/1/net/http/transport_security_...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/1/net/http/transport_security_...
net/http/transport_security_state.cc:610: GetDomainState(host, true,
&domain_state);
GetDomainState(host, true /* SNI enabled */, &domain_state);

(And below.)

[agl, 2013-12-18 15:49:27]

LGTM

[palmer, 2013-12-18 21:04:33]

> This seems like it might be a bit of a partial solution. For example, if
> http://example.com is currently sending HSTS headers and we add preloaded
pins, would
> the dynamic state still shadow them?

I don't think so, because GetDomainState calls GetStaticDomainState, and then
overlays the result of GetDynamicDomainState on top of the result. So I think
pins would only be shadowed if the dynamic state actually included pins.

[palmer, 2013-12-18 21:04:39]

https://codereview.chromium.org/103803012/diff/1/net/http/transport_security_...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/1/net/http/transport_security_...
net/http/transport_security_state.cc:610: GetDomainState(host, true,
&domain_state);
On 2013/12/18 15:49:19, agl wrote:
> GetDomainState(host, true /* SNI enabled */, &domain_state);
> 
> (And below.)

Done.

[Ryan Sleevi, 2013-12-18 21:48:17]

As a temporary solution, this LGTM, but we've discussed in the past why this is
dangerous and has issues.

Most importantly, by using GetDomainState, rather than GetDynamicState, you end
up taking the static state from the file and persisting it to the users'
preferences.

As such, our ability to rapidly respond or change the static state (eg: through
a binary update) is lost, because user preferences will have shadowed the old
static state into the dynamic state, preventing the new (correct) static state
from taking effect.

[agl, 2013-12-18 21:52:32]

On 2013/12/18 21:48:17, Ryan Sleevi wrote:
> Most importantly, by using GetDomainState, rather than GetDynamicState, you
end
> up taking the static state from the file and persisting it to the users'
> preferences.

Ah, that's a very good point ... and makes it unlandable. We can't poison local
state like that for users. It would mean that we couldn't update pins if we
needed to.

(
Not LGTM
NACK
NO
(I can't remember which works.)

[Ryan Sleevi, 2013-12-18 21:59:21]

On 2013/12/18 21:52:32, agl wrote:
> On 2013/12/18 21:48:17, Ryan Sleevi wrote:
> > Most importantly, by using GetDomainState, rather than GetDynamicState, you
> end
> > up taking the static state from the file and persisting it to the users'
> > preferences.
> 
> Ah, that's a very good point ... and makes it unlandable. We can't poison
local
> state like that for users. It would mean that we couldn't update pins if we
> needed to.
> 
> (
> Not LGTM
> NACK
> NO
> (I can't remember which works.)

Not LGTM.

Sorry, I thought you were on board with it.

[agl, 2013-12-18 22:01:05]

As a short term solution, how about: if there's a static match, use it.
Otherwise look for dynamic information.

[palmer, 2013-12-18 22:27:38]

> As a short term solution, how about: if there's a static match, use it.
> Otherwise look for dynamic information.

I had the same thought. Working on it.

[Ryan Sleevi, 2013-12-27 00:21:05]

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:617: //GetDomainState(host, true /* SNI
enabled */, &domain_state);
Remove

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:641: //GetDomainState(host, true /* SNI
enabled */, &domain_state);
Remove

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:861: // See crbug.com/29386.
Avoid pronouns in comments.

// TODO(palmer): Static pins are unconditionally checked, preventing
// dynamic pins from being used to replace the static pins. This
// ensures that a dynamic header update for HSTS does not replace
// statically-loaded pins, but does prevent a dynamic header update
// via HPKP for statically pinned sites.
// See http://crbug.com/29386 and [bug for refactor] for more details.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:864: // in the very near future (as of Dec
2013). TODO(palmer).
note: remove this bit.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:868: bool did_restore_static_state = false;
The naming here is a bit confusing, because you're not actually "restoring" the
pins (which suggests mutating the DomainState).

HashValueVector preloaded_static_hashes;
HashValueVector preloaded_bad_static_hashes;
bool did_find_static_hashes = false;

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:869: if (IsBuildTimely()) {
if (TransportSecurityState::IsBuildTimely()) - you're in DomainState here, so
I'm surprised this code would compile?

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:873: std::string
host_sub_chunk(&canonicalized_host[i],
This duplicates significant portions of GetStaticDomainState(), which makes me
really uncomfortable.

Our underlying goal is this:
If receiving an HSTS header, *static* SPKI pins are not persisted, *dynamic*
SPKI pins (if any) are, and the *dynamic* HSTS state should replace the *static*
HSTS state.

If receiving an HPKP header, *static* SPKI pins are not persisted, *dynamic*
SPKI pins are updated and persisted, *static* HSTS state is not persisted and
*dynamic* HSTS state (if any) is.

Within DomainState, we already track static vs dynamic state information for
HPKP via
- pkp_observed
- static_spki_hashes vs dynamic_spki_hashes
- dynamic_spki_hashes_expiry
- bad_static_spki_hashes

We fail to distinguish between the two for pkp_include_subdomains

However, we do *not* track static vs dynamic state information for HSTS, namely
- sts_observed
- upgrade_mode
- upgrade_expiry
- sts_include_subdomains

A 'reasonable' quick-fix to DomainState would be to explicitly track the static
vs dynamic data members for PKP and STS, along with bools to track whether a
static or dynamic state exists.

Persistence would *only* persist dynamic state, if any, while evaluation would
always check dynamic state (if any) before static state.

DomainState {
  struct HSTSState {
    // sts_observed (new to track static as well)
    base::Time last_observed;

    // upgrade_expiry
    base::Time upgrade_expiry;

    // upgrade_mode (new to track static vs dynamic)
    UpgradeMode upgrade_mode;

    // sts_include_subdomains (new to track static vs dynamic)
    bool include_subdomains;
  };

  HPKPState {
    // pkp_observed (new to track static vs dynamic)
    base::Time last_observed;

    // dynamic_spki_hashes_expiry (new to track static vs dynamic)
    base::Time expiry;

    // formerly [static/dynamic]_spki_hashes
    HashValueVector spki_hashes;

    // formerly bad_static_spki_hashes;
    HashValueVector bad_spki_hashes;
  };

  bool has_static_hsts;
  HSTSState static_hsts_state;
  HSTSState dynamic_hsts_state;

  bool has_static_pkp;
  HPKPState static_hpkp_state;
  HPKPState dynamic_hpkp_state;
};


And then updating the functions as appropriate. Of course, this may require more
significant gutter-work, just due to the renaming.

This would also solve the related bug you're trying to solve here, where an HSTS
header on a sub-domain will mask the HPKP state from the parent. By explicitly
storing the static and dynamic state differently, you could continue walking
static state for PKP info even after you found HSTS info from dynamic state.

This is a much larger refactor though, as I write it out, but that's effectively
what you're trying to do here. Your current case handles HSTS-masking-HPKP, but
doesn't handle HPKP-masking-HSTS (although less likely).

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:881: i, &restored_static_state,
&did_restore_static_state)) {
You're unconditionally checking against SNI hosts. This seems incorrect, because
you've lost the sni_enabled flag.

[palmer, 2014-01-03 22:43:43]

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:617: //GetDomainState(host, true /* SNI
enabled */, &domain_state);
On 2013/12/27 00:21:06, Ryan Sleevi wrote:
> Remove

Done.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:641: //GetDomainState(host, true /* SNI
enabled */, &domain_state);
On 2013/12/27 00:21:06, Ryan Sleevi wrote:
> Remove

Done.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:861: // See crbug.com/29386.
On 2013/12/27 00:21:06, Ryan Sleevi wrote:
> Avoid pronouns in comments.
> 
> // TODO(palmer): Static pins are unconditionally checked, preventing
> // dynamic pins from being used to replace the static pins. This
> // ensures that a dynamic header update for HSTS does not replace
> // statically-loaded pins, but does prevent a dynamic header update
> // via HPKP for statically pinned sites.
> // See http://crbug.com/29386 and [bug for refactor] for more details.

Done.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:864: // in the very near future (as of Dec
2013). TODO(palmer).
On 2013/12/27 00:21:06, Ryan Sleevi wrote:
> note: remove this bit.

Done.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:869: if (IsBuildTimely()) {
> if (TransportSecurityState::IsBuildTimely()) - you're in DomainState here, so
> I'm surprised this code would compile?

<spiderman>...and I'm just sitting here, compiling it</spiderman>

Anyway, done (for improved clarity).

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:873: std::string
host_sub_chunk(&canonicalized_host[i],
On 2013/12/27 00:21:06, Ryan Sleevi wrote:
> This duplicates significant portions of GetStaticDomainState(), which makes me
> really uncomfortable.
> 
> Our underlying goal is this:
> If receiving an HSTS header, *static* SPKI pins are not persisted, *dynamic*
> SPKI pins (if any) are, and the *dynamic* HSTS state should replace the
*static*
> HSTS state.
> 
> If receiving an HPKP header, *static* SPKI pins are not persisted, *dynamic*
> SPKI pins are updated and persisted, *static* HSTS state is not persisted and
> *dynamic* HSTS state (if any) is.
> 
> Within DomainState, we already track static vs dynamic state information for
> HPKP via
> - pkp_observed
> - static_spki_hashes vs dynamic_spki_hashes
> - dynamic_spki_hashes_expiry
> - bad_static_spki_hashes
> 
> We fail to distinguish between the two for pkp_include_subdomains
> 
> However, we do *not* track static vs dynamic state information for HSTS,
namely
> - sts_observed
> - upgrade_mode
> - upgrade_expiry
> - sts_include_subdomains
> 
> A 'reasonable' quick-fix to DomainState would be to explicitly track the
static
> vs dynamic data members for PKP and STS, along with bools to track whether a
> static or dynamic state exists.
> 
> Persistence would *only* persist dynamic state, if any, while evaluation would
> always check dynamic state (if any) before static state.
> 
> DomainState {
>   struct HSTSState {
>     // sts_observed (new to track static as well)
>     base::Time last_observed;
> 
>     // upgrade_expiry
>     base::Time upgrade_expiry;
> 
>     // upgrade_mode (new to track static vs dynamic)
>     UpgradeMode upgrade_mode;
> 
>     // sts_include_subdomains (new to track static vs dynamic)
>     bool include_subdomains;
>   };
> 
>   HPKPState {
>     // pkp_observed (new to track static vs dynamic)
>     base::Time last_observed;
> 
>     // dynamic_spki_hashes_expiry (new to track static vs dynamic)
>     base::Time expiry;
> 
>     // formerly [static/dynamic]_spki_hashes
>     HashValueVector spki_hashes;
> 
>     // formerly bad_static_spki_hashes;
>     HashValueVector bad_spki_hashes;
>   };
> 
>   bool has_static_hsts;
>   HSTSState static_hsts_state;
>   HSTSState dynamic_hsts_state;
> 
>   bool has_static_pkp;
>   HPKPState static_hpkp_state;
>   HPKPState dynamic_hpkp_state;
> };
> 
> 
> And then updating the functions as appropriate. Of course, this may require
more
> significant gutter-work, just due to the renaming.
> 
> This would also solve the related bug you're trying to solve here, where an
HSTS
> header on a sub-domain will mask the HPKP state from the parent. By explicitly
> storing the static and dynamic state differently, you could continue walking
> static state for PKP info even after you found HSTS info from dynamic state.
> 
> This is a much larger refactor though, as I write it out, but that's
effectively
> what you're trying to do here. Your current case handles HSTS-masking-HPKP,
but
> doesn't handle HPKP-masking-HSTS (although less likely).

Done.

https://codereview.chromium.org/103803012/diff/40001/net/http/transport_secur...
net/http/transport_security_state.cc:881: i, &restored_static_state,
&did_restore_static_state)) {
> You're unconditionally checking against SNI hosts. This seems incorrect,
because
> you've lost the sni_enabled flag.

Handled this by getting rid of that code.

[palmer, 2014-01-27 23:47:30]

Am I approaching something plausible? PTAL. :)

[Ryan Sleevi, 2014-01-31 20:24:43]

Awesome work, palmer. I've not taken an in-depth review of the whole CL, but
I've instead focused just on the TSS, Persister, and Domain State to try to make
sure I fully understand those (and their expected/accepted behaviours) before
going too further.

I think we should liberally sprinkle in comments here to explain some of the
logic/decisions, even if it's "self-obvious" from the code, to make it clear
that "Oh yeah, this behaviour, it IS intentional" or the like :)

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_persister.cc:259:
SPKIHashesFromListValue(*pins_list, &domain_state.static_pkp.spki_hashes);
I'm.. surprised.. we're deserializing this still. Perhaps not for this CL, but
I'm curious if/when we can drop this, as it doesn't seem like we'd ever want to
deserialize into the static list?

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:139: // |newer| and converges them into
|older|.
It took me a little bit to parse this comment, in particular "converges them
into |older|"

style wise,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param...


// Updates |other| to contain the most recent observations found
// between the states |state| and |other|.
bool MergeDomainStates(const TransportSecurityState::DomainState& state,
                       TransportSecurityState::DomainState* other) {

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:148: return true;
why bool if you always return true?

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:164: DomainState dynamic_state =
static_state;
Add a comment explaining why you initialize dynamic_state to static_state, as
opposed to leaving it uninitialized. This isn't entirely clear to me,
considering line 174

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:174: MergeDomainStates(&static_state,
dynamic_state);
Add a comment here as to why you're updating |static_state| with information
that is contained in |dynamic_state|, which is a bit surprising.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:893:
HashesIntersect(static_pkp.spki_hashes, hashes)) {
This bit surprises me.

Consider a preload of (A, B, C) and a header observation of (A, B). This code
will, in effect, allow C even though we've observed a header that says to *not*
allow C

if (HashesIntersect(dynamic_pkp.spki_hashes, hash) ||
    (dynamic_pkp.spki_hashes.empty() && HashesIntersect(static_pkp.spki_hashes,
hashes))) {

}?

(With a corresponding comment explaining the above logic)

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:905: return dynamic_sts.upgrade_mode ==
MODE_FORCE_HTTPS ||
If we observed a dynamic header with an explicit expiration of 0, should we
still use static pins?

(I'm happy to punt on this as being a bug in our implementation, I just wanted
to make sure we agreed that it is a puntable bug and not a critical bug)

[palmer, 2014-01-31 22:20:51]

Thanks for the review!

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_persister.cc:259:
SPKIHashesFromListValue(*pins_list, &domain_state.static_pkp.spki_hashes);
On 2014/01/31 20:24:43, Ryan Sleevi wrote:
> I'm.. surprised.. we're deserializing this still. Perhaps not for this CL, but
> I'm curious if/when we can drop this, as it doesn't seem like we'd ever want
to
> deserialize into the static list?

I've added a TODO and filed a bug to get rid of it.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:139: // |newer| and converges them into
|older|.
On 2014/01/31 20:24:43, Ryan Sleevi wrote:
> It took me a little bit to parse this comment, in particular "converges them
> into |older|"
> 
> style wise,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param...
> 
> 
> // Updates |other| to contain the most recent observations found
> // between the states |state| and |other|.
> bool MergeDomainStates(const TransportSecurityState::DomainState& state,
>                        TransportSecurityState::DomainState* other) {

Done.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:148: return true;
> why bool if you always return true?

Same reason as I have curly braces for 1-line if blocks: legacy of debugging. :)
Removed curlies and changed bool to void.

Also moved it up into the anonymous namespace.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:164: DomainState dynamic_state =
static_state;
> Add a comment explaining why you initialize dynamic_state to static_state, as
> opposed to leaving it uninitialized. This isn't entirely clear to me,
> considering line 174

I think this is legacy from before I wrote MergeDomainStates. Removing it does
not change the test-passingness, so removed.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:174: MergeDomainStates(&static_state,
dynamic_state);
On 2014/01/31 20:24:43, Ryan Sleevi wrote:
> Add a comment here as to why you're updating |static_state| with information
> that is contained in |dynamic_state|, which is a bit surprising.

Done.

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:893:
HashesIntersect(static_pkp.spki_hashes, hashes)) {
> Consider a preload of (A, B, C) and a header observation of (A, B). This code
> will, in effect, allow C even though we've observed a header that says to
*not*
> allow C

Have we in fact decided that? (Maybe we have; I haven't looked at the I-D in too
long.)

Note that the current behavior is to fail (slightly) open, which seems prudent
given the risk of bricking and given the expected-known-goodness of static
hashes.

> if (HashesIntersect(dynamic_pkp.spki_hashes, hash) ||
>     (dynamic_pkp.spki_hashes.empty() &&
HashesIntersect(static_pkp.spki_hashes,
> hashes))) {
> 
> }?

That change causes HttpSecurityHeadersTest.NoClobberPins to fail (perhaps
wrongly).

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:905: return dynamic_sts.upgrade_mode ==
MODE_FORCE_HTTPS ||
> If we observed a dynamic header with an explicit expiration of 0, should we
> still use static pins?

I think so, following the same logic as above ("static believed known-good").

But I could be convinced either way. I should really get back into the I-D...

> (I'm happy to punt on this as being a bug in our implementation, I just wanted
> to make sure we agreed that it is a puntable bug and not a critical bug)

I think it is puntable, if it is a bug.

[palmer, 2014-02-20 22:13:26]

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/150001/net/http/transport_secu...
net/http/transport_security_state.cc:893:
HashesIntersect(static_pkp.spki_hashes, hashes)) {
On 2014/01/31 22:20:51, Chromium Palmer wrote:
> > Consider a preload of (A, B, C) and a header observation of (A, B). This
code
> > will, in effect, allow C even though we've observed a header that says to
> *not*
> > allow C
> 
> Have we in fact decided that? (Maybe we have; I haven't looked at the I-D in
too
> long.)
> 
> Note that the current behavior is to fail (slightly) open, which seems prudent
> given the risk of bricking and given the expected-known-goodness of static
> hashes.
> 
> > if (HashesIntersect(dynamic_pkp.spki_hashes, hash) ||
> >     (dynamic_pkp.spki_hashes.empty() &&
> HashesIntersect(static_pkp.spki_hashes,
> > hashes))) {
> > 
> > }?
> 
> That change causes HttpSecurityHeadersTest.NoClobberPins to fail (perhaps
> wrongly).

Where do we think we are on this? I'd like to get this bug closed, if we think
we are at all close. Can the remaining concerns be handled in subsequent CLs?

[palmer, 2014-03-07 01:07:47]

> Where do we think we are on this? I'd like to get this bug closed, if we think
> we are at all close. Can the remaining concerns be handled in subsequent CLs?

Pleeeeeease take a look. Although not perfect, I believe this CL is a
significant improvement over the status quo.

[Ryan Sleevi, 2014-03-07 01:39:18]

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:523: std::string
domain("accounts.google.com");
Use the same form as the other tests? See line 456

Also add a comment indicating a.g.c has preloaded pins. Or just change this to
docs.google.com

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: // pins. Assert sanity.
"Assert sanity" is superflous.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:535: // check the static_pkp data.
Rework this comment:

// Add a dynamic HSTS header.
// CheckPublicKeyPins should still pass when given the original
// |saved_hashes|, indicating that the static PKP data is still
// configured for the domain.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:545: std::string header = "max-age =
10000; " + good_pin + "; " + backup_pin;
// Add a HPKP header, which should only update the dynamic state.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:554:
EXPECT_TRUE(domain_state.ShouldUpgradeToSSL());
// HSTS should still be configured for this domain.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:556:
EXPECT_TRUE(domain_state.CheckPublicKeyPins(saved_hashes));
What are you testing here? Add a comment.

1) Should you be testing that good_pin is valid?
2) Should you be testing that backup_pin is valid?
3) Should you be testing that a non-existant pin is not valid?
4) Does saved_hashes contain good_pin/backup_pin, or are they new pins?

It's not clear what this is testing, so let's expand more.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_persister.cc:257: //
comment nit:

// TODO(palmer): http://crbug.com/339907: Stop deserializing into the
// static set.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_state.cc:173: MergeDomainStates(dynamic_state,
&static_state);
BUG: static_state.domain gets all full of lies here.

Consider static state for a.b.example.com, dynamic state for b.example.com

GetDynamicDomainState will return the state for b.example.com (because it works
through the host_sub_chunks in canoncalized_host ). GetStaticDomainState will
return the state for a.b.example.com

static_state.domain will be "a.b.example.com", dynamic_state.domain will be
"b.example.com"

When you merge these, static_state.dynamic_pkp / dynamic_sts will be set to
dynamic_state.dynamic_pkp / dynamic_sts. However, .domain won't be updated.

As a result, you run the risk of now serializing back to the dynamic store
"a.b.example.com", duplicating the entry already stored under "b.example.com" -
and now no longer updated on any new observations of b.example.com.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_state.h:142: // static state.
Let's expand/reword this comment.

// When checking whether or not a pin is valid, both dynamic and
// static HPKP state are checked. While dynamic state is
// checked first, static state is always consulted. Unless BOTH
// static and dynamic states are empty, the set of SPKIs from the
// TLS peer MUST be present in one of the states.


That is, your comment about "dynamic state takes precedence" makes me think we
ignore static state when we have dynamic state - but that's not true, is it?

[palmer, 2014-03-14 21:33:39]

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:523: std::string
domain("accounts.google.com");
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> Use the same form as the other tests? See line 456
> 
> Also add a comment indicating a.g.c has preloaded pins. Or just change this to
> http://docs.google.com

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: // pins. Assert sanity.
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> "Assert sanity" is superflous.

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:535: // check the static_pkp data.
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> Rework this comment:
> 
> // Add a dynamic HSTS header.
> // CheckPublicKeyPins should still pass when given the original
> // |saved_hashes|, indicating that the static PKP data is still
> // configured for the domain.

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:545: std::string header = "max-age =
10000; " + good_pin + "; " + backup_pin;
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> // Add a HPKP header, which should only update the dynamic state.

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:554:
EXPECT_TRUE(domain_state.ShouldUpgradeToSSL());
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> // HSTS should still be configured for this domain.

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:556:
EXPECT_TRUE(domain_state.CheckPublicKeyPins(saved_hashes));
> What are you testing here? Add a comment.
> 
> 1) Should you be testing that good_pin is valid?
> 2) Should you be testing that backup_pin is valid?
> 3) Should you be testing that a non-existant pin is not valid?
> 4) Does saved_hashes contain good_pin/backup_pin, or are they new pins?
> 
> It's not clear what this is testing, so let's expand more.

It's to check that a good pin is still valid. Added a comment to that effect.

I think other unit tests test the other cases. This test is just to make sure
that clobbering is not going on.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_persister.cc:257: //
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> comment nit:
> 
> // TODO(palmer): http://crbug.com/339907: Stop deserializing into the
> // static set.

Done.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_state.cc:173: MergeDomainStates(dynamic_state,
&static_state);
On 2014/03/07 01:39:19, Ryan Sleevi wrote:
> BUG: static_state.domain gets all full of lies here.
> 
> Consider static state for http://a.b.example.com, dynamic state for
http://b.example.com
> 
> GetDynamicDomainState will return the state for http://b.example.com (because
it works
> through the host_sub_chunks in canoncalized_host ). GetStaticDomainState will
> return the state for http://a.b.example.com
> 
> static_state.domain will be "a.b.example.com", dynamic_state.domain will be
> "b.example.com"
> 
> When you merge these, static_state.dynamic_pkp / dynamic_sts will be set to
> dynamic_state.dynamic_pkp / dynamic_sts. However, .domain won't be updated.
> 
> As a result, you run the risk of now serializing back to the dynamic store
> "a.b.example.com", duplicating the entry already stored under "b.example.com"
-
> and now no longer updated on any new observations of http://b.example.com.

Ah, yes. OK, I'm going to change the code so that callers always call
GetDynamicDomainState and GetStaticDomainState explicitly. That will cause a lot
of churn in this CL, so brace yourselves.

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_state.h:142: // static state.
> // When checking whether or not a pin is valid, both dynamic and
> // static HPKP state are checked. While dynamic state is
> // checked first, static state is always consulted. Unless BOTH
> // static and dynamic states are empty, the set of SPKIs from the
> // TLS peer MUST be present in one of the states.
> 
> 
> That is, your comment about "dynamic state takes precedence" makes me think we
> ignore static state when we have dynamic state - but that's not true, is it?

Well, wouldn't it be a good idea to allow site operators to turn off pinning by
dynamic means? The I-D says:

"""UAs that support additional sources of pinning information MUST use the most
recently observed pinning information when performing Pin Validation for a host.
 The most recently observed pinning information is determined based upon the
most recent Effective Pin Date, as described in Section 2.3.2."""

Suggesting that it should, indeed, be possible for Chrome to ignore static state
if there is newer dynamic state that calls for no pinning.

[Ryan Sleevi, 2014-03-14 21:55:28]

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/270001/net/http/transport_secu...
net/http/transport_security_state.h:142: // static state.
On 2014/03/14 21:33:39, Chromium Palmer wrote:
> > // When checking whether or not a pin is valid, both dynamic and
> > // static HPKP state are checked. While dynamic state is
> > // checked first, static state is always consulted. Unless BOTH
> > // static and dynamic states are empty, the set of SPKIs from the
> > // TLS peer MUST be present in one of the states.
> > 
> > 
> > That is, your comment about "dynamic state takes precedence" makes me think
we
> > ignore static state when we have dynamic state - but that's not true, is it?
> 
> Well, wouldn't it be a good idea to allow site operators to turn off pinning
by
> dynamic means? The I-D says:
> 
> """UAs that support additional sources of pinning information MUST use the
most
> recently observed pinning information when performing Pin Validation for a
host.
>  The most recently observed pinning information is determined based upon the
> most recent Effective Pin Date, as described in Section 2.3.2."""
> 
> Suggesting that it should, indeed, be possible for Chrome to ignore static
state
> if there is newer dynamic state that calls for no pinning.

Yes, but your comment is unclear on that, which is why I raised it.

"First checks dynamic state, and then checks static state" - except IIRC, you
were/are always checking static state (eg: not allowing dynamic state to
override)

Some degree of rewording the comment is good, perhaps as simple as "first checks
dynamic state, if present, and if not, continues to check static state"

Indicating that static state is *never* checked if dynamic state is present.

Or, you know, indicates that whichever is 'newer' is checked :)

[Ryan Sleevi, 2014-04-08 20:29:10]

I think your refactor is on the right approach for simplifying the public
interactions.

https://codereview.chromium.org/103803012/diff/310001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/103803012/diff/310001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:506: * TSS::GetPublicKeyPins?
indent issue?

Still trying to grok why this was disabled.

https://codereview.chromium.org/103803012/diff/310001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/310001/net/http/transport_secu...
net/http/transport_security_state.h:138: std::string domain;
Should this be migrated into the STS and PKP states? That is, the |domain|
component may differ for them.

https://codereview.chromium.org/103803012/diff/310001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/103803012/diff/310001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3500: if
(!transport_security_state_->CheckPublicKeyPins(
There's a potential of a perf hit here from having to lookup twice (first for
has, then for check).

I'm inclined to suggest that the perf hit is acceptable, but this could be
refactored into something like HasAndCheck() [name sucks, you can do better than
I], that returns a tristate (doesn't apply / matches / doesn't match)

[palmer, 2014-04-21 19:17:24]

>
https://codereview.chromium.org/103803012/diff/310001/net/http/http_security_...
> net/http/http_security_headers_unittest.cc:506: * TSS::GetPublicKeyPins?
> indent issue?
> 
> Still trying to grok why this was disabled.

It was disabled just for hacking around. It's back on now.

>
https://codereview.chromium.org/103803012/diff/310001/net/http/transport_secu...
> File net/http/transport_security_state.h (right):
> 
>
https://codereview.chromium.org/103803012/diff/310001/net/http/transport_secu...
> net/http/transport_security_state.h:138: std::string domain;
> Should this be migrated into the STS and PKP states? That is, the |domain|
> component may differ for them.

That is a significant change and this CL is already monstrous enough. Let's take
it up next, in a new CL/bug.

>
https://codereview.chromium.org/103803012/diff/310001/net/socket/ssl_client_s...
> net/socket/ssl_client_socket_nss.cc:3500: if
> (!transport_security_state_->CheckPublicKeyPins(
> There's a potential of a perf hit here from having to lookup twice (first for
> has, then for check).
> 
> I'm inclined to suggest that the perf hit is acceptable, but this could be
> refactored into something like HasAndCheck() [name sucks, you can do better
than
> I], that returns a tristate (doesn't apply / matches / doesn't match)

Agree. But again, a topic for a later CL. For now, it's correct. (At least, the
tests pass on my machine...)

PTAL at latest patchset. Thanks! I think we are close.

[agl, 2014-04-24 20:51:09]

LGTM, although sleevi was doing this review so you might want to wait for him.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:113: return GetDynamicDomainState(host,
&state);
Doesn't this mean that HPKP headers make SSL error fatal? Is that intended?

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:120: if (static_state.ShouldUpgradeToSSL())
just return static_state.ShouldUpgradeToSSL() directly.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:137: if
(static_state.CheckPublicKeyPins(hashes, failure_log))
again, can just return the function's return value directly.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:152: if (static_state.HasPublicKeyPins())
... and here.

[Ryan Sleevi, 2014-04-24 21:40:20]

I know how desperate you are to finish this, and I'm with you. A few notes below
though...

https://codereview.chromium.org/103803012/diff/370001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/103803012/diff/370001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1267:
result->SetString("domain", domain);
Let's file a follow-up bug to fix this domain ambiguity/mangling.

I don't see why the UI can't display the (correct) versions for both, even if
the other places are ambiguous (but in a way that doesn't matter)

https://codereview.chromium.org/103803012/diff/370001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:524: TEST_F(HttpSecurityHeadersTest,
NoClobberPins) {
Document test

// Tests that when a static HSTS and a static HPKP entry are present,
// adding a dynamic HSTS header does not clobber the static HPKP entry.
// Further, adding a dynamic HPKP entry could not affect the HSTS entry
// for the site.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:113: return GetDynamicDomainState(host,
&state);
On 2014/04/24 20:51:09, agl wrote:
> Doesn't this mean that HPKP headers make SSL error fatal? Is that intended?

No, it's a bug.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:120: if (static_state.ShouldUpgradeToSSL())
On 2014/04/24 20:51:09, agl wrote:
> just return static_state.ShouldUpgradeToSSL() directly.

No, this is to catch the fall through where the static domain state may say
nothing (eg: pins, but not HSTS), but the server has later sent an HSTS header

Also, this check order seems inverted, since isn't dynamic state supposed to be
able to undo static state?

[agl, 2014-04-24 21:45:54]

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:120: if (static_state.ShouldUpgradeToSSL())
On 2014/04/24 21:40:21, Ryan Sleevi wrote:
> No, this is to catch the fall through where the static domain state may say
> nothing (eg: pins, but not HSTS), but the server has later sent an HSTS header

Ah, I see. I was thinking that if positive static overrode a negative dynamic
then a negative static should override too. (I.e. emergency push to disable
HSTS.)

But I think you're right: dynamic should come first, with fallthough for the
negative case to static.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:137: if
(static_state.CheckPublicKeyPins(hashes, failure_log))
On 2014/04/24 20:51:09, agl wrote:
> again, can just return the function's return value directly.

Again, I was assuming that static pins overrode, but that might not be the case,
so you might want to disregard this.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:152: if (static_state.HasPublicKeyPins())
On 2014/04/24 20:51:09, agl wrote:
> ... and here.

Sorry, this one was nonsense.

[palmer, 2014-04-25 00:59:58]

(patchset comin' right up)

https://codereview.chromium.org/103803012/diff/370001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/103803012/diff/370001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1267:
result->SetString("domain", domain);
> Let's file a follow-up bug to fix this domain ambiguity/mangling.

https://code.google.com/p/chromium/issues/detail?id=366936

https://codereview.chromium.org/103803012/diff/370001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:524: TEST_F(HttpSecurityHeadersTest,
NoClobberPins) {
On 2014/04/24 21:40:21, Ryan Sleevi wrote:
> Document test
> 
> // Tests that when a static HSTS and a static HPKP entry are present,
> // adding a dynamic HSTS header does not clobber the static HPKP entry.
> // Further, adding a dynamic HPKP entry could not affect the HSTS entry
> // for the site.

Done.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:113: return GetDynamicDomainState(host,
&state);
> > Doesn't this mean that HPKP headers make SSL error fatal? Is that intended?
> 
> No, it's a bug.

According to our draft as written, it's a feature:

http://tools.ietf.org/html/draft-ietf-websec-key-pinning-11#section-2.6

(Since HSTS has the same specified behavior, and since DynamicDomainState
currently holds only STS and PKP information, the current "return true if any
dynamic stuff" is a short-cut/code conciseness technique. If you like, I can
make the policy more explicit so that if dynamic state ever includes e.g. TACK
and if TACK has a different rule, we can handle it.)

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:120: if (static_state.ShouldUpgradeToSSL())
On 2014/04/24 21:45:55, agl wrote:
> On 2014/04/24 21:40:21, Ryan Sleevi wrote:
> > No, this is to catch the fall through where the static domain state may say
> > nothing (eg: pins, but not HSTS), but the server has later sent an HSTS
header
> 
> Ah, I see. I was thinking that if positive static overrode a negative dynamic
> then a negative static should override too. (I.e. emergency push to disable
> HSTS.)
> 
> But I think you're right: dynamic should come first, with fallthough for the
> negative case to static.

Done.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:137: if
(static_state.CheckPublicKeyPins(hashes, failure_log))
Applied the same "dynamic first" policy as above.

https://codereview.chromium.org/103803012/diff/370001/net/http/transport_secu...
net/http/transport_security_state.cc:152: if (static_state.HasPublicKeyPins())
Dynamic first, as above.

[Ryan Sleevi, 2014-05-05 23:24:23]

LGTM mod nits, which you don't need a re-review for.

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:124: if (static_state.ShouldUpgradeToSSL())
just combine?

if (GetStaticDomainState(...) &&
    static_state.ShouldUpgradeToSSL()) {
  return true;
}

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:142: return true;
ditto re: combining

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:226: i->second.pkp.spki_hashes.clear();
Shouldn't you also update the expiry (to clear it out)?

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.h:220: bool GetDynamicDomainState(const
std::string& host, DomainState* result);
Document? Seems you nuked the entire comment.

Do these need to be public? Previously they were private, and your comment on
158 suggests these are test only.

[palmer, 2014-05-06 18:37:59]

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:124: if (static_state.ShouldUpgradeToSSL())
On 2014/05/05 23:24:23, Ryan Sleevi wrote:
> just combine?
> 
> if (GetStaticDomainState(...) &&
>     static_state.ShouldUpgradeToSSL()) {
>   return true;
> }

Done.

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:142: return true;
On 2014/05/05 23:24:23, Ryan Sleevi wrote:
> ditto re: combining

Done.

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.cc:226: i->second.pkp.spki_hashes.clear();
On 2014/05/05 23:24:23, Ryan Sleevi wrote:
> Shouldn't you also update the expiry (to clear it out)?

Done.

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/103803012/diff/430001/net/http/transport_secu...
net/http/transport_security_state.h:220: bool GetDynamicDomainState(const
std::string& host, DomainState* result);
> Document? Seems you nuked the entire comment.

Documented.

> Do these need to be public? Previously they were private, and your comment on
> 158 suggests these are test only.

chrome/browser/ui/webui/net_internals/net_internals_ui.cc needs them.

[palmer, 2014-05-06 22:57:24]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-05-06 22:59:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/palmer@chromium.org/103803012/450001

[palmer, 2014-05-07 02:59:45]

The CQ bit was unchecked by palmer@chromium.org

[palmer, 2014-05-07 02:59:55]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-05-07 03:02:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/palmer@chromium.org/103803012/450001

[commit-bot: I haz the power, 2014-05-07 05:03:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 11:03:49]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 11:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-07 11:19:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[palmer, 2014-05-07 19:54:04]

eroman: Can you please take a look at

    chrome/browser/resources/net_internals/hsts_view.js
    chrome/browser/ui/webui/net_internals/net_internals_ui.cc

? Thanks!

[eroman, 2014-05-07 21:17:16]

net_internals lgtm

https://codereview.chromium.org/103803012/diff/450001/chrome/browser/resource...
File chrome/browser/resources/net_internals/hsts_view.js (right):

https://codereview.chromium.org/103803012/diff/450001/chrome/browser/resource...
chrome/browser/resources/net_internals/hsts_view.js:120: var t;
Is there any point to the variables "t" and "b"? They are assigned however never
read from. Just remove them.

If you do need them, then at a minimum choose better names :)

https://codereview.chromium.org/103803012/diff/450001/chrome/browser/resource...
chrome/browser/resources/net_internals/hsts_view.js:132:
addTextNode(this.queryOutputDiv_, ' static_sts_include_subdomains: ');
I feel like this code has reached a critical mass of values that it should be
refactored to be data driven:

var entries = {
  "domain": result.domain,
  "static_upgrade_mode": modeToString(result.static_upgrade_mode),
  "static_sts_include_subdomains", result.static_sts_include_subdomains
  // So on and so forth...
};

for (var key in entries) {
  var value = entries[key];
  addTextNode(this.queryOutputDiv_, ' ' + key + ': ');
  addNodeWithText(this.queryOutputDiv_, 'tt', value)
  addNode(this.queryOutputDiv_, 'br');
}

Or alternately if the intent is to pretty print everything in the result object
perhaps something like this would work, and save having to update this as much
in the future when the result object changes:

for (var key in result) {
  var value = result[key];
  
  // Pretty print some values differently...
  if (key == 'static_upgrade_mode' || key == 'dynamic_upgrade_mode') {
    value = modeToString(value);
  }

  addTextNode(this.queryOutputDiv_, ' ' + key + ': ');
  addNodeWithText(this.queryOutputDiv_, 'tt', value)
  addNode(this.queryOutputDiv_, 'br');
}

[palmer, 2014-05-07 21:27:55]

Thanks eroman! I assigned a bug to myself to do this clean-up ASAP.

[palmer, 2014-05-07 21:28:05]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-05-07 21:29:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/palmer@chromium.org/103803012/450001

[commit-bot: I haz the power, 2014-05-07 21:46:31]

Change committed as 268966