encryption: support renaming encrypted directory #14095

tabokie · 2023-01-30T06:51:01Z

Signed-off-by: tabokie [email protected]

What is changed and how it works?

Issue Number: Ref #12842

What's Changed:

Introduce a new format to store data keys for encrypted files. The old format stores mapping from file path to encryption key. The new format stores two mappings: directory path to directory ID, (dir ID + relative file path) to encryption key.

Only file path matching certain patterns will be encoded using the new format. So the change should be backward compatible for v1 architecture.

Compatibility with RocksDB:
(1) RocksDB uses Env::RenameFile to rename directory. We uses LinkFile + DeleteFile under the hood for atomicity. To be compatible, link_file and delete_file needs to detect if the path is a file or directory.
(2) We don't manage Env::CreateDir call from RocksDB. Instead the directory ID is only allocated when a file is created under the dir.
(3) We don't manage Env::DeleteDir call from RocksDB. Needs to call KeyManager::delete_dir manually.

Other options:

Using file system identifier (e.g. inode + dev id): it is not stable across server restart.
Editing all files under the renamed directory: it takes a long time to scan out all matching files within the dict.

Related changes

pingcap/kvproto#1098

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2023-01-30T06:51:02Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

tabokie · 2023-01-30T06:51:33Z

Still a draft, opinions welcome. cc @BusyJay @tonyxuqqi

tonyxuqqi · 2023-01-30T23:02:48Z

components/encryption/src/manager/mod.rs

+                    self.dir_dict
+                        .lock()
+                        .unwrap()
+                        .get((dir_id, relative_path))


(dir_id, relative_path) does not match dir_dict's definition HashMap<u64, ..>

tonyxuqqi · 2023-01-30T23:09:15Z

components/encryption/src/manager/mod.rs

@@ -347,6 +404,29 @@ impl Dicts {
        };
        self.rotate_key(key_id, data_key, master_key)
    }
+
+    fn rename_dir(&self, src_name: &str, dst_name: &str) -> Result<()> {


It seems to me the dir_dict needs to be updated as well.
For example, when a snapshot is applied, the tablet is renamed from tablet_snapshot/123_5_tmp to tablets/123_5.
All the keys in the dir_dict contains the prefix 123_5_tmp.
Now after the rename, we're still keeping the 123_5_tmp in the dir_dict.
But when the caller try to decrypt a SST under 123_5, the path will be 123_5/xxx.sst and it won't find the match.

Because "tablet_snapshot/123_5_tmp" is a first level directory under the whitelist, it will be encoded using dir ID. So the data keys are all stored using (dir_id, xx.sst). This way the dir renaming only needs to change the dir_id mapping.

Signed-off-by: tabokie <[email protected]>

BusyJay · 2023-01-31T12:54:51Z

components/encryption/src/manager/mod.rs

+    // makes it possible to rename directory without modifying records of every file.
+    //
+    // The reasons for having a whitelist instead of treating all directories this way:
+    // (1) keep v1 data keys file backward compatible.


We don't need to consider compatibility for now (may not even in the future). Simple is better.

It's about v1 data, because v1 and v2 share the implementation of DataKeyManager. To implement a completely new DataKeyManagerV2 is not economical because not all files should be encoded with dir id (for the reason (2)), they should fallback to the old format.

because not all files should be encoded with dir id

Can you give some examples that will not be encoded with dir id?

Local backup export file to an arbitrary user-specified folder; nested folder such as titandb.

@tabokie DataKeyManager is referenced in many codes , so I agree it's better to keep DataKeyManager as an entry struct for encryption/decryption. However, I also think we can simply copy all the current implementation for v1, just wrap under DataKeyManager. And the new implementation can only take effect on v2. It may simplify the logic.
Otherwise it may introduce regression.
Maybe in the future once the new implementation is battle proved, then we unify the two implementations.

@tonyxuqqi Missed your comment yesterday. Implementing two managers looks really bad, you have to trait them because there're so many places use them. But I think it's doable to separate the v2 functionality into a new plugin struct, i.e. if the whitelist is empty (for v1), the plugin is None and we don't create the new key file etc.

…rypt-rename Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

tonyxuqqi · 2023-03-01T23:35:39Z

components/encryption/src/file_dict_file.rs

-enum LogRecord {
-    Insert(FileInfo),
-    Remove,
+pub trait DictionaryItem: Sized + Clone {


Please add comments for 'DictionaryItem' and 'ProtobufDictionary'.

tonyxuqqi · 2023-03-02T00:28:53Z

components/test_util/src/encryption.rs

@@ -32,6 +32,7 @@ pub fn new_file_security_config(dir: &tempfile::TempDir) -> EncryptionConfig {
        file_dictionary_rewrite_threshold: 100000,
        master_key: master_key_cfg.clone(),
        previous_master_key: master_key_cfg,
+        v2_directory_whitelist: Vec::new(),


What will happen if there's one v2's folder missing from this list?

(1) The keys cannot be found, and (2) this directory cannot be renamed (the rename will raise an error). (1) shouldn't really happen unless you change the whitelist half way. The plan is to hardcode it and forbid configuration.

…rypt-rename Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

BusyJay · 2023-04-04T12:38:37Z

How about making rename as a call of TabletFactory? So that if we support different type of tablet (column store for example), we can use the same entry point. It should also help make mapping not depend on pattern.

tabokie · 2023-04-04T13:14:14Z

It should also help make mapping not depend on pattern.

How? In specific how are new_file and get_file implemented?

components/encryption/src/file_dict_file.rs

components/encryption/src/config.rs

components/encryption/src/file_dict_file.rs

tests/failpoints/cases/test_encryption.rs

Signed-off-by: tabokie <[email protected]>

tabokie · 2023-04-12T04:01:25Z

/release

sre-bot · 2023-04-12T04:50:50Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/9f454e875109efe215fdc4480846f0ba03a8ed69/centos7/tikv-server.tar.gz

ti-chi-bot · 2023-04-13T08:52:53Z

@tabokie: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…h importing data keys (#14583) ref #12842, ref #14095, ref #14097 support renaming encrypted dir (inefficiently) and batch importing data keys Signed-off-by: tabokie <[email protected]>

…h importing data keys (tikv#14583) ref tikv#12842, ref tikv#14095, ref tikv#14097 support renaming encrypted dir (inefficiently) and batch importing data keys Signed-off-by: tabokie <[email protected]>

…h importing data keys (tikv#14583) ref tikv#12842, ref tikv#14095, ref tikv#14097 support renaming encrypted dir (inefficiently) and batch importing data keys Signed-off-by: tabokie <[email protected]> Signed-off-by: CalvinNeo <[email protected]>

…h importing data keys (tikv#14583) ref tikv#12842, ref tikv#14095, ref tikv#14097 support renaming encrypted dir (inefficiently) and batch importing data keys Signed-off-by: tabokie <[email protected]> Signed-off-by: lidezhu <[email protected]>

tonyxuqqi · 2023-05-17T05:52:21Z

/close

ti-chi-bot · 2023-05-17T05:52:24Z

@tonyxuqqi: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 30, 2023

tabokie force-pushed the 230129-encrypt-rename branch from 5a89211 to c0d3818 Compare January 30, 2023 06:53

tabokie mentioned this pull request Jan 30, 2023

server: pass encryption key with tablet snapshot #14097

Merged

tonyxuqqi reviewed Jan 30, 2023

View reviewed changes

support renaming encrypted directory

c3eb782

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the 230129-encrypt-rename branch from c0d3818 to c3eb782 Compare January 31, 2023 10:02

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2023

BusyJay reviewed Jan 31, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2023

tabokie mentioned this pull request Feb 8, 2023

config: disallow encryption for v2 #14190

Merged

tabokie added 3 commits February 9, 2023 14:09

Merge branch 'master' of https://github.com/tikv/tikv into 230129-enc…

09d1f39

…rypt-rename Signed-off-by: tabokie <[email protected]>

make it work and add a test

e119871

Signed-off-by: tabokie <[email protected]>

avoid the hassle around holding reference in item

5da9494

Signed-off-by: tabokie <[email protected]>

tabokie removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2023

make it work on rocksdb and add some tests

031603b

Signed-off-by: tabokie <[email protected]>

tabokie requested review from tonyxuqqi and BusyJay February 13, 2023 02:34

tonyxuqqi reviewed Mar 1, 2023

View reviewed changes

tonyxuqqi reviewed Mar 2, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2023

tabokie added 2 commits April 4, 2023 14:12

Merge branch 'master' of https://github.com/tikv/tikv into 230129-enc…

ad642a3

…rypt-rename Signed-off-by: tabokie <[email protected]>

add comment

9cdc448

Signed-off-by: tabokie <[email protected]>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2023

BusyJay reviewed Apr 10, 2023

View reviewed changes

tabokie added 3 commits April 10, 2023 19:16

fix tests

3b0aacc

Signed-off-by: tabokie <[email protected]>

address comment

44c2cd3

Signed-off-by: tabokie <[email protected]>

initialize allowlist for v2

9f454e8

Signed-off-by: tabokie <[email protected]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2023

tabokie mentioned this pull request Apr 17, 2023

raftstore-v2: support renaming encrypted dir (inefficiently) and batch importing data keys #14583

Merged

ti-chi-bot bot closed this May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encryption: support renaming encrypted directory #14095

encryption: support renaming encrypted directory #14095

tabokie commented Jan 30, 2023 •

edited

Loading

ti-chi-bot commented Jan 30, 2023

tabokie commented Jan 30, 2023

tonyxuqqi Jan 30, 2023

tonyxuqqi Jan 30, 2023

tabokie Jan 31, 2023

BusyJay Jan 31, 2023

tabokie Jan 31, 2023

BusyJay Feb 13, 2023

tabokie Feb 13, 2023

tonyxuqqi Mar 2, 2023

tabokie Mar 3, 2023

tonyxuqqi Mar 1, 2023 •

edited

Loading

tonyxuqqi Mar 2, 2023

tabokie Mar 2, 2023

BusyJay commented Apr 4, 2023

tabokie commented Apr 4, 2023

tabokie commented Apr 12, 2023

sre-bot commented Apr 12, 2023

ti-chi-bot commented Apr 13, 2023

tonyxuqqi commented May 17, 2023

ti-chi-bot bot commented May 17, 2023

encryption: support renaming encrypted directory #14095

encryption: support renaming encrypted directory #14095

Conversation

tabokie commented Jan 30, 2023 • edited Loading

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Jan 30, 2023

tabokie commented Jan 30, 2023

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

tonyxuqqi Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Apr 4, 2023

tabokie commented Apr 4, 2023

tabokie commented Apr 12, 2023

sre-bot commented Apr 12, 2023

ti-chi-bot commented Apr 13, 2023

tonyxuqqi commented May 17, 2023

ti-chi-bot bot commented May 17, 2023

tabokie commented Jan 30, 2023 •

edited

Loading

tonyxuqqi Mar 1, 2023 •

edited

Loading