Currently a change detection would be triggered and during processing, a CRLF is converted to LF, which updates the `postfix-accounts.cf` file and triggers another change event.
No need for the first approach to add an account, and it is the culprit for causing the CRLF to appear.
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>
* tests: Ensure excessive FD limits are avoided
Processes that run as daemons (`postsrsd` and `fail2ban-server`) initialize by closing all FDs (File Descriptors).
This behaviour queries that maximum limit and iterates through the entire range even if only a few FDs are open. In some environments (Docker, limit configured by distro) this can be a range exceeding 1 billion (from kernel default of 1024 soft, 4096 hard), causing an 8 minute delay with heavy CPU activity.
`postsrsd` has since been updated to use `close_range()` syscall, and `fail2ban` will now iterate through `/proc/self/fd` (open FDs) which should resolve the performance hit. Until those updates reach our Docker image, we need to workaround it with `--ulimit` option.
NOTE: If `docker.service` on a distro sets `LimitNOFILE=` to approx 1 million or lower, it should not be an issue. On distros such as Fedora 36, it is `LimitNOFILE=infinity` (approx 1 billion) that causes excessive delays.
* chore: Use Docker host limits instead
Typically on modern distros with systemd, this should equate to 1024 (soft) and 512K (hard) limits. A distro may override the built-in global defaults systemd sets via setting `DefaultLimitNOFILE=` in `/etc/systemd/user.conf` and `/etc/systemd/system.conf`.
* tests(fix): Better prevent non-deterministic failures
- `no_containers.bats` tests the external script `setup.sh` (without `-c`). It's expected that no existing DMS container is running - otherwise it may attempt to use that container and fail. Detect this and fail early via `setup_file()` step.
- `mail_hostname.bats` had a odd timing failure with teardown due to the last tests bringing the containers down earlier (`docker stop` paired with the `docker run --rm`). Adding a moment of delay via `sleep` helps avoid that false positive scenario.
## Quick Summary
Resolves a `TODO` task with `addmailuser`.
## Overview
The main change is adding three new methods in `common.bash`, which replace the completion delay in `addmailuser` / `setup email add` command.
Other than that:
- I swapped `sh -c 'addmailuser ...'` to `setup email add ...`.
- Improved three tests in `setup-cli.bats` for `setup email add|update|del` (_logic remains effectively the same still_).
- Rewrote the `TODO` comment for `setup-cli.bats` test on `setup email del` to better clarify the concern, but the test itself was no longer affected due to changes prior to this PR, so I enabled the commented out assertion.
- Removed unnecessary waits. The two `skip` tests in `test/tests.bats` could be enabled again after this PR.
- Additional fixes to tests were made during the PR (see discussion comments for details), resolving race conditions.
Individual commit messages of the PR provide additional details if helpful.
---
## Relevant commit messages
* chore: Remove creation delay in `addmailuser`
This was apparently only for supporting tests that need to wait on account creation being ready to test against.
As per the removed inline docs, it should be fine to remove once tests are updated to work correctly without it.
* tests(feat): Add two new common helper methods
`wait_until_account_maildir_exists()` provides the same logic `addmailuser` command was carrying, to wait upon the account dir creation in `/var/mail`.
As this was specifically to support tests, it makes more sense as a test method.
`add_mail_account_then_wait_until_ready()` was added to handle the common pattern of creating account and waiting on it. An internal assert will ensure the account was successfully created first during the test before attempting to wait.
* tests(feat): Add common helper for waiting on change event to be processed
The current helper is more complicated for no real benefit, it only detects when a change is made that would trigger a change event in the `changedetector` service. Our usage of this in tests however is only interested in waiting out the completion of the change event.
Remove unnecessary change event waits. These waits should not be necessary if handled correctly.
* tests: `addmailuser` to `add_mail_account_then_wait_until_ready mail()`
This helper method is used where appropriate.
- A password is not relevant (optional).
- We need to wait on the creation on the account (Dovecot and `/var/mail` directory).
* tests: `setup-cli` revise `add`, `update`, `del` tests
The delete test was failing as the `/var/mail` directory did not yet exist.
There is now a proper delay imposed in the `add` test now shares the same account for both `update` and `del` tests resolving that failure.
Additionally tests use better asserts where appropriate and the wait + sleep logic in `add` has been improved (now takes 10 seconds to complete, approx half the time than before).
The `del` test TODO while not technically addressed is no longer relevant due to the tests being switched to `-c` option (there is a separate `no container` test file, but it doesn't provide a `del` test).
* tests(fix): Ensure Postfix is reachable after waiting on ClamAV
There is not much reason to check before waiting on ClamAV.
It is more helpful to debug failures from `nc` mail send commands if we know that nothing went wrong inbetween the ClamAV wait time.
Additionally added an assertion which should provide more information if this part of the test setup fails again.
* tests(fix): Move health check to the top
This test is a bit fragile. It relies on defaults for the healthcheck with intervals of 30 seconds.
If the check occurs while Postfix is down due a change event from earlier tests and the healthcheck kicks in at that point, then if there is not enough time to refresh the health status from `unhealthy`, the test will fail with a false-positive as Postfix is actually working and up again..
* tests(fix): Wait on directory to be removed
Workaround that tries not to introduce heavier delays by waiting on a full change event to complete in the previous `email update` if possible.
There is a chance that the account has the folder deleted, but restored from an active change event (for password update, then the account delete).
The new version uses our `log.sh` helper to simplify logging
significantly. Moreover, the script was adjusted to the current style
and the GitHub workflow was streamlined. The workflow is ot providing
the version anymore (which was useless anyway), and has been compacted.
* 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>
* tests: Update testing submodules (bats-assert, bats-support)
These two submodules were migrated to the `bats-core` organization, where they continued to receive updates.
* tests: Use tagged release of `bats-core/bats-support`
This is technically one commit backwards, but no relevant difference has been made since, other than moving the submodule to the `bats-core` organization.
* tests: Bump `bats-assert` to August 2022 (master)
No official release tag since Nov 2018, but a fair amount of changes since then.
* tests: Bump `bats-core` to `v1.7.0` release
* tests(fix): Correctly use assertions
Some tests were updated as the upgrade of bats submodules had `assert` methods raise awareness of incorrect usage.
This additionally revealed some existing tests that weren't meant to be using `run`, which swallowed failures from surfacing.
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.
* chore: Create bare new test file `setup-cli.bats`
Bare minimum to setup a new test.
* chore: Transfer over relevant tests
* chore: `mail` container name to dynamic `${TEST_NAME}`
Only applied where it's relevant. Next commit will handle the config path correction.
* chore: Use `TEST_TMP_CONFIG` for referencing local config directory
Could technically use the existing function call. Some paths were using a hard-coded config location.
Both have been converted to `TEST_TMP_CONFIG` and related `grep` calls normalizing the quote mark usage, escaping doesn't seem necessary.
* tests(fix): Create container without providing extra args reference var
If a variable name (of an array) was not provided to reference, this would fail trying to reference `'`.
* chore: Remove `SYS_PTRACE` capability from docs and configs
* chore: Remove `SYS_PTRACE` capability from tests
Doesn't seem to be required. It was originally added when the original change detection feature PR apparently needed it to function.
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`.
* 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
* tests(fix): Increase some timeouts
Running tests locally via a VM these tests would fail sometimes due to the time from being queued and Amavis actually processing being roughly around 30 seconds.
There should be no harm in raising this to 60 seconds, other than delaying a failure case which will ripple through other time sensitive tests.
It's better to pass when functionality is actually correct but just needs a bit longer to complete.
* tests(fix): Don't setup an invalid hostname
During container startup `helpers/dns.sh` would panic with `hostname -f` failing.
Dropping `--domainname` for this container is fine and does not affect the point of it's test.
---
It's unclear why this does not occur in CI. Possibly changes within the docker daemon since as CI runs docker on Ubuntu 20.04? (2020).
For clarity, this may be equivalent to setting a hostname of `domain.com.domain.com`, or `--hostname` value truncated the NIS domain (`--domainname`) of the same value.
IIRC, it would still fail with both options using different values if `--hostname` was multi-label. I believe I've documented how non-deterministic these options can be across different environments.
`--hostname` should be preferred. There doesn't seem to be any reason to actually need `--domainname` (which is NIS domain name, unrelated to the DNS domain name). We still need to properly investigate reworking our ENV support that `dns.sh` manages.
---
Containers were also not removing themselves after failures either (missing teardown). Which would cause problems when running tests again.
* chore: Normalize white-space
Sets a consistent indent size of 2 spaces. Previously this varied a fair bit, sometimes with tabs or mixed tabs and spaces.
Some formatting with blank lines.
Easier to review with white-space in diff ignored. Some minor edits besides blank lines, but no change in functionality.
* fix: `setup.sh` target container under test
Some of the `setup.sh` commands did not specify the container which was problematic if another `docker-mailserver` container was running, causing test failures.
This probably doesn't help with `test/no_container.bats`, but at least prevents `test/tests.bats` failing at this point.
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>
* 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
