Skip to content

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

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

Description

@williamdes
williamdes
opened on Jul 6, 2022
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 ?

Activity

  1. GermanCoding commented on Jul 6, 2022

    @GermanCoding
    GermanCoding
    on Jul 6, 2022
    Owner

    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.

  2. williamdes commented on Jul 6, 2022

    @williamdes
    williamdes
    on Jul 6, 2022
    CollaboratorAuthor

    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

  3. GermanCoding commented on Jul 8, 2022

    @GermanCoding
    GermanCoding
    on Jul 8, 2022
    Owner

    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.

  4. williamdes commented on Jul 8, 2022

    @williamdes
    williamdes
    on Jul 8, 2022
    CollaboratorAuthor

    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 ;)

  5. williamdes commented on Jul 8, 2022

    @williamdes
    williamdes
    on Jul 8, 2022
    CollaboratorAuthor

    By the way my mails use the docker mailserver project

  6. williamdes commented on Jul 9, 2022

    @williamdes
    williamdes
    on Jul 9, 2022
    CollaboratorAuthor

    Just added a test for this in #8

  7. williamdes commented on Jul 9, 2022

    @williamdes
    williamdes
    on Jul 9, 2022
    CollaboratorAuthor

    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)

  8. williamdes commented on Jul 10, 2022

    @williamdes
    williamdes
    on Jul 10, 2022
    CollaboratorAuthor

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

    • if you look at other plugins
      • Most of them use config.inc.php.dist files
      • Have commented out the config lines in config.inc.php` files

    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"

  9. GermanCoding commented on Jul 11, 2022

    @GermanCoding
    GermanCoding
    on Jul 11, 2022
    Owner

    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.

  10. williamdes commented on Jul 12, 2022

    @williamdes
    williamdes
    on Jul 12, 2022
    CollaboratorAuthor

    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 #4 (comment)

  11. alecpl commented on Jul 13, 2022

    @alecpl
    alecpl
    on Jul 13, 2022

    .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.

  12. GermanCoding commented on Jul 13, 2022

    @GermanCoding
    GermanCoding
    on Jul 13, 2022
    Owner

    .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

    • the plugin's composer.json requires an old version of the plugin installer that doesn't have this feature
    • for plugins included in the dist, the installer is probably not run in the first place

    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?

  13. dilyanpalauzov commented on Dec 18, 2022

    @dilyanpalauzov
    dilyanpalauzov
    on Dec 18, 2022

    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).

  14. GermanCoding
    changed the title [-]Handle other format for TLS[/-] [+]Conflicting config.inc.php files due to installer autocopy[/+] on Dec 23, 2022

  15. GermanCoding commented on Dec 23, 2022

    @GermanCoding
    GermanCoding
    on Dec 23, 2022
    Owner

    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

  16. GermanCoding commented on Dec 27, 2022

    @GermanCoding
    GermanCoding
    on Dec 27, 2022
    Owner

    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.

  17. williamdes commented on Dec 28, 2022

    @williamdes
    williamdes
    on Dec 28, 2022
    CollaboratorAuthor

    Maybe you should ping upstream to know what is best?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions