Bug 225292
Summary: | Merge Review: audit | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nobody's working on this, feel free to take it <nobody> | ||||
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | 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
Nobody's working on this, feel free to take it
2007-01-29 21:07:39 UTC
I'll go ahead and review this... look for a full review in a bit. 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? 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. >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... Created attachment 291797 [details]
patch for spec
Here's the patch. Let me know what you think.
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. 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. Can you change the Prereq: coreutils to Requires(pre): coreutils That should work and meet the guidelines... Thoughts? 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. 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. How about I do the %{python_sitelib} macro trick? That should work great... let me know if you want me to test here... 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? 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! Closing bug. Thanks for the helpful suggestions. |