Bug 218022 - Review Request: clamsmtp - SMTP filter daemon for anti-virus checking using ClamAV
Summary: Review Request: clamsmtp - SMTP filter daemon for anti-virus checking using C...
Keywords:
Status: CLOSED DUPLICATE of bug 555059
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-12-01 11:07 UTC by Matthias Saou
Modified: 2010-01-26 00:03 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-13 17:48:03 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Matthias Saou 2006-12-01 11:07:07 UTC
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.

Comment 1 manuel wolfshant 2006-12-01 16:11:58 UTC
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...

Comment 2 Matthias Saou 2006-12-01 16:38:02 UTC
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.

Comment 3 manuel wolfshant 2006-12-01 17:18:40 UTC
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

Comment 4 manuel wolfshant 2006-12-01 17:20:30 UTC
Oh, I forgot to mention: it also builds without problems on x86_64

Comment 5 Matthias Saou 2006-12-01 17:34:04 UTC
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?

Comment 6 manuel wolfshant 2006-12-01 18:27:21 UTC
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


Comment 7 Dominik 'Rathann' Mierzejewski 2006-12-01 20:05:34 UTC
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?

Comment 8 Matthias Saou 2006-12-04 11:42:15 UTC
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.

Comment 9 Matthias Saou 2007-03-01 14:34:46 UTC
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.

Comment 10 manuel wolfshant 2007-03-01 15:37:05 UTC
Show stopper: missing requires, usage of chkconfig and service impose
Requires(post): /sbin/chkconfig
Requires(preun): /sbin/chkconfig
Requires(preun): /sbin/service


Comment 11 Jason Tibbitts 2007-07-04 15:34:17 UTC
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.

Comment 12 Matthias Saou 2007-08-31 15:04:28 UTC
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 ;-)

Comment 13 David Woodhouse 2008-10-22 18:16:08 UTC
Were you going to move the clamd socket and pidfile to /var/run, and the logfile to /var/log?

Comment 14 Matthias Saou 2008-12-22 20:44:40 UTC
(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?

Comment 15 Jason Tibbitts 2008-12-22 20:52:41 UTC
I've found the selinux folks easy enough to work with; it's worth at least asking them for help with a policy.

Comment 16 Jason Tibbitts 2009-07-10 20:26:56 UTC
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.)

Comment 17 Matthias Saou 2009-07-13 09:40:53 UTC
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!

Comment 18 Jason Tibbitts 2009-07-14 21:50:30 UTC
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.

Comment 19 Mamoru TASAKA 2010-01-13 17:48:03 UTC
Once closing as a duplicate of the new submission (not by me)

*** This bug has been marked as a duplicate of bug 555059 ***


Note You need to log in before you can comment on or make changes to this bug.