Add option to quarantine message

[frankcrawford]

Any thoughts about adding an option to quarantine rather than reject messages?

[gdt]

As far as I know the milter interface does not have any quarantine mechanism. You could implement quarantining in an MTA, but it's likely a lot of work.

Can you point to any other milters that have quarantine functionality?

[andybalholm]

According to http://cpansearch.perl.org/src/AVAR/Sendmail-PMilter-0.98/doc/milter-protocol.txt, there is a SMFIR_QUARANTINE ('q') response, added in Sendmail 8.13.

[gdt]

Thanks, that looks simple enough. So presumably one could add a -q option to quarantine over some # of points, parallel to the -r option, and reject would take priority if both thresholds were met. (E.g, -q 5 -r 10.) It of course remains to make an MTA system implement quarantining and allow management of it, which seems much more complicated than delivering spam to a spam folder.

[andybalholm]

I think Postfix implements the quarantine action for milters too, since http://www.postfix.org/MILTER_README.html says:

The "quarantine" action is like "accept" but freezes the message in the "hold" queue, and is available with Postfix 2.6 or later.

[frankcrawford]

Yes, the milter command has been in place for over 10 years, but isn't widely used. The only place I've seen it is in the clamav-milter.

I think it should be simple to do it similar to the a reject, but not certain of the details to add it myself.

[frankcrawford]

BTW, if you are doing centralised spam management, it is simpler if it stays in an MTA queue than in individual spam folders, and also easier to release it with no corruption, if the user requests it. Taking a copy of the email and trying to reinject it often causes issues with MIME emails.

[frankcrawford]

I know this is from a long time ago, but I noticed in the official site there is a patch for adding quarantine to spamass-milter. It was never rolled in and is for an older version, but it is a start.

You can see it here http://savannah.nongnu.org/patch/index.php?7950

[andybalholm]

I've attempted to update the patch for the current version, and created a "quarantine" branch in this repository. Go ahead and test the quarantine branch; if you report that it works, I'll merge it to master.

(I don't run SpamAssassin myself any more, so I can't test it.)

[frankcrawford]

@andybalholm okay, give me a few days to test it.

[frankcrawford]

Sorry, been tied up. I've just got around to doing some testing.

I did notice one minor issue, a compilation warning:

g++ -DHAVE_CONFIG_H -I. -I. -I.     -g -O2 -Wall -fno-default-inline -fno-inline  -c -o spamass-milter.o `test -f 'spamass-milter.cpp' || echo './'`spamass-milter.cpp
spamass-milter.cpp: In function ‘int assassinate(SMFICTX*, SpamAssassin*)’:
spamass-milter.cpp:634:44: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]
  634 |                 if ( smfi_quarantine( ctx, "SPAM" ) != MI_SUCCESS) {
      |                                            ^~~~~~
g++  -g -O2 -Wall -fno-default-inline -fno-inline    -o spamass-milter  spamass-milter.o  -lmilter  

However, easy fix:

diff --git a/spamass-milter.cpp b/spamass-milter.cpp
index 356fe82..8e3932d 100644
--- a/spamass-milter.cpp
+++ b/spamass-milter.cpp
@@ -631,7 +631,7 @@ assassinate(SMFICTX* ctx, SpamAssassin* assassin)
        if (do_quarant)
        {
                debug(D_MISC, "Quarantining");
-               if ( smfi_quarantine( ctx, "SPAM" ) != MI_SUCCESS) {
+               if ( smfi_quarantine( ctx, const_cast<char *>("SPAM") ) != MI_SUCCESS) {
                        throw string( "Failed to quarantine" );
                }
                return SMFIS_CONTINUE;

Anyway, I'll test it out over the next few days and let you know if any issues come up.

[frankcrawford]

P.S. I did notice one other item, in that I'm currently using the Fedora distributed version, which I don't know where they found it, but "Ignores messages if the sender has authenticated via SMTP AUTH."

This does look similar to your authencated user patch, but also seems to be different coding, so I'm not sure if they are the same effect or not.

Any chance you can add this in sometime?

[frankcrawford]

I have it in place and will see how it goes over the next week, however, I did find one other issue, that was causing it to crash during runtime. It was missing the `:' in the argument definition, and hence was failing to handle the argument for the new argument.

diff --git a/spamass-milter.cpp b/spamass-milter.cpp
index 356fe82..7340777 100644
--- a/spamass-milter.cpp
+++ b/spamass-milter.cpp
@@ -195,7 +195,7 @@ int
 main(int argc, char* argv[])
 {
    int c, err = 0;
-   const char *args = "aAfd:mMp:P:r:l:u:D:i:b:B:e:xS:R:c:C:g:T:Q";
+   const char *args = "aAfd:mMp:P:r:l:u:D:i:b:B:e:xS:R:c:C:g:T:Q:";
    char *sock = NULL;
    char *group = NULL;
    bool dofork = false;
@@ -631,7 +631,7 @@ assassinate(SMFICTX* ctx, SpamAssassin* assassin)
     if (do_quarant)
     {
         debug(D_MISC, "Quarantining");
-       if ( smfi_quarantine( ctx, "SPAM" ) != MI_SUCCESS) {
+        if ( smfi_quarantine( ctx, const_cast<char *>("SPAM") ) != MI_SUCCESS) {
             throw string( "Failed to quarantine" );
         }
         return SMFIS_CONTINUE;

[andybalholm]

I've applied your changes to the quarantine branch.

[frankcrawford]

I've now been running the quarantine option for a bit over 2 weeks, and it works fine. I haven't seen any issues.

However, there was one initial surprise I did receive, the bitbucket address (-b|-B) no longer works, because it operates by manipulating the reject code. This is not really a bug, just something that should be noted in any man page update.

[andybalholm]

What changes should we make to the documentation?

[frankcrawford]

Add the option obviously, but also probably something in the -B|-b that it only works with the -r option, and something in the -Q option that it -B|-b has is ignored for any messages that are quarantined.

Because you can use both -r and -Q (with differing values) -B|-b will still work on any rejected messages, just not on quarantined messages.

[andybalholm]

I've pushed a commit with documentation changes to the quarantine branch. How does it look?

[frankcrawford]

Looks good. I made a few minor additions:

diff --git a/spamass-milter.1.in b/spamass-milter.1.in
index 2576baa..29f0b61 100644
--- a/spamass-milter.1.in
+++ b/spamass-milter.1.in
@@ -56,7 +56,8 @@ Always scan and tag messages but treat
 .Fl i
 and
 .Fl a
-as an exception from rejecting and defering mails classified as spam.
+as an exception from rejecting, quarantining and defering mails
+classified as spam.
 .It Fl b Ar spamaddress
 Redirects tagged spam to the specified email address.
 All envelope recipients are removed, and inserted into the message as
@@ -189,6 +190,7 @@ headers as well.
 Both tagged and untagged mail gets passed through unchanged.
 To be useful, this option should be used with the
 .Fl r ,
+.Fl Q ,
 .Fl b , 
 or
 .Fl B
@@ -203,7 +205,8 @@ Create the file
 .Ar pidfile ,
 containing the processid of the milter.
 .It Fl Q Ar nn
-Quarantine scanned email if it greater than or equal to
+Quarantine scanned email in sendmail's quarantine queue,
+if it greater than or equal to
 .Ar nn .
 If 
 .Ar -1 ,
@@ -211,6 +214,11 @@ quarantine scanned email if SpamAssassin tags it as spam (useful if you
 are also using the
 .Fl u
 flag, and users have changed their required_hits value).
+.Pp
+This can be used with
+.Fl r ,
+but reject takes precedence, so the value given here should have a lower
+value.
 .It Fl r Ar nn
 Reject scanned email if it greater than or equal to
 .Ar nn .

[andybalholm]

I've applied those, and merged quarantine into master.

[frankcrawford]

@andybalholm FYI, I've convinced the Fedora maintainer to add this patch to the Fedora rawhide distribution. It will probably filter through the distribution over time.