Remove certificates from Channel ID

net/extras/sqlite/sqlite_channel_id_store.cc

 // Copyright 2014 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/extras/sqlite/sqlite_channel_id_store.h"
 
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/metrics/histogram.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cookies/cookie_util.h"
 #include "net/ssl/ssl_client_cert_type.h"
 #include "sql/error_delegate_util.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 #include "url/gurl.h"
 
 namespace {
 
 // Version number of the database.
-const int kCurrentVersionNumber = 4;
+const int kCurrentVersionNumber = 5;
 const int kCompatibleVersionNumber = 1;
 
-// Initializes the certs table, returning true on success.
-bool InitTable(sql::Connection* db) {
-  // The table is named "origin_bound_certs" for backwards compatability before
-  // we renamed this class to SQLiteChannelIDStore.  Likewise, the primary
-  // key is "origin", but now can be other things like a plain domain.
-  if (!db->DoesTableExist("origin_bound_certs")) {
-    if (!db->Execute(
-            "CREATE TABLE origin_bound_certs ("
-            "origin TEXT NOT NULL UNIQUE PRIMARY KEY,"
-            "private_key BLOB NOT NULL,"
-            "cert BLOB NOT NULL,"
-            "cert_type INTEGER,"
-            "expiration_time INTEGER,"
-            "creation_time INTEGER)")) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 }  // namespace
 
 namespace net {
 
 // This class is designed to be shared between any calling threads and the
 // background task runner. It batches operations and commits them on a timer.
 class SQLiteChannelIDStore::Backend
     : public base::RefCountedThreadSafe<SQLiteChannelIDStore::Backend> {
  public:
   Backend(
@@ skipping 135 matching lines @@
                  base::Unretained(this)));
 
   if (!db_->Open(path_)) {
     NOTREACHED() << "Unable to open cert DB.";
     if (corruption_detected_)
       KillDatabase();
     db_.reset();
     return;
   }
 
-  if (!EnsureDatabaseVersion() || !InitTable(db_.get())) {
+  if (!EnsureDatabaseVersion()) {
     NOTREACHED() << "Unable to open cert DB.";
     if (corruption_detected_)
       KillDatabase();
     meta_table_.Reset();
     db_.reset();
     return;
   }
 
   db_->Preload();
 
   // Slurp all the certs into the out-vector.
   sql::Statement smt(db_->GetUniqueStatement(
-      "SELECT origin, private_key, cert, cert_type, expiration_time, "
-      "creation_time FROM origin_bound_certs"));
+      "SELECT  host, private_key, public_key, creation_time FROM channel_id"));
   if (!smt.is_valid()) {
     if (corruption_detected_)
       KillDatabase();
     meta_table_.Reset();
     db_.reset();
     return;
   }
 
   while (smt.Step()) {
-    SSLClientCertType type = static_cast<SSLClientCertType>(smt.ColumnInt(3));
-    if (type != CLIENT_CERT_ECDSA_SIGN)
-      continue;
-    std::string private_key_from_db, cert_from_db;
+    std::string private_key_from_db, public_key_from_db;
     smt.ColumnBlobAsString(1, &private_key_from_db);
-    smt.ColumnBlobAsString(2, &cert_from_db);
+    smt.ColumnBlobAsString(2, &public_key_from_db);
     scoped_ptr<DefaultChannelIDStore::ChannelID> channel_id(
         new DefaultChannelIDStore::ChannelID(
-            smt.ColumnString(0),  // origin
-            base::Time::FromInternalValue(smt.ColumnInt64(5)),
-            base::Time::FromInternalValue(smt.ColumnInt64(4)),
-            private_key_from_db,
-            cert_from_db));
+            smt.ColumnString(0),  // host
+            base::Time::FromInternalValue(smt.ColumnInt64(3)),
+            private_key_from_db, public_key_from_db));
     channel_ids->push_back(channel_id.release());
   }
 
   UMA_HISTOGRAM_COUNTS_10000(
       "DomainBoundCerts.DBLoadedCount",
       static_cast<base::HistogramBase::Sample>(channel_ids->size()));
   base::TimeDelta load_time = base::TimeTicks::Now() - start;
   UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.DBLoadTime",
                              load_time,
                              base::TimeDelta::FromMilliseconds(1),
                              base::TimeDelta::FromMinutes(1),
                              50);
   DVLOG(1) << "loaded " << channel_ids->size() << " in "
            << load_time.InMilliseconds() << " ms";
 }
 
 bool SQLiteChannelIDStore::Backend::EnsureDatabaseVersion() {
   // Version check.
   if (!meta_table_.Init(
           db_.get(), kCurrentVersionNumber, kCompatibleVersionNumber)) {
     return false;
   }
 
   if (meta_table_.GetCompatibleVersionNumber() > kCurrentVersionNumber) {
     LOG(WARNING) << "Server bound cert database is too new.";
     return false;
   }
 
   int cur_version = meta_table_.GetVersionNumber();
-  if (cur_version == 1) {
-    sql::Transaction transaction(db_.get());
-    if (!transaction.Begin())
-      return false;
+
+  sql::Transaction transaction(db_.get());
+  if (!transaction.Begin())
+    return false;
+
+  // Create new table if it doesn't already exist
+  if (!db_->DoesTableExist("channel_id")) {
     if (!db_->Execute(
-            "ALTER TABLE origin_bound_certs ADD COLUMN cert_type "
-            "INTEGER")) {
-      LOG(WARNING) << "Unable to update server bound cert database to "
-                   << "version 2.";
+            "CREATE TABLE channel_id ("
+            "host TEXT NOT NULL UNIQUE PRIMARY KEY,"
+            "private_key BLOB NOT NULL,"
+            "public_key BLOB NOT NULL,"
+            "creation_time INTEGER)")) {
       return false;
     }
-    // All certs in version 1 database are rsa_sign, which are unsupported.
-    // Just discard them all.
-    if (!db_->Execute("DELETE from origin_bound_certs")) {
-      LOG(WARNING) << "Unable to update server bound cert database to "
-                   << "version 2.";
-      return false;
-    }
-    ++cur_version;
-    meta_table_.SetVersionNumber(cur_version);
-    meta_table_.SetCompatibleVersionNumber(
-        std::min(cur_version, kCompatibleVersionNumber));
-    transaction.Commit();
   }
 
-  if (cur_version <= 3) {
-    sql::Transaction transaction(db_.get());
-    if (!transaction.Begin())
-      return false;
-
-    if (cur_version == 2) {
-      if (!db_->Execute(
-              "ALTER TABLE origin_bound_certs ADD COLUMN "
-              "expiration_time INTEGER")) {
-        LOG(WARNING) << "Unable to update server bound cert database to "
-                     << "version 4.";
-        return false;
-      }
-    }
-
-    if (!db_->Execute(
-            "ALTER TABLE origin_bound_certs ADD COLUMN "
-            "creation_time INTEGER")) {
+  // Migrate from previous versions to new version if possible
+  if (cur_version >= 2 && cur_version <= 4) {
+    sql::Statement statement(db_->GetUniqueStatement(
+        "SELECT origin, cert, private_key, cert_type FROM origin_bound_certs"));
+    sql::Statement insert_statement(
+        db_->GetUniqueStatement("INSERT INTO channel_id VALUES (?, ?, ?, ?)"));

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

Recommendation: Make the column names explicit

INSERT INTO channel_id (host, private_key, public_key, creation_time) VALUES (?,
?, ?, ?)

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

Done.

+    if (!statement.is_valid() || !insert_statement.is_valid()) {
       LOG(WARNING) << "Unable to update server bound cert database to "
-                   << "version 4.";
+                   << "version 5.";
       return false;
     }
 
-    sql::Statement statement(
-        db_->GetUniqueStatement("SELECT origin, cert FROM origin_bound_certs"));
-    sql::Statement update_expires_statement(db_->GetUniqueStatement(
-        "UPDATE origin_bound_certs SET expiration_time = ? WHERE origin = ?"));
-    sql::Statement update_creation_statement(db_->GetUniqueStatement(
-        "UPDATE origin_bound_certs SET creation_time = ? WHERE origin = ?"));
-    if (!statement.is_valid() || !update_expires_statement.is_valid() ||
-        !update_creation_statement.is_valid()) {
-      LOG(WARNING) << "Unable to update server bound cert database to "
-                   << "version 4.";
-      return false;
-    }
-
     while (statement.Step()) {
       std::string origin = statement.ColumnString(0);
       std::string cert_from_db;
       statement.ColumnBlobAsString(1, &cert_from_db);
+      std::string private_key;
+      statement.ColumnBlobAsString(2, &private_key);
+      if (statement.ColumnInt64(3) != CLIENT_CERT_ECDSA_SIGN) {
+        continue;
+      }

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

No braces for one-line conditionals, right?

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

Done.

       // Parse the cert and extract the real value and then update the DB.
       scoped_refptr<X509Certificate> cert(X509Certificate::CreateFromBytes(
           cert_from_db.data(), static_cast<int>(cert_from_db.size())));
       if (cert.get()) {
-        if (cur_version == 2) {
-          update_expires_statement.Reset(true);
-          update_expires_statement.BindInt64(
-              0, cert->valid_expiry().ToInternalValue());
-          update_expires_statement.BindString(1, origin);
-          if (!update_expires_statement.Run()) {
-            LOG(WARNING) << "Unable to update server bound cert database to "
-                         << "version 4.";
-            return false;
-          }
+        insert_statement.Reset(true);
+        insert_statement.BindString(0, origin);
+        insert_statement.BindBlob(1, private_key.data(),
+                                  static_cast<int>(private_key.size()));
+        base::StringPiece spki;
+        if (!asn1::ExtractSPKIFromDERCert(cert_from_db, &spki)) {
+          LOG(WARNING) << "Unable to extract SPKI from cert when migrating "
+                          "channel id database to version 5.";
+          return false;

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

Is this correct? Should you skip that channel ID instead? Why or why not?

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

The thought here is that the db should only contain valid certs - if we have a
cert that we can't extract a spki from, then something has probably gone wrong
with the db and we should blow it away (that's what this currently does). Before
we try to extract the spki from the der cert, we've already constructed an
X500Certificate from the der cert, so this call should succeed.

On the other hand, when we check the cert key type and verify that it's ecdsa,
if that check fails we just skip it, even though there should never be a
non-ecdsa key/cert in the db at v>=2.

I'd argue that if we can't extract the spki (yet were able to parse the cert),
then something has gone wrong and we should fail harder than if we have a
cert/key of the wrong type.

         }
-
-        update_creation_statement.Reset(true);
-        update_creation_statement.BindInt64(
-            0, cert->valid_start().ToInternalValue());
-        update_creation_statement.BindString(1, origin);
-        if (!update_creation_statement.Run()) {
-          LOG(WARNING) << "Unable to update server bound cert database to "
-                       << "version 4.";
+        insert_statement.BindBlob(2, spki.data(), spki.size());

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

In the other cases, you cast to an int. Why do you not need to? the size_type
for BasicStringPiece (what influences base::StringPiece) is size_t

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

I don't know why other places cast to an int - presumably for some platform
other than linux. It works fine without the cast on linux. It should be
consistent - I can either remove all the casts (and find out why they were there
with the trybots if they were actually needed), or add the cast here and go with
what's in the rest of the file (and assume that whoever added the casts had a
good reason).

+        insert_statement.BindInt64(3, cert->valid_start().ToInternalValue());
+        if (!insert_statement.Run()) {
+          LOG(WARNING) << "Unable to update channel id database to "
+                       << "version 5.";
           return false;
         }
       } else {
         // If there's a cert we can't parse, just leave it.  It'll get replaced
         // with a new one if we ever try to use it.
         LOG(WARNING) << "Error parsing cert for database upgrade for origin "
                      << statement.ColumnString(0);
       }
     }
-
-    cur_version = 4;
-    meta_table_.SetVersionNumber(cur_version);
-    meta_table_.SetCompatibleVersionNumber(
-        std::min(cur_version, kCompatibleVersionNumber));
-    transaction.Commit();
   }
 
+  if (cur_version < kCurrentVersionNumber) {
+    sql::Statement statement(
+        db_->GetUniqueStatement("DROP TABLE origin_bound_certs"));
+    if (!statement.Run()) {
+      LOG(WARNING) << "Error dropping old origin_bound_certs table";
+      return false;
+    }
+    meta_table_.SetVersionNumber(kCurrentVersionNumber);
+    meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);

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

Just confirming - these are part of part of the transaction as well, right?

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

That's my intention and that's my understanding of how the sql code works -
meta_table_ is operating on the same sql::Connection as the rest of the
statements here.

+  }
+  transaction.Commit();
+
   // Put future migration cases here.
 
-  // When the version is too old, we just try to continue anyway, there should
-  // not be a released product that makes a database too old for us to handle.
-  LOG_IF(WARNING, cur_version < kCurrentVersionNumber)

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

Why is this deleted?

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

This check never would have fired earlier - it was added to be defensive of
someone adding a new version and breaking things (like what I'm doing). The old
migration code here upgraded one version at a time, and updated cur_version to
match what it had just been upgraded to. Since there is upgrade logic for every
version number 1 to 4, this check never would have tripped.

In my new code, cur_version has different semantics. Instead of upgrading one
version at a time, I upgrade from each supported previous version (1 thru 4) to
the new version (5) in a single pass, and I also don't update cur_version -
cur_version keeps the version number that the db had before the migration
started. With this new meaning, I have an if for this conditional (see line 319
on the right) that finishes handling part of the migration. This log statement
doesn't make any sense in the new code, so it was removed.

-      << "Server bound cert database version " << cur_version
-      << " is too old to handle.";
-
   return true;
 }
 
 void SQLiteChannelIDStore::Backend::DatabaseErrorCallback(
     int error,
     sql::Statement* stmt) {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
   if (!sql::IsErrorCatastrophic(error))
     return;
@@ skipping 86 matching lines @@
     pending_.swap(ops);
     num_pending_ = 0;
   }
 
   // Maybe an old timer fired or we are already Close()'ed.
   if (!db_.get() || ops.empty())
     return;
 
   sql::Statement add_statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
-      "INSERT INTO origin_bound_certs (origin, private_key, cert, cert_type, "
-      "expiration_time, creation_time) VALUES (?,?,?,?,?,?)"));
+      "INSERT INTO channel_id (host, private_key, public_key, "
+      "creation_time) VALUES (?,?,?,?)"));
   if (!add_statement.is_valid())
     return;
 
   sql::Statement del_statement(db_->GetCachedStatement(
-      SQL_FROM_HERE, "DELETE FROM origin_bound_certs WHERE origin=?"));
+      SQL_FROM_HERE, "DELETE FROM channel_id WHERE host=?"));
   if (!del_statement.is_valid())
     return;
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return;
 
   for (PendingOperationsList::iterator it = ops.begin(); it != ops.end();
        ++it) {
     // Free the certs as we commit them to the database.
     scoped_ptr<PendingOperation> po(*it);
     switch (po->op()) {
       case PendingOperation::CHANNEL_ID_ADD: {
         add_statement.Reset(true);
         add_statement.BindString(0, po->channel_id().server_identifier());
         const std::string& private_key = po->channel_id().private_key();
         add_statement.BindBlob(
             1, private_key.data(), static_cast<int>(private_key.size()));
-        const std::string& cert = po->channel_id().cert();
-        add_statement.BindBlob(2, cert.data(), static_cast<int>(cert.size()));
-        add_statement.BindInt(3, CLIENT_CERT_ECDSA_SIGN);
+        const std::string& public_key = po->channel_id().public_key();
+        add_statement.BindBlob(2, public_key.data(),
+                               static_cast<int>(public_key.size()));
         add_statement.BindInt64(
-            4, po->channel_id().expiration_time().ToInternalValue());
-        add_statement.BindInt64(
-            5, po->channel_id().creation_time().ToInternalValue());
+            3, po->channel_id().creation_time().ToInternalValue());
         if (!add_statement.Run())
           NOTREACHED() << "Could not add a server bound cert to the DB.";
         break;
       }
       case PendingOperation::CHANNEL_ID_DELETE:
         del_statement.Reset(true);
         del_statement.BindString(0, po->channel_id().server_identifier());
         if (!del_statement.Run())
           NOTREACHED() << "Could not delete a server bound cert from the DB.";
         break;
@@ skipping 23 matching lines @@
 }
 
 void SQLiteChannelIDStore::Backend::BackgroundDeleteAllInList(
     const std::list<std::string>& server_identifiers) {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
   if (!db_.get())
     return;
 
   sql::Statement del_smt(db_->GetCachedStatement(
-      SQL_FROM_HERE, "DELETE FROM origin_bound_certs WHERE origin=?"));
+      SQL_FROM_HERE, "DELETE FROM channel_id WHERE host=?"));
   if (!del_smt.is_valid()) {
     LOG(WARNING) << "Unable to delete channel ids.";
     return;
   }
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin()) {
     LOG(WARNING) << "Unable to delete channel ids.";
     return;
   }
@@ skipping 45 matching lines @@
   backend_->SetForceKeepSessionState();
 }
 
 SQLiteChannelIDStore::~SQLiteChannelIDStore() {
   backend_->Close();
   // We release our reference to the Backend, though it will probably still have
   // a reference if the background task runner has not run Close() yet.
 }
 
 }  // namespace net