Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMAP auth is broken due to change of config variables, error on ssl mode #52

Closed
janaurka opened this issue Mar 16, 2019 · 25 comments
Closed

Comments

@janaurka
Copy link

Hi *

After upgrading to user_external v0.6 logging in using IMAP does not work anymore.

I’m seeing the following in the logs:

{"reqId":"5iUZ0(...)","level":3,"time":"2019-03-16T02:30:58+00:00","remoteAddr":"$IP","user":"--","app":"PHP","method":"POST","url":"\/login?user=$USER","message":"Use of undefined constant ssl - assum
ed 'ssl' at \/var\/www\/nextcloud\/config\/config.php#33","userAgent":"Mozilla\/5.0 (X11; Linux x86_64) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/73.0.3683.75 Safari\/537.36","version":"15.0.5.3"}
{"reqId":"bM1aectC(...)","level":3,"time":"2019-03-16T02:30:58+00:00","remoteAddr":"$IP","user":"--","app":"PHP","method":"GET","url":"\/login?user=$USER","message":"Use of undefined constant ssl - assumed 'ssl
' at \/var\/www\/nextcloud\/config\/config.php#33","userAgent":"Mozilla\/5.0 (X11; Linux x86_64) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/73.0.3683.75 Safari\/537.36","version":"15.0.5.3"}
{"reqId":"gzT2LKW(...)","level":3,"time":"2019-03-16T02:30:59+00:00","remoteAddr":"$IP","user":"--","app":"PHP","method":"GET","url":"\/cron.php","message":"Use of undefined constant ssl - assumed 'ssl' at \/var\/www\/next
cloud\/config\/config.php#33","userAgent":"Mozilla\/5.0 (X11; Linux x86_64) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/73.0.3683.75 Safari\/537.36","version":"15.0.5.3"}

I have changed the config from something like:

  array (
    0 => 
    array (
      'class' => 'OC_User_IMAP',
      'arguments' => 
      array (
        0 => '{$IMAP-IP:993/imap/ssl/novalidate-cert}',
      ),
    ),
  ),

to

  array (
    array (
      'class' => 'OC_User_IMAP',
      'arguments' => 
      array (
        '$IMAP-IP', 993, ssl
      ),
    ),
  ),

(as it should be according to the updated readme).

which caused the error message in nextcloud.log as shown above.

Running Nextcloud Stable latest (15.0.5).

# php-fpm7.0 --version
PHP 7.0.33-0ubuntu0.16.04.2 (fpm-fcgi)
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.33-0ubuntu0.16.04.2, Copyright (c) 1999-2017, by Zend Technologies

# uname -a
Linux nextcloud 4.4.0-143-generic #169-Ubuntu SMP Thu Feb 7 07:56:38 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

# cat /etc/*-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.6 LTS"
NAME="Ubuntu"
VERSION="16.04.6 LTS (Xenial Xerus)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 16.04.6 LTS"
VERSION_ID="16.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
VERSION_CODENAME=xenial
UBUNTU_CODENAME=xenial

Also changing the way the config file is parsed during a release is kinda uncool, especially since there is no remark in the release comments as breaking changes. Would be cool, if breaking changes would be announced at least in the GH release and if possible also in the Nextcloud UI/occ when trying to perform an update.

+ it would be great if there would be a list of possible options (eg for the ssl mode) at least somewhere in the code :)

lmk if you need more logoutput and stuff. I’ve fixed the problem for now by c&p the imap.php file from the release before and simply not running any sanity check on the files, which is ... suboptimal.

cheers

@janaurka janaurka added 0. Needs triage bug Something isn't working labels Mar 16, 2019
@MarBie77
Copy link

Just ran into the same issue, you have to escape the ssl_mode param:

array (
    array (
      'class' => 'OC_User_IMAP',
      'arguments' => 
      array (
        '$IMAP-IP', 993, 'ssl', ''
      ),
    ),
  ),

and I added an empty domain param, so the new source code doesn't change the username.

I hope this helps.

@megamaced
Copy link

thanks @MarBie77 that fixed it for me

@violoncelloCH
Copy link
Member

violoncelloCH commented Mar 16, 2019

thank you for reporting! @janaurka
as @MarBie77 said, the sslmode needs to be in quotes too, because it's a string (I already made PR #53 to fix the documentation; it's waiting for approving review so we can merge it)

see #49 for the changed IMAP auth:
we now (starting from v0.6.0) use roundcube's IMAP implementation, which uses direct socket connections, so we can get rid of the deprecated php-imap library
this deprecated php-imap library didn't support >TLS 1.0
with the old implementation we directly passed the parameters (arguments) to php-imap, but the parameters in this form don't fit the new implementation what is why we changed the documentation for the configuration...

Please kindly note that this app is not above v1.0.0, so breaking changes can happen any time.
However you're right, that I should've added a warning in the Github release. (added one now :) )

Regarding the possible sslmodes I first have to check what roundcube takes as parameters there. At least 'ssl' or 'tls' should be possible there.

@janaurka
Copy link
Author

and I added an empty domain param, so the new source code doesn't change the username.

D*mn, I’ve thought that I tested this quickly and it did not work for me. Although I did not enter an empty string for the domain. Whatever if it works, I’m pretty happy

@violoncelloCH: awesome thx, also for writing/maintaining this plugin. i did not mean to be harsh or so. I’m aware on how stressful/annoying it can be to maintain open source software and some random people open angry bugs and basically yell at you for doing stuff they can use for free. again: danke tuusig (thanks a lot in swiss german).

I’d leave it open for now, feel free to close @violoncelloCH as soon as everything is merged to master and release is carved or whatever.

@daviddahlberg
Copy link

daviddahlberg commented Mar 16, 2019

The following works for me[tm]:
'$ip:993/imap/ssl/novalidate-cert'
->'$hostname',993,ssl

And I put the hostname into the chrooted hosts file (/var/www/etc/hosts), so that validation may succeed.

But I still would prefer to have some documentation on how to put those the PHP parameters correctly into the config file.

@violoncelloCH
Copy link
Member

thank you @janaurka for these words :)

as far as I understand from https://github.com/roundcube/roundcubemail/wiki/Configuration#imap-server-connection and roundcube's source code (https://github.com/roundcube/roundcubemail/blob/master/program/lib/Roundcube/rcube_imap_generic.php#L972-L987 && https://github.com/roundcube/roundcubemail/blob/master/program/lib/Roundcube/rcube_imap_generic.php#L1034-L1062) 'ssl' or 'tls' are the only supported parameters.
I added a commit accordingly to the open PR #53.

@daviddahlberg I think I don't fully get what you mean. Do you mean documentation about non-validated certs?

@Erling74
Copy link

A quick update of available apps ended here on this topic... ;-)
Thanks to this post I replaced:

array (
0 =>
array (
'class' => 'OC_User_IMAP',
'arguments' =>
array (
0 => '{localhost:993/imap/ssl/novalidate-cert}',
),
),
),

First I replaced with:
array (
array (
'class' => 'OC_User_IMAP',
'arguments' =>
array (
'localhost', 993, 'ssl', ''
),
),
),

Unfortunate this didn't solve the problem... After replacing "localhost" with the FQDN of my server everything worked like a charm again, so I assume you can't ignore the certificate check anymore.

Is this correct or is there a different way to do this?

@PaulFreund
Copy link

I also tried various different config settings starting from the old working parameters (993 and SSL against my domain name) and still can not authenticate. Dovecot shows the following:

Mar 17 15:21:56 XXXX dovecot: imap-login: Disconnected (no auth attempts in 1 secs): user=<>, rip=::1, lip=::1, TLS, session=<xxxx/xxxxxxxxxxxxxxxxxxxxxx>

I reverted back to 0.5.1 and it worked again, especially the user= field in the log is then populated which indicates that the requests to IMAP are malformed

@lsbbs
Copy link

lsbbs commented Mar 19, 2019

The code taken from roundcube is giving in this way another problem:
I have a nextcloud instance running where I have users from two domains (this to domains are two departments of a company). But on the IMAP server are more than this two domains.
Striping out the domain part will now create a problem with duplicate usernames.
Without restricting the login to domains users from other domains can login. Not good, and not wanted.

@Flachzange
Copy link

After replacing "localhost" with the FQDN of my server everything worked like a charm again,

I can confirm this. With localhost I get only "Login failed".

However, I see now the following error messages:

[PHP] Error: Undefined offset: 1 at /www/htdocs/..../user_external/lib/imap/imap_rcube.php#149

[PHP] Error: Undefined index: force_caps at /www/htdocs/.../user_external/lib/imap/imap_rcube.php#945

@violoncelloCH
Copy link
Member

@daviddahlberg @Erling74 @Flachzange

After replacing "localhost" with the FQDN of my server everything worked like a charm again, so I assume you can't ignore the certificate check anymore.

indeed, looks like roundcube's implementation doesn't allow this... however I think the advantages are way bigger than the disadvantages and ignoring certificate check is an edge-case anyway (you can simply use the FQDN or plain IMAP if you want to use localhost). However PRs (also to the documentation) are always welcome!


@PaulFreund could you open a new issue with details about your problem and configuration?


@lsbbs that seems to be not related to the change to roundcube's imap implementation. It looks like it's coming from this commit: a7e276f (this stripes away the domain part, if a domain is specified) from which version did you update to v0.6? Could you also create a new issue with further details to continue discussing this?

@Flachzange
Copy link

@violoncelloCH
Any clue regarding the error messages that show up for me (see above)? Thx

@violoncelloCH
Copy link
Member

@Flachzange take a look at #56 (that's the same)

@violoncelloCH violoncelloCH pinned this issue Mar 23, 2019
@alexhass
Copy link

Changed config as noted above, but now nextcloud windows client fails to authenticate. The web auth works, but that does not help.

@violoncelloCH
Copy link
Member

@alexhass are you using app-passwords / tokens for your sync client? If so, it might be worth a try to set new ones and revoke the old ones.

@lsbbs
Copy link

lsbbs commented Mar 28, 2019 via email

@violoncelloCH
Copy link
Member

@lsbbs as I said, if you want to continue discussing this, open a specific issue for it. This issue here is definitely not the place to do it, as this issue is solved (documentation) and is only still open as information.

@madmas
Copy link

madmas commented Apr 25, 2019

Hi @violoncelloCH,
I must say that your statement

Please kindly note that this app is not above v1.0.0, so breaking changes can happen any time.

is somewhat inadequate.
The code of this app was part of nexcloud core and as such declared being stable quite some time ago. People have build systems with it relying on that functionality. Separating it out as a external app is a goot step I think, but doning so as a pre-1.0 and now telling people to not rely on it is just ignoring the way Nextcloud has come and might sounds arrogant to the users.

This failed upgrade has given me a lot of headaches today. Please be more careful with changes to such a sensitive module.
The points @lsbbs has made need to be taken into account. Are there open issues for those, yet?

Thanks a lot for your work on nextcloud, nevertheless.

@Flachzange
Copy link

Completely agree with @madmas. These ways of working will make people lose trust in the development of the plugin and nextcloud in general. (still appreciate the work though)

@violoncelloCH
Copy link
Member

@madmas I get your point...
if it should be considered stable because it was in Nextcloud core is something we could discuss and would probably never end with it...
It's not meant to say that people should not rely on it. "breaking changes" simply mean something that changes completely and needs intervention from the administrator to keep it working. What would have completely broken the application (at least at some point in the future) would have been to simply keep the old implementation. The php_imap library would have been removed when you update your php and/or distro and then it wouldn't have worked any more at all...
I know the ideal way would have been to provide some migrations which would automagically transform the old config into the new one (but looking at the different IMAP server setups, this would be quite impossible to be successful for everyone anyway). I apologize for not having done that; I neither have that much experience yet nor did I have the time to do it. I'll try to do it better next time. :)

@madmas & @lsbbs please take a look at #68 and #64; please split those two completely separate points

@trueshanti
Copy link

i am confused .. NC16 still lists this Plugin .. which one is the active development ? "https://github.com/nextcloud/apps/tree/master/user_external" or "https://github.com/nextcloud/user_external" ?

@violoncelloCH
Copy link
Member

@trueshanti what do you mean with "still lists this plugin"?
as you can see in the version in the apps repository, the user_external app has been moved (and extracted) to it's own, dedicated repo, that's here (https://github.com/nextcloud/user_external)... the apps repo will be archived soon, all work continues here in this repo...

@trueshanti
Copy link

Hi @janaurka

i lost track of the solution-path on this

After upgrading to user_external v0.6 logging in using IMAP does not work anymore.

What was your solution to this ?
Is IMAP-auth (full email as login) expected to work again in nv16 ?

kindly

@trueshanti
Copy link

@trueshanti what do you mean with "still lists this plugin"?

What i meant: in Nextcloud16 the Plugin "external user" contends itself with "Authenticate user login against ... IMAP" . to my understanding (albeit 0.6.3 just released) this is an actual feature , yes ? so it should work ?

@trueshanti
Copy link

nevermind .. i found #64 to be the right issue for me .. thank you

bors bot added a commit to Mailu/Mailu that referenced this issue Jun 24, 2019
1030: Update user_external example for nextcloud r=mergify[bot] a=kesselb

## What type of PR?

documentation

## What does this PR do?

Update the user_external example for Nextcloud due an upstream change. PHP will remove the imap extension. Newer user_external releases requires a different configuration for imap.

### Related issue(s)
- nextcloud/user_external#52

## Prerequistes
Before we can consider review and merge, please make sure the following list is done and checked.
If an entry in not applicable, you can check it or remove it from the list.

- [x] In case of feature or enhancement: documentation updated accordingly
- [x] Unless it's docs or a minor change: add [changelog](https://mailu.io/master/contributors/guide.html#changelog) entry file.


Co-authored-by: Daniel Kesselberg <mail@danielkesselberg.de>
@violoncelloCH violoncelloCH unpinned this issue Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests