Separate http_security_headers from transport_security_state

net/url_request/url_request_http_job.cc

Index: net/url_request/url_request_http_job.cc
===================================================================
--- net/url_request/url_request_http_job.cc	(revision 165283)
+++ net/url_request/url_request_http_job.cc	(working copy)
@@ -667,93 +667,45 @@
 
 // NOTE: |ProcessStrictTransportSecurityHeader| and
 // |ProcessPublicKeyPinsHeader| have very similar structures, by design.
-// They manipulate different parts of |TransportSecurityState::DomainState|,
-// and they must remain complementary. If, in future changes here, there is
-// any conflict between their policies (such as in |domain_state.mode|), you
-// should resolve the conflict in favor of the more strict policy.
 void URLRequestHttpJob::ProcessStrictTransportSecurityHeader() {
   DCHECK(response_info_);
-
-  const URLRequestContext* ctx = request_->context();
+  TransportSecurityState* security_state =
+      request_->context()->transport_security_state();
   const SSLInfo& ssl_info = response_info_->ssl_info;
 
-  // Only accept strict transport security headers on HTTPS connections that
-  // have no certificate errors.
+  // Only accept HSTS headers on HTTPS connections that have no
+  // certificate errors.
   if (!ssl_info.is_valid() || IsCertStatusError(ssl_info.cert_status) ||
-      !ctx->transport_security_state()) {
+      !security_state)
     return;
-  }
 
-  TransportSecurityState* security_state = ctx->transport_security_state();
-  TransportSecurityState::DomainState domain_state;
-  const std::string& host = request_info_.url.host();
-
-  bool sni_available =
-      SSLConfigService::IsSNIAvailable(ctx->ssl_config_service());
-  if (!security_state->GetDomainState(host, sni_available, &domain_state))
-    // |GetDomainState| may have altered |domain_state| while searching. If
-    // not found, start with a fresh state.
-    domain_state.upgrade_mode =
-        TransportSecurityState::DomainState::MODE_FORCE_HTTPS;
-
-  HttpResponseHeaders* headers = GetResponseHeaders();
-  std::string value;
-  void* iter = NULL;
-  base::Time now = base::Time::Now();
-
   // http://tools.ietf.org/html/draft-ietf-websec-strict-transport-sec:
   //
   //   If a UA receives more than one STS header field in a HTTP response
   //   message over secure transport, then the UA MUST process only the
   //   first such header field.
-  bool seen_sts = false;
-  while (headers->EnumerateHeader(&iter, "Strict-Transport-Security", &value)) {
-    if (seen_sts)
-      return;
-    seen_sts = true;
-    TransportSecurityState::DomainState domain_state;
-    if (domain_state.ParseSTSHeader(now, value))
-      security_state->EnableHost(host, domain_state);
-  }
+  HttpResponseHeaders* headers = GetResponseHeaders();
+  std::string value;
+  if (headers->EnumerateHeader(NULL, "Strict-Transport-Security", &value))
+    security_state->AddHSTSHeader(request_info_.url.host(), value);
 }
 
 void URLRequestHttpJob::ProcessPublicKeyPinsHeader() {
   DCHECK(response_info_);
-
-  const URLRequestContext* ctx = request_->context();
+  TransportSecurityState* security_state =
+      request_->context()->transport_security_state();
   const SSLInfo& ssl_info = response_info_->ssl_info;
 
-  // Only accept public key pins headers on HTTPS connections that have no
+  // Only accept HPKP headers on HTTPS connections that have no
   // certificate errors.
   if (!ssl_info.is_valid() || IsCertStatusError(ssl_info.cert_status) ||
-      !ctx->transport_security_state()) {
+      !security_state)
     return;
-  }
 
-  TransportSecurityState* security_state = ctx->transport_security_state();
-  TransportSecurityState::DomainState domain_state;
-  const std::string& host = request_info_.url.host();
-
-  bool sni_available =
-      SSLConfigService::IsSNIAvailable(ctx->ssl_config_service());
-  if (!security_state->GetDomainState(host, sni_available, &domain_state))
-    // |GetDomainState| may have altered |domain_state| while searching. If
-    // not found, start with a fresh state.
-    domain_state.upgrade_mode =
-        TransportSecurityState::DomainState::MODE_DEFAULT;
-
   HttpResponseHeaders* headers = GetResponseHeaders();
-  void* iter = NULL;
   std::string value;
-  base::Time now = base::Time::Now();
-
-  while (headers->EnumerateHeader(&iter, "Public-Key-Pins", &value)) {
-    // Note that ParsePinsHeader updates |domain_state| (iff the header parses
-    // correctly), but does not completely overwrite it. It just updates the
-    // dynamic pinning metadata.
-    if (domain_state.ParsePinsHeader(now, value, ssl_info))
-      security_state->EnableHost(host, domain_state);
-  }
+  if (headers->EnumerateHeader(NULL, "Public-Key-Pins", &value))
+    security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info);

[Ryan Sleevi, 2012/11/13 19:02:32]

Pretty sure that HPKP should be using HasHeader && GetNormalizedHeader, so that
the headers are coalesced

[palmer, 2012/11/13 19:35:04]

As mentioned in previous comments, I tend to think we should match HSTS
behavior, i.e. not coalesce. Unless we strongly feel that HSTS was wrong to
specify that?

[Ryan Sleevi, 2012/11/13 19:43:15]

RFC 2616 4.2 is what causes the problems

"Multiple message-header fields with the same field-name MAY be present in a
message if and only if the entire field-value for that header field is defined
as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the
multiple header fields into one "field-name: field-value" pair, without changing
the semantics of the message, by appending each subsequent field-value to the
first, each separated by a comma. The order in which header fields with the same
field-name are received is therefore significant to the interpretation of the
combined field value, and thus a proxy MUST NOT change the order of these field
values when a message is forwarded."

So it looks like I'm wrong here, because we're using ";" as the separator rather
than ",". I seem to recall Julian Reschke pointed this out on the list, and I
also seem to recall Adam Barth having an opinion on the ABNF of HSTS.

So yeah, it seems like only one PKP header may be present. I'm not sure what the
security motivations of that are/were, but c'est le vie.

[unsafe, 2012/11/13 23:20:18]

I like that sentiment, if it means what I think which is: Done.

 }
 
 void URLRequestHttpJob::OnStartCompleted(int result) {