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

Upgrade to linux-pam-1.5.3-r1 #2049

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

markafarrell
Copy link
Contributor

@markafarrell markafarrell commented Jun 21, 2024

Update linux-pam to version 1.5.3-r1

Fixes: flatcar/Flatcar#1474

Update linux-pam to version 1.5.3-r1 and enable the use of vendordir

Vendor dir allows us to install config into /usr/lib/pam/security.

pam modules will first look for configuration in /etc/security and if they are not found there will fallback to /usr/lib/pam/security

Previously we had sconfigdir set to /usr/lib/pam which resulted in configuration being installed to /usr/lib/pam but left us unable to configure pam modules (as the pam modules used this path as their only source of configuration)

How to use

# Remove symlink for limits.conf
sudo rm -rf limits.conf

# Copy default limits.conf
sudo cp /usr/lib/pam/security/limits.conf .

# Add extra configuration to limits
echo '* soft nofile 515' | sudo tee -a /etc/security/limits.conf

# Check the current limits
ulimit -Sn

# logout
exit

# Login
ulimit -Sn

Confirm that ulimit is has changed to 515

Testing done

[Describe the testing you have done before submitting this PR. Please include both the commands you issued as well as the output you got.]

  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)
  • Inspected CI output for image differences: /boot and /usr size, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.

Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the upstream Gentoo diff between the old and new PAM versions and this lines up. I would prefer to see this vendor change upstream, but we already have our own custom changes. I'll look at upstreaming those another time.

Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually cancel that, this undoes the Flatcar-specific permissions adjustment on /sbin/unix_chkpwd. Please restore that.

@chewi
Copy link
Contributor

chewi commented Jun 24, 2024

To help you, here are our relevant changes.

--- /dev/fd/63  2024-06-24 12:13:12.434175945 +0100
+++ /home/chewi/Projects/flatcar/scripts/repos/flatcar-overlay/sys-libs/pam/pam-1.5.1_p20210622-r1.ebuild       2024-06-23 19:14:56.550235722 +0100
@@ -7,7 +7,7 @@
 # Can reconsider w/ EAPI 8 and IDEPEND, bug #810979
 TMPFILES_OPTIONAL=1
 
-inherit autotools db-use fcaps toolchain-funcs usr-ldscript multilib-minimal
+inherit autotools db-use toolchain-funcs usr-ldscript multilib-minimal
 
 GIT_COMMIT="fe1307512fb8892b5ceb3d884c793af8dbd4c16a"
 DOC_SNAPSHOT="20210610"
@@ -47,6 +47,7 @@
 S="${WORKDIR}/linux-${PN}-${GIT_COMMIT}"
 
 PATCHES=(
+       "${FILESDIR}"/${PN}-1.5.0-locked-accounts.patch
        "${FILESDIR}"/${PN}-1.5.1-musl.patch
 )
 
@@ -91,18 +93,24 @@
 multilib_src_install() {
        emake DESTDIR="${D}" install \
                sepermitlockdir="/run/sepermit"
-
-       gen_usr_ldscript -a pam pam_misc pamc
 }
 
 multilib_src_install_all() {
        find "${ED}" -type f -name '*.la' -delete || die
 
+       # Flatcar: The pam_unix module needs to check the password of
+       # the user which requires read access to /etc/shadow
+       # only. Make it suid instead of using CAP_DAC_OVERRIDE to
+       # avoid a pam -> libcap -> pam dependency loop.
+       fperms 4711 /sbin/unix_chkpwd
+
        # tmpfiles.eclass is impossible to use because
        # there is the pam -> tmpfiles -> systemd -> pam dependency loop
 
        dodir /usr/lib/tmpfiles.d
 
+       rm "${D}/etc/environment"
+       cp "${FILESDIR}/tmpfiles.d/pam.conf" "${D}"/usr/lib/tmpfiles.d/${CATEGORY}-${PN}-config.conf
        cat ->>  "${D}"/usr/lib/tmpfiles.d/${CATEGORY}-${PN}.conf <<-_EOF_
                d /run/faillock 0755 root root
        _EOF_
@@ -128,8 +136,4 @@
        ewarn "  lsof / | grep -E -i 'del.*libpam\\.so'"
        ewarn ""
        ewarn "Alternatively, simply reboot your system."
-
-       # The pam_unix module needs to check the password of the user which requires
-       # read access to /etc/shadow only.
-       fcaps cap_dac_override sbin/unix_chkpwd
 }

Copy link

github-actions bot commented Jun 24, 2024

@markafarrell
Copy link
Contributor Author

@chewi I noticed there is an issue for upgrading linux pam to >= 1.6.0 flatcar/Flatcar#1349

It looks like upstream that it is still masked though.

https://github.com/gentoo/gentoo/blob/master/sys-libs/pam/pam-1.6.1.ebuild

Not sure how we want to handle this?

Should we upgrade to 1.6.1 in this PR or wait until upstream unmasks it?

@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch 3 times, most recently from 3a484bd to a4a4c60 Compare June 25, 2024 01:25
@markafarrell
Copy link
Contributor Author

Actually cancel that, this undoes the Flatcar-specific permissions adjustment on /sbin/unix_chkpwd. Please restore that.

Fixed

@markafarrell
Copy link
Contributor Author

markafarrell commented Jun 25, 2024

musl patch is no longer required as it was fixed by linux-pam/linux-pam#433

The fix is present in linux-pam >= 1.5.3

@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch from a4a4c60 to 6e3dd16 Compare June 25, 2024 02:22
@markafarrell markafarrell requested a review from chewi June 25, 2024 02:23
Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better, thank you. There's little difference between the 1.5.3 and 1.6.1 ebuilds, so let's just take this for now.

@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch from 6e3dd16 to 6737ee8 Compare June 25, 2024 09:16
@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch 2 times, most recently from af54101 to 8d029df Compare June 26, 2024 00:04
@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch from 8d029df to 01e9516 Compare June 26, 2024 00:05
@markafarrell markafarrell reopened this Jun 26, 2024
@markafarrell markafarrell requested a review from tormath1 June 26, 2024 00:14
@markafarrell markafarrell force-pushed the feature/update-linux-pam-1.5.3 branch from 4b64796 to d5833e2 Compare June 26, 2024 23:01
@markafarrell
Copy link
Contributor Author

markafarrell commented Jun 28, 2024

CI is failing on multiple tests for

        cluster.go:125: + emerge-gitclone
        cluster.go:125: Cloning into '/var/lib/portage/scripts'...
        cluster.go:125: error: pathspec 'b79b81648c' did not match any file(s) known to git
        cluster.go:125: >>> No git repositories configured.

@chewi @tormath1 Any ideas on how to fix this?

@markafarrell
Copy link
Contributor Author

Ok that git-hash appears to correspond to b79b816

I think this (https://github.com/flatcar/flatcar-dev-util/blob/flatcar-master/emerge-gitclone#L14) is trying to checkout https://github.com/flatcar/scripts.git at that commit (b79b816) but is failing because that commit is on my fork not in a branch.

Is it possible for CI to run successfully for a PR from a fork?

Or am I missing something here?

@tormath1 tormath1 merged commit 72e0eaf into flatcar:main Jun 28, 2024
4 of 7 checks passed
@tormath1
Copy link
Contributor

@markafarrell thanks again for your contribution! I already seen this CI failure in the past, I think it's not related to these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to modify pam module configuration using files in /etc/security
3 participants