Remove certificates from Channel ID

net/extras/sqlite/sqlite_channel_id_store_unittest.cc

Index: net/extras/sqlite/sqlite_channel_id_store_unittest.cc
diff --git a/net/extras/sqlite/sqlite_channel_id_store_unittest.cc b/net/extras/sqlite/sqlite_channel_id_store_unittest.cc
index 17024f485cbc317500ae23e92a68de9351f80f37..51a8ecfe3193b54900d32bc4eaffd52ffe47a4b1 100644
--- a/net/extras/sqlite/sqlite_channel_id_store_unittest.cc
+++ b/net/extras/sqlite/sqlite_channel_id_store_unittest.cc
@@ -11,6 +11,7 @@
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "net/base/test_data_directory.h"
+#include "net/cert/asn1_util.h"
 #include "net/extras/sqlite/sqlite_channel_id_store.h"
 #include "net/ssl/ssl_client_cert_type.h"
 #include "net/test/cert_test_util.h"
@@ -85,7 +86,6 @@ class SQLiteChannelIDStoreTest : public testing::Test {
     store_->AddChannelID(
         DefaultChannelIDStore::ChannelID("google.com",
                                          base::Time::FromInternalValue(1),
-                                         base::Time::FromInternalValue(2),
                                          "a",
                                          "b"));
   }
@@ -100,7 +100,6 @@ TEST_F(SQLiteChannelIDStoreTest, TestPersistence) {
   store_->AddChannelID(
       DefaultChannelIDStore::ChannelID("foo.com",
                                        base::Time::FromInternalValue(3),
-                                       base::Time::FromInternalValue(4),
                                        "c",
                                        "d"));
 
@@ -129,16 +128,14 @@ TEST_F(SQLiteChannelIDStoreTest, TestPersistence) {
   }
   ASSERT_EQ("google.com", goog_channel_id->server_identifier());
   ASSERT_STREQ("a", goog_channel_id->private_key().c_str());
-  ASSERT_STREQ("b", goog_channel_id->cert().c_str());
+  ASSERT_STREQ("b", goog_channel_id->public_key().c_str());
   ASSERT_EQ(1, goog_channel_id->creation_time().ToInternalValue());
-  ASSERT_EQ(2, goog_channel_id->expiration_time().ToInternalValue());
   ASSERT_EQ("foo.com", foo_channel_id->server_identifier());
   ASSERT_STREQ("c", foo_channel_id->private_key().c_str());
-  ASSERT_STREQ("d", foo_channel_id->cert().c_str());
+  ASSERT_STREQ("d", foo_channel_id->public_key().c_str());
   ASSERT_EQ(3, foo_channel_id->creation_time().ToInternalValue());
-  ASSERT_EQ(4, foo_channel_id->expiration_time().ToInternalValue());
 
-  // Now delete the cert and check persistence again.
+  // Now delete the keypair and check persistence again.
   store_->DeleteChannelID(*channel_ids[0]);
   store_->DeleteChannelID(*channel_ids[1]);
   store_ = NULL;
@@ -149,7 +146,7 @@ TEST_F(SQLiteChannelIDStoreTest, TestPersistence) {
       new SQLiteChannelIDStore(temp_dir_.path().Append(kTestChannelIDFilename),
                                base::MessageLoopProxy::current());
 
-  // Reload and check if the cert has been removed.
+  // Reload and check if the keypair has been removed.
   Load(&channel_ids);
   ASSERT_EQ(0U, channel_ids.size());
   // Close the store.
@@ -163,7 +160,6 @@ TEST_F(SQLiteChannelIDStoreTest, TestDeleteAll) {
   store_->AddChannelID(
       DefaultChannelIDStore::ChannelID("foo.com",
                                        base::Time::FromInternalValue(3),
-                                       base::Time::FromInternalValue(4),
                                        "c",
                                        "d"));
 
@@ -267,7 +263,7 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV1) {
       sql::Statement smt(db.GetUniqueStatement(
           "SELECT value FROM meta WHERE key = \"version\""));
       ASSERT_TRUE(smt.Step());
-      EXPECT_EQ(4, smt.ColumnInt(0));
+      EXPECT_EQ(5, smt.ColumnInt(0));
       EXPECT_FALSE(smt.Step());
     }
   }
@@ -307,6 +303,7 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV2) {
     add_smt.BindInt64(3, 64);
     ASSERT_TRUE(add_smt.Run());
 
+    // Malformed certs will be ignored and not migrated.
     ASSERT_TRUE(db.Execute(
         "INSERT INTO \"origin_bound_certs\" VALUES("
         "'foo.com',X'AA',X'BB',64);"));
@@ -324,18 +321,16 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV2) {
 
     // Load the database and ensure the certs can be read.
     Load(&channel_ids);
-    ASSERT_EQ(2U, channel_ids.size());
+    ASSERT_EQ(1U, channel_ids.size());
 
     ASSERT_EQ("google.com", channel_ids[0]->server_identifier());
-    ASSERT_EQ(GetTestCertExpirationTime(), channel_ids[0]->expiration_time());
+    ASSERT_EQ(GetTestCertCreationTime(), channel_ids[0]->creation_time());
     ASSERT_EQ(key_data, channel_ids[0]->private_key());
-    ASSERT_EQ(cert_data, channel_ids[0]->cert());
 
-    ASSERT_EQ("foo.com", channel_ids[1]->server_identifier());
-    // Undecodable cert, expiration time will be uninitialized.
-    ASSERT_EQ(base::Time(), channel_ids[1]->expiration_time());
-    ASSERT_STREQ("\xaa", channel_ids[1]->private_key().c_str());
-    ASSERT_STREQ("\xbb", channel_ids[1]->cert().c_str());
+    base::StringPiece spki_from_cert;
+    ASSERT_TRUE(asn1::ExtractSPKIFromDERCert(cert_data, &spki_from_cert));
+    std::string spki_string(spki_from_cert.data(), spki_from_cert.size());
+    EXPECT_EQ(spki_string, channel_ids[0]->public_key());
 
     store_ = NULL;
     // Make sure we wait until the destructor has run.
@@ -348,7 +343,7 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV2) {
       sql::Statement smt(db.GetUniqueStatement(
           "SELECT value FROM meta WHERE key = \"version\""));
       ASSERT_TRUE(smt.Step());
-      EXPECT_EQ(4, smt.ColumnInt(0));
+      EXPECT_EQ(5, smt.ColumnInt(0));
       EXPECT_FALSE(smt.Step());
     }
   }
@@ -390,6 +385,7 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV3) {
     add_smt.BindInt64(4, 1000);
     ASSERT_TRUE(add_smt.Run());
 
+    // Malformed certs will be ignored and not migrated.
     ASSERT_TRUE(db.Execute(
         "INSERT INTO \"origin_bound_certs\" VALUES("
         "'foo.com',X'AA',X'BB',64,2000);"));
@@ -407,20 +403,16 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV3) {
 
     // Load the database and ensure the certs can be read.
     Load(&channel_ids);
-    ASSERT_EQ(2U, channel_ids.size());
+    ASSERT_EQ(1U, channel_ids.size());
 
     ASSERT_EQ("google.com", channel_ids[0]->server_identifier());
-    ASSERT_EQ(1000, channel_ids[0]->expiration_time().ToInternalValue());
     ASSERT_EQ(GetTestCertCreationTime(), channel_ids[0]->creation_time());
     ASSERT_EQ(key_data, channel_ids[0]->private_key());
-    ASSERT_EQ(cert_data, channel_ids[0]->cert());
 
-    ASSERT_EQ("foo.com", channel_ids[1]->server_identifier());
-    ASSERT_EQ(2000, channel_ids[1]->expiration_time().ToInternalValue());
-    // Undecodable cert, creation time will be uninitialized.
-    ASSERT_EQ(base::Time(), channel_ids[1]->creation_time());
-    ASSERT_STREQ("\xaa", channel_ids[1]->private_key().c_str());
-    ASSERT_STREQ("\xbb", channel_ids[1]->cert().c_str());
+    base::StringPiece spki_from_cert;
+    ASSERT_TRUE(asn1::ExtractSPKIFromDERCert(cert_data, &spki_from_cert));
+    std::string spki_string(spki_from_cert.data(), spki_from_cert.size());
+    EXPECT_EQ(spki_string, channel_ids[0]->public_key());
 
     store_ = NULL;
     // Make sure we wait until the destructor has run.
@@ -433,23 +425,23 @@ TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV3) {
       sql::Statement smt(db.GetUniqueStatement(
           "SELECT value FROM meta WHERE key = \"version\""));
       ASSERT_TRUE(smt.Step());
-      EXPECT_EQ(4, smt.ColumnInt(0));
+      EXPECT_EQ(5, smt.ColumnInt(0));
       EXPECT_FALSE(smt.Step());
     }
   }
 }
 
-TEST_F(SQLiteChannelIDStoreTest, TestRSADiscarded) {
+TEST_F(SQLiteChannelIDStoreTest, TestUpgradeV4) {
   // Reset the store.  We'll be using a different database for this test.
   store_ = NULL;
 
-  base::FilePath v4_db_path(temp_dir_.path().AppendASCII("v4dbrsa"));
+  base::FilePath v4_db_path(temp_dir_.path().AppendASCII("v4db"));
 
   std::string key_data;
   std::string cert_data;
   ReadTestKeyAndCert(&key_data, &cert_data);
 
-  // Create a version 4 database with a mix of RSA and ECDSA certs.
+  // Create a version 3 database.

[Ryan Sleevi, 2015/04/15 22:50:02]

typo? You say UpgradeV4, but then say v3 here

[nharper, 2015/04/25 02:59:19]

Typo. Fixed.

   {
     sql::Connection db;
     ASSERT_TRUE(db.Open(v4_db_path));
@@ -467,17 +459,17 @@ TEST_F(SQLiteChannelIDStoreTest, TestRSADiscarded) {
         "creation_time INTEGER);"));
 
     sql::Statement add_smt(db.GetUniqueStatement(
-        "INSERT INTO origin_bound_certs "
-        "(origin, private_key, cert, cert_type, expiration_time, creation_time)"
-        " VALUES (?,?,?,?,?,?)"));
+        "INSERT INTO origin_bound_certs (origin, private_key, cert, cert_type, "
+        "expiration_time, creation_time) VALUES (?,?,?,?,?,?)"));
     add_smt.BindString(0, "google.com");
     add_smt.BindBlob(1, key_data.data(), key_data.size());
     add_smt.BindBlob(2, cert_data.data(), cert_data.size());
     add_smt.BindInt64(3, 64);
