Allow block cloning across encrypted datasets. Rebase #14705 to master, added tests. #15544

oromenahar · 2023-11-19T01:25:45Z

Block cloning is now possible between two encrypted datasets that share the same encryption root.
This is a rebase of #14705 from @pjd and new tests. Sorry don't know how to add new commits to the old PR.

Motivation and Context

Tests are important for this feature. And we can fix a lot with good tests, before handling it to any user.

Description

The tests are described in the commit message.
There is a problem, with childs and siblings. They don't share the same encryption root. Every master key contains a key, an init Vector and a hmac. I guess that the init Vector and/or hmac changes when creating a child. But wanna throw this against the full pipeline. I'm working currently on encryption and key handling, but there is not that much time for me. Until I figure this out, it's just a quick guess without any deep dive and I wanna see the pipeline results.

Missing tests

If I forgot to test something just describe the test and I'm going to add it later.

cloning from non-encrypted to encrypted datasets
cloning from encrypted to non-encrypted datasets
cloning from a snapshot dir to a filesystem that is not the source of that snapshot
cloning from a filesystem created from a snapshot clone to another unrelated dataset
moving dataset to a new encryption root
And so on. Probably some positive versions of these too.
send | recv (change key) -> clone

How Has This Been Tested?

By running the new test for this.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

oromenahar · 2023-11-19T01:28:32Z

The important case to recover a file from a snapshot is working. So happy recovering files from snapshots.

robn · 2023-11-19T03:07:29Z

No. See analysis in #14705 (comment) . Cloning encrypted blocks is only possible if they share the same master key, which it can only be between a dataset, its snapshots, and cloned datasets created from those snapshots. The encryption root is irrelevant, as that's only the holder of the wrapping key used to unlock the master key (datasets can be moved to another encryption root).

mmatuska · 2023-11-19T11:31:53Z

We cannot check against the common encryption root.

Here is a test that makes this not work:

zfs create -o mountpoint=none -o encryption=on -o keyformat=passphrase rpool/ENC1
zfs create rpool/ENC1/ds1
zfs create -o mountpoint=none -o encryption=on -o keyformat=passphrase rpool/ENC2
zfs create rpool/ENC2/ds2
zfs change-key -o keyformat=passphrase rpool/ENC2/ds2
zfs rename rpool/ENC2/ds2 rpool/ENC1/ds2
zfs change-key -i rpool/ENC1/ds2

Now the datasets rpool/ENC1/ds1 and rpool/ENC1/ds2 are under the same encryption root but have different master keys.
Now just copy_file_range(2) a file from ds1 to ds2 and you get a read error when reading the file from ds2.

The safest way would be to compare master keys (or check if the same master key is used), the second safest to allow only snapshots and clones of the same dataset because they have to use the same key.

scineram · 2023-11-19T11:53:27Z

Encrypted dedup already works only within clone family (same master key). How does the check for that work?

mmatuska · 2023-11-19T13:56:09Z

@oromenahar @robn I have created #15545 that does the right job IMO

oromenahar · 2023-11-19T14:52:37Z

I have updated my PR with @mmatuska code. The tests are sucessfull.
There is a function called spa_crypto_key_compare I think we should use this to compare the crypto_keys.

mmatuska · 2023-11-19T21:59:05Z

I have updated my PR with @mmatuska code. The tests are sucessfull. There is a function called spa_crypto_key_compare I think we should use this to compare the crypto_keys.

I agree we can use spa_crypto_key_compare().
But this functions returns -1, 0 or 1, so we should probably change:

if (spa != dmu_objset_spa(osb))
	return (1);

to this:

if (spa != dmu_objset_spa(osb))
	return (SET_ERROR(EINVAL));

For debugging purposes.

oromenahar · 2023-11-19T23:27:58Z

if (spa != dmu_objset_spa(osb))
	return (1);

do we even need to check if the spa is the same? And I would say even if they are not the same, the crypto key can't be the same, so the comparing is doing what I expect. Telling me, thats different doesn't matter on what condition. It don't tells you exactly why it's not the same. But this can have several reasons.

If I would change something, it would be

	ret = spa_keystore_lookup_key(spa, obja, FTAG, &dcka);
	if (ret != 0)
		return (ret);

to

	ret = spa_keystore_lookup_key(spa, obja, FTAG, &dcka);
	if (ret != 0)
		return (1);

Or even change it to boolean_t as a return value. Which is just true or false without any error.

All of this is not how the return values of for example memcmp function works but the closest possible solution.

I'm curious to receive your feedback @mmatuska .

robn · 2023-11-19T23:53:30Z

I am extremely nervous about getting this wrong, so I would like to see a lot of negative tests as well, for things that shouldn't work:

cloning from non-encrypted to encrypted datasets
cloning from encrypted to non-encrypted datasets
cloning from a snapshot dir to a filesystem that is not the source of that snapshot
cloning from a filesystem created from a snapshot clone to another unrelated dataset
And so on. Probably some positive versions of these too.

tests/zfs-tests/tests/functional/block_cloning/block_cloning.kshlib

oromenahar · 2023-11-20T07:32:21Z

Good point for the test. Thanks for the suggestions, add it later.

man/man7/zpool-features.7

module/zfs/dsl_crypt.c

Majiir · 2023-11-20T19:47:57Z

module/zfs/dsl_crypt.c

+		return (ret);
+	}
+
+	ret = spa_crypto_key_compare(dcka, dckb);


This checks that the datasets have the same dsl_crypto_key, which includes the wrapping key. I think that means two datasets with the same master key can have different dsl_crypto_key if one of them had its encryption root changed.

We could instead compare zk_guid which should match as long as the underlying master key is the same. I have a branch with this check, I'll clean it up and share. We should also add a test around this.

6001dad switches the check to zk_guid and also cleans up the comparison logic according to other review comments.

I'm working on test cases. So far, I have found that performing a raw send into the same pool and then changing the key of one of the datasets is a reasonable way to end up with two different datasets with different encryption roots and the same master key.

@Majiir I'm not sure actually. Changing the raw "master" key should be possible unless reencryption/changing the master key will be implemented. spa_crypto_key_compare uses the dck_obj wich should be unique for every master key. This represents the ID of ZAP object. I think this should be the same for a shared master key, shouldn't it?

Oh than the ZAP-ID could be different. Maybe you are right. Than spa_crypto_key_compare wouldn't return the truth, if you are right? Not sure, going to debug it in the next days. Thanks for the implementation and the commit.

The on-disk structure is the wrapping key, not the master key. It's possible in principle for the same master key to be stored in multiple wrapping keys. In those cases, we would expect dck_obj to differ but the master key (and zk_guid) to match.

In testing, I verified with the send | recv+change-key scenario that using spa_crypto_key_compare does not allow block cloning, while it works using the commit I linked above.

While building tests for this, I ran into a bug where ZFS crashes after send | recv+change-key+cp from another encrypted dataset. Crash log is here.

I still need to get the ZFS test suite running, but for now I have a reproducer in the form of a NixOS VM test. You don't need NixOS to run it, just the Nix package manager, which can run on your (Linux) distro of choice. To run it:

nix run github:Majiir/nixpkgs/35f0c1ad00cf4d72eeb7ffcc78e2e2a1037c6a07#nixosTests.zfs-block-cloning.stable.driver

You can find the test script here.

I tried applying #15543 and it didn't fix the issue.

@Majiir do you tested if the wrapping key is different when using send | recv + something....?
If this is so I would a hint to the original crypto_compare_function used a few minutes ago.

How did you crash this? Are you waiting for the sync before doing anything? Do you think it's related to this PR? Was sick last weekend and didn't had time to test/read your nix.

The crash no longer occurs on latest master (8ad73bf at time of writing) and block cloning works between the original filesystem and the send/recv'd one. 🎉

oromenahar · 2023-11-21T01:11:35Z

I'm added a list the the first post which contains the missing tests and will later contains a tests wich are implemented. I'm going to update this list when adding a test or somebody suggest a test. I will push a new version of this, including an equal-function (@amotin suggestion), when I added tests, and the discussion with the documentation is finished. Until than I need a nap.

man/man7/zpool-features.7

amotin

Looks good to me. Just fix checkstyle and few comments.

module/zfs/dsl_crypt.c

oromenahar · 2023-11-29T19:17:45Z

I think we covered a lot of thoughts on three different PRs while working on encrypted cross clone. I prefer if this is merged to master. I want to create an issue to collect different possible test scenarios. I will than create a PR and update the new PR with the tests from time to time. Not all of the tests named above is written by now. But I want that people can see this issue and provide ideas. I would mange this a little bit.

All of this different scenarios must not be based on encrypted cross clone and wouldn't be super related to this PR. I covered some of the important scenarios with test. (hope so)
All of them are working as expected.

Do you think we can merge this @behlendorf and @amotin ?

tests/zfs-tests/tests/functional/block_cloning/block_cloning_cross_enc_dataset.ksh

oromenahar · 2023-11-29T21:48:00Z

@robn can you please check the test block-cloning kshlib? I changed it to the cross-plattform md5digest as well, and this is your code.

man/man7/zpool-features.7

module/zfs/zfs_vnops.c

When two datasets share the same master encryption key, it is safe to clone encrypted blocks. Currently only snapshots and clones of a dataset share with it the same encryption key. Signed-off-by: Kay Pedersen <[email protected]> Original-patched-by: Pawel Jakub Dawidek <[email protected]>

Added a test for: - Clone from encrypted sibling to encrypted sibling with non encrypted parent - Clone from encrypted parent to inherited encrypted child - Clone from child to sibling with encrypted parent - Clone from snapshot to the original datasets - Clone from foreign snapshot to a foreign dataset Signed-off-by: Kay Pedersen <[email protected]>

- cloning from non-encrypted to encrypted datasets - cloning from encrypted to non-encrypted datasets Signed-off-by: Kay Pedersen <[email protected]>

When two datasets share the same master encryption key, it is safe to clone encrypted blocks. Currently only snapshots and clones of a dataset share with it the same encryption key. Added a test for: - Clone from encrypted sibling to encrypted sibling with non encrypted parent - Clone from encrypted parent to inherited encrypted child - Clone from child to sibling with encrypted parent - Clone from snapshot to the original datasets - Clone from foreign snapshot to a foreign dataset - Cloning from non-encrypted to encrypted datasets - Cloning from encrypted to non-encrypted datasets Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Original-patch-by: Pawel Jakub Dawidek <[email protected]> Signed-off-by: Kay Pedersen <[email protected]> Closes openzfs#15544

When two datasets share the same master encryption key, it is safe to clone encrypted blocks. Currently only snapshots and clones of a dataset share with it the same encryption key. Added a test for: - Clone from encrypted sibling to encrypted sibling with non encrypted parent - Clone from encrypted parent to inherited encrypted child - Clone from child to sibling with encrypted parent - Clone from snapshot to the original datasets - Clone from foreign snapshot to a foreign dataset - Cloning from non-encrypted to encrypted datasets - Cloning from encrypted to non-encrypted datasets Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Original-patch-by: Pawel Jakub Dawidek <[email protected]> Signed-off-by: Kay Pedersen <[email protected]> Closes #15544

oromenahar mentioned this pull request Nov 19, 2023

Allow block cloning across encrypted datasets. #14705

Closed

13 tasks

oromenahar mentioned this pull request Nov 19, 2023

Allow block cloning across encrypted datasets #15545

Closed

13 tasks

oromenahar force-pushed the brt-crypt branch from 0abf958 to 5521dc2 Compare November 19, 2023 14:50

oromenahar force-pushed the brt-crypt branch from 5521dc2 to b381b36 Compare November 19, 2023 15:18

robn reviewed Nov 19, 2023

View reviewed changes

tests/zfs-tests/tests/functional/block_cloning/block_cloning.kshlib Show resolved Hide resolved

AllKind reviewed Nov 20, 2023

View reviewed changes

man/man7/zpool-features.7 Show resolved Hide resolved

amotin reviewed Nov 20, 2023

View reviewed changes

module/zfs/dsl_crypt.c Outdated Show resolved Hide resolved

Majiir reviewed Nov 20, 2023

View reviewed changes

behlendorf added Status: Code Review Needed Ready for review and testing Component: Encryption "native encryption" feature labels Nov 27, 2023

oromenahar force-pushed the brt-crypt branch from b381b36 to 1b0d7d8 Compare November 28, 2023 22:28

AllKind reviewed Nov 29, 2023

View reviewed changes

man/man7/zpool-features.7 Outdated Show resolved Hide resolved

amotin approved these changes Nov 29, 2023

View reviewed changes

module/zfs/dsl_crypt.c Outdated Show resolved Hide resolved

oromenahar force-pushed the brt-crypt branch from 1b0d7d8 to c8998f7 Compare November 29, 2023 19:10

amotin reviewed Nov 29, 2023

View reviewed changes

tests/zfs-tests/tests/functional/block_cloning/block_cloning_cross_enc_dataset.ksh Show resolved Hide resolved

amotin reviewed Nov 29, 2023

View reviewed changes

tests/zfs-tests/tests/functional/block_cloning/block_cloning_cross_enc_dataset.ksh Outdated Show resolved Hide resolved

oromenahar force-pushed the brt-crypt branch from c8998f7 to a51b6d9 Compare November 29, 2023 21:37

oromenahar requested a review from robn November 29, 2023 21:46

behlendorf approved these changes Dec 1, 2023

View reviewed changes

man/man7/zpool-features.7 Outdated Show resolved Hide resolved

module/zfs/zfs_vnops.c Show resolved Hide resolved

oromenahar added 3 commits December 1, 2023 21:31

Added more block cross clone test:

641f06c

- cloning from non-encrypted to encrypted datasets - cloning from encrypted to non-encrypted datasets Signed-off-by: Kay Pedersen <[email protected]>

oromenahar force-pushed the brt-crypt branch from a51b6d9 to 641f06c Compare December 1, 2023 20:31

This was referenced Dec 1, 2023

ZTS: Add Test Case for Block Cloning Replay #15614

Closed

ZTS block cloning coverage #15622

Open

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 1, 2023

behlendorf merged commit c7b6119 into openzfs:master Dec 5, 2023
21 of 26 checks passed

oromenahar deleted the brt-crypt branch December 9, 2023 14:01

mmatuska mentioned this pull request Dec 26, 2023

[2.2] BRT and other fixes into 2.2.3-staging #15714

Merged

13 tasks

sethtroisi mentioned this pull request Apr 5, 2024

PANIC: unencrypted block in encrypted object set #16065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow block cloning across encrypted datasets. Rebase #14705 to master, added tests. #15544

Allow block cloning across encrypted datasets. Rebase #14705 to master, added tests. #15544

oromenahar commented Nov 19, 2023 •

edited

Loading

oromenahar commented Nov 19, 2023

robn commented Nov 19, 2023

mmatuska commented Nov 19, 2023 •

edited

Loading

scineram commented Nov 19, 2023

mmatuska commented Nov 19, 2023

oromenahar commented Nov 19, 2023 •

edited

Loading

mmatuska commented Nov 19, 2023

oromenahar commented Nov 19, 2023 •

edited

Loading

robn commented Nov 19, 2023

oromenahar commented Nov 20, 2023

Majiir Nov 20, 2023

Majiir Nov 21, 2023

oromenahar Nov 21, 2023

oromenahar Nov 21, 2023

Majiir Nov 21, 2023

Majiir Nov 21, 2023

oromenahar Nov 28, 2023

Majiir Dec 9, 2023

oromenahar commented Nov 21, 2023

amotin left a comment

oromenahar commented Nov 29, 2023

oromenahar commented Nov 29, 2023

Allow block cloning across encrypted datasets. Rebase #14705 to master, added tests. #15544

Allow block cloning across encrypted datasets. Rebase #14705 to master, added tests. #15544

Conversation

oromenahar commented Nov 19, 2023 • edited Loading

Motivation and Context

Description

Missing tests

How Has This Been Tested?

Types of changes

Checklist:

oromenahar commented Nov 19, 2023

robn commented Nov 19, 2023

mmatuska commented Nov 19, 2023 • edited Loading

scineram commented Nov 19, 2023

mmatuska commented Nov 19, 2023

oromenahar commented Nov 19, 2023 • edited Loading

mmatuska commented Nov 19, 2023

oromenahar commented Nov 19, 2023 • edited Loading

robn commented Nov 19, 2023

oromenahar commented Nov 20, 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

oromenahar commented Nov 21, 2023

amotin left a comment

Choose a reason for hiding this comment

oromenahar commented Nov 29, 2023

oromenahar commented Nov 29, 2023

oromenahar commented Nov 19, 2023 •

edited

Loading

mmatuska commented Nov 19, 2023 •

edited

Loading

oromenahar commented Nov 19, 2023 •

edited

Loading

oromenahar commented Nov 19, 2023 •

edited

Loading