Use pam_env to properly setup system-wide env

[kirelagin]

This is the proper fix of #2688.

[edolstra]

Seems good to me. Maybe environment.systemVariables should be called environment.sessionVariables (since they're bound to PAM sessions, and environment.variables is also system-wide).

[wmertens]

Hmmm - where did NIX_CONF_DIR go? Shouldn't that be a systemVariable as well then?

[kirelagin]

They are systemVariables now, obviously.

[kirelagin]

Ah, I see what you are asking about.
NIX_CONF_DIR is an envVar, and all the envVars are systemVariables now.

[wmertens]

I like it, and the name environment.sessionVariables as well. Will this not break things when switching to this configuration? I think maybe people will have to log out to have sudo behave the same?

👍

[kirelagin]

I chose the name systemVariables exactly because they are now more “system” then they used to be. Previously they were set by shells rc and preserved in sudoers (which is basically a hacky workaround).
Now they are initialised earlier, so we get them both in shells and in shell-less (that is, system) things. So, again, they are “system” because they are used almost always, even when there is no user involved.

My guess is that those variables are also available now in services, so we don't have to pass them explicitly, even though I was too lazy to test this.

[kirelagin]

BTW, I was going to rename environment.variables to environment.shellVariables, but something stopped me. Can't remember now what exactly.

[kirelagin]

Ok, I've checked systemd docs and it looks like that it won't open a PAM session for a service unless explicitly asked to do so (via PAMName).

[vcunat]

Merged, thanks!

[vcunat]

Picked into release, too.

[kirelagin]

Not sure picking into release was a good idea. As mentioned in the comments, this requires a relogin.

[vcunat]

I don't see why requiring relogin should be a problem for release. We e.g. push kernel updates there, and they even need a reboot :-)

[kirelagin]

Right, that's not a critical issue, but still there is a difference.
After you upgrade the kernel your system keeps working as it was until you restart.
In this case your system kinda breaks until you relogin. Well, not really breaks, but strange things might start happening when you run sudo until you relogin because of those three variables that I removed from sudoers.

[vcunat]

Yes, it's unfortunate that such things happen... e.g. when running xfce4session the session is killed on typical nixos-rebuild switch (whenever dbus gets restarted IIRC); or trying to resume into the updated system after suspend-to-disk will also fail (at least when kernel changed).

I don't know how to approach such updates, where you may need some restarting action.

[edolstra]

I've reverted this in release-14.04, because IMHO this kind of global behaviour change is not a good idea there. At least not until it has had a lot of soak time in master.

[edolstra]

Urgh, I screwed up, reverted it in master instead. Branches are hard...

[vcunat]

Heh, OK.

[vcunat]

It's just the thing that I did see people complaining about the channel checkout error right after installation (I ran into that one as well), which is unfixed in our default ISOs now. It could be worked-around by merging #2688 into release instead.

[kirelagin]

Merging #2688 is never a good idea. It can be worked around by adding the relevant variable to sudoers as I suggested in #2688. This might be indeed a good idea to fix the installation ISO right away.