pam: implement a zfs_key pam module #9903

felixdoerre · 2020-01-28T05:53:15Z

Implements a pam module for automatically loading zfs encryption keys for home datasets. Currently the pam module does:

load a zfs key and mounts the dataset when a session opens
unmounts the dataset and unloads the key when the session closes
when the user is logged on and changes the password, the modules
changes the encryption key.
See also implement pam_zfs_key #9886

How Has This Been Tested?

I've tested this locally after compiling with:

CFLAGS += -Wall -pedantic
# Mark libspl and libzfs as system header to disable compiliner warnings inside these headers.
CFLAGS += -isystem /usr/include/libspl -isystem /usr/include/libzfs
# automatically discover other include libraries
CFLAGS += $(shell pkg-config --cflags libzfs)
LDFLAGS += -lcrypto $(shell pkg-config --libs libzfs) -Wl,--as-needed

pam_zfs_key.so: pam_zfs_key.c
        $(CC) $(CPPFLAGS) $(CFLAGS) -fPIC -shared $^ $(LDFLAGS) -o $@

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux 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.

felixdoerre · 2020-01-28T05:59:30Z

@behlendorf do you already have some feedback for this? Do you have some hints on how to include this into the overall building process?
Locally (for testing on debian unstable), I have installed the module like this:

$ ls /lib/x86_64-linux-gnu/security/pam_zfs_key.so 
/lib/x86_64-linux-gnu/security/pam_zfs_key.so
$ cat /usr/share/pam-configs/zfs_key
Name: Unlock zfs datasets for user
Default: yes
Priority: 128
Auth-Type: Additional
Auth:
	optional	pam_zfs_key.so
Session-Interactive-Only: yes
Session-Type: Additional
Session:
	optional	pam_zfs_key.so
Password-Type: Additional
Password:
	optional	pam_zfs_key.so

and then installed it with pam-auth-update into the system-wide pam configuration. Where would we include such config files?

codecov · 2020-01-28T13:03:58Z

Codecov Report

Merging #9903 into master will decrease coverage by 13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9903      +/-   ##
==========================================
- Coverage      79%      66%     -13%     
==========================================
  Files         386      304      -82     
  Lines      122448   105121   -17327     
==========================================
- Hits        97036    69899   -27137     
- Misses      25412    35222    +9810

Flag	Coverage Δ
#kernel	`?`
#user	`66% <ø> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca7ea23...85b5f20. Read the comment docs.

InsanePrawn · 2020-01-28T13:18:06Z

Thanks for tackling this!

Just FYI, I came across this repository recently: https://github.com/BenKerry/zfscrypt

felixdoerre · 2020-01-30T07:59:50Z

I have updated the PAM module to:

