Milter-greylist

Here s the promised SpamAssassin patch. # greylist.conf example1 spamdsock inet 127.0.0.1:783 racl whitelist default dacl blacklist spamd  15 dacl greylist

On Fri, Sep 26, 2008 at 02:07:09PM +0200, Manuel Badzong wrote:
> As soon as the changes are applied, I'll document the changes in the man
> page.

Well, it's preferable to keep code and doc in sync when doing commits...

About the code, it looks great, but just one question: why storing the 
socket type as a string? You could use an int for that, whith values
PF_LOCAL/PF_INET ...

-- 
Emmanuel Dreyfus
manu@...

Sorry, the above patch does not work with the latest sources (I used old code). This one works fine with the latest snapshot from cvs. I also added

On Fri, Sep 26, 2008 at 04:21:54PM +0200, Manuel Badzong wrote:
> Sorry, the above patch does not work with the latest sources (I used
> old code). This one works fine with the latest snapshot from cvs.
> 
> I also added documentation to greylist.conf(5).

If you see no problem, I'll rename sa_* stuff into spamd_* (and sa.c into 
spamd.c), I feel it is more self-describing.

You did not include a license for the two new files... 

-- 
Emmanuel Dreyfus
manu@...

On Fri, Sep 26, 2008 at 02:50:07PM +0000, Emmanuel Dreyfus wrote:
> About the code, it looks great, but just one question: why storing the
> socket type as a string? You could use an int for that, whith values
> PF_LOCAL/PF_INET ...

No real reason. It doesn't add complexity and the code stays smaller?


> If you see no problem, I'll rename sa_* stuff into spamd_* (and sa.c into 
> spamd.c), I feel it is more self-describing.

Of course. As I started working on the patch I prefix everything sa_
(SpamAssassin) until SA clashed with a macro in milter-greylist.h. spamd
is definitely the better choice.


> You did not include a license for the two new files... 

Could you paste a BSD License in?


Thanks, Manuel

On Fri, Sep 26, 2008 at 05:04:53PM +0200, Manuel Badzong wrote:
> No real reason. It doesn't add complexity and the code stays smaller?

OTOH, you save a few calls to strcmp().

> Could you paste a BSD License in?

Done. I heavily edited it (renames, style, indents), so please thest that 
the in-tree version still works. At least it builds.

-- 
Emmanuel Dreyfus
manu@...

... Thanks. There was a small typo in spamd_inet_socket. I also removed unnecassary code. Patch attached. Cheers, Manuel diff -ruN milter-greylist/spamd.c

The following patch fixes a typo in spamd.c and fixes a missing include in acl.h (didn t build on netbsd with gcc 4.1.2). Manuel diff -ruN

Manuel Badzong <manuel@...> wrote:

