* 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>
* 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.
* disabled unreliable test
The "quota exceeded" test is unreliable and failed too often lately for
my taste. Therefore, I'd like to disable it because there is no use in
having such a test.
* corrected PR id in URL
* 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>
These are improvements for better supporting the requirements of other tests.
- Opted for passing an array reference instead of an ENV file. This seems to be a better approach and supports more than just ENV changes.
- Likewise, shifted to a `create` + `start` approach, instead of `docker run` for added flexibility.
- Using `TEST_TMP_CONFIG` instead of `PRIVATE_CONFIG` to make the difference in usage with config volume in tests more clear.
- Changed the config volume from read-only volume mount to be read-write instead, which seems required for other tests.
- Added notes about logged failures from a read-only config volume during container startup.
- Added `TEST_CA_CERT` as a default CA cert path for the test files volume. This can be used by default by openssl methods.
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.
These files will replace the existing `test/config/letsencrypt` content which has some random provisioned FQDN for letsencrypt that doesn't match the FQDN tested, `acme.json` files with FQDNs that don't match those certs FQDNs and changes to certs that won't expire until 2031. `test/config/letsencrypt` will be removed with the associated test update PR.
The changes amount to:
- Re-configuring the FQDN values that some certs were created for (_needed for flexibility in testing_).
- Adding an `*.example.test` wildcard (_both RSA and ECDSA_).
- Adding `acme.json` encoded versions (_traefik extraction support will use these instead_).
- Updated / new internal docs for maintainers of this content.
For more detailed information on those changes, please see the associated commit messages via the PR.
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>
* 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.
