GNOME Bugzilla – Bug 559781
pam module: support auto_start_if=... option
Last modified: 2009-06-26 16:55:19 UTC
There's an auto_start option for the pam module. However, to really properly integrate the pam module on a distro, it's much better to put the pam keyring line in /etc/pam.d/common-auth (eg) than in /etc/pam.d/gdm: this way, it will be working most of the time. However, we don't want to have auto_start happening for all services. So a auto_start_if=... option would help: you'd list the services here for which you accept auto_start behavior. So, to keep the same results as what we have right now, we'd have "auto_start_if=gdm" (but people could also do "auto_start_if=gdm,xdm").
Created attachment 122197 [details] [review] Patch Note: this depends on the patch in bug 558636. Patch was originally written by Thorsten Kukuk and I modified it a bit. Needs some testing, though.
Tested the patch a bit and it works fine.
Note that if we implement bug #560488, we'll want to do this another way. We should rename "auto_start_if" to "if" and only do the things we're interested in if the service is in the "if=" option. That would be orthogonal to the auto_start issue. So you'd have: auth optional pam_gnome_keyring.so if=gdm session optional pam_gnome_keyring.so auto_start if=gdm
Interesting. Is this so that we can keep the gnome-keyring lines just in one place instead of two?
Yep. Except that it's "just in one place instead of 42" and not two :-) This would make it easier to use gnome-keyring in different kinds of sessions, and not only the ones started by gdm. Or to unlock the keyring in a more opportunistic way if it's already running but locked.
Cool. Let's do it. I just have the following comments, minor things: * Can me make the evaluate_inlist string parsing a bit more robust? For example handle spaces around commas? * Can we make the argument parsing easier to read? Perhaps by storing away the length of the 'auto_start_if=' prefix in a constant or variable? Other than that, I'd say go ahead and commit it.
Stef: do you mind if I also change the patch to use if instead of auto_start_if? See comment #3 for an explanation with a example. I think it's slightly better since you can have "if=" only if you want, and you can still get something like auto_start_if by using "auto_start if=" I might do it tonight, else it will be in a week since I'll be travelling. Unless I do it in the plane :-)
So do you mean the module does nothing if the if=xxx doesn't match? If so that makes sense, I guess. It seems that this would have been faced by other PAM modules before... What do they do? Is there a precedent?
(In reply to comment #8) > So do you mean the module does nothing if the if=xxx doesn't match? If so that > makes sense, I guess. Yep. > It seems that this would have been faced by other PAM modules before... What do > they do? Is there a precedent? Thorsten proposed the auto_start_if= stuff. And changing it to if= is not really a big change from there. So I'd assume that's okay from a PAM point of view, but don't trust me :-) I don't know if there's a precedent, unfortunately.
Great. Could you update the docs here once you've made the changes? http://live.gnome.org/GnomeKeyring/Pam Is 'only_if=' clearer? Your call though...
Created attachment 129451 [details] [review] Updated patch
(In reply to comment #6) > Cool. Let's do it. I just have the following comments, minor things: > > * Can me make the evaluate_inlist string parsing a bit more robust? For > example > handle spaces around commas? I actually don't think it's needed. If the config line in pam is: auth optional pam_gnome_keyring.so only_if=gdm, gnome-scrensaver then gnome-screensaver will appear as an option. So there can't be space there. Or am I missing something? > * Can we make the argument parsing easier to read? Perhaps by storing away > the length of the 'auto_start_if=' prefix in a constant or variable? Done, it's clearer now. > Other than that, I'd say go ahead and commit it. (waiting for 2.27 now, unless you want this to go in for 2.26, but that's a bit risky, I'd say) (In reply to comment #10) > Is 'only_if=' clearer? Your call though... Yeah, it's probably a bit clearer, so now it's only_if.
Cool. Am I correct in assuming that the gkr pam module does nothing if the only_if= doesn't match? Do we want to force for 'password' (ie: pam_sm_chauthtok) since we'd like our login keyring to track the user password as much as possible? But maybe you've already considered this. Other than that, looks good for inclusion in 2.27.
(In reply to comment #13) > Cool. Am I correct in assuming that the gkr pam module does nothing if the > only_if= doesn't match? Yep. > Do we want to force for 'password' (ie: pam_sm_chauthtok) since we'd like our > login keyring to track the user password as much as possible? But maybe you've > already considered this. Well, I would say it's up to the admin to not use only_if for password :-)
Good call.
(In reply to comment #13) > Other than that, looks good for inclusion in 2.27. Now that gnome-keyring has branched, can this please get committed to master?
Committed to master.