> The following patch fixes a typo in spamd.c and fixes a missing include
> in acl.h (didn't build on netbsd with gcc 4.1.2).

Got it.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Here s another patch for spamd.c. Fixes a call to freeaddrinfo() with bad pointer. Manuel

Manuel Badzong <manuel@...> wrote:

> Here's another patch for spamd.c. Fixes a call to freeaddrinfo() with
> bad pointer.

Got it.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Hi folks,

great work, but just want to add some quick comments which may
lead to changes ...

On Fri, Sep 26, 2008 at 02:07:09PM +0200, Manuel Badzong wrote:
[..]
> diff -ruN milter-greylist/Makefile.in mg_manu/Makefile.in
> --- milter-greylist/Makefile.in	2008-09-07 04:12:15.000000000 +0200
> +++ mg_manu/Makefile.in	2008-09-26 12:26:18.000000000 +0200
> @@ -56,10 +56,10 @@
>  OBJ= 		milter-greylist.o pending.o sync.o dnsrbl.o list.o macro.o \
>  		conf_yacc.o dump_yacc.o conf.o autowhite.o dump.o spf.o \
>  		acl.o urlcheck.o stat.o clock.o geoip.o fd_pool.o prop.o \
> -		ldapcheck.o dkimcheck.o p0f.o
> +		ldapcheck.o dkimcheck.o p0f.o sa.o
>  SRC= 		milter-greylist.c pending.c sync.c conf.c macro.c stat.c \
>  		clock.c autowhite.c dump.c spf.c acl.c dnsrbl.c list.c \
> -		urlcheck.c geoip.c prop.c ldapcheck.c dkimcheck.c p0f.c
> +		urlcheck.c geoip.c prop.c ldapcheck.c dkimcheck.c p0f.c sa.c
>  GENSRC=		conf_yacc.c conf_lex.c dump_yacc.c dump_lex.c  
>  
>  VPATH=		${SRCDIR}
> diff -ruN milter-greylist/acl.c mg_manu/acl.c
> --- milter-greylist/acl.c	2008-09-23 20:55:11.000000000 +0200
> +++ mg_manu/acl.c	2008-09-26 11:25:58.000000000 +0200
> @@ -90,6 +90,9 @@
>  #ifdef USE_DKIM
>  #include "dkimcheck.h"
>  #endif
> +#ifdef USE_SA
> +#include "sa.h"
> +#endif
>  #include "macro.h"
>  #include "clock.h"
>  #include "milter-greylist.h"
> @@ -367,6 +370,14 @@
>  	  acl_print_list, acl_add_list, 
>  	  NULL, acl_list_filter },
>  #endif
> +#ifdef USE_SA
> +	{ AC_SA, MULTIPLE_OK, AS_DATA, "spamd",
> +	  AT_NONE, AC_NONE, AC_NONE,  EXF_SA,
> +	  acl_print_null, NULL, NULL, sa_spam },
> +	{ AC_SASCORE, MULTIPLE_OK, AS_DATA, "spamd score",
> +	  AT_OPNUM, AC_NONE, AC_NONE,  EXF_SA,
> +	  acl_print_opnum, acl_add_opnum, NULL, sa_score },
> +#endif
>  };
>  
>  struct {
> diff -ruN milter-greylist/acl.h mg_manu/acl.h
> --- milter-greylist/acl.h	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/acl.h	2008-09-26 11:06:05.000000000 +0200
> @@ -52,7 +52,7 @@
>  typedef enum { AS_NONE, AS_RCPT, AS_DATA, AS_ANY, } acl_stage_t;
>  typedef enum { AT_NONE, AT_STRING, AT_REGEX, AT_NETBLOCK, AT_OPNUM, 
>  	       AT_CLOCKSPEC, AT_DNSRBL, AT_URLCHECK, AT_MACRO, 
> -	       AT_LIST, AT_PROP, AT_SPF, AT_DKIM, 
> +	       AT_LIST, AT_PROP, AT_SPF, AT_DKIM,
>  	       AT_LDAPCHECK } acl_data_type_t;
>  
>  typedef enum {
> @@ -108,6 +108,8 @@
>  	AC_P0F,
>  	AC_P0F_RE,
>  	AC_P0F_LIST,
> +	AC_SA,
> +	AC_SASCORE,
>  } acl_clause_t;
>  
>  struct acl_clause;
> @@ -304,6 +306,7 @@
>  		      struct acl_param *, struct mlfi_priv *);
>  int acl_msgsize_cmp(acl_data_t *, acl_stage_t, 
>  		    struct acl_param *, struct mlfi_priv *);
> +int acl_opnum_cmp(int, enum operator, int);
>  int myregexec(struct mlfi_priv *, acl_data_t *, 
>  	      struct acl_param *, const char *);
>  
> @@ -340,4 +343,5 @@
>  #define EXF_LDAPCHECK	(1 << 27)
>  #define	EXF_DKIM	(1 << 28)
>  #define EXF_P0F		(1 << 29)
> +#define EXF_SA		(1 << 30)
>  #endif /* _ACL_H_ */
> diff -ruN milter-greylist/conf.c mg_manu/conf.c
> --- milter-greylist/conf.c	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/conf.c	2008-09-25 22:37:49.000000000 +0200
> @@ -78,6 +78,9 @@
>  #ifdef USE_P0F
>  #include "p0f.h"
>  #endif
> +#ifdef USE_SA
> +#include "sa.h"
> +#endif
>  #include "autowhite.h"
>  #include "conf.h"
>  #include "sync.h"
> @@ -467,5 +470,9 @@
>  #ifdef USE_P0F
>  	c->c_p0fsock[0] = '\0';
>  #endif
> +#ifdef USE_SA
> +	c->c_sasock[0] = '\0';
> +	c->c_sasocktype[0] = '\0';
> +#endif
>  	return;
>  }
> diff -ruN milter-greylist/conf.h mg_manu/conf.h
> --- milter-greylist/conf.h	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/conf.h	2008-09-25 20:57:08.000000000 +0200
> @@ -112,6 +112,10 @@
>  #ifdef USE_P0F
>  	char c_p0fsock[QSTRLEN + 1];
>  #endif
> +#ifdef USE_SA
> +	char c_sasock[QSTRLEN + 1];
> +	char c_sasocktype[QSTRLEN + 1];
> +#endif
>  };
>  
>  /* c_forced flags */
> diff -ruN milter-greylist/conf_lex.l mg_manu/conf_lex.l
> --- milter-greylist/conf_lex.l	2008-09-07 18:50:16.000000000 +0200
> +++ mg_manu/conf_lex.l	2008-09-26 11:23:49.000000000 +0200
> @@ -100,6 +100,9 @@
>  ldapcheck	[Ll][Dd][Aa][Pp][Cc][Hh][Ee][Cc][Kk]
>  p0fsock		[Pp]0[Ff][Ss][Oo][Cc][Kk]
>  p0f		[Pp]0[Ff]
> +spamdsock	[Ss][Pp][Aa][Mm][Dd][Ss][Oo][Cc][Kk]
> +spamdsockt	[Ii][Nn][Ee][Tt]|[Uu][Nn][Ii][Xx]
> +spamd		[Ss][Pp][Aa][Mm][Dd]
>  openlist	"{"
>  closelist	"}"
>  nextln		"\\".*"\n"
> @@ -230,6 +233,13 @@
>  {ldapcheck}	{ return LDAPCHECK; }
>  {p0fsock}	{ return P0FSOCK; }
>  {p0f}		{ BEGIN(S_REGEX); return P0F; }
> +{spamdsock}	{ return SPAMDSOCK; }
> +{spamdsockt}	{ 
> +			strncpy(yylval.spamdsockt, yytext, QSTRLEN);
> +			yylval.spamdsockt[QSTRLEN] = '\0';
> +			return SPAMDSOCKT;
> +		}
> +{spamd}		{ return SPAMD; }
>  {pidfile}	{ return PIDFILE; }
>  {dumpfile}	{ return GLDUMPFILE; }
>  {subnetmatch}	{ return SUBNETMATCH; }
> diff -ruN milter-greylist/conf_yacc.y mg_manu/conf_yacc.y
> --- milter-greylist/conf_yacc.y	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/conf_yacc.y	2008-09-26 11:23:07.000000000 +0200
> @@ -1,4 +1,4 @@
> -%token TNUMBER ADDR IPADDR IP6ADDR CIDR HELO FROM RCPT EMAIL PEER AUTOWHITE GREYLIST NOAUTH NOACCESSDB EXTENDEDREGEX NOSPF QUIET TESTMODE VERBOSE PIDFILE GLDUMPFILE QSTRING TDELAY SUBNETMATCH SUBNETMATCH6 SOCKET USER NODETACH REGEX REPORT NONE DELAYS NODELAYS ALL LAZYAW GLDUMPFREQ GLTIMEOUT DOMAIN DOMAINNAME SYNCADDR SYNCSRCADDR PORT ACL WHITELIST DEFAULT STAR DELAYEDREJECT DB NODRAC DRAC DUMP_NO_TIME_TRANSLATION LOGEXPIRED GLXDELAY DNSRBL LIST OPENLIST CLOSELIST BLACKLIST FLUSHADDR CODE ECODE MSG SM_MACRO UNSET URLCHECK RACL DACL GLHEADER BODY MAXPEEK STAT POSTMSG FORK GETPROP CLEAR PROP AUTH TLS SPF DKIMCHECK MSGSIZE RCPTCOUNT OP NO SLASH MINUS COMMA TIME GEOIPDB GEOIP PASS FAIL SOFTFAIL NEUTRAL UNKNWON ERROR SELF SPF_STATUS LDAPCONF LDAPCHECK P0FSOCK P0F
> +%token TNUMBER ADDR IPADDR IP6ADDR CIDR HELO FROM RCPT EMAIL PEER AUTOWHITE GREYLIST NOAUTH NOACCESSDB EXTENDEDREGEX NOSPF QUIET TESTMODE VERBOSE PIDFILE GLDUMPFILE QSTRING TDELAY SUBNETMATCH SUBNETMATCH6 SOCKET USER NODETACH REGEX REPORT NONE DELAYS NODELAYS ALL LAZYAW GLDUMPFREQ GLTIMEOUT DOMAIN DOMAINNAME SYNCADDR SYNCSRCADDR PORT ACL WHITELIST DEFAULT STAR DELAYEDREJECT DB NODRAC DRAC DUMP_NO_TIME_TRANSLATION LOGEXPIRED GLXDELAY DNSRBL LIST OPENLIST CLOSELIST BLACKLIST FLUSHADDR CODE ECODE MSG SM_MACRO UNSET URLCHECK RACL DACL GLHEADER BODY MAXPEEK STAT POSTMSG FORK GETPROP CLEAR PROP AUTH TLS SPF DKIMCHECK MSGSIZE RCPTCOUNT OP NO SLASH MINUS COMMA TIME GEOIPDB GEOIP PASS FAIL SOFTFAIL NEUTRAL UNKNWON ERROR SELF SPF_STATUS LDAPCONF LDAPCHECK P0FSOCK P0F SPAMDSOCK SPAMDSOCKT SPAMD
>  
>  %{
>  #include "config.h"
> @@ -42,6 +42,9 @@
>  #ifdef USE_P0F
>  #include "p0f.h"
>  #endif
> +#ifdef USE_SA
> +#include "sa.h"
> +#endif
>  #include "stat.h"
>  #include "clock.h"
>  #include "spf.h"
> @@ -75,6 +78,7 @@
>  	char prop[QSTRLEN + 1];
>  	enum spf_status spf_status;
>  	enum spf_status dkim_status;
> +	char spamdsockt[QSTRLEN + 1];
>  	}
>  %type <ipaddr> IPADDR;
>  %type <ip6addr> IP6ADDR;
> @@ -88,6 +92,7 @@
>  %type <op> OP;
>  %type <prop> PROP;
>  %type <spf_status> SPF_STATUS;
> +%type <spamdsockt> SPAMDSOCKT;
>  
>  %%
>  lines	:	lines netblock '\n' 
> @@ -137,6 +142,7 @@
>  	|	lines ldapcheckdef '\n'
>  	|	lines ldapconfdef '\n'
>  	|	lines p0fsockdef '\n'
> +	|	lines spamdsockdef '\n'
>  	|	lines listdef '\n'
>  	|	lines '\n'
>  	|
> @@ -392,6 +398,19 @@
>  #endif
>  				}
>  	;
> +spamdsockdef:	SPAMDSOCK SPAMDSOCKT QSTRING	{
> +#ifdef USE_SA
> +				char path[QSTRLEN + 1];
> +
> +				sa_sock_set($2, quotepath(path, $3, QSTRLEN));
> +#else
> +				mg_log(LOG_INFO, 
> +				    "spamassassin support not compiled in, "
> +				    "ignore line %d", 
> +				    conf_line);
> +#endif
> +				}
> +	;
>  geoipdb:	GEOIPDB QSTRING	{
>  #ifdef USE_GEOIP
>  				char path[QSTRLEN + 1];
> @@ -646,6 +665,8 @@
>  	|	geoip_clause
>  	|	prop_clause
>  	|	propregex_clause
> +	|	spamd_clause
> +	|	spamd_score_clause
>  	;
>  
>  acl_values:	acl_value
> @@ -725,6 +746,31 @@
>  #endif
>  			}
>  	;
> +spamd_clause:		SPAMD {
> +#ifdef USE_SA
> +				acl_add_clause(AC_SA, NULL);
> +#else
> +				mg_log(LOG_INFO, 
> +				 "spamassassin support not compiled in, ignore line %d", 
> +				 conf_line);
> +#endif
> +			}
> +	;
> +spamd_score_clause:	SPAMD OP TNUMBER {
> +#ifdef USE_SA
> +				struct acl_opnum_data aond;
> +
> +				aond.op = $2;
> +				aond.num = atoi($3);
> +				
> +				acl_add_clause(AC_SASCORE, &aond);
> +#else
> +				mg_log(LOG_INFO, 
> +				 "spamassassin support not compiled in, ignore line %d", 
> +				 conf_line);
> +#endif
> +			}
> +	;
>  geoip_clause:		GEOIP QSTRING {
>  #ifdef USE_GEOIP
>  				char ccode[IPADDRSTRLEN + 1];
> diff -ruN milter-greylist/configure.ac mg_manu/configure.ac
> --- milter-greylist/configure.ac	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/configure.ac	2008-09-26 12:47:06.000000000 +0200
> @@ -427,6 +427,11 @@
>  	[  --enable-p0f		Enable p0f support],
>  	[if test x$enableval = xyes; then CFLAGS=$CFLAGS" -DUSE_P0F"; fi])
>  
> +# Check for SpamAssassin
> +AC_ARG_ENABLE(spamassassin, 
> +	[  --enable-spamassassin	Enable SpamAssassin support],
> +	[if test x$enableval = xyes; then CFLAGS=$CFLAGS" -DUSE_SA"; fi])
> +
>  # Check for libmilter. For sendmail-8.12.1, -lsm is required too.
>  # This uses a gross hack on the second AC_CHECK_LIB first argument, but using 
>  # [-lsm] in the optionnal 5th argument does not seems to help at all.
> diff -ruN milter-greylist/milter-greylist.c mg_manu/milter-greylist.c
> --- milter-greylist/milter-greylist.c	2008-09-23 20:55:11.000000000 +0200
> +++ mg_manu/milter-greylist.c	2008-09-25 20:21:11.000000000 +0200
> @@ -353,6 +353,9 @@
>  	priv->priv_p0f = NULL;
>  	p0f_lookup(priv);
>  #endif
> +#ifdef USE_SA
> +	priv->priv_sa = 0;
> +#endif
>  	return SMFIS_CONTINUE;
>  }
>  
> diff -ruN milter-greylist/milter-greylist.h mg_manu/milter-greylist.h
> --- milter-greylist/milter-greylist.h	2008-09-07 02:13:34.000000000 +0200
> +++ mg_manu/milter-greylist.h	2008-09-26 11:09:05.000000000 +0200
> @@ -213,6 +213,11 @@
>  #ifdef USE_P0F
>  	char *priv_p0f;
>  #endif
> +#ifdef USE_SA
> +	int priv_sa;
> +	int priv_sa_spam;
> +	int priv_sa_score10;
> +#endif
>  };
>  
>  sfsistat mlfi_connect(SMFICTX *, char *, _SOCK_ADDR *);
> diff -ruN milter-greylist/sa.c mg_manu/sa.c
> --- milter-greylist/sa.c	1970-01-01 01:00:00.000000000 +0100
> +++ mg_manu/sa.c	2008-09-26 12:51:49.000000000 +0200
> @@ -0,0 +1,429 @@
> +#ifdef USE_SA
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <netdb.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <sysexits.h>
> +#include <syslog.h>
> +
> +#include "acl.h"
> +#include "conf.h"
> +#include "queue.h"
> +#include "milter-greylist.h"
> +
> +#include "sa.h"
> +
> +#define SA_PORT "783"
> +#define SA_BUFLEN 1024
> +
> +#define SA_SPAMD_SPAMD "SPAMD/"
> +#define SA_SPAMD_VERSION "1.1"
> +#define SA_SPAMD_OK "EX_OK"
> +#define SA_SPAMD_SPAM "Spam: "
> +#define SA_SPAMD_TRUE "True"
> +#define SA_SPAMD_FALSE "False"
> +
> +#define SA_ERR_PROTO "sa spamd protocol error"
> +#define SA_ERR_VERSION "sa spamd protocol version mismatch"
> +#define SA_ERR_STATUS "sa spamd returned non-ok"
> +
> +static int sa_check(acl_data_t *, acl_stage_t, struct acl_param *, struct mlfi_priv *);
> +static void sa_rcvhdr(struct mlfi_priv *, char *, int);
> +static char *sa_trim(char *);
> +static int sa_read(int, char *, size_t);
> +static int sa_write(int, char *, size_t);
> +static int sa_socket(char *, char *);
> +static int sa_unix_socket(char *);
> +static int sa_inet_socket(char *, char *);
> +
> +static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

Not needed, see below ...

[..]
> +static void
> +sa_rcvhdr(priv, str, len)
> +	struct mlfi_priv *priv;
> +	char *str;
> +	int len;
> +{
> +	struct rcpt rcpt;
> +	char ipstr[IPADDRSTRLEN];
> +	char myhostname[ADDRLEN + 1];
> +	char now[SA_BUFLEN];
> +	time_t t;
> +	struct tm *tm;

Add this here, explanation see below ...
+     struct tm tm_buf;

> +
> +	iptostring(SA(&priv->priv_addr), priv->priv_addrlen, ipstr, sizeof(ipstr));
> +
> +	if(gethostname(myhostname, SA_BUFLEN)) {
> +		mg_log(LOG_WARNING, "sa gethostname failed");
> +		strcpy(myhostname, "unknown");
> +	}
> +
> +	/*
> +	 * localtime isn't thread-safe.
> +	 */
> +	t = time(NULL);
> +	pthread_mutex_lock(&mutex);
> +	tm = localtime(&t);
> +	if (strftime(now, len, "%a, %d %b %Y %H:%M:%S %z", tm) == 0) {
> +		mg_log(LOG_WARNING, "sa strftime failed");
> +		now[0] = '\0';
> +	}
> +	pthread_mutex_unlock(&mutex);

I would suggest to use localtime_r() (gmtime_r(), respectively) which
makes the use of a mutex obsolete.


+	tm = localtime_r(&t, &tm_buf);

Another problem may arise with strftime formatstring: %z is GNU libc
specific which is probably not portable. Solaris, for instance, do not
have this format specifier.

Taking a look at the sendmail source code (arpadate.c) there is a
respectable and portable routine calculating the offset from the
difference of gmtime() and localtime().

But to make it short, such a beast may not be necessary in this case:
The timestamp in the artificial milter received line is not that
important, especially regarding the timezone. I would suggest to use
gmtime() with a static timezone offset +0000, the time information will
be correct anyway, e.g.


+	tm = gmtime_r(&t, &tm_buf);
+	if (strftime(now, len, "%a, %d %b %Y %H:%M:%S +0000", tm) == 0) {


I hope, this helps ;)

Regards,

Johann

Johann Klasek <johann@...> wrote:

> I hope, this helps ;)

It does, but please send a patch! 

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On Thu, Oct 02, 2008 at 05:43:30AM +0200, manu@... wrote:
> Johann Klasek <johann@...> wrote:
> 
> > I hope, this helps ;)
> 
> It does, but please send a patch! 

Ok, got it ...

If no one protests and no further discussion happens ...
see attachment ;)

Johann