philpennock/sieve-connect

Handle patched Cyrus versions that don't resend capabilities

gsiv opened this issue · 2 comments

gsiv commented

Phil,

this is the same patch I sent in a while ago. I'm only adding it here for
referencing purposes.

This patch adds support for the special case of patched Cyrus servers
which behave differently from what sieve-connect would infer from their
version numbers.

Some modern servers get patched to not resend their capabilities after
STARTTLS.  For example, Kolab's Cyrus gets patched this way to ensure
compatibility with older Kontact versions (Cyrus timsieved
v2.3.16-kolab-nocaps).

sieve-connect simply needs to do nothing in regards to compatibilities
after STARTTLS in this case.  This patch applies this behavior to any
Cyrus servers which carry "nocaps" in their version string.

---
 sieve-connect.pre.pl |   32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/sieve-connect.pre.pl b/sieve-connect.pre.pl
index 0474bcb..20bdb35 100755
--- a/sieve-connect.pre.pl
+++ b/sieve-connect.pre.pl
@@ -699,12 +699,34 @@ if (exists $capa{STARTTLS}) {
        # This means that if they don't support NOOP by 2.3.14, I have to
        # figure out how to decide what is safe and backtrack which version
        # precisely was the first to send the capability response correctly.
+       #
+       # There is also at least one special version which needs special
+       # treatment despite having a higher version number: Kolab's patched
+       # "nocaps" Cyrus version (see below).
        my $use_noop = 1;
-       if (exists $capa{"IMPLEMENTATION"} and
-               $capa{"IMPLEMENTATION"} =~ /^Cyrus timsieved v2\.3\.(\d+)\z/ and
-               $1 >= 13) {
+       my $timsieved_nocaps = 0;
+       my $cyrus_version_extension = '';
+       if (exists $capa{"IMPLEMENTATION"}) {
+           if ($capa{"IMPLEMENTATION"} =~ /^Cyrus timsieved v2\.3\.(\d+)\z/
+                   and $1 >= 13) {
                debug("--- Cyrus drops connection with dubious log msg if send NOOP, skip that");
                $use_noop = 0;
+           } elsif ($capa{"IMPLEMENTATION"} =~
+                   /^Cyrus timsieved v2\.3\.\d+-([\w-]*nocaps)\z/ ) {
+               $cyrus_version_extension = $1;
+               # Special case: Cyrus may have been patched to not resend its
+               # capabilities for compatibility with older Kontact versions.
+               # Kolab does this, for example; see
+               # https://roundup.kolab.org/issue2443.  This patch may even
+               # have been applied to more modern versions such as v2.3.16.
+               # For this reason, we need to specifically check for this case.
+               # If a nocaps server is detected, nothing needs to be done (see
+               # the $timsieved_nocaps check below).
+               debug("--- Special Cyrus server version detected:" .
+                       " $cyrus_version_extension");
+               $use_noop = 0;
+               $timsieved_nocaps = 1;
+           }
        }

        if ($use_noop) {
@@ -713,6 +735,10 @@ if (exists $capa{STARTTLS}) {
                parse_capabilities($sock,
                        sent_a_noop     => $noop_tag,
                        external_first  => $prioritise_auth_external);
+       } elsif ($timsieved_nocaps) {
+           # For the nocaps version of Cyrus, nothing should be done here.
+           debug("--- $cyrus_version_extension: using capabilities" .
+                   " transmitted before STARTTLS!");
        } else {
                parse_capabilities($sock,
                        external_first  => $prioritise_auth_external);

I've just seen, after merging to master, that I messed up $env when putting your name into the commit.

Author: Gernot Schulz gernot@intevation.de <pdp@spodhuis.org>

My stupidity is now recorded for eternity, but the intent is clear and I don't think this warrants a force-push to clean up.

gsiv commented

On Mon, Aug 29, 2016 at 01:57:58PM -0700, Phil Pennock wrote:

My stupidity is now recorded for eternity, but the intent is clear
and I don't think this warrants a force-push to clean up.

You're absolutely right; don't worry about it. Thanks for accepting
the patch!