Allow unencrypted children of encrypted datasets

When encryption was first added to ZFS, we made a decision to
prevent users from creating unencrypted children of encrypted
datasets. The idea was to prevent users from inadvertently
leaving some of their data unencrypted. However, since the
release of 0.8.0, some legitimate reasons have been brought up
for this behavior to be allowed. This patch simply removes this
limitation from all code paths that had checks for it and updates
the tests accordingly.

Reviewed-by: Jason King <jason.king@joyent.com>
Reviewed-by: Sean Eric Fagan <sef@ixsystems.com>
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tom Caputi <tcaputi@datto.com>
Closes openzfs#8737
Closes openzfs#8870

include/sys/dsl_crypt.h

@@ -209,7 +209,6 @@ void dsl_dataset_create_crypt_sync(uint64_t dsobj, dsl_dir_t *dd,
     struct dsl_dataset *origin, dsl_crypto_params_t *dcp, dmu_tx_t *tx);
 uint64_t dsl_crypto_key_create_sync(uint64_t crypt, dsl_wrapping_key_t *wkey,
     dmu_tx_t *tx);
-int dmu_objset_clone_crypt_check(dsl_dir_t *parentdd, dsl_dir_t *origindd);
 uint64_t dsl_crypto_key_clone_sync(dsl_dir_t *origindd, dmu_tx_t *tx);
 void dsl_crypto_key_destroy_sync(uint64_t dckobj, dmu_tx_t *tx);
 

lib/libzfs/libzfs_crypto.c

@@ -740,14 +740,6 @@ zfs_crypto_create(libzfs_handle_t *hdl, char *parent_name, nvlist_t *props,
 		pcrypt = ZIO_CRYPT_OFF;
 	}
 
-	/* Check for encryption being explicitly truned off */
-	if (crypt == ZIO_CRYPT_OFF && pcrypt != ZIO_CRYPT_OFF) {
-		ret = EINVAL;
-		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-		    "Invalid encryption value. Dataset must be encrypted."));
-		goto out;
-	}
-
 	/* Get the inherited encryption property if we don't have it locally */
 	if (!local_crypt)
 		crypt = pcrypt;
@@ -849,10 +841,7 @@ int
 zfs_crypto_clone_check(libzfs_handle_t *hdl, zfs_handle_t *origin_zhp,
     char *parent_name, nvlist_t *props)
 {
-	int ret;
 	char errbuf[1024];
-	zfs_handle_t *pzhp = NULL;
-	uint64_t pcrypt, ocrypt;
 
 	(void) snprintf(errbuf, sizeof (errbuf),
 	    dgettext(TEXT_DOMAIN, "Encryption clone error"));
@@ -865,40 +854,12 @@ zfs_crypto_clone_check(libzfs_handle_t *hdl, zfs_handle_t *origin_zhp,
 	    nvlist_exists(props, zfs_prop_to_name(ZFS_PROP_KEYLOCATION)) ||
 	    nvlist_exists(props, zfs_prop_to_name(ZFS_PROP_ENCRYPTION)) ||
 	    nvlist_exists(props, zfs_prop_to_name(ZFS_PROP_PBKDF2_ITERS))) {
-		ret = EINVAL;
 		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
 		    "Encryption properties must inherit from origin dataset."));
-		goto out;
-	}
-
-	/* get a reference to parent dataset, should never be NULL */
-	pzhp = make_dataset_handle(hdl, parent_name);
-	if (pzhp == NULL) {
-		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-		    "Failed to lookup parent."));
-		return (ENOENT);
+		return (EINVAL);
 	}
 
-	/* Lookup parent's crypt */
-	pcrypt = zfs_prop_get_int(pzhp, ZFS_PROP_ENCRYPTION);
-	ocrypt = zfs_prop_get_int(origin_zhp, ZFS_PROP_ENCRYPTION);
-
-	/* all children of encrypted parents must be encrypted */
-	if (pcrypt != ZIO_CRYPT_OFF && ocrypt == ZIO_CRYPT_OFF) {
-		ret = EINVAL;
-		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-		    "Cannot create unencrypted clone as a child "
-		    "of encrypted parent."));
-		goto out;
-	}
-
-	zfs_close(pzhp);
 	return (0);
-
-out:
-	if (pzhp != NULL)
-		zfs_close(pzhp);
-	return (ret);
 }
 
 typedef struct loadkeys_cbdata {

lib/libzfs/libzfs_dataset.c

@@ -4632,16 +4632,9 @@ zfs_rename(zfs_handle_t *zhp, const char *target, boolean_t recursive,
 			    "with the new name"));
 			(void) zfs_error(hdl, EZFS_EXISTS, errbuf);
 		} else if (errno == EACCES) {
-			if (zfs_prop_get_int(zhp, ZFS_PROP_ENCRYPTION) ==
-			    ZIO_CRYPT_OFF) {
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-				    "cannot rename an unencrypted dataset to "
-				    "be a decendent of an encrypted one"));
-			} else {
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-				    "cannot move encryption child outside of "
-				    "its encryption root"));
-			}
+			zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+			    "cannot move encrypted child outside of "
+			    "its encryption root"));
 			(void) zfs_error(hdl, EZFS_CRYPTOFAILED, errbuf);
 		} else {
 			(void) zfs_standard_error(zhp->zfs_hdl, errno, errbuf);

lib/libzfs/libzfs_sendrecv.c

@@ -2827,7 +2827,7 @@ recv_fix_encryption_hierarchy(libzfs_handle_t *hdl, const char *destname,
 		is_clone = zhp->zfs_dmustats.dds_origin[0] != '\0';
 		(void) zfs_crypto_get_encryption_root(zhp, &is_encroot, NULL);
 
-		/* we don't need to do anything for unencrypted filesystems */
+		/* we don't need to do anything for unencrypted datasets */
 		if (crypt == ZIO_CRYPT_OFF) {
 			zfs_close(zhp);
 			continue;
@@ -4210,34 +4210,6 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 			goto out;
 		}
 
-		/*
-		 * It is invalid to receive a properties stream that was
-		 * unencrypted on the send side as a child of an encrypted
-		 * parent. Technically there is nothing preventing this, but
-		 * it would mean that the encryption=off property which is
-		 * locally set on the send side would not be received correctly.
-		 * We can infer encryption=off if the stream is not raw and
-		 * properties were included since the send side will only ever
-		 * send the encryption property in a raw nvlist header. This
-		 * check will be avoided if the user specifically overrides
-		 * the encryption property on the command line.
-		 */
-		if (!raw && rcvprops != NULL &&
-		    !nvlist_exists(cmdprops,
-		    zfs_prop_to_name(ZFS_PROP_ENCRYPTION))) {
-			uint64_t crypt;
-
-			crypt = zfs_prop_get_int(zhp, ZFS_PROP_ENCRYPTION);
-
-			if (crypt != ZIO_CRYPT_OFF) {
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-				    "parent '%s' must not be encrypted to "
-				    "receive unenecrypted property"), name);
-				err = zfs_error(hdl, EZFS_BADPROP, errbuf);
-				zfs_close(zhp);
-				goto out;
-			}
-		}
 		zfs_close(zhp);
 
 		newfs = B_TRUE;
@@ -4274,6 +4246,24 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 	    &oxprops, &wkeydata, &wkeylen, errbuf)) != 0)
 		goto out;
 
+	/*
+	 * When sending with properties (zfs send -p), the encryption property
+	 * is not included because it is a SETONCE property and therefore
+	 * treated as read only. However, we are always able to determine its
+	 * value because raw sends will include it in the DRR_BDEGIN payload
+	 * and non-raw sends with properties are not allowed for encrypted
+	 * datasets. Therefore, if this is a non-raw properties stream, we can
+	 * infer that the value should be ZIO_CRYPT_OFF and manually add that
+	 * to the received properties.
+	 */
+	if (stream_wantsnewfs && !raw && rcvprops != NULL &&
+	    !nvlist_exists(cmdprops, zfs_prop_to_name(ZFS_PROP_ENCRYPTION))) {
+		if (oxprops == NULL)
+			oxprops = fnvlist_alloc();
+		fnvlist_add_uint64(oxprops,
+		    zfs_prop_to_name(ZFS_PROP_ENCRYPTION), ZIO_CRYPT_OFF);
+	}
+
 	err = ioctl_err = lzc_receive_with_cmdprops(destsnap, rcvprops,
 	    oxprops, wkeydata, wkeylen, origin, flags->force, flags->resumable,
 	    raw, infd, drr_noswap, cleanup_fd, &read_bytes, &errflags,

module/zfs/dmu_objset.c

@@ -1348,13 +1348,6 @@ dmu_objset_clone_check(void *arg, dmu_tx_t *tx)
 		return (SET_ERROR(EINVAL));
 	}
 
-	error = dmu_objset_clone_crypt_check(pdd, origin->ds_dir);
-	if (error != 0) {
-		dsl_dataset_rele(origin, FTAG);
-		dsl_dir_rele(pdd, FTAG);
-		return (error);
-	}
-
 	dsl_dataset_rele(origin, FTAG);
 	dsl_dir_rele(pdd, FTAG);
 

module/zfs/dmu_recv.c

@@ -327,7 +327,7 @@ dmu_recv_begin_check(void *arg, dmu_tx_t *tx)
 		/* Open the parent of tofs */
 		ASSERT3U(strlen(tofs), <, sizeof (buf));
 		(void) strlcpy(buf, tofs, strrchr(tofs, '/') - tofs + 1);
-		error = dsl_dataset_hold_flags(dp, buf, dsflags, FTAG, &ds);
+		error = dsl_dataset_hold(dp, buf, FTAG, &ds);
 		if (error != 0)
 			return (error);
 
@@ -345,13 +345,13 @@ dmu_recv_begin_check(void *arg, dmu_tx_t *tx)
 			error = dmu_objset_create_crypt_check(ds->ds_dir,
 			    drba->drba_dcp, &will_encrypt);
 			if (error != 0) {
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (error);
 			}
 
 			if (will_encrypt &&
 			    (featureflags & DMU_BACKUP_FEATURE_EMBED_DATA)) {
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (SET_ERROR(EINVAL));
 			}
 		}
@@ -364,25 +364,25 @@ dmu_recv_begin_check(void *arg, dmu_tx_t *tx)
 		error = dsl_fs_ss_limit_check(ds->ds_dir, 1,
 		    ZFS_PROP_FILESYSTEM_LIMIT, NULL, drba->drba_cred);
 		if (error != 0) {
-			dsl_dataset_rele_flags(ds, dsflags, FTAG);
+			dsl_dataset_rele(ds, FTAG);
 			return (error);
 		}
 
 		error = dsl_fs_ss_limit_check(ds->ds_dir, 1,
 		    ZFS_PROP_SNAPSHOT_LIMIT, NULL, drba->drba_cred);
 		if (error != 0) {
-			dsl_dataset_rele_flags(ds, dsflags, FTAG);
+			dsl_dataset_rele(ds, FTAG);
 			return (error);
 		}
 
 		/* can't recv below anything but filesystems (eg. no ZVOLs) */
 		error = dmu_objset_from_ds(ds, &os);
 		if (error != 0) {
-			dsl_dataset_rele_flags(ds, dsflags, FTAG);
+			dsl_dataset_rele(ds, FTAG);
 			return (error);
 		}
 		if (dmu_objset_type(os) != DMU_OST_ZFS) {
-			dsl_dataset_rele_flags(ds, dsflags, FTAG);
+			dsl_dataset_rele(ds, FTAG);
 			return (SET_ERROR(ZFS_ERR_WRONG_PARENT));
 		}
 
@@ -392,31 +392,31 @@ dmu_recv_begin_check(void *arg, dmu_tx_t *tx)
 			error = dsl_dataset_hold_flags(dp, drba->drba_origin,
 			    dsflags, FTAG, &origin);
 			if (error != 0) {
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (error);
 			}
 			if (!origin->ds_is_snapshot) {
 				dsl_dataset_rele_flags(origin, dsflags, FTAG);
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (SET_ERROR(EINVAL));
 			}
 			if (dsl_dataset_phys(origin)->ds_guid != fromguid &&
 			    fromguid != 0) {
 				dsl_dataset_rele_flags(origin, dsflags, FTAG);
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (SET_ERROR(ENODEV));
 			}
 			if (origin->ds_dir->dd_crypto_obj != 0 &&
 			    (featureflags & DMU_BACKUP_FEATURE_EMBED_DATA)) {
 				dsl_dataset_rele_flags(origin, dsflags, FTAG);
-				dsl_dataset_rele_flags(ds, dsflags, FTAG);
+				dsl_dataset_rele(ds, FTAG);
 				return (SET_ERROR(EINVAL));
 			}
 			dsl_dataset_rele_flags(origin,
 			    dsflags, FTAG);
 		}
 
-		dsl_dataset_rele_flags(ds, dsflags, FTAG);
+		dsl_dataset_rele(ds, FTAG);
 		error = 0;
 	}
 	return (error);

module/zfs/dsl_crypt.c

@@ -1610,15 +1610,8 @@ dsl_dir_rename_crypt_check(dsl_dir_t *dd, dsl_dir_t *newparent)
 	int ret;
 	uint64_t curr_rddobj, parent_rddobj;
 
-	if (dd->dd_crypto_obj == 0) {
-		/* children of encrypted parents must be encrypted */
-		if (newparent->dd_crypto_obj != 0) {
-			ret = SET_ERROR(EACCES);
-			goto error;
-		}
-
+	if (dd->dd_crypto_obj == 0)
 		return (0);
-	}
 
 	ret = dsl_dir_get_encryption_root_ddobj(dd, &curr_rddobj);
 	if (ret != 0)
@@ -1747,34 +1740,6 @@ dsl_dataset_promote_crypt_sync(dsl_dir_t *target, dsl_dir_t *origin,
 	kmem_free(keylocation, ZAP_MAXVALUELEN);
 }
 
-int
-dmu_objset_clone_crypt_check(dsl_dir_t *parentdd, dsl_dir_t *origindd)
-{
-	int ret;
-	uint64_t pcrypt, crypt;
-
-	/*
-	 * Check that we are not making an unencrypted child of an
-	 * encrypted parent.
-	 */
-	ret = dsl_dir_get_crypt(parentdd, &pcrypt);
-	if (ret != 0)
-		return (ret);
-
-	ret = dsl_dir_get_crypt(origindd, &crypt);
-	if (ret != 0)
-		return (ret);
-
-	ASSERT3U(pcrypt, !=, ZIO_CRYPT_INHERIT);
-	ASSERT3U(crypt, !=, ZIO_CRYPT_INHERIT);
-
-	if (crypt == ZIO_CRYPT_OFF && pcrypt != ZIO_CRYPT_OFF)
-		return (SET_ERROR(EINVAL));
-
-	return (0);
-}
-
-
 int
 dmu_objset_create_crypt_check(dsl_dir_t *parentdd, dsl_crypto_params_t *dcp,
     boolean_t *will_encrypt)
@@ -1805,13 +1770,6 @@ dmu_objset_create_crypt_check(dsl_dir_t *parentdd, dsl_crypto_params_t *dcp,
 	ASSERT3U(pcrypt, !=, ZIO_CRYPT_INHERIT);
 	ASSERT3U(crypt, !=, ZIO_CRYPT_INHERIT);
 
-	/*
-	 * We can't create an unencrypted child of an encrypted parent
-	 * under any circumstances.
-	 */
-	if (crypt == ZIO_CRYPT_OFF && pcrypt != ZIO_CRYPT_OFF)
-		return (SET_ERROR(EINVAL));
-
 	/* check for valid dcp with no encryption (inherited or local) */
 	if (crypt == ZIO_CRYPT_OFF) {
 		/* Must not specify encryption params */

tests/zfs-tests/tests/functional/cli_root/zfs_create/zfs_create_encrypted.ksh

@@ -51,10 +51,10 @@
 # yes	unspec	0	1	no	no keyformat specified
 # yes	unspec	1	0	yes	new encryption root, crypt inherited
 # yes	unspec	1	1	yes	new encryption root, crypt inherited
-# yes	off	0	0	no	unencrypted child of encrypted parent
-# yes	off	0	1	no	unencrypted child of encrypted parent
-# yes	off	1	0	no	unencrypted child of encrypted parent
-# yes	off	1	1	no	unencrypted child of encrypted parent
+# yes	off	0	0	yes	unencrypted child of encrypted parent
+# yes	off	0	1	no	keylocation given, but crypt off
+# yes	off	1	0	no	keyformat given, but crypt off
+# yes	off	1	1	no	keyformat given, but crypt off
 # yes	on	0	0	yes	inherited encryption, local crypt
 # yes	on	0	1	no	no keyformat specified for new key
 # yes	on	1	0	yes	new encryption root
@@ -113,7 +113,9 @@ log_must eval "echo $PASSPHRASE | zfs create -o keyformat=passphrase" \
 log_must eval "echo $PASSPHRASE | zfs create -o keyformat=passphrase" \
 	"-o keylocation=prompt $TESTPOOL/$TESTFS2/c4"
 
-log_mustnot zfs create -o encryption=off $TESTPOOL/$TESTFS2/c5
+log_must zfs create -o encryption=off $TESTPOOL/$TESTFS2/c5
+log_must test "$(get_prop 'encryption' $TESTPOOL/$TESTFS2/c5)" == "off"
+
 log_mustnot zfs create -o encryption=off -o keylocation=prompt \
 	$TESTPOOL/$TESTFS2/c5
 log_mustnot zfs create -o encryption=off -o keyformat=passphrase \
@@ -122,13 +124,13 @@ log_mustnot zfs create -o encryption=off -o keyformat=passphrase \
 	-o keylocation=prompt $TESTPOOL/$TESTFS2/c5
 
 log_must eval "echo $PASSPHRASE | zfs create -o encryption=on" \
-	"$TESTPOOL/$TESTFS2/c5"
+	"$TESTPOOL/$TESTFS2/c6"
 log_mustnot zfs create -o encryption=on -o keylocation=prompt \
-	$TESTPOOL/$TESTFS2/c6
+	$TESTPOOL/$TESTFS2/c7
 log_must eval "echo $PASSPHRASE | zfs create -o encryption=on" \
-	"-o keyformat=passphrase $TESTPOOL/$TESTFS2/c6"
+	"-o keyformat=passphrase $TESTPOOL/$TESTFS2/c7"
 log_must eval "echo $PASSPHRASE | zfs create -o encryption=on" \
-	"-o keyformat=passphrase -o keylocation=prompt $TESTPOOL/$TESTFS2/c7"
+	"-o keyformat=passphrase -o keylocation=prompt $TESTPOOL/$TESTFS2/c8"
 
 log_pass "ZFS creates datasets only if they have a valid combination of" \
 	"encryption properties set."