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

pam_fscrypt/config: prioritise over other session modules #278

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

ramcq
Copy link
Contributor

@ramcq ramcq commented Mar 2, 2021

Services launched by systemd user sessions on Debian / Ubuntu systems are often not able to access the home directory, because there is no guarantee / requirement that pam_fscrypt is sequenced before pam_systemd. Although this pam-config mechanism is Debian-specific, the config file is provided here upstream and unmodified in Debian. Raising the priority here so that it's always ordered ahead of pam_systemd will solve issues such as #270, https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964951 and https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1889416.

@google-cla
Copy link

google-cla bot commented Mar 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ramcq
Copy link
Contributor Author

ramcq commented Mar 2, 2021

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Mar 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ramcq
Copy link
Contributor Author

ramcq commented Mar 2, 2021

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Mar 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ramcq
Copy link
Contributor Author

ramcq commented Mar 2, 2021

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Mar 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ramcq
Copy link
Contributor Author

ramcq commented Mar 2, 2021

Sorry about spam. This it the most labour-intensive one-line PR I've ever sent. I've signed the corporate CLA... got the right e-mail address on my GitHub account... not sure what else is needed...?!

@ebiggers
Copy link
Collaborator

ebiggers commented Mar 3, 2021

I've signed the corporate CLA... got the right e-mail address on my GitHub account... not sure what else is needed...?!

Did you follow https://cla.developers.google.com/about#add-contributors to add your email address as an authorized contributor for your company? I think that might be the missing step.

@ebiggers
Copy link
Collaborator

ebiggers commented Mar 3, 2021

The actual fix looks good to me, although it would be helpful if there was some more reasoning behind the exact priority value that is being chosen (100). It needs to be higher priority than pam_systemd (0) and lower priority than pam_unix (256), but are there any additional constraints?

Also, can you please wrap your commit message at 72 columns, as per the usual convention?

Thanks!

Services launched by systemd user sessions on Debian / Ubuntu systems
are often not able to access the home directory, because there is no
guarantee / requirement that pam_fscrypt is sequenced before
pam_systemd.

Although this pam-config mechanism is Debian-specific, the config file
is provided here upstream and unmodified in Debian. Raising the
priority here so that it's always ordered ahead of pam_systemd will
solve issues such as google#270,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964951 and
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1889416.

After a survey of pam-config files available in Debian bullseye, the
value of 100 was chosen as it appears after most other plugins that
could be involved in more explicit homedir configuration (eg pam_mount
at 128) but before those which seem unlikely to work without a home
directory (eg pam_ssh at 64).
@ramcq
Copy link
Contributor Author

ramcq commented Mar 3, 2021

I've signed the corporate CLA... got the right e-mail address on my GitHub account... not sure what else is needed...?!

Did you follow https://cla.developers.google.com/about#add-contributors to add your email address as an authorized contributor for your company? I think that might be the missing step.

I may just have been too impatient - the corporate CLA has only come back to me from DocuSign today. Also (although both addresses are on my GitHub account) from an abundance of caution I've just made sure the commit author matched the current primary e-mail address on my Google account.

@ramcq
Copy link
Contributor Author

ramcq commented Mar 3, 2021

The actual fix looks good to me, although it would be helpful if there was some more reasoning behind the exact priority value that is being chosen (100). It needs to be higher priority than pam_systemd (0) and lower priority than pam_unix (256), but are there any additional constraints?

I made a small survey of the pam-configs files present in Debian bullseye that include Session plugins: session-priorities.txt. Without auditing every PAM plugin to see what precisely is done, it seems that things which interact with / fetch network credentials and groups are first with very high priorities, followed by eg pam_mount at 128 which could be implementing a necessary site-specific policy to mount the home directory, and then some things at 64 such as pam_ssh (launches ssh-agent) which seem unlikely to function properly without the home directory already being available. So, 100 was a slightly arbitrary choice, but it doesn't seem an unreasonable one. Happy to amend if you feel differently, but I've added a brief note to the commit message along these lines.

Also, can you please wrap your commit message at 72 columns, as per the usual convention?

Done!

Thanks!

Welcome! Thanks for fscrypt. :)

@ebiggers ebiggers merged commit 90a96e4 into google:master Mar 3, 2021
@ebiggers
Copy link
Collaborator

ebiggers commented Mar 3, 2021

Merged, thanks for the fix!

I'll probably need to fix the directions for Arch Linux too (https://wiki.archlinux.org/index.php/Fscrypt#PAM_module). They currently say to "append" pam_fscrypt, which would imply putting it after pam_systemd.

There is also the Enabling the PAM module on other Linux distros section in the README. It already says to put pam_fscrypt "after pam_unix", so it sort of recommends the right thing already, but it could be clarified.

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

Successfully merging this pull request may close these issues.

2 participants