Treat HSTS and HPKP state independently.

Treat HSTS and HPKP state independently.

Although we have historically, and in static preloads, treated HSTS and HPKP as
part of the same underlying mechanism, the new headers consider them completely
orthogonal. Our current implementation has bugs where, particular where
includeSubdomains is involved, HPKP and HSTS entries get mixed together. This
CL does the following:

- Include separate domain strings for HPKP and HSTS in the output of
  GetDynamicDomainState. This allows net-internals to report on the two
  separately.

- Switch tests to query TransportSecurityState's public API rather than
  manipulate DomainState directly, to reduce dependency on it.

- Make AddHSTSHeader, AddHSTS, etc., follow the same codepath. Notably the
  header variants called GetDynamicDomainState to get the template which means
  an includeSubdomains HPKP state on a parent domain would get copied over.

- AddHPKPHeader no longer appends the old pins to the new set.

- Make DeleteAllDynamicDataSince clear STS and PKP state independently.
  Notably, the old version would almost never drop DomainState entries because
  pkp.last_observed would be uninitialized and never pass the check.

- Make GetDynamicDomainState stitch together the appropriate STS and PKP
  results to form its output DomainState. This avoids includeSubdomains and
  expiration from one mechanism interacting with that of another.

- Add tests for all this.

We should remove DomainState altogether and leave PKPState and STSState as
separate entities (with some consideration for how they were historically
stored on disk), but this CL leaves that alone for now.

BUG=444511
Committed: https://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38
Cr-Commit-Position: refs/heads/master@{#311734}

[davidben, 2015-01-13 01:15:22]

davidben@chromium.org changed reviewers:
+ palmer@chromium.org, rsleevi@chromium.org

[davidben, 2015-01-13 01:15:22]

Here's an initial pass at this. There's probably many more tests to be added and
I still need to set up something to do manual tests. But it does pass the unit
tests I've added.

I've intentionally /not/ pulled DomainState out of the picture altogether yet. I
think we should long-term, but that would require teasing out a story for the
on-disk state we maintain under the old system. (I'm envisioning that once
DomainState is gone altogether, we'd have two GetDynamicDomainState functions
and GetStaticDomainState would return two outputs because preloads still tie
them together.)

This does mean we have some awkward bits like presence of (dynamic) HSTS and
HPKP is a little funny. HSTS conditions on UpgradeMode which I think is fairly
sound. HPKP conditions on whether the pin is empty. I haven't dug into the spec
for whether an HPKP header with zero pins is supposed to be dropped on the floor
or if it does something[*].

[*] It's not a priori a no-op. If you don't do HSTS but do HPKP, you don't get
the redirect, but you still get fatal SSL errors, regardless of the pinning.

[davidben, 2015-01-13 18:19:32]

Updated to add a test for a case I hadn't considered (but accidentally fixed
anyway). Also accordingly simplified a bit of the HPKP header parsing logic, now
that it's not defined to append pins.

[Ryan Sleevi, 2015-01-13 21:56:41]

Wow, huge props for cleanup. A few comments below, mostly LG

[Note, if you do get bored and what to separate out storage in the preload file,
we do know of use cases that would benefit]

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers.cc:288: hashes->clear();
BUG: You should not clear this unless things parse successfully. Doesn't this
have the side-effect of nuking the effective pins if a header fails to parse?
Either way, it's "surprising" (note how we intentionally don't update return
values until line 325, when we're guaranteed success)

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers.cc:329: hashes->push_back(*i);
These are both HashValueVectors.

Why don't you just hashes->swap(pins)?

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers_unittest.cc:472: // Test that parsing a different
header resets the hashes.
Is there any test for when we have one pin header, but encounter an invalid pin
header, and ensure that the original pin header was not botched? This would have
caught the situation I described.

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

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state.h:85: // The following members are not valid
when stored in |enabled_hosts_|:
This is a public structure, so what does it mean to the caller re:
|enabled_hosts_|

Put differently, describe this comment from the public API point of view, rather
than the implementation details.

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

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:141:
EXPECT_FALSE(state.ShouldUpgradeToSSL("com"));
EXPECT_FALSE(...("doyouyahoo.com"))

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:161: const base::Time
current_time(base::Time::Now());
why is current_time a member?

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:168:
EXPECT_FALSE(state.ShouldUpgradeToSSL("example1.com"));
Need a comment here. It took a while staring at this code to understand why

EXPECT_TRUE(StatementA)
EXPECT_FALSE(StatementB)
EXPECT_FALSE(StatementA)

was OK (it's because StatementB forces the entry out)

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:177: state.AddHPKP("example1.com",
older, false, GetSampleSPKIHashes());
Shouldn't you test one with an older entry and one with a newer entry and ensure
only the older entry is expired?

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:234:
TEST_F(TransportSecurityStateTest, IndependentInsertion) {
Should you also test the interaction of static & dynamic overrides and that they
independently work? Or is that already covered?

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:244:
EXPECT_TRUE(state.HasPublicKeyPins("foo.example1.com"));
EXPECT_TRUE(state.ShouldUpgradeToSSL("example1.com"));
EXPECT_FALSE(state.HasPublicKeyPins("example1.com"));

?

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:290: // versions of the current
entry.
This description is a bit confusing.

[davidben, 2015-01-13 23:30:48]

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers.cc:288: hashes->clear();
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> BUG: You should not clear this unless things parse successfully. Doesn't this
> have the side-effect of nuking the effective pins if a header fails to parse?

Nah. AddHPKPHeader only ever calls it on a temporary now. (What I meant when I
said I accidentally fixed the pins overlaying.) Though I agree that shouldn't
nuke it. Done.

> Either way, it's "surprising" (note how we intentionally don't update return
> values until line 325, when we're guaranteed success)

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers.cc:329: hashes->push_back(*i);
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> These are both HashValueVectors.
> 
> Why don't you just hashes->swap(pins)?

Yeah, that's better. Done.

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/826423009/diff/40001/net/http/http_security_h...
net/http/http_security_headers_unittest.cc:472: // Test that parsing a different
header resets the hashes.
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> Is there any test for when we have one pin header, but encounter an invalid
pin
> header, and ensure that the original pin header was not botched? This would
have
> caught the situation I described.

Added one. (Although it wasn't a bug in the old version either.)

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

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state.h:85: // The following members are not valid
when stored in |enabled_hosts_|:
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> This is a public structure, so what does it mean to the caller re:
> |enabled_hosts_|
> 
> Put differently, describe this comment from the public API point of view,
rather
> than the implementation details.

(I just copied this from below. :-P) Moved the comment to |enabled_hosts_|. For
public uses of this struct, the domain field is always valid.

(This is kind of weird and possibly also worth tweaking...)

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

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:141:
EXPECT_FALSE(state.ShouldUpgradeToSSL("com"));
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> EXPECT_FALSE(...("doyouyahoo.com"))

Done.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:161: const base::Time
current_time(base::Time::Now());
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> why is current_time a member?

No longer applicable.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:168:
EXPECT_FALSE(state.ShouldUpgradeToSSL("example1.com"));
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> Need a comment here. It took a while staring at this code to understand why
> 
> EXPECT_TRUE(StatementA)
> EXPECT_FALSE(StatementB)
> EXPECT_FALSE(StatementA)
> 
> was OK (it's because StatementB forces the entry out)

Done.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:177: state.AddHPKP("example1.com",
older, false, GetSampleSPKIHashes());
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> Shouldn't you test one with an older entry and one with a newer entry and
ensure
> only the older entry is expired?

That's in the IndependentExpiry test. I've folded that into this one.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:234:
TEST_F(TransportSecurityStateTest, IndependentInsertion) {
On 2015/01/13 21:56:41, Ryan Sleevi wrote:
> Should you also test the interaction of static & dynamic overrides and that
they
> independently work? Or is that already covered?

HttpSecurityHeadersTest.NoClobberPins covers some of that. I didn't do much with
that since this CL doesn't really touch it. Also not sure what the behavior
should be; e.g. before and after this CL dynamic + includeSubdomains +
example.com will override static + foo.example.com. We'll almost certainly need
that changed to support the carve-outs we were talking about today.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:244:
EXPECT_TRUE(state.HasPublicKeyPins("foo.example1.com"));
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> EXPECT_TRUE(state.ShouldUpgradeToSSL("example1.com"));
> EXPECT_FALSE(state.HasPublicKeyPins("example1.com"));
> 
> ?

Done.

https://codereview.chromium.org/826423009/diff/40001/net/http/transport_secur...
net/http/transport_security_state_unittest.cc:290: // versions of the current
entry.
On 2015/01/13 21:56:40, Ryan Sleevi wrote:
> This description is a bit confusing.

Done. (The original intent with this test was to make sure we weren't overlaying
the pins, though it doesn't use the AddHPKPHeader codepath that was a problem
anyway.)

[Ryan Sleevi, 2015-01-15 01:20:25]

rsleevi@chromium.org changed reviewers:
+ mmenke@chromium.org

[Ryan Sleevi, 2015-01-15 01:20:26]

LGTM.

Adding Matt for the net-internals review.

[mmenke, 2015-01-15 15:32:20]

net-internals cc and js files LGTM (Largely deferring to Sleevi)

[palmer, 2015-01-15 19:24:53]

LGTM. Thank you!

[davidben, 2015-01-15 20:45:17]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-01-15 20:45:46]

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

[commit-bot: I haz the power, 2015-01-15 21:05:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-15 21:06:59]

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