Centralize the logic for checking public key pins

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 476ae94bd681cfcae362f53dcac998dea95dd9b5..930ae42fdc8af7451a19f64ed880a3f06d1872a5 100644
--- a/net/http/transport_security_state_unittest.cc
+++ b/net/http/transport_security_state_unittest.cc
@@ -37,6 +37,7 @@
 namespace net {
 
 class TransportSecurityStateTest : public testing::Test {
+ public:
   virtual void SetUp() {
 #if defined(USE_OPENSSL)
     crypto::EnsureOpenSSLInit();
@@ -45,6 +46,14 @@ class TransportSecurityStateTest : public testing::Test {
 #endif
   }
 
+  static void DisableStaticPinning(TransportSecurityState* state) {
+    state->enable_static_pinning_ = false;
+  }
+
+  static void EnableStaticPinning(TransportSecurityState* state) {

[wtc, 2014/08/07 23:39:13]

These methods (and the EnableStaticPinning test) should be renamed
DisableStaticPins and EnableStaticPins if enable_static_pinning_ is renamed
enable_static_pins_.

[Ryan Hamilton, 2014/08/08 00:54:01]

Done.

+    state->enable_static_pinning_ = true;
+  }
+
  protected:
   bool GetStaticDomainState(TransportSecurityState* state,
                             const std::string& host,
@@ -162,6 +171,19 @@ TEST_F(TransportSecurityStateTest, DeleteDynamicDataForHost) {
   EXPECT_FALSE(state.GetDynamicDomainState("yahoo.com", &domain_state));
 }
 
+TEST_F(TransportSecurityStateTest, EnableStaticPinning) {
+  TransportSecurityState state;
+  TransportSecurityState::DomainState domain_state;
+
+  EnableStaticPinning(&state);
+  EXPECT_TRUE(
+      state.GetStaticDomainState("chrome.google.com", true, &domain_state));
+
+  DisableStaticPinning(&state);
+  EXPECT_FALSE(
+      state.GetStaticDomainState("chrome.google.com", true, &domain_state));
+}
+
 TEST_F(TransportSecurityStateTest, IsPreloaded) {
   const std::string paypal = "paypal.com";
   const std::string www_paypal = "www.paypal.com";
@@ -172,6 +194,7 @@ TEST_F(TransportSecurityStateTest, IsPreloaded) {
   const std::string aypal = "aypal.com";
 
   TransportSecurityState state;
+  EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:40]

Let's make this test more explicit about the STS bit vs the PKP part

Delete this line, and remove line 203

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

   TransportSecurityState::DomainState domain_state;
 
   EXPECT_TRUE(GetStaticDomainState(&state, paypal, true, &domain_state));
@@ -186,6 +209,7 @@ TEST_F(TransportSecurityStateTest, IsPreloaded) {
 
 TEST_F(TransportSecurityStateTest, PreloadedDomainSet) {
   TransportSecurityState state;
+  EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:39]

Remove this

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

   TransportSecurityState::DomainState domain_state;
 
   // The domain wasn't being set, leading to a blank string in the
@@ -200,6 +224,7 @@ TEST_F(TransportSecurityStateTest, PreloadedDomainSet) {
 
 static bool StaticShouldRedirect(const char* hostname) {
   TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:39]

Definitely removed - would have caught this bug :)

[Ryan Hamilton, 2014/08/08 00:54:01]

Done.

   TransportSecurityState::DomainState domain_state;
   return state.GetStaticDomainState(
              hostname, true /* SNI ok */, &domain_state) &&
@@ -208,12 +233,14 @@ static bool StaticShouldRedirect(const char* hostname) {
 
 static bool HasStaticState(const char* hostname) {
   TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:39]

This should be removed, I think. At least from the usage.

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

   TransportSecurityState::DomainState domain_state;
   return state.GetStaticDomainState(hostname, true /* SNI ok */, &domain_state);
 }
 
 static bool HasStaticPublicKeyPins(const char* hostname, bool sni_enabled) {
   TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticPinning(&state);
   TransportSecurityState::DomainState domain_state;
   if (!state.GetStaticDomainState(hostname, sni_enabled, &domain_state))
     return false;
@@ -227,6 +254,7 @@ static bool HasStaticPublicKeyPins(const char* hostname) {
 
 static bool OnlyPinningInStaticState(const char* hostname) {
   TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticPinning(&state);
   TransportSecurityState::DomainState domain_state;
   if (!state.GetStaticDomainState(hostname, true /* SNI ok */, &domain_state))
     return false;
@@ -238,6 +266,7 @@ static bool OnlyPinningInStaticState(const char* hostname) {
 
 TEST_F(TransportSecurityStateTest, Preloaded) {
   TransportSecurityState state;
+  EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:40]

So, we should probably split this in two, with only the latter calling
EnableStaticPinning

which domains you need to transfer over "should" be easy - the
OnlyPinningInStaticState / HasStaticPublicPins tests move over.

I'm not sure how we got here, since BuiltinCertPins was meant to be this split
(in that it only tests for pinning data). You should be able to do a superbowl
shuffle of those few pinned bits into the BuiltinCertPins

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

   TransportSecurityState::DomainState domain_state;
 
   // We do more extensive checks for the first domain.
@@ -495,6 +524,7 @@ TEST_F(TransportSecurityStateTest, LongNames) {
 
 TEST_F(TransportSecurityStateTest, BuiltinCertPins) {
   TransportSecurityState state;
+  EnableStaticPinning(&state);
   TransportSecurityState::DomainState domain_state;
 
   EXPECT_TRUE(
@@ -585,6 +615,8 @@ TEST_F(TransportSecurityStateTest, PinValidationWithoutRejectedCerts) {
   }
 
   TransportSecurityState state;
+  EnableStaticPinning(&state);
+
   TransportSecurityState::DomainState domain_state;
   EXPECT_TRUE(
       state.GetStaticDomainState("blog.torproject.org", true, &domain_state));
@@ -597,6 +629,7 @@ TEST_F(TransportSecurityStateTest, PinValidationWithoutRejectedCerts) {
 
 TEST_F(TransportSecurityStateTest, OptionalHSTSCertPins) {
   TransportSecurityState state;
+  EnableStaticPinning(&state);
   TransportSecurityState::DomainState domain_state;
 
   EXPECT_FALSE(StaticShouldRedirect("www.google-analytics.com"));
@@ -629,6 +662,7 @@ TEST_F(TransportSecurityStateTest, OverrideBuiltins) {
   EXPECT_FALSE(StaticShouldRedirect("www.google.com"));
 
   TransportSecurityState state;
+  EnableStaticPinning(&state);

[Ryan Sleevi, 2014/08/07 23:48:39]

This should be removed. This is an HSTS test.

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

   TransportSecurityState::DomainState domain_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);