Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/clamsmtp.spec SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/clamsmtp-1.8-1.src.rpm Description: ClamSMTP is an SMTP filter that allows you to check for viruses using the ClamAV anti-virus software. It accepts SMTP connections and forwards the SMTP commands and responses to another SMTP server. The 'DATA' email body is intercepted and scanned before forwarding.
Not an official review since I am just contributor. First sight observations: - the package is named clamsmtp but almost everywhere in config and script files it behaves as being named clamd.smtp. However it looks that this comes from upstream. - BuildRoot has a sane value, but not the one recommended at http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1 - rpmlint on the src.rpm gives two warnings 1)W: clamsmtp strange-permission clamsmtpd.init 0744 A file that you listed to include in your package has strange permissions. Usually, a file should have 0644 permissions. Since the file is a startup script, this should be ignored 2)W: clamsmtp setup-not-quiet You should use -q to have a quiet extraction of the source tarball, as this generate useless lines of log ( for buildbot, for example setup -q should be used if possible - rpmlint on the binary gives a lot of errors about usage of non-standard UID/GID. Since the daemon runs under its own freshly created user, all these can be ignored. W: clamsmtp conffile-without-noreplace-flag /etc/clamd.d/smtp.conf This might be edited by the user, so I think that marking it as noreplace could be useful. E: clamsmtp non-standard-uid /var/lib/clamsmtp/tmp clamsmtp E: clamsmtp non-standard-gid /var/lib/clamsmtp/tmp clamsmtp E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp/tmp 0750 This directory should not exist. At least not for pid, socket etc which should not be placed here. W: clamsmtp dangling-relative-symlink /usr/sbin/clamd.smtp clamd Symlink to clamd, provided by the (required) clamav-server package E: clamsmtp non-standard-uid /etc/clamsmtpd.conf clamsmtp E: clamsmtp non-standard-gid /etc/clamsmtpd.conf clamsmtp E: clamsmtp non-readable /etc/clamsmtpd.conf 0640 E: clamsmtp non-standard-uid /var/lib/clamsmtp clamsmtp E: clamsmtp non-standard-gid /var/lib/clamsmtp clamsmtp E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp 0750 All these can be ignored due to daemon running as a new user which owns the above E: clamsmtp incoherent-logrotate-file /etc/logrotate.d/clamd.smtp logrotate file should be named clamsmtp E: clamsmtp init-script-name-with-dot /etc/rc.d/init.d/clamd.smtp recommended name is clamsmtp E: clamsmtp no-status-entry /etc/rc.d/init.d/clamd.smtp W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamd.smtp E: clamsmtp subsys-not-used /etc/rc.d/init.d/clamd.smtp The init.d/clamd.smtp script just starts clamd.wrapper from the clamav package. If possible, adding a couple of lines to verify the status would be nice, but I guess that it can be ignored. How ever,I would say that a stop entry should exist W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamsmtpd init.d/clamsmtpd claims it can do start, stop, restart, status, condrestart. However only start, stop and restart are implemented W: clamsmtp incoherent-subsys /etc/rc.d/init.d/clamsmtpd $prog the package is named clamsmtp so the script should be called clamsmtp too, not clamsmtpd - the %install stage makes use of the non-recommended %makeinstall macro (http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002) - configuration files are created as here documents at install time. I have not seen any comments about this in the wiki, but I think that using standard files included as %source would be saner and less error prone - a bunch of files are created in non-standard places (/var/lib/clamsmtp/clamd.smtp.log instead of /var/log/clamsmtp; /var/lib/clamsmtp/clamd.smtp.pid instead of /var/run/clamsmtp; I would also recommend something in the line of /var/run/clamav/clamd.smtp rather then /var/lib/clamsmtp/clamd.smtp.sock) Now, the review list: -OK: named according to packaging guidelines -OK: program licensed as GPL, original tar.gz includes the license, license filed in spec matches the actual license -OK: spec is written in AE but includes configurations files and startup scripts as here documents which make it hard to follow -OK: source matchs upstream (04da6aab94934641fcf9e7a7598346fb clamsmtp-1.8.tar.gz for both files) -OK: builds succesfully in mock/i386 -OK: no build dependency -OK: no locale -OK: no shared libraries, so no need for calling ldconfig -OK: not relocatable -OK: the package owns the files/directories it creates but does not respect FHS (see above my comments about this) -OK: does not include duplicates -OK: permissions are very sane -OK: includes a %clean section -OK: consistent usage of macros -OK: all content is permissable -OK: no large documents, so no need for separate -doc package -OK: %doc really includes just docs, runtime functionality not affected by them -OK: No header/static/.la/.pc files -OK: Not a GUI so no need for .desktop -OK: owns only its files -OK: scriptlets are sane; the daemons are added bit not started by default, conditionally restarted at upgrade, -SHOULD: works as advertised (at least does not segfault..) Bottom line: - I think all those here documents should be placed in independent SOURCE files - the startup scripts NEEDWORK (either implementing the missing advertised functions or removing those functions from the help line) - %makeinstall should be avoided - maybe something can be done about the inconsistent name usage ? clamsmtp (package name) is almost not used at all...
Thanks a lot for all these comments. Although you can't currently do any formal review or approval, this does help a lot the process ;-) New spec file : http://ftp.es6.freshrpms.net/tmp/extras/clamsmtp.spec New srpm : http://ftp.es6.freshrpms.net/tmp/extras/clamsmtp-1.8-2.src.rpm - I've externalized all of the clamd integration files. - I've added -q to %setup. - I've replaced %makeinstall with the DESTDIR method (and it works) > W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamsmtpd > init.d/clamsmtpd claims it can do start, stop, restart, status, condrestart. > However only start, stop and restart are implemented The script can't do "reload" indeed, but all advertised functions are there. About the inconsisten naming, I don't think it is. There are two quite different things in this package : The clamsmtpd daemon itself, which if from the upstream clamsmtp project (I would have named the project "clamsmtpd", myself...), and then all the "glue" I add in order to not have to configure anything manually. This "glue" is what is called "clamd.smtp" since this is how Enrico packaged ClamAV : You can run your own instance called "clamd.whatever", and here it's really useful since clamsmtp can run its own instance, thus be completely standalone and working out of the box.
First of all, I must appologize, you are right. The restart/status/condrestart were there, I have not noticed them. Unofficial review of the 2nd release: - spec file is indeed much cleaner, all previous here documents are available as independent entities - only remaining warning on the src.rpm is W: clamsmtp strange-permission clamsmtpd.init 0755 which can be ignored - rpmlint warnings on the binary: W: clamsmtp conffile-without-noreplace-flag /etc/clamd.d/smtp.conf E: clamsmtp non-standard-uid /var/lib/clamsmtp/tmp clamsmtp E: clamsmtp non-standard-gid /var/lib/clamsmtp/tmp clamsmtp E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp/tmp 0750 W: clamsmtp dangling-relative-symlink /usr/sbin/clamd.smtp clamd E: clamsmtp non-standard-uid /etc/clamsmtpd.conf clamsmtp E: clamsmtp non-standard-gid /etc/clamsmtpd.conf clamsmtp E: clamsmtp non-readable /etc/clamsmtpd.conf 0640 E: clamsmtp non-standard-uid /var/lib/clamsmtp clamsmtp E: clamsmtp non-standard-gid /var/lib/clamsmtp clamsmtp E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp 0750 E: clamsmtp incoherent-logrotate-file /etc/logrotate.d/clamd.smtp E: clamsmtp init-script-name-with-dot /etc/rc.d/init.d/clamd.smtp E: clamsmtp no-status-entry /etc/rc.d/init.d/clamd.smtp W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamd.smtp E: clamsmtp subsys-not-used /etc/rc.d/init.d/clamd.smtp W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamsmtpd W: clamsmtp incoherent-subsys /etc/rc.d/init.d/clamsmtpd $prog All of them have been commented before (comment #1): - UID/GID ones can be safely ignored; - those regarding permissions can be ignored, being due to running as a different (new) user; - the warning dangling relative link is false, being solved by the required clamav-server package; - per comment #2, the messages which mention inconsistent naming init-script-name-with-dot,no-status-entry, no-reload-entry for clamd.smtp should also be ignored; I am not sure about the subsys-not-used one, but I think it also can be ignored because clamd.smtp is just a wrapper - missing reload entries are not blockers - the logrotate file could be named clamsmtp thus avoiding the warning; same goes for /etc/rc.d/init.d/clamsmtpd. On the other hand since clamsmtpd is used everywhere but in the name of the project, I guess we should ignore these warnings. I still believe that log/pid/socket should not be placed in /var/lib/clamsmtp but under /var/log/ (or /var/log/clamsmtp) and /var/run, thus respecting FHS (http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOGLOGFILESANDDIRECTORIES for instance), as specified by http://fedoraproject.org/wiki/Packaging/Guidelines#head-e1c5548cbbe551c7a43d375c524ab2ea0188557e
Oh, I forgot to mention: it also builds without problems on x86_64
So you suggest : - Log file as /var/log/clamsmtpd.log (should be possible without being in a sub-dir if the daemon drops privileges after opening the log file) - Socket file as /var/run/clamsmtpd/clamd.smtp.socket - PID file as /var/run/clamsmtpd/clamsmtpd.pid - What about the temp dir? /var/spool/clamsmtpd/ ? And once we have all of these... which one would you suggest be the new home directory for the clamsmtp user?
Log file: yes. Or even /var/log/clamsmtpd Socket: yes. Pid file: yes. temp dir: /tmp or better yet /tmp/clamsmtp or even its very own home, wherever it is. There is no recommendation in the wiki AFAIK. As a matter of taste, I always have a separate partition for /tmp so of course I prefer temp files to get created there; it definitely simplifies management and keeps things consistent. Not to mention that /tmp is mounted noexec,nosuid homedir: dhcpd and rpm and the clamav package maintained by Enrico have there homes under /var/lib; The clamav and amavis versions I use (dag) have homes directly under /var. I guess /var/lib/clamsmtp (or clamsmtpd for consistence sake) would be a safe bet
I'm not sure if you are aware of this, but clamsmtp has already been submitted by me in bug 177401. Well, since I was too busy to finish that, maybe I'll review yours instead. What do you think, Matthias?
Oh, no, I had missed it too (like for postgrey), sorry. I've looked at the selinux issue, and it seems that my package will be suffering from the same inet port problem. I'd be more than happy to have you review my package.
Just FYI, I've moved the files to this location : http://ftp.es6.freshrpms.net/tmp/extras/clamsmtp/ The package probably still needs to include the suggestions from above, as well as someone inclined to make a formal review.
Show stopper: missing requires, usage of chkconfig and service impose Requires(post): /sbin/chkconfig Requires(preun): /sbin/chkconfig Requires(preun): /sbin/service
Has there been any progress on incorporating the review feedback? I'm sure someone will provide a complete review once there's a fixed package.
http://thias.fedorapeople.org/review/clamsmtp/ * Fri Aug 31 2007 Matthias Saou <http://freshrpms.net/> 1.9-1 - Update to 1.9. - Add missing scriplets requirements. - Fix License field, it's a 3 clause BSD. Hint : If someone had this ASSIGNED, I wouldn't forget to look at why it's still not CLOSED ;-)
Were you going to move the clamd socket and pidfile to /var/run, and the logfile to /var/log?
(In reply to comment #13) > Were you going to move the clamd socket and pidfile to /var/run, and the > logfile to /var/log? I've updated to 1.10 and made those two changes, but during some testing on a system with selinux enabled, I now get this : ERROR: LOCAL: Socket file /var/run/clamsmtp/clamd.smtp.sock could not be bound: Permission denied With these selinux log entry : type=1400 audit(0.000:35061): avc: denied { create } for pid=24756 comm="clamd.smtp" name="clamd.smtp.sock" scontext=unconfined_u:system_r:clamd_t:s0 tcontext=unconfined_u:object_r:var_run_t:s0 tclass=sock_file For this to work, should I try to include something in the package or ask to get the default policy updated?
I've found the selinux folks easy enough to work with; it's worth at least asking them for help with a policy.
Did anything ever happen regarding selinux? Or did you want to have this reviewed with the known caveat of selinux issues? (That would be OK, although it should be documented that problems exist and the selinux folks should be contacted to try and work out a solution.)
No, I haven't done anything regarding that issue yet. If anyone has the time and will to get that sorted out, by asking for changes to the system policy to be made I assume, then that would be great and I'd be very grateful!
I'm going to mark this as not being ready for review; if you would like for it to return to the review queue, please clear the whiteboard. Unfortunately selinux isn't really my forte and indeed I'm struggling with the pain it causes in some of my other packages. I don't really know enough about this package to act as an intermediary between you and the selinux folks, but perhaps someone else does.
Once closing as a duplicate of the new submission (not by me) *** This bug has been marked as a duplicate of bug 555059 ***