Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226228 - Merge Review: pam
Summary: Merge Review: pam
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: F9MergeReviewTarget
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:20 UTC by Nobody's working on this, feel free to take it
Modified: 2008-01-23 22:57 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-01-23 22:56:55 UTC
kevin: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 20:20:27 UTC
Fedora Merge Review: pam

http://cvs.fedora.redhat.com/viewcvs/devel/pam/
Initial Owner: tmraz@redhat.com

Comment 1 Kevin Fenzi 2008-01-11 19:15:41 UTC
I'd be happy to review this... look for a full review in a bit. 

Comment 2 Kevin Fenzi 2008-01-11 21:16:48 UTC
See below - Package meets naming and packaging guidelines
See below - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
a6472db4afe13850cb401922211bba4e  ./Linux-PAM-0.99.8.1.tar.bz2
a6472db4afe13850cb401922211bba4e  ./Linux-PAM-0.99.8.1.tar.bz2.1
6b5fc356fdbb0b7cdbbdc80419043cac  ./Linux-PAM-0.99.8.1.tar.bz2.sign
6b5fc356fdbb0b7cdbbdc80419043cac  ./Linux-PAM-0.99.8.1.tar.bz2.sign.1
OK - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Spec has needed ldconfig in post and postun
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have dist tag
OK - Should package latest version
15 open bugs - check for outstanding bugs on package.

Issues:

1. I see that upstream is named Linux-PAM. Perhaps consider re-naming it?

2. Might add a comment about why this package needs it's own private copy
of the db package.

3. shouldn't the license of pam_tty_audit.c be GPLv2 per RedHat guidelines?

4. Can some of the tests and such be moved from the install section to a %test
section?
like the dlopen tests and so forth.

5. Might ask upstream to include a copy of the GPL COPYING file too.

6. Why strip the binaries?
# Forcibly strip binaries.
strip $RPM_BUILD_ROOT%{_sbindir}/* ||:

debuginfo should pull that out.

7. Might note that we can depreciate the pre/post hacks for USEMD5 after a while.

8. No need to require 'coreutils'.

9. 15 open bugs
You might look at https://bugzilla.redhat.com/show_bug.cgi?id=218063
and https://bugzilla.redhat.com/show_bug.cgi?id=428444 in particular.

10. rpmlint says:

pam.src:212: E: use-of-RPM_SOURCE_DIR

You should be able to remove the following lines from prep:
cp %{SOURCE5} .
cp %{SOURCE6} .
cp %{SOURCE7} .

Just refer to the sources directly when installing.

Ignore:

pam.src:246: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/security
pam.src:327: E: hardcoded-library-path in /lib/security
pam.src: W: strange-permission dlopen.sh 0755
pam.x86_64: E: setuid-binary /sbin/pam_timestamp_check root 04755
pam.x86_64: E: non-standard-executable-perm /sbin/pam_timestamp_check 04755
pam.x86_64: E: executable-marked-as-config-file /etc/security/namespace.init
pam.x86_64: E: non-readable /sbin/unix_update 0700
pam.x86_64: E: non-standard-executable-perm /sbin/unix_update 0700
pam.x86_64: E: setuid-binary /sbin/unix_chkpwd root 04755
pam.x86_64: E: non-standard-executable-perm /sbin/unix_chkpwd 04755
pam.x86_64: E: non-readable /etc/security/opasswd 0600
pam.x86_64: W: log-files-without-logrotate /var/log/faillog
pam.x86_64: W: conffile-without-noreplace-flag /etc/security/console.perms
pam.x86_64: W: conffile-without-noreplace-flag
/etc/security/console.perms.d/50-default.perms
pam.x86_64: W: dangerous-command-in-%post rm
pam.x86_64: E: zero-length /etc/security/opasswd

Fix if you like:

pam.src: W: mixed-use-of-spaces-and-tabs (spaces: line 130, tab: line 137)

11. Might add a %{?_smp_mflags} to make?


Comment 3 Tomas Mraz 2008-01-14 12:51:29 UTC
(In reply to comment #2)
> Issues:
> 
> 1. I see that upstream is named Linux-PAM. Perhaps consider re-naming it?
I don't think it's worth the hassle - on the administrative side and on the
users' confusion side as well.
 
> 2. Might add a comment about why this package needs it's own private copy
> of the db package.
OK, I've extended the comment on line 76.

> 3. shouldn't the license of pam_tty_audit.c be GPLv2 per RedHat guidelines?
No, this module will be upstreamed in the next upstream release, so it should
keep the preferred upstream licence.

> 4. Can some of the tests and such be moved from the install section to a %test
> section?
> like the dlopen tests and so forth.
What is the %test section good for? I cannot find any mention of %test anywhere.
I'd prefer to have these simple tests run as part of the build/install process,
they are pretty simple and fast.

> 5. Might ask upstream to include a copy of the GPL COPYING file too.
Will do.

> 6. Why strip the binaries?
> # Forcibly strip binaries.
> strip $RPM_BUILD_ROOT%{_sbindir}/* ||:
> 
> debuginfo should pull that out.
That is a workaround hack for an old problem with rpmbuild where it didn't strip
setuid binaries. Removed.

> 7. Might note that we can depreciate the pre/post hacks for USEMD5 after a while.
They are not too useful anymore and even can break things. Removed.

> 8. No need to require 'coreutils'.
Why not? I need 'install' in %post
 
> 9. 15 open bugs
> You might look at https://bugzilla.redhat.com/show_bug.cgi?id=218063
WONTFIXed - current rpm shouldn't complain anymore
> and https://bugzilla.redhat.com/show_bug.cgi?id=428444 in particular.
NOTABUG - there is already BuildRequires: libtool

> 10. rpmlint says:
> 
> pam.src:212: E: use-of-RPM_SOURCE_DIR
> 
> You should be able to remove the following lines from prep:
> cp %{SOURCE5} .
> cp %{SOURCE6} .
> cp %{SOURCE7} .
> 
> Just refer to the sources directly when installing.
Both changes done.

> Ignore:
> 
> pam.src:246: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/security
> pam.src:327: E: hardcoded-library-path in /lib/security
> pam.src: W: strange-permission dlopen.sh 0755
> pam.x86_64: E: setuid-binary /sbin/pam_timestamp_check root 04755
> pam.x86_64: E: non-standard-executable-perm /sbin/pam_timestamp_check 04755
> pam.x86_64: E: executable-marked-as-config-file /etc/security/namespace.init
> pam.x86_64: E: non-readable /sbin/unix_update 0700
> pam.x86_64: E: non-standard-executable-perm /sbin/unix_update 0700
> pam.x86_64: E: setuid-binary /sbin/unix_chkpwd root 04755
> pam.x86_64: E: non-standard-executable-perm /sbin/unix_chkpwd 04755
> pam.x86_64: E: non-readable /etc/security/opasswd 0600
> pam.x86_64: W: log-files-without-logrotate /var/log/faillog
> pam.x86_64: W: conffile-without-noreplace-flag /etc/security/console.perms
> pam.x86_64: W: conffile-without-noreplace-flag
> /etc/security/console.perms.d/50-default.perms
> pam.x86_64: W: dangerous-command-in-%post rm
> pam.x86_64: E: zero-length /etc/security/opasswd
> 
> Fix if you like:
> 
> pam.src: W: mixed-use-of-spaces-and-tabs (spaces: line 130, tab: line 137)
> 
> 11. Might add a %{?_smp_mflags} to make?
Unfortunately pam doesn't build with it yet. I'll fix the Makefiles in future
and add this then. 

Fixes are in pam-0.99.8.1-15.fc9.


Comment 4 Kevin Fenzi 2008-01-22 05:25:59 UTC
>> 1. I see that upstream is named Linux-PAM. Perhaps consider re-naming it?
>I don't think it's worth the hassle - on the administrative side and on the
>users' confusion side as well.

Yeah, likely so... just thought I would mention it.

>> 2. Might add a comment about why this package needs it's own private copy
>> of the db package.
>OK, I've extended the comment on line 76.

Great, thanks!

>> 3. shouldn't the license of pam_tty_audit.c be GPLv2 per RedHat guidelines?
>No, this module will be upstreamed in the next upstream release, so it should
>keep the preferred upstream licence.

ok. Fair enough.

>> 4. Can some of the tests and such be moved from the install section to a %test
>> section?
>> like the dlopen tests and so forth.
>What is the %test section good for? I cannot find any mention of %test anywhere.
>I'd prefer to have these simple tests run as part of the build/install process,
>they are pretty simple and fast.

Sorry, my mistake there. I meant a %check section...

http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-scripts.html#S3-RPM-INSIDE-CHECK-SCRIPT

>> 5. Might ask upstream to include a copy of the GPL COPYING file too.
>Will do.

Thanks.

>> 6. Why strip the binaries?
>> # Forcibly strip binaries.
>> strip $RPM_BUILD_ROOT%{_sbindir}/* ||:
>>
>> debuginfo should pull that out.
>That is a workaround hack for an old problem with rpmbuild where it didn't strip
>setuid binaries. Removed.

Great, thanks.

>> 7. Might note that we can depreciate the pre/post hacks for USEMD5 after a while.
>They are not too useful anymore and even can break things. Removed.

Great, thanks.

>> 8. No need to require 'coreutils'.
>Why not? I need 'install' in %post

http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions
coreutils is in the base build env.

>> 9. 15 open bugs
>> You might look at https://bugzilla.redhat.com/show_bug.cgi?id=218063
>WONTFIXed - current rpm shouldn't complain anymore
>> and https://bugzilla.redhat.com/show_bug.cgi?id=428444 in particular.
>NOTABUG - there is already BuildRequires: libtool

ok.

>> 10. rpmlint says:
>>
>> pam.src:212: E: use-of-RPM_SOURCE_DIR
>>
>> You should be able to remove the following lines from prep:
>> cp %{SOURCE5} .
>> cp %{SOURCE6} .
>> cp %{SOURCE7} .
>>
>> Just refer to the sources directly when installing.
>Both changes done.

Great, thanks.

>> Ignore:
>>
>> pam.src:246: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/security
>> pam.src:327: E: hardcoded-library-path in /lib/security
>> pam.src: W: strange-permission dlopen.sh 0755
>> pam.x86_64: E: setuid-binary /sbin/pam_timestamp_check root 04755
>> pam.x86_64: E: non-standard-executable-perm /sbin/pam_timestamp_check 04755
>> pam.x86_64: E: executable-marked-as-config-file /etc/security/namespace.init
>> pam.x86_64: E: non-readable /sbin/unix_update 0700
>> pam.x86_64: E: non-standard-executable-perm /sbin/unix_update 0700
>> pam.x86_64: E: setuid-binary /sbin/unix_chkpwd root 04755
>> pam.x86_64: E: non-standard-executable-perm /sbin/unix_chkpwd 04755
>> pam.x86_64: E: non-readable /etc/security/opasswd 0600
>> pam.x86_64: W: log-files-without-logrotate /var/log/faillog
>> pam.x86_64: W: conffile-without-noreplace-flag /etc/security/console.perms
>> pam.x86_64: W: conffile-without-noreplace-flag
>> /etc/security/console.perms.d/50-default.perms
>> pam.x86_64: W: dangerous-command-in-%post rm
>> pam.x86_64: E: zero-length /etc/security/opasswd
>>
>> Fix if you like:
>>
>> pam.src: W: mixed-use-of-spaces-and-tabs (spaces: line 130, tab: line 137)
>>
>> 11. Might add a %{?_smp_mflags} to make?
>Unfortunately pam doesn't build with it yet. I'll fix the Makefiles in future
>and add this then.

Great. You might add a comment to the spec when you get a chance
mentioning this so it's not added in before it's ready upstream.

>
>Fixes are in pam-0.99.8.1-15.fc9.
>

Looks good. You might revisit items 4, 8 and 11, but none of them
are blockers at all. I see no further issues, so this package is
APPROVED.

Feel free to close RAWHIDE when you have looked at 4, 8 and 11 again. 

Comment 5 Patrice Dumas 2008-01-22 08:28:15 UTC
(In reply to comment #4)
> 
> >> 8. No need to require 'coreutils'.
> >Why not? I need 'install' in %post
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions
> coreutils is in the base build env.

But not necessarily in the install environment. So 
Requires(post) is needed to ensure that coreutils is installed 
before pam.

Comment 6 Tomas Mraz 2008-01-23 22:56:55 UTC
I've fixed both issues 4 and 11. 8 is actually right as it is as Patrice already
said above.


Comment 7 Tomas Mraz 2008-01-23 22:57:15 UTC
BTW, thanks for the review.


Note You need to log in before you can comment on or make changes to this bug.