GNOME Bugzilla – Bug 627530
Maintain environment with PAM (with patch)
Last modified: 2011-03-04 18:34:44 UTC
Created attachment 168431 [details] [review] Patch to maintain the target environment solely in PAM This patch gets gdm-session-worker to build the target environment using the pam_env* functions instead of in a g_hash_table. This was suggested in a comment in the gdm_session_worker_set_environment /* FIXME: maybe we should use use pam_putenv instead of our * own hash table, so pam can override our choices if it knows * better? */ As comment says, the advantage is it gives PAM access to the environment. Note that this isn't currently as useful as it could be because gdm-simple-slave sends nearly all of the environment variables right at the very end. It would be nice if it would send them as soon as their value was established. For environment variables set after (at least some of) PAM has run, it might also be nice to have gdm-simple-slave pass along a default flag to tell gdm-session-worker whether it should override a PAM setting or not. Nonetheless, it is an improvement on the current situation as at least PAM gets access to some of the environment variables, and a necessary step in any further work to this regard. Cheers! -Tyson PS: This is an update of the patch (so it applies against HEAD) for the one I sent back in March to the mail list.
Looks sensible to me.
Created attachment 168960 [details] [review] Maintain the target environment solely in PAM (fixed pam_getenv argument and related free issues) Turns out there was a bug in that first patch. I had accidentally passed the wrong variable to pam_getenv, which was also masking two freeing issues. Please find attached an updated patch. Cheers! -Tyson
I definitely appreciate the time that went into this patch. It's a useful contribution and we should consider pushing it. I am a little hesitant to push it without it fixing a problem in a specific PAM module though. This area of the code is a bit finicky and I'm worried a change here could potentially break existing setups. My thoughts are we hold off for now, but add a comment to the code pointing to this bug. If there's a problem in the future then we land the patch. We couldn't potentially land it sooner, but probably not for 2.32 since we're already at code freeze after the up coming release.
Thanks for looking at this Ray and Brian. I don't know if it is strictly required by any PAM modules, but it can be quite useful in conjunction with the pam_env module. Specifically, in our system, due to some interactions between ssh and pam_mount, we used pam_env to override/provide early defaults for some environment variables. This caused problems with GDM maintaining a separate environment. Cheers! -Tyson
Comment on attachment 168960 [details] [review] Maintain the target environment solely in PAM (fixed pam_getenv argument and related free issues) alright, let's land this after free. There's a few spacing issues we make sure we mop up before committing this though.
(In reply to comment #5) > (From update of attachment 168960 [details] [review]) > alright, let's land this after free. There's a few spacing issues we make sure > we mop up before committing this though. Ping - as the patch has the status "accepted-commit_after_freeze", can this get a commit now in either gtk3 or master or both, please?
(In reply to comment #5) > (From update of attachment 168960 [details] [review] [details]) > alright, let's land this after free. There's a few spacing issues we make sure > we mop up before committing this though. Ping - as the patch has the status "accepted-commit_after_freeze", can this get a commit now in either gtk3 or master or both, please?
Comment on attachment 168960 [details] [review] Maintain the target environment solely in PAM (fixed pam_getenv argument and related free issues) In now.