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 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: ---
Embargoed:
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.