STARTTLS support since v0.9 missing

[sophie-h]

My config was

      array (
        0 => 'mail.example.org',
        1 => 143,
        2 => 'tls',
        3 => '',
      ),

and it stopped working with v0.9:

ERROR: Could not connect to imap server via curl: error:1408F10B:SSL routines:ssl3_get_record:wrong version number

Looking at the new IMAP code for v0.9 it looks like STARTTLS support might have been dropped?

[violoncelloCH]

Thanks for reporting!

@rollbrettler could you take a look at this? I couldn't easily find if it's simply something that's not supported by curl...

On the other hand, are there specific reasons to use starttls rather than a connection which is encrypted from the beginning?

[ymage]

Well @violoncelloCH,

On the latter point, not everyone has the choice of protocols supported by their IMAP provider or has the choice of opened outbound ports with their company firewall.

And, here, it could be seen as a regression :-/

[sophie-h]

On the other hand, are there specific reasons to use starttls rather than a connection which is encrypted from the beginning?

There is a tendency to see IMAPS as deprecated and STARTTLS as the default protocol.

[sophie-h]

I couldn't easily find if it's simply something that's not supported by curl...

It's supported by curl. The following has to be set

$protocol = 'imap'; // not 'imaps'
curl_setopt($ch, CURLOPT_FTP_SSL, 1); 

[violoncelloCH]

thanks for the clarification @sophie-h !

Did you already test if this works?

According to this documentation in current versions of curl it is CURLOPT_USE_SSL instead of FTP: https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html

Would you be up for a PR? :)

[sophie-h]

Did you already test if this works?

According to this documentation in current versions of curl it is CURLOPT_USE_SSL instead of FTP: https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html

The constant is undocumented at php.net, but it seems to exists. Both variants work.

Would you be up for a PR? :)

Would be good to know what the exact v0.8 behavior was. I would guess

• tls STARTTLS
• ssl IMAPS

But was there support for unencrypted connections?

[mconstabel]

Hello,

on my server with dovecot and STARTTLS at Port 143, this works:

-       $protocol = $this->sslmode ? "imaps" : "imap";
+       $protocol = ($this->sslmode == 'ssl') ? "imaps" : "imap";
        $url = "{$protocol}://{$this->mailbox}:{$this->port}";
        $ch = curl_init();
+       if ($this->sslmode == 'tls') {
+           curl_setopt($ch, CURLOPT_USE_SSL, true);
+           //curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, 0);
+           //curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 0);
+       }
        curl_setopt($ch, CURLOPT_URL, $url);

To allow self-sgned certificates, uncomment the two extra lines.

  array (
    0 => 
    array (
      'class' => 'OC_User_IMAP',
      'arguments' => 
      array (
        0 => 'srv-mail',
        1 => 143,
        2 => 'tls',
        3 => 'domain.de',
        4 => true,
        5 => false,
      ),
    ),
  ),

[DatAres37]

I've got the same problem. Changing it to ssl and port 993 works.

[Disassembler0]

Same here. ~400 affected users, just lovely. Fortunately the fix proposed by @mconstabel works nicely.

On the other hand, are there specific reasons to use starttls rather than a connection which is encrypted from the beginning?

@violoncelloCH, there are. From network/sysadmin standpoint, you have only a single endpoint/service to manage, from user standpoint, ISP or hotspot firewalls may limit your access to well known ports, which unfortunately often means those which were well-known in 1995. Those may not be strong reasons, but it's still a perfectly legitimate, RFC-defined way to connect to mail servers, so there's no strong reason to remove the functionality either.

[mconstabel]

Update 2: Doesn't work always, see next post. Use complete code 3 posts above.

Update, it seems to be enough to switch only for ssl to imaps, else stay on imap.

-       $protocol = $this->sslmode ? "imaps" : "imap";
+       $protocol = ($this->sslmode == 'ssl') ? "imaps" : "imap";
        $url = "{$protocol}://{$this->mailbox}:{$this->port}";

The old code use imaps for ssl AND tls, which seems wrong.

For tls use imap as protocol and curl uses STARTTLS if needed.

[Disassembler0]

Update, it seems to be enough to switch only for ssl to imaps, else stay on imap.

This doesn't work for me. I have a setup where STARTTLS is mandatory on 143 and my version of curl doesn't send STARTTLS automatically (Ubuntu 18.04, curl 7.58.0, PHP 7.2.29). Swift look into the curl code suggests that use_ssl option needs to be set to true even for STARTTLS.

[Aquariu]

Dear all, Thanks for creating this issue to begin with and for rapidly providing a workaround.

In my case this is what I got in the logs:

ERROR: Could not connect to imap server via curl: error setting certificate verify locations:\n CAfile: /etc/certs/cabundle.pem\n CApath: /etc/ssl/certs","userAgent":"Mac OS X/10.14.6 (18G87) AddressBookCore/1","version":"18.0.3.0"}

Is there a reasonable way I should have inferred that the app was responsible for this ?

As others said. I see no valid reason to remove functionality that may break instances without warning.

On a related note, back a while ago when you switched to the roundcube lib, a couple of versions ago, there were very polite but also not very polite requests from sysadmins for you to warn admins beforehand when you introduce changes could break something on their side. Is there something that could be done to help detect this in testing phase, to avoid traps like this one ? I fell for it again and would love to avoid a third one :)

Cheers!

[staaki]

Swift look into the curl code suggests that use_ssl option needs to be set to true even for STARTTLS.

Please do not use true as a value for CURLOPT_USE_SSL. This option expects an integer and as true evaluates to 1 this would be interpreted as CURLUSESSL_TRY - opening the possibility of a downgrade attack. Instead, using CURLUSESSL_ALL properly fails with an error message of "STARTTLS not supported." if the server does not indicate STARTTLS support in its capability message.

Other than that, I use a similar patch like the earlier one by @mconstabel and it works fine here, too:

--- a/lib/imap.php
+++ b/lib/imap.php
@@ -85,15 +85,19 @@
                                        $groups[] = $pieces[1];
                }

-               $protocol = $this->sslmode ? "imaps" : "imap";
+               $protocol = ($this->sslmode == "ssl") ? "imaps" : "imap";
                $url = "{$protocol}://{$this->mailbox}:{$this->port}";
                $ch = curl_init();
                curl_setopt($ch, CURLOPT_URL, $url);
                curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
-               curl_setopt($ch, CURLOPT_USERPWD, $username.":".$password);
+               curl_setopt($ch, CURLOPT_USERNAME, $username);
+               curl_setopt($ch, CURLOPT_PASSWORD, $password);
                curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10);
+               if ($this->sslmode) {
+                       curl_setopt($ch, CURLOPT_USE_SSL, CURLUSESSL_ALL);
+               }

-               $canconnect = curl_exec($ch);
+               $canconnect = curl_exec($ch) !== false;

                if($canconnect) {
                        curl_close($ch);

[jficz]

Not cool guys. This is the second time a breaking change happened without warning.

Anyway, a hotfix workaround to force STARTTLS is

$protocol = "imap";
curl_setopt($ch, CURLOPT_USE_SSL, CURLUSESSL_ALL);

If you need more customization (like multiple domains with different config, etc.), you'll have to go with some kind of if..else structure - basically what @staaki did above.

[realkinetix]

You guys broke TLS and haven't posted an announcement on the module? Sysadmins are just supposed to upgrade the module and trust that their IMAP auth is still going to work?

This version should not be available in our nextcloud apps page at all when we're on stable. This is a major break!

This was reported 5 days ago. What gives?

[violoncelloCH]

please calm down! I know that this is not cool for you and we/I should have warned about possible breaking (but still, I don't really know which way I could have done this), however this was simply not expected; I'm not an IMAP expert and STARTTLS simply fell through our testing. Please note that I'm a volunteer maintaining this app in my free time. I'm even not using this app any more, I'm simply doing it so you can just click on update for new releases and that this app doesn't get incompatible on every new major Nextcloud release (and therefore blocks you from updating your Nextcloud) and mostly that contributions get some feedback and get merged... So please don't think that complaining here helps to make me more motivated to "fix this as soon as possible". (I'm not talking to those who brought valuable feedback and inputs, especially in the beginning.) Instead of complaining, why don't you open a Pull Request with a (possible) fix? That way we would've possibly already merged and released it. But instead of spending the little bit of free time I currently have available, I feel like I need to explain myself in here now... So please think one time more before complaining (in general, not just to a volunteer)...

[violoncelloCH]

So a fix is in #138 ; any feedback, testing, reviews etc welcome! As you @Aquariu asked about ways to help for testing.. that'd be always welcome... I'm still thinking about the best way to organize this, but maybe we could just create a list with everyone who wants to get notified when testing would be needed. What do you think?

[jficz]

Testing should imho be automated to maximum extent possible. There aren't that many combinations of configuration options so maybe test each combination using your test suite?

Learning from previous mistakes, both cases were, iirc, related to ssl/tls. That might be a good starting point to look for sensitive changes.

About informing admins - it might be as simple as adhering to Semantic versioning and maintaining a changelog. A sensible admin doesn't blindly hit "upgrade" button when major version changes.

I actually checked the release here on github before upgrading but it didn't mention anything that would point to any possibly breaking changes so I upgraded.

The most important thing imho is that this app, when used, is critical to service availability and if it breaks, everything breaks - possibly including admin not being able to log in to the instance and check the logs, which is pretty bad (also the logs weren't too useful in this specific case).

Any breaking change (i.e. a major version increase if adhering to semver) includes a real possibility of service downtime so it is important for both sides to understand the stakes. Devs should detect possibly breaking changes, include them in changelogs and bump versions accordingly. Admins should have a rollback plan handy in case of major version upgrade and ideally test the upgrade before deploying to production.

The key information devs need to communicate to admins is "here be dragons". The easiest way is to use a common versioning scheme that implies such information.

[ymage]

Thanks to you @violoncelloCH

[pgaufillet]

By the way: a simple work around is to revert manually to 0.8.0 (uninstall the app, unpack the 0.8.0 archive by hand).

[violoncelloCH]

Testing should imho be automated to maximum extent possible. There aren't that many combinations of configuration options so maybe test each combination using your test suite?

well of course... you're more than welcome to help creating/writing those... (there is drone and travis ci available from the Nextcloud organisation) but to discuss this we should switch over to #40 or a new issue; thanks!

Learning from previous mistakes, both cases were, iirc, related to ssl/tls. That might be a good starting point to look for sensitive changes.

the update a year ago needed to completely change the configuration for IMAP because we couldn't keep using this string for the deprecated php-imap extension... (but it [the roundbube lib] brought several other problems, some we didn't manage to fix until now; but this is why we use curl now thanks to @rollbrettler

regarding SEMVER: this is actually why I incremented the minor version (not just the patch version)... take a look at paragraph 4 of the resource you cited (https://semver.org/#spec-item-4)... Of course we could increment the major version, but this would imply that this app should suddenly be considered as more stable than during the past years; clearly something I don't feel like doing so is what I'm supposed to do...

[rollbrettler]

@violoncelloCH thanks for fixing it and sorry for coming to late :bowing_woman: