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