Treat HSTS and HPKP state independently.

net/http/transport_security_state_unittest.cc

Index: net/http/transport_security_state_unittest.cc
diff --git a/net/http/transport_security_state_unittest.cc b/net/http/transport_security_state_unittest.cc
index b9369faaf8339d10d85385b6ce57401d40e85424..e104a1e29793aead7af2b88f58bfd1fa7644a763 100644
--- a/net/http/transport_security_state_unittest.cc
+++ b/net/http/transport_security_state_unittest.cc
@@ -55,42 +55,42 @@ class TransportSecurityStateTest : public testing::Test {
     state->enable_static_pins_ = true;
   }
 
+  static HashValueVector GetSampleSPKIHashes() {
+    HashValueVector spki_hashes;
+    HashValue hash(HASH_VALUE_SHA1);
+    memset(hash.data(), 0, hash.size());
+    spki_hashes.push_back(hash);
+    return spki_hashes;
+  }
+
  protected:
   bool GetStaticDomainState(TransportSecurityState* state,
                             const std::string& host,
                             TransportSecurityState::DomainState* result) {
     return state->GetStaticDomainState(host, result);
   }
-
-  void EnableHost(TransportSecurityState* state,
-                  const std::string& host,
-                  const TransportSecurityState::DomainState& domain_state) {
-    return state->EnableHost(host, domain_state);
-  }
 };
 
 TEST_F(TransportSecurityStateTest, SimpleMatches) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
   bool include_subdomains = false;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("yahoo.com"));
 }
 
 TEST_F(TransportSecurityStateTest, MatchesCase1) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
   bool include_subdomains = false;
   state.AddHSTS("YAhoo.coM", expiry, include_subdomains);
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("yahoo.com"));
 }
 
 TEST_F(TransportSecurityStateTest, Fuzz) {
@@ -117,80 +117,238 @@ TEST_F(TransportSecurityStateTest, Fuzz) {
 
 TEST_F(TransportSecurityStateTest, MatchesCase2) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("YAhoo.coM", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("YAhoo.coM"));
   bool include_subdomains = false;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
-  EXPECT_TRUE(state.GetDynamicDomainState("YAhoo.coM", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("YAhoo.coM"));
 }
 
 TEST_F(TransportSecurityStateTest, SubdomainMatches) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
   bool include_subdomains = true;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
-  EXPECT_TRUE(state.GetDynamicDomainState("foo.yahoo.com", &domain_state));
-  EXPECT_TRUE(state.GetDynamicDomainState("foo.bar.yahoo.com", &domain_state));
-  EXPECT_TRUE(
-      state.GetDynamicDomainState("foo.bar.baz.yahoo.com", &domain_state));
-  EXPECT_FALSE(state.GetDynamicDomainState("com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("yahoo.com"));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.yahoo.com"));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.bar.yahoo.com"));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.bar.baz.yahoo.com"));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("com"));

[Ryan Sleevi, 2015/01/13 21:56:40]

EXPECT_FALSE(...("doyouyahoo.com"))

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

Done.

+}
+
+TEST_F(TransportSecurityStateTest, FatalSSLErrors) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+
+  state.AddHSTS("example1.com", expiry, false);
+  state.AddHPKP("example2.com", expiry, false, GetSampleSPKIHashes());
+
+  // The presense of either HSTS or HPKP is enough to make SSL errors fatal.
+  EXPECT_TRUE(state.ShouldSSLErrorsBeFatal("example1.com"));
+  EXPECT_TRUE(state.ShouldSSLErrorsBeFatal("example2.com"));
+}
+
+// Tests that HPKP and HSTS state both expire. Also tests that expired entries
+// are pruned.
+TEST_F(TransportSecurityStateTest, Expiration) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());

[Ryan Sleevi, 2015/01/13 21:56:40]

why is current_time a member?

[davidben, 2015/01/13 23:30:47]

No longer applicable.

+  const base::Time older = current_time - base::TimeDelta::FromSeconds(1000);
+
+  // Note: this test assumes that inserting an entry with an expiration time in
+  // the past works and is only pruned on query.
+  state.AddHSTS("example1.com", older, false);
+  EXPECT_TRUE(TransportSecurityState::Iterator(state).HasNext());
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("example1.com"));

[Ryan Sleevi, 2015/01/13 21:56:40]

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)

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

Done.

+  EXPECT_FALSE(TransportSecurityState::Iterator(state).HasNext());
+
+  state.AddHPKP("example1.com", older, false, GetSampleSPKIHashes());
+  EXPECT_TRUE(TransportSecurityState::Iterator(state).HasNext());
+  EXPECT_FALSE(state.HasPublicKeyPins("example1.com"));
+  EXPECT_FALSE(TransportSecurityState::Iterator(state).HasNext());
+
+  state.AddHSTS("example1.com", older, false);
+  state.AddHPKP("example1.com", older, false, GetSampleSPKIHashes());

[Ryan Sleevi, 2015/01/13 21:56:40]

Shouldn't you test one with an older entry and one with a newer entry and ensure
only the older entry is expired?

[davidben, 2015/01/13 23:30:47]

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

+  EXPECT_TRUE(TransportSecurityState::Iterator(state).HasNext());
+  EXPECT_FALSE(state.ShouldSSLErrorsBeFatal("example1.com"));
+  EXPECT_FALSE(TransportSecurityState::Iterator(state).HasNext());
 }
 
 TEST_F(TransportSecurityStateTest, InvalidDomains) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
   bool include_subdomains = true;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