-    add_smt.BindInt64(4, GetTestCertExpirationTime().ToInternalValue());
-    add_smt.BindInt64(5, base::Time::Now().ToInternalValue());
+    add_smt.BindInt64(4, 1000);

[Ryan Sleevi, 2015/04/15 22:50:02]

Does this mean we migrate expired channel IDs? Is that expected?

(I don't know - probably; Matt would probably know)

[mattm, 2015/04/15 23:11:46]

Seems right to me, since we've been ignoring the expiration for a long time.

[nharper, 2015/04/25 02:59:19]

Yes, it means we migrate keys that have expired certs. This is fine (and
intended), because we never checked the expiration time.

+    add_smt.BindInt64(5, GetTestCertCreationTime().ToInternalValue());
     ASSERT_TRUE(add_smt.Run());
 
+    // Add an RSA cert to the db. This cert should be ignored in the migration.
     add_smt.Clear();
     add_smt.Assign(db.GetUniqueStatement(
         "INSERT INTO origin_bound_certs "
@@ -490,25 +482,51 @@ TEST_F(SQLiteChannelIDStoreTest, TestRSADiscarded) {
     add_smt.BindInt64(4, GetTestCertExpirationTime().ToInternalValue());
     add_smt.BindInt64(5, base::Time::Now().ToInternalValue());
     ASSERT_TRUE(add_smt.Run());
+
+    // Malformed certs will be ignored and not migrated.
+    ASSERT_TRUE(db.Execute(
+        "INSERT INTO \"origin_bound_certs\" VALUES("
+        "'bar.com',X'AA',X'BB',64,2000,3000);"));
   }
 
-  ScopedVector<DefaultChannelIDStore::ChannelID> channel_ids;
-  store_ =
-      new SQLiteChannelIDStore(v4_db_path, base::MessageLoopProxy::current());
+  // Load and test the DB contents twice.  First time ensures that we can use
+  // the updated values immediately.  Second time ensures that the updated
+  // values are saved and read correctly on next load.
+  for (int i = 0; i < 2; ++i) {
+    SCOPED_TRACE(i);
 
-  // Load the database and ensure the certs can be read.
-  Load(&channel_ids);
-  // Only the ECDSA cert (for google.com) is read, the RSA one is discarded.
-  ASSERT_EQ(1U, channel_ids.size());
+    ScopedVector<DefaultChannelIDStore::ChannelID> channel_ids;
+    store_ =
+        new SQLiteChannelIDStore(v4_db_path, base::MessageLoopProxy::current());
 
-  ASSERT_EQ("google.com", channel_ids[0]->server_identifier());
-  ASSERT_EQ(GetTestCertExpirationTime(), channel_ids[0]->expiration_time());
-  ASSERT_EQ(key_data, channel_ids[0]->private_key());
-  ASSERT_EQ(cert_data, channel_ids[0]->cert());
+    // Load the database and ensure the certs can be read.
+    Load(&channel_ids);
+    ASSERT_EQ(1U, channel_ids.size());
 
-  store_ = NULL;
-  // Make sure we wait until the destructor has run.
-  base::RunLoop().RunUntilIdle();
+    ASSERT_EQ("google.com", channel_ids[0]->server_identifier());
+    ASSERT_EQ(GetTestCertCreationTime(), channel_ids[0]->creation_time());
+    ASSERT_EQ(key_data, channel_ids[0]->private_key());
+
+    base::StringPiece spki_from_cert;
+    ASSERT_TRUE(asn1::ExtractSPKIFromDERCert(cert_data, &spki_from_cert));
+    std::string spki_string(spki_from_cert.data(), spki_from_cert.size());
+    EXPECT_EQ(spki_string, channel_ids[0]->public_key());
+
+    store_ = NULL;
+    // Make sure we wait until the destructor has run.
+    base::RunLoop().RunUntilIdle();
+
+    // Verify the database version is updated.
+    {
+      sql::Connection db;
+      ASSERT_TRUE(db.Open(v4_db_path));
+      sql::Statement smt(db.GetUniqueStatement(
+          "SELECT value FROM meta WHERE key = \"version\""));
+      ASSERT_TRUE(smt.Step());
+      EXPECT_EQ(5, smt.ColumnInt(0));
+      EXPECT_FALSE(smt.Step());
+    }
+  }
 }
 
 }  // namespace net