Spec URL: http://dl.atrpms.net/all/fail2ban.spec SRPM URL: http://dl.atrpms.net/all/fail2ban-0.6.2-1.at.src.rpm Description: Fail2ban scans log files like /var/log/pwdfail or /var/log/apache/error_log and bans IP that makes too many password failures. It updates firewall rules to reject the IP address.
Expected rpmlint output: E: fail2ban only-non-binary-in-usr-lib W: fail2ban service-default-enabled /etc/rc.d/init.d/fail2ban o service-default-enabled: The service may be enabled, but in absence of /etc/fail2ban.conf (which is the default) it will not start. o only-non-binary-in-usr-lib: Unfortunately it installs its python bits under /usr/lib. I'm not sure why upstream has chosen so, it looks quite similar to how yum deals with its python files, too.
I will review this, however, now I am preparing for going back to my parents' home so it may be tomorrow.
*** Bug 210424 has been marked as a duplicate of this bug. ***
Well, A. First for general packaging issue of this package: E: fail2ban only-non-binary-in-usr-lib ! Well, for this package moving all files in /usr/lib to %{_datadir} seems very easy and I recommend it (currently not a blocker, however would you contact with upstream?) * And... for this package the directory is /usr/lib, not %{_libdir}!! You can check this by setup.py (hard-coded) W: fail2ban service-default-enabled /etc/rc.d/init.d/fail2ban * Umm.. I think this should be avoided. This warning is due to the line --------------------------------------------------------------- # chkconfig: 345 91 9 --------------------------------------------------------------- of /etc/rc.d/init.d/fail2ban . The description "345" means that fail2ban service is automatically enabled when installed on the level of 3-5 (man 8 chkconfig) And... > The service may be enabled, but in absence of > /etc/fail2ban.conf (which is the default) it will not start. * I think only the default behaviour of this script is unkind because fail2ban won't start but no error message is printed out. Current message is: ------------------------------------------------------ Starting fail2ban: ------------------------------------------------------ Some messages like ------------------------------------------------------ Starting fail2ban: configulation file not found [ FAILED ] ------------------------------------------------------ should be printed out. Also, the exit status of the failure should not be 0. Even I copyed /usr/share/doc/fail2ban/fail2ban.conf.iptables to /etc/fail2ban.conf, no message is printed out. Some messages which tells that starting daemon succeeded should be printed out. Well, then... B. From http://fedoraproject.org/wiki/Packaging/Guidelines : ! Licensing - Well, this package is licensed under GPL, however, GPL document is not included in source tarball. Currently this is not a blocker, however, please ask the upstream to include GPL document to source tarball. ! Filesystem Layout - Described above (not a blocker) - My opinion is fail2ban should be under %{_sbindir}. - Usually config files of initscripts should be under %{_sysconfdir}/sysconfig * Scriptlets requirements ( http://fedoraproject.org/wiki/Packaging/ScriptletSnippets ) - For /sbin/chkconfig and etc Please write Requires(post): /sbin/chkconfig and others - condrestart scriptlet on %postun stage is needed * File and Directory Ownership - My opinion is that this package should own /var/log/fail2ban as a ghost file. C. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines : = Okay, except for written in A and B.
Thanks for the first pass. I agree on the /usr/lib stuff. I'll fix it and contact upstream. I also agree with most other points and am disappointed for forgetting /sbin/chkconfig :/ I'll attack all points and come back with "-2"
Spec URL: http://dl.atrpms.net/all/fail2ban.spec SRPM URL: http://dl.atrpms.net/all/fail2ban-0.6.2-2.at.src.rpm * Fri Dec 29 2006 Axel Thimm <Axel.Thimm> - 0.6.2-2 - Move /usr/lib/fail2ban to %%{_datadir}/fail2ban. - Don't default chkconfig to enabled. - Add dependencies on service/chkconfig. - Use example iptables/ssh config as default config.
Well: * Would you explain why you think that condrestart treatment of the service on %postun stage is unneeded? * I still does not like the appearance of the start/exit of fai2ban service. Currently: -------------------------------------------- [root@localhost ~]# service fail2ban start Starting fail2ban: [root@localhost ~]# service fail2ban stop Stopping fail2ban: -------------------------------------------- This should be like: -------------------------------------------- [root@localhost ~]# service sshd start Starting sshd: [ OK ] [root@localhost ~]# service sshd stop Stopping sshd: [ OK ] -------------------------------------------- * And.. -------------------------------------------- [ "${NETWORKING}" = "no" ] && exit 0 [ -f /etc/fail2ban.conf ] || exit 0 --------------------------------------------- should be "exit 1" or something else: exit code 0 is wrong IMO. Also some messages which tells why starting fail2ban failed should be printed out. * Still I think (strongly) that /usr/bin/fail2ban should be moved under /usr/sbin because this is a sysadmin tool ... and /etc/fail2ban.conf should be /etc/sysconfig/fail2ban . * And I think this package should own /var/log/fail2ban
> * Would you explain why you think that condrestart treatment of the > service on %postun stage is unneeded? Yes, I consider fail2ban in this respect to be as fragile as for example the iptables or httpd services: I don't want to automate therestart, the sysadmin should do that manually and watch for side effects. > [ "${NETWORKING}" = "no" ] && exit 0 This is the typical snipplet used throught all FC packages: $ grep -l '\[ "${NETWORKING}" = "no" \] && exit 0' /etc/init.d/* | tr '\n' ' ' /etc/init.d/bgpd /etc/init.d/btseed /etc/init.d/bttrack /etc/init.d/dhcdbd /etc/init.d/fail2ban /etc/init.d/gkrellmd /etc/init.d/innd /etc/init.d/netfs /etc/init.d/network /etc/init.d/nfs /etc/init.d/nfslock /etc/init.d/ospfd /etc/init.d/postgresql /etc/init.d/ripd /etc/init.d/rpcgssd /etc/init.d/rpcidmapd /etc/init.d/rpcsvcgssd /etc/init.d/sendmail /etc/init.d/zebra > [ -f /etc/fail2ban.conf ] || exit 0 Same here $ grep -l '\[ -f .* \] || exit 0' /etc/init.d/* | tr '\n' ' ' /etc/init.d/acpid /etc/init.d/anacron /etc/init.d/bgpd /etc/init.d/bootparamd /etc/init.d/capi /etc/init.d/clamav /etc/init.d/cpuspeed /etc/init.d/dhcp6r /etc/init.d/dhcp6s /etc/init.d/dhcpd /etc/init.d/dhcrelay /etc/init.d/dund /etc/init.d/exim /etc/init.d/fail2ban /etc/init.d/gkrellmd /etc/init.d/hidd /etc/init.d/hsqldb /etc/init.d/innd /etc/init.d/irda /etc/init.d/irqbalance /etc/init.d/mdmonitor /etc/init.d/mdmpd /etc/init.d/netdump /etc/init.d/netfs /etc/init.d/nscd /etc/init.d/ospf6d /etc/init.d/ospfd /etc/init.d/pand /etc/init.d/portmap /etc/init.d/radiusd /etc/init.d/radvd /etc/init.d/restorecond /etc/init.d/rgmanager /etc/init.d/rhnsd /etc/init.d/ripd /etc/init.d/ripngd /etc/init.d/sendmail /etc/init.d/spamassassin /etc/init.d/squid /etc/init.d/syslog /etc/init.d/winbind /etc/init.d/yppasswdd /etc/init.d/ypserv /etc/init.d/ypxfrd /etc/init.d/zaptel /etc/init.d/zebra > --------------------------------------------- > should be "exit 1" or something else: exit code 0 is > wrong IMO. Also some messages which tells why starting > fail2ban failed should be printed out. Well, it is obviously a Fedora convention not to do so. Whether it is right or wrong is a different thing, but fail2ban has to blend in properly so the above are correct. Anything else would have to be discussed with the FPC. > * Still I think (strongly) that /usr/bin/fail2ban should > be moved under > /usr/sbin because this is a sysadmin tool You can use fail2ban as a user, too. > ... and /etc/fail2ban.conf should be /etc/sysconfig/fail2ban . No, that's wrong, /etc/sysconfig carries config files for the init files themselves (e.g. what arguments to use for calling a daemon), everything else is defined by the application, e.g. check httpd, ntpd and so on. > * And I think this package should own /var/log/fail2ban Again no other packages caters for its logfile ownership, having fail2ban behave differently is wrong. But I 100% with you on defining a general solution, just not through a package submission. You're welcome to raise the issues at fedora-packaging instead.
Well: A. From http://fedoraproject.org/wiki/Packaging/Guidelines ! Licensing - Please ask upstream to include to src tarball a copy of GPL (not a blocker) = Scriptlets requirements (In reply to comment #8) > > * Would you explain why you think that condrestart treatment of the > > service on %postun stage is unneeded? > > Yes, I consider fail2ban in this respect to be as > fragile as for example the > iptables or httpd services: I don't want to automate therestart However, I found on your spec file (0.6.2-2) ------------------------------------------ %post /sbin/chkconfig --add %{name} /sbin/service %{name} condrestart > /dev/null 2>&1 <- THIS LINE ------------------------------------------ Perhaps you may want to remove the line (I don't object to it according to your opinition). Note: leaving the line needs "Requires(post): /sbin/service" B. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines = okay, except for the issues on A. ------------------------------------------------------------------ This package (fail2ban) is APPROVED by me. ------------------------------------------------------------------- NOTE: the issue under discussion on fedora-maintainers about "very short APPROVED comments" was actually caused by my review.......
Upstream has been notified on both /usr/lib and license issue > /sbin/service %{name} condrestart > /dev/null 2>&1 <- THIS LINE Yes, that needs to go away. I added it, then regretted it and forgot to remove it. :/ > This package (fail2ban) is APPROVED by me. Thanks, I'll remove the line and import 0.6.2-3
Package Change Request ====================== Package Name: fail2ban New Branches: EL-4 EL-5 Owners: maxamillion Fedora owner has been contacted and expressed no interest in EPEL, I would like to maintain the EPEL package for EL-4 and EL-5 Email citing upstream packagers response to supporting EPEL: > According to > > https://admin.fedoraproject.org/pkgdb/packages/name/fail2ban#Fedora%20EPEL5 > , fail2ban is available. A yum search fail2ban doesn't show it, however, > and http://download.fedora.redhat.com/pub/epel/5/i386/repoview/F.group.html > doesn't show it. Am I missing something, or is there still a step before it > goes fully available? I don't even see it in the testing branch. This was a bad entry, I am not supporting EPEL. You can get a fail2ban working on RHEL5/CentOS5 at ATrpms or DAG. -- Axel.Thimm at ATrpms.net
cvs done.