Thoughts on PAM environment variables

[regiontog]

First of all thanks for making this library, it has made creating a Display Manager more enjoyable.

As you know it's kinda difficult to get any good information on how to make DMs, but I've managed to get to a point where I can comfortably use mine and I thought I'd share some of what I have learned. I made a fork of this project here with some changes I needed to get everything to work properly w.r.t. logind/systemd.

• pam_systemd.so - which is needed to get logind working - reads some PAM environment variables. So the user should have a way of setting the PAM vars propably. These also needs to be set before pam_open_session is called.
• As I understand it setting the process' actual environment variables could be confusing, especially if a single process is managing multiple PAM sessions. The better way imo is using Command::envs to set all of the PAM environment in the child process. For example like I have done in my DM.
• The PATH environment variable is probably better left to pam_env.so so that individual users or distributions can set the PATH variable in /etc/environment or ~/.pam_environment.

[1wilkens]

Hey thanks so much for your comment and links! I'll check them out later today or this week! I also got my DM to the point of "barely working" and recently got motivated again to work on it, so I'm looking forward to check out your progress.

With regards to pam, I think we could discuss which variables should be set unconditionally and which maybe could be set via optional arguments to open_session or something.

[regiontog]

Depending on how general this library wants to be passing in those as arguments to open_session could work. I don't really know pam that well so afaik open_session could be used in other contexts than DMs?

In any case I think it would be difficult but not impossible to have an exhaustive list of arguments to open_session. Another idea could be to have a Logind struct with Logind::open_session that sets the arguments for pam_systemd.so before calling Authenticator::open_session. That way it would be easier to add support for different pam modules arguments without breaking the API. I also think that it's awkward how coupled everything is to the /etc/pam.d/service-name config file. If the user of this library has not configured pam_systemd.so there nothing would work. So I feel like good documentation is needed around logind if that usecase is to be supported.

In order to make the logind session work(so that it is assigned a seat etc) I needed to set these variables(even though I think I read somewhere that pam_systemd should figure out some of these by itself it didn't):

$XDG_SESSION_TYPE="x11"
$XDG_SESSION_CLASS="user"
$XDG_VTNR=[vtnr]
$XDG_SEAT="seat0"

I get this logind session with my DM:

~ > loginctl show-session $XDG_SESSION_ID
Id=1
User=1000
Name=alan
Timestamp=Wed 2019-05-22 09:30:21 CEST
TimestampMonotonic=9991632
VTNr=7
Seat=seat0
Remote=no
Service=webdm
Scope=session-1.scope
Leader=588
Audit=1
Type=x11
Class=user
Active=yes
State=active
IdleHint=no
IdleSinceHint=0
IdleSinceHintMonotonic=0
LockedHint=no

$XDG_SESSION_DESKTOP is also mentioned by the pam_systemd.so documentation so it would be nice to have that parameter as well.

[1wilkens]

Depending on how general this library wants to be passing in those as arguments to open_session could work.

Since the crate is called pam I think we want to be as general as possible and not only support DMs. ;)
I took a rough look at the changes in your fork and I would be happy to merge most of it (especially the changes to make the environment struct an Iterator and the renaming of the handler methods). Could you open a PR for that?

[mtlll]

