PAM: Warn about issue with systemd-user service

[ColMelvin]

Motivation and Context

The systemd-user service currently opens PAM sessions but fails to close them (systemd/systemd#8598), which prevents the pam_zfs_key module from working as intended. If a system administrator were to miss this defect in their configuration, their users would not gain the full benefit of the pam_zfs_key module, as their encrypted datasets would not be locked until reboot.

Description

Add a warning message when the systemd-user service is detected to help system administrators notice and diagnose the problem with their PAM configuration more quickly.

How Has This Been Tested?

Test Environment: Fedora 34 Server on VM

Steps:
0. Disabled SELinux

1. Installed patched pam_zfs_key.so module
2. Configured /etc/pam.d/system-auth (included by systemd-user) to use pam_zfs_key. For the session section, used

session optional                   pam_zfs_key.so

3. Rebooted system and logged in
4. Verified warning message appeared in journalctl -b0
5. Configured /etc/pam.d/system-auth with

session [success=1 default=ignore] pam_succeed_if.so service = systemd-user
session optional                   pam_zfs_key.so

6. Rebooted system and logged in
7. Verified warning message did not appear in journalctl -b0

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:

• 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.

[ColMelvin]

I kept this message generic in case other services are found that violate our expectation of 1 session open and 1 session close (perhaps in a different way than systemd-user). However, system administrators might benefit from a more detailed warning.

I'm open to suggestions

[gdevenyi]

I think the message should point at either a ZFS issue or a systemd issue where the status can be checked for it being fixed.

[ColMelvin]

Some ideas:

• Expect issues; see https://github.com/systemd/systemd/issues/8598
• Session will never close; see https://github.com/systemd/systemd/issues/8598
• Key will not be unloaded because of https://github.com/systemd/systemd/issues/8598

[gdevenyi]

Maybe a date? "As of 2022-01-28" + one of your suggested wordings.

I like the "Key will not be unloaded"

[ColMelvin]

With a date in the message, we'll likely want to keep the value up-to-date with - at least - yearly commits. Given the original bug is over 3 years old, and the complexity required for its correct resolution, I expect this would require a long term commitment to update the date.

Let's soften it with "key may not be unloaded", which leaves open the possibility that the issue gets fixed and makes removing the warning message less urgent if a fix for systemd/systemd#8598 does materialize.

[ColMelvin]

@gdevenyi What are the next steps for getting this approved?

[nabijaczleweli]

What's the point of returning a tristate if the only user just void-casts it? The documentation says warn, and it just warns, so why not just warn?

static void
session_roundtrip_warn(pam_handle_t *pamh)
{
	const char *service;
	if (pam_get_item(pamh, PAM_SERVICE, (const void **)&service)
	    != PAM_SUCCESS)
		pam_syslog(pamh, LOG_NOTICE, "Unable to identify PAM service");
	else if (strcmp("systemd-user", service) == 0)
		pam_syslog(pamh, LOG_WARNING,
		    "Key may not be unloaded because of "
		    "https://github.com/systemd/systemd/issues/8598");
}

[ColMelvin]

I hoped to build on this change and add functionality based around the return value. However, I think those changes could generate a more vibrant discussion. Since this function provides value even in isolation, I decided to submit it separately.

Regardless, it doesn't seem to matter; both forms compile to the same binary on my system.

[gdevenyi]

@gdevenyi What are the next steps for getting this approved?

I'm just a random drive-by commenter who made a suggestion, I have no power here :)

[ColMelvin]

I'm just a random drive-by commenter who made a suggestion, I have no power here :)

Well, thank you for the dialog! I appreciated the feedback.