fix: Enable DH parameters (ffdhe4096) by default #2192

polarathene · 2021-09-14T08:59:03Z

Description

Summary

DH params were only provided during setup-stack.sh.
We now just do the copy operation in Dockerfile to always have DH params file by default.
doveadm in newer versions of Dovecot (community/bullseye) seems to validate Dovecot configs and will fail if setup-stack.sh is not run first (eg when using setup.sh and no existing running container).
Some feature disparity, but this was never officially supported / documented, thus not a breaking change IMO.

Problem

setup-stack.sh currently copies the ffdhe4096.pem DH param file when none exists and no custom DH params file is provided by a user. This only applies when initializing a container instance via the default command however..

When you provide another command, such as the setup.sh management ones, and a container instance needs to be started instead of using an existing one, this will not run setup-stack.sh and thus default DH params are not guaranteed.

With our current Dovecot in buster-slim images, this has gone unnoticed as no error is raised. With the community edition of Dovecot that some users replace our shipped version with, or with the newer Dovecot in the bullseye-slim image we want to upgrade to, it seems that doveadm will validate the 10-ssl.conf config, which involves ssl_dh pointing to a file that doesn't exist. doveadm throws an error and refuses to run as a result, even though this file is unnecessary.

Solution

As the cause has been identified, this PR resolves the issue by copying the ffdhe4096.pem to Dovecot and Postfix locations during a build via the Dockerfile.

The DH param handling in setup-stack.sh has been heavily refactored to remove no longer relevant logic, and make it easier to maintain. Commit history of this PR has carefully iterated on this process with relevant commit messages for anyone interested in following the changes made in an easier to grok manner.

Likewise, the related tests were updated and I've opted to combine them all into a single new refactored DRY variant. As ONE_DIR handling was removed from the relevant setup-stack.sh function, the test could also drop testing both states if desired. There is minimal increase in test time from the ONE_DIR handling, so I've left it as-is for now.

Note: While this resolves a failure with bullseye-slim image, the TLS cipher suite test continues to fail on bullseye-slim for other reasons AFAIK (presently investigating).

I do not consider this a breaking change despite some of the changes. The original PR that added the feature added no documentation, and I did several searches through the git repo files to double check. If we don't advertise official support for providing custom DH params, this seems fine.

AFAIK most/all users would have just been using the default supplied DH params (ffdhe4096.pem). Custom DH params is still supported and that has only changed in removing the variant ONE_DIR=1 (mail-state/lib-shared/) directory. The ONE_DIR=0 approach (add custom file to /tmp/docker-mailserver/dhparams.pem) is now the the approach regardles of ONE_DIR setting.

I've not yet added documentation of the feature (eg providing custom params, what are the default params). If you would like that as well with this PR let me know.

Type of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change that does improve existing functionality)
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

This file only exists at the location to be copied to separate `postfix` and `dovecot` locations. `/etc/postfix/shared` seems like an inappropriate location to have been using.

These are effectively the same, did a diff comparison: - Identified minor inconsistencies and in `_notify` output. - Parameterized service names and paths. - Added some comments to better grok the flow. Next these two methods can be collapsed into a single one with input args to make it DRY.

This only needs to modify the `DH_CUSTOM` path var. More DRY, the conditional branch for `ONE_DIR=0` (else), is kept and shifts it's indentation one level to the left. I don't see any value in checking the alternative service for dh params file to copy over, so that's now dropped too.

Another conditional check is dropped and the default fallback message for existing DH params file is no longer relevant. Improved the remaining `_notify` messages. Collapsing the warning into a single logged message also seemed relevant. Custom provided DH params now use `cp -f` to overwrite the existing default `ffdhe4096.pem` used.

- Normalize whitespace (had mixed indentation of spaces and tabs, and mixed indentation width of 2 and 4 spaces). - Improved formatting of docker args. - (default test file) Swapped order of ONE_DIR bool definitions, and `PRIVATE_CONFIG` var duplicated with ONE_DIR suffix instead of recycling the var. - Updated the grep tests checking docker logs for warning message.

This feature was introduced by the PR: docker-mailserver#1463 There is no official documented support for custom DH parameters. As no guarantee is provided, this is considered an internal change, not a breaking one. There is no apparent need for special handling with `ONE_DIR=1`.

Now DRY and ready to merge the other two DH param tests into.

georglauterbach · 2021-09-14T09:09:55Z

I'd love to see a bit of documentation on this :) Other than that, LGTM 👍🏼

polarathene · 2021-09-14T09:13:17Z

I'd love to see a bit of documentation on this

Sure, I'll sort that out after I figure out the other bullseye failure with TLS tests.

polarathene · 2021-09-14T13:34:37Z

Test failure can be ignored, fixed by the v10.2 release setup.sh change.

Lint errors to address:

DH_DEFAULT_CHECKSUM="$(sha512sum ${DH_DEFAULT_PARAMS} | awk '{print $1}')" => "SC2086: Double quote to prevent globbing and word splitting."
DMS_ONE_DIR=0 => "SC2030: Modification of DMS_ONE_DIR is local (to subshell caused by @BATS test)."
-v "${PRIVATE_CONFIG}:/tmp/docker-mailserver" => "SC2031: PRIVATE_CONFIG was modified in a subshell. That change might be lost."

These 3 ShellCheck failures seem like they should be ignored?

1st one I have no clue how to fix since it's already wrapping the $() (subshell?) with double quotes.
The last two are due to how bats works AFAIK and non-issue.

target/scripts/startup/setup-stack.sh

test/mail_tls_dhparams.bats

georglauterbach · 2021-09-14T14:57:45Z

LGTM. I addressed the linting problems with comments above. Note that GitHub now has a "batch" feature with which you can commit multiple suggestions at once.

casperklein · 2021-09-14T17:49:56Z

Very nice simplification of our DH parameter stuff! I was not able to find anything to complain 😉 Just waiting till Georgs suggestion are applied, to approve this PR 👍

wernerfred

LGTM 👍🏻 well done

Co-authored-by: Georg Lauterbach <[email protected]>

polarathene

~~I'll need to apply the other lint corrections (same lint issues raised on similar lines). Then docs will follow.~~

Additionally updates the acme RFC URL to what it now redirects to.

github-actions · 2021-09-14T23:23:38Z

Documentation preview for this PR is ready! 🎉

Built with commit: bbedf5f

wernerfred

LGTM 🚀

The failing action is due to missing setup.sh changes right? https://github.com/docker-mailserver/docker-mailserver/pull/2192/checks?check_run_id=3604545578#step:8:331

polarathene · 2021-09-15T08:29:36Z

The failing action is due to missing setup.sh changes right?

Correct :)

Almost got another PR ready that is intended for 10.2 release. So that issue should be taken care of soon enough.

polarathene added 10 commits September 14, 2021 13:34

Move ffdhe4096.pem from /etc/postfix to /etc/dms

5c42b7f

This file only exists at the location to be copied to separate `postfix` and `dovecot` locations. `/etc/postfix/shared` seems like an inappropriate location to have been using.

Refactor postfix and dovecot dh methods to wrap shared dh logic

ba1a1a9

Refactor DH params default test

1f2ea17

Now DRY and ready to merge the other two DH param tests into.

Combine custom DH param tests into default test

229b7e9

Rename DH param test

491bf34

Appease the Linting gods

babc642

NorseGaud assigned polarathene Sep 14, 2021

NorseGaud added area/security kind/possible bug priority/high labels Sep 14, 2021

polarathene mentioned this pull request Sep 14, 2021

fix: Relax openssl security level for intermediate TLS_LEVEL #2193

Merged

6 tasks

georglauterbach reviewed Sep 14, 2021

View reviewed changes

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

georglauterbach reviewed Sep 14, 2021

View reviewed changes

test/mail_tls_dhparams.bats Outdated Show resolved Hide resolved

georglauterbach reviewed Sep 14, 2021

View reviewed changes

test/mail_tls_dhparams.bats Show resolved Hide resolved

wernerfred reviewed Sep 14, 2021

View reviewed changes

Apply suggestions from code review

898ddc2

Co-authored-by: Georg Lauterbach <[email protected]>

polarathene commented Sep 14, 2021

View reviewed changes

polarathene marked this pull request as draft September 14, 2021 21:34

polarathene added 2 commits September 15, 2021 09:44

Appease the linting gods once more

fa5323f

docs: Add instructions to use custom DH params

deb7f21

Additionally updates the acme RFC URL to what it now redirects to.

Praise be eclint and CI

ca49f9d

polarathene force-pushed the fix/enable-default-dhparams branch from 1742ccf to ca49f9d Compare September 14, 2021 23:20

polarathene marked this pull request as ready for review September 14, 2021 23:22

Merge branch 'master' into fix/enable-default-dhparams

bbedf5f

georglauterbach approved these changes Sep 15, 2021

View reviewed changes

wernerfred approved these changes Sep 15, 2021

View reviewed changes

polarathene merged commit 08cd4d3 into docker-mailserver:master Sep 15, 2021

polarathene mentioned this pull request Oct 19, 2021

man: describe how to apply config changes at runtime systemd/zram-generator#105

Merged

polarathene mentioned this pull request Jan 11, 2023

tests(refactor): Adjust mail_tls_dhparams.bats #2994

Merged

4 tasks

polarathene mentioned this pull request Aug 4, 2025

update: drop support for DHE params #4538

Open

4 tasks

Uh oh!

fix: Enable DH parameters (ffdhe4096) by default #2192

fix: Enable DH parameters (ffdhe4096) by default #2192

Uh oh!

Conversation

polarathene commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary

Problem

Solution

Type of change

Checklist:

Uh oh!

georglauterbach commented Sep 14, 2021

Uh oh!

polarathene commented Sep 14, 2021

Uh oh!

polarathene commented Sep 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented Sep 14, 2021

Uh oh!

casperklein commented Sep 14, 2021

Uh oh!

wernerfred left a comment

Choose a reason for hiding this comment

Uh oh!

polarathene left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 14, 2021

Uh oh!

wernerfred left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polarathene commented Sep 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polarathene commented Sep 14, 2021 •

edited

Loading

polarathene left a comment •

edited

Loading

wernerfred left a comment •

edited

Loading