Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 220789
Summary: | Review Request: fail2ban - Ban IPs that make too many password failures | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Axel Thimm <axel.thimm> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | maxamillion, wcervini |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-12-31 18:54:16 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: | 163779 |
Description
Axel Thimm
2006-12-27 00:04:36 UTC
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. |