Bug 167714 - Review Request: pam_usb: PAM module for use with DSA key pairs and removable devices
Summary: Review Request: pam_usb: PAM module for use with DSA key pairs and removable...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: David Lawrence
URL: http://www.pamusb.org
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-09-07 15:19 UTC by Dmitry Butskoy
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2005-12-29 14:37:18 UTC
Type: ---
jwboyer: fedora-cvs+


Attachments (Terms of Use)

Description Dmitry Butskoy 2005-09-07 15:19:31 UTC
Spec Url: http://dmitry.butskoy.name/pam_usb/pam_usb.spec
SRPM Url: http://dmitry.butskoy.name/pam_usb/pam_usb-0.3.2-1.src.rpm

Description: 
This PAM module provides authentication through DSA private/public keys.
Public keys placed on the target computer, whereas private keys are stored
on some removable device, including USB storages/flash drives, cdroms,
floppies, etc.

Any kind of mountable devices (not removable only) can be used.

Due to using of DSA key pairs, the passwordless authentication can be organized
(if a private key is stored not crypted on the media).



Additional info:

I separate hotplug stuff as an additional binary package, default not built (IMHO, it looks like not related to pam_usb, still not documented, etc.)

There is an article about pam_usb, a tip of the week at fedoranews site: http://fedoranews.org/mediawiki/index.php/Tip_of_the_Week_2005-08-08

Comment 1 Wart 2005-12-19 23:53:14 UTC
I haven't been approved as a reviewer yet, so this remains an unofficial review.
 I'll take reviewing responsibility as soon as I have permission to do so.

Good:
+ Source matches upstream
+ GPL license ok.  Includes license text.  Matches upstream.
+ ChangeLog ok.
+ No tag violations
+ BuildRoot ok
+ No unnecessary Requires
+ No unnecessary BR
+ Summary and description ok.
+ Includes relevant documentation.
+ No desktop file needed.
+ Consistent macro usage.
+ Not relocatable
+ Code, not content
+ Package names contains an underscore, but so does upstream, so I'm willing to
ignore it.
+ Source matches upstream
+ 0wns its own directories.
+ Shared library not in default linker path; ldconfig not needed.
+ No duplicates in %files
+ permissions look ok
+ Builds in mock (for i386)


Needswork:
- Doesn't install on x86_64.  spec file correctly uses %{_lib}, but
  src/Makefile hardcodes $(DESTDIR)/lib/.
- Conditional compilation of hotplug:  Is this really necessary?  For FE I
  would think that you'd always want to build the hotplug module.  I would
  recomend either removing this conditonal completely, or changing it so
  that the hotplug package is built by default.

Check:
. Runs - I'll test that as soon as the x86_64 package works.


Additional comments:

rpmlint output seems harmless:
W: pam_usb unstripped-binary-or-object /lib/security/pam_usb.so

Commas are not necessary in BuildRequires.  My preferencs is to remove
them, but this is not a blocker.

It looks like 0.3.3 is available upstream.  Have you considered upgrading?


Comment 2 Dmitry Butskoy 2005-12-20 12:13:03 UTC
> src/Makefile hardcodes $(DESTDIR)/lib/.
Add a patch for Makefiles for this and another issues.

> Conditional compilation of hotplug
Now just a separate package, compiled unconditionally

> pam_usb unstripped-binary-or-object /lib/security/pam_usb.so
Now stripped properly...

> It looks like 0.3.3 is available upstream. 
Upgraded.


New SPEC: http://dmitry.butskoy.name/pam_usb/pam_usb.spec
New SRPM: http://dmitry.butskoy.name/pam_usb/pam_usb-0.3.3-1.src.rpm




Comment 3 Ville Skyttä 2005-12-20 15:45:24 UTC
Note that hotplug agents are pretty much obsoleted by udev in FC5.  For an
example, see the openct package in Extras CVS (devel branch).

Comment 4 Dmitry Butskoy 2005-12-20 16:55:09 UTC
Does it mean that pam_usb-hotplug will not work under FC5 completely?
In other words, should we do separate things for FC5 or not?

Comment 5 Wart 2005-12-21 06:00:03 UTC
(In reply to comment #4)
> Does it mean that pam_usb-hotplug will not work under FC5 completely?
> In other words, should we do separate things for FC5 or not?

That's how I understand it.  When using udev on FC4 the -hotplug package isn't
needed either.  Depending on which older releases you want to package this for,
you could possibly get rid of the -hotplug package altogether.

btw, I finally got around to running this package and it works great.  :)

The only other suggestion that I would have is to include a couple of sample
pam.d configuration files in %doc that show how it should be used in Fedora Core
4.  For example, I had to add the following just before the pam_stack.so line in
/etc/pam.d/login:

auth  sufficient  pam_usb.so check_if_mounted mount_opts=ro quiet

but had to use the following in /etc/pam.d/gdm:

auth  sufficient  pam_usb.so check_if_mounted mount_opts=ro allow_remote

...and still haven't gotten /etc/pam.d/xscreensaver to work right.  In debug
mode the logs indicate that authentication succeeds, but it doesn't actually log
me back in.

Comment 6 Dmitry Butskoy 2005-12-22 15:31:42 UTC
* add /etc/udev/rules.d file instead of /etc/hotplug.d

* add your pam.d examples in %doc

New SPEC: http://dmitry.butskoy.name/pam_usb/pam_usb.spec
New SRPM: http://dmitry.butskoy.name/pam_usb/pam_usb-0.3.3-2.src.rpm

> and still haven't gotten /etc/pam.d/xscreensaver to work right.
Perhaps it could chagnge after switch to udev ...



Comment 7 Wart 2005-12-28 23:36:20 UTC
All needswork items have been fixed.  Package now compiles and runs on x86_64.

rpmlint has no complaints for the base package, and the following output for
pam_usb-hotplug, both of which can be ignored.

W: pam_usb-hotplug no-documentation
E: pam_usb-hotplug executable-marked-as-config-file /etc/pam_usb/handlers/xlock.sh

APPROVED

Comment 8 Dmitry Butskoy 2007-05-02 10:39:41 UTC
I'm orphaning this, please, change the initial owner in owners.list .


Note You need to log in before you can comment on or make changes to this bug.