From e635d27ebcee7c130bc56c20f1a66182f2254703 Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Wed, 8 Apr 2026 10:17:40 +1000 Subject: [PATCH] Add ability to set user properties while changing encryption key `zfs change-key` changes the key used to encrypt a ZFS dataset. When used programmatically, it may be useful to track some external state related to the key in a user property. E.g. a generation number, expiration date, or application-specific source of the key. This can be done today by running `zfs set user:prop=value` before or after running `zfs change-key`. However, this introduces a race condition where the property may not be set even though the key has changed, or vice versa (depending on the order the commands are executed). This can be addressed by using a channel program (`zfs program`) which calls both `zfs.sync.change_key()` and `zfs.sync.set_prop()`, changing the property and key atomically. However, it is nontrivial to write such a channel program to handle error cases, and provide the new key securely (e.g. without logging it). This issue proposes to enhance `zfs change-key` to be able to atomically set user properties while changing the encryption key. Currently `zfs change-key` accepts `-o property=value` arguments, but the only valid properties are keylocation, keyformat, and pbkdf2iters. We will enhance this to also allow user properties, e.g. `-o user:prop=value`. User properties will also be allowed when using `zfs change-key -i` to inherit the key from the parent dataset. Original-patch-by: Matthew Ahrens External-issue: https://www.illumos.org/issues/17847 Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18407 --- cmd/zfs/zfs_main.c | 7 +- include/sys/dsl_crypt.h | 3 +- lib/libzfs/libzfs_crypto.c | 52 +++++++++----- man/man8/zfs-load-key.8 | 9 ++- module/zfs/dsl_crypt.c | 15 +++- module/zfs/zfs_ioctl.c | 16 +++-- tests/runfiles/common.run | 3 +- tests/runfiles/sanity.run | 3 +- tests/zfs-tests/tests/Makefile.am | 1 + .../zfs_change-key_userprop.ksh | 72 +++++++++++++++++++ 10 files changed, 146 insertions(+), 35 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_userprop.ksh diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 48e563181fd..631ddda5c6e 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -32,6 +32,7 @@ * Copyright (c) 2019, loli10K * Copyright 2019 Joyent, Inc. * Copyright (c) 2019, 2020 by Christian Schwarz. All rights reserved. + * Copyright 2026 Oxide Computer Company */ #include @@ -8739,12 +8740,6 @@ zfs_do_change_key(int argc, char **argv) } } - if (inheritkey && !nvlist_empty(props)) { - (void) fprintf(stderr, - gettext("Properties not allowed for inheriting\n")); - usage(B_FALSE); - } - argc -= optind; argv += optind; diff --git a/include/sys/dsl_crypt.h b/include/sys/dsl_crypt.h index 1a088b8f3d3..14413072a59 100644 --- a/include/sys/dsl_crypt.h +++ b/include/sys/dsl_crypt.h @@ -200,7 +200,8 @@ void dsl_crypto_recv_raw_key_sync(struct dsl_dataset *ds, int dsl_crypto_recv_raw(const char *poolname, uint64_t dsobj, uint64_t fromobj, dmu_objset_type_t ostype, nvlist_t *nvl, boolean_t do_key); -int spa_keystore_change_key(const char *dsname, dsl_crypto_params_t *dcp); +int spa_keystore_change_key(const char *dsname, dsl_crypto_params_t *dcp, + nvlist_t *userprops); int dsl_dir_rename_crypt_check(dsl_dir_t *dd, dsl_dir_t *newparent); int dsl_dataset_promote_crypt_check(dsl_dir_t *target, dsl_dir_t *origin); void dsl_dataset_promote_crypt_sync(dsl_dir_t *target, dsl_dir_t *origin, diff --git a/lib/libzfs/libzfs_crypto.c b/lib/libzfs/libzfs_crypto.c index f461ad41405..b302718edfa 100644 --- a/lib/libzfs/libzfs_crypto.c +++ b/lib/libzfs/libzfs_crypto.c @@ -17,6 +17,7 @@ /* * Copyright (c) 2017, Datto, Inc. All rights reserved. * Copyright 2020 Joyent, Inc. + * Copyright 2026 Oxide Computer Company */ #include @@ -1536,34 +1537,51 @@ zfs_crypto_unload_key(zfs_handle_t *zhp) static int zfs_crypto_verify_rewrap_nvlist(zfs_handle_t *zhp, nvlist_t *props, - nvlist_t **props_out, char *errbuf) + boolean_t inheritkey, nvlist_t **props_out, char *errbuf) { int ret; nvpair_t *elem = NULL; - zfs_prop_t prop; nvlist_t *new_props = NULL; - new_props = fnvlist_alloc(); - /* * loop through all provided properties, we should only have - * keyformat, keylocation and pbkdf2iters. The actual validation of - * values is done by zfs_valid_proplist(). + * keyformat, keylocation and pbkdf2iters, and user properties. + * The actual validation of values is done by zfs_valid_proplist(). */ while ((elem = nvlist_next_nvpair(props, elem)) != NULL) { const char *propname = nvpair_name(elem); - prop = zfs_name_to_prop(propname); - switch (prop) { + switch (zfs_name_to_prop(propname)) { case ZFS_PROP_PBKDF2_ITERS: case ZFS_PROP_KEYFORMAT: case ZFS_PROP_KEYLOCATION: + if (inheritkey) { + ret = EINVAL; + zfs_error_aux(zhp->zfs_hdl, + dgettext(TEXT_DOMAIN, + "Only user properties may be set with " + "'zfs change-key -i'")); + goto error; + } break; + case ZPROP_INVAL: + if (zfs_prop_user(propname)) + break; + zfs_fallthrough; default: ret = EINVAL; - zfs_error_aux(zhp->zfs_hdl, dgettext(TEXT_DOMAIN, - "Only keyformat, keylocation and pbkdf2iters may " - "be set with this command.")); + if (inheritkey) { + zfs_error_aux(zhp->zfs_hdl, + dgettext(TEXT_DOMAIN, + "Only user properties may be set with " + "'zfs change-key -i'")); + } else { + zfs_error_aux(zhp->zfs_hdl, + dgettext(TEXT_DOMAIN, + "Only keyformat, keylocation, pbkdf2iters, " + "and user properties may be set with this " + "command.")); + } goto error; } } @@ -1642,17 +1660,17 @@ zfs_crypto_rewrap(zfs_handle_t *zhp, nvlist_t *raw_props, boolean_t inheritkey) goto error; } + /* validate the provided properties */ + ret = zfs_crypto_verify_rewrap_nvlist(zhp, raw_props, inheritkey, + &props, errbuf); + if (ret != 0) + goto error; + /* * If the user wants to use the inheritkey variant of this function * we don't need to collect any crypto arguments. */ if (!inheritkey) { - /* validate the provided properties */ - ret = zfs_crypto_verify_rewrap_nvlist(zhp, raw_props, &props, - errbuf); - if (ret != 0) - goto error; - /* * Load keyformat and keylocation from the nvlist. Fetch from * the dataset properties if not specified. diff --git a/man/man8/zfs-load-key.8 b/man/man8/zfs-load-key.8 index 912f55d753b..b0af3553472 100644 --- a/man/man8/zfs-load-key.8 +++ b/man/man8/zfs-load-key.8 @@ -29,8 +29,9 @@ .\" Copyright 2019 Richard Laager. All rights reserved. .\" Copyright 2018 Nexenta Systems, Inc. .\" Copyright 2019 Joyent, Inc. +.\" Copyright 2026 Oxide Computer Company .\" -.Dd July 11, 2022 +.Dd January 30, 2026 .Dt ZFS-LOAD-KEY 8 .Os . @@ -53,6 +54,7 @@ .Op Fl o Ar keylocation Ns = Ns Ar value .Op Fl o Ar keyformat Ns = Ns Ar value .Op Fl o Ar pbkdf2iters Ns = Ns Ar value +.Op Fl o Ar user:prop Ns = Ns Ar value .Ar filesystem .Nm zfs .Cm change-key @@ -157,6 +159,7 @@ Unloads the keys for all encryption roots in all imported pools. .Op Fl o Ar keylocation Ns = Ns Ar value .Op Fl o Ar keyformat Ns = Ns Ar value .Op Fl o Ar pbkdf2iters Ns = Ns Ar value +.Op Fl o Ar user:prop Ns = Ns Ar value .Ar filesystem .Xc .It Xo @@ -173,7 +176,7 @@ This command may also be used to change the .Sy keyformat , and .Sy pbkdf2iters -properties as needed. +properties as needed, as well as set user properties. If the dataset was not previously an encryption root it will become one. Alternatively, the .Fl i @@ -209,7 +212,7 @@ This is effectively equivalent to running .It Fl o Ar property Ns = Ns Ar value Allows the user to set encryption key properties .Pq Sy keyformat , keylocation , No and Sy pbkdf2iters -while changing the key. +and user properties while changing the key. This is the only way to alter .Sy keyformat and diff --git a/module/zfs/dsl_crypt.c b/module/zfs/dsl_crypt.c index 9cb1536642d..9207737f908 100644 --- a/module/zfs/dsl_crypt.c +++ b/module/zfs/dsl_crypt.c @@ -17,6 +17,7 @@ /* * Copyright (c) 2017, Datto, Inc. All rights reserved. * Copyright (c) 2018 by Delphix. All rights reserved. + * Copyright 2026 Oxide Computer Company */ #include @@ -1241,6 +1242,7 @@ dsl_crypto_key_sync(dsl_crypto_key_t *dck, dmu_tx_t *tx) typedef struct spa_keystore_change_key_args { const char *skcka_dsname; dsl_crypto_params_t *skcka_cp; + nvlist_t *skcka_userprops; } spa_keystore_change_key_args_t; static int @@ -1253,6 +1255,8 @@ spa_keystore_change_key_check(void *arg, dmu_tx_t *tx) dsl_crypto_params_t *dcp = skcka->skcka_cp; uint64_t rddobj; + /* we assume skcka_userprops has already been verified */ + /* check for the encryption feature */ if (!spa_feature_is_enabled(dp->dp_spa, SPA_FEATURE_ENCRYPTION)) { ret = SET_ERROR(ENOTSUP); @@ -1539,6 +1543,10 @@ spa_keystore_change_key_sync(void *arg, dmu_tx_t *tx) VERIFY0(dsl_dataset_hold(dp, skcka->skcka_dsname, FTAG, &ds)); ASSERT(!ds->ds_is_snapshot); + /* set user properties */ + dsl_props_set_sync_impl(ds, ZPROP_SRC_LOCAL, skcka->skcka_userprops, + tx); + if (dcp->cp_cmd == DCP_CMD_NEW_KEY || dcp->cp_cmd == DCP_CMD_FORCE_NEW_KEY) { /* @@ -1617,14 +1625,19 @@ spa_keystore_change_key_sync(void *arg, dmu_tx_t *tx) dsl_dataset_rele(ds, FTAG); } +/* + * Note: assumes userprops has already been checked for validity. + */ int -spa_keystore_change_key(const char *dsname, dsl_crypto_params_t *dcp) +spa_keystore_change_key(const char *dsname, dsl_crypto_params_t *dcp, + nvlist_t *userprops) { spa_keystore_change_key_args_t skcka; /* initialize the args struct */ skcka.skcka_dsname = dsname; skcka.skcka_cp = dcp; + skcka.skcka_userprops = userprops; /* * Perform the actual work in syncing context. The blocks modified diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 1f9487f5957..fe98e7db073 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -41,7 +41,7 @@ * Copyright (c) 2019, 2020 by Christian Schwarz. All rights reserved. * Copyright (c) 2019, 2021, 2023, 2024, Klara Inc. * Copyright (c) 2019, Allan Jude - * Copyright 2024 Oxide Computer Company + * Copyright 2026 Oxide Computer Company */ /* @@ -7601,7 +7601,7 @@ zfs_ioc_change_key(const char *dsname, nvlist_t *innvl, nvlist_t *outnvl) int ret; uint64_t cmd = DCP_CMD_NONE; dsl_crypto_params_t *dcp = NULL; - nvlist_t *args = NULL, *hidden_args = NULL; + nvlist_t *props = NULL, *hidden_args = NULL; if (strchr(dsname, '@') != NULL || strchr(dsname, '%') != NULL) { ret = (SET_ERROR(EINVAL)); @@ -7609,14 +7609,20 @@ zfs_ioc_change_key(const char *dsname, nvlist_t *innvl, nvlist_t *outnvl) } (void) nvlist_lookup_uint64(innvl, "crypt_cmd", &cmd); - (void) nvlist_lookup_nvlist(innvl, "props", &args); + (void) nvlist_lookup_nvlist(innvl, "props", &props); (void) nvlist_lookup_nvlist(innvl, ZPOOL_HIDDEN_ARGS, &hidden_args); - ret = dsl_crypto_params_create_nvlist(cmd, args, hidden_args, &dcp); + ret = dsl_crypto_params_create_nvlist(cmd, props, hidden_args, &dcp); if (ret != 0) goto error; - ret = spa_keystore_change_key(dsname, dcp); + /* The keylocation property is set from dcp->cp_keylocation. */ + (void) nvlist_remove_all(props, zfs_prop_to_name(ZFS_PROP_KEYLOCATION)); + + if ((ret = zfs_check_userprops(props)) != 0) + goto error; + + ret = spa_keystore_change_key(dsname, dcp, props); if (ret != 0) goto error; diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 38da750ff1f..e6966600deb 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -190,7 +190,8 @@ tags = ['functional', 'cli_root', 'zfs_bookmark'] [tests/functional/cli_root/zfs_change-key] tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format', 'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location', - 'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones'] + 'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones', + 'zfs_change-key_userprop'] tags = ['functional', 'cli_root', 'zfs_change-key'] [tests/functional/cli_root/zfs_clone] diff --git a/tests/runfiles/sanity.run b/tests/runfiles/sanity.run index f8aa6aed58e..ca16bee67dd 100644 --- a/tests/runfiles/sanity.run +++ b/tests/runfiles/sanity.run @@ -104,7 +104,8 @@ tags = ['functional', 'cli_root', 'zfs_bookmark'] [tests/functional/cli_root/zfs_change-key] tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format', 'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location', - 'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones'] + 'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones', + 'zfs_change-key_userprop'] tags = ['functional', 'cli_root', 'zfs_change-key'] [tests/functional/cli_root/zfs_clone] diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index c438733b6a1..172122ec841 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -666,6 +666,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/cli_root/zfs_change-key/zfs_change-key_load.ksh \ functional/cli_root/zfs_change-key/zfs_change-key_location.ksh \ functional/cli_root/zfs_change-key/zfs_change-key_pbkdf2iters.ksh \ + functional/cli_root/zfs_change-key/zfs_change-key_userprop.ksh \ functional/cli_root/zfs/cleanup.ksh \ functional/cli_root/zfs_clone/cleanup.ksh \ functional/cli_root/zfs_clone/setup.ksh \ diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_userprop.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_userprop.ksh new file mode 100755 index 00000000000..0f6709693bb --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_userprop.ksh @@ -0,0 +1,72 @@ +#!/bin/ksh -p +# SPDX-License-Identifier: CDDL-1.0 +# +# CDDL HEADER START +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# +# CDDL HEADER END +# + +# +# Copyright 2026 Oxide Computer Company +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/cli_root/zfs_load-key/zfs_load-key_common.kshlib + +# +# DESCRIPTION: +# 'zfs change-key -o user:prop=val' should set a user property while changing +# or inheriting the key. +# +# STRATEGY: +# 1. Create a parent encrypted dataset +# 2. Create a child dataset as an encryption root +# 3. Change parent key while setting a user property +# 4. Verify the user property is set on the parent +# 5. Make the child inherit the parent's key while setting a user property +# 6. Verify the user property is set on the child +# + +verify_runnable "both" + +function cleanup +{ + datasetexists $TESTPOOL/$TESTFS1 && \ + log_must zfs destroy -r $TESTPOOL/$TESTFS1 +} +log_onexit cleanup + +log_assert "'zfs change-key -o user:prop=value' should set a user property" + +log_must eval "echo $PASSPHRASE | zfs create -o encryption=on" \ + "-o keyformat=passphrase -o keylocation=prompt $TESTPOOL/$TESTFS1" +log_must eval "echo $PASSPHRASE1 | zfs create -o encryption=on" \ + "-o keyformat=passphrase -o keylocation=prompt" \ + "$TESTPOOL/$TESTFS1/child" + +log_must verify_encryption_root $TESTPOOL/$TESTFS1/child \ + "$TESTPOOL/$TESTFS1/child" + +log_must eval "echo $PASSPHRASE2 | zfs change-key -o user:prop=parentvalue" \ + "$TESTPOOL/$TESTFS1" +log_must eval "zfs get -H -o value user:prop $TESTPOOL/$TESTFS1 | \ + grep -q parentvalue" + +log_must zfs change-key -i -o user:prop=abcd -o user:prop2=efgh \ + $TESTPOOL/$TESTFS1/child +log_must verify_encryption_root $TESTPOOL/$TESTFS1/child "$TESTPOOL/$TESTFS1" +log_must eval "zfs get -H -o value user:prop $TESTPOOL/$TESTFS1/child | \ + grep -q abcd" +log_must eval "zfs get -H -o value user:prop2 $TESTPOOL/$TESTFS1/child | \ + grep -q efgh" + +log_pass "'zfs change-key -o user:prop=value' sets a user property"