Added support for multiple homes in pam_zfs_key module

[dvaerum]

Disclamer: I don't normally code C, so this is a best-effort attempt. There can easily be something I have overlooked when it comes to memory handling. So I am open for feedback and help to improve the code 😁

This implemented support for having multiple datasets unlocked and mounted when a session is opened.
Example: homes=rpool/home,tank/users

Extra unit tests have been added

A man page document has been added man 8 pam_zfs_key (since none existed 😭). A few references to the new man page have also been added in other documents.

Motivation and Context

I have 2 pools on my system, both containing user data, so I want to be able to easily decrypt and mount the user dataset on both pools on user login.
So I have made this patch for the pam_zfs_key module, which worked on my system. So I thought I wanted to share it.
I want to upstream this patch because there may be other people with this problem.

Description

This patch adds support for comma-separated homes= prefixes in pam_zfs_key and makes it so that pam_zfs_key can manage encrypted ZFS datasets across multiple pools. This patch is, as far as I can tell, fully backward compatible with existing PAM configs.

How Has This Been Tested?

I created a VM setup for the development and testing. When I applied the patch on my workstation. For the final test.
I am running NixOS, so I simply applied the patch as an overlay and rebuilt my system.

nixpkgs.overlays = [
  ( final: prev:  let
      zfs-patch = prev.fetchpatch {
        url = "https://github.com/dvaerum/zfs/commit/3c5b105333d862efaff1296ed5cf28ec8f8058cf.patch";
        hash = "sha256-7VsiuxcS1fGRx2WGfk6M0oosjvAuAFrwKnLMj7EvBhg=";
      };
    in {
      zfs = prev.zfs.overrideAttrs (old: {
        patches = (old.patches or [ ]) ++ [ zfs-patch ];
      });
      zfs_2_4 = prev.zfs_2_4.overrideAttrs (old: {
        patches = (old.patches or [ ]) ++ [ zfs-patch ];
      });
      zfs_unstable = prev.zfs_unstable.overrideAttrs (old: {
        patches = (old.patches or [ ]) ++ [ zfs-patch ];
      });
    }
  )
];

I have expanded the existing unittests for the PAM module to also cover multi-homes config to make sure that this feature continues to work, if accepted. However, I may not have covered everything, but if you see missing test scenarios, I will try to add them 😁

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• 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.

[tonyhutter]

I don't use encryption so maybe this is a dumb question - can homes be any dataset, or does it have to be home directories specifically? If it can be any dataset, would it make sense to name it something more generic, like just datasets?

[dvaerum]

I don't use encryption so maybe this is a dumb question - can homes be any dataset, or does it have to be home directories specifically? If it can be any dataset, would it make sense to name it something more generic, like just datasets?

You are bringing up a very good point. So originally homes= would be the dataset which children would be the homes for different users.

So if you have the zpool/HOME when the homes would be zpool/HOME/alice, zpool/HOME/bob and so on zpool/HOME/<username> but you would set homes=zpool/HOME because that is there the homes are "located".

What I have done, is to make it so that you can have multiple "homes" by adding support for a comma separated list like homes=zpool/HOME,extra-pool/HOME,....

You might be right, it would probably make more sense to rename it to something like datasets= instead of homes=. I haven’t really thought about it much, and I’m likely just too used to the current naming to notice 😅
Another reason for me to keep homes= was to keep backward-compatibility with people's config.

Hope all of this helps you understand 😊 Now I got a question, with all of this, would you still keep the name homes=, or would you change it to datasets= ? And if yes, how would you handle backward-compatibility? I have some ideas, but I would like to hear what you thinks

[tonyhutter]

Another reason for me to keep homes= was to keep backward-compatibility with people's config.

Ok, if people have already been using it as homes, then that's good enough reason to keep the name as-is.

[dvaerum]

Ok, if people have already been using it as homes, then that's good enough reason to keep the name as-is.

That is good to hear 😁 Are there other things that catch your eye? I am asking because this is my 1st commit to ZFS, and I am curious if I am on the right track 😅

[tonyhutter]

Is there a danger in going over 16 entries?

/* Maximum number of home prefixes supported in comma-separated homes= */
#define	MAX_HOMES_PREFIXES	16

I ask, because I removed the restriction, and was able to have a homes line with 100 entries (I didn't test higher than that). Here's my changes:

diff

diff --git a/contrib/pam_zfs_key/pam_zfs_key.c b/contrib/pam_zfs_key/pam_zfs_key.c
index b9e12a762..d5513b7a4 100644
--- a/contrib/pam_zfs_key/pam_zfs_key.c
+++ b/contrib/pam_zfs_key/pam_zfs_key.c
@@ -68,9 +68,6 @@ pam_syslog(pam_handle_t *pamh, int loglevel, const char *fmt, ...)
 
 #include <sys/mman.h>
 
-/* Maximum number of home prefixes supported in comma-separated homes= */
-#define        MAX_HOMES_PREFIXES      16
-
 static const char PASSWORD_VAR_NAME[] = "pam_zfs_key_authtok";
 static const char OLD_PASSWORD_VAR_NAME[] = "pam_zfs_key_oldauthtok";
 
@@ -800,10 +797,8 @@ foreach_dataset(pam_handle_t *pamh, zfs_key_config_t *config,
        char *token = strtok_r(homes_copy, ",", &saveptr);
        int success_count = 0;
        boolean_t failed = B_FALSE;
-       int prefix_count = 0;
 
-       while (token != NULL && prefix_count < MAX_HOMES_PREFIXES) {
-               prefix_count++;
+       while (token != NULL) {
                /* Temporarily set homes_prefix to this single prefix */
                config->homes_prefix = token;
                char *dataset = zfs_key_config_get_dataset(pamh, config);
@@ -827,12 +822,6 @@ foreach_dataset(pam_handle_t *pamh, zfs_key_config_t *config,
                token = strtok_r(NULL, ",", &saveptr);
        }
 
-       if (token != NULL) {
-               pam_syslog(pamh, LOG_WARNING,
-                   "MAX_HOMES_PREFIXES (%d) reached, remaining homes ignored",
-                   MAX_HOMES_PREFIXES);
-       }
-
        config->homes_prefix = saved_prefix;
        free(homes_copy);
        pam_syslog(pamh, LOG_DEBUG,
diff --git a/tests/zfs-tests/tests/functional/pam/pam_basic.ksh b/tests/zfs-tests/tests/functional/pam/pam_basic.ksh
index 886e8290c..2d323ecb3 100755
--- a/tests/zfs-tests/tests/functional/pam/pam_basic.ksh
+++ b/tests/zfs-tests/tests/functional/pam/pam_basic.ksh
@@ -87,4 +87,26 @@ keystatus unavailable
 log_mustnot ismounted "$TESTPOOL/pam-multi-home/${username}"
 keystatus_mh unavailable
 
+# Test a 'homes' with many entries
+allhomes="$TESTPOOL/pam-multi-home1"
+for i in {2..$PAM_MULTI_HOME_COUNT} ; do
+       allhomes="$allhomes,$TESTPOOL/pam-multi-home$i"
+done
+
+genconfig "homes=$allhomes runstatedir=${runstatedir}"
+
+echo "testpass" | pamtester ${pamservice} ${username} open_session
+for i in {1..$PAM_MULTI_HOME_COUNT} ; do
+       references 1
+       log_must ismounted "$TESTPOOL/pam-multi-home$i/${username}"
+       keystatus_mh available $i
+done
+
+log_must pamtester ${pamservice} ${username} close_session
+for i in {1..$PAM_MULTI_HOME_COUNT} ; do
+       references 0
+       log_mustnot ismounted "$TESTPOOL/pam-multi-home$i/${username}"
+       keystatus_mh unavailable $i
+done
+
 log_pass "done."
diff --git a/tests/zfs-tests/tests/functional/pam/setup.ksh b/tests/zfs-tests/tests/functional/pam/setup.ksh
index 23e823320..f3a62c0c7 100755
--- a/tests/zfs-tests/tests/functional/pam/setup.ksh
+++ b/tests/zfs-tests/tests/functional/pam/setup.ksh
@@ -43,4 +43,11 @@ echo "testpass" | zfs create -o encryption=aes-256-gcm -o keyformat=passphrase -
 log_must zfs unmount "$TESTPOOL/pam-multi-home/${username}"
 log_must zfs unload-key "$TESTPOOL/pam-multi-home/${username}"
 
+for i in {1..$PAM_MULTI_HOME_COUNT} ; do
+       log_must zfs create -o mountpoint="$TESTDIR-multi-home$i" "$TESTPOOL/pam-multi-home$i"
+       echo "testpass" | zfs create -o encryption=aes-256-gcm -o keyformat=passphrase -o keylocation=prompt "$TESTPOOL/pam-multi-home$i/${username}"
+       log_must zfs unmount "$TESTPOOL/pam-multi-home$i/${username}"
+       log_must zfs unload-key "$TESTPOOL/pam-multi-home$i/${username}"
+done
+
 log_pass
diff --git a/tests/zfs-tests/tests/functional/pam/utilities.kshlib.in b/tests/zfs-tests/tests/functional/pam/utilities.kshlib.in
index 8c922ad08..5c14f5fc9 100644
--- a/tests/zfs-tests/tests/functional/pam/utilities.kshlib.in
+++ b/tests/zfs-tests/tests/functional/pam/utilities.kshlib.in
@@ -28,6 +28,7 @@ runstatedir="${TESTDIR}_run"
 pammodule="@pammoduledir@/pam_zfs_key.so"
 pamservice="pam_zfs_key_test"
 pamconfig="/etc/pam.d/${pamservice}"
+PAM_MULTI_HOME_COUNT=20
 
 function keystatus {
     log_must [ "$(get_prop keystatus "$TESTPOOL/pam/${username}")" = "$1" ]

[dvaerum]

Is there a danger in going over 16 entries?

Not that I know of, and I don't know what that would be 😅. I was just not really sure that allowing "infinity" was a good idea either, so I set 16 as the max because I made the assumption that people would not need more, and if they did, it was easy to change it in the future.

So there's not much more reason behind it when that

[tonyhutter]

Is there a danger in going over 16 entries?

Not that I know of, and I don't know what that would be 😅. I was just not really sure that allowing "infinity" was a good idea either

I would just remove the limit. We should assume that the config->homes_prefix being passed in is valid.

[dvaerum]

Is there a danger in going over 16 entries?

Not that I know of, and I don't know what that would be 😅. I was just not really sure that allowing "infinity" was a good idea either

I would just remove the limit. We should assume that the config->homes_prefix being passed in is valid.

Okay, I have removed the limit and added your unittests 😁

[tonyhutter]

Looks like some of the tests aren't passing now:

    FAIL pam/pam_basic (expected PASS)
    FAIL pam/pam_change_unmounted (expected PASS)
    FAIL pam/pam_nounmount (expected PASS)
    FAIL pam/pam_short_password (expected PASS)

[dvaerum]

Looks like some of the tests aren't passing now:

    FAIL pam/pam_basic (expected PASS)
    FAIL pam/pam_change_unmounted (expected PASS)
    FAIL pam/pam_nounmount (expected PASS)
    FAIL pam/pam_short_password (expected PASS)

Yes, and I believe I have found the problem, I just have not had the time to do anything about it and probably also will not have time the next week. But I will get it fixed 😅