Bug 225292

Summary: Merge Review: audit
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: redhat-bugzilla, sgrubb
Target Milestone: ---Flags: kevin: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.6.8-2 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-03-07 18:02:43 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: 426387    
Attachments:
Description Flags
patch for spec none

Description Nobody's working on this, feel free to take it 2007-01-29 21:07:39 UTC
Fedora Merge Review: audit

http://cvs.fedora.redhat.com/viewcvs/devel/audit/

Comment 1 Kevin Fenzi 2008-01-11 16:07:19 UTC
I'll go ahead and review this... look for a full review in a bit. 

Comment 2 Kevin Fenzi 2008-01-11 17:47:18 UTC
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-d559ee7363418a5840ce63090c608c991cd39ce6

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?

Comment 3 Steve Grubb 2008-01-11 20:46:18 UTC
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.

Comment 4 Kevin Fenzi 2008-01-16 04:17:57 UTC
>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-c81b037a3a0d08f98eb9cb50594f5de73d1e461d

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

Comment 5 Kevin Fenzi 2008-01-16 04:18:52 UTC
Created attachment 291797 [details]
patch for spec 

Here's the patch. Let me know what you think.

Comment 6 Steve Grubb 2008-01-16 12:13:24 UTC
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.

Comment 7 Steve Grubb 2008-01-19 21:41:11 UTC
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.

Comment 8 Kevin Fenzi 2008-01-22 18:29:58 UTC
Can you change the 
Prereq: coreutils
to
Requires(pre): coreutils

That should work and meet the guidelines... 

Thoughts?



Comment 9 Steve Grubb 2008-01-22 18:59:50 UTC
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.

Comment 10 Kevin Fenzi 2008-02-21 03:10:01 UTC
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. 


Comment 11 Steve Grubb 2008-02-21 16:38:15 UTC
How about I do the %{python_sitelib} macro trick?

Comment 12 Kevin Fenzi 2008-02-22 02:41:21 UTC
That should work great... let me know if you want me to test here... 

Comment 13 Steve Grubb 2008-02-25 11:30:34 UTC
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?

Comment 14 Kevin Fenzi 2008-02-25 19:23:37 UTC
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!

Comment 15 Steve Grubb 2008-03-07 18:02:43 UTC
Closing bug. Thanks for the helpful suggestions.