Bug 165899
Summary: | Review Request: pam_pkcs11 : PKCS #11 PAM login module | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Tom "spot" Callaway <tcallawa> |
Component: | Package Review | Assignee: | Ville Skyttä <scop> |
Status: | CLOSED RAWHIDE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dcantrell, dmitry, fedora-package-review, jmccann, jonsito, notting, rrelyea, rstrode, tmraz |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://www.opensc.org/pam_pkcs11 | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-07-17 19:29:13 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: | 188268 |
Description
Tom "spot" Callaway
2005-08-13 20:03:36 UTC
Glad to see you're submitting this, will review. Some initial comments from skimming the specfile, not actually tested yet: - pam_pkcs11 0.5.2 is out (and renamed to pam_pkcs11 upstream), see http://oasis.dit.upm.es/~jantonio/pam-pkcs11/downloads/ , maybe update? - License seems to be LGPL, not GPL - Package is self-obsoleting, suggesting changing the obsoletes to "<" (or dropping them altogether), also in -tools - I wonder if automake is needed for building (not checked), I don't see it called and no patches are applied etc - Unowned %{_sysconfdir}/pkcs11, %{_libdir}/pam_pkcs11 and %{_datadir}/pkcs11_login dirs - Shouldn't the /etc stuff be somewhere below /etc/pki? - configuring with --disable-static could be cleaner (untested) Then, a related note: pam_opensc will most likely be dropped in the next upstream opensc release, with this being the replacement (unbundled, I hope). At that time, I think I would be good to respin this one and add Obsoletes: pam_opensc < $somever here instead of the opensc package. But that's in the future, I don't think we should rush that yet. New SRPM: http://www.auroralinux.org/people/spot/review/pam_pkcs11-0.5.2-1.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/pam_pkcs11.spec I don't see any problem with it Obsoleting pam_opensc when the time comes. --disable-static didn't work, it broke the compile. All other changes were made. Still just poking around, I have nothing to test this with right now (but will have later this week), assorted notes (some of them cosmetic): - I would personally just nuke the whole %{_datadir}/pam_pkcs11 dir, the same files are already installed in /etc/pki/pkcs11 as config files. Exception: pam.d_login.example could be added to main package's %doc, it doesn't seem to be available elsewhere. - Maybe move the default location of the "default" module to somewhere below %{_libdir} too, /lib/security appears somewhat kind of reserved for pam modules and pkcs11_module.so there seems slightly misplaced to me. This wouldn't cause any /lib vs /usr/lib mountedness problems; pam_pkcs11.so won't function anyway if /usr is not mounted. Upstream seems to have changed to /usr/lib/pam_pkcs11/pkcs11_module.so in svn, http://www.opensc.org/pam_pkcs11/file/trunk/etc/pam_pkcs11.conf.example - Hardcoded /lib/security in src/pam_pkcs11/Makefile* will probably break on x86_64 and friends. Maybe "%ifarch x86_64 ppc64 sparc64 ia64" (dunno what's the exact list of potentially affected archs) and just move the module to /lib64/security if so. - Ditto, hardcoded /usr/lib/pkcs11/opensc-pkcs11.so for the opensc PKCS #11 module location and /usr/lib/pam_pkcs11 for the mappers will cause problems on the above archs. - %changelog mentions --disable-static, but it's not used - %defattr missing from -tools - Doubly-owned %{_sysconfdir}/pki/pkcs11 in both main package and -tools, doesn't really hurt, though. With the above taken care of, go ahead and commit to CVS, the rest can be worked out there before the first build. I'll recheck a bit later when I have access to a smart card reader again. ping :) OK. -2 with all those fixes is checked into CVS. More findings, this time actually testing the module: - Path to pam_pkcs11.conf in the pki patch is wrong, should be /etc/pki/pkcs11, not /etc/pki/pam_pkcs11. - pam_pkcs11.conf mentions $HOME/.ssh/authorized_keys for the opensc mapper, that's almost certainly wrong and should probably be $HOME/.eid/authorized_certificates - Security: all /etc/pki/pkcs11/*_mappers contain _enabled_ example entries, they should be commented out or removed. - Undefined symbols: trying to use the opensc mapper (with debug on) barfs: [...] DEBUG:mapper_mgr.c:77: dlopen failed for module: opensc path: /usr/lib/pam_pkcs11/opensc_mapper.so Error: /usr/lib/pam_pkcs11/opensc_mapper.so: undefined symbol: is_empty_str DEBUG:mapper_mgr.c:77: dlopen failed for module: null path: /usr/lib/pam_pkcs11/null_mapper.so Error: /usr/lib/pam_pkcs11/null_mapper.so: undefined symbol: mapper_find_user [...] - Note: FC-3 doesn't have /etc/pki; you decide whether that matters and whether to do something about it in this package. - Cosmetics from comment 3 (feel free to ignore, won't mention these again): 0.5.2-1 changelog still mentions --disable-static being used, but it's not. Also, the double ownership of the /etc/pki/pkcs11 dir remains in main and -tools %files. - /usr/bin/make_hash_link.sh smells a bit too generic IMHO, could consider moving it eg. to /etc/pki/pkcs11, you decide. Hmm, actually it seems that /etc/pki/pam_pkcs11 is used consistently throughout the pki patch, but the specfile installs stuff to /etc/pki/pkcs11. Maybe fix the specfile instead of the patch. Re: opensc mapper: http://opensc.org/pipermail/opensc-devel/2005-August/006936.html Drop it and the config file part for now? The mapper is not useful for anything AFAICT, see its code... More fun, digest mapper bombs after verifying the CA cert, let me know if you want a backtrace: *** glibc detected *** su: free(): invalid next size (fast): 0x0913ae08 *** ======= Backtrace: ========= /lib/libc.so.6[0x5eb124] /lib/libc.so.6(__libc_free+0x77)[0x5eb65f] /usr/lib/pam_pkcs11/digest_mapper.so(get_mapent+0x22)[0xeec135] /usr/lib/pam_pkcs11/digest_mapper.so(mapfile_find+0x120)[0xeec476] /usr/lib/pam_pkcs11/digest_mapper.so(mapfile_match+0x30)[0xeec5a6] /usr/lib/pam_pkcs11/digest_mapper.so[0xeec7c7] /lib/security/pam_pkcs11.so(match_user+0x73)[0xa66f6f] /lib/security/pam_pkcs11.so(pam_sm_authenticate+0x7d4)[0xa661d1] /lib/libpam.so.0(_pam_dispatch+0x28c)[0xda8d8c] /lib/libpam.so.0(pam_authenticate+0x47)[0xdaae9b] su(main+0x30c)[0xd0682f] /lib/libc.so.6(__libc_start_main+0xdf)[0x59cd5f] su[0xd05941] Doesn't look too good :( What do you think about similar packages pamusb (http://pamusb.sourceforge.net) and pam-x509 (http://pam-x509.sourceforge.net) ? Whether has sense to add them to FE too? I'm not familiar with either of those, but they seem useful and substantially different from pam_pkcs11. OTOH, the two seem to have pretty much equivalent functionality so maybe pick what you (or whoever packages them) considers better, and submit it first. FWIW, the last release of pam-x509 was almost 2 years ago and for pam_usb some 9 months ago, and both are listed as "Production/Stable". I'm Juan Antonio MartÃnez, co-author and current maintainer of pam_pkcs11 at http://www.opensc.org/pam_pkcs11 First of all, I'm glad to see interest on our PAM module, but what about emailing us and notifying that some work in porting to Fedora was in progress?. tsk, tsk, nettiquette please. ( Mantainers of other distros already contacted us ) Perhaps you'd better to test and port our new incomming pam_pkcs11-0.5.3 package, actually in test stage. Release is scheduled at Sept 12 2005 You'll notice that most of bugs described here are solved. 0.5.2 is really a obsolete version. In particular neither opensc, nor openssh, nor ldap mappers works, and there are known bugs in mapfile handling btw Incomming 0.5.3 is intensively used in some production environments, and is full docummented (man pages, html docs, doxygen API and so ) I'm really interested in rpm packaging (I work in a FC4 environment). So perhaps you'll find better working together Please take a look at svn tree, and send comments, suggestions and patches to jonsito _at_ teleline _dot_ es, or even better to OpenSC development list opensc-devel _at_ lists _dot_ opensc _dot_ org Juan Antonio (In reply to comment #11) > what about > emailing us and notifying that some work in porting to Fedora was in > progress?. tsk, tsk, nettiquette please. Heh, I _did_ notify the opensc list about this submission half an hour ago ;) Umm, because I'm swamped? :) If you're interested in rpm packaging, perhaps you should maintain this package? I'm more than willing to let the upstream maintainer own this. "If you're interested in rpm packaging, perhaps you should maintain this package?" Mmmh. Not sure: I don't really know the intrinsics of Fedora Team: release politics, Filesystem structure, and so. In fact the OpenSC team philosophy is not to distribute binaries, just sources. ( "Let the coders code and packagers package", or as we say in Spain "Zapatero a tus zapatos" :-) For Pam_pkcs11, I only provide a "reference .spec file", not a FC4 .spec one. Of course, i've tested it under FC4, but certainly sure that is not the correct one. I lack of Fedora RPM HOWTO knowledge... Anyway, I'm interested in: - Knowing progress on pam_pkcs11 packaging on several distros (AFAIK pam_pkcs11 is distributed by Mandriva, Debian, and Fedora) - Get feedback from packagers, and perhaps include a "dist_patches" directory into OpenSC svn. Not really interested in mantain several trees. - Receive notices, suggestions, bug-reports, contribs and so from Fedora channels As I told Ville Skyttä, I think pam_pkcs11 should not be "Fedora oriented". So let me stand as neutral as possible. But let me being informed too :-) Juan Antonio BTW: where can I find Fedora packaging and FHS guidelines? Packaging guidelines can be found at: http://fedoraproject.org/wiki/PackagingGuidelines There is also a useful checklist at: http://fedoraproject.org/wiki/PackageReviewGuidelines There are also work-in-progress guides: http://fedoraproject.org/wiki/Docs/Drafts/BuildingPackagesGuide http://koti.welho.com/vskytta/packagers-handbook/packagers-handbook.html I don't know of any Fedora FHS guidelines yet, so I try to look at what other packages do. I don't think there is such a thing as "Fedora FHS guidelines", there's just the FHS, http://www.pathname.com/fhs/ Once this package is in, it's trivial to add your email address to the initial Cc list so you'd be Cc'd for all Bugzilla traffic related to this package, if that's what you want. FYI: pam_pkcs11-0.5.3 just released. It fixes all the problems related here, plus adding lots of features http://www.opensc.org/files/pam_pkcs11-0.5.3.tar.gz I'm in the progress of updating this submission to 0.5.3. and I'm wondering whether the "badstatic" patch is still required. 0.5.3 appears to compile fine without it; not yet runtime-tested with nor without. Tom, could you clarify what is it for, and Juan, could you have a look at the patch? It doesn't apply as-is to 0.5.3, but the intention should be clear. http://cvs.fedora.redhat.com/viewcvs/rpms/pam_pkcs11/devel/pam_pkcs11-0.5.2-badstatic.patch?root=extras&rev=1.1&view=auto BTW, in the process of updating, I submitted a couple of new patches upstream: http://www.opensc.org/pam_pkcs11/ticket/9 http://www.opensc.org/pam_pkcs11/ticket/10 http://www.opensc.org/pam_pkcs11/ticket/11 Tom, Juan, could you clarify the status of the "badstatic" patch, see comment 18? OpenSC 0.10.0 is out, as expected without the pam module, so I made this obsolete pam_opensc < 0.10.0 in CVS (and this bug is now blocking my OpenSC update). Still waiting for comments about what to do with the "badstatic" patch, see comment 18. Tom, Juan, anyone? Sorry for the delay... I'm a bit busy :-( In release 0.5.3 FIND_USER, MATCH_USER, FIND_ENTRIES and MODULE_END _must_ be declared static, as they may be statically linked, to avoid "duplicate symbols" MODULE_INIT should never be used, unless included in dynamically loaded mappers Anyway, in current tree, "match", "find", and "entries" are used only by null mapper, that is to be statically linked. So let it remain "static" to get sure that these functions will not be duplicated In fact, my idea is remove remove these macros on next release, as they were a dirty patch for non-complete mappers Juan Antonio PS: in a few weeks I'll restart work on pam_pkcs11. Okay, thanks for the comments, Juan. I have now committed my local changes to CVS. Tom, please note that there are differences between devel and FC[34]; devel is tuned for OpenSC 0.10.0 and FC[34] for 0.9.6 (paths, Obsoletes), and I intend to keep OpenSC that way for the foreseeable future, so please don't do blind branch syncs in this package :) There's one more thing that would be good to address before releasing: I think make_hash_link.sh is a too generic name for a script to be included in /usr/bin. Tom, feel free to address (or not) this the way you see fit; as far as I'm concerned, this is approved. ping? pong :-) About make_hash_link.sh . My last svn commit to opensc.org site makes pam_pkcs11 now handle either hash_dir (old behaviour) or cacert files. In the later case, there is no real need to use make_hash_link.sh script anymore. My plan is move this script to /usr/lib/pam_pkcs11 (or whatever site according to LSB) Just starting several "semi-cosmetic" changes to code and preparing a new 0.6 release Juan Antonio Thanks for the update. I'll let spot decide whether to wait for 0.6 or publish the current stuff in Extras CVS. I'm going to wait for 0.6. Juan, if you'd be so kind as to notify me when it releases, I would be grateful. :) Any news from 0.6 (I can't find www.opensc.org) ? No idea, but the website is (temporarily?) at http://www.opensc-project.org/ I'm temporally off. My Idea is to fix some minor issues on current svn version and release it as 0.6 on April, as soon as I return to my "normal" life So it's time for fixes and patches. No more features planned at this moment Hi all, I didn't realize we had a proposed pam_pkcs11 package up. We need this for smart card login. I have some rpm with 0.5.3 and some changes so it can use NSS rather than raw pkcs_11. Package is here: The latest versions appear to be at: http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pam_pkcs11.spec http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pam_pkcs11-0.5.3-3.src.rpm thanks. bob I am happily handing this over to Bob for ownership. This should be Core, not Extras. Hi Bob, - I'm not sure what our policy on Epoch is (whether to explicitly list it or not when it's 0), but you might want to check that. - You might want to %define macros for the version numbers in your dependencies - You require automake but you don't use it - You might want to follow the more standard %patchN -p1 -b .description convention for patch lines - Should we standardize on an nss dir? maybe /etc/pki/nss or something? I think there is some precedent here, but I don't know what it is. - make_hash_link.sh is a horrible name for something in %{_bindir}, can we rename it or move it to libexecdir or something? Maybe mark the example files as %doc instead of shipping them in datadir? If epoch is 0, don't list it explicitly. Thanks. I'll make those changes. Hmm a common nssdir for system apps does sound like a good idea. /etc/pki/nss sounds reasonable. Users should have their own, obviously. bob Hope I'm not stepping on any toes but I've tried to update the SRPMS based on Ray's comments. http://acs.pha.jhu.edu/~mccannwj/pam_pkcs11/pam_pkcs11-0.5.3-3.src.rpm http://acs.pha.jhu.edu/~mccannwj/pam_pkcs11/pam_pkcs11.spec For now I've disabled building the ldap mapping because it fails to compile with the following message: http://acs.pha.jhu.edu/~mccannwj/pam_pkcs11/pam-pkcs11-ldap-failure.txt Suggestions are quite welcome! even in the form of a complete source RPM.;) I've taken your updated spec and created a new one, however. The rpm includes the proper Makefile.in, so I've removed the automake requires rather than accually use automake. Your spec contained some ideas i've incorporated into my new spec file. New files are available at: http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pam_pkcs11.spec http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pam_pkcs11-0.5.3-3.src.rpm thanks, bob Why do you prefer to patch Makefile.in rather than Makefile.am ? Isn't patching a generated file rather fragile? I'm patching both. I patch the .am file, then I run automake (carefully using the same version of automake as the original source) to generate the .in file, then patching both. This means builds can be made without automake, but someone can take the source rpm, make their own changes to .am and run automake themselves. bob THis is now under Core review, need to get this review finished so the rest of the smart card stuff can come in. The packages are looking pretty good to me. Let's get them in. Bill what do you think? Works for me. Accepting. Please let me know when you build this into rawhide and I'll make the comps changes for this package set. pam_pkcs11 has now been imported. bob Built into rawhide. |