do ref-counting when a user opens multiple sessions
do nothing for users with uid < 1000
there is a minimal build-configuration in the automake build system, that at least builds the module. Additional work is needed to install this module correctly.
On Debian this is required for installation:
install the shared library to /lib/<arch-specifier>/security/pam_zfs_key.so
install pam-config-zfs_key to /usr/share/pam-configs/zfs_key, so that this module can be automatically enabled by pam-auth-update.
@behlendorf can you help me with the build system to install this module correctly?
(additionally I've ensured that the code is correctly formatted)

felixdoerre · 2020-01-30T08:37:10Z

It seems most of the build-servers miss pam development headers. On debian they are in libpam0g-dev. How can I ensure they are installed? Or do we want the pam module to be excluded from CI?

behlendorf

On debian they are in libpam0g-dev. How can I ensure they are installed?

I've opened PR openzfs/zfs-buildbot#181 against the zfs-buildbot repository which adds the missing dependency. Can you just confirm that we don't need anything else besides the libpam0g-dev / pam-devel packages?

We definitely want to make sure this gets build by the CI. It would be good if we could also perform some basic testing with it. But I'm not sure how easy it would be to automate this kind of testing.

config/always-pam.m4

contrib/pam_zfs_key/Makefile.am

contrib/pam_zfs_key/pam-config-zfs_key

contrib/pam_zfs_key/pam_zfs_key.c

config/always-pam.m4

contrib/pam_zfs_key/Makefile.am

contrib/pam_zfs_key/pam_zfs_key.c

jengelh · 2020-04-26T14:49:10Z

rpm/generic/zfs.spec.in

+%if 0%{?_pam}
+%{_pammoduledir}/*
+%{_pamconfigsdir}/*
+%endif


Distributions might possibly put these in separate subpackages.

so you would suggest to move them to a libpam_zfs_key and pam_zfs_key package in a new files -n-section

contrib/pam_zfs_key/pam_zfs_key.c

felixdoerre

Thanks @jengelh for your review! I (hopefully) resolved the missing error checks and replied with some quick follow-up questions.

felixdoerre · 2020-04-26T20:26:03Z

contrib/pam_zfs_key/pam_zfs_key.c

+			return (PAM_SERVICE_ERR);
+		}
+		struct sigaction oldact;
+		sigchild_default(&oldact);


The reason for which I needed/added these new processes/forks was that they were required for mounting to work. Otherwise I would receive an error:

mount: only root can use the --no-canonicalize option

as a note: (regarding child processes) libzfs already invokes a child process on linux to do the real mount anyways:

zfs/lib/libzfs/os/linux/libzfs_mount_os.c

Line 302 in 4bc7219

do_mount(const char *src, const char *mntpt, char *opts, int flags)

so we should rewrite the zfsutil code to use libmount1 instead of spawning a real mount process?

felixdoerre · 2020-04-26T20:27:14Z

rpm/generic/zfs.spec.in

+%if 0%{?_pam}
+%{_pammoduledir}/*
+%{_pamconfigsdir}/*
+%endif


so you would suggest to move them to a libpam_zfs_key and pam_zfs_key package in a new files -n-section

plumbeo · 2020-05-20T17:12:23Z

Hi @felixdoerre, could you make unmounting the dataset and unloading the key at the end of the session optional (like pam_fscrypt does, for example)? It’s not the common case and it certainly shouldn’t be the default but It could be useful in some situations.

valpackett · 2020-06-16T11:21:28Z

Would it be possible to use the ZFS passphrase as the only password? i.e. password required pam_zfs_key.so and no password hash in /etc at all, and we're authenticated when the zfs dataset is successfully unlocked?

felixdoerre · 2020-06-16T16:16:03Z

@myfreeweb Although this is currently not implemented, I believe this this could be possible. One would call lzc_load_key with noop = B_TRUE to validate the password. However, I am not really sure how this would be communicated back to pam, and if a hybrid solution is possible (password for system users with uid < 1000 or without a home dataset in /etc/shadow and other passwords disabled in /etc/shadow and managed as zfs dataset keys) and which one would take precedence when both passwords are set.

valpackett · 2020-06-16T16:23:22Z

with noop = B_TRUE to validate the password

Is it currently not unlocking at password stage? If it is, I don't see why it would have to be a separate no-op check, wouldn't it be possible to directly answer "yes" when the unlock has succeeded?

if a hybrid solution is possible

Isn't that just how PAM works in general? I would expect all modules to be queried in config order, until one of them says yes.

felixdoerre · 2020-06-16T22:23:59Z

password checking still needs to work when the dataset is unlocked. I believe you need noop = B_TRUE to check the password when the key is already loaded.

config/user-pam.m4

contrib/Makefile.am

contrib/pam_zfs_key/Makefile.am

contrib/pam_zfs_key/pam_zfs_key.c

behlendorf · 2020-06-19T23:23:49Z

contrib/pam_zfs_key/pam_zfs_key.c

+	}
+	zfs_key_config config;
+	zfs_key_config_load(pamh, &config, argc, argv);
+	if (config.uid < 1000) {


See previous comment.

behlendorf · 2020-06-19T23:25:40Z

contrib/pam_zfs_key/pam_zfs_key.c

+	}
+	zfs_key_config config;
+	zfs_key_config_load(pamh, &config, argc, argv);
+	if (config.uid < 1000) {


See previous comment.

tests/test-runner/bin/zts-report.py

tests/zfs-tests/tests/functional/pam/setup.ksh

behlendorf · 2020-06-19T23:31:40Z

It would be great if you could also rebase this on the latest version of master.

contrib/pam_zfs_key/pam_zfs_key.c

ghost

We can improve the FreeBSD logging later.

currently the pam module does: * load a zfs key and mounts the dataset when a session opens * unmounts the dataset and unloads the key when the session closes * when the user is logged on and changes the password, the modules changes the encryption key. Signed-off-by: Felix Dörre <felix@dogcraft.de> Closes openzfs#9886

This PAM module allows unlocking encrypted user home datasets when logging in (and changing passphrase when changing the account password), see openzfs/zfs#9903 Also supposed to unload the key when the last session for the user is done, but there are EBUSY issues: openzfs/zfs#11222 (comment) Submitted by: Greg V <greg_unrelenting.technology> Reviewed by: mm MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D28018

This PAM module allows unlocking encrypted user home datasets when logging in (and changing passphrase when changing the account password), see openzfs/zfs#9903 Also supposed to unload the key when the last session for the user is done, but there are EBUSY issues: openzfs/zfs#11222 (comment) Submitted by: Greg V <greg_unrelenting.technology> Reviewed by: mm Differential Revision: https://reviews.freebsd.org/D28018 (cherry picked from commit ee21ee1)

@jengelh

Implements a pam module for automatically loading zfs encryption keys for home datasets. The pam module: - loads a zfs key and mounts the dataset when a session opens. - unmounts the dataset and unloads the key when the session closes. - when the user is logged on and changes the password, the module changes the encryption key. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: @jengelh <jengelh@inai.de> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Felix Dörre <felix@dogcraft.de> Closes openzfs#9886 Closes openzfs#9903

@jengelh

Implements a pam module for automatically loading zfs encryption keys for home datasets. The pam module: - loads a zfs key and mounts the dataset when a session opens. - unmounts the dataset and unloads the key when the session closes. - when the user is logged on and changes the password, the module changes the encryption key. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: @jengelh <jengelh@inai.de> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Felix Dörre <felix@dogcraft.de> Closes openzfs#9886 Closes openzfs#9903

This PAM module allows unlocking encrypted user home datasets when logging in (and changing passphrase when changing the account password), see openzfs/zfs#9903 Also supposed to unload the key when the last session for the user is done, but there are EBUSY issues: openzfs/zfs#11222 (comment) Submitted by: Greg V <greg_unrelenting.technology> Reviewed by: mm MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D28018

behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Jan 28, 2020

felixdoerre force-pushed the pam_zfs_key branch from 695790f to 969b790 Compare January 30, 2020 07:51

felixdoerre force-pushed the pam_zfs_key branch from 969b790 to ea435a0 Compare January 30, 2020 08:03

felixdoerre force-pushed the pam_zfs_key branch 2 times, most recently from 6cec28a to f44bdfe Compare February 2, 2020 16:36

felixdoerre force-pushed the pam_zfs_key branch 4 times, most recently from 00ba991 to 7c05a38 Compare February 8, 2020 23:55

felixdoerre marked this pull request as ready for review February 9, 2020 06:32

behlendorf requested changes Feb 14, 2020

View reviewed changes

felixdoerre force-pushed the pam_zfs_key branch from a365482 to d3c5f7e Compare February 15, 2020 12:33

felixdoerre requested a review from behlendorf February 15, 2020 17:24

felixdoerre force-pushed the pam_zfs_key branch 4 times, most recently from 765440f to 85b5f20 Compare February 20, 2020 21:22

rlaager reviewed Mar 5, 2020

View reviewed changes

contrib/pam_zfs_key/pam_zfs_key.c Outdated Show resolved Hide resolved

felixdoerre force-pushed the pam_zfs_key branch 2 times, most recently from bcadf82 to 56487ff Compare March 8, 2020 10:33

jengelh reviewed Apr 26, 2020

View reviewed changes

felixdoerre force-pushed the pam_zfs_key branch 2 times, most recently from 9629bd0 to 0b0e902 Compare April 26, 2020 20:11

felixdoerre commented Apr 26, 2020

View reviewed changes

felixdoerre force-pushed the pam_zfs_key branch 3 times, most recently from 44c47d6 to 78dc01b Compare June 2, 2020 09:02

felixdoerre force-pushed the pam_zfs_key branch 2 times, most recently from bf36043 to 555424c Compare June 7, 2020 23:46

behlendorf reviewed Jun 19, 2020

View reviewed changes

felixdoerre force-pushed the pam_zfs_key branch from 555424c to fda154e Compare June 20, 2020 08:57

ghost reviewed Jun 20, 2020

View reviewed changes

contrib/pam_zfs_key/pam_zfs_key.c Outdated Show resolved Hide resolved

felixdoerre force-pushed the pam_zfs_key branch from fda154e to a4b30e0 Compare June 20, 2020 16:31

behlendorf approved these changes Jun 22, 2020

View reviewed changes

ghost approved these changes Jun 22, 2020

View reviewed changes

felixdoerre force-pushed the pam_zfs_key branch from a4b30e0 to e871f3d Compare June 22, 2020 20:37

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Design Review Needed Architecture or design is under discussion labels Jun 23, 2020

behlendorf mentioned this pull request Jun 23, 2020

Mount generator fixes #10477

Closed

12 tasks

behlendorf merged commit 221e670 into openzfs:master Jun 25, 2020

felixdoerre mentioned this pull request Nov 21, 2020

pam_zfs_key failures lead to password prompts for root su #11222

Closed

rptb1 mentioned this pull request Mar 19, 2023

Coverage is not checked by CI Ravenbrook/mps#139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pam: implement a zfs_key pam module #9903

pam: implement a zfs_key pam module #9903

felixdoerre commented Jan 28, 2020 •

edited

Loading

felixdoerre commented Jan 28, 2020

codecov bot commented Jan 28, 2020 •

edited

Loading

InsanePrawn commented Jan 28, 2020

felixdoerre commented Jan 30, 2020 •

edited

Loading

felixdoerre commented Jan 30, 2020

behlendorf left a comment

jengelh Apr 26, 2020

felixdoerre Apr 26, 2020

felixdoerre left a comment

felixdoerre Apr 26, 2020

felixdoerre Apr 26, 2020

plumbeo commented May 20, 2020

valpackett commented Jun 16, 2020 •

edited

Loading

felixdoerre commented Jun 16, 2020

valpackett commented Jun 16, 2020

felixdoerre commented Jun 16, 2020

behlendorf Jun 19, 2020

behlendorf Jun 19, 2020

behlendorf commented Jun 19, 2020

ghost left a comment

pam: implement a zfs_key pam module #9903

pam: implement a zfs_key pam module #9903

Conversation

felixdoerre commented Jan 28, 2020 • edited Loading

How Has This Been Tested?

Types of changes

Checklist:

felixdoerre commented Jan 28, 2020

codecov bot commented Jan 28, 2020 • edited Loading

Codecov Report

InsanePrawn commented Jan 28, 2020

felixdoerre commented Jan 30, 2020 • edited Loading

felixdoerre commented Jan 30, 2020

behlendorf left a comment

Choose a reason for hiding this comment

jengelh Apr 26, 2020

Choose a reason for hiding this comment

felixdoerre Apr 26, 2020

Choose a reason for hiding this comment

felixdoerre left a comment

Choose a reason for hiding this comment

felixdoerre Apr 26, 2020

Choose a reason for hiding this comment

felixdoerre Apr 26, 2020

Choose a reason for hiding this comment

plumbeo commented May 20, 2020

valpackett commented Jun 16, 2020 • edited Loading

felixdoerre commented Jun 16, 2020

valpackett commented Jun 16, 2020

felixdoerre commented Jun 16, 2020

behlendorf Jun 19, 2020

Choose a reason for hiding this comment

behlendorf Jun 19, 2020

Choose a reason for hiding this comment

behlendorf commented Jun 19, 2020

ghost left a comment

Choose a reason for hiding this comment

felixdoerre commented Jan 28, 2020 •

edited

Loading

codecov bot commented Jan 28, 2020 •

edited

Loading

felixdoerre commented Jan 30, 2020 •

edited

Loading

valpackett commented Jun 16, 2020 •

edited

Loading