Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[COMPARE ALL CHANGES] [encryption] Manage file key by the file to encrypt itself #17

Closed
wants to merge 19 commits into from
Closed

[COMPARE ALL CHANGES] [encryption] Manage file key by the file to encrypt itself #17

wants to merge 19 commits into from

Conversation

acelyc111
Copy link
Member

No description provided.

acelyc111 and others added 11 commits August 16, 2023 19:12
@acelyc111
Add KeyManagedEncryptedEnv and AESBlockCipher (#4)
1db2301 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@113b363

Summary:
Introduce `KeyManagedEncryptedEnv` which wraps around `EncryptedEnv` but
provides an `KeyManager` API to enable key management per file. Also
implements `AESBlockCipher` with OpenSSL.

Test Plan:
not tested yet. will update.

Signed-off-by: Yi Wu <[email protected]>
Signed-off-by: tabokie <[email protected]>
@acelyc111
encryption: change to use openssl EVP API (#5)
9f909ba 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@3d44a33

Summary:
Instead of using openssl's raw `AES_encrypt` and `AES_decrypt` API,
which is a low level call to encrypt or decrypt exact one block (16
bytes), we change to use the `EVP_*` API. The former is deprecated, and
will use the default C implementation without AES-NI support. Also the
EVP API is capable of handing CTR mode on its own.

Test Plan:
will add tests

Signed-off-by: Yi Wu <[email protected]>

---------

Signed-off-by: Yi Wu <[email protected]>
Co-authored-by: yiwu-arbug <[email protected]>
@acelyc111
Fix NewRandomRWFile and ReuseWritableFile in KeyManagedEncryptedEnv (#6)
260fee5 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@2360562

Summary:
Fix NewRandomRWFile and ReuseWritableFile misuse of `GetFile()` and
`NewFile()`. See inline comments.

Test Plan:
manual test with tikv

Signed-off-by: Yi Wu <[email protected]>

Co-authored-by: yiwu-arbug <[email protected]>
@acelyc111 @hunterlxt
Atomize RenameFile in KeyManagedEncryptedEnv (facebook#7)
3e994db 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@93e89a5

fix bug: tikv/tikv#9115

Summary: we need to update encryption metadata via
encryption::DataKeyManager, which cannot combine with the actual file
operation into one atomic operation. In RenameFile, when the src_file
has been removed, power is off, then we may lost the file info of
src_file next restart.

Signed-off-by: Xintao [[email protected]](mailto:[email protected])

Signed-off-by: Xintao <[email protected]>
Co-authored-by: Xintao <[email protected]>
@acelyc111
Atomize Rename operation when encryption is enabled (facebook#8)
f6ac09d 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@bbd27cf

used LinkFile instead of RenameFile api of key manager. But LinkFile
needs check the dst file information, in RenameFile logic, we don't care
about that. So just skip encryption for current file.

Signed-off-by: Xintao [[email protected]](mailto:[email protected])
@acelyc111
Fix the bug that the key manager is not updated during the Rename (fa…
3b14183 
…cebook#9)

apache/incubator-pegasus#1575

Cherry-pick from
tikv@1868d12

Signed-off-by: Xintao <[email protected]>
Signed-off-by: tabokie <[email protected]>
@acelyc111
Add sm4 encryption (facebook#10)
0277015 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@4cebfc1

* Add SM4-CTR encryption algorithm
* Adjust block size for sm4 encryption
* Add UT for SM4 encryption
* Adjust macros indentation for sm4
* Fix format for adding sm4

Signed-off-by: Jarvis Zheng <[email protected]>
@acelyc111
Check OPENSSL_NO_SM4 before using sm4 encryption (facebook#11)
5efb80e 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@9464766

In some env, user installed openssl by yum install, and the openssl
software may compiled with OPENSSL_NO_SM4 flag, so although the version
is >= 1.1.1, but we still could not use sm4 in that situation.

Signed-off-by: Jarvis Zheng <[email protected]>
@acelyc111 @tabokie
hook delete dir in encrypted env (facebook#334) (facebook#12)
f61f3ea 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@acc624f

* hook delete dir in encrypted env
* add a comment

Signed-off-by: tabokie <[email protected]>
Co-authored-by: Xinye Tao <[email protected]>
@acelyc111
fix renaming encrypted directory (facebook#13)
05d042a 
apache/incubator-pegasus#1575

Cherry-pick from
tikv@14f36f8
(without compaction related code)

* fix renaming encrypted directory

Signed-off-by: tabokie <[email protected]>
@acelyc111
[encryption] Manage file key by the file to encrypt itself
7ec2a94
@acelyc111 acelyc111 mentioned this pull request Aug 16, 2023
Merged
WHBANG
WHBANG reviewed Aug 17, 2023
View reviewed changes
encryption/encryption.cc
// middle?
// Handle partial block at the end. The partial block is copied to buffer to
// fake a full block.
if (remaining_data_size > 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use assert to check whether remaining_data_size is 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a valid case while remaining_data_size > 0.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I mean use assert to replace if.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both cases (remaining_data_size > 0 and remaining_data_size == 0) are valid.

acelyc111 added a commit that referenced this pull request Sep 15, 2023
@acelyc111
[encryption] Manage file key by the file to encrypt itself (#16)
e9206ed 
apache/incubator-pegasus#1575

After all encryption related patches been cherry-picked from
[tikv](https://github.com/tikv/rocksdb/commits/6.29.tikv) and
merged, now we will improve the encrytion, including:
- Fix action job `build-linux-encrypted_env-no_compression-no_openssl`
  to build binaries without openssl and compression libs correctly.
- Fix action job `build-linux-encrypted_env-openssl` to export the
  `ENCRYPTED_ENV` enviroment variable correctly.
- Don not skip tests which are skipped by TiKV.
- Refactor `AESCTRCipherStream` and `AESEncryptionProvider` to support
  manage file key by the file itself, according to the design docs in
[Data at rest
encryption](apache/incubator-pegasus#1575).
- Remove all KeyManager related codes.
- Replace KeyManager tests by AES encryption tests.
- Refactor encryption/encryption_test.cc and add more tests.
- Make it possible to construct AESEncryptionProvider object via
  `EncryptionProvider::CreateFromString()` by registering a
  factory in "encryption" library.
  It's possible to construct an object by URI: `AES`, `AES://test` or
  `AES:<instance_key>,<EncryptionMethod>`.
- `ldb` tool support to parse `--fs_uri` flags as the URI mentioned
above.
- Add tests to create AESEncryptionProvider object in
  `CreateEncryptedEnvTest.CreateEncryptedFileSystem`
- `db_bench` support to run benchmark with encryption enabled, by adding
new flags for `db_bench`, they are `encryption_method` and
`encryption_instance_key`.
- Move code from the exported header directory (i.e.
include/rocksdb/encryption.h)
to rocksdb internal (i.e. encryption/encryption.h), do not expose them
to users.
- Code format.

Review hint: #17 shows all the
code changes
from the base branch (i.e. `pegasus-kv:v8.3.2-pegasus`), you can review
it together to
make sure the request branch `acelyc111:pk_enc_new` doesn't have vice
effect on the base.

Manual test:
```
// Generate some data.
./db_bench --encryption_method=AES128CTR --encryption_instance_key=test_instance_key  --num=10000

// Dump WAL OK
./tools/ldb --fs_uri="provider=AES; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES://test; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:test_instance_key,AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log

// Dump WAL failed. Pass bad provider parameters to --fs_uri, e.g.
./tools/ldb --fs_uri="provider=AES1:test_instance_key,1AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:bad_test_instance_key,AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:test_instance_key,AES192CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log

// The same to other ldb tools.

```
acelyc111 added a commit that referenced this pull request Sep 15, 2023
@acelyc111
[encryption] Manage file key by the file to encrypt itself (#16)
7bc2528 
apache/incubator-pegasus#1575

After all encryption related patches been cherry-picked from
[tikv](https://github.com/tikv/rocksdb/commits/6.29.tikv) and
merged, now we will improve the encrytion, including:
- Fix action job `build-linux-encrypted_env-no_compression-no_openssl`
  to build binaries without openssl and compression libs correctly.
- Fix action job `build-linux-encrypted_env-openssl` to export the
  `ENCRYPTED_ENV` enviroment variable correctly.
- Don not skip tests which are skipped by TiKV.
- Refactor `AESCTRCipherStream` and `AESEncryptionProvider` to support
  manage file key by the file itself, according to the design docs in
[Data at rest
encryption](apache/incubator-pegasus#1575).
- Remove all KeyManager related codes.
- Replace KeyManager tests by AES encryption tests.
- Refactor encryption/encryption_test.cc and add more tests.
- Make it possible to construct AESEncryptionProvider object via
  `EncryptionProvider::CreateFromString()` by registering a
  factory in "encryption" library.
  It's possible to construct an object by URI: `AES`, `AES://test` or
  `AES:<instance_key>,<EncryptionMethod>`.
- `ldb` tool support to parse `--fs_uri` flags as the URI mentioned
above.
- Add tests to create AESEncryptionProvider object in
  `CreateEncryptedEnvTest.CreateEncryptedFileSystem`
- `db_bench` support to run benchmark with encryption enabled, by adding
new flags for `db_bench`, they are `encryption_method` and
`encryption_instance_key`.
- Move code from the exported header directory (i.e.
include/rocksdb/encryption.h)
to rocksdb internal (i.e. encryption/encryption.h), do not expose them
to users.
- Code format.

Review hint: #17 shows all the
code changes
from the base branch (i.e. `pegasus-kv:v8.3.2-pegasus`), you can review
it together to
make sure the request branch `acelyc111:pk_enc_new` doesn't have vice
effect on the base.

Manual test:
```
// Generate some data.
./db_bench --encryption_method=AES128CTR --encryption_instance_key=test_instance_key  --num=10000

// Dump WAL OK
./tools/ldb --fs_uri="provider=AES; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES://test; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:test_instance_key,AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log

// Dump WAL failed. Pass bad provider parameters to --fs_uri, e.g.
./tools/ldb --fs_uri="provider=AES1:test_instance_key,1AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:bad_test_instance_key,AES128CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log
./tools/ldb --fs_uri="provider=AES:test_instance_key,AES192CTR; id=EncryptedFileSystem" dump_wal --walfile=/tmp/rocksdbtest-1000/dbbench/000004.log

// The same to other ldb tools.

```
@acelyc111 acelyc111 closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WHBANG WHBANG WHBANG left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acelyc111 @WHBANG