Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 165919
Summary: | Review Request: pam_ssh Pluggable Authentication Module for ssh | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Patrice Dumas <pertusus> |
Component: | Package Review | Assignee: | Tom "spot" Callaway <tcallawa> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dmitry, fedora-package-review |
Target Milestone: | --- | Flags: | j:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://sourceforge.net/projects/pam-ssh/ | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-09-01 09:53:26 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Patrice Dumas
2005-08-14 13:25:08 UTC
Review: Good: - rpmlint returns nothing - naming is fine - meets PackagingGuidelines - license ok (BSD), matches source - spec file in Am. English, legible - package builds ok on x86 (FC4) - all BuildRequires ok - no locales - no libs (pam so is ok) - not relocatable - doesn't make new directories - %clean section ok - no duplicate %files - macro use is consistent - code not content - no need for -docs, -devel - .la files deleted Needswork: - COPYING needs to be included in %doc - URL is wrong in Source: ... should be: http://dl.sourceforge.net/sourceforge/pam-ssh/%{name}-%{version}.tar.bz2 Very Bad: - md5sums do NOT match upstream [spot@swoop ~]$ md5sum rpmbuild/SOURCES/pam_ssh-1.91.tar.bz2 4f4e796b28883387261ac662c972b562 rpmbuild/SOURCES/pam_ssh-1.91.tar.bz2 [spot@swoop ~]$ md5sum pam_ssh-1.91.tar.bz2 57a3aa476394efa219a8a99f527d4e4b pam_ssh-1.91.tar.bz2 Please fix the Needswork and Very Bad items and I'll re-review. Hopefully this should be fixed in: http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-2.src.rpm Oh, you have outsripped me! I wanted to add this package to FC Extra too... :-) We successfully use pam_ssh already more than year. There are two patches (made by me) which are necessary. A first patch makes the "session" part stand-alone (i.e., when "auth" part was not used). This is very useful in combination with pam_console login/gdm authentication. A second patch moves state files from ~/.ssh to system /var/run . This provides properly cleanup after system crash. Also, I change the spec file. Openssl dependencies are added, words "using SSH' are changed to "using SSH keys", etc. My SRPMS: http://dmitry.butskoy.name/pam_ssh/pam_ssh-1.91-1.src.rpm Spec file: http://dmitry.butskoy.name/pam_ssh/pam_ssh.spec Patrice, Tom, Please, check my variant (and my patches). We use this package logn enough at our system, these patches are caused by real life... Patrick, Dmitry's patches look good... you should merge them into your spec. The md5sums match upstream now, and all the blockers are resolved. Tom, Did you check my patches? Not spec-file changes only? (Noone downloaded my srpm yet). Patrice, Probably it will be better if you use my spec-file, just rewrite changelog section, having mentioned me once because of two patches added. Dmitry, Your spec file needs some cleanups, whereas Patrice's spec file is OK at this point. It should be easy enough to merge your patches into Patrice's spec. Your patches look ok to me, but I'm relying on the fact that you've used this package. :) Dmitry, I update my spec with your patches and changes, test it and repost it. Where can I download it from?? :-) The version-release in the last changelog entry is 0:1.91-1, srpms has 1.91-3 Is it actually gcc-c++ needed?? Patrice, Please, merge in summary and description too. IMHO my phrases describe this pachage more precisely. (I mean "SSH keys" instead of just "SSH", and the fact that additional session calls just sets environment without extra ssh-agent invocation). New version at http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-4.src.rpm I removed gcc-c++ it is an old leftover when I used mach and there was a libtool bug. Now gcc-c++ is in the build root and hpefully the libtool bug has been solved. I merged your description but I changed it a bit. Tell me if you don't like it: This PAM module provides single sign-on behavior for UNIX using SSH keys. Users are authenticated by decrypting their SSH private keys with the password provided. In the first PAM login session phase, an ssh-agent process is started and keys are added. The same agent is used for the following PAM session. Regarding the patch that allows the information to be in /var/run/pam_ssh I find it good, except that (at least for me) the files have permissions such that the user cannot read the agent informations back from that file. I think this is unfortunate: -r-------- 2 root dumas 134 aoû 15 15:09 dumas -r-------- 2 root dumas 134 aoû 15 15:09 dumas-:0 A possible solution could be to have the user own that file. In that case thee user may read it but still cannot delete it. If the user should be able to delete the file the a directory should be created in /var/run/pam_ssh for the user and the files placed in that directory. Is there a specific reason why you made this file unreadable and unremovable by the user? Change also "SSH" to "SSH keys" in the summary, for the same reason. Your phrase about session phase is better than mine. But for more transparency we should tell somehow that session phase sets an appropriate environment, which locates ssh-agent. Otherwise some people will not understand, how this module works. About /var/run/pam_ssh permissions:
These files are "readable" due to "openpam_restore_cred()" call before we open
(or create) them. I.e., actually we open them under root.
Therefore:
> Is there a specific reason why you made this file unreadable and unremovable by
the user?
Is there any reason we should made this files readable/removable for the user?
(For example, all under /var/run/console/ is owned by root).
Also, The idea to create a directory under /var/run/pam_ssh/ was considered by me. Unfortunately, system initscripts only clear things like "/var/run/<dir>/*" . To clear "/var/run/pam_ssh/<dir>/*" some changes to /etc/init.d/rc.sysinit should be done. To avoid it I has applied a way without subdirectory creation. IMHO, it is even better for security that the user cannot read/remove this files. And pam_console case is an existing precedent which votes for it. Here is another try. I added a phrase about the environment variables set by pam_ssh but I wasn't very precise. I don't think technical details should appear in the rpm description. They belong to the package documentation. A patch to the man pages or the README should be a better for precisions about the environment variables in my opinion. I also didn't change the summary because I think it is an unneeded precision that restrict the pam_ssh scope. Otherwise said pam_ssh is usefull with ssh, not only for ssh keys. I myself don't use pam_ssh for the single sign on capabilities but because it starts the agent. http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-5.src.rpm Now agree with description :-) But I not agree that this is a technical details. It is important to specify that this module works with keys, and things related to keys (ssh-agent). This module does not start any ssh remote terminals, does not operate as an authentication proxy, does not use ssh as "check if a command invokation is success", etc. etc. If we say just "SSH", the user can misunderstand it. If we say "SSH keys", it is more precisely. "SSH keys" mean that pam_ssh works with ssh keys, starts the sshkey-related program "ssh-agent" (if needed), and nothing also. Regarding the permissions on the /var/run/pam_ssh/ files there is a use for a file readable by the user. Indeed if the user log through a mean without the pam_ssh module used he still can use the agent by sourcing the file. Now as to have a file the user may modify the aim would be to let the user manage the pam_ssh related files. Also this shouldn't have security implications (related with ssh) as it is how things are currently done; the user may manipulate the environment anyway. In my opinion the difference is that now the files are in /var/run/pam_ssh and not in the user home directory. So having files or dirs owned by the users may create issues because of that. For example if there is a directory owned by a user as I described above the user may fill the var file system. And if the user is the file owner he should be able to change the permissions, thus may for example permit other users to read or modify it or change the perms such that the file is unreadable (although this last change should not be a big deal). Using /var/run which should be local to the computer is good, but not allowing the user to tweak the files created by pam_ssh is an important departure from the upstream so I think this should be certain that it is needed. During one year of using pam_ssh, we never read these files directly. Is it actually needed for most users? Even advanced users? What practical cases when the user logins passing pam_ssh, but the same ssh-agent is necessary for him? I agree that it is some departure from upstream. Therefore I did not send /var/run patch upstream. (I consider it is mostly Fedora-related). But at least for Fedora it is needed. IMHO, it is in the RedHat style, and should be useful for most Fedora users. I personnaly used these files. I enabled pam_ssh only when loging in from gdm or a login, so when I login through ssh it could be usefull. Also I had to manually delete the files when the computer or the session crashed without removing it. Admitedly if it is a computer crash te files should be removed by the initscripts but if the computer cannot be rebooted then only root can remove them. Even if the user cannot delete the file I believe that emptying them should do the trick. Maybe I should add that the pam_ssh modules are optionnal in my setup. I propose for the summary : Smmary: PAM module for use with SSH keys that can launch the agent To me that summary sounds like it's the keys that would launch the agent. And that: Summary: PAM module for use with SSH keys, launching agent if needed May be: Summary: PAM module for use with SSH keys and ssh-agent Patrice, I think these files may be made world-readable (r--r--r--). It should not lead to security problems as the corresponding information (ssh agent`s pid and socket) can be received other way (using "ps" command ans "ls /tmp"). You really believe it is useful for all users? Access to local ssh-agent by extern, non-related remote login may be useful, but "will never be recommended" by Fedora people, as I guess. Therefore IMHO we should not focus pam_ssh for this case. By the way, our examples (using pam_ssh together with new pam_console ability to authenticate login user): /etc/pam.d/login: #%PAM-1.0 auth required pam_securetty.so auth sufficient pam_console.so auth required pam_stack.so service=system-auth auth optional pam_ssh.so try_first_pass auth required pam_nologin.so account required pam_stack.so service=system-auth password required pam_stack.so service=system-auth # pam_selinux.so close should be the first session rule session required pam_selinux.so close session required pam_stack.so service=system-auth session optional pam_console.so session optional pam_ssh.so # pam_selinux.so open should be the last session rule session required pam_selinux.so multiple open /etc/pam.d/gdm: #%PAM-1.0 auth required pam_env.so auth sufficient pam_console.so auth required pam_stack.so service=system-auth auth optional pam_ssh.so try_first_pass auth required pam_nologin.so account required pam_stack.so service=system-auth password required pam_stack.so service=system-auth session required pam_stack.so service=system-auth session optional pam_console.so session optional pam_ssh.so The result is one password typing for all consoles and gdm (pam_console) and for all crypted keys to access remote hosts (pam_ssh) . Dmitry, I'll use the last Summary you proposed. Regarding the use of pam_ssh related ssh-agent information I didn't said that users sould use the information setup by pam_ssh. But the same user login with or without pam_ssh should be able to use that information. Imagine that your setup is used on the computer zeus, your login is dumas. pam_ssh is used for pam.d/login and pam.d/gdm but not for pam.d/sshd. There is an ssh server running on zeus. You login at zeus gdm, this starts an ssh-agent. Now you walk to another room and login with ssh to zeus. If you can read the information setup by pam_ssh, like eval `cat /var/run/pam_ssh/dumas`, you will use the agent. It doesn't means that other user need have access to your pam_ssh information but you need to have that access. It is possible if file is -r--r--r-- root user Or if file is -r-------- user user In that case the user may modify the file content (but not remove it). If you think that this sis a usefull feature, please implement the one you prefer in your patch, otherwise I could do it too. I shall think about it and probably implement some variant in my patch, tomorrow... Tom, What your opinion on it? (comment #18, comment #24, comment #26, ...) Well, I think you should run the idea past the upstream maintainer. If he thinks its a good idea to make the permissions restrictive, then I'd go with that. There is certainly precedent in pam for doing a restrictive model. Patrice, As there is a "stand-alone" patch (pam_ssh-1.91-getpwnam.patch), you can now just use the session phase for your remote ssh login. It was impossible with the current upstream version, because it does not allow to use the session phase separately from the auth phase. (Typically., SSH keys are used for remote ssh login authentication, not pam auth). Now you can just use /etc/pam.d/sshd as: #%PAM-1.0 auth required pam_stack.so service=system-auth auth required pam_nologin.so account required pam_stack.so service=system-auth password required pam_stack.so service=system-auth session required pam_stack.so service=system-auth session optional pam_ssh.so (Make sure sshd`s option "UsePrivilegeSeparation" is set to "no", IMHO). As you see, there is no more necessity to "cat /var/run/pam_ssh/<user>" by the unprivileged user. Does this way satisfy you? Is there any other practical cases where the files should be user-readable? If not, let`s keep the present restrictive variant. If somebody will complain, we shall change it at the next updating. This additionnal feature is indeed very suitable for the use I have of pam_ssh. However this force the sysadmin to use pam_ssh for every authentification channel. I still think that the user should be able to see the content of that file. The change should be documented anyway, in the man page. OK. There is no any information leaks if we made "r--r--r--" permissons. Typical contents of such files is: SSH_AUTH_SOCK=/tmp/ssh-nRQKz11544/agent.11544; export SSH_AUTH_SOCK; SSH_AGENT_PID=11545; export SSH_AGENT_PID; echo Agent pid 11545; Agent pid can be always found by anyone using "ps" command, auth sock can be found by "ls -l /tmp", etc. I have not found files under /var/run which would not belong to root (or other special account), therefore I don`t want to make these files owned by a user. What do you mean "documented anyway in the man page" ? There is no mention in pam_ssh.8 about ~/.ssh/agent-* files, therefore nothing to change... Patrice, Get the changed /var/run patch (makes "r--r--r--"): http://dmitry.butskoy.name/pam_ssh/pam_ssh-1.91-var_run.patch Thanks Dmitry, I am fine with your updated patch. Regarding documentation I know it wasn't documented but I added some documentation nevertheless to show how the fedora package differs from the upstream. The new srpm is here: http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-6.src.rpm For me it is ready to publish. pam_ssh-1.91-man_agent_files.diff: first, the word "Fedora" must begins Upper case :-) . Second, we should mention /var/run/pam_ssh/* filename, but IMHO the comment about upstream is extra. The majority of users, even advanced, do not know what "upstream" means... Anyway, pam_ssh.8 is not a place where it should be noted. About dependencies: IMHO "openssh-clients" always implies "openssh" too, may be specify "openssh-clients" only. And we must specify "Requires: openssl". "Openssh" implies it currently, but it is hypothetically possible that openssh uses another ssl-library (gnutls or mosilla-nss) :-) I will replace "upstream" with original. And if the place where it should be noted isn't pam_ssh.8, where is it? In pam_ssh.8 there is allready a fair amount of technical information, I think it shouldn't be bad to have that there. The README seems to terse to me for that use. Regarding dependencies: I removed openssh from Requires. We should not require openssl, and openssh doesn't require it. It requires libcrypto.so.5, just like pam_ssh and this dependency is picked up automatically by rpm. So if another library is used it will be picked up automatically too, still anothere devel package would have to be precised in BuildRequires. I even removed pam from the requires as it is picked up by libpam.so.0. I have checked that pam_ssh builds in mock. Here is the next srpm: http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-7.src.rpm comment #34, comment #35 -- IMHO these are cosmetic things, do not want to discuss about it. Tom, For me, the package is ready too. Good work guys. Close this when its built in the buildsystem. Patrice, What your plans about completion of this package (add to cvs etc.)? I now need to be sponsored, I think. Could it be done by you or by Tom? I cannot find the Sponsors page on the wiki. Patrice, I'll sponsor you. Go ahead and do your paperwork. Dmitry, do you want to be CCed for each bug report? Yes, Patrice, sure! So I added you to the owner.list. Tell me if you don't want to. I have built the pam_ssh package on all platforms. I made a mistake on FC-4 by doing the make tag before cvs commit, so the tag was recorded but not applied and after I commited I couldn't tag because the tag allready existed. So I added .1 to the release of FC-3 and FC-4 packages. If I am not wrong this should allow for updating between fc versions as 1.fc3 < 1.fc4 < fc5. Hmmm... May be FE admins can fix this mistake? (See https://admin.fedora.redhat.com/accounts/dump-group.cgi?group=cvsextras&role_type=sponsor&format=html) Anyway, currently it looks little bit ugly... 1.fc3 < 1.fc4 < fc5 - this isn't right fc5 < 1.fc3 < 1.fc4 is the correct ordering. So you should rebuild the fc5 version too. But it is possible to move the mistaken tag after the commit (too late now if you built the 1.fcx packages) - simply use: make tag TAG_OPTS=-F Or, just bump all three spec files to 2%{?dist}, retag them all, and rebuild them all. Done Package Change Request ====================== Package Name: pam_ssh New Branches: el6 Owners: buc Git done (by process-git-requests). |