PathBuilderKeyRolloverTest.TestMultipleRootMatchesOnlyOneWorks shouldn't depend on trust store orde…

net/cert/internal/path_builder_unittest.cc

 // 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/path_builder.h"
 
 #include "base/base_paths.h"
 #include "base/cancelable_callback.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ skipping 568 matching lines @@
     EXPECT_EQ(newroot_, result.paths[0]->path[2]);
   }
 }
 
 // Tests that multiple trust root matches on a single path will be considered.
 // Both roots have the same subject but different keys. Only one of them will
 // verify.
 TEST_F(PathBuilderKeyRolloverTest, TestMultipleRootMatchesOnlyOneWorks) {
   // Both newroot and oldroot are trusted.
   TrustStore trust_store;
-  // Note: The test assumes newroot will be tried before oldroot.
-  // Currently this depends on the order the roots are added.
   trust_store.AddTrustedCertificate(newroot_);
   trust_store.AddTrustedCertificate(oldroot_);
 
   // Only oldintermediate is supplied, so the path with newroot should fail,
   // oldroot should succeed.
   CertIssuerSourceStatic sync_certs;
   sync_certs.AddCert(oldintermediate_);
 
   CertPathBuilder::Result result;
   CertPathBuilder path_builder(target_, &trust_store, &signature_policy_, time_,
                                &result);
   path_builder.AddCertIssuerSource(&sync_certs);
 
   EXPECT_EQ(CompletionStatus::SYNC, RunPathBuilder(&path_builder));
 
   EXPECT_EQ(OK, result.error());
-  ASSERT_EQ(2U, result.paths.size());
+  // There may be one or two paths attempted depending if the path builder tried
+  // using newroot first.
+  // TODO(mattm): Once TrustStore is an interface, this could be fixed with a
+  // mock version of TrustStore that returns roots in a deterministic order.

[eroman, 2016/07/25 22:13:44]

As a long term goal I think we want the path building to yield the same result
on each platform.
That may mean using an std::map or otherwise using a path priority scheme.

+  ASSERT_LE(1U, result.paths.size());
+  ASSERT_GE(2U, result.paths.size());
 
-  // Path builder will first attempt: target <- oldintermediate <- newroot
-  // but it will fail since oldintermediate is signed by oldroot.
-  EXPECT_EQ(ERR_CERT_AUTHORITY_INVALID, result.paths[0]->error);
-  ASSERT_EQ(3U, result.paths[0]->path.size());
-  EXPECT_EQ(target_, result.paths[0]->path[0]);
-  EXPECT_EQ(oldintermediate_, result.paths[0]->path[1]);
-  EXPECT_EQ(newroot_, result.paths[0]->path[2]);
+  if (result.paths.size() == 2) {
+    // Path builder may first attempt: target <- oldintermediate <- newroot
+    // but it will fail since oldintermediate is signed by oldroot.
+    EXPECT_EQ(ERR_CERT_AUTHORITY_INVALID, result.paths[0]->error);
+    ASSERT_EQ(3U, result.paths[0]->path.size());
+    EXPECT_EQ(target_, result.paths[0]->path[0]);
+    EXPECT_EQ(oldintermediate_, result.paths[0]->path[1]);
+    EXPECT_EQ(newroot_, result.paths[0]->path[2]);
+  }
 
   // Path builder will next attempt:
   // target <- old intermediate <- oldroot
   // which should succeed.
-  EXPECT_EQ(OK, result.paths[1]->error);
-  ASSERT_EQ(3U, result.paths[1]->path.size());
-  EXPECT_EQ(target_, result.paths[1]->path[0]);
-  EXPECT_EQ(oldintermediate_, result.paths[1]->path[1]);
-  EXPECT_EQ(oldroot_, result.paths[1]->path[2]);
+  EXPECT_EQ(OK, result.paths[result.best_result_index]->error);
+  ASSERT_EQ(3U, result.paths[result.best_result_index]->path.size());
+  EXPECT_EQ(target_, result.paths[result.best_result_index]->path[0]);
+  EXPECT_EQ(oldintermediate_, result.paths[result.best_result_index]->path[1]);
+  EXPECT_EQ(oldroot_, result.paths[result.best_result_index]->path[2]);
 }
 
 // Tests that the path builder doesn't build longer than necessary paths.
 TEST_F(PathBuilderKeyRolloverTest, TestRolloverLongChain) {
   // Only oldroot is trusted.
   TrustStore trust_store;
   trust_store.AddTrustedCertificate(oldroot_);
 
   // New intermediate and new root are provided synchronously.
   CertIssuerSourceStatic sync_certs;
@@ skipping 464 matching lines @@
   EXPECT_EQ(OK, result.paths[1]->error);
   ASSERT_EQ(3U, result.paths[1]->path.size());
   EXPECT_EQ(target_, result.paths[1]->path[0]);
   EXPECT_EQ(newintermediate_, result.paths[1]->path[1]);
   EXPECT_EQ(newroot_, result.paths[1]->path[2]);
 }
 
 }  // namespace
 
 }  // namespace net