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: add call to pam_umask #27870

Merged
merged 1 commit into from Jun 7, 2023
Merged

pam: add call to pam_umask #27870

merged 1 commit into from Jun 7, 2023

Conversation

msekletar
Copy link
Contributor

Setting umask for user sessions via UMASK setting in /etc/login.defs is a well-known feature. Let's make sure that user manager also runs with this umask value.

Follow-up for 5e37d19.

Setting umask for user sessions via UMASK setting in /etc/login.defs is
a well-known feature. Let's make sure that user manager also runs with
this umask value.

Follow-up for 5e37d19.
@msekletar msekletar requested a review from fbuihuu May 31, 2023 17:04
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label May 31, 2023
@fbuihuu
Copy link
Contributor

fbuihuu commented Jun 1, 2023

It looks OK to me.

@poettering
Copy link
Member

is that pam modulo that commonly used?

@msekletar
Copy link
Contributor Author

is that pam modulo that commonly used?

I don't think people change default umask for user sessions often but when they do and setting is not reflected in e.g. gnome-terminal (launched as --user service these days) they are certainly surprised.

@msekletar
Copy link
Contributor Author

Btw, I don't know about any other way to set default umask for user sessions other than setting UMASK= in /etc/login.defs.

Copy link
Contributor

@ldv-alt ldv-alt left a comment

Choose a reason for hiding this comment

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

If the systemwide pam configuration uses pam_umask.so, it would make sense to use it here as well.
If pam_umask.so is not used systemwide, it wouldn't harm either.

@ldv-alt ldv-alt added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jun 7, 2023
@keszybz
Copy link
Member

keszybz commented Jun 7, 2023

Yeah, this looks reasonable. Similarly to how we added pam_namespace, having this in our stack makes things "just work" for people who need this, and is not a problem for people who don't need it.

jammy-ppc64el: TEST-29-PORTABLE: FAIL (1813 s), not related.

@keszybz keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Jun 7, 2023
@keszybz keszybz merged commit 159f1b7 into systemd:main Jun 7, 2023
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants