Separate http_security_headers from transport_security_state

net/base/x509_cert_types.cc

Index: net/base/x509_cert_types.cc
===================================================================
--- net/base/x509_cert_types.cc	(revision 163343)
+++ net/base/x509_cert_types.cc	(working copy)
@@ -7,10 +7,13 @@
 #include <cstdlib>
 #include <cstring>
 
+#include "base/base64.h"
 #include "base/logging.h"
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
 #include "base/string_piece.h"
+#include "base/string_split.h"
+#include "base/string_util.h"
 #include "base/time.h"
 #include "net/base/x509_certificate.h"
 
@@ -37,7 +40,6 @@
 
 }  // namespace
 
-// static
 bool IsSHA1HashInSortedArray(const SHA1HashValue& hash,
                              const uint8* array,
                              size_t array_byte_len) {
@@ -47,6 +49,45 @@
                          CompareSHA1Hashes);
 }
 
+bool HashesIntersect(const HashValueVector& a,
+                     const HashValueVector& b) {
+  for (HashValueVector::const_iterator i = a.begin(); i != a.end(); ++i) {
+    HashValueVector::const_iterator j =
+      std::find_if(b.begin(), b.end(), HashValuesEqualPredicate(*i));
+    if (j != b.end())
+      return true;
+  }
+  return false;
+}
+
+std::string HashesToBase64String(const HashValueVector& hashes) {
+  std::string str;
+  for (size_t i = 0; i != hashes.size(); ++i) {
+    if (i != 0)
+      str += ",";
+    str += hashes[i].WriteBase64String();
+  }
+  return str;
+}
+
+bool Base64StringToHashes(const std::string& hashes_str,
+                          HashValueVector* hashes) {
+  if (!hashes_str.empty()) {

[Ryan Sleevi, 2012/10/25 01:59:09]

Rather than structuring code along the
if (success) {
  // do stuff
} [else optional error handling]

pattern, Chromium/Google Open Source code tries to follow the

if (error) {
  // error handling
  return
}
// do stuff

style of structure. This helps reduce indentation levels.

suggested rewrite:
if (hashes_str.empty())
  return [something]

[Ryan Sleevi, 2012/10/25 01:59:09]

design nit:
You have not documented what the pre-and-post conditions of this function in the
header file, but as such, I'm not sure the current behaviours are a good API.

The current API appends data to |hashes|, and returns success when there are no
hashes. Typically, we try to make this very clear in the naming of the function,
as the general expectation is that it'll replace |hashes|, and also try to make
invalid inputs fail.

hashes->clear();
if (hashes_str.empty())
  return false;
// Parse

[unsafe, 2012/10/25 06:59:54]

Fixed - documented, clears |hashes|, returns true even if none.

+    std::vector<std::string> type_and_b64s;
+    base::SplitString(hashes_str, ',', &type_and_b64s);
+
+    for (size_t i = 0; i != type_and_b64s.size(); i++) {

[Ryan Sleevi, 2012/10/25 01:59:09]

style nit: pre-increment (++i)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

[unsafe, 2012/10/25 06:59:54]

Done.

+      std::string type_and_b64;
+      RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64);

[Ryan Sleevi, 2012/10/25 01:59:09]

RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64s[i]);

That said, I believe the real function, based on the spec, should be
net::HttpUtil::TrimLWS(), which net/base cannot use or call (layering violation)

This is part of why I think this function may belong somewhere under net/http to
handle header parsing.

[unsafe, 2012/10/25 06:59:54]

This function isn't processing the HTTP header, it process net_internals input
currently, and could be used for unit tests or debugging, so I think its OK at
this level.

+      net::HashValue hash;
+      if (!hash.ParseBase64String(type_and_b64))
+        return false;
+      hashes->push_back(hash);
+    }
+  }
+  return true;
+}
+
 CertPrincipal::CertPrincipal() {
 }
 
@@ -157,6 +198,36 @@
   }
 }
 
+bool HashValue::ParseBase64String(const std::string& value) {
+  std::string b64;

[Ryan Sleevi, 2012/10/25 01:59:09]

naming nit: b64 is too abbreviated. Applies throughout the file (eg:
Base64StringToHashes)

(See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...
)

perf: use base::StringPiece to avoid needing to make a copy of these values. You
can still trim prefix/suffices as possible.

[unsafe, 2012/10/25 06:59:54]

Changed the names.
It's about the same efficiency as the previous TransportSecurityState::ParsePin.
 I'm not familiar with base::StringPiece.  This is used to read the dynamic
JSON, so I suppose efficiency matters slightly, can you give me more pointers?

+  if (value.substr(0, 5) == "sha1/") {
+    tag = HASH_VALUE_SHA1;
+    b64 = value.substr(5, 28);  // length of base64 string
+  } else if (value.substr(0, 7) == "sha256/") {
+    tag = HASH_VALUE_SHA256;
+    b64 = value.substr(7, 44);  // length of base64 string
+  } else {
+    return false;
+  }
+
+  std::string decoded;
+  if (!base::Base64Decode(b64, &decoded) || decoded.size() != size()) {
+    return false;
+  }
+  memcpy(data(), decoded.data(), size());
+  return true;
+}
+
+std::string HashValue::WriteBase64String() const {
+  std::string b64;
+  base::Base64Encode(std::string((const char*)data(), size()), &b64);

[Ryan Sleevi, 2012/10/25 01:59:09]

design nit: Encode expects a base::StringPiece rather than an std::string.
base::StringPiece lets you avoid the copy.

style nit: Use C++ casts (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...
)

[unsafe, 2012/10/25 06:59:54]

Done.

+  if (tag == HASH_VALUE_SHA1)
+    return std::string("sha1/" + b64);
+  else if (tag == HASH_VALUE_SHA256)
+    return std::string("sha256/" + b64);
+  return std::string("unknown/" + b64);

[Ryan Sleevi, 2012/10/25 01:59:09]

nit: Write this as a switch statement, with no "default" case, and an error
return after the switch, and the compiler will (should?) warn any time the tag
is not handled.

Also, NOTREACHED() here, since WriteBase64String() should never be called with
an unsupported hash value (Parsing shouldn't use this, because it's user input,
but outputting is a coder issue)

[unsafe, 2012/10/25 06:59:54]

Changed to a switch.  The existing code in this file does not folow the "no
default case" idiom, so I didn't.  Should all the switch statements be changed?

+}
+
 size_t HashValue::size() const {
   switch (tag) {
     case HASH_VALUE_SHA1: