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