PKI library Mac trust store integration

net/cert/internal/trust_store_mac_unittest.cc

Index: net/cert/internal/trust_store_mac_unittest.cc
diff --git a/net/cert/internal/trust_store_mac_unittest.cc b/net/cert/internal/trust_store_mac_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..02e65290e17808b730d8a91f5882205a80b572ee
--- /dev/null
+++ b/net/cert/internal/trust_store_mac_unittest.cc
@@ -0,0 +1,315 @@
+// Copyright 2016 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/cert/internal/trust_store_mac.h"
+
+#include "base/base_paths.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/path_service.h"
+#include "base/process/launch.h"
+#include "base/strings/string_split.h"
+#include "net/cert/internal/cert_errors.h"
+#include "net/cert/internal/test_helpers.h"
+#include "net/cert/pem_tokenizer.h"
+#include "net/cert/test_keychain_search_list_mac.h"
+#include "net/cert/x509_certificate.h"
+#include "net/test/test_data_directory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+// The PEM block header used for DER certificates
+const char kCertificateHeader[] = "CERTIFICATE";
+
+::testing::AssertionResult ReadTestPem(const std::string& file_name,
+                                       const std::string& block_name,
+                                       std::string* result) {
+  const PemBlockMapping mappings[] = {
+      {block_name.c_str(), result},
+  };
+
+  return ReadTestDataFromPemFile(file_name, mappings);
+}

[Ryan Sleevi, 2017/02/14 19:26:17]

Is this helper needed?

[mattm, 2017/02/16 22:06:09]

merged it into the other function

+
+::testing::AssertionResult ReadTestCert(

[Ryan Sleevi, 2017/02/14 19:26:17]

Perhaps document? :)

[mattm, 2017/02/16 22:06:09]

Done.

+    const std::string& file_name,
+    scoped_refptr<ParsedCertificate>* result) {
+  std::string der;
+  ::testing::AssertionResult r = ReadTestPem(
+      "net/data/ssl/certificates/" + file_name, "CERTIFICATE", &der);

[Ryan Sleevi, 2017/02/14 19:26:17]

Why not use kCertificateHeader here, like you do elsewhere?

[mattm, 2017/02/16 22:06:09]

Done.

+  if (!r)
+    return r;
+  CertErrors errors;
+  *result = ParsedCertificate::Create(der, {}, &errors);
+  if (!*result) {
+    return ::testing::AssertionFailure()
+           << "ParseCertificate::Create() failed:\n"
+           << errors.ToDebugString();
+  }
+  return ::testing::AssertionSuccess();
+}
+
+std::vector<std::string> GetDerCertsFromMatchingItems(

[Ryan Sleevi, 2017/02/14 19:26:17]

Document?

[mattm, 2017/02/16 22:06:09]

Done.

+    CFArrayRef matching_items) {
+  std::vector<std::string> result;
+
+  for (CFIndex i = 0, item_count = CFArrayGetCount(matching_items);
+       i < item_count; ++i) {
+    SecCertificateRef match_cert_handle = reinterpret_cast<SecCertificateRef>(
+        const_cast<void*>(CFArrayGetValueAtIndex(matching_items, i)));
+    if (!match_cert_handle) {
+      ADD_FAILURE() << "null matching_item " << i;
+      continue;
+    }
+    base::ScopedCFTypeRef<CFDataRef> der_data(
+        SecCertificateCopyData(match_cert_handle));
+    if (!der_data) {
+      ADD_FAILURE() << "SecCertificateCopyData error";
+      continue;
+    }
+    result.push_back(std::string(
+        reinterpret_cast<const char*>(CFDataGetBytePtr(der_data.get())),
+        CFDataGetLength(der_data.get())));
+  }
+
+  return result;
+}
+
+bool MatchingItemsMatch(CFArrayRef matching_items,
+                        ParsedCertificateList expected_matches) {
+  std::vector<std::string> der_result_matches(
+      GetDerCertsFromMatchingItems(matching_items));
+  std::sort(der_result_matches.begin(), der_result_matches.end());
+
+  std::vector<std::string> der_expected_matches;
+  for (const auto& it : expected_matches)
+    der_expected_matches.push_back(it->der_cert().AsString());
+  std::sort(der_expected_matches.begin(), der_expected_matches.end());
+
+  if (der_expected_matches == der_result_matches)
+    return true;
+
+  // Print some extra information for debugging.
+  EXPECT_EQ(der_expected_matches, der_result_matches);
+  return false;
+}

