Separate http_security_headers from transport_security_state

net/base/x509_cert_types.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_cert_types.h"
 
 #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"
 
 namespace net {
 
 namespace {
 
 // Helper for ParseCertificateDate. |*field| must contain at least
 // |field_len| characters. |*field| will be advanced by |field_len| on exit.
 // |*ok| is set to false if there is an error in parsing the number, but left
 // untouched otherwise. Returns the parsed integer.
 int ParseIntAndAdvance(const char** field, size_t field_len, bool* ok) {
   int result = 0;
   *ok &= base::StringToInt(base::StringPiece(*field, field_len), &result);
   *field += field_len;
   return result;
 }
 
 // CompareSHA1Hashes is a helper function for using bsearch() with an array of
 // SHA1 hashes.
 int CompareSHA1Hashes(const void* a, const void* b) {
   return memcmp(a, b, base::kSHA1Length);
 }
 
 }  // namespace
 
-// static
 bool IsSHA1HashInSortedArray(const SHA1HashValue& hash,
                              const uint8* array,
                              size_t array_byte_len) {
   DCHECK_EQ(0u, array_byte_len % base::kSHA1Length);
   const size_t arraylen = array_byte_len / base::kSHA1Length;
   return NULL != bsearch(hash.data, array, arraylen, base::kSHA1Length,
                          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() {
 }
 
 CertPrincipal::CertPrincipal(const std::string& name) : common_name(name) {}
 
 CertPrincipal::~CertPrincipal() {
 }
 
 std::string CertPrincipal::GetDisplayName() const {
   if (!common_name.empty())
@@ skipping 90 matching lines @@
     case HASH_VALUE_SHA1:
       return fingerprint.sha1.Equals(other.fingerprint.sha1);
     case HASH_VALUE_SHA256:
       return fingerprint.sha256.Equals(other.fingerprint.sha256);
     default:
       NOTREACHED() << "Unknown HashValueTag " << tag;
       return false;
   }
 }
 
+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:
       return sizeof(fingerprint.sha1.data);
     case HASH_VALUE_SHA256:
       return sizeof(fingerprint.sha256.data);
     default:
       NOTREACHED() << "Unknown HashValueTag " << tag;
       // Although this is NOTREACHED, this function might be inlined and its
       // return value can be passed to memset as the length argument. If we
@@ skipping 15 matching lines @@
       return fingerprint.sha1.data;
     case HASH_VALUE_SHA256:
       return fingerprint.sha256.data;
     default:
       NOTREACHED() << "Unknown HashValueTag " << tag;
       return NULL;
   }
 }
 
 }  // namespace net