adduser(8): support creation of ZFS dataset #881

jgrafton · 2023-10-31T17:55:04Z

On systems utilizing ZFS, default to creating a ZFS dataset for a new user's home directory if the parent directory resides on a ZFS dataset. Add a flag that disables this behavior if the administrator explicitly does not want it.

If run during installation from within a chroot, set mountpoint to legacy after dataset creation and mount directly into the chroot. Then umount and reset the mountpoint to inherit from parent.

Also support ZFS default encryption on user's home directory.

Requested here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263234

no-usernames-left · 2023-10-31T17:59:30Z

Failed to start an instance: FAILED_PRECONDITION: Monthly compute limit exceeded!

Whoops. Maybe resubmit in a few hours? 😅

usr.sbin/adduser/adduser.8

usr.sbin/adduser/adduser.conf.5

usr.sbin/adduser/adduser.sh

jgrafton · 2023-11-01T21:16:04Z

I simplified the code with your suggestions @delphij. This revision just adds a dataset if the parent resides on a dataset like you mentioned. I'll get to work on adding encryption in a later revision. Thanks again!

jgrafton · 2023-11-08T13:58:31Z

Discovered bug during installation in chrooted environment. The skeleton data for the user's home directory is copied into the underlying user directory instead of the mounted dataset. Marking this draft until I resolve the issue.

kevans91 · 2023-12-28T07:07:28Z

usr.sbin/adduser/adduser.sh

@@ -202,6 +203,8 @@ save_config() {
 	echo "msgfile=$msgfile"		>> ${ADDUSERCONF}
 	echo "disableflag=$disableflag" >> ${ADDUSERCONF}
 	echo "uidstart=$uidstart"       >> ${ADDUSERCONF}
+	echo "Zflag=$Zflag"       >> ${ADDUSERCONF}


I'd maybe call this zfscreate=no, just to be a little more obvious from the name

Good call. I changed both Zencrypt and Zcreate to yes/no bools to remove confusion.

kevans91 · 2023-12-28T07:08:18Z

usr.sbin/adduser/adduser.sh

@@ -202,6 +203,8 @@ save_config() {
 	echo "msgfile=$msgfile"		>> ${ADDUSERCONF}
 	echo "disableflag=$disableflag" >> ${ADDUSERCONF}
 	echo "uidstart=$uidstart"       >> ${ADDUSERCONF}
+	echo "Zflag=$Zflag"       >> ${ADDUSERCONF}
+	echo "Zencrypt=$Zencrypt"       >> ${ADDUSERCONF}


I don't see a manpage addition for Zencrypt, but with a more descriptive name for Zflag you could make that instead a tri-state: zfscreate=yes|no|encrypted

You're right, Zflag is super confusing! I set Zcreate and Zencrypt to yes/no bools but didn't implement the tri-state since the two booleans seem to "flow" better in the script.

kevans91 · 2023-12-28T07:09:27Z

usr.sbin/adduser/adduser.sh

+#	Determine if homeprefix is located on a ZFS filesystem and if
+#	so, enable ZFS home dataset creation.
+#
+get_zfs_home() {


This seems like it would override a configured Zflag=no, but I'm also not sure I understand the logic; zfs list would exit 0 if it found a prefix, and -z "${zfs_homeprefix} would flip Zflag on if we didn't detect a prefix.

There's an additional caveat that using zfs will trigger a load of zfs.ko on purely ufs systems; I'd recommend bailing out before calling it if kldstat -q -m zfs indicates that zfs isn't there

Oh, right, Zflag means "no zfs dataset"... the name is definitely getting me here.

kevans91 · 2023-12-28T07:13:39Z

usr.sbin/adduser/adduser.sh

+			Zencrypt=
+			break
+			;;
+		[Yy][Ee][Ss]|[Yy][Ee]|[Yy])


I think accepting "ye" is a little nonstandard; this should probably just be a case-insensitive "yes" or case-insensitive "y"

I agree, it's a bit strange. However, I followed the user input convention already used in the script.

Huh, indeed- funky

kevans91 · 2023-12-28T07:14:43Z

usr.sbin/adduser/adduser.sh

+		echo -n "$_prompt"
+		read _input
+
+		[ -z "$_input" ] && _input=Zencrypt


The prompt describes "no" as the default, but this seems to be trying to use the current value of Zencrypt as the default? (But it should have been $Zencrypt... this should probably be no to match the prompt, though)

That's a bug, fixed.

kevans91 · 2023-12-28T07:15:30Z

usr.sbin/adduser/adduser.sh

+#
+create_zfs_chrooted_dataset() {
+	${ZFSCMD} create -u ${zfsopt} "${zhome}"
+	if [ "$?" -ne 0 ]; then


More concisely written as:

if ! ${ZFSCMD} create ...

kevans91 · 2023-12-28T07:17:09Z

usr.sbin/adduser/adduser.sh

+#   Create ZFS dataset owned by the user that was just added.
+#
+create_zfs_dataset() {
+	${ZFSCMD} create ${zfsopt} "${zhome}"


Ditto above; you're not using the exit code otherwise, might as well just test it directly

kevans91 · 2023-12-28T07:17:30Z

usr.sbin/adduser/adduser.sh

+#   Give new user ownership of newly created zfs dataset.
+#
+set_zfs_perms() {
+	${ZFSCMD} allow "${username}" create,destroy,mount,snapshot "${zhome}"


Ditto above w/ exit code

kevans91

I think this LGTM- thanks!

jgrafton · 2024-01-02T03:40:49Z

I think this LGTM- thanks!

Thanks Kyle!

igalic · 2024-01-16T13:48:33Z

@kevans91 and go ahead and commit this?

bsdimp · 2024-01-17T16:32:48Z

usr.sbin/adduser/adduser.8

@@ -32,7 +32,7 @@
 .Nd command for adding new users
 .Sh SYNOPSIS
 .Nm
-.Op Fl CDENShq
+.Op Fl CDENSZhq


This man page needs a .Dd bump.

bsdimp · 2024-01-17T16:33:47Z

usr.sbin/adduser/adduser.sh

+	# create ZFS dataset before home directory is created with pw
+	if [ "${Zcreate}" = "yes" ]; then
+		if [ "${Zencrypt}" = "yes" ]; then
+			echo "Enter encryption keyphrase for ZFS dataset (${zhome}):"


This doesn't look like it is prompting for anything or passing the results along, why not?

"zfs create" actually asks for the passphrase when encryption is requested but doesn't give any indication on what it's for. The echo lets the user know what the coming password request is for.

bsdimp · 2024-01-17T16:35:32Z

Just a nit an a quick question

On systems utilizing ZFS, default to creating a ZFS dataset for a new user's home directory if the parent directory resides on a ZFS dataset. Add a flag that disables this behavior if the administrator explicitly does not want it. If run during installation from within a chroot, set mountpoint to legacy after dataset creation and mount directly into the chroot. Then umount and reset the mountpoint to inherit from parent. Also support ZFS default encryption on user's home directory.

bsdimp · 2024-04-11T18:15:39Z

merged. Thanks!

On systems utilizing ZFS, default to creating a ZFS dataset for a new user's home directory if the parent directory resides on a ZFS dataset. Add a flag that disables this behavior if the administrator explicitly does not want it. If run during installation from within a chroot, set mountpoint to legacy after dataset creation and mount directly into the chroot. Then umount and reset the mountpoint to inherit from parent. Also support ZFS default encryption on user's home directory. Feedback by: delphij Reviewed by: imp, kevans Pull Request: #881

On systems utilizing ZFS, default to creating a ZFS dataset for a new user's home directory if the parent directory resides on a ZFS dataset. Add a flag that disables this behavior if the administrator explicitly does not want it. If run during installation from within a chroot, set mountpoint to legacy after dataset creation and mount directly into the chroot. Then umount and reset the mountpoint to inherit from parent. Also support ZFS default encryption on user's home directory. Feedback by: delphij Reviewed by: imp, kevans Pull Request: #881 (cherry picked from commit 215c0a5)

On systems utilizing ZFS, default to creating a ZFS dataset for a new user's home directory if the parent directory resides on a ZFS dataset. Add a flag that disables this behavior if the administrator explicitly does not want it. If run during installation from within a chroot, set mountpoint to legacy after dataset creation and mount directly into the chroot. Then umount and reset the mountpoint to inherit from parent. Also support ZFS default encryption on user's home directory. Feedback by: delphij Reviewed by: imp, kevans Pull Request: freebsd/freebsd-src#881

delphij suggested changes Oct 31, 2023

View reviewed changes

jgrafton force-pushed the zfsadduser branch from d6e0d8e to 11a9dd4 Compare November 1, 2023 21:12

jgrafton marked this pull request as draft November 8, 2023 13:54

jgrafton force-pushed the zfsadduser branch from c325d2b to c1effaa Compare December 7, 2023 20:53

jgrafton marked this pull request as ready for review December 7, 2023 20:58

kevans91 suggested changes Dec 28, 2023

View reviewed changes

kevans91 approved these changes Jan 2, 2024

View reviewed changes

bsdimp reviewed Jan 17, 2024

View reviewed changes

jgrafton force-pushed the zfsadduser branch from 758aa8e to 6982705 Compare January 19, 2024 18:09

grahamperrin mentioned this pull request Feb 23, 2024

[Feature]: Installation option: ZFS-encrypted USER HOME dataset ghostbsd/issues#61

Open

bsdimp closed this Apr 11, 2024

grahamperrin mentioned this pull request Nov 14, 2024

add encryption option: per-user encrypted homdir openzfs/openzfs-docs#379

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adduser(8): support creation of ZFS dataset #881

adduser(8): support creation of ZFS dataset #881

jgrafton commented Oct 31, 2023 •

edited

Loading

no-usernames-left commented Oct 31, 2023 •

edited

Loading

jgrafton commented Nov 1, 2023

jgrafton commented Nov 8, 2023

kevans91 Dec 28, 2023

jgrafton Dec 31, 2023

kevans91 Dec 28, 2023

jgrafton Dec 31, 2023

kevans91 Dec 28, 2023

kevans91 Dec 28, 2023

kevans91 Dec 28, 2023

jgrafton Dec 31, 2023

kevans91 Jan 2, 2024

kevans91 Dec 28, 2023

jgrafton Dec 31, 2023

kevans91 Dec 28, 2023

kevans91 Dec 28, 2023

kevans91 Dec 28, 2023

kevans91 left a comment

jgrafton commented Jan 2, 2024

igalic commented Jan 16, 2024

bsdimp Jan 17, 2024

bsdimp Jan 17, 2024

jgrafton Jan 19, 2024

bsdimp commented Jan 17, 2024

bsdimp commented Apr 11, 2024

adduser(8): support creation of ZFS dataset #881

adduser(8): support creation of ZFS dataset #881

Conversation

jgrafton commented Oct 31, 2023 • edited Loading

no-usernames-left commented Oct 31, 2023 • edited Loading

jgrafton commented Nov 1, 2023

jgrafton commented Nov 8, 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

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

kevans91 left a comment

Choose a reason for hiding this comment

jgrafton commented Jan 2, 2024

igalic commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdimp commented Jan 17, 2024

bsdimp commented Apr 11, 2024

jgrafton commented Oct 31, 2023 •

edited

Loading

no-usernames-left commented Oct 31, 2023 •

edited

Loading