libroach: ensure encryption flags are passed if previously used.

marc · marc · commit fda5612869f4 · 2018-08-16T12:57:45.000-04:00
Add some safeguards around the encryption flag.
When we have a `COCKROACH_REGISTRY` file, we absolutely must keep using
it to keep track of all files.

It gets created the first time a file is created when using the
file_registry which currently only happens when
`--enterprise-encryption` is specified.

This means that if we have a registry file, we MUST have encryption
options (and be using a CCL build).

Also add a simple interactive test to check encryption transition and
handling of some bad args (wrong keys, no encryption flag)

Release note: None
diff --git a/c-deps/libroach/ccl/db.cc b/c-deps/libroach/ccl/db.cc
@@ -126,6 +126,13 @@ rocksdb::Status DBOpenHookCCL(std::shared_ptr<rocksdb::Logger> info_log, const s
     return rocksdb::Status::OK();
   }
 
+  // The Go code sets the "file_registry" storage version if we specified encryption flags,
+  // but let's double check anyway.
+  if (!db_opts.use_file_registry) {
+    return rocksdb::Status::InvalidArgument(
+        "on-disk version does not support encryption, but we found encryption flags");
+  }
+
   // We use a logger at V(0) for encryption status instead of the existing info_log.
   // This should only be used to occasional logging (eg: key loading and rotation).
   std::shared_ptr<rocksdb::Logger> logger(NewDBLogger(0));
@@ -146,13 +153,6 @@ rocksdb::Status DBOpenHookCCL(std::shared_ptr<rocksdb::Logger> info_log, const s
                   status.getState());
   }
 
-  // The Go code sets the "file_registry" storage version if we specified encryption flags,
-  // but let's double check anyway.
-  if (!db_opts.use_file_registry) {
-    return rocksdb::Status::InvalidArgument(
-        "on-disk version does not support encryption, but we found encryption flags");
-  }
-
   // Parse extra_options.
   cockroach::ccl::baseccl::EncryptionOptions opts;
   if (!opts.ParseFromArray(options.data, options.len)) {
diff --git a/c-deps/libroach/ccl/db_test.cc b/c-deps/libroach/ccl/db_test.cc
@@ -51,13 +51,16 @@ TEST_F(CCLTest, DBOpenHook) {
 }
 
 TEST_F(CCLTest, DBOpen) {
+  // Use a real directory, we need to create a file_registry.
+  TempDirHandler dir;
+
   {
     // Empty options: no encryption.
     DBOptions db_opts = defaultDBOptions();
     DBEngine* db;
     db_opts.use_file_registry = true;
 
-    EXPECT_STREQ(DBOpen(&db, DBSlice(), db_opts).data, NULL);
+    EXPECT_STREQ(DBOpen(&db, ToDBSlice(dir.Path("")), db_opts).data, NULL);
     DBEnvStatsResult stats;
     EXPECT_STREQ(DBGetEnvStats(db, &stats).data, NULL);
     EXPECT_STREQ(stats.encryption_status.data, NULL);
@@ -68,6 +71,24 @@ TEST_F(CCLTest, DBOpen) {
 
     DBClose(db);
   }
+  {
+    // No options but file registry exists.
+    DBOptions db_opts = defaultDBOptions();
+    DBEngine* db;
+    db_opts.use_file_registry = true;
+
+    // Create bogus file registry.
+    ASSERT_OK(rocksdb::WriteStringToFile(rocksdb::Env::Default(), "",
+                                         dir.Path(kFileRegistryFilename), true));
+
+    auto ret = DBOpen(&db, ToDBSlice(dir.Path("")), db_opts);
+    EXPECT_STREQ(std::string(ret.data, ret.len).c_str(),
+                 "Invalid argument: encryption was used on this store before, but no encryption "
+                 "flags specified. You need a CCL build and must fully specify the "
+                 "--enterprise-encryption flag");
+    ASSERT_OK(rocksdb::Env::Default()->DeleteFile(dir.Path(kFileRegistryFilename)));
+  }
+
   {
     // Encryption enabled.
     DBOptions db_opts = defaultDBOptions();
@@ -84,7 +105,7 @@ TEST_F(CCLTest, DBOpen) {
     ASSERT_TRUE(enc_opts.SerializeToString(&tmpstr));
     db_opts.extra_options = ToDBSlice(tmpstr);
 
-    EXPECT_STREQ(DBOpen(&db, DBSlice(), db_opts).data, NULL);
+    EXPECT_STREQ(DBOpen(&db, ToDBSlice(dir.Path("")), db_opts).data, NULL);
     DBEnvStatsResult stats;
     EXPECT_STREQ(DBGetEnvStats(db, &stats).data, NULL);
     EXPECT_STRNE(stats.encryption_status.data, NULL);
diff --git a/c-deps/libroach/db.cc b/c-deps/libroach/db.cc
@@ -137,10 +137,9 @@ namespace cockroach {
 
 // DBOpenHookOSS mode only verifies that no extra options are specified.
 rocksdb::Status DBOpenHookOSS(std::shared_ptr<rocksdb::Logger> info_log, const std::string& db_dir,
-                              const DBOptions opts, EnvManager* env_mgr) {
-  if (opts.extra_options.len != 0) {
-    return rocksdb::Status::InvalidArgument(
-        "DBOptions has extra_options, but OSS code cannot handle them");
+                              const DBOptions db_opts, EnvManager* env_mgr) {
+  if (db_opts.extra_options.len != 0) {
+    return rocksdb::Status::InvalidArgument("encryption options are not supported in OSS builds");
   }
   return rocksdb::Status::OK();
 }
@@ -197,6 +196,23 @@ DBStatus DBOpen(DBEngine** db, DBSlice dir, DBOptions db_opts) {
       return ToDBStatus(status);
     }
 
+    status = file_registry->CheckNoRegistryFile();
+    if (!status.ok()) {
+      // We have a file registry, this means we've used encryption flags before
+      // and are tracking all files on disk. Running without encryption (extra_options empty)
+      // will bypass the file registry and lose changes.
+      // In this case, we have multiple possibilities:
+      // - no extra_options: this fails here
+      // - extra_options:
+      //   - OSS: this fails in the OSS hook (OSS does not understand extra_options)
+      //   - CCL: fails if the options do not parse properly
+      if (db_opts.extra_options.len == 0) {
+        return ToDBStatus(rocksdb::Status::InvalidArgument(
+            "encryption was used on this store before, but no encryption flags specified. You need "
+            "a CCL build and must fully specify the --enterprise-encryption flag"));
+      }
+    }
+
     // EnvManager takes ownership of the file registry.
     env_mgr->file_registry.swap(file_registry);
   } else {
@@ -214,12 +230,6 @@ DBStatus DBOpen(DBEngine** db, DBSlice dir, DBOptions db_opts) {
     return ToDBStatus(hook_status);
   }
 
-  // TODO(mberhault):
-  // - check available ciphers somehow?
-  //   We may have a encrypted files in the registry file but running without encryption flags.
-  // - pass read-only flag though, we should not be modifying file/key registries (including key
-  //   rotation) in read-only mode.
-
   // Register listener for tracking RocksDB stats.
   std::shared_ptr<DBEventListener> event_listener(new DBEventListener);
   options.listeners.emplace_back(event_listener);
diff --git a/c-deps/libroach/db_test.cc b/c-deps/libroach/db_test.cc
@@ -13,6 +13,7 @@
 // permissions and limitations under the License.
 
 #include "db.h"
+#include "file_registry.h"
 #include "include/libroach.h"
 #include "status.h"
 #include "testutils.h"
@@ -30,29 +31,78 @@ TEST(Libroach, DBOpenHook) {
   // Try extra_options with anything at all.
   db_opts.extra_options = ToDBSlice("blah");
   EXPECT_ERR(DBOpenHookOSS(nullptr, "", db_opts, nullptr),
-             "DBOptions has extra_options, but OSS code cannot handle them");
+             "encryption options are not supported in OSS builds");
 }
 
 TEST(Libroach, DBOpen) {
-  DBOptions db_opts = defaultDBOptions();
-  DBEngine* db;
-
-  EXPECT_STREQ(DBOpen(&db, DBSlice(), db_opts).data, NULL);
-  DBEnvStatsResult stats;
-  EXPECT_STREQ(DBGetEnvStats(db, &stats).data, NULL);
-  EXPECT_STREQ(stats.encryption_status.data, NULL);
-  EXPECT_EQ(stats.total_files, 0);
-  EXPECT_EQ(stats.total_bytes, 0);
-  EXPECT_EQ(stats.active_key_files, 0);
-  EXPECT_EQ(stats.active_key_bytes, 0);
-
-  // Fetch registries, parse, and check that they're empty.
-  DBEncryptionRegistries result;
-  EXPECT_STREQ(DBGetEncryptionRegistries(db, &result).data, NULL);
-  EXPECT_STREQ(result.key_registry.data, NULL);
-  EXPECT_STREQ(result.file_registry.data, NULL);
-
-  DBClose(db);
+  // Use a real directory, we need to create a file_registry.
+  TempDirHandler dir;
+
+  {
+    DBOptions db_opts = defaultDBOptions();
+
+    DBEngine* db;
+    EXPECT_STREQ(DBOpen(&db, ToDBSlice(dir.Path("")), db_opts).data, NULL);
+    DBEnvStatsResult stats;
+    EXPECT_STREQ(DBGetEnvStats(db, &stats).data, NULL);
+    EXPECT_STREQ(stats.encryption_status.data, NULL);
+    EXPECT_EQ(stats.total_files, 0);
+    EXPECT_EQ(stats.total_bytes, 0);
+    EXPECT_EQ(stats.active_key_files, 0);
+    EXPECT_EQ(stats.active_key_bytes, 0);
+
+    // Fetch registries, parse, and check that they're empty.
+    DBEncryptionRegistries result;
+    EXPECT_STREQ(DBGetEncryptionRegistries(db, &result).data, NULL);
+    EXPECT_STREQ(result.key_registry.data, NULL);
+    EXPECT_STREQ(result.file_registry.data, NULL);
+
+    DBClose(db);
+  }
+  {
+    // We're at storage version FileRegistry, but we don't have one.
+    DBOptions db_opts = defaultDBOptions();
+    db_opts.use_file_registry = true;
+
+    DBEngine* db;
+    EXPECT_STREQ(DBOpen(&db, ToDBSlice(dir.Path("")), db_opts).data, NULL);
+    DBClose(db);
+  }
+  {
+    // We're at storage version FileRegistry, and the file_registry file exists:
+    // this is not supported in OSS mode, or without encryption options.
+    DBOptions db_opts = defaultDBOptions();
+    db_opts.use_file_registry = true;
+
+    // Create bogus file registry.
+    ASSERT_OK(rocksdb::WriteStringToFile(rocksdb::Env::Default(), "",
+                                         dir.Path(kFileRegistryFilename), true));
+
+    DBEngine* db;
+    auto ret = DBOpen(&db, ToDBSlice(dir.Path("")), db_opts);
+    EXPECT_STREQ(std::string(ret.data, ret.len).c_str(),
+                 "Invalid argument: encryption was used on this store before, but no encryption "
+                 "flags specified. You need a CCL build and must fully specify the "
+                 "--enterprise-encryption flag");
+    ASSERT_OK(rocksdb::Env::Default()->DeleteFile(dir.Path(kFileRegistryFilename)));
+  }
+  {
+    // We're at storage version FileRegistry, and the file_registry file exists.
+    // We do have encryption options, but those are not supported in OSS builds.
+    DBOptions db_opts = defaultDBOptions();
+    db_opts.use_file_registry = true;
+    db_opts.extra_options = ToDBSlice("blah");
+
+    // Create bogus file registry.
+    ASSERT_OK(rocksdb::WriteStringToFile(rocksdb::Env::Default(), "",
+                                         dir.Path(kFileRegistryFilename), true));
+
+    DBEngine* db;
+    auto ret = DBOpen(&db, ToDBSlice(dir.Path("")), db_opts);
+    EXPECT_STREQ(std::string(ret.data, ret.len).c_str(),
+                 "Invalid argument: encryption options are not supported in OSS builds");
+    ASSERT_OK(rocksdb::Env::Default()->DeleteFile(dir.Path(kFileRegistryFilename)));
+  }
 }
 
 TEST(Libroach, BatchSSTablesForCompaction) {
diff --git a/c-deps/libroach/file_registry.h b/c-deps/libroach/file_registry.h
@@ -55,6 +55,7 @@ class FileRegistry {
 
   // Load the file registry. Errors on file reading/parsing issues.
   // OK if the file does not exist or is empty.
+  // This does **not** create a file registry.
   rocksdb::Status Load();
 
   // Returns a hash-set of all used env types.
diff --git a/pkg/cli/interactive_tests/test_encryption.tcl b/pkg/cli/interactive_tests/test_encryption.tcl
@@ -0,0 +1,90 @@
+#! /usr/bin/env expect -f
+#
+source [file join [file dirname $argv0] common.tcl]
+
+set storedir "encryption_store"
+set keydir "$storedir/keys"
+
+spawn /bin/bash
+send "PS1=':''/# '\r"
+eexpect ":/# "
+
+proc file_has_size {filepath size} {
+  if {! [file exist $filepath]} {
+    report "MISSING EXPECTED FILE: $filepath"
+    exit 1
+  }
+  set fsize [file size $filepath]
+  if { $fsize != $size } {
+		report "WRONG FILE SIZE FOR: $filepath. EXPECTED $size, GOT $fsize"
+		exit 1
+	}
+}
+
+start_test "Generate encryption keys."
+send "mkdir -p $keydir\n"
+send "$argv gen encryption-key -s 128 $keydir/aes-128.key\r"
+eexpect "successfully created AES-128 key: $keydir/aes-128.key"
+send "$argv gen encryption-key -s 192 $keydir/aes-192.key\r"
+eexpect "successfully created AES-192 key: $keydir/aes-192.key"
+send "$argv gen encryption-key -s 256 $keydir/aes-256.key\r"
+eexpect "successfully created AES-256 key: $keydir/aes-256.key"
+file_has_size "$keydir/aes-128.key" "48"
+file_has_size "$keydir/aes-192.key" "56"
+file_has_size "$keydir/aes-256.key" "64"
+end_test
+
+start_test "Start normal node."
+send "$argv start --insecure --store=$storedir\r"
+eexpect "node starting"
+interrupt
+eexpect "shutdown completed"
+send "$argv debug encryption-status $storedir\r"
+eexpect ""
+end_test
+
+start_test "Restart with plaintext."
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=plain,old-key=plain\r"
+eexpect "node starting"
+interrupt
+eexpect "shutdown completed"
+send "$argv debug encryption-status $storedir --enterprise-encryption=path=$storedir,key=plain,old-key=plain\r"
+eexpect "    \"Active\": true,\r\n    \"Type\": \"Plaintext\","
+# Try starting without the encryption flag.
+send "$argv start --insecure --store=$storedir\r"
+eexpect "encryption was used on this store before, but no encryption flags specified."
+end_test
+
+start_test "Restart with AES-128."
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-128.key,old-key=plain\r"
+eexpect "node starting"
+interrupt
+eexpect "shutdown completed"
+send "$argv debug encryption-status $storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-128.key,old-key=plain\r"
+eexpect "    \"Active\": true,\r\n    \"Type\": \"AES128_CTR\","
+# Try starting without the encryption flag.
+send "$argv start --insecure --store=$storedir\r"
+eexpect "encryption was used on this store before, but no encryption flags specified."
+# Try with the wrong key.
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-192.key,old-key=plain\r"
+eexpect "key_manager does not have a key with ID"
+end_test
+
+start_test "Restart with AES-256."
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-256.key,old-key=$keydir/aes-128.key\r"
+eexpect "node starting"
+interrupt
+eexpect "shutdown completed"
+send "$argv debug encryption-status $storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-256.key,old-key=plain\r"
+eexpect "    \"Active\": true,\r\n    \"Type\": \"AES256_CTR\","
+# Startup again, but don't specify the old key, it's no longer in use.
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-256.key,old-key=plain\r"
+eexpect "node starting"
+interrupt
+# Try starting without the encryption flag.
+send "$argv start --insecure --store=$storedir\r"
+eexpect "encryption was used on this store before, but no encryption flags specified."
+# Try with the wrong key.
+send "$argv start --insecure --store=$storedir --enterprise-encryption=path=$storedir,key=$keydir/aes-192.key,old-key=plain\r"
+eexpect "key_manager does not have a key with ID"
+end_test
