Encryption with Change Management #7020

matthewvon · 2020-06-23T20:49:24Z

Encryption at rest is often an enterprise requirement. This PR provides an implementation that uses OpenSSLs AES 256 CTL encryption.

The code is written in such a way that the programmer can easily switch to a different library and/or encryption cipher. And the switch can happen later without requiring the end user to dump their entire database then reload under the new library / cipher. The encryption library is intentionally loaded at runtime, not compiled into rocksdb. This allows for situations such as HeartBleed where users want a quick update of security code without waiting for a new product release. They simply load the new library and restart the database.

The code also allows for encryption keys to change. There must be a "current key" for writing and reading. There can also be previous keys for reading older .sst files. This implies that the end user can migrate from older keys either by simply waiting for compactions to naturally replace older keys, or by forcing a compaction of all files. The latter might take time, but the database is still operational.

…db into mv-changable-encryption

matthewvon · 2020-06-23T23:14:32Z

I am staring at this error and it really looks like a compiler bug:

https://travis-ci.org/github/facebook/rocksdb/jobs/701431752

Open to making any fixes that I am missing.

matthewvon · 2020-06-23T23:25:09Z

@ajkr @mrambacher @yiwu-arbug As promised, here is the second encryption PR that exploits inheritance from #6830. Mark has already pointed out that part of the library_loader is already present Env. I will adjust to use it Wednesday.

pdillinger · 2020-06-24T15:00:45Z

include/rocksdb/env_encrypt2.h

+
+  bool Valid() const { return valid_; };
+  const Sha1Description_t& key_desc() const { return key_desc_; };
+  const AesCtrKey_t& key() const { return key_; };


We hit this all the time with gcc 4.8, which is why we added the build. Ctor param shadows this member function.

@pdillinger Thank you for taking time to explain. I can easily adapt the constructor.

this is complete and passing tests now.

…db into mv-changable-encryption

yiwu-arbug

Thank you for adding the EncryptedEnv interface and the continuing work. I learn a lot of crypto basics from the original EncryptedEnv implementation.

I have some questions. They are probably more related to the original EncryptedEnv, but since those logic are reused, I'd take the chance for discussion

We are concatenating 8 bytes nonce and 8 bytes counter to form a 16 bytes openssl IV. This is incompatible with openssl format, where openssl use the whole 16 bytes as a 128-bits big endian integer counter. Do we consider making the format compatible with openssl, so potentially we can use other openssl compatible tools to encrypt/decrypt file? (of cause there's also the file prefix needs to be strip beforehand).
We are encrypting (and decrypting) data block by block (the 16 bytes AES block), each time initiating openssl cipher again. Do we consider encrypting data in bulk. Say if rocksdb want to write 1MB into a file, we call EVP_EncryptInit_ex, EVP_EncryptUpdate and EVP_EncryptFinal set of functions only once? (actually maybe 3 times to take care of unaligned blocks at the begining and at the end). This depends on item 1. I don't have benchmark and not sure how much benefit it is, but at least it will save a lot of function calls.
It is undocumented, but with CTR mode openssl seems to support encrypting in-place (use the same address for encryption input and output). Have we consider using the fact to reduce number of memcopy?

And some minor comments. Thanks.

include/rocksdb/env_encrypt2.h

yiwu-arbug · 2020-06-25T05:46:44Z

util/library_loader.h

+// Base class / interface
+//  expectation is to derive one class for unux and one for Windows
+//
+class LibraryLoader {


Could we also have static link option?

I see two ways to achieve static link. First is a conditional compile. Second is to exploit PosixEnv::LoadLibrary()'s ability to pull symbols from either external library or running executable. I prefer the latter. It creates one code path for both scenarios. It would also allow the user to ship with a "current" version and then override at runtime if an update version comes out with a critical fix. The feature would require an option setting. If you like this idea, would you suggest which options structure is most appropriate?

might still need a conditional compile to make sure the symbols exist in the executable. I am guessing that a link against .a instead of .so would not pull in the symbols/functions needed unless there is "code calling them". any thoughts here are welcome

Not familiar with dylib loading, but it remind me Jemalloc allow to add prefixes to its methods so to not conflict with, say, libc. But if OpenSSL don't support that, maybe we just conditional compile to choose whether static link to dynamic link?

openssl does not directly support prefixes with dylib loading. I can make both static+dynamic or dynamic only work via compile time flag. If static is not correctly identified as missing at compile time, the compile fails. A couple of recent test passes are suggesting the dynamic load might be having dependency problems in your test infrastructure (I have added some debug code to figure this out. Might have to make test for openssl more exhaustive in build_detect_platform). I am continuing to work this issue.

yiwu-arbug · 2020-06-25T05:53:30Z

util/library_loader.h

+
+  EVP_CIPHER_CTX *EVP_CIPHER_CTX_new(void) const { return cipher_new_(); };
+
+  void EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx) { cipher_free_(ctx); }


EVP_CIPHER_CTX_free does not exist in openssl 1.0.2. Can we add a version check for openssl?

Agreed. And it ticked me off. The code already compensates. Please see (and the 3 lines following):

rocksdb/util/library_loader.cc

Line 96 in 09d5080

ctx_free_ = (EVP_MD_CTX_free_t)functions_["EVP_MD_CTX_free"];

The code simply maps the ENV_CIPHER_CTX_destroy into the cipher_free_ pointer since parameters are same.

OpenSSL 1.0.2 doesn't seems to provide EVP_CIPHER_CTX_destroy as well. With 1.0.2 the ctx can only be created on stack. https://www.openssl.org/docs/man1.0.2/man3/EVP_EncryptInit_ex.html

Ok. That would actually simplify the current code. Using thread_local to get around recoding a bunch of stuff. Keeps one context per thread ... which equates to one context per read or write call.

I have pulled the source code to 1.0.1u. It has functions _new, _free, and _cleanup. I have pulled the source code to 1.0.2.u. It has the functions _new, _free, and _cleanup. Going to strongly suggest a documentation problem on their website. My code is using _new, _free, and _reset. ENV_CIPHER_CTX_reset() is a name change for _cleanup. Source code uses the two equivalently inside _free and _init. I believe the real action item here is to create code that will pull _cleanup as an alternative entry point to _reset.

I was happily looking at maybe using a stack based copy of ENV_CIPHER_CTX. There is no destructor and there are internal allocations that need clean up. _new & _free might be safer in the long run, though calling _cleanup/_reset when done with stack struct accomplishes the same. Looking at other alternatives too based upon review of ctrl128.c reference you posted elsewhere in this PR.

yiwu-arbug · 2020-06-25T06:21:29Z

include/rocksdb/env_encrypt2.h

+      if (sizeof(marker) == marker_slice.size() &&
+          marker_slice.starts_with(Marker)) {
+        // code_version currently unused
+        uint8_t code_version = (uint8_t)marker_slice[7];


can we verify code_version is indeed 0, or return error?

Same for ReadSeqEncryptionPrefix.

oops ... will fix.

env/env_encrypt2.cc

…talized function parameters

matthewvon · 2020-06-25T15:03:28Z

@yiwu-arbug Per your item one about 16 versus 32 byte IV. The NIST standard and samples use 16 byte IV AES CTR 256. The code is currently verified against that standard. I have two sources that say 16 byte/128bit is proper IV for AES 256:

The encrypt/decrypt samples from the second document are part of env_encrypt2_test.cc.

Do you have a reference that I can review? There is nothing to prevent a coder from implementing a different cipher with different IV size. In fact, the purpose of env_encrypt2's design is to enable and encourage those changes and differences.

matthewvon · 2020-06-25T15:10:39Z

@yiwu-arbug Can you point me to source code or such for item three's update in place?

update: I believe this confirms your "update in place" for software AES implementation.

https://github.com/openssl/openssl/blob/master/crypto/aes/aes_core.c

Searching now for confirmation when hardware AES used (I know it is likely true, but feel obligated to verify).

update 2: it looks to me that the memmove within EncryptedWritableFile::Append() is related to getting data for encryption aligned with offsets within files. Once the addresses and offsets are aligned on proper boundaries, encryption/decryption is an "update in place". The shifting is about alignment.

update 3: ok, I understand now how to eliminate the need for alignment of buffers and offsets for CTR mode. I do not know what other derived code might be depending upon the alignment. Therefore, I am going to copy those routines from EncryptedEnv into EnctypedEnv2 for modification. This should also simplify reuse of the EVP_CIPHER_CTX object.

update 4: hold the phone. the memmove is because we are handed a const buffer from the caller. we do not know the impact of encrypt in place to the caller. so the memmove is to create a copy that we then encrypt in place. the current EncryptionEnv is clean. The alignment stuff is purely a side benefit ... and likely required for use_direct_io.

…er module. switched to static const char *

…db into mv-changable-encryption

matthewvon · 2020-06-25T17:26:31Z

@mrambacher I have converted code to PosixEnv::LoadLibrary(). That revealed a bug in env_posix.cc where constants used for library suffixes within std::string might not have initialized by the time LoadLibrary is called. Switched those three constants to static const char * so that compiler initializes, not loader.

… of SetKeys

matthewvon · 2020-08-09T22:50:15Z

As of this moment, all checks have again passed. At least two of the recent failures were infrastructure related ... did not check 178cf31.

…table member variable

…ption

… dead code

mrambacher

This code should also be made to work with CMake and on Windows (if possible).

mrambacher · 2020-11-09T16:04:11Z

env/env_openssl.cc

+thread_local static std::unique_ptr<EVP_CIPHER_CTX, void (*)(EVP_CIPHER_CTX*)>
+    aes_context(nullptr, &do_nothing);


Can we use the util/thread_local class?

mrambacher · 2020-11-09T16:45:11Z

include/rocksdb/env_openssl.h

+struct ShaDescription {
+  uint8_t desc[EVP_MAX_MD_SIZE];
+  bool valid;
+


Is there a reason this class/struct is still required in the public API?

mrambacher · 2020-11-09T16:45:30Z

include/rocksdb/env_openssl.h

+struct AesCtrKey {
+  uint8_t key[EVP_MAX_KEY_LENGTH];
+  bool valid;


Is this class still required in the public API?

mrambacher · 2020-11-09T16:46:28Z

include/rocksdb/env_openssl.h

+class OpenSSLEncryptionProvider : public EncryptionProvider {
+ public:


Is this class still required in the public API?

mrambacher · 2020-11-09T16:47:03Z

env/env_openssl_impl.h

+class EncryptedWritableFileV2 : public EncryptedWritableFile {
+ public:


Is this class still required?

mrambacher · 2020-11-09T18:52:52Z

include/rocksdb/env_openssl.h

+
+#pragma once
+
+#ifdef ROCKSDB_OPENSSL_AES_CTR


Is this still required in this header (along with the openssl includes)?

mrambacher · 2020-11-09T18:53:06Z

include/rocksdb/env_openssl.h

@@ -0,0 +1,180 @@
+//  copyright (c) 2011-present, Facebook, Inc.  All rights reserved.


Should this file still be in the public API?

mrambacher · 2020-11-09T18:59:30Z

util/library_loader.cc

+size_t LibraryLoader::GetEntryPoints(std::map<std::string, void*>& functions) {
+  size_t num_found{0};


Can we get rid of this class and have a "LoadSymbols API" off the Dynamic Library class?

mrambacher · 2020-11-09T19:04:52Z

util/library_loader.cc

+    encrypt_init_ = (EVP_EncryptInit_ex_t)functions_["EVP_EncryptInit_ex"];
+    aes_256_ctr_ = (EVP_aes_256_ctr_t)functions_["EVP_aes_256_ctr"];
+    encrypt_update_ = (EVP_EncryptUpdate_t)functions_["EVP_EncryptUpdate"];
+    encrypt_final_ = (EVP_EncryptFinal_ex_t)functions_["EVP_EncryptFinal_ex"];


Are any of these allowed to be NULL? Is there an advantage to doing it as LoadSymbols versus a direct call (LoadSymbol) with a better check?

mrambacher · 2020-11-09T19:09:08Z

util/library_loader.cc

+UnixLibCrypto::UnixLibCrypto() : LibraryLoader(crypto_lib_name_) {
+  if (is_valid_) {


Is there a reason to keep this in a separate file rather than in the env_openssl file?

…piled

matthewvon and others added 12 commits June 23, 2020 15:48

add env_encrypt2 and library_loader files and tests to build

f601a32

make library_loader conditional on ROCKSDB_OPENSSL_AES_CTR flag

385702a

execute clang-format against the library_loader files

e464b9c

another clang-format change

47f5ba6

another clang-format change

c2e7a3b

correct TARGETS file via script

d2f7f49

use ROCKSDB_NAMESPACE instead of rocksdb

ee5347c

Merge branch 'mv-changable-encryption' of github.com:matthewvon/rocks…

1fc8d3b

…db into mv-changable-encryption

more clang-format changes

3a99b7f

have test code use updated openssl api

b611245

more clang-format

261d6b9

clean up library_loader_test for compile when openssl not found

c7e3c71

facebook-github-bot added the CLA Signed label Jun 23, 2020

pdillinger reviewed Jun 24, 2020

View reviewed changes

matthewvon added 3 commits June 24, 2020 12:51

address shadow variable issue

1aebcb5

Merge branch 'mv-changable-encryption' of github.com:matthewvon/rocks…

767db12

…db into mv-changable-encryption

fix another shadow problem

8d5cfa6

yiwu-arbug reviewed Jun 25, 2020

View reviewed changes

matthewvon and others added 2 commits June 25, 2020 10:45

remove _t suffix. remove virtual from override functions. change capi…

53e36b6

…talized function parameters

clang-format fixes

09d5080

matthewvon and others added 4 commits June 25, 2020 13:17

static const std::string were not initializing before call from anoth…

ee7f477

…er module. switched to static const char *

convert from pre PosixEnv::LoadLibrary routines to use of LoadLibrary.

8010570

Merge branch 'mv-changable-encryption' of github.com:matthewvon/rocks…

605d7f5

…db into mv-changable-encryption

clang-format cleanup

fcca653

address PR comments concerning test of code_version and thread safety…

2af6433

… of SetKeys

whitespace change to force new CI run

ac523ae

MatthewVon added 2 commits August 14, 2020 11:52

env_openssl_test would crash on OSX with static keylock. Made it a mu…

cda48e7

…table member variable

correct key_lock to key_lock_ since now a member variable

a9135ca

matthewvon mentioned this pull request Aug 21, 2020

Feature/mv encrypt 2 stardog-union/rocksdb#15

Merged

matthewvon added 14 commits October 19, 2020 10:08

WIP, safety check-in and update from rocksdb master, DOES NOT COMPILE

5180b7b

safety check-in ... does not build

1b0c99c

Merge branch 'master' into mv-changable-encryption

67bc3b8

finish conversion to new EncryptionProvider model

97a767b

add missing override marker to TEST_Initialize

a1a61eb

remove obsolete kCTROpenSSL declare/define

9e903c5

must make OpenSSLEncryptionProvider a compile option within env_encry…

b55ec0e

…ption

update NormalizingEnvWrapper to properly delete encryptionEnv

9767058

oops ... needed clang-format

8e2a09c

guessing a circleCI test was failing due to overlapped test execution?

3dfa896

correct formatting

7d90321

output from python3 buckifier/buckify_rocksdb.py

fcf3737

forgot to use lock when adding key to provider. also cleaned out some…

019cf6b

… dead code

clang-format wanted one blank line removed

5dc2a57

mrambacher reviewed Nov 9, 2020

View reviewed changes

matthewvon added 7 commits November 22, 2020 13:51

address PR comments. Not complete.

aaa8e16

remove util/library_loader. merge key components into env_openssl.

e501208

forgot to run clang-format. reversed testing change to CMakeLists.txt

bde3d5c

add option for AES encryption via OpenSSL dynamic library

21a89e8

clean up unused variable warning when dynamic library support not com…

3f12534

…piled

stop compile of OpenSSL AES if DynamicLibrary disabled

690ce77

clang-format env_openssl.cc

97df484

CMCDragonkai mentioned this pull request Jul 14, 2022

Transparent Encryption/Decryption Layer between LevelDB and Filesystem "Block Manager" MatrixAI/js-db#5

Open

3 tasks

mrambacher mentioned this pull request Sep 12, 2023

WIP Support AES encryption #11819

Closed

acelyc111 mentioned this pull request Sep 13, 2023

Data at rest encryption apache/incubator-pegasus#1575

Closed


		EVP_CIPHER_CTX *EVP_CIPHER_CTX_new(void) const { return cipher_new_(); };

		void EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx) { cipher_free_(ctx); }

		thread_local static std::unique_ptr<EVP_CIPHER_CTX, void ()(EVP_CIPHER_CTX)>
		aes_context(nullptr, &do_nothing);

		class OpenSSLEncryptionProvider : public EncryptionProvider {
		public:

		class EncryptedWritableFileV2 : public EncryptedWritableFile {
		public:

		@@ -0,0 +1,180 @@
		// copyright (c) 2011-present, Facebook, Inc. All rights reserved.

		size_t LibraryLoader::GetEntryPoints(std::map<std::string, void*>& functions) {
		size_t num_found{0};

		UnixLibCrypto::UnixLibCrypto() : LibraryLoader(crypto_lib_name_) {
		if (is_valid_) {


		#pragma once

		#ifdef ROCKSDB_OPENSSL_AES_CTR

Encryption with Change Management #7020

Are you sure you want to change the base?

Encryption with Change Management #7020

Conversation

matthewvon commented Jun 23, 2020 • edited Loading

matthewvon commented Jun 23, 2020

matthewvon commented Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwu-arbug left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewvon Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewvon Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewvon commented Jun 25, 2020

matthewvon commented Jun 25, 2020 • edited Loading

matthewvon commented Jun 25, 2020

matthewvon commented Aug 9, 2020

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewvon commented Jun 23, 2020 •

edited

Loading

matthewvon commented Jun 23, 2020 •

edited

Loading

yiwu-arbug left a comment •

edited

Loading

matthewvon Jun 25, 2020 •

edited

Loading

matthewvon Jun 25, 2020 •

edited

Loading

matthewvon commented Jun 25, 2020 •

edited

Loading