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
Summary: | Review Request: pam_usb: PAM module for use with DSA key pairs and removable devices | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Dmitry Butskoy <dmitry> |
Component: | Package Review | Assignee: | Wart <wart> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-extras-list |
Target Milestone: | --- | Flags: | jwboyer:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://www.pamusb.org | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-12-29 14:37:18 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
Dmitry Butskoy
2005-09-07 15:19:31 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? > 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 Note that hotplug agents are pretty much obsoleted by udev in FC5. For an example, see the openct package in Extras CVS (devel branch). 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? (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. * 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 ... 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 I'm orphaning this, please, change the initial owner in owners.list . |