Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225292
Summary: Merge Review: audit Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: nobody@fedoraproject.org QAContact: fedora-package-review@redhat.com CC: sgrubb@redhat.com
Fedora Merge Review: audit
http://cvs.fedora.redhat.com/viewcvs/devel/audit/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora Version|devel |rawhide
tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |426387 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |kevin@tummy.com Status|NEW |ASSIGNED Flag| |fedora-review?
------- Additional Comments From kevin@tummy.com 2008-01-11 11:07 EST ------- I'll go ahead and review this... look for a full review in a bit.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-01-11 12:47 EST ------- OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (LGPLv2+, GPLv2+) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: 53ede8c7422cb251d01d06c7a5e3027b audit-1.6.5.tar.gz 53ede8c7422cb251d01d06c7a5e3027b audit-1.6.5.tar.gz.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 - Headers/static libs in -devel subpackage. See below - 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. See below - Package is a GUI app and has a .desktop file
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 subpackages require base package with fully versioned depend. OK - Should have dist tag OK - Should package latest version OK - check for outstanding bugs on package.
Issues:
1. You should not call rpm from inside a spec file. Just use the hard coded version of selinux-policy thats available?
2. The Source0 url should be: http://people.redhat.com/sgrubb/audit/audit-1.6.5.tar.gz
3. Why is the check section commented? Not working? Perhaps add a comment why it's not working and when it's expected to be added.
4. Do you need to ship static libs here?
5. The postun for libs can be simplified from: %postun libs /sbin/ldconfig 2>/dev/null
to:
%postun libs -p /sbin/ldconfig
6. Please use desktop-file-install to install the desktop file... see: http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce...
7. You can probibly remove Prereq: coreutils
8. rpmlint says:
The following seem ignorable:
audispd-plugins.x86_64: E: non-readable /sbin/audispd-zos-remote 0750 audispd-plugins.x86_64: E: non-standard-executable-perm /sbin/audispd-zos-remote 0750 audispd-plugins.x86_64: E: non-readable /etc/audisp/plugins.d/audispd-zos-remote.conf 0640 audispd-plugins.x86_64: E: non-readable /etc/audisp/zos-remote.conf 0640 audispd-plugins.x86_64: E: non-readable /etc/audisp/plugins.d/syslog.conf 0640 audispd-plugins.x86_64: W: non-conffile-in-etc /etc/audisp/plugins.d/syslog.conf audit.x86_64: E: non-standard-dir-perm /etc/audisp/plugins.d 0750 audit.x86_64: E: non-readable /etc/audit/audit.rules 0640 audit.x86_64: E: non-standard-dir-perm /usr/lib64/audit 0750 audit.x86_64: E: non-readable /sbin/aulastlog 0750 audit.x86_64: E: non-standard-executable-perm /sbin/aulastlog 0750 audit.x86_64: E: non-standard-dir-perm /etc/audit 0750 audit.x86_64: E: non-readable /sbin/autrace 0750 audit.x86_64: E: non-standard-executable-perm /sbin/autrace 0750 audit.x86_64: E: non-readable /sbin/auditctl 0750 audit.x86_64: E: non-standard-executable-perm /sbin/auditctl 0750 audit.x86_64: E: non-readable /sbin/auditd 0750 audit.x86_64: E: non-standard-executable-perm /sbin/auditd 0750 audit.x86_64: E: non-readable /etc/audisp/audispd.conf 0640 audit.x86_64: E: non-readable /sbin/audispd 0750 audit.x86_64: E: non-standard-executable-perm /sbin/audispd 0750 audit.x86_64: E: non-readable /etc/audit/auditd.conf 0640 audit.x86_64: E: non-standard-dir-perm /etc/audisp 0750 audit.x86_64: E: non-readable /etc/audisp/plugins.d/af_unix.conf 0640 audit.x86_64: E: non-standard-dir-perm /var/log/audit 0750 audit.x86_64: E: non-readable /etc/sysconfig/auditd 0640 audit-libs.x86_64: W: no-documentation audit-libs.x86_64: E: non-readable /etc/libaudit.conf 0640 audit-libs-python.x86_64: W: no-documentation audit-libs-python.x86_64: E: non-standard-executable-perm /usr/lib64/python2.5/site-packages/auparse.so 0775 audit.x86_64: W: service-default-enabled /etc/rc.d/init.d/auditd audit.x86_64: W: service-default-enabled /etc/rc.d/init.d/auditd
These seem like they should be addressed:
audit.x86_64: W: non-conffile-in-etc /etc/audisp/plugins.d/af_unix.conf
Make it a config?
audit.src:21: W: prereq-use coreutils
Remove it?
audit.src:232: E: hardcoded-library-path in /usr/lib/python?.?/site-packages/audit.py*
Are these arch independent? Then this can be ignored.
audit.x86_64: W: log-files-without-logrotate /var/log/audit
Add a logrotate file?
audit.x86_64: W: dangerous-command-in-%post mv
Do you need those upgrade commands in post anymore? Perhaps remove them or comment on what version is migrated here so it can be removed down the road.
system-config-audit.x86_64: W: symlink-should-be-relative /usr/libexec/system-config-audit-server /usr/bin/consolehelper
Make this a relative link?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-01-11 15:46 EST ------- BTW, I use essentially the same spec file for upstream, RHEL, and Fedora. I don't like making changes for one that affects another since the audit system is under heavy development. If it were an older stable package, I wouldn't care so much.
#1 fixed, #2 fixed but I like the shorter version better...why else have an url? #3 its a reminder to get it working at some point - added a comment, #4 sometimes people like to make a utility that runs early or from busybox. I'd rather delete it in a few more weeks. #5 it already was that way, #6 will look into it another day, patches are welcome, #7 that was put there because it was required. There was a bz opened that this was the fix for so I can't get rid of it, #8 a) af_unix must be that way due to a mistake that must be overwritten. I'll change that another time. b) coreutils has to be there. c) I don't know a better way to do this patches welcome please test on x86_64, though. d) logrotate is the enemy of audit. Audit must do its own rotation for security purposes. e) those upgrade commands are for audit 1.0.x systems. f) where is this done in the spec file? I don't see any symlinking of consolehelper. Then again, consolehelper had better be in /usr/bin and not some relative directory.
audit-1.6.5-3 has the changes from this review in it. When you see if finish going through koji successfully, please feel free to look it over.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-01-15 23:17 EST -------
BTW, I use essentially the same spec file for upstream, RHEL, and Fedora. I don't like making changes for one that affects another since the audit system is under heavy development. If it were an older stable package, I wouldn't care so much.
Totally understood.
#1 fixed,
Thanks. Looks good.
#2 fixed but I like the shorter version better...why else have an url?
The Source url can be very different from the URL field. Tools like spectool -g and so forth look for the Source at a absolute URL. So, it's best to specify the entire thing.
#3 its a reminder to get it working at some point - added a comment,
ok, sounds good.
#4 sometimes people like to make a utility that runs early or from busybox. I'd rather delete it in a few more weeks.
ok. Possibly you could split them out into a -static subpackage?
#5 it already was that way,
Doese't seem to be. It's not a big deal, but doing the %postun libs -p /sbin/ldconfig means it just calls ldconfig, where if it's not using the -p it will spawn a shell and pass the contents (ldconfig) to it. Just a fork of a bash different I guess.
#6 will look into it another day, patches are welcome,
Ok. Will attach a patch here.
#7 that was put there because it was required. There was a bz opened that this was the fix for so I can't get rid of it,
Odd. Do you know the bug number? The guidelines forbid this now: http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=(prereq)#head-c...
#8 a) af_unix must be that way due to a mistake that must be overwritten. I'll change that another time.
ok. Might also make a note in the spec about it in case someone wonders.
b) coreutils has to be there.
coreutils is in the base buildroot, and will always be there. See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions
c) I don't know a better way to do this patches welcome please test on x86_64, though.
Yeah, I guess the python bits are arch independent, but the package is arch, so it complains. Nothing I can think of to do unless the python audit bits split out into their own noarch package. ;(
d) logrotate is the enemy of audit. Audit must do its own rotation for security purposes.
Hum. I guess that makes some sense.
e) those upgrade commands are for audit 1.0.x systems.
Yeah, and we should keep supporting the last 3 releases for upgrades. If audit1.0.x is newer than that, keep it.
f) where is this done in the spec file? I don't see any symlinking of consolehelper. Then again, consolehelper had better be in /usr/bin and not some relative directory.
It's not in the spec, it's part of the 'make install', ie upstream. /usr/libexec/system-config-audit-server -> /usr/bin/consolehelper
audit-1.6.5-3 has the changes from this review in it. When you see if finish going through koji successfully, please feel free to look it over.
Excellent. Thanks for the quick response here...
Will attach a patch for items 5, 6, 7...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-01-15 23:18 EST ------- Created an attachment (id=291797) --> (https://bugzilla.redhat.com/attachment.cgi?id=291797&action=view) patch for spec
Here's the patch. Let me know what you think.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-01-16 07:13 EST ------- Regarding item #7, bug 199587 is why coreutils was added. Coreutils is an install time problem. The problem hasn't resurfaced in the last 1.5 years since we put it there.
Regarding item #8a, I'll make the change but at a later date. I fixed this in F8 branch since they never saw the problem I was covering up, its a rawhide issue. Nobody should really be messing with that config file, so I don't think its much to worry about right now.
Regard item #8e, I use essentially the same spec file for Fedora, RHEL5, and upstream so that I can do a diff and make sure I'm not missing something important. I have to hand edit each file since the histories are different. When I fork development so that new stuff is not going into RHEL5, I'll make the change. For F7/8 its harmless since it errors on the first "if" and skips the whole thing.
Thanks for the patches. I'll look them over and reply separately.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-01-19 16:41 EST ------- The patch was applied except for the coreutils piece. This is in audit-1.6.6-1 which has been built. I fixed a couple more things besides this patch.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-01-22 13:29 EST ------- Can you change the Prereq: coreutils to Requires(pre): coreutils
That should work and meet the guidelines...
Thoughts?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-01-22 13:59 EST ------- Sure. I'll add it to 1.6.7 release which should be out in the next week or two.
I think that leaves just the hardcoded library path issue for audit.py.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-02-20 22:10 EST ------- yeah.
So, what is the issue with just replacing the hard coded '/usr/lib' with '%{_libdir}' ? The python .so's are already under libdir, so they are fine.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-02-21 11:38 EST ------- How about I do the %{python_sitelib} macro trick?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From kevin@tummy.com 2008-02-21 21:41 EST ------- That should work great... let me know if you want me to test here...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
------- Additional Comments From sgrubb@redhat.com 2008-02-25 06:30 EST ------- I checked in the changes to cvs - you should be able to see it. I did not build it as this is not quite important enough to warrant pushing out to rawhide. So, do we lack anything else wrt the audit package?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
------- Additional Comments From kevin@tummy.com 2008-02-25 14:23 EST ------- That looks pretty good... there is a new rpmlint warning: audit-libs-python.x86_64: E: non-standard-executable-perm /usr/lib64/python2.5/site-packages/auparse.so 0775
Might look at making sure thats 0755? In any case not a blocker...
Everything looks pretty good here, this package is APPROVED. Feel free to close rawhide.
Thanks again for your prompt fixes and answers!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: audit
https://bugzilla.redhat.com/show_bug.cgi?id=225292
sgrubb@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Fixed In Version| |1.6.8-2 Resolution| |CURRENTRELEASE
------- Additional Comments From sgrubb@redhat.com 2008-03-07 13:02 EST ------- Closing bug. Thanks for the helpful suggestions.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla