pam: debian: Explicitly include pam_limits for xrdp sessions

[ximion]

Hello!
This change makes xrdp-sesman PAM explicitly request pam_limits in its Debian configuration.
This is needed, as on Debian, pam_limits is not automatically included for common sessions, and only for "login" sessions and session managers. So, xrdp has to include it explicitly to make limits take effect.
Without the change, limits might be set very low, which can yield a lot of really odd problems especially when running scientific software (we found out that things like MATLAB takes 15min to launch, and some Python code outright refuses to work. Especially for the MATLAB issue, the cause was fairly difficult to find).

This apparently has been a problem for a while, a web search yielded posts like https://florent.clairambault.fr/xrdp-and-the-ulimits-nofile-issue/ - but the solution given there isn't ideal, as it will alter the limits of system daemons as well, which may not be intended.

I think fixing this in xrdp is the better approach, and also way easier. We have been running with this configuration change for a year without any issues so far.

See https://wiki.debian.org/Limits for details on limits on Debian.
Thank you for considering the patch! :-)

[metalefty]

LGTM.

[matt335672]

I'm happy with the concept too - it seems like a great idea.

I've done a bit of checking around, and this appears to be the default on Fedora, Arch and openSuSE. In terms of major distros it's only the Debian family that's missing out.

@ximion - at the risk of seeming picky, could you move the line down in the file to sit with the rest of the session lines, like this? Then I'll merge it for you.

--- a/instfiles/pam.d/xrdp-sesman.debian
+++ b/instfiles/pam.d/xrdp-sesman.debian
@@ -5,8 +5,6 @@ auth     required  pam_env.so readenv=1 envfile=/etc/default/locale
 -auth    optional  pam_gnome_keyring.so
 -auth    optional  pam_kwallet5.so
 
-session    required   pam_limits.so
-
 @include common-account
 
 @include common-password
@@ -15,6 +13,7 @@ session    required   pam_limits.so
 session    required     pam_loginuid.so
 # Update wtmp/lastlog
 session    optional     pam_lastlog.so quiet
+session    required   pam_limits.so
 @include common-session
 -session optional  pam_gnome_keyring.so auto_start
 -session optional  pam_kwallet5.so auto_start

[metalefty]

Let's backport this one to v0.10 after the changes @matt335672 mentioned.

[matt335672]

I agree. I'd like to make sure @ximion is happy with it before merging.

[metalefty]

Changes required as @matt335672 mentioned.

[ximion]

@matt335672 Thanks for the feedback, I moved the line downwards - I originally placed it higher up as that's where it was for regular sessions, for other sessions it is indeed listed later though, so I guess that will be fine. It does seem to be placed before pam_loginuid though, so that's where I put it.

I really doubt that order matters that much (as long as it comes before the common-session include) but PAM is a bit magic and is one of the things in Linux that had the most amount of surprises. That's why I moved the line to its current position, just in case it matters afterall.

It is in the session-block now, but let me know if you need it elsewhere.
A backport would be really nice indeed, thank you all for the review and generally work on xrdp!

[metalefty]

Approved.

@ximion Thanks for your contribution! This change will be included in the next release planed in this December.