* chore: Only replace `CHKSUM_FILE` when a change has been processed
* chore: Change Detection service should be the last daemon started
* chore: Remove 10 second startup delay for change detector
There should be no concern with conflicts as any writes should have already been done by the time this daemon service is started.
* tests(fix): `smtp_delivery.bats` must wait for Amavis
The change event for adding a user can be processed much sooner now, which means Amavis may not yet be ready.
Added extra condition to wait on at least the Amavis port being reachable, and some failure asserts with the mail queue to better catch / debug when this problem occurs.
* chore: Add some minor delay to avoid Amavis failing to connect
* tests(refactor): Make test cases for opendkim keysizes DRY
- These all do roughly the same logic that can be split into two separate methods.
- `_should_generate_dkim_key()` covers a bit more logic as it can be leveraged to handle other test cases that also perform the same logic.
- The `config/opendkim/` doesn't seem necessary for tests. Only the first few test cases here are testing against it, so we can conditionally make that available. `process_check_restart.bats` also depended on it to run OpenDKIM successfully, but this was due to the `setup-stack.sh` config defaults failing to find an "empty" file forcing `supervisord` to constantly restart the process..
- With this, there we inverse the default opendkim config, so we don't have to mount unique / empty subfolders for each test case, followed by copying over the two extra configs.
* tests(refactor): DRY up more test cases
All the remaining test cases but the last one were refactored here for a clean commit diff. The last test case will be refactored in the following commit.
Plenty of repeated logic spread across these test cases, now condensed into shared methods.
* tests(refactor): Make final test case DRY
* chore: Migrate to new testing helpers
* chore: Revise test case descriptions
* tests(refactor): Improve and simplify assertions
* tests(refactor): Use common container setup instead of `docker run`
- As the majority of test cases are only running `open-dkim` helper, we don't actually have to wait for a full container setup. So an alternative container start is called.
- Also improves assertions a bit more instead of just counting lines.
- Some test cases don't bind mount all of `/tmp/docker-mailserver` contents, thus don't raise permission errors on subsequent test runs.
- Instead of `rm -f` on some config files, have opted to mount them read-only instead, or alternatively mount an anonymous empty volume instead.
- Collapsed the first three test cases into one, thus no `setup_file()` necessary.
- Shift the `_wait_for_finished_setup_in_container()` method into `_common_container_setup()` instead since nothing else is using `_common_container_start()` yet, this allows for avoiding the wait.
* tests(refactor): Collapse dkim key size test cases into single test case
This makes these tests a bit more DRY, and enhances the raised quality issue with these tests. Now not only is the domain checked in the generated DNS dkim record, but we also verify the key size is corrected in the public and private keys via openssl.
* chore: Revise container names
* chore: Swap order of test case 1 and 2
* tests(refactor): Assert generated log output
- `__should_have_tables_trustedhosts_for_domain` shifted in each test case to just after generating the domains keys.
- Asserts `open-dkim` logs instead of just counting them.
- Added checks for domains that should not be present in a test case.
- Additional coverage and notes about the alias from vhost `@localdomain.com`
- Single assert statement with switch statement as all are using common args.
* chore: Minor changes
* tests(refactor): Share `find` logic in helpers and tests
* tests(fix): Listing file content does not need to match line order
The order printed from local system vs CI differed causing the CI to fail. The order of lines is irrelevant so `--index` is not required.
Additionally correct the prefix of the called method to be only one `_` now that it's a `common.bash` helper method.
* chore: Collapse custom DKIM selector test into custom DKIM domain test
These cover the same test logic for the most part, the first domain could also be testing the custom selector.
`special_use_folders.bats` + `mailbox_format_dbox` can assert lines instead, removing the need for `--partial`.
* Apply suggestions from code review
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* chore: Split switch statement method into wrapper methods
---------
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* only add Amavis configuration to Postfix when enabled
Since I am running Rspamd nowadays, I noticed there still are ports open
that belong to Amavis. This is because the Amavis configuration is a
fixed part of Postfix's `master.cf`. I changed that. Now, the Amavis
section is added when Amavis really is enabled.
I took the chance and added proper indentation to `master.cf`; hence the
diff is a bit fuzzy. **But**, only the Amavis part was adjusted, the
rest is just styling.
This appears to have been added to replace the `fam` package in an early version of DMS with Courier for IMAP instead of Dovecot on an Ubuntu 14.04 base image.
It does not appear to serve a purpose anymore.
* chore: Remove the wrapper script for `fail2ban`
- This does not appear necessary. The server can be run with foreground mode.
- `daemons-stack.sh` removal of the socket can be handled by the fail2ban server when using the `-x` option.
* chore: Remove `touch /var/log/auth.log`
These were both added as supposed fixes in 2016 for the then Ubuntu 2014 base image.
Removing them causes no failures in tests.
* fix: Install optional python packages for `fail2ban`
These have barely any overhead in layer weight. The DNS package may provide some QoL improvements, while the `pyinotify` is a better alternative than polling logs to check for updates.
We have `gamin` package installed but `fail2ban` would complain in the log that it was not able to initialize the module for it. There only appears to be a `python-gamin` dependent on EOL python 2, no longer available from Debian Bullseye.
* added options to toggle OpenDKIM & OpenDMARC
rspamd can provide DKIM signing and DMARC checking itself, so users
should be able to disable OpenDKIM & OpenDMARC. The default is left at
1, so users have to to opt-in when the want to disable the features.
* misc small enhancements
* adjusted start of rspamd
The order of starting redis + rspamd was reversed (now correct) and
rspamd now starts with the correct user.
* adjusted rspamd core configuration
The main configuration was revised. This includes AV configuration as
well as worker/proxy/controller configuration used to control the main
rspamd processes.
The configuration is not tested extensively, but well enough that I am
confident to go forward with it until we declare rspamd support as
stable.
* update & improve the documentation
* add tests
These are some initial tests which test the most basic functionality.
* tests(refactor): Improve consistency and documentation for test helpers (#3012)
* added `ALWAYS_RUN` target `Makefile` recipies (#3013)
This ensures the recipies are always run.
Co-authored-by: georglauterbach <44545919+georglauterbach@users.noreply.github.com>
* adjusted rspamd test to refactored test helper functions
* improve documentation
* apply suggestions from code review (no. 1 by @polarthene)
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* streamline heredoc (EOM -> EOF)
* adjust rspamd test (remove unnecessary run arguments)
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* fix: RSPAM ENV should only add to array if ENV enabled
* fix: Correctly match ownership for Postfix content
- `/var/lib/postfix` dir and content is `postfix:postfix`, not `postfix:root`.
- `/var/spool/postfix` is `root:root` not `postfix:root` like it's content.
- Add additional comments, including ownership changes by Postfix to `/var/spool/postfix` when process starts / restarts.
* fix: Ensure correct `chown -R` user and groups applied
These were all fine except for clamav not using the correct clamav group. `fetchmail` group is `nogroup` as per the group set by the debian package.
Additionally formatted the `-eq 1 ]]` content to align on the same columns, and added additional comment about the purpose of this `chown -R` usage so that it's clear what bug / breakage it's attempting to prevent / fix.
* refactor: `misc-stack.sh` conditional handling
The last condition doesn't get triggered at all AFAIK. Nor does it make sense to make a folder path with `mkdir -p` to symlink to when the container does not have anything to copy over?
- If that was for files, the `mkdir -p` approach seems invalid?
- If it was for a directory that could come up later, it should instead be created in advance? None of the current values for `FILES` seem to hit this path.
Removing as it doesn't seem relevant to current support.
Symlinking was done for each case, I've opted to just perform that after the conditional instead.
Additional inline docs added for additional context.
* chore: Move amavis `chown -R` fix into `misc-stack.sh`
This was handled separately for some reason. It belongs with the other services handling this fix in `misc-stack.sh`.
The `-h` option isn't relevant, when paired with `-R` it has no effect.
* fix: Dockerfile should preserve `clamav` ownership with `COPY --link`
The UID and GID were copied over but would not match `clamav` user and group due to numeric ID mismatch between containers. `--chown=clamav` fixes that.
* chore: Workaround `buildx` bug with separate `chown -R`
Avoids increasing the image weight from this change by leveraging `COPY` in the final stage.
* chore: `COPY --link` from a separate stage instead of relying on scratch
The `scratch` approach wasn't great. A single layer invalidation in the previous stage would result in a new 600MB layer to store.
`make build` with this change seems to barely be affected by such if a change came before copying over the linked stage, although with `buildx` and the `docker-container` driver with `--load` it would take much longer to import and seemed to keep adding storage. Possibly because I was testing with a minimal `buildx` command, that wasn't leveraging proper cache options?
* lint: Appease the linting gods
* chore: Align `misc-stack.sh` paths for `chown -R` operations
Review feedback
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* fix: Reduce one extra cache layer copy
No apparent advantage of a `COPY --link` initially in separate stage.
Just `COPY --chown` in the separate stage and `COPY --link` the stage content. 230MB less in build cache used.
* fix: Remove separate ClamAV stage by adding `clamav` user explicitly
Creating the user before the package is installed allows to ensure a fixed numeric ID that we can provide to `--chown` that is compatible with `--link`.
This keeps the build cache minimal for CI, without being anymore complex as a workaround than the separate stage was for the most part.
* chore: Add reference link regarding users to `misc-stack.sh`
* tests(refactor): `mail_changedetector.bats` - Leverage DRY methods
`supervisorctl tail` is not the most reliably way to get logs for the latest change detection and has been known to be fragile in the past.
We can instead read the full log for the service directly with `tac` and `sed` to extract all log content since the last change detection.
Common asserts have also been extracted out into separate methods.
* tests(chore): Remove sleep and redundant change event
Container 1 is still blocked at this point from an existing lock and change event.
Make the lock stale immediately and no extra sleep is required when paired with the helper method to wait until the event is processed (which should remove the stale lock).
* tests(refactor): Add more DRY methods
- Simplify the test case so it's easier to grok.
- 2nd test case (blocking) extracts out initial setup into a separate method and merges the later service restart logic which is redundant.
- Additional comments for improved context of what is going on / expected.
* tests(chore): Revise the change detection helper method
- Add explicit counting arg to change detection support.
- Extract revised logic into it's own generic helper method.
- Utilize this for a separate method that monitors for a change event having started, but not waiting for completion.
This allows dropping the 40 sec of remaining `sleep` in `mail_changedetector` test. It was also required due to potentially missing the timing of a change event completing concurrently in a 2nd container that needed to be waited on and then checked.
* tests(chore): Migrate to current test conventions
- Switch to common container setup helpers
- Update container name and change usage to variables instead.
- Adopt the new convention of prefix variable for test cases (revised test case descriptions).
* tests(chore): Remove legacy change detection
This has since been replaced with the new helper watches the `changedetector` service logs directly instead of only detecting a change has occurred via checksum comparison.
No tests use this method anymore, it was originally for `tests.bats`. Thus the tests in `test_helper.bats` are being dropped too. The new helper has test coverage in `changedetector` tests.
* chore: Lock removal should not incur `sleep 5` afterwards
- A new lock should be created by this script after removal. The sleep doesn't help avoid a race condition with lock file creation after removal.
- Reduces test time as a bonus.
- Added some additional comments to test.
* tests(chore): `tls_letsencrypt.bats` leverage improved change detection
- No need to wait on the change detection service anymore during container startup.
- No need to count change events processed either as waiting a fixed duration is no longer relied on.
- This makes the reload count method redundant, dropped.
* tests(chore): Convert `setup-cli.bats` to new test conventions
This test file was already adapted to the original common setup helpers.
- `TEST_NAME` replaced with `CONTAINER_NAME`.
- Prefix var added, test case descriptions drop explicit prefix.
- No other changes.
* tests(chore): Extract out helpers related to change-detection
- New helper file for sharing these helpers to tests.
- Includes the helpful log method from changedetector tests.
- No longer need to maintain duplicate copies of these methods during the test migration. All tests that use them are now importing the separate helper file.
- `tls_letsencrypt.bats` has switched to using the log helper.
- Generic log count helper is removed from `test_helper/common.bash` as any test that needs it in future can adapt to `helper/common.bash`.
* tests(refactor): `tls_letsencrypt.bats` remove `_get_service_logs()`
This helper does not seem useful as moving away from `supervisorctl tail` and no other tests had a need for it.
* tests(chore): Remove common setup methods from `test_helper/common.bash`
No other tests depend on this. Future tests will adopt the revised versions from `helper/setup.bash`.
Additionally updates `helper/setup.bash` comments that are no longer applicable to `TEST_TMP_CONFIG` and `CONTAINER_NAME`.
* chore: Use `|| true` to simplify setting `EXPECTED_COUNT` correctly
* chore: Drop ENV `ENABLE_POSTFIX_VIRTUAL_TRANSPORT`
* tests(chore): Remove redundant `dovecot-lmtp` config
None of this is needed. Only relevant change is changing the LMTP service listener for Dovecot and that can be delegated to `user-patches.sh`.
* tests(refactor): Use `user-patches.sh` instead of replacing config file
The only relevant changes in `test/config/dovecot-lmtp` regarding LMTP was:
- `/etc/dovecot/dovecot.conf` (`protocols = imap lmtp`) and `/etc/dovecot/protocols.d/` (`protocols = $protocols lmtp`).
- `conf.d/10-master.conf` only changed the LMTP service listener from a unix socket to TCP on port 24 (_this was the only change required for the test to pass_).
None of those configs are required as:
- `protocols = imap pop3 lmtp` [is the upstream default](https://doc.dovecot.org/settings/core/#core_setting-protocols), no need to add `lmtp`.
- The LMTP service listener is now configured for the test with `user-patches.sh`.
* tests(refactor): `mail_lmtp_ip.bats`
- Converted to new testing conventions and common container helpers.
- `ENABLE_POSTFIX_VIRTUAL_TRANSPORT` was not relevant, dropped.
- Revised test cases, logic remains the same.
- Large custom config used was not documented and doesn't appear to serve any purpose. Simplified by replacing with a single modification with `user-patches.sh`.
- Added some additional comments for context of test and improvements that could be made.
* tests(chore): Adjust comments
The comment from `mail_hostname` provides no valid context, it was likely copied over from `tests.bats` in Oct 2020 by accident.
The email sent is just for testing, nothing relevant to LMTP.
---
Added additional comment for test to reference extra information from.
* tests(chore): Update similar log line matching
Extracts out the match pattern and formatting commands into separate vars (reduces horizontal scrolling), and includes extra docs about what the matched line should be expected to look like.
* fix: Workaround `postconf` write settle logic
After updating `main.cf`, to avoid an enforced delay from reading the config by postfix tools, we can ensure the modified time is at least 2 seconds in the past as a workaround. This should be ok with our usage AFAIK.
Shaves off 2+ seconds roughly off each container startup, reduces roughly 2+ minutes off tests.
* chore: Only modify `mtime` if less than 2 seconds ago
- Slight improvement by avoiding unnecessary writes with a conditional check on the util method.
- Can more comfortably call this during `postfix reload` in the change detection cycle now.
- Identified other tests that'd benefit from this, created a helper method to call instead of copy/paste.
- The `setup email restrict` command also did a modification and reload. Added util method here too.
* tests(fix): `mail_smtponly.bats` should wait for Postfix
- `postfix reload` fails if the service is not ready yet.
- `service postfix reload` and `/etc/init.d/postfix reload` presumably wait until it is ready? (as these work regardless)
* chore: Review feedback - Move reload method into utilities
* chore: Set `TLS_INTERMEDIATE_SUITE` to only use TLS 1.2 ciphersuites
Removes support of the following cipher suites that are only valid for TLS 1.0 + 1.1:
- `ECDHE-ECDSA-AES128-SHA`
- `ECDHE-RSA-AES128-SHA`
- `ECDHE-ECDSA-AES256-SHA`
- `ECDHE-RSA-AES256-SHA`
- `DHE-RSA-AES128-SHA`
- `DHE-RSA-AES256-SHA`
* chore: Update TLS version min and ignore settings
These are now the same as modern settings.
* fix: Remove min TLS support workaround
No longer required now that outdated TLS versions have been dropped.
* tests: Remove support for TLS 1.0 and 1.1 ciphersuites
* tests: Remove support for TLS 1.0 and 1.1 ciphersuites (Port 25)
The removed SHA1 cipher suites are still supported in TLS 1.2, thus they've been excluded for port 25 via the `SHA1` exclusion pattern in `main.cf`.
With `reload` a change detection event during local testing can be processed in less than a second according to logs. Previously this was 5+ seconds (_plus additional downtime for Postfix/Dovecot to become available again_).
In the past it was apparently an issue to use `<service> reload` due to a concern with the PID for wrapper scripts that `supervisorctl` managed, thus `supervisorctl <service> restart` had been used. Past discussions with maintainers suggest this is not likely an issue anymore, and `reload` should be fine to switch to now 👍
---
**NOTE:** It may not be an issue in the CI, but on _**local systems running tests may risk failure in `setup-cli.bats` from a false positive**_ due to 1 second polling window of the test helper method, and a change event being possible to occur entirely between the two checks undetected by the current approach.
If this is a problem, we may need to think of a better way to catch the change. The `letsencrypt` test counts how many change events are expected to have been processed, and this could technically be leveraged by the test helper too.
---
**NOTE:** These two lines (_with regex pattern for postfix_) are output in the terminal when using the services respective `reload` commands:
```
postfix/master.*: reload -- version .*, configuration /etc/postfix
dovecot: master: Warning: SIGHUP received - reloading configuration
```
I wasn't sure how to match them as they did not appear in the `changedetector` log (_**EDIT:** they appear in the main log output, eg `docker logs <container name>`_).
Instead I've just monitored the `changedetector` log messages, which should be ok for logic that previously needed to ensure Dovecot / Postfix was back up after the `restart` was issued.
---
Commit history:
* chore: Change events `reload` Dovecot and Postfix instead of `restart`
Reloading is faster than restarting the processes.
Restarting is a bit heavy handed here and may no longer be necessary for general usage?
* tests: Adapt tests to support service `reload` instead of `restart`
* chore: Additional logging for debugging change event logs
* fix: Wait on change detection, then verify directory created
Change detection is too fast now (0-1 seconds vs 5+).
Directory being waited on here was created near the end of a change event, reducing that time to detect a change by the utility method further.
We can instead check that the directory exists after the change detection event is completed.
* chore: Keep using the maildir polling check
We don't presently use remote storage in tests, but it might be relevant in future when testing NFS.
This at least avoids any confusing failure happening when that scenario is tested.
As per deprecation notice from v11.3 release notes, and a related prior PR; this ENV is to be removed.
It's no longer considered useful, and none of the tests that configured it were actually using it for relaying anything.
This new script is a clean way of handling the installation of packages.
I think the huge `RUN` command in `Dockerfile` was hard to read and
maintain.
Using a script is a non-issue, as the image is rebuilt whenever the
script is touched.
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
This PR prepares for other PRs that use the newly introduced helper
functions. The `_log` function itself was adjusted a bit to be shorter
and more concise.
* outsourcing env variable setup
This commit contains major parts of the work of refactoring the setup
and usage of environment variables. It outsources the setup into its own
script and provides dedicated functions to be executed at a later point in time.
A **new** env variable was added: `USER_PROVISIONG` which provides a
better way of defining which method / protocol to use when it comes to
setting up users. This way, the `ENABLE_LDAP` variable is deprecated,
but all of this is backwards compatible due to a "compatibility layer", a function provided by the new variables script.
This is not a breaking change. It mostly refators internal scripts. The
only change facing the user-side is the deprecation of `ENABLE_LDAP`. We
can prolong the period of deprecation for this variable as long as we
want, because the new function that ensures backwards compatibility
provides a clean interface for the future.
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
When waiting on an account to be added to `postfix-accounts.cf`, Dovecot account creation during the startup process had already run.
Startup continued without properly creating the mail account for Dovecot. Methods like `setup email list` (with `ENABLE_QUOTAS=1`) would fail. `changedetector` service was required to be triggered to re-create Dovecot users.
- Wrapped the logic for wait + shutdown into a function call.
- Moved `_create_accounts()` to bottom of the setup function.
See the associated PR for more detailed commentary on specific changes.
### Commands refactored:
- User (**All:** add / list / update / del + _dovecot-master variants_)
- Quota (**All:** set / del)
- Virtual Alias (**All:** add / list /del)
- Relay (**All:** add-relayhost / add-sasl / exclude-domain)
### Overall changes involve:
- **Fairly common structure:**
- `_main` method at the top provides an overview of logical steps:
- After all methods are declared beneath it (_and imported from the new `helpers/database/db.sh`_), the `_main` is called at the bottom of the file.
- `delmailuser` additionally processes option support for `-y` prior to calling `_main`.
- `__usage` is now consistent with each of these commands, along with the `help` command.
- Most logic delegated to new helper scripts. Some duplicate content remains on the basis that it's low-risk to maintenance and avoids less hassle to jump between files to check a single line, usually this is arg validation.
- Error handling should be more consistent, along with var names (_no more `USER`/`EMAIL`/`FULL_EMAIL` to refer to the same expected value_).
- **Three new management scripts** (in `helpers/database/manage/`) using a common structure for managing changes to their respective "Database" config file.
- `postfix-accounts.sh` unified not only add and update commands, but also all the dovecot-master versions, a single password call for all 4 of them, with a 5th consumer of the password prompt from the relay command `addsaslpassword`.
- These scripts delegate actual writes to `helpers/database/db.sh` which provides a common API to support the changes made.
- This is more verbose/complex vs the current inline operations each command currently has, as it provides generic support instead of slightly different variations being maintained, along with handling some edge cases that existed and would lead to bugs (notably substring matches).
- Centralizing changes here seems wiser than scattered about. I've tried to make it easy to grok, hopefully it's not worse than the current situation.
- List operations were kept in their respective commands, `db.sh` is only really managing writes. I didn't see a nice way for removing the code duplication for list commands as the duplication was fairly minimal, especially for `listalias` and `listdovecotmasteruser` which were quite simple in their differences in the loop body.
- `listmailuser` and `delmailuser` also retain methods exclusive to respective commands, I wasn't sure if there was any benefit to move those, but they were refactored.
* fix: Reload `amavisd-new` when vhost config is updated
Amavis was not aware of new domains in `/etc/postfix/vhost` as it did not refresh it's sources upon change detection.
- Inline docs for `check-for-changes.sh` have been shuffled around and revised a bit.
- Change processing extracted from the main change detection loop method to their own methods:
- `_get_changed_files()` - Clarifies what is going on (and how) without having to look it up. To reduce noise in the main logic loop, extracted to a separate method.
- `_postfix_dovecot_changes()` - The bulk of change processing was moved to this method. I've added conditionals to only run relevant logic.
- `_ssl_changes()` - Just shifted, no logic changed. `REGEX_NEVER_MATCH` and `ACME_CERT_DIR` vars scope set to `local`.
* chore: Split vhost helper method and use filepath vars
- Helpers `accounts.sh` and `aliases.sh` can move their vhost code into this helper.
- They share duplicate code with `bin/open-dkim` which will also leverage this vhost helper going forward.
* chore: Sync vhost generation logic into helper
- Chunky commit, but mostly copy/paste of logic into a common method.
- `bin/open-dkim` additionally wrapped relevant logic in a function call and revised inline docs.
* chore: Include LDAP vhost support
- Revises notes for LDAP vhost support.
- This now ensures LDAP users get vhost rebuilt to match the startup script for when change detection support is enabled.
- `bin/open-dkim` will additionally be able to support the default `DOMAINNAME` var (set via `helpers/dns.sh`) for LDAP users instead of requiring them to provide one explicitly.
* chore(`bin/open-dkim`): Ensure `DOMAINNAME` is properly set
- This will ensure LDAP users insert the same `DOMAINNAME` value as used during container startup.
- The container itself should panic at startup (during `helpers/dns.sh`) if this isn't configured correctly already, thus it should not introduce any breaking change to users of this utility?
* chore: Set the 2nd value as blank `_`
Line is split by a delimiter such as white-space (or via IFS: `|`), the blank `_` var is to indicate we're not interested in that value, but still leverage how `read -r` works, instead of splitting the var ourselves first thing.
* chore: Remove shellcheck disable lines
No longer applicable with the switch to `_`
* chore: Remove requirement for `postfix-accounts.cf`
This is an old requirement from when the change detector service was first introduced. It's no longer relevant.
* chore: Do not needlessly create `postfix-aliases.cf`
The config was created regardless to workaround early change detection support. No longer necessary to require the file to exist.
* chore: Drop guards requiring `/tmp/docker-mailserver` to exist
Legacy guards when this was the only location change detection location supported.
There does not appear to be any need for changing into this directory at the start of `check-for-changes.sh` as we use absolute filepaths (originally monitored files were checked with relative paths to this config dir).
* chore: Revise inline docs
* chore: Add change detection monitoring for extra configs
These are also handled at run-time in the current change detection support, so it makes sense to allows these config updates to also trigger change events.
* chore(`aliases.sh`): Filepath to local var `DATABASE_VIRTUAL`
* chore(`accounts.sh`): Filepath to local var `DATABASE_ACCOUNTS`
* chore(`accounts.sh`): Filepath to local var `DATABASE_VIRTUAL`
* chore(`accounts.sh`): Filepath to local var `DATABASE_DOVECOT_MASTERS`
* chore(`bin/open-dkim`): Filepaths to local vars (accounts,virtual,vhost)
* chore(`relay.sh`): Filepath to local var `DATABASE_SASL_PASSWD`
* chore: Rename method
Prior PR feedback suggested a better helper method name.
* chore: Normalize filtering config lines as input for iterating
* chore: Remove `_is_comment` helper method
No longer serving a purpose with more appropriate filter method for pre-processing the entire config file.
* fix(listmailuser): Don't parse comments
Avoids passing comments to `dovecot_quota_to_hr()` which fails to handle it and would throws errors.
* chore: Move config filter method to `helpers/utils.sh`
This helper was to support an earlier ENV for SASL auth support. When extracting logic into individual helpers, it was assumed this was separate from relay support, which it appears was not the case.
---
The `SASL_PASSWD` ENV is specified in tests but no longer used. There is no `external-domain.com` relay configured or tested against anywhere in the project.
The ENV was likely used in tests prior to improved relay support that allowed for adding more than a single set of relay credentials.
---
It likewise has no real relevance anywhere else outside of `relay.sh` as it's the only portion of code to operate with it.
It's only relevant for SASL auth as an SMTP client, not the SMTP server (`smtpd`) SASL support that is delegated to Dovecot. Functionality has been completely migrated into `relay.sh` as a result.
Documentation is poor for this ENV, it is unlikely in wide use? Should consider for removal.
---
The ENV has been dependent upon `RELAY_HOST` to actually enable postfix to use `/etc/postfix/sasl_passwd`, thus not likely relevant in existing setups?
---
Migrate `/etc/postfix/sasl_passwd` check from `tests.bats` as it belongs to relay tests.
* chore: Fix typo
* chore: Apply explicit chroot default for `sender-cleanup`
The implicit default is set to `y` as a compatibility fallback, but otherwise it is [advised to set to `n` going forward](http://www.postfix.org/COMPATIBILITY_README.html#chroot).
Test was changed to catch any backwards-compatibility logs, not just those for `chroot=y`. `using` added as a prefix to avoid catching log message whenever a setting is changed that the default compatibility level is active.
* chore: Set `compatibility_level` in `main.cf`
We retain the level`2` value previously set via scripts. This avoids log noise that isn't helpful.
Applied review feedback to give maintainers some context with this setting and why we have it presently set to `2`.
* fix: Conditionally add service state
These services will no longer copy over state unless they are enabled.
The biggest offender here was ClamAV as it's database that is baked into the docker image is over 200MB and would copy over to every container instance with a volume mounted state directory.
* chore: Add Dovecot to conditional support
* chore: Extract change-detection method to it's own helper
This doesn't really belong in `helpers/ssl.sh`. Moving to it's own helper script.
* chore: Co-locate related change-detection method from container startup
It seems relevant to migrate the related support during startup for the change detection feature into this helper.
I opted to move the call from `start-mailserver.sh` into the `_setup` call at the end for a more explicit/visible location.
* chore: Move `CHKSUM_FILE` into `helpers/change-detection.sh`
It belongs there, not in `helpers/index.sh`.
* chore: Revise inline documentation
* tests(fix): Ensure correct functionality
Presently `test/test_helper.bats` is using it's own `CHKSUM_FILE` instead of sourcing the var for the filepath.
`test_helper/common.bash` was calling a method to check for changes, but this helper may not correctly detect letsencrypt related changes as these are not ENV rely on, but global vars handled by `helpers/dns.sh`, so that should be run first like it is for `check-for-changes.sh`.
* tests(chore): Use `CHKSUM_FILE` var from helper
* chore: `addmailuser` should use `CHKSUM_FILE` var
* chore: Update `check-for-changes.sh` log message with correct path
* chore: Make `_populate_relayhost_map` easier to grok
Changes to `sed` handling that made it quicker to grok, and thus easier for maintainers like myself:
- Switched regex to [extended regex](https://www.gnu.org/software/sed/manual/html_node/Extended-regexps.html).
- Extracted `sed` patterns to be self-descriptive local vars.
- Used a function to reduce noise from intent of loop input (each line as `DOMAIN_PART`).
Input for the loop is filtered through `sort -u` to drop duplicates, reducing iterations.
`DOMAIN` loop var renamed to less vague `DOMAIN_PART`. Additional comment in the containing method clarifies what the domain part refers to.
---
`|` regexp syntax needed to be escaped due to switch. Not documented in the earlier link. `-r`/`-E` (ERE) aka extended regexp syntax is [detailed here](https://learnbyexample.github.io/learn_gnused/breere-regular-expressions.html#cheatsheet-and-summary).
* chore: Drop unnecessary postfix parameters
`smtp_tls_note_starttls_offer = yes` - Only adds a log entry to let you know when an unencrypted connection was made, but STARTTLS was offered:
https://www.postfix.org/postconf.5.html#smtp_tls_note_starttls_offer
`smtp_tls_CAfile` is unnecessary. This was added before `smtp_tls_CApath = /etc/ssl/certs` was several months later via a separate PR.
* chore: Move `smtp_` parameters to relevant sections
These have been shifted to relevant logic for now.
---
NOTE: `SASL_PASSWD` previously needed to define `RELAY_HOST` to set `smtp_sasl_password_maps` to enable the `/etc/postfix/sasl_passwd` table. This change now additionally blocks early on in `_relayhost_sasl`. Not likely important due to `RELAY_HOST` logic, user should be using the `RELAY_USER` + `RELAY_PASSWORD` ENV or `postfix-sasl-password.cf` instead.
Especially the sender dependent parameters which are only relevant with user provided configs really.
`SASL_PASSWD` is the oldest ENV for relay support before any other relay feature arrived. It is poorly documented and should not be used.
Potential breakage risk considered acceptable.
* chore: Revise inline docs
Further clarifying current processing logic and adding some additional notes for future work.
* chore: Use a common ENV relay-host getter
The mapping should be in sync between the two configs.
I also wanted to raise awareness of current state of support, which will likely need some refactoring.
This also removes the need for the `RELAY_PORT` fallback method.
The log message was adjusted as configuration is potentially for more than one relay host beyond the currently required ENV config to enable support.
---
NOTE: The ENV `DEFAULT_RELAY_HOST` skips modifying the default transport for an authenticated relay (locked behind `RELAY_HOST` to activate). It presently will only relay mail through a relay host on port 25 instead of delivering directly to the destination. A separate use-case.
* chore: Revise config examples
More verbose example configs with expanded documentation.
Additional doc references for SASL support and cautioning maintainers that may reference popular relay service providers docs. May later be migrated to a "maintainers" section in official docs and link to that.
Brief overview description of what `_populate_relayhost_map` is doing.
* chore: Add notes pertaining to future work
`_populate_relayhost_map` will get some refactoring in future and likely introduce some breaking changes for a future major release.
* chore: Better document relay support inline
This helper now includes a description of it's purpose, links to relevant user docs and supported `setup.sh` commands.
Intent is to keep a maintainer of the feature aware of anything relevant to this feature.
* consistently name functions (starting with `_`) in `start-mailserver.sh`
Most of the functions that execute the different stacks during startup
were not prefixed with `_`, but all our other functions are. This has
now been fixed.
* cleanup in `start-mailserver.sh`
I adjusted the comments for all sections in the start script so they are
properly displayed again.
Dovecot master accounts can now be configured in DMS via `setup.sh`.
A master account is useful for administration purposes, or to perform mailbox backups of every user account over IMAP.
Upstream Docs: https://doc.dovecot.org/configuration_manual/authentication/master_users/
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* chore: Extract letsencrypt logic into methods
This allows other scripts to share the functionality to discover the correct letsencrypt folder from the 3 possible locations (where specific order is important).
As these methods should now return a string value, the `return 1` after a panic is now dropped.
* chore: Update comments
The todo is resolved with this PR, `_setup_ssl` will be called by both cert conditional statements with purpose for each better documented to maintainers at the start of the logic block.
* refactor: Defer most logic to helper/ssl.sh
The loop is no longer required, extraction is delegated to `_setup_ssl` now.
For the change event prevention, we retrieve the relevant FQDN via the new helper method, beyond that it's just indentation diff.
`check-for-changes.sh` adjusted to allow locally scoped var declarations by wrapping a function. Presently no loop control flow is needed so this seems fine. Made it clear that `CHANGED` is local and `CHKSUM_FILE` is not.
Panic scope doesn't require `SSL_TYPE` for context, it's clearly`letsencrypt`.
* fix: Correctly match wildcard results
Now that the service configs are properly updated, when the services restart they will return a cert with the SAN `DNS:*.example.test`, which is valid for `mail.example.test`, however the test function did not properly account for this in the regexp query.
Resolved by truncating the left-most DNS label from FQDN and adding a third check to match a returned wildcard DNS result.
Extracted out the common logic to create the regexp query and renamed the methods to communicate more clearly that they check the FQDN is supported, not necessarily explicitly listed by the cert.
* tests(letsencrypt): Enable remaining tests
These will now pass. Adjusted comments accordingly.
Added an additional test on a fake FQDN that should still be valid to a wildcard cert (SNI validation in a proper setup would reject the connection afterwards).
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
This PR does two small things:
1. The log level, in case it is unset, will now be "calculated" from
`/etc/dms-settings` and not always default to `info`. This way, we
can ensure that more often than not, the log level the user chose
when starting DMS is used everywhere.
2. I noticed that the way I obtained the log level could be used to
obtain any env variable's log level. I therefore added a function to
`utils.sh` in case we use it in the future.
* first adjustments to use Fail2Ban with nftables
* replace `iptables` -> `nftables` and adjust tests
nftables lists IPs a bit differently , so the order was adjusted for the
tests to be more flexible.
* line correction in mailserver.env
* change from `.conf` -> `.local` and remove redundant config
* revert HEREDOC to `echo`
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* refactored `check-for-changes.sh`
I refactored `check-for-changes.sh` and used the new log. `_notify` can
therefore be deleted as it is used no more.
I opted to source `/etc/dms-settings` as a whole to
future-proof the script. When the DNS adjustments PRs (that do not exist
by now but will exit in the future) are done, we can then remove
`_obtain_hostname_and_domainname` because we're already writing the
variables to `/etc/dms-settings`. I left instructions in the script in
the form of TODO comments.
Because we now log the date for all messages of the changedetector, we
need to `tail` a bit more log than before.
* refactored `daemon-stack.sh`
A new method was introduced to uniformaly start daemons and log output
accordingly. The methods for daemon start were renamed (plural ->
singular), therefore the adjustments in `start-mailserver.sh`.
* cleaned Fetchmail setup from `daemon-stack.sh`
Not sure why, but the Fetchmail setup was somehow happening in
`daemon-stack.sh` - this is not supposed to be the case. I relocated the
setup into `setup-stack.sh`, where it belong.
* delete old, unnecessary script in `target/bin/`
These are unused leftovers from the last commit, that relocated the
setup of Fetchmail into `setup.stack.sh`.
* corrected changedetector function name
* Apply suggestions from code review
* adjusted `debug-fetchmail` script
It is absolutely fine to source `setup-stack.sh` because sourcing the
script does not execute a single function (by desing of the script).
This way, we retain functionality.
* praise be ShellCheck
* added `log.sh` to `debug-fetchmail` as a dependency
* final cleanup
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* `update-check.sh` now uses the new log
* refactored `setup-stack.sh`
The changes are:
1. Replaced `""` wiht `''` where possible (reasoning: Bash is very
implicit and I'd like to use `''` where possible to indicate no
variables are expanded here)
2. `> /file` -> `>/file` according to our style guide
3. Some log adjustments for messages where I deemed it appropriate
4. Then, an error message from a Dovecot setup was also prevented (by
adding a check whether the directory is present before a `: >...`
command would create a file in this directory).
These are all small, miscellaneous changes that I wanted to combine into
one commit and ultimately one PR because I see no point in opening a PR
for every small change here. I hope this is fine.
* added a small `sleep` to the `_shutdown` function
This ensure the last log message is actually logged before Supervisor
logs the message that it received a SIGTERM. This makes reading the log
easier because now the causal relationship is shown (we are terminating
Supervisor, and not someone else and we're just logging it).
I forgot to replace `""` with `''` in `update-check.sh`, so I included
it here because this is the last commit before PR review.
* re-add exit on successful update (only)
* re-added date information to update-check log messages
* added `_log_with_date` function
The new function will log a message with a proper timestamp. This is all
handled in `log.sh`, we therefore not need to source other files too.
This will be used in the future by `check-for-changes.sh` as well :)
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* refactored scripts located under `target/bin/`
The scripts under `target/bin/` now use the new log and I replaced some
`""` with `''` on the way. The functionality stays the same, this mostly
style and log.
* corrected fail2ban (script and tests)
* corrected OpenDKIM log output in tests
* reverted (some) changes to `sedfile`
Moreover, a few messages for BATS were streamlined and a regression in
the linting script reverted.
* apple PR feedback
* improve log output from `fail2ban` script
The new output has a single, clear message with the '[ ERROR ] '
prefix, and then output that explains the error afterwards. This is
coherent with the logging style which should be used while providing
more information than just a single line about IPTables not functioning.
* simplified `setquota` script
* consistently named the `__usage` function
Before, scripts located under `target/bin/` were using `usage` or
`__usage`. Now, they're using `__usage` as they should.
* improved `sedfile`
With `sedfile`, we cannot use the helper functions in a nice way because
it is used early in the Dockerfile at a stage where the helper scripts
are not yet copied. The script has been adjusted to be canonical with
all the other scripts under `target/bin/`.
* fixed tests
* removed `__usage` from places where it does not belong
`__usage` is to be used on wrong user input, not on other failures as
well. This was fixed in `delquota` and `setquota`.
* apply PR review feedback
* added new `_log` function
With `_log`, the `_notify` method wa rendered obsolete. `_notify` was
not completely removed due to test failures in `check-for-changes.sh`.
The new `_log` function properly uses log levels such as `trace`,
`debug`, `info`, `warn` and `error`. It provides a cleaner solution
and renders `DMS_DEBUG` obsolete too (as only `_notify` depends on it).
* converted all helper script to new `_log` function
* converted all startup stacks to new `log` function
* `start-mailserver.sh` now uses new `_log` function
* final test and misc small script adjustments
* updated documentation
The new setup will now set env variables on one place and on one place
only. The old setup used two separate places wich is not DRY and
confusing.
Some default values changed:
1. PFLOGSUMM_TRIGGER: logrotate => none
2. REPORT_SENDER: mailserver-report@HOSTNAME => mailserver-report@DOMAIN
3. REPORT_RECIPIENT: "0" => POSTMASTER_ADDRESS
One env variable was renamed: REPORT_INTERVAL => LOGROTATE_INTERVAL
I believe these defaults to be more sensible, especially the REPORT_RECIPIENT
address. The PFLOGSUMM_TRIGGER value was changed to `none` because otherwise
people would start getting daily Postfix log summary reports automatically.
Now, this is opt-in, and reports are sent only when enabled properly.
Some of the variables changed were marked as deprecated. I removed the note,
as the variables now bear some (sane) defaults again for other variables
(i.e.) REPORT_RECIPIENT is now default for other recipient addresses.
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* get rid of subshell + exec
The new way of executing `sha512sum` should work as well as the old way
but without the clutter and possible problems the usage of subshells +
exec incurs.
Moreover, there was a misconception about array expansion. Using `""`
around an expanding array (`${ARRAY[@]}`) is quite fine (and actually
the preffered way), not because it makes the expansion _one_ string
(this would be `${ARRAY[*]}`), but it makes sure when elements are
expanded, each element has `""` around them so to speak, i.e. there is
no re-splitting of these elements.
* removed old concerns in comments
* increase test and check for changes sleep duration
The `DYNAMIC_FILES` var was quote wrapped, treating all filepaths to create checksums for as a single string that would be ignored instead of processed individually.
Removed the quotes, and changed the for loop to an array which accomplishes the same goal.
* fix: Prevent unnecessary change detection event
`acme.json` change would extract new cert files, which would then be hashed after restarting services and considered a change event, running through the logic again and restarting services once more when that was not required.
The checksum entries for those cert files are now replaced with new entries containing updated checksum hashes, after `acme.json` extraction.
Removes duplicate logic from `check-for-changes.sh` that is used/maintained elsewhere to avoid risk of problems, as this code is already starting to diverge / rot.
---
Previously the change detection support has had code added for rebuilding config upon change detection which is the same as code run during startup scripts. Unfortunately over time this has fallen out of sync. Mostly the startup scripts would get maintenance and the contributor and reviewers may not have been aware of the duplicate code handled by `check-for-changes.sh`.
That code was starting to diverge in addition to some changes in structure (_eg: relay host logic seems interleaved here vs separated out in startup scripts_). I wanted to address this before it risks becoming a much bigger headache.
Rather than bloat `helper-functions.sh` further, I've added a `helpers/` folder extracting relevant common logic between startup scripts and `changedetector`. If you want to follow that process I've kept scoped commits to make those diffs easier. Some minor changes/improvements were added but nothing significant.
---
- chore: Extract relay host logic to new `relay.sh` helper
- chore: Extract `/etc/postfix/sasl_passwd` logic to new `sasl.sh` helper
- chore: Extract `postfix-accounts.cf` logic to new `accounts.sh` helper
- chore: Extract `/etc/aliases` logic to new `aliases.sh` helper
- chore: Extract `/etc/postfix/vhost` logic to new `postfix.sh` helper
- chore: Add inline docs for Postfix configs
> These are possibly more verbose than needed and can be reduced at a later stage.
> They are helpful during this refactor process while investigating that everything is handled correctly.
`accounts.sh`:
- Add note regarding potential bug for bare domain setups with `/etc/postfix/vhost` and `mydestination` sharing same domain value.
`relay.sh`:
- Remove the tabs for a single space delimiter, revised associated comment.
- Add PR reference for original `_populate_relayhost_map` implementation which has some useful details.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* chore: Normalize container setup
Easier to grok what is different between configurations.
- Container name usage replaced with variable
- Volumes defined earlier and redeclared when relevant (only real difference is `VOLUME_LETSENCRYPT`)
- Contextual comment about the `acme.json` copy.
- Quoting `SSL_TYPE`, `SSL_DOMAIN` and `-h` values for syntax highlighting.
- Moved `-t` and `${NAME}` to separate line.
- Consistent indentation.
* chore: DRY test logic
Extracts out repeated test logic into methods
* chore: Scope configs to individual test cases (1/3)
- Preparation step for shifting out the container configs to their own scoped test cases. Split into multiple commits to ease reviewing by diffs for this change.
- Re-arrange the hostname and domain configs to match the expected order of the new test cases.
- Shuffle the hostname and domainname grouped tests into tests per container config scope.
- Collapse the `acme.json` test cases into single test case.
* chore: Scope configs to individual test cases (2/3)
- Shifts the hostname and domainname container configs into their respective scoped test cases.
- Moving the `acme.json` container config produces a less favorable diff, so is deferred to a follow-up commit.
- Test cases updated to refer to their `${CONTAINER_NAME}` var instead of the hard-coded string name.
* chore: Scope configs to individual test cases (3/3)
Final commit to shift out the container configs.
- Common vars are exported in `setup_file()` for the test cases to use without needing to repeat the declaration in each test case.
- `teardown_file()` shifts container removal at end of scoped test case.
* chore: Adapt to `common_container_setup` template
- `CONTAINER_NAME` becomes `TEST_NAME` (`common.bash` helper via `init_with_defaults`).
- `docker run ...` and related configuration is now outsourced to the `common.bash` helper, only extra args that the default template does not cover are defined in the test case.
- `TARGET_DOMAIN`establishes the domain folder name for `/etc/letsencrypt/live`.
- `_should*` methods no longer manage a `CONTAINER_NAME` arg, instead using the `TEST_NAME` global that should be valid as test is run as a sequence of test cases.
- `PRIVATE_CONFIG` and the `private_config_path ...` are now using the global `TEST_TMP_CONFIG` initialized at the start of each test case, slightly different as not locally defined/scoped like `PRIVATE_CONFIG` would be within the test case, hence the explicit choice of a different name for context.
* chore: Minor tweaks
- Test case comment descriptions.
- DRY: `docker rm -f` lines moved to `teardown()`
- Use `wait_for_service` helper instead of checking the `changedetector` script itself is running.
- There is a startup delay before the `changedetector` begins monitoring, wait until it ready event is logged.
- Added a helper to query logs for a service (useful later).
- `/bin/sh` commands reduced to `sh`.
- Change the config check to match and compare output, not number of lines returned. Provides better failure output by bats to debug against.
* chore: Add more test functions for `acme.json`
This just extracts out existing logic from the test case to functions to make the test case itself more readable/terse.
* chore: Housekeeping
No changes, just moving logic around and grouping into inline functions, with some added comments.
* chore: Switch to `example.test` certs
This also required copying the source files to match the expected letsencrypt file structure expected in the test/container usage.
* chore: Delete `test/config/letsencrypt/`
No longer necessary, using the `example.test/` certs instead.
These letsencrypt certs weren't for the domains they were used for, and of course long expired.
* chore: Housekeeping
Add more maintainer comments, rename some functions.
* tests: Expand `acme.json` extraction coverage
Finally able to add more test coverage! :)
- Two new methods to validate expected success/failure of extraction for a given FQDN.
- Added an RSA test prior to the wildcard to test a renewal simulation (just with different cert type).
- Added extra method to make sure we're detecting multiple successful change events, not just a previous logged success (false positive).
* tests: Refactor the negotiate_tls functionality
Covers all ports (except POP) and correctly tests against expected verification status with new `example.test` certs.
The `FQDN` var will be put to use in a follow-up commit.
* tests: Verify the certs contain the expected FQDNs
* chore: Extract TLS test methods into a separate helper script
Can be useful for other TLS tests to utilize.
* chore: Housekeeping
* chore: Fix test typo
There was a mismatch between the output and expected output between these two files "find key for" and "find key & cert for". Changed to "find key and/or cert for" to make the warning more clear that it's issued for either or both failure conditions.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Better logical flow, handling and inline documentation.
Despite the verbosity, it's better to make this visible here for maintenance and debugging purposes than trying to dig through issue/PR or commit history for it.
* fix: Panic when HOSTNAME is misconfigured
* chore: Add more comment docs for maintainers
* tests(fix): Use `--domainname` not ENV `DOMAINNAME`
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Previously we only monitored for `$HOSTNAME` in `/etc/letsencrypt/live` and only for hard-coded `.pem` filenames.
This ensures we check the locations of other locations that may not match `$HOSTNAME`, which we also support. Ideally in future at least the directory to look in would be better known in advance..
Split into scoped commits with messages if further details are needed, view those via the associated PR :)
**Commit Summary:**
**`check-for-changes.sh`**
- Prevent `SSL_DOMAIN` silently skipping when value has wildcard prefix `*.` (_at least this was known as a bugfix when originally committed in linked PR_).
- Improved inlined docs for maintainers.
- Additional logging for debugging.
**`helper-functions.sh:_extract_certs_from_acme`**:
- Fail if the input arg (_`$CERT_DOMAIN`, aka the FQDN_) provided for extraction is empty.
- Use `$CERT_DOMAIN` in place of `$HOSTNAME` and `$1` for a consistent value (_previously could mismatch, eg with `SSL_DOMAIN` defined_).
- The conditional is now only for handling extraction failure (_key or cert value is missing from extraction_).
- Log an actual warning or success (debug) based on outcome.
- Don't use `SSL_DOMAIN` with wildcard value for the `mkdir` letsencrypt directory name (_wildcard prefix `*.` is first stripped instead_).
**`acme_extract`** (_new python utility for `acme.json` handling_):
- Extracted out into a python script that can be treated as a utility in the `$PATH` like other helper scripts. It can now be used and optionally tested directly instead of via `helper-functions.sh`.
-Made compatible with Python 3, as Python 2 is EOL and no longer in newer versions of Debian.
Mostly cleans up the code and documents it better, although there are some minor fixes for handling `SSL_DOMAIN` ENV and additional logging added for spotting issues related to it in future when troubleshooting.
Commits are scoped with context messages for easing review if necessary. Overview of changes:
Traefik specific:
- Logic extracted out into it's own function.
- Conditional reworked to assist with debugging.
- `SSL_DOMAIN` must not be empty when attempting to extract.
- Added additional notes.
`SSL_TYPE=letsencrypt` case:
- Revised top note block.
- Correct handling for `SSL_DOMAIN`.
- Removed some unnecessary nesting.
- Less repetitive error message for `LETSENCRYPT_DOMAIN`.
- Added use of panics where appropriate (kept `return 1` so failures still exit functionality early).
- Improved inline docs.
Dovecot quota support would log auth failures when Postfix validated incoming mail to accept/reject and the `check_policy_service` for `quota-status` was queried with a recipient that was an account alias.
When Dovecot is not aware of the user account, it will not be able to check a quota and inform Postfix that everything is fine, Postfix will accept the mail and send it to Dovecot, where if the quota is exceeded will result in a bounce back to the sender. This is considered "backscatter" and can be abused by spammers forging the sender address which can get your server blacklisted.
The solution is to either disable quota support `ENABLE_QUOTAS=0`, or as a workaround, add dummy accounts to Dovecot userdb for aliases in `postfix-virtual.cf` (not `postfix-aliases.cf`), these dummy accounts will map to the real user account mailbox (real users are defined in `postfix-accounts.cf`).
The workaround is naive, in that we only check for basic 1-to-1 alias mapping to real accounts. This will still be an issue for aliases that map to another alias or multiple addresses (real or alias). Unfortunately Postfix will not expand aliases until accepting mail where this would be too late.
A better solution is to proxy the `check_policy_service` from Dovecot `quota-status` that Postfix queries in `main.cf:smtpd_recipient_restrictions`, however this requires a fair amount more of additional work and still requires an implementation to recursively query aliases for nested or multiple address mappings, which can then be forwarded to the `quota-status` service configured by Dovecot in `/etc/dovecot/conf.d/90-quota.conf`.
LDAP users are unaffected as quota support is not supported/implemented with `docker-mailserver` at this time, it is always considered disabled when using LDAP.
---
Additionally Dovecot configuration for `passdb` has been fixed to use the correct password hash scheme of `SHA512-CRYPT`.
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* function _defunc removed
* _shutdown is better than just notify in that cases
* PANIC_TYPE 'fail-init' introduced
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
"Brief" summary/overview of changes. See the PR discussion or individual commits from the PR for more details.
---
Only applies to the `docs/content/**` content (_and `setup` command_). `target/` and `test/` can be normalized at a later date.
* Normalize to `example.com`
- Domains normalized to `example.com`: `mywebserver.com`, `myserver.tld`, `domain.com`, `domain.tld`, `mydomain.net`, `my-domain.tld`, `my-domain.com`, `example.org`, `whoami.com`.
- Alternative domains normalized to `not-example.com`: `otherdomain.com`, `otherdomain.tld`, `domain2.tld`, `mybackupmx.com`, `whoareyou.org`.
- Email addresses normalized to `admin@example.com` (in `ssl.md`): `foo@bar.com`, `yourcurrentemail@gmail.com`, `email@email.com`, `admin@domain.tld`.
- Email addresses normalized to `external-account@gmail.com`: `bill@gates321boom.com`, `external@gmail.com`, `myemail@gmail.com`, `real-email-address@external-domain.com`.
- **`faq.md`:** A FAQ entry title with `sample.domain.com` changed to `subdomain.example.com`.
- **`mail-fetchmail.md`:** Config examples with FQDNs for `imap`/`pop3` used `example.com` domain for a third-party, changed to `gmail.com` as more familiar third-party/external MTA.
* Normalize config volume path
- Normalizing local config path references to `./docker-data/dms/config/`: `./config/`, `config/`, \``config`\`, `/etc/` (_volume mount src path prefix_).
- Normalize DMS volume paths to `docker-data/dms/mail-{data,state,log}`: `./mail`, `./mail-state` `./data/mail`, `./data/state`, `./data/logs`, `./data/maildata`, `./data/mailstate`, `./data/maillogs`, (_dropped/converted data volumes: `maildata`, `mailstate`_).
- Other docker images also adopt the `docker-data/{service name}/` prefix.
* `ssl.md` - Use `dms/custom-certs` where appropriate.
* Apply normalizations to README and example `docker-compose.yml`
---
Common terms, sometimes interchangeably used or now invalid depending on context: `mail`, `mail container`, `mail server`, `mail-server`, `mailserver`,`docker-mailserver`, `Docker Mailserver`.
Rough transformations applied to most matches (_conditionally, depending on context_):
- 'Docker Mailserver' => '`docker-mailserver`'
- 'mail container' => '`docker-mailserver`' (_optionally retaining ' container'_)
- 'mail server' => 'mail-server' / '`docker-mailserver`'
- 'mail-server' => '`docker-mailserver`'
- 'mailserver' => 'mail-server' / '`docker-mailserver`'
Additionally I checked `docker run` (_plus `exec`, `logs`, etc, sub-commands_) and `docker-compose` commands. Often finding usage of `mail` instead of the expected `mailserver`
Additionally changes `mailserver` hostname in k8s to `mail` to align with other non-k8s examples.
---
* drive-by revisions
Mostly minor revisions or improvements to docs that aren't related to normalization effort.
* fix: Spam bounced test copy/paste typo
* tests(docs): Expand inline documentation
Should assist maintainers like myself that are not yet familiar with this functionality, saving some time :)
* Refactor bounced test + Introduce initial container template
DRY'd up the test and extracted a common init pattern for other tests to adopt in future.
The test does not need to run distinct containers at once, so a common name is fine, although the `init_with_defaults()` method could be given an arg to add a suffix: `init_with_defaults "_${BATS_TEST_NUMBER}"` which could be called in `setup()` for tests that can benefit from being run in parallel.
Often it seems the containers only need the bare minimum config such as accounts provided to actually make the container happy to perform a test, so sharing a `:ro` config mount is fine, or in future this could be better addressed.
---
The test would fail if the test cases requiring smtp access ran before postfix was ready (_only a few seconds after setup scripts announce being done_). Added the wait condition for smtp, took a while to track that failure down.
* 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.
- `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.
- I've not touched LetsEncrypt section as I don't have time to investigate into that yet (not familiar with that portion).
---
* 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.
---
* Error handling for SSL_TYPE
- Added a panic utility to exit early when SSL_TYPE 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 SSL_TYPE collapsed into catch all panic, while it's contents is now mapped to a new 'disabled' value.~~
---
* 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.
---
* 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'.
---
* 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.
---
* 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:
- Replace `--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.
`tests.bats` had another starttls test for imap:
- Workaround for now is to give the main test container `SSL_TYPE=snakeoil`.
---
* 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.
---
* 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.
---
* Default SSL_TYPE becomes `''` (aka equivalent to desired `disabled` case)
- 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.
---
* Add inline documentation for `dms_panic`
- This could later be better formatted and placed into contributor docs.
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.
The two previous `_shutdown` methods can benefit from using `dms_panic` wrapper instead to standardize on panic messages.
Although these two config lines have not changed since `debian:buster-slim` image, Dovecot seems to now be affected by it which results in rejecting cipher suites below TLS v1.2.
To continue supporting the `intermediate` TLS_LEVEL, we now need to relax the global config. Dovecot could alternatively be given a modified openssl config to only affect it's interaction with openssl.
Postfix is unaffected and continues to support TLS <1.2 cipher suites when configured to.
This feature was originally introduced by the PR: https://github.com/docker-mailserver/docker-mailserver/pull/1463
- Assign default DH params to use via Dockerfile build instead of copy and update at runtime.
- Parameterized service names and paths.
- Refactor postfix and dovecot dh methods to wrap shared dh logic
- 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.
- There is no apparent need for special handling with `ONE_DIR=1`. Dropped it.
- Refactor DH params tests
- Combine custom and default DH param tests into single test file
- docs: Add instructions to use custom DH params
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.
* changed the locking function to better support multiple servers running at once and sharing the same config
* helper function testing now runs inside of container
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Centralize the collection of the HOSTNAME and DOMAINAME so that it's predictable and uniform across the various scripts (using the helper). Ensure it supports the various configurations users can have (both subdomain and without subdomain, override and no override).
---
* using _obtain_hostname_and_domainname helper + covers when not a subdomain
doc: OVERRIDE_HOSTNAME takes priority
* added tests for non-subdomain hostname + further improvements
* moved SRS DOMAINANME tests into hostname test file + Allowing DOMAINNAME ENV to override what would be automatically set
---
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* docker_container first, then fall back to docker_image
+ test changes to support
+ test change to wait for smtp port to fix flakey tests since https://github.com/docker-mailserver/docker-mailserver/pull/2104
* quick fix
* Update setup.sh
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* fix: make sure the SASLAUTHD_LDAP_HOST/PROTO logic makes sense and use LDAP_SERVER_HOST as a fallback (#1983)
* chore(docs): document changes to LDAP/SASLAUTHD as of #1983
* fix!: apply default value modifications suggested in #1983https://github.com/docker-mailserver/docker-mailserver/issues/1983#issuecomment-844848224
* chore(test): Test SASLAUTHD_LDAP_SERVER with protocol and ..._SSL=0, as well as with default bind credentials
Note that there are currently no regression tests for this as there's only one setup_file, so that would require big changes to the testing methodology.
* refactor!: completely remove SASLAUTHD_LDAP_SSL and SASLAUTHD_LDAP_PROTO
Co-authored-by: Georg Lauterbach <44545919+aendeavor@users.noreply.github.com>
Co-authored-by: Frederic Werner <20406381+wernerfred@users.noreply.github.com>
* mail binary
* initial work
* make env vars available
* typo
* some fixes
* make script ugly, to satisfy linter..
* mailserver.env updated
* Version to welcome message added
* remove VERSION file references
* VERSION --> DMS_VERSION
* fetch remote version
* variable usage
* Quoting added
* edge test & docu
* dash removed
* subject changed
* re-add VERSION
* VERSION added
* new file: VERSION
* rewrite
* unnecessary additions from fail2ban PR removed
* UPDATE_CHECK_INTERVAL added
* syntax check & _log function
* comment added
* final commit
* Use dovecot's LDAP uris option instead of hosts (fixes#1510)
* Clean up variables & environment documentation for #1901
Co-authored-by: Frederic Werner <20406381+wernerfred@users.noreply.github.com>
Co-authored-by: Georg Lauterbach <44545919+aendeavor@users.noreply.github.com>
* splitting start-mailserver.sh
* refactoring part 2
* refactored setup-stack.sh
* stzarted adjusting target/bin/*.sh to use new usage format
* corrected lowercase-uppercase test error
* better handling of .bashrc variable export
* linting tests and fix for default assignements
* last stylistic changes and rebase
My `~` substitution and any usage of `/` within `start-mailserver.sh` has been replaced with the `|` delimiter instead as advised for matching style guide preference. Note there are other `sed` substitution delimiters still in use such as `+`.
Also added warning for empty `SSL_TYPE` ENV var that may result in an internal state config persist bug when changing `SSL_TYPE` depending on how a container is restarted.
* feat: Change Postfix smtpd_tls key and cert files to chain_files
Since Postfix 3.4, `smtpd_tls_cert_file` and `smtpd_tls_key_file` have been deprecated in favor of `smtpd_tls_chain_files` which supports a list of values where a single or sequence of file paths provide a private key followed by it's certificate chain.
* feat: Dual certificate support
`smtpd_tls_chain_files` allows for multiple key+cert bundles so that you can provide different key types, such as ECDSA and RSA.
To maintain compatibility with the current CERT/KEY ENV vars only a 2nd certificate is supported.
Since Dovecot 2.2.31 a related feature is also available, but it is limited to only providing one alternative certificate via separate cert and key settings.
---
This feature enables support for multiple certificates, eg for serving modern ECDSA certs with RSA as fallback.
* chore: Refactor variable names to meet style guide
Improved some comments too.
* chore: Have function definitions respect style guide
* chore: Minor edits to comments
* chore: Expand on comments for maintenance, alert of insecure config
When `SSL_TYPE` isn't properly setup, we're still offering SSL connections but not warning in logs about the insecurity of such, or why a misconfiguration may have occurred.
This commit more clearly communicates to the user that they should look into the issue before considering deploying to production.
The `TODO` comments communicate to any future maintainer to consider treating these improper configs as disabling TLS instead.
* fix: Use `snakeoil` cert
I mistakenly thought this was placeholder text, which broke some tests. This adds the two files in the correct order (private key followed by cert/chain), to fix that issue.
* fix: Disable alt cert for Dovecot if necessary
Certain scenarios may persist state of previously configured alt cert via ENV vars that are removed from a future run. If the config is not reset to original immutable state, this will correctly disable the config from using alt cert unintentionally.
* fix: Satisfy ShellCheck lint
By switching from string var to array / list expansion, this better stores the extracted result and applies it in a manner that ShellCheck linting approves, removing the need to disable the rule.
* feat: Support dual cert test
Few tweaks to the test script allows re-purposing it for covering dual cert support as well.
* chore: Rearranged cert and key lines
A little reorganization, mostly placing private key ahead of related cert lines.
* chore: Refactor `_set_certificate`
This should make the parameters a little less confusing.
Previously was 3 parameters, but the Postfix parameter (1st) may look like two variables if you don't pay attention to the surrounding quotes; while the Dovecot parameters (2nd + 3rd) would have an opposing order. There was also a variant where the `FULLKEYCHAIN` var was passed in three times.
Now it's two params, with the 2nd param as an optional one. If the 2nd param is provided, then the two params are in the order of private key then certificate, otherwise if only a single parameter it's a single PEM file with the full cert chain and private key bundled.
This avoids implying that Postfix and Dovecot might use different files.
* chore: Document current state of `SSL_TYPE` logic better
Inlined for the benefit of anyone else maintaining this section if I'm unable to address the concerns within my own time.
* docs: ENV vars
`TLS_LEVEL=old` isn't in the codebase anymore, not likely to be relevant to retain.
No point in documenting what is considered invalid / unsupported config value in the first place for `SSL_TYPE`.
`SSL_TYPE=manual` was missing documentation for both related file path ENV vars, they've been added along with their alt fallback variants.
* chore: Update Dovecot LMTP SSL test config
Not sure how relevant this is, the file isn't complete sync with the main dovecot `10-ssl.conf` config, adding the support just in case.
* chore: Rename `FULLKEYCHAIN` to avoid confusion
There doesn't appear to be a standardized name for this type of file bundle, and `keychain` may be misleading (fullkeychain often provides macOS keychain results on search engines).
Opting for a more explicit `KEY_WITH_FULLCHAIN` name instead.
* fix: Invalid var name
`_set_certificate` refactor commit accidentally changed a var name and committed that breaking the dual cert support (thanks tests!).
* test: Refactor `mail_ssl_manual.bats`
Proper test return values instead of `wc -l` based checking.
Tests with dual cert support active, tests that feature (to better detect failure case.
Third test case was unable to verify new self-signed certificate, added new certs signed with self-signed root CA.
Adjusted openssl `CApath` parameter to use `CAfile` instead as `letsencrypt` cert was replaced thus CA cert is missing from the system trust store.
* test: Properly check for files in `mail_ssl_manual.bats`
Fixes lint error.
Also realized I was accidentally asserting a file exists in the test environment, not within the container.
Resolved that and also added an additional test case to ensure the ENV var files are valid when passed in, in the event a change misconfigures them and that the issue is identified earlier.
* chore: Apply PR review feedback
Better format some strings that had mixed quotes when they weren't necessary.
Additionally DRYed up the config path for Postfix and Dovecot within the `_setup_ssl` method.
Co-authored-by: Georg Lauterbach <infrastructure@itbsd.com>
* let dkim generator accept selector as parameter
* test dkim-generator with selector parameter
* fix: correct name of domain argument in usage
* fix: adapt command to new syntax
* tests: use different quotes
* tests: use different quotes
* tests: remove domains that were never added
* style: change test name
* refactor: dkim setup
* style: remove trailing whitespace
* tests: remove test of removed dummy file
Co-authored-by: Frederic Werner <20406381+wernerfred@users.noreply.github.com>
* Migrate PR#1730 from tomav/docker-mailserver repo to new
docker-mailserver/docker-mailserver repo
* Resolved review comments
* Moved counter increment to have consistency between fetchmail process
and fetchmail config files
* Added tests for new fetchmail option
Co-authored-by: Georg Lauterbach <44545919+aendeavor@users.noreply.github.com>
Georg Lauterbach
Brennan Kinney
Casper
Gabriel Euzet
René Plötz
Andreas Perhab
Thomas VIAL
Maximilian Hippler
Andrew Cornford
Ikko Ashimine
NorseGaud
Christian Raue
Philipp Fruck
Andreas Egli
Nathan Pierce
William Desportes
Georg Lauterbach
Moritz Marquardt
Moritz Marquardt
landergate
Astro
Ask Bjørn Hansen
brainkiller