-  EXPECT_TRUE(state.GetDynamicDomainState("www-.foo.yahoo.com", &domain_state));
-  EXPECT_TRUE(
-      state.GetDynamicDomainState("2\x01.foo.yahoo.com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("www-.foo.yahoo.com"));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("2\x01.foo.yahoo.com"));
 }
 
-TEST_F(TransportSecurityStateTest, DeleteAllDynamicDataSince) {
+// Tests that HPKP and HSTS state are queried independently for subdomain
+// matches.
+TEST_F(TransportSecurityStateTest, IndependentSubdomain) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+
+  state.AddHSTS("example1.com", expiry, true);
+  state.AddHPKP("example1.com", expiry, false, GetSampleSPKIHashes());
+
+  state.AddHSTS("example2.com", expiry, false);
+  state.AddHPKP("example2.com", expiry, true, GetSampleSPKIHashes());
+
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.example1.com"));
+  EXPECT_FALSE(state.HasPublicKeyPins("foo.example1.com"));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("foo.example2.com"));
+  EXPECT_TRUE(state.HasPublicKeyPins("foo.example2.com"));
+}
+
+// Tests that HPKP and HSTS state are queried independently for expiry.
+TEST_F(TransportSecurityStateTest, IndependentExpiry) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+  const base::Time older = current_time - base::TimeDelta::FromSeconds(1000);
+
+  state.AddHSTS("example1.com", expiry, false);
+  state.AddHPKP("example1.com", older, false, GetSampleSPKIHashes());
+
+  state.AddHSTS("example2.com", older, false);
+  state.AddHPKP("example2.com", expiry, false, GetSampleSPKIHashes());
+
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("example1.com"));
+  EXPECT_FALSE(state.HasPublicKeyPins("example1.com"));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("example2.com"));
+  EXPECT_TRUE(state.HasPublicKeyPins("example2.com"));
+}
+
+// Tests that HPKP and HSTS state are inserted and overridden independently.
+TEST_F(TransportSecurityStateTest, IndependentInsertion) {

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

Should you also test the interaction of static & dynamic overrides and that they
independently work? Or is that already covered?

[davidben, 2015/01/13 23:30:47]

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.

+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+
+  // Place an includeSubdomains HSTS entry below a normal HPKP entry.
+  state.AddHSTS("example1.com", expiry, true);
+  state.AddHPKP("foo.example1.com", expiry, false, GetSampleSPKIHashes());
+
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.example1.com"));
+  EXPECT_TRUE(state.HasPublicKeyPins("foo.example1.com"));

[Ryan Sleevi, 2015/01/13 21:56:40]

EXPECT_TRUE(state.ShouldUpgradeToSSL("example1.com"));
EXPECT_FALSE(state.HasPublicKeyPins("example1.com"));

?

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

Done.

+
+  // Drop the includeSubdomains from the HSTS entry.
+  state.AddHSTS("example1.com", expiry, false);
+
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("foo.example1.com"));
+  EXPECT_TRUE(state.HasPublicKeyPins("foo.example1.com"));
+
+  // Place an includeSubdomains HPKP entry below a normal HSTS entry.
+  state.AddHSTS("foo.example2.com", expiry, false);
+  state.AddHPKP("example2.com", expiry, true, GetSampleSPKIHashes());
+
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.example2.com"));
+  EXPECT_TRUE(state.HasPublicKeyPins("foo.example2.com"));
+
+  // Drop the includeSubdomains from the HSTS entry.
+  state.AddHPKP("example2.com", expiry, false, GetSampleSPKIHashes());
+
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("foo.example2.com"));
+  EXPECT_FALSE(state.HasPublicKeyPins("foo.example2.com"));
+}
+
+// Tests that GetDynamicDomainState appropriately stitches together the results
+// of HSTS and HPKP.
+TEST_F(TransportSecurityStateTest, DynamicDomainState) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry1 = current_time + base::TimeDelta::FromSeconds(1000);
+  const base::Time expiry2 = current_time + base::TimeDelta::FromSeconds(2000);
+
+  state.AddHSTS("example.com", expiry1, true);
+  state.AddHPKP("foo.example.com", expiry2, false, GetSampleSPKIHashes());
+
+  TransportSecurityState::DomainState domain_state;
+  ASSERT_TRUE(state.GetDynamicDomainState("foo.example.com", &domain_state));
+  EXPECT_TRUE(domain_state.ShouldUpgradeToSSL());
+  EXPECT_TRUE(domain_state.HasPublicKeyPins());
+  EXPECT_TRUE(domain_state.sts.include_subdomains);
+  EXPECT_FALSE(domain_state.pkp.include_subdomains);
+  EXPECT_EQ(expiry1, domain_state.sts.expiry);
+  EXPECT_EQ(expiry2, domain_state.pkp.expiry);
+  EXPECT_EQ("example.com", domain_state.sts.domain);
+  EXPECT_EQ("foo.example.com", domain_state.pkp.domain);
+}
+
+// Tests that new pins override entries at both parent domains or previous
+// versions of the current entry.

[Ryan Sleevi, 2015/01/13 21:56:40]

This description is a bit confusing.

[davidben, 2015/01/13 23:30:47]

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.)

+TEST_F(TransportSecurityStateTest, NewPinsOverride) {
   TransportSecurityState state;
   TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+  HashValue hash1(HASH_VALUE_SHA1);
+  memset(hash1.data(), 0x01, hash1.size());
+  HashValue hash2(HASH_VALUE_SHA1);
+  memset(hash2.data(), 0x02, hash1.size());
+  HashValue hash3(HASH_VALUE_SHA1);
+  memset(hash3.data(), 0x03, hash1.size());
+
+  state.AddHPKP("example.com", expiry, true, HashValueVector(1, hash1));
+
+  ASSERT_TRUE(state.GetDynamicDomainState("foo.example.com", &domain_state));
+  ASSERT_EQ(1u, domain_state.pkp.spki_hashes.size());
+  EXPECT_TRUE(domain_state.pkp.spki_hashes[0].Equals(hash1));
+
+  state.AddHPKP("foo.example.com", expiry, false, HashValueVector(1, hash2));
+
+  ASSERT_TRUE(state.GetDynamicDomainState("foo.example.com", &domain_state));
+  ASSERT_EQ(1u, domain_state.pkp.spki_hashes.size());
+  EXPECT_TRUE(domain_state.pkp.spki_hashes[0].Equals(hash2));
+
+  state.AddHPKP("foo.example.com", expiry, false, HashValueVector(1, hash3));
+
+  ASSERT_TRUE(state.GetDynamicDomainState("foo.example.com", &domain_state));
+  ASSERT_EQ(1u, domain_state.pkp.spki_hashes.size());
+  EXPECT_TRUE(domain_state.pkp.spki_hashes[0].Equals(hash3));
+}
+
+TEST_F(TransportSecurityStateTest, DeleteAllDynamicDataSince) {
+  TransportSecurityState state;
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
   const base::Time older = current_time - base::TimeDelta::FromSeconds(1000);
 
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
   bool include_subdomains = false;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
 
   state.DeleteAllDynamicDataSince(expiry);
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
-  EXPECT_EQ(TransportSecurityState::DomainState::MODE_FORCE_HTTPS,
-            domain_state.sts.upgrade_mode);
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("yahoo.com"));
   state.DeleteAllDynamicDataSince(older);
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
-  EXPECT_EQ(TransportSecurityState::DomainState::MODE_DEFAULT,
-            domain_state.sts.upgrade_mode);
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
+
+  // |state| should be empty now.
+  EXPECT_FALSE(TransportSecurityState::Iterator(state).HasNext());
 }
 
 TEST_F(TransportSecurityStateTest, DeleteDynamicDataForHost) {
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
   bool include_subdomains = false;
   state.AddHSTS("yahoo.com", expiry, include_subdomains);
 
-  EXPECT_TRUE(state.GetDynamicDomainState("yahoo.com", &domain_state));
-  EXPECT_FALSE(state.GetDynamicDomainState("example.com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("yahoo.com"));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("example.com"));
   EXPECT_TRUE(state.DeleteDynamicDataForHost("yahoo.com"));
-  EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
+  EXPECT_FALSE(state.ShouldUpgradeToSSL("yahoo.com"));
 }
 
 TEST_F(TransportSecurityStateTest, EnableStaticPins) {
@@ -243,10 +401,12 @@ TEST_F(TransportSecurityStateTest, PreloadedDomainSet) {
   // chrome://net-internals/#hsts UI. So test that.
   EXPECT_TRUE(
       state.GetStaticDomainState("market.android.com", &domain_state));
-  EXPECT_EQ(domain_state.domain, "market.android.com");
+  EXPECT_EQ(domain_state.sts.domain, "market.android.com");
+  EXPECT_EQ(domain_state.pkp.domain, "market.android.com");
   EXPECT_TRUE(state.GetStaticDomainState(
       "sub.market.android.com", &domain_state));
-  EXPECT_EQ(domain_state.domain, "market.android.com");
+  EXPECT_EQ(domain_state.sts.domain, "market.android.com");
+  EXPECT_EQ(domain_state.pkp.domain, "market.android.com");
 }
 
 static bool StaticShouldRedirect(const char* hostname) {
@@ -697,13 +857,11 @@ TEST_F(TransportSecurityStateTest, OverrideBuiltins) {
   EXPECT_FALSE(StaticShouldRedirect("www.google.com"));
 
   TransportSecurityState state;
-  TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
-  domain_state.sts.expiry = expiry;
-  EnableHost(&state, "www.google.com", domain_state);
+  state.AddHSTS("www.google.com", expiry, true);
 
-  EXPECT_TRUE(state.GetDynamicDomainState("www.google.com", &domain_state));
+  EXPECT_TRUE(state.ShouldUpgradeToSSL("www.google.com"));
 }
 
 TEST_F(TransportSecurityStateTest, GooglePinnedProperties) {