Don't set MODE_DEFAULT when adding HPKP header.

Leave that to the default value, or whatever value an HSTS header set.

TEST=net_unittests; after visiting https://www.mv-rechberghausen.de, query
that domain in chrome://net-internal/#hsts and expect pins AND mode STRICT.

BUG=226068


Review URL: https://chromiumcodereview.appspot.com/13483007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@193443 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 65 insertions, 23 deletions

diff --git a/chrome_frame/test/net/fake_external_tab.cc b/chrome_frame/test/net/fake_external_tab.cc
index f5bbfba..c065333 100644
--- a/chrome_frame/test/net/fake_external_tab.cc
+++ b/chrome_frame/test/net/fake_external_tab.cc
@@ -254,10 +254,11 @@ void FilterDisabledTests() {
 
     // These tests use HTTPS, and IE's trust store does not have the test
     // certs. So these tests time out waiting for user input. The
-    // functionality they test (HTTP Strict Transport Security) does not
-    // work in Chrome Frame anyway.
+    // functionality they test (HTTP Strict Transport Security and
+    // HTTP-based Public Key Pinning) does not work in Chrome Frame anyway.
     "URLRequestTestHTTP.ProcessSTS",
     "URLRequestTestHTTP.ProcessSTSOnce",
+    "URLRequestTestHTTP.ProcessSTSAndPKP",
 
     // These tests have been disabled as the Chrome cookie policies don't make
     // sense or have not been implemented for the host network stack.
diff --git a/net/data/url_request_unittest/hsts-and-hpkp-headers.html b/net/data/url_request_unittest/hsts-and-hpkp-headers.html
new file mode 100644
index 0000000..364322d
--- /dev/null
+++ b/net/data/url_request_unittest/hsts-and-hpkp-headers.html
@@ -0,0 +1 @@
+This file is boring; all the action's in the .mock-http-headers.
diff --git a/net/data/url_request_unittest/hsts-and-hpkp-headers.html.mock-http-headers b/net/data/url_request_unittest/hsts-and-hpkp-headers.html.mock-http-headers
new file mode 100644
index 0000000..2b66356
--- /dev/null
+++ b/net/data/url_request_unittest/hsts-and-hpkp-headers.html.mock-http-headers
@@ -0,0 +1,8 @@
+HTTP/1.1 200 OK
+Cache-Control: private
+Content-Type: text/html; charset=ISO-8859-1
+X-Multiple-Entries: a
+X-Multiple-Entries: b
+Strict-Transport-Security: max-age=12300
+Strict-Transport-Security: max-age=12300; includeSubdomains
+Public-Key-Pins: max-age=50000; pin-sha1="K9e3/nFL5j90GuVJOJBv6WXpvcs="; pin-sha256="kd16uBd5KFa9IJjF0X+8B+BXdAWkYYRZruNKDZ0M9Zw="
diff --git a/net/http/http_security_headers.cc b/net/http/http_security_headers.cc
index 7671c8e..4848441 100644
--- a/net/http/http_security_headers.cc
+++ b/net/http/http_security_headers.cc
@@ -164,9 +164,10 @@ bool ParseAndAppendPin(const std::string& value,
 //     the UA, the UA MUST ignore the unrecognized directives and if the
 //     STS header field otherwise satisfies the above requirements (1
 //     through 4), the UA MUST process the recognized directives.
-bool ParseHSTSHeader(const base::Time& now, const std::string& value,
-                     base::Time* expiry,         // OUT
-                     bool* include_subdomains) {  // OUT
+bool ParseHSTSHeader(const base::Time& now,
+                     const std::string& value,
+                     base::Time* expiry,
+                     bool* include_subdomains) {
   uint32 max_age_candidate = 0;
   bool include_subdomains_candidate = false;
 
diff --git a/net/http/http_security_headers_unittest.cc b/net/http/http_security_headers_unittest.cc
index 4f1e580..9f2f37b 100644
--- a/net/http/http_security_headers_unittest.cc
+++ b/net/http/http_security_headers_unittest.cc
@@ -422,5 +422,5 @@ TEST_F(HttpSecurityHeadersTest, ValidPinsHeadersSHA1) {
 TEST_F(HttpSecurityHeadersTest, ValidPinsHeadersSHA256) {
   TestValidPinsHeaders(HASH_VALUE_SHA256);
 }
-};
 
+};    // namespace net
diff --git a/net/http/transport_security_state.cc b/net/http/transport_security_state.cc
index 3fb95dd..49ba1c1 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -637,7 +637,6 @@ bool TransportSecurityState::AddHPKPHeader(const std::string& host,
   if (ParseHPKPHeader(now, value, ssl_info.public_key_hashes,
                       &domain_state.dynamic_spki_hashes_expiry,
                       &domain_state.dynamic_spki_hashes)) {
-    domain_state.upgrade_mode = DomainState::MODE_DEFAULT;
     domain_state.created = now;
     EnableHost(host, domain_state);
     return true;
diff --git a/net/http/transport_security_state.h b/net/http/transport_security_state.h
index a4e3f33..933c86c 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -108,10 +108,9 @@ class NET_EXPORT TransportSecurityState
 
     // Are subdomains subject to this DomainState?
     //
-    // TODO(palmer): Decide if we should have separate |pin_subdomains| and
-    // |upgrade_subdomains|. Alternately, and perhaps better, is to separate
-    // DomainState into UpgradeState and PinState (requiring also changing the
-    // serialization format?).
+    // TODO(palmer): We need to have separate |pin_subdomains| and
+    // |upgrade_subdomains|. Trevor Perrin is working on a new storage model
+    // that will enable this.
     bool include_subdomains;
 
     // Optional; hashes of static pinned SubjectPublicKeyInfos. Unless both
diff --git a/net/http/transport_security_state_unittest.cc b/net/http/transport_security_state_unittest.cc
index d4fac45..0aa309b 100644
--- a/net/http/transport_security_state_unittest.cc
+++ b/net/http/transport_security_state_unittest.cc
@@ -149,18 +149,12 @@ TEST_F(TransportSecurityStateTest, DeleteDynamicDataForHost) {
 }
 
 TEST_F(TransportSecurityStateTest, IsPreloaded) {
-  const std::string paypal =
-      CanonicalizeHost("paypal.com");
-  const std::string www_paypal =
-      CanonicalizeHost("www.paypal.com");
-  const std::string a_www_paypal =
-      CanonicalizeHost("a.www.paypal.com");
-  const std::string abc_paypal =
-      CanonicalizeHost("a.b.c.paypal.com");
-  const std::string example =
-      CanonicalizeHost("example.com");
-  const std::string aypal =
-      CanonicalizeHost("aypal.com");
+  const std::string paypal = CanonicalizeHost("paypal.com");
+  const std::string www_paypal = CanonicalizeHost("www.paypal.com");
+  const std::string a_www_paypal = CanonicalizeHost("a.www.paypal.com");
+  const std::string abc_paypal = CanonicalizeHost("a.b.c.paypal.com");
+  const std::string example = CanonicalizeHost("example.com");
+  const std::string aypal = CanonicalizeHost("aypal.com");
 
   TransportSecurityState state;
   TransportSecurityState::DomainState domain_state;
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index c3c68a1..1e2e7e6 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -3807,6 +3807,45 @@ TEST_F(URLRequestTestHTTP, ProcessSTSOnce) {
   EXPECT_FALSE(domain_state.include_subdomains);
 }
 
+TEST_F(URLRequestTestHTTP, ProcessSTSAndPKP) {
+  TestServer::SSLOptions ssl_options;
+  TestServer https_test_server(
+      TestServer::TYPE_HTTPS,
+      ssl_options,
+      base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest")));
+  ASSERT_TRUE(https_test_server.Start());
+
+  TestDelegate d;
+  URLRequest request(
+      https_test_server.GetURL("files/hsts-and-hpkp-headers.html"),
+      &d,
+      &default_context_);
+  request.Start();
+  MessageLoop::current()->Run();
+
+  // We should have set parameters from the first header, not the second.
+  TransportSecurityState* security_state =
+      default_context_.transport_security_state();
+  bool sni_available = true;
+  TransportSecurityState::DomainState domain_state;
+  EXPECT_TRUE(security_state->GetDomainState(
+      TestServer::kLocalhost, sni_available, &domain_state));
+  EXPECT_EQ(TransportSecurityState::DomainState::MODE_FORCE_HTTPS,
+            domain_state.upgrade_mode);
+#if defined(OS_ANDROID)
+  // Android's CertVerifyProc does not (yet) handle pins.
+#else
+  EXPECT_TRUE(domain_state.HasPublicKeyPins());
+#endif
+  EXPECT_NE(domain_state.upgrade_expiry,
+            domain_state.dynamic_spki_hashes_expiry);
+
+  // TODO(palmer): In the (near) future, TransportSecurityState will have a
+  // storage model allowing us to have independent values for
+  // include_subdomains. At that time, extend this test.
+  //EXPECT_FALSE(domain_state.include_subdomains);
+}
+
 TEST_F(URLRequestTestHTTP, ContentTypeNormalizationTest) {
   ASSERT_TRUE(test_server_.Start());