search

GermanCoding / Roundcube_TLS_Icon

Roundcube plugin that displays a lock icon next to the subject line, showing the encryption state of an inbound mail
Other
10 stars 3 forks source link

Conflicting config.inc.php files due to installer autocopy #4

Open williamdes opened 2 years ago

williamdes commented 2 years ago 
Received: from mail-oi1-f171.google.com (mail-oi1-f171.google.com [209.85.167.171])
    (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
    key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
    (No client certificate requested)
    by xxxx.xxxxx.wdes.eu (Postfix) with ESMTPS id 32C204606D
    for <williamdes@wdes.fr>; Wed, 6 Jul 2022 00:27:21 +0000 (UTC)

Do you want me to make a PR and fancy tests ?

GermanCoding commented 2 years ago

This looks like the usual Postfix TLS received header, so this should work already?

But yeah generally I'm not opposed to supporting other header formats.

williamdes commented 2 years ago

It seems like it does not work, I have set for two ignored hops but it does not help I think it would be helpful to build a unit test suite

GermanCoding commented 2 years ago

Yeah definetly, testing is always useful. The received header above definetly looks like it should work, but maybe some other headers get in the way?

Edit: Oh, also the support for ignoring headers isn't currently in 1.1, only if you pull dev-master via composer. Probably going to make a new release soon.

williamdes commented 2 years ago

Thank you for the release ! French translations work as expected, the hops do not help and internal or external emails are not handled correctly So I will make other PRs to improve and test all that ;)

williamdes commented 2 years ago

By the way my mails use the docker mailserver project

williamdes commented 2 years ago

Just added a test for this in https://github.com/GermanCoding/Roundcube_TLS_Icon/pull/8

williamdes commented 2 years ago

After some debug I found out that the config is set to 0 for some reason (probably the default config is loaded, why ? how ?)

["tls_icon_ignore_hops"]=> int(0)
williamdes commented 2 years ago

Hi @GermanCoding I did figure out what was wrong, it comes from this plugin:

So I commented out the line in /usr/src/roundcubemail/plugins/tls_icon/config.inc.php and restarted the docker container And now my config /var/roundcube/config/tls_icon.php is taken in account. And the TLS header that I reported works fine !

What do you think about what I reported ?

docker-compose.yml

roundcube-webmail:
        build: ../../docker/roundcube
        restart: on-failure:15
        volumes:
          - ../../config/roundcube/config/:/var/roundcube/config/:ro
        environment:
            ROUNDCUBEMAIL_SIEVE_HOST: imap.example.org
            ROUNDCUBEMAIL_DEFAULT_HOST: ssl://imap.example.org
            ROUNDCUBEMAIL_DEFAULT_PORT: 993
            ROUNDCUBEMAIL_SMTP_SERVER: tls://smtp.example.org
            ROUNDCUBEMAIL_SMTP_PORT: 587
            ROUNDCUBEMAIL_PLUGINS: archive,zipdownload,vcard_attachments,markasjunk,managesieve,enigma,additional_message_headers,acl,show_additional_headers,userinfo,contextmenu,authres_status,show_folder_size,quota,thunderbird_labels,tls_icon
            ROUNDCUBEMAIL_UPLOAD_MAX_FILESIZE: 30M
            ROUNDCUBEMAIL_DB_TYPE: mysql
            ROUNDCUBEMAIL_DB_HOST: db-host
            ROUNDCUBEMAIL_DB_PORT: 3306
            ROUNDCUBEMAIL_DB_USER: ${ROUNDCUBEMAIL_DB_USER}
            ROUNDCUBEMAIL_DB_PASSWORD: ${ROUNDCUBEMAIL_DB_PASSWORD}
            ROUNDCUBEMAIL_DB_NAME: ${ROUNDCUBEMAIL_DB_NAME}
            ROUNDCUBEMAIL_SKIN: elastic
        ports:
            - "80:80"
GermanCoding commented 2 years ago

I don't see a common behavior where roundcube plugins comment out their defaults. You sometimes see commented out config lines where it does not make sense to set an explicit default, but that isn't the case here - it should be a sensible default value (and it also happens to be the same value as if unset). What plugins specifically do you see that have their entire configs commented out?

It's unfortunate that other definitions of the same config key can get ignored, but I don't see any good option to fix this. What I would suggest is to make any modification to the plugin config in the plugin's config file directly and not try to do this anywhere else. The Roundcube plugin installer is what copies config.inc.php.dist to config.inc.php, but it should not touch existing configs. Hence you should be able to reliably modify config.inc.php directly and have that persisted across instance rebuilds. That's what I do for most of my plugins.

This may require persisting plugin folders within docker, which is not great, but it wasn't me who decided to have plugin configs live in the plugin folder.

williamdes commented 2 years ago

What plugins specifically do you see that have their entire configs commented out?

All the plugins of this list (they are not commented but in dist mode):

So I would conclude that there is something did on purpose so that config.inc.php files do not exist

@thomascube @alecpl Could you comment about config loading for plugins ? On this plugin my config created and mounted by Docker is not taken in account until I comment out the config line at config.inc.php of this plugin or remove the file. As I said in https://github.com/GermanCoding/Roundcube_TLS_Icon/issues/4#issuecomment-1179718514

alecpl commented 2 years ago

.dist files are samples and usually (I can't talk about the 3rd party plugins) they contain all plugin options and their defaults. User has to copy config.inc.php.dist to config.inc.php because .dist files are not being used. Some plugins will work without config.inc.php file just fine, some might not.

GermanCoding commented 2 years ago

.dist files are samples and usually (I can't talk about the 3rd party plugins) they contain all plugin options and their defaults. User has to copy config.inc.php.dist to config.inc.php because .dist files are not being used. Some plugins will work without config.inc.php file just fine, some might not.

Yes, though nowadays the Roundcube plugin installer copies config.inc.php.dist to config.inc.php automatically: https://github.com/roundcube/plugin-installer/blob/c4335e20b86cfe3a184ccf24d675c6a0338a372a/src/Roundcube/Composer/ExtensionInstaller.php#L64

I think this was added in roundcube/plugin-installer v0.3.0

The question is now, what if the user doesn't actually want that, because the config is managed externally/somewhere else? In this case it appears that the automatically generated config.inc.php takes precedence, which is not desired in this case.

It appears that, for some plugins, config.inc.php is not created automatically. I guess that is because either

A suggested workaround seems to be to have everything commented out by default (so that when config.inc.php is created automatically it doesn't contain anything useful), which doesn't sound appealing to me. What's the supposed flow here?

dilyanpalauzov commented 1 year ago

Here come headers inserted by sendmail. I show the two uppermost received headers, only the second one is relative:

Received: from mail.aegee.org ([unix socket])    by mail.aegee.org (Cyrus 3.4.4) with LMTPA;     Sun, 18 Dec 2022 07:03:24 +0000
Received: from 69-171-232-143.mail-mail.facebook.com (69-171-232-143.mail-mail.facebook.com [69.171.232.143])   by mail.aegee.org (8.17.1/8.17.1) with ESMTPS id 2BI73F8b1489360    (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO)  for <my@address>; Sun, 18 Dec 2022 07:03:16 GMT

verify=NO means that the sending server has not volunarily presented a certificate. Another example with verify=OK

Received: from mail.aegee.org ([unix socket])    by mail.aegee.org (Cyrus 3.4.4) with LMTPA;     Fri, 16 Dec 2022 22:41:09 +0000
Received: from smtp.github.com (out-18.smtp.github.com [192.30.252.201])    by mail.aegee.org (8.17.1/8.17.1) with ESMTPS id 2BGMf4uY685293 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for <my@address>; Fri, 16 Dec 2022 22:41:05 GMT

Please add parsing for sendmail-generated Received: headers and possibly add an option whether only postfix or only sendmail generated headers shall be handled (if this option would make things faster).

GermanCoding commented 1 year ago

This issue has went a bit sideways, we're currently tracking an issue related to config.inc.php here. I have moved @dilyanpalauzov's comment to a new issue #11

GermanCoding commented 1 year ago

Regarding this issue:

I tried some things and I noticed an issue:

When I comment out the config option in config.inc.php I get error messages from PHP

PHP Error: Failed to load config from /var/www/html/plugins/tls_icon/config.inc.php in /var/www/html/program/lib/Roundcube/rcube_plugin.php on line 166

The problem appears to be that if all config options are commented out, no $config is defined for the plugin, leading to a configuration error. I can fix that by adding a dummy $config[] = 0;, but is that going to conflict with $config definitions elsewhere?

So even if we were to comment-out everything by default, that's still going to cause problems in setups where the default config.inc.php is not overriden. So I'm really out of ideas as to how to solve this, except downgrading to an old version of the plugin installer that did not yet have the config autocopy.

williamdes commented 1 year ago

Maybe you should ping upstream to know what is best?