Skip to content
This repository was archived by the owner on Jun 21, 2018. It is now read-only.

Comments

Enforce per-user limits#41

Merged
KellerFuchs merged 1 commit intohashbang:masterfrom
KellerFuchs:users
Apr 26, 2016
Merged

Enforce per-user limits#41
KellerFuchs merged 1 commit intohashbang:masterfrom
KellerFuchs:users

Conversation

@KellerFuchs
Copy link
Member

@KellerFuchs KellerFuchs commented Dec 29, 2015

Given the urgency of this, I will take a lack of (dis)approval as an approval by timeout in 15h30 (2015-12-30, 11:00 UTC).

@ecliptik
Copy link

Not as familiar with the systemd changes, but the pam and limits.conf updates look good and should help with limiting abuse.

@KellerFuchs
Copy link
Member Author

Yes. I held back on merging because I found a bug in the systemd changes: it seems user@.service is special enough that something weird goes on with the Slice parameter.
I've been ill and mostly unable to investigate for the past days, though...

@KellerFuchs
Copy link
Member Author

It seems that the issue is pam_systemd(8) (or logind) having user-${UID} hardcoded somewhere ...

@KellerFuchs
Copy link
Member Author

Seems there is already an issue for that: systemd/systemd#2305

@KellerFuchs
Copy link
Member Author

This is stuck waiting on systemd/systemd#2556
The confinement part (DevicePolicy=closed) should be moved to namespace.conf and a script anyway, as systemd doesn't seem to support some equivalent of pam_namespace's umnt_remnt option.

@KellerFuchs
Copy link
Member Author

@hashbang/administrators Sorry for the erroneous PR close.
Somebody reviews and merges?

@daurnimator
Copy link
Member

daurnimator commented Apr 26, 2016

Don't our user ids for public users start at 3000?

Otherwise, why doesn't the 'users' group work?

session required pam_limits.so

# Environment setup
session required pam_env.so
Copy link
Member

Choose a reason for hiding this comment

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

pam_env should remain first to unset any dangerous environmental variables.

@KellerFuchs
Copy link
Member Author

KellerFuchs commented Apr 26, 2016

@daurnimator Each user has their own group, the users group is not really a thing.
Also, unless I'm mistaken, no environment variable is currently unset (nor would it matter, since PAM modules shouldn't take config from the environment), but I have no problem with putting pam_env first.

Also, there are some legacy users with UID < 3000:

% getent passwd deleriux
deleriux:*:1019:1019:deleriux:/home/deleriux:/bin/bash

Set per-user limits for the number fo processes and open files.
This is only effective now,
  as the “users” group is wrong (LDAP cruft),
  and the right PAM module wasn't in use...
@KellerFuchs
Copy link
Member Author

@ChickenNuggers reviewed too.

@KellerFuchs KellerFuchs merged commit 4acd95c into hashbang:master Apr 26, 2016
@KellerFuchs KellerFuchs deleted the users branch April 26, 2016 23:50
@KellerFuchs
Copy link
Member Author

Deployed

@ghost
Copy link

ghost commented Sep 20, 2017

@KellerFuchs look at this way of setting per-user CPU and memory limits. Personally I think it is more flexible than templating user-${UID}.slice because, for example, you may load user limits from config file or ask LDAP server.

@daurnimator
Copy link
Member

@BerserkerTroll interesting solution: slightly racey (though users will be constrained after the fact, so no issue?), but I think it's as good as we are going to get.

@RyanSquared
Copy link
Member

It could also be helpful for the (very long term) plans for allowing users to run "large" jobs - we were planning a system where users could "request" power to run a large job for a time, this could help with it.

@KellerFuchs
Copy link
Member Author

@BerserkerTroll That would be much better done with pam_exec, but yes that could be one option.

@ghost
Copy link

ghost commented Sep 21, 2017

@KellerFuchs didn't know about pam_exec.

@daurnimator
Copy link
Member

Created #182 to track.

@ghost
Copy link

ghost commented Oct 11, 2017

The second task is still not completed?

@KellerFuchs
Copy link
Member Author

@BerserkerTroll Which “second task” ?

@ghost
Copy link

ghost commented Oct 11, 2017

@KellerFuchs the second checkbox was unchecked until recently.

@KellerFuchs
Copy link
Member Author

@BerserkerTroll Yes, this has been done in a separate pull request, you are commenting on a change from late 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants