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