X.509-related cleanup

net/base/x509_cert_types_mac.cc

Index: net/base/x509_cert_types_mac.cc
diff --git a/net/base/x509_cert_types_mac.cc b/net/base/x509_cert_types_mac.cc
index 14d5eee26b188bb7bb3e64504b1807f09d4756c6..6cd691068e2fc38cb6ace9e0147116421e5903dc 100644
--- a/net/base/x509_cert_types_mac.cc
+++ b/net/base/x509_cert_types_mac.cc
@@ -14,7 +14,9 @@
 
 namespace net {
 
-static const CSSM_OID* kOIDs[] = {
+namespace {
+
+const CSSM_OID* kOIDs[] = {
     &CSSMOID_CommonName,
     &CSSMOID_LocalityName,
     &CSSMOID_StateProvinceName,
@@ -25,65 +27,6 @@ static const CSSM_OID* kOIDs[] = {
     &CSSMOID_DNQualifier      // This should be "DC" but is undoubtedly wrong.
 };                            // TODO(avi): Find the right OID.
 
-// Converts raw CSSM_DATA to a std::string. (Char encoding is unaltered.)
-static std::string DataToString(CSSM_DATA data);
-
-// Converts raw CSSM_DATA in ISO-8859-1 to a std::string in UTF-8.
-static std::string Latin1DataToUTF8String(CSSM_DATA data);
-
-// Converts big-endian UTF-16 to UTF-8 in a std::string.
-// Note: The byte-order flipping is done in place on the input buffer!
-static bool UTF16BigEndianToUTF8(char16* chars, size_t length,
-                                 std::string* out_string);
-
-// Converts big-endian UTF-32 to UTF-8 in a std::string.
-// Note: The byte-order flipping is done in place on the input buffer!
-static bool UTF32BigEndianToUTF8(char32* chars, size_t length,
-                                 std::string* out_string);
-
-// Adds a type+value pair to the appropriate vector from a C array.
-// The array is keyed by the matching OIDs from kOIDS[].
-  static void AddTypeValuePair(const CSSM_OID type,
-                               const std::string& value,
-                               std::vector<std::string>* values[]);
-
-// Stores the first string of the vector, if any, to *single_value.
-static void SetSingle(const std::vector<std::string> &values,
-                      std::string* single_value);
-
-
-void CertPrincipal::Parse(const CSSM_X509_NAME* name) {

[wtc, 2010/11/17 21:31:53]

PERSONAL OPINION: when a file becomes stable, it may not be
a good idea to move large blocks of code around to conform
to the Style Guide's recommendation of same
declaration/definition order, because it generates noise
in the diffs.

I am willing to live with imperfect matching of
declaration and definition orders, in exchange for diffs
that show me only what actually was changed.

Since you've already made the change and bulach has
reviewed it, no need to change it back.

[Ryan Sleevi, 2010/11/18 08:21:36]

It was more about pushing this code into the anonymous namespace than the
ordering. Because the CertPrincipal function was smack in the middle, it had to
be shifted out, and unfortunately the diff didn't take that well.

So I can better understand, is the concern in general against moving code as
part of cleanup? Or even more broadly, cleanup tasks? I know the Google Style
Guide lists it as acceptable to disregard the standard when working existing
non-conformant code, what I wasn't sure is where it fits for things like this
situation: where several files in net/base/ diverge for no discernible reason
from the pattern of the rest of the files next to it and from the Style Guide in
general (the Mac cleanup, the unittests, and this). Are these changes generally
not desirable?

-  std::vector<std::string> common_names, locality_names, state_names,
-      country_names;
-
-  std::vector<std::string>* values[] = {
-      &common_names, &locality_names,
-      &state_names, &country_names,
-      &(this->street_addresses),
-      &(this->organization_names),
-      &(this->organization_unit_names),
-      &(this->domain_components)
-  };
-  DCHECK(arraysize(kOIDs) == arraysize(values));
-
-  for (size_t rdn = 0; rdn < name->numberOfRDNs; ++rdn) {
-    CSSM_X509_RDN rdn_struct = name->RelativeDistinguishedName[rdn];
-    for (size_t pair = 0; pair < rdn_struct.numberOfPairs; ++pair) {
-      CSSM_X509_TYPE_VALUE_PAIR pair_struct =
-          rdn_struct.AttributeTypeAndValue[pair];
-      AddTypeValuePair(pair_struct.type,
-                       DataToString(pair_struct.value),
-                       values);
-    }
-  }
-
-  SetSingle(common_names, &this->common_name);
-  SetSingle(locality_names, &this->locality_name);
-  SetSingle(state_names, &this->state_or_province_name);
-  SetSingle(country_names, &this->country_name);
-}
-
-
 // The following structs and templates work with Apple's very arcane and under-
 // documented SecAsn1Parser API, which is apparently the same as NSS's ASN.1
 // decoder:
@@ -137,7 +80,7 @@ struct KeyValuePairs {
 };
 
 static const SecAsn1Template kKeyValuePairSetTemplate[] = {
-  { SEC_ASN1_SET_OF, offsetof(KeyValuePairs,pairs),
+  { SEC_ASN1_SET_OF, offsetof(KeyValuePairs, pairs),
       kKeyValuePairTemplate, sizeof(KeyValuePairs) }
 };
 
@@ -146,10 +89,98 @@ struct X509Name {
 };
 
 static const SecAsn1Template kNameTemplate[] = {
-  { SEC_ASN1_SEQUENCE_OF, offsetof(X509Name,pairs_list),
+  { SEC_ASN1_SEQUENCE_OF, offsetof(X509Name, pairs_list),
       kKeyValuePairSetTemplate, sizeof(X509Name) }
 };
 
+// Converts raw CSSM_DATA to a std::string. (Char encoding is unaltered.)
+std::string DataToString(CSSM_DATA data) {
+  return std::string(
+      reinterpret_cast<std::string::value_type*>(data.Data),
+      data.Length);
+}
+
+// Converts raw CSSM_DATA in ISO-8859-1 to a std::string in UTF-8.
+std::string Latin1DataToUTF8String(CSSM_DATA data) {
+  string16 utf16;
+  if (!CodepageToUTF16(DataToString(data), base::kCodepageLatin1,
+                       base::OnStringConversionError::FAIL, &utf16))
+    return "";
+  return UTF16ToUTF8(utf16);
+}
+
+// Converts big-endian UTF-16 to UTF-8 in a std::string.
+// Note: The byte-order flipping is done in place on the input buffer!
+bool UTF16BigEndianToUTF8(char16* chars, size_t length,
+                          std::string* out_string) {
+  for (size_t i = 0; i < length; i++)
+    chars[i] = EndianU16_BtoN(chars[i]);
+  return UTF16ToUTF8(chars, length, out_string);
+}
+
+// Converts big-endian UTF-32 to UTF-8 in a std::string.
+// Note: The byte-order flipping is done in place on the input buffer!
+bool UTF32BigEndianToUTF8(char32* chars, size_t length,
+                          std::string* out_string) {
+  for (size_t i = 0; i < length; i++)

[bulach, 2010/11/17 12:16:50]

nit: ++i

+    chars[i] = EndianS32_BtoN(chars[i]);
+#if defined(WCHAR_T_IS_UTF32)
+  return WideToUTF8(reinterpret_cast<const wchar_t*>(chars),
+                    length, out_string);
+#else
+#error This code doesn't handle 16-bit wchar_t.
+#endif
+}
+
+// Adds a type+value pair to the appropriate vector from a C array.
+// The array is keyed by the matching OIDs from kOIDS[].
+void AddTypeValuePair(const CSSM_OID type,
+                      const std::string& value,
+                      std::vector<std::string>* values[]) {
+  for (size_t oid = 0; oid < arraysize(kOIDs); ++oid) {
+    if (CSSMOIDEqual(&type, kOIDs[oid])) {
+      values[oid]->push_back(value);
+      break;
+    }
+  }
+}
+
+// Stores the first string of the vector, if any, to *single_value.
+void SetSingle(const std::vector<std::string> &values,

[bulach, 2010/11/17 12:16:50]

nit: s/> &values/>& values/

+                      std::string* single_value) {

[bulach, 2010/11/17 12:16:50]

nit:unindent

+  // We don't expect to have more than one CN, L, S, and C.
+  LOG_IF(WARNING, values.size() > 1) << "Didn't expect multiple values";
+  if (values.size() > 0)
+    *single_value = values[0];
+}
+
+bool match(const std::string &str, const std::string &against) {

[bulach, 2010/11/17 12:16:50]

nit: s/string &/string& / for both params

+  // TODO(snej): Use the full matching rules specified in RFC 5280 sec. 7.1
+  // including trimming and case-folding: <http://www.ietf.org/rfc/rfc5280.txt>.
+  return against == str;
+}
+
+bool match(const std::vector<std::string> &rdn1,
+           const std::vector<std::string> &rdn2) {

[bulach, 2010/11/17 12:16:50]

nit: s/> &rd/>& rd/ for the two above

+  // "Two relative distinguished names RDN1 and RDN2 match if they have the
+  // same number of naming attributes and for each naming attribute in RDN1
+  // there is a matching naming attribute in RDN2." --RFC 5280 sec. 7.1.
+  if (rdn1.size() != rdn2.size())
+    return false;
+  for (unsigned i1 = 0; i1 < rdn1.size(); ++i1) {
+    unsigned i2;
+    for (i2 = 0; i2 < rdn2.size(); ++i2) {
+      if (match(rdn1[i1], rdn2[i2]))
+          break;
+    }
+    if (i2 == rdn2.size())
+      return false;
+  }
+  return true;
+}
+
+}  // namespace
+
 bool CertPrincipal::ParseDistinguishedName(const void* ber_name_data,
                                            size_t length) {
   DCHECK(ber_name_data);
@@ -182,7 +213,7 @@ bool CertPrincipal::ParseDistinguishedName(const void* ber_name_data,
   };
   DCHECK(arraysize(kOIDs) == arraysize(values));
 
-  for (int rdn=0; name[rdn].pairs_list; ++rdn) {
+  for (int rdn = 0; name[rdn].pairs_list; ++rdn) {
     KeyValuePair *pair;
     for (int pair_index = 0;
          NULL != (pair = name[rdn].pairs_list[0][pair_index].pairs);
@@ -235,59 +266,47 @@ bool CertPrincipal::ParseDistinguishedName(const void* ber_name_data,
   return true;
 }
 
+void CertPrincipal::Parse(const CSSM_X509_NAME* name) {
+  std::vector<std::string> common_names, locality_names, state_names,
+      country_names;
 
-// SUBROUTINES:
-
-static std::string DataToString(CSSM_DATA data) {
-  return std::string(
-      reinterpret_cast<std::string::value_type*>(data.Data),
-      data.Length);
-}
-
-static std::string Latin1DataToUTF8String(CSSM_DATA data) {
-  string16 utf16;
-  if (!CodepageToUTF16(DataToString(data), base::kCodepageLatin1,
-                       base::OnStringConversionError::FAIL, &utf16))
-    return "";
-  return UTF16ToUTF8(utf16);
-}
-
-bool UTF16BigEndianToUTF8(char16* chars, size_t length,
-                          std::string* out_string) {
-  for (size_t i = 0; i < length; i++)
-    chars[i] = EndianU16_BtoN(chars[i]);
-  return UTF16ToUTF8(chars, length, out_string);
-}
-
-bool UTF32BigEndianToUTF8(char32* chars, size_t length,
-                          std::string* out_string) {
-  for (size_t i = 0; i < length; i++)
-    chars[i] = EndianS32_BtoN(chars[i]);
-#if defined(WCHAR_T_IS_UTF32)
-  return WideToUTF8(reinterpret_cast<const wchar_t*>(chars),
-                    length, out_string);
-#else
-#error This code doesn't handle 16-bit wchar_t.
-#endif
-}
+  std::vector<std::string>* values[] = {
+      &common_names, &locality_names,
+      &state_names, &country_names,
+      &(this->street_addresses),
+      &(this->organization_names),
+      &(this->organization_unit_names),
+      &(this->domain_components)
+  };
+  DCHECK(arraysize(kOIDs) == arraysize(values));
 
-  static void AddTypeValuePair(const CSSM_OID type,
-                               const std::string& value,
-                               std::vector<std::string>* values[]) {
-  for (size_t oid = 0; oid < arraysize(kOIDs); ++oid) {
-    if (CSSMOIDEqual(&type, kOIDs[oid])) {
-      values[oid]->push_back(value);
-      break;
+  for (size_t rdn = 0; rdn < name->numberOfRDNs; ++rdn) {
+    CSSM_X509_RDN rdn_struct = name->RelativeDistinguishedName[rdn];
+    for (size_t pair = 0; pair < rdn_struct.numberOfPairs; ++pair) {
+      CSSM_X509_TYPE_VALUE_PAIR pair_struct =
+          rdn_struct.AttributeTypeAndValue[pair];
+      AddTypeValuePair(pair_struct.type,
+                       DataToString(pair_struct.value),
+                       values);
     }
   }
+
+  SetSingle(common_names, &this->common_name);
+  SetSingle(locality_names, &this->locality_name);
+  SetSingle(state_names, &this->state_or_province_name);
+  SetSingle(country_names, &this->country_name);
 }
 
-static void SetSingle(const std::vector<std::string> &values,
-                      std::string* single_value) {
-  // We don't expect to have more than one CN, L, S, and C.
-  LOG_IF(WARNING, values.size() > 1) << "Didn't expect multiple values";
-  if (values.size() > 0)
-    *single_value = values[0];
+bool CertPrincipal::Matches(const CertPrincipal& against) const {
+  return match(common_name, against.common_name) &&
+      match(common_name, against.common_name) &&

[bulach, 2010/11/17 12:16:50]

301 and 302 are redundant

[Ryan Sleevi, 2010/11/18 08:21:36]

Thanks! Good catch!

+      match(locality_name, against.locality_name) &&
+      match(state_or_province_name, against.state_or_province_name) &&
+      match(country_name, against.country_name) &&
+      match(street_addresses, against.street_addresses) &&
+      match(organization_names, against.organization_names) &&
+      match(organization_unit_names, against.organization_unit_names) &&
+      match(domain_components, against.domain_components);
 }
 
 }  // namespace net