[Ryan Sleevi, 2017/02/14 19:26:17]

A (perhaps convoluted) way in GTest+GMock to handle this would be

std::vector<std::string> SecCertificateArrayAsDER(CFArrayRef array);
std::vector<std::string> ParsedCertificateListAsDER(ParsedCertificateList list);

EXPECT_THAT(SecCertificateArrayAsDER(...),
UnorderedElementsAreArray(ParsedCertificateListAsDER(...));

Gives you even more verbose pretty-printers.

[mattm, 2017/02/16 22:06:09]

Done.

+
+}  // namespace
+
+TEST(TrustStoreMacTest, MultiRootNotTrusted) {

[Ryan Sleevi, 2017/02/14 19:26:17]

Document what this is testing :)

[mattm, 2017/02/16 22:06:09]

Done.

+  std::unique_ptr<TestKeychainSearchList> test_keychain_search_list(
+      TestKeychainSearchList::Create());
+  ASSERT_TRUE(test_keychain_search_list);
+  base::FilePath keychain_path(
+      GetTestCertsDirectory().AppendASCII("multi-root.keychain"));

[Ryan Sleevi, 2017/02/14 19:26:17]

The thing that makes me nervous from our API inconsistency is that here on 108,
we use GetTestCertsDirectory(), but on line 43, we don't. 

I realize that's partly due to ReadTestDataFromPemFile not basing itself in the
test certs directory, but it does create some weird API skew.

[mattm, 2017/02/16 22:06:09]

This is true, but I guess it doesn't worry me too much. As in, it might be nice
to clean up somehow, but I'm not sure there's enough payoff to make it worth it.

+  // SecKeychainOpen does not fail if the file doesn't exist, so assert it here
+  // for easier debugging.
+  ASSERT_TRUE(base::PathExists(keychain_path));
+  SecKeychainRef keychain;
+  OSStatus status =
+      SecKeychainOpen(keychain_path.MaybeAsASCII().c_str(), &keychain);

[Ryan Sleevi, 2017/02/14 19:26:17]

.InitializeInto()

[mattm, 2017/02/16 22:06:09]

Done.

+  ASSERT_EQ(errSecSuccess, status);
+  ASSERT_TRUE(keychain);
+  base::ScopedCFTypeRef<SecKeychainRef> scoped_keychain(keychain);
+  test_keychain_search_list->AddKeychain(keychain);
+
+  TrustStoreMac trust_store(kSecPolicyAppleSSL);
+
+  scoped_refptr<ParsedCertificate> a_by_b, b_by_c, b_by_f, c_by_d, c_by_e,
+      f_by_e, d_by_d, e_by_e;
+  ASSERT_TRUE(ReadTestCert("multi-root-A-by-B.pem", &a_by_b));
+  ASSERT_TRUE(ReadTestCert("multi-root-B-by-C.pem", &b_by_c));
+  ASSERT_TRUE(ReadTestCert("multi-root-B-by-F.pem", &b_by_f));
+  ASSERT_TRUE(ReadTestCert("multi-root-C-by-D.pem", &c_by_d));
+  ASSERT_TRUE(ReadTestCert("multi-root-C-by-E.pem", &c_by_e));
+  ASSERT_TRUE(ReadTestCert("multi-root-F-by-E.pem", &f_by_e));
+  ASSERT_TRUE(ReadTestCert("multi-root-D-by-D.pem", &d_by_d));
+  ASSERT_TRUE(ReadTestCert("multi-root-E-by-E.pem", &e_by_e));
+
+  base::ScopedCFTypeRef<CFDataRef> normalized_name_b =
+      TrustStoreMac::GetMacNormalizedIssuer(a_by_b);
+  ASSERT_TRUE(normalized_name_b);
+  base::ScopedCFTypeRef<CFDataRef> normalized_name_c =
+      TrustStoreMac::GetMacNormalizedIssuer(b_by_c);
+  ASSERT_TRUE(normalized_name_c);
+  base::ScopedCFTypeRef<CFDataRef> normalized_name_f =
+      TrustStoreMac::GetMacNormalizedIssuer(b_by_f);
+  ASSERT_TRUE(normalized_name_f);
+  base::ScopedCFTypeRef<CFDataRef> normalized_name_d =
+      TrustStoreMac::GetMacNormalizedIssuer(c_by_d);
+  ASSERT_TRUE(normalized_name_d);
+  base::ScopedCFTypeRef<CFDataRef> normalized_name_e =
+      TrustStoreMac::GetMacNormalizedIssuer(f_by_e);
+  ASSERT_TRUE(normalized_name_e);
+
+  // Test that the matching keychain items are found, even though they aren't
+  // trusted.
+  {
+    base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+        TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+            normalized_name_b.get());
+    EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {b_by_c, b_by_f}));

[Ryan Sleevi, 2017/02/14 19:26:17]

It's not clear to me what this segment of things is testing. Is it testing that
the entries are in the .keychain file? If not, could you clarify?

[mattm, 2017/02/16 22:06:09]

It independently testing that the SecItemCopyMatching part works properly with
some known data.. although the SystemCerts does test that too I think this is
still useful since it can do some limited testing that doesn't depend on what's
in the system root store.

+  }
+
+  {
+    base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+        TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+            normalized_name_c.get());
+    EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {c_by_d, c_by_e}));
+  }
+
+  {
+    base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+        TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+            normalized_name_f.get());
+    EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {f_by_e}));
+  }
+
+  {
+    base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+        TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+            normalized_name_d.get());
+    EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {d_by_d}));
+  }
+
+  {
+    base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+        TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+            normalized_name_e.get());
+    EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {e_by_e}));
+  }
+
+  // None of the certs should return any matching TrustAnchors, since the test
+  // certs in the keychain aren't trusted (unless someone manually added and
+  // trusted the test certs on the machine the test is being run on).
+  for (const auto& cert :
+       {a_by_b, b_by_c, b_by_f, c_by_d, c_by_e, f_by_e, d_by_d, e_by_e}) {
+    TrustAnchors matching_anchors;
+    trust_store.FindTrustAnchorsForCert(cert, &matching_anchors);
+    EXPECT_EQ(0u, matching_anchors.size());
+  }
+}
+
+TEST(TrustStoreMacTest, SystemCerts) {
+  std::string find_certificate_default_search_list_output;
+  ASSERT_TRUE(
+      base::GetAppOutput({"security", "find-certificate", "-a", "-p", "-Z"},
+                         &find_certificate_default_search_list_output));
+  std::string find_certificate_system_roots_output;
+  ASSERT_TRUE(base::GetAppOutput(
+      {"security", "find-certificate", "-a", "-p", "-Z",
+       "/System/Library/Keychains/SystemRootCertificates.keychain"},
+      &find_certificate_system_roots_output));

[Ryan Sleevi, 2017/02/14 19:26:17]

Document a little more what these calls are doing :)

why find-certificate? Why search for the "SHA-1 hash: " string vs the XML?
(Note, I'm not asking you to change the usage, just document :D)

[mattm, 2017/02/16 22:06:09]

done.

(It's trust-settings-export that has XML output, find-certificate is just text.)

+
+  TrustStoreMac trust_store(kSecPolicyAppleX509Basic);
+
+  base::ScopedCFTypeRef<SecPolicyRef> sec_policy(SecPolicyCreateBasicX509());
+  ASSERT_TRUE(sec_policy);
+
+  int true_pos = 0;
+  int true_neg = 0;
+  int false_pos = 0;
+  int false_neg = 0;
+  for (const std::string& hash_and_pem : base::SplitStringUsingSubstr(
+           find_certificate_system_roots_output +
+               find_certificate_default_search_list_output,
+           "SHA-1 hash: ", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
+    std::string::size_type eol_pos = hash_and_pem.find_first_of("\r\n");
+    ASSERT_NE(std::string::npos, eol_pos);
+    std::string hash_text = hash_and_pem.substr(0, eol_pos);
+
+    SCOPED_TRACE(hash_text);
+    // TODO(mattm): The same cert might exist in both lists, could de-dupe
+    // before testing?
+
+    PEMTokenizer pem_tokenizer(hash_and_pem, {kCertificateHeader});
+    ASSERT_TRUE(pem_tokenizer.GetNext());
+    std::string cert_der(pem_tokenizer.data());
+    ASSERT_FALSE(pem_tokenizer.GetNext());
+
+    CertErrors errors;
+    // Note: don't actually need to make a ParsedCertificate here, just need
+    // the der bytes. But parsing it here ensures the test can skip any certs
+    // that won't be returned due to parsing failures inside TrustStoreMac.
+    // The parsing options set here need to match the ones used in
+    // trust_store_mac.cc.
+    ParseCertificateOptions options;
+    options.allow_invalid_serial_numbers = true;
+    scoped_refptr<ParsedCertificate> cert =
+        ParsedCertificate::Create(cert_der, options, &errors);
+    if (!cert) {
+      LOG(WARNING) << "ParseCertificate::Create " << hash_text << " failed:\n"
+                   << errors.ToDebugString();
+      continue;
+    }
+
+    X509Certificate::OSCertHandle cert_handle =
+        X509Certificate::CreateOSCertHandleFromBytes(
+            cert->der_cert().AsStringPiece().data(), cert->der_cert().Length());
+    if (!cert_handle) {
+      ADD_FAILURE() << "CreateOSCertHandleFromBytes " << hash_text;
+      continue;
+    }
+    base::ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle);

[Ryan Sleevi, 2017/02/14 19:26:17]

Why not just force this scoper-wrapper around line 251-252?

[mattm, 2017/02/16 22:06:09]

Done.

+    base::ScopedCFTypeRef<CFDataRef> mac_normalized_subject(
+        SecCertificateCopyNormalizedSubjectContent(cert_handle, nullptr));
+    if (!mac_normalized_subject) {
+      ADD_FAILURE() << "SecCertificateCopyNormalizedSubjectContent "
+                    << hash_text;
+      continue;
+    }
+
+    // Check if this cert is considered a trust anchor by TrustStoreMac.
+    TrustAnchors trust_anchors;
+    trust_store.FindTrustAnchorsByMacNormalizedSubject(mac_normalized_subject,
+                                                       &trust_anchors);
+    bool is_trust_anchor = false;
+    for (const auto& anchor : trust_anchors) {
+      ASSERT_TRUE(anchor->cert());
+      if (anchor->cert()->der_cert() == cert->der_cert())
+        is_trust_anchor = true;
+    }
+
+    // Check if this cert is considered a trust anchor by the OS.
+    base::ScopedCFTypeRef<SecTrustRef> trust;
+    ASSERT_EQ(noErr, SecTrustCreateWithCertificates(cert_handle, sec_policy,
+                                                    trust.InitializeInto()));
+    ASSERT_EQ(noErr,
+              SecTrustSetOptions(trust, kSecTrustOptionLeafIsCA |
+                                            kSecTrustOptionAllowExpiredRoot));
+
+    SecTrustResultType trust_result;
+    ASSERT_EQ(noErr, SecTrustEvaluate(trust, &trust_result));
+    bool expected_trust_anchor =
+        ((trust_result == kSecTrustResultProceed) ||
+         (trust_result == kSecTrustResultUnspecified)) &&
+        (SecTrustGetCertificateCount(trust) == 1);
+
+    EXPECT_EQ(expected_trust_anchor, is_trust_anchor);
+
+    if (expected_trust_anchor) {
+      if (is_trust_anchor) {
+        true_pos++;
+      } else {
+        false_neg++;
+      }
+    } else {
+      if (is_trust_anchor) {
+        false_pos++;
+      } else {
+        true_neg++;
+      }
+    }
+  }
+
+  LOG(INFO) << "true_pos: " << true_pos;
+  LOG(INFO) << "true_neg: " << true_neg;
+  LOG(INFO) << "false_pos: " << false_pos;
+  LOG(INFO) << "false_neg: " << false_neg;
+}
+
+}  // namespace net