Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPM: Split out pam_zfs_key into separate package #13026

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

ColMelvin
Copy link
Contributor

Motivation and Context

The RPM packages provided at http://download.zfsonlinux.org/fedora/$releasever/$basearch/ do not include the pam_zfs_key PAM module. To get this feature, the zfs source RPM may be rebuilt --with pam; however, this approach requires rebuilding the zfs RPM for every minor version update.

The PAM module is stable and does not need to be rebuilt for every version of ZFS. By splitting it out into a separate RPM, it can be built and installed much less frequently on a system that receives regular ZFS updates from the official channels.

Description

Create a separate pam_zfs_key package for the PAM module components, an optional addition to the deliverables, in much the same way as the Python bindings are released as a separate python#-pyzfs package.

The module's name follows Fedora precedent for RPM packages providing PAM modules.

How Has This Been Tested?

Test Environment: Fedora 34 Server on VM

Steps:

  1. Installed zfs RPM from upstream Yum repository
  2. Downloaded zfs source RPM from upstream Yum repository, apply patch, and build new RPM (--with pam)
  3. Installed pam_zfs_key RPM package
  4. Configured system to use pam_zfs_key.so PAM module
  5. Rebooted and logged in
  6. Verified pam_zfs_key.so PAM module performed as expected

Further steps checking for regression of #13001:

  1. Uninstalled pam-devel RPM
  2. Built patched RPM without --with pam
  3. Verified no pam_zfs_key RPM was created
  4. Built patched RPM --with pam
  5. Verified RPM build failure from missing dependency

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

ColMelvin added a commit to ColMelvin/zfs that referenced this pull request Jan 28, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
@behlendorf
Copy link
Contributor

Splitting the pam components in to their own package which can then be optionally installed seems pretty reasonable. Looking around briefly, it appears the convention for this would be to name the new package something like zfs-pam-<version>. We could then provide these packages from the official repositories.

Alternately, we could enable this support in the provided packages. This was originally not done out of an abundance of caution when the pam support was new, but that is something we could revisit. What would be the consequences, if any, of enabling this in the shipped packages?

@mskarbek
Copy link
Contributor

mskarbek commented Feb 3, 2022

+1 for zfs-pam name convention.

@behlendorf behlendorf added Component: Packaging custom packages Status: Code Review Needed Ready for review and testing labels Feb 4, 2022
@ColMelvin
Copy link
Contributor Author

Impact of Inclusion

As I've come to understand PAM, the inclusion of %{_libdir}/security/pam_zfs_key.so in the RPM will have no impact on a system unless the system is configured to use pam_zfs_key.so in one of its PAM configuration files (/etc/pam or /etc/pam.d/*). I would expect any distribution to only include the PAM modules they ship in their default PAM configuration(s). Thus, I don't see how including this file in the zfs RPM would cause an issue on a default system.

That said, we also include %{_datadir}/pam-configs/zfs_key in the RPM, which is part of the PAMConfigFrameworkSpec. I know RedHat and Fedora do not use this, but instead prefer authselect, so they would not be impacted. I have not looked into what other distributions use, but I believe this framework depends upon debconfig, so it would seem relegated to Debian-based flavors of Linux and those flavors eschew RPMs, usually. Therefore, this file might also have no impact on a default system (for supported distros).

Assuming pam_zfs_key.so is included in a PAM configuration, the impact would depend on how the system administrator configured the module for the PAM service. If the module session is configured as optional (as is recommended), then any issues would be logged by syslog but logins would not be impeded. If configured as requisite, required or similar, then any issue in the PAM module would prevent a session from opening, which would be bad. I know of one edge case, where logging in with an SSH key instead of a password causes an error in pam_zfs_key.so (see also #13050).

Possible Alteration

We could remove %{_datadir}/pam-configs/zfs_key from the package, which would likely eliminate any risk of accidental configuration. I don't think it would be necessary given what I've discovered on Red Hat systems; however, I haven't looked into every distribution supported by OpenZFS packages (including Debian-based ones). Regardless, distributing only the shared library could provide a middle ground between caution and availability for a default install of ZFS.

Package Scope

Splitting out a separate package would capture everything related to PAM in one place and would provide a separation between ZFS and its addon. There may be minor maintenance benefits to this (though we already section off the PAM components via %if %{with pam}), but I think it would help with dependent packages.

For instance, I am developing an SELinux policy for PAM (but not for ZFS as a whole). I don't know if it would be better to create a separate selinux package or if it would be worthwhile to include the policy with the pam_zfs_key.so library. Regardless, I do think it's worthwhile to separate the SELinux work from the zfs RPM, as it is explicitly not a policy for ZFS as a whole.

Naming Convention

I checked the Guidelines for Naming Fedora Packages document for guidance. As I read it, the Addon Packages section prefers the pam_$module_name format over the $package-pam format. Actually, it almost looks like they expect the pam_$module_name format, but then there are a handful of packages named $package-pam in their official repositories. I'm not sure what differentiates them.

I originally went with pam_zfs_key because I saw more packages with that format than with the zfs-pam format on Fedora 35. I'm open to following another documented convention used by (at least some of) the impacted distros.

@behlendorf
Copy link
Contributor

Thanks for the careful analysis above. Given that, I think you're right, let's go ahead and split the pam bits off in to their own package. This way we can at least clearly express dependencies on it, either way shouldn't really change the overall maintenance burden. Once we have it split in to it's own package we'll start building it by default and including it in the repos.

there are a handful of packages named $package-pam in their official repositories. I'm not sure what differentiates them.

According to the referenced documentation it sounds to me like those packages were probably grandfathered in. But since there is an established convention, I agree let's make sure to follow it. As I read it, that would mean we should name the package pam-zfs though. Unless the _key part is part of the convention for pam packages I missed?

Let's also get @felixdoerre's through on this.

@felixdoerre
Copy link
Contributor

Splitting pam_zfs_key into a separate package and building it by default seems like a good idea. Is there any configuration template required for authselect to work naturally? Like the one for the PAMConfigFramework? If so, we could include that as well.
On debian the pam module is already in its own package (libpam-zfs).

@ColMelvin
Copy link
Contributor Author

I'm not well versed in authselect, but reading man 5 authselect-profiles, I get the impression that the best way to support it would be to add additional lines to the upstream profiles (i.e. https://github.com/authselect/authselect/tree/master/profiles). The upstream profiles already have a built-in option for ecryptfs (out of the approximately 10 options available per profile).

@ColMelvin
Copy link
Contributor Author

@behlendorf What steps are required to complete this pull request? I assume the automated checks need to pass, however, I'm unsure how I might re-run a failing check.

@behlendorf
Copy link
Contributor

Thanks for reminding me. Based on the conversation above it sounds like there's general agreement that splitting the package is a good idea. Also based on your research we don't anticipate any unexpected side effects. I believe that just leaves nailing down the naming convention with the two candidates being pam_zfs_key or pam_zfs. Personally, I prefer the shorter pam_zfs, unless the _key suffiix is an important part of the convention? I don't feel strongly about it either way.

@behlendorf
Copy link
Contributor

Oh, and if you rebase this PR on the latest commits in master and force up it you should be a cleaner CI run.

ColMelvin added a commit to ColMelvin/zfs that referenced this pull request Mar 10, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
@ColMelvin
Copy link
Contributor Author

@behlendorf It looks like there are consistent failures on some Debian-based systems, which I find surprising. While there were inconsistent failures across all of the checks, 3 of the 4 unsuccessful checks included the following:

Tests with results other than PASS that are unexpected:
    FAIL pam/pam_basic (expected PASS)
    FAIL pam/pam_nounmount (expected PASS)
    FAIL pam/pam_short_password (expected PASS)

How do I get further information so I can debug the problem?

@behlendorf
Copy link
Contributor

You can download the full test artifacts for the Ubuntu GitHub actions builders here. Slightly more convenient are the Debian logs from buildbot. Take a look at the summary link, it will show which tests failed and try and pull excepts from the relevant logs. e.g.

Test (Linux): /usr/share/zfs/zfs-tests/tests/functional/pam/pam_basic (run as root) [00:00] [FAIL]
11:01:44.73 SUCCESS: ismounted testpool/pam/pamtestuser exited 1
11:01:44.73 SUCCESS: [ unavailable == unavailable ]
11:01:44.74 pamtester: successfully opened a session
11:01:44.74 cat: /mnt/testdir_run/1002: No such file or directory
11:01:44.74 
11:01:44.74 ERROR: [  == 1 ] exited 1
11:01:44.74 NOTE: Performing test-fail callback (/usr/share/zfs/zfs-tests/callbacks/zfs_dbgmsg.ksh)

But it looks to me like what happened here is the new package didn't get installed on Debian and Ubuntu. We've got a little bit of infrastructure in the config/deb.am file which is responsible for converting the rpm packages to debs for testing. If you update that to include the new package I suspect that'll sort it out.

The other test failures look unrelated. There are still a few of those we occasionally see.

Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 17, 2022
@behlendorf behlendorf merged commit 3ce3d30 into openzfs:master Mar 18, 2022
@ColMelvin ColMelvin deleted the feature.separate-pam-rpm branch March 19, 2022 06:51
@ColMelvin
Copy link
Contributor Author

@felixdoerre FYI: It looks like Fedora 36 (and, likely, future Red Hat releases) may lean more heavily on authselect per the accepted change: Make Authselect Mandatory. I'm unsure how this will affect pam_zfs_key users who have already configured their systems and upgrade to Fedora 36.

nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 2, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Create a separate `pam_zfs_key` package for the PAM module components,
an optional addition to the deliverables, in much the same way as the
Python bindings are released as a separate `python#-pyzfs` package.

This makes it clear when the PAM module is shipped with the package,
since it's now in its own package.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Chris Lindee <chris.lindee+github@gmail.com>
Closes: openzfs#13026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Packaging custom packages Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants