tags 496924 wontfix thanks On Thu, Aug 28, 2008 at 07:18:43PM +0200, Juan Luis Belmonte wrote: > As part of the GSoC project PamNssInstaller > (http://wiki.debian.org/SummerOfCode2008/PamNssDebianInstaller). > [libpam-runtime] > - update-pam tool added, tool to manage pam.d/ files > - added lines in debian/rules and debian/libpam-runtime.install for the > installation fo the tool
> [libpam-doc] > -update-pam manpage > -added lines in doc/man/Makefile.in for the manpage installation Unfortunately, as we discussed by IRC/mail, this tool does not sufficiently cover the relevant use cases for me to be willing to accept this patch. - The syntax requires one to specify the ordering of modules when invoking the tool, which means that the caller has to a) know what other modules are enabled, and b) know where the requested module should be ordered with respect to each. This is too low-level, and not scalable. - The tool has to be invoked separately for each of auth, account, password, and session, implying that these are always separable. They are not; there are cases where adding a module to, e.g., account but not auth will result in a broken stack, which means that adding configs of the various types needs to be treated as atomic from the point of view of the caller. - There is insufficient information stored by this tool to support idempotent operation (e.g., removing a line a second time due to maintainer script failure after the line has already been removed), or to recognize that an admin has made local changes overriding the tool and that these should not be overwritten without first asking the admin for consent. - There is no interface for interacting with the user, to allow the user to select which installed modules should be enabled. - The tool permits editing of other config files under /etc/pam.d/, besides /etc/pam.d/common-*. These files are conffiles belonging to the individual services, not to PAM; and editing them in this manner is a violation of two policy requirements. It's most regrettable that your GSoC mentor didn't talk to me before you began coding, because this issue is one that I've been working on intermittently since last November, and actively since June. I would have been happy to provide you with guidance so that you were working towards an acceptable design. Since this wasn't done, I have myself been working in the meantime on implementing the required tool, which is now in beta testing in Ubuntu, and available as a bzr branch at <nosmart+http://bzr.debian.org/pkg-pam/debian/features/config-framework/>. I plan to upload this tool to Debian as soon as lenny is released, at which point I will close this bug. There are other minor issues that I see with the patch that could be easily overcome, if not for the design flaws. Here's a list of some of these, in the hopes that you find this feedback useful: - Your patch installs this tool in /sbin. Since this is not needed for system recovery, it does not belong in /sbin; it's an administration tool, and should be shipped in /usr/sbin. - use File::Basename qw(basename); use File::Temp qw(tempfile); These modules are included in the 'perl-modules' package, not in perl-base; using them would require a dependency on perl-modules, which is ill-advised because libpam-runtime is part of the Essential closure (login Pre-Depends: libpam-runtime), and perl-modules is explicitly not. - The manpage handling is done as a patch to upstream build rules. Since I don't think this tool would ever be submitted to Linux-PAM upstream (it encodes logic specific to decisions taken by the Debian PAM packages), this doesn't seem appropriate. - [\s\t]+ This is bad regex style; "\s" means "any whitespace character", which implies \t :) - The indentation of the script is very inconsistent, which makes it difficult to follow. You might want to use something like perltidy on your scripts, so that blocks are indented consistently as this improves legibility and maintainability. Cheers, -- Steve Langasek Give me a lever long enough and a Free OS Debian Developer to set it on, and I can move the world. Ubuntu Developer http://www.debian.org/ [EMAIL PROTECTED] [EMAIL PROTECTED] -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
