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

polarathene · 2021-09-15T14:33:17Z

Description

This PR does some refactoring and fixes within the _setup_ssl method. Due to the amount of changes, it may be easier to review via individual commits (each has commit message detailing changes).This is easier through apps like GitKraken.

If not reviewing via commits, I strongly advise disabling white-space noise from the diff. Other noise near the end with some test files from the housekeeping commit

One notable commit is the removal of mkcert.sh usage from Dovecot. There was discussion to replace it with a smallstep binary, but presently I don't think there is a need for such, I've opted to just use the same dummy "snakeoil" certs that Postfix is configured to use.

This turned out being larger than expected. If desirable I could cherry pick out mkcert.sh removal. Should all be fine for v10.2.

dms_panic lacks tests. It looks like I implemented something similar to errex or _shutdown, both in helper-functions.sh? Is it worth keeping around or should I drop it?

Introducing `dms_panic`

I needed to better communicate failure, but wanted to be DRY, so I put together this utility for all scripts to leverage. During this review it was iterated on a bit.

An invalid SSL_TYPE or a valid value with invalid configuration will now panic, exiting the container and emitting a fatal error to the logs.

Internal cert storage path/config changed

Previously we've been using /etc/postfix/ssl for Postfix and Dovecot. This was noticed when working on mkcert.sh removal. Instead I've moved this location to /etc/dms/tls for both to share, since sharing the postfix location seemed undesirable.

This is internal change only, but may impact user-patches.sh scripts from users, especially one that is referenced in our TLS docs page, although that script is a bit out of date already. EDIT: A future PR after this one has since updated the docs to reflect that.

Change to default `SSL_TYPE` (partially affects tests)

Click to expand

I initially replaced '' (empty / unset) case for SSL_TYPE to be disabled value instead and panic if SSL_TYPE had not been set.. However this is a bit of a hassle with tests atm so I've reverted that case value while keeping the modified disabled behaviour. No one should be using such a value in production regardless.

For tests that do require SSL/TLS, it's only because of a test with openssl making a starttls connection. For that I've just added a snakeoil case that doesn't panic or adjust the SSL/TLS settings. In tests.bats, this should only be the IMAP related test but as most tests use the same container I've just added snakeoil to that container for all tests.

Both SSL_TYPE values differ from the original '' used for tests which would set ssl=yes, disable_plaintext_auth=no:

'' => ssl=no, disable_plaintext_auth=no, additionally disables for Postfix as well, previously only Dovecot was adjusted.
snakeoil => ssl=required, disable_plaintext_auth=yes (defaults)

As no certs are provided/configured for either SSL_TYPE, when SSL / TLS is available, it uses the "snakeoil" certs provided from upstream. These are self-signed / insecure but fine for testing, actual tests that care about such use more appropriate certs.

Changes related to tests

Click to expand

`tests.bats`

There were 2 remaining checking ssl: tests in this file, they have been dropped. These are from 2016, one is already covered in my mail_ssl_manual.bats test, while the other checks a lets-encrypt CA cert that was issued March 2016, but has expired since March 2021. This was required back in 2016 when LetsEncrypt was new and needing broader adoption of compatible CA certs in OS trust stores IIRC.

$ step-cli certificate inspect lets-encrypt-x3-cross-signed.pem

...

        Issuer: O=Digital Signature Trust Co.,CN=DST Root CA X3
        Validity
            Not Before: Mar 17 16:40:46 2016 UTC
            Not After : Mar 17 16:40:46 2021 UTC
        Subject: C=US,O=Let's Encrypt,CN=Let's Encrypt Authority X3

...

git blame:

Docker MailServer V2 #109 ([relevant direct commit referencing issue])(Docker MailServer V2 #109)
Improved start-mailserver output #420

nmap tests have been dropped. These were added about 4-5 years ago via these two PRs:

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.

mail_with_ldap.bats - Migrated --link docker run option to use --network + --network-alias. --link is deprecated, future breakage is expected. No other tests use --link.

mail_privacy.bats - Removed irrelevant lines from. When this test was extracted from tests.bats, it copied over some excess comment headings. The test itself looks a bit vague too.

Type of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

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

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.

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

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

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

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

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.

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.

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.

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.

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.

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

georglauterbach

LGTM

target/scripts/helper-functions.sh

This could later be better formatted and placed into contributor docs.

casperklein · 2021-09-17T21:43:15Z

I finished reviewing. That seems to be a lot of work, especially the detailed description of each commit - awesome! Also congratulation to your sed wizard achievement 😄 I didn't know about -e too. I used -e alot with grep, but not with sed so far 👍

All in all it LGTM. Just the panic/shutdown function should be merged IMO, as they both try to achieve the same: shutting down the container.

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

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.

target/scripts/startup/setup-stack.sh

The two previous `_shutdown` methods can benefit from using `dms_panic` wrapper instead to standardize on panic messages.

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

georglauterbach · 2021-09-19T10:20:13Z

LGTM. We will wait for #2189 to be merged so we can rebase and see what the tests do.

georglauterbach

LGTM 👍🏼

target/scripts/helper-functions.sh

* completely refactored `start-mailserver.sh` * added braces; correctly formatted tabs / spaces * included `start-mailserver` into shellcheck checks * cleanup * removed unnecessary shellcheck comments adding braces and "" where necessary * corrected some mistakes in CONTRIBUTING * Makefile now uses correct shellcheck

casperklein added this to the v10.2.0 milestone Sep 15, 2021

casperklein assigned polarathene Sep 15, 2021

polarathene added 12 commits September 16, 2021 14:38

Refactor SSL_TYPE=disabled

174c650

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

chore: Improve inline documentation

326fa48

tests(fix): manual ssl

b37bddf

tests(chore): normalize white-space

8fa7d2c

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.

Remove the expired lets-encrypt cert

2584d95

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.

Default SSL_TYPE becomes disabled

bd75d10

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 force-pushed the fix/remove-mkcert-usage branch from 6c2abb9 to bd75d10 Compare September 16, 2021 11:55

tests(fix): imap starttls + relocate comment

c7fae6e

`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 marked this pull request as ready for review September 16, 2021 14:40

polarathene requested a review from georglauterbach September 16, 2021 14:40

georglauterbach previously approved these changes Sep 16, 2021

View reviewed changes

target/scripts/helper-functions.sh Outdated Show resolved Hide resolved

target/scripts/helper-functions.sh Show resolved Hide resolved

polarathene mentioned this pull request Sep 17, 2021

fix: Don't needlessly invalidate cache layers #2197

Merged

4 tasks

Add inline documentation for dms_panic

48aac7a

This could later be better formatted and placed into contributor docs.

polarathene dismissed georglauterbach’s stale review via 48aac7a September 17, 2021 13:42

Panic with kill (shutdown) not exit (errex)

6b7b019

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

This comment has been minimized.

Sign in to view

Obsolete _shutdown_with_message

fb74332

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.

casperklein reviewed Sep 17, 2021

View reviewed changes

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

Add misconfigured panic type + adopt dms_panic elsewhere

ea3e3e0

The two previous `_shutdown` methods can benefit from using `dms_panic` wrapper instead to standardize on panic messages.

casperklein approved these changes Sep 19, 2021

View reviewed changes

polarathene requested a review from georglauterbach September 19, 2021 03:14

georglauterbach approved these changes Sep 19, 2021

View reviewed changes

target/scripts/helper-functions.sh Show resolved Hide resolved

georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/medium pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged labels Sep 19, 2021

This comment has been minimized.

Sign in to view

polarathene merged commit c851f5b into docker-mailserver:master Sep 19, 2021

georglauterbach removed the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Sep 19, 2021

casperklein mentioned this pull request Sep 19, 2021

function _defunc removed #2199

Merged

11 tasks

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

polarathene commented Sep 15, 2021 •

edited

georglauterbach left a comment

casperklein commented Sep 17, 2021

This comment has been minimized.

This comment has been minimized.

georglauterbach commented Sep 19, 2021

georglauterbach left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

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

Conversation

polarathene commented Sep 15, 2021 • edited

Description

Introducing dms_panic

Internal cert storage path/config changed

Change to default SSL_TYPE (partially affects tests)

Changes related to tests

tests.bats

Type of change

Checklist:

georglauterbach left a comment

Choose a reason for hiding this comment

casperklein commented Sep 17, 2021

This comment has been minimized.

This comment has been minimized.

georglauterbach commented Sep 19, 2021

georglauterbach left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

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

polarathene commented Sep 15, 2021 •

edited

Introducing `dms_panic`

Change to default `SSL_TYPE` (partially affects tests)

`tests.bats`