I wanted to comment on this issue, since I've also been writing my own toy desktop/login manager in rust recently, and I've been digging into what constitutes a valid logind session and all this. I've read over most of the code in this library, as well as some of @regiontog's fork and his WebDM project. It seems to me that the code in initialize_environment is totally correct except for the PATH variable. And it might indeed be better to return an environment to apply rather than setting it for the current process, but this is a minor complaint. I don't know which distro @regiontog is using, and I also don't know which PAM configuration his WebDM is using(the code just says to use the "webdm" file, but there's no such file tracked in the git repo). But on my distro(Arch Linux), when using the correct PAM config file, I do indeed get all the $XDG_* variables set, as well as some Systemd related dbus stuff. And I get a valid session with logind. Indeed, my environment after initialize_session() is identical to the one I get after using the default tty login program, except for randomly generated identifiers for dbus etc. In my case I used the file /etc/pam.d/login, which the one used by the tty login screen. The contents are as follows:

login:

#%PAM-1.0

auth       required     pam_securetty.so
auth       requisite    pam_nologin.so
auth       include      system-local-login
account    include      system-local-login
session    include      system-local-login 

system-local-login:

#%PAM-1.0
auth      include   system-login
account   include   system-login
password  include   system-login
session   include   system-login

system-login:

#%PAM-1.0

auth       required   pam_tally2.so        onerr=succeed file=/var/log/tallylog
auth       required   pam_shells.so
auth       requisite  pam_nologin.so
auth       include    system-auth

account    required   pam_tally2.so 
account    required   pam_access.so
account    required   pam_nologin.so
account    include    system-auth

password   include    system-auth

session    optional   pam_loginuid.so
session    optional   pam_keyinit.so       force revoke
session    include    system-auth
session    optional   pam_motd.so          motd=/etc/motd
session    optional   pam_mail.so          dir=/var/spool/mail standard quiet
-session   optional   pam_systemd.so
session    required   pam_env.so

system-auth:

#%PAM-1.0
auth      required  pam_unix.so     try_first_pass nullok
auth      optional  pam_permit.so
auth      required  pam_env.so

account   required  pam_unix.so
account   optional  pam_permit.so
account   required  pam_time.so

password  required  pam_unix.so     try_first_pass nullok sha512 shadow
password  optional  pam_permit.so

session   required  pam_limits.so
session   required  pam_unix.so
session   optional  pam_permit.so

As you can see pam_systemd.so is only included in the system-login file, not system-auth(which is the file referenced in the examples for this library). If you invoke system-auth, only the absolute minimal checking against /etc/passwd will be performed. using system-local-login should ensure the correct env variables are set up my pam_env.

Of course, the organisation of pam.d differs from distro to distro, and a quick look at my debian box reveals a completely different setup. Generally though you should refer to your distro's documentation to find out which pam file to invoke, or which ones you need to include from your custom pam files to stay secure and compatible.

Now, I've read some of the systemd-logind documentation, and it specifically says to not talk to logind directly, but let pam_systemd do it. Talking to systemd, setting specific environment variables or calling into specific pam modules shouldn't be the responsibility of this library as it exists now, which is mainly as a wrapper around the PAM conversation API for clients.

The only real bug I found in it was this line

self.set_env("PATH", "$PATH:/usr/local/sbin:/usr/local/bin:/usr/bin")?;

this leads to the following PATH variable on arch linux:

$ grep PATH badenv                                                                                                                                                                                                                                                                            
PATH=/home/mtl/.cargo/bin:$PATH:/usr/local/sbin:/usr/local/bin:/usr/bin

Note that the .cargo part is from my local shell .zprofile.
As you can see, this code just inserts the literal '$PATH' into the variable. This is probably not what was intended. Either way, this library shouldn't initialize PATH. This is either done by pam_env or by the shell when started with -l(to start a login shell).

I also want to say thanks for developing this library and pam-sys. They've made my life a lot easier writing my own DM in rust. I hope my insights from banging my head against the wall for 3 days will be helpful somehow :D

[1wilkens]

Hey @mtlll this is very useful information indeed! I am quite impressed that you got a valid systemd session using pam! That is more than I can say for myself (and I'm also on Arch) ;)

Some thoughts (in no specific order of importance):

• Great to know, that we should not talk directly to logind. That simplifies the library a lot and I was very hesitant to add logind-specific stuff to a library that is supposed to wrap PAM.
• The $PATH thing is just plain wrong. I am not sure, if we don't need to set it at all (have not yet fully understood pam_env yet), but literal $PATH has to go. I'll try to change that in the next days.
• Returning the environment may be better, I have to think about this some more.

Thanks for your insights! I may have a closer look at yalm to see how you managed to get a session going ;)

[mtlll]

I think $PATH should also be handled by pam_env. It can also be initialised by the shell when given the -l option(to start as a login shell).

Again, I didn't really do anything special to get the env set properly. All I did was use the right pam service file(in my case I used login because my login manager is TUI-based and it made sense, more generally system-local-login is probably what you want).

You're welcome to look at yalm, but keep in mind it's still a work in progress, and I'm writing yalm mainly as a project to learn rust, so the code is a bit of a mess right now :)