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

fix: Remove mkcert.sh usage + _setup_ssl refactor. #2196

Merged

Commits on Sep 15, 2021

  1. chore(refactor): DRY up the _setup_ssl method

    `/etc/postfix/ssl` was a bit misleading in usage here. As a maintainer (of my own contribution!) I was confused why only `/etc/postfix/ssl` was referenced and not `/etc/dovecot/ssl`.
    
    The postfix specific path is unnecessary, dovecot was referencing it via it's config, the same can be done from postfix to a generic DMS specific config location instead.
    
    This location is defined and created early as `/etc/dms/tls` (with var `DMS_TLS_PATH`). All usage of `/etc/postfix/ssl` has been replaced, making it easier to grok. Several `mkdir` commands related to this have been dropped as a result.
    
    Likewise, a related `TMP_DMS_TLS_PATH` var provides a reference to the config volume path `/tmp/docker-mailserver` which is used for conditions on presently hard-coded paths.
    
    Other values that benefit from being DRY have been lifted up into vars. Definitely easier to follow now and makes some further opportunities clearer to tackle in a future refactor.
    
    I've not touched LetsEncrypt section as I don't have time to investigate into that yet (not familiar with that portion).
    
    ---
    
    `chmod` has been updated where appropriate. Public key/cert is acceptable to have as readable by non-root users (644). The custom type with single fullchain file was not root accessible only, but should as it contains a private key.
    
    That said, the security benefit can be a bit moot due to source files that were copied remain present, the user would be responsible to ensure similar permissions on their source files.
    polarathene committed Sep 15, 2021
    Configuration menu
    Copy the full SHA
    b800ad4 View commit details
    Browse the repository at this point in the history

Commits on Sep 16, 2021

  1. chore: Remove mkcert logic and dovecot cert

    No longer serving a purpose.
    
    Our own TLS startup script handles a variety of cert scenarios, while the dropped code was always generating a self-signed cert and persisting an unused cert regardless with `ONE_DIR=1`.
    
    To avoid similar issues that DH params had with doveadm validating filepath values in the SSL config, the default dummy values match postfix pointing to "snakeoil" cert. That serves the same purpose as mkcert was covering in the image.
    
    Bonus, no more hassle with differing mkcert target paths for users replacing our supplied Dovecot with the latest community edition.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    5c1012b View commit details
    Browse the repository at this point in the history
  2. Error handling for TLS_LEVEL

    - Added a panic utility to exit early when TLS_LEVEL conditions are misconfigured.
    - Some info text had order of key/cert occurrence swapped to be consistent with key then cert.
    - Some existing comments moved and rephrased.
    - Additional comments added.
    - `-f` test for cert files instead of `-e` (true also for directories/devices/symlinks).
    - _notify messages lifted out of conditionals so that they always output when the case is hit.
    - Empty TLS_LEVEL collapsed into catch all panic, while it's contents is now mapped to a new 'disabled' value.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    2422b0e View commit details
    Browse the repository at this point in the history
  3. Use sedfile + improve sed expressions + update case style

    - Uses sedfile when appropriate (file change intentional, not optional match/check).
    - sed expressions modified to be DRY and reduce escaping via `-r` flag (acceptable if actual text content contains no `?`,`+`,`()` or `{}` characters, [otherwise they must be escaped](https://www.gnu.org/software/sed/manual/html_node/Extended-regexps.html)).
    - sed captures anything matched between the parenthesis`()` and inserts it via `\1` as part of the replacement.
    - case statements adopt the `(` prefix, adopting recent shell style for consistency.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    95ae6dd View commit details
    Browse the repository at this point in the history
  4. Refactor SSL_TYPE=disabled

    Postfix is also disabled now.
    
    Included heavy inline documentation reference for maintainers.
    
    Dropped an obsolete postfix config option 'use_tls' on the relayhost function, it was replaced by 'security_level'.
    fix dms_panic
    
    
    add comment
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    174c650 View commit details
    Browse the repository at this point in the history
  5. I'm a friggin' sed wizard now

    - The `modern` TLS_LEVEL is the default values for the configs they modify. As such, `sedfile` outputs an "Error" which isn't an actual concern, back to regular `sed`.
    
    - I realized that multiple edits for the same file can all be done at once via `-e` (assuming other sed options are the same for each operation), and that `g` suffix is global scope for single line match, not whole file (default as sed iterates through individual lines).
    
    - Some postfix replacements have `smtp` and `smtpd` lines, collapsed into a single `smtpd?` instead now that I know sed better.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    f3489f6 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    326fa48 View commit details
    Browse the repository at this point in the history
  7. tests(fix): manual ssl

    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    b37bddf View commit details
    Browse the repository at this point in the history
  8. tests(chore): normalize white-space

    Normalizing on white-space indentation and removing tabbed indentation.
    
    Toggle white-space ignore on the diff for easy verification. Taking care of this before updating these tests to pass.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    8fa7d2c View commit details
    Browse the repository at this point in the history
  9. tests(fix): Tests that require SSL/TLS to pass

    SSL_TYPE=snakeoil added as temporary workaround.
    
    ---
    
    nmap tests are being dropped. These were added about 4-5 years ago, I have since made these redundant with the `testssl.sh` tests.
    
    Additionally the `--link` option is deprecated and IIRC these grades were a bit misleading when I initially used nmap in my own TLS cipher suite update PRs in the past.
    
    The removed SSL test is already handled in mail_ssl_manual.bats
    
    ---
    
    ldap test replaces `--link` alias option with `--network` and alias assignment.
    
    Parameterized some values and added the `SSL_TYPE` to resolve the starttls test failure.
    
    ---
    
    privacy test also needed `SSL_TYPE` to pass the starttls test.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    66739b3 View commit details
    Browse the repository at this point in the history
  10. Remove the expired lets-encrypt cert

    This expired in March 2021. It was originally required when first added back in 2016 as LetsEncrypt was fairly new and not as broadly accepted into OS trust stores.
    
    No longer the case today.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    2584d95 View commit details
    Browse the repository at this point in the history
  11. chore: Housekeeping

    Not required for this PR branch, little bit of tidying up while working on these two test files.
    
    privacy test copied over content when extracted from `tests.bats` that isn't relevant.
    
    ldap test was not as easy to identify the source of DOVECOT_TLS. Added comment to make the prefix connection to `configomat.sh` and `.ext` files more easier to find.
    
    Additionally converted the two localhost FQDN to vars.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    828d635 View commit details
    Browse the repository at this point in the history
  12. Default SSL_TYPE becomes disabled

    This is to prevent other tests from failing by hitting the panic catchall case.
    
    More ideal would be adjusting tests to default to `disabled`, rather than treating `disabled` as an empty / unset SSL_TYPE value.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    bd75d10 View commit details
    Browse the repository at this point in the history
  13. tests(fix): imap starttls + relocate comment

    `tests.bats` had another starttls test for imap.
    
    Workaround for now is to give the main test container `SSL_TYPE=snakeoil`.
    
    In the ldap test my comment broke the command naturally. I forget that it only works like that in Dockerfile.
    polarathene committed Sep 16, 2021
    Configuration menu
    Copy the full SHA
    c7fae6e View commit details
    Browse the repository at this point in the history

Commits on Sep 17, 2021

  1. Add inline documentation for dms_panic

    This could later be better formatted and placed into contributor docs.
    polarathene committed Sep 17, 2021
    Configuration menu
    Copy the full SHA
    48aac7a View commit details
    Browse the repository at this point in the history
  2. Panic with kill (shutdown) not exit (errex)

    - `kill 1` from `_shutdown` will send SIGTERM signal to PID 1 (init process).
    - `exit 1` within the `start-mailserver.sh` init scripts context, will just exit the initialization script leaving the container running when it shouldn't.
    polarathene committed Sep 17, 2021
    Configuration menu
    Copy the full SHA
    6b7b019 View commit details
    Browse the repository at this point in the history
  3. Obsolete _shutdown_with_message

    As per review feedback, delegate to `_shutdown` instead by moving the fatal`_notify` and accepting an arg to provide shutdown context within `_shutdown`, which is now expected to only be called if something considered fatal has occurred.
    polarathene committed Sep 17, 2021
    Configuration menu
    Copy the full SHA
    fb74332 View commit details
    Browse the repository at this point in the history

Commits on Sep 18, 2021

  1. Add misconfigured panic type + adopt dms_panic elsewhere

    The two previous `_shutdown` methods can benefit from using `dms_panic` wrapper instead to standardize on panic messages.
    polarathene committed Sep 18, 2021
    Configuration menu
    Copy the full SHA
    ea3e3e0 View commit details
    Browse the repository at this point in the history
  2. Refactor dms_panic

    - Arg order was changed to move the scope (optional) to the last arg.
    - Rather than copy/paste PANIC_ vars as an arg, encourage wrapper methods instead that implicitly provide the PANIC_TYPE arg.
    - PANIC_TYPE variant vars no longer required, switched to direct strings.
    - Added inline comment about expected PANIC_INFO value/format for each panic type case.
    - Revised doc comments.
    polarathene committed Sep 18, 2021
    Configuration menu
    Copy the full SHA
    44793f3 View commit details
    Browse the repository at this point in the history