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?? :-)

Here it is:

http://www.environnement.ens.fr/docs/fc-srpms/pam_ssh-1.91-3.src.rpm

  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).