* 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`
* chore: Co-locate process checking and process restart verification
Extract the test cases for checking a process is running and properly restarts from various test files into a single one:
Core (always running):
opendkim, opendmarc, master (postfix)
ENV dependent:
amavi (amavisd-new), clamd, dovecot, fail2ban-server (fail2ban), fetchmail, postgrey, postsrsd, saslauthd
These now run off a single container with the required ENV and call a common function (the revised version in parallel test cases).
* fix(saslauthd): Quote wrap supervisor config vars
`saslauth.conf` calls `-O` option for most commands defined with an ENV that may be empty/null. This would cause the process to silently fail / die.
This doesn't happen if quote wrapping the ENV, which calls `-O` with an empty string.
Not necessary, but since one of `postgrey` ENV were quote wrapped in `supervisor-app.conf`, I've also done the same there.
* fix(postsrsd): Change supervisor `autorestart` policy to `true`
The PR that introduced the config switched from `true` to `unexpected` without any context. That prevents restart working when the process is killed. Setting to `true` instead will correctly restart the service.
* chore: Remove disabled postgrey test file
`mail_with_postgrey_disabled_by_default.bats` only checked the migrated test cases, removed as no longer serving a purpose.
* tests(refactor): Make `_should_restart_when_killed()` more reliable
The previous version did not ensure that the last checks process was actually restarted, only that it was running.
It turns out that `pkill` is only sending the signal, there can be some delay before the original process is actually killed and restarted.
This can be identified with `pgrep --older <seconds>`. First ensure the process is at a specified age, then after killing check that the process is not running that is at least that old, finally check that there is a younger process actually running.. (_could fail if a process doesn't restart, or there is a delay such as imposed by `sleep` in wrapper scripts for postfix and fail2ban_)
The helper method is not used anywhere else now, move it into this test instead. It has been refactored to accomodate the needs for `--older`, and `--list-full` provides some output that can be matched (similar for `pkill --echo`).
* test(docs): Add inline notes about processes
* chore: Compress test cases into single case with loop
Moves the list of processes into array vars to iterate through instead.
If a failure occurs, the process name is visible along with line number in `_should_restart_when_killed()` to identify what went wrong.
* chore: Handle `FETCHMAIL_PARALLEL=1` process checks as well
* tests: Add test case for disabled ENV
Additional coverage to match what other test files were doing before, ensuring that these ENV can prevent their respective service from running.
* chore: Move `clamd` enabled check to it's own test case
Not sure about this.
It reduces the time of CPU activity (sustained full load on a thread) and increase in memory usage (1GB+ loading signatures database), but as a separate test case it also adds 10 seconds without reducing the time of the test case it was extracted from.
* chore: Make `disabled` variant the 1st test case
* fix: Adjust test cases to pass when using slower wrapper scripts
* tests(refactor): `mail_fetchmail.bats` updated to new format
Additionally merges in the parallel test file.
* chore: Move `config/fetchmail.cf` into separate sub-directory
Keep out of the default base config for tests.
* chore: Change `fetchmail.cf` FQDNs to `.test` TLD
Changed the first configs remote and local user values to more clearly document what their values should represent (_and that they don't need to be a full mail address, that's just what our Dovecot is configured with for login_).
Shifted the `here` to the end of the `is` line. It's optional syntax, only intended to contrast with the remote `there` for readability.
Additionally configured imap protocol. Not tested or verified if that's correct configuration for usage with imap protocol instead. The fetchmail feature tests are currently lacking.
Added an inline doc into the fetchmail test to reference a PR about the importance of the trailing `.` in the config. Updated the partial matching to ensure it matches for that in the value as well.
* chore: Finalize `process-check-restart.bats`
Few minor adjustments. The other ENV for clamd doesn't seem to provide any benefit, trim out the noise. Added a note about why it's been split out.
Fetchmail parallel configs are matching the config file path in the process command that is returned. The `.rc` suffix is just to add further clarity to that.
* 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.
## 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).
* 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`.
Presently relay-host support modifies `main.cf` settings directly. This adjusts the default transport (`smtp`) which other transports in `master.cf` inherit from.
When configuring for implicit TLS to a `relay-host` this would set `main.cf:smtp_tls_wrappermode = yes` and affect the transport `master.cf:smtp-amavis` which does not set an override like it does for `smtp_tls_security_level`. This causes Amavis to fail working which the default transport relies on due to `main.cf:content_filter`.
Easy fix, by explicitly adding the override `smtp_tls_wrappermode=no`.`no` is default in `main.cf` so inheriting this setting hasn't been a problem in the past for other relay-hosts using StartTLS.
* 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: Remove `DATABASE` fallback ENV
This was introduced without any mention or need, thus removing until a real use-case requires it.
* chore: Remove `USER_DATABASE` fallback ENV
Likewise, nothing requires this to be customizable.
* chore: Consistently use single quote strings
* chore: Extract letsencrypt logic into methods
This allows other scripts to share the functionality to discover the correct letsencrypt folder from the 3 possible locations (where specific order is important).
As these methods should now return a string value, the `return 1` after a panic is now dropped.
* chore: Update comments
The todo is resolved with this PR, `_setup_ssl` will be called by both cert conditional statements with purpose for each better documented to maintainers at the start of the logic block.
* refactor: Defer most logic to helper/ssl.sh
The loop is no longer required, extraction is delegated to `_setup_ssl` now.
For the change event prevention, we retrieve the relevant FQDN via the new helper method, beyond that it's just indentation diff.
`check-for-changes.sh` adjusted to allow locally scoped var declarations by wrapping a function. Presently no loop control flow is needed so this seems fine. Made it clear that `CHANGED` is local and `CHKSUM_FILE` is not.
Panic scope doesn't require `SSL_TYPE` for context, it's clearly`letsencrypt`.
* fix: Correctly match wildcard results
Now that the service configs are properly updated, when the services restart they will return a cert with the SAN `DNS:*.example.test`, which is valid for `mail.example.test`, however the test function did not properly account for this in the regexp query.
Resolved by truncating the left-most DNS label from FQDN and adding a third check to match a returned wildcard DNS result.
Extracted out the common logic to create the regexp query and renamed the methods to communicate more clearly that they check the FQDN is supported, not necessarily explicitly listed by the cert.
* tests(letsencrypt): Enable remaining tests
These will now pass. Adjusted comments accordingly.
Added an additional test on a fake FQDN that should still be valid to a wildcard cert (SNI validation in a proper setup would reject the connection afterwards).
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
This PR does two small things:
1. The log level, in case it is unset, will now be "calculated" from
`/etc/dms-settings` and not always default to `info`. This way, we
can ensure that more often than not, the log level the user chose
when starting DMS is used everywhere.
2. I noticed that the way I obtained the log level could be used to
obtain any env variable's log level. I therefore added a function to
`utils.sh` in case we use it in the future.
* first adjustments to use Fail2Ban with nftables
* replace `iptables` -> `nftables` and adjust tests
nftables lists IPs a bit differently , so the order was adjusted for the
tests to be more flexible.
* line correction in mailserver.env
* change from `.conf` -> `.local` and remove redundant config
* revert HEREDOC to `echo`
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* refactored `check-for-changes.sh`
I refactored `check-for-changes.sh` and used the new log. `_notify` can
therefore be deleted as it is used no more.
I opted to source `/etc/dms-settings` as a whole to
future-proof the script. When the DNS adjustments PRs (that do not exist
by now but will exit in the future) are done, we can then remove
`_obtain_hostname_and_domainname` because we're already writing the
variables to `/etc/dms-settings`. I left instructions in the script in
the form of TODO comments.
Because we now log the date for all messages of the changedetector, we
need to `tail` a bit more log than before.
* refactored `daemon-stack.sh`
A new method was introduced to uniformaly start daemons and log output
accordingly. The methods for daemon start were renamed (plural ->
singular), therefore the adjustments in `start-mailserver.sh`.
* cleaned Fetchmail setup from `daemon-stack.sh`
Not sure why, but the Fetchmail setup was somehow happening in
`daemon-stack.sh` - this is not supposed to be the case. I relocated the
setup into `setup-stack.sh`, where it belong.
* delete old, unnecessary script in `target/bin/`
These are unused leftovers from the last commit, that relocated the
setup of Fetchmail into `setup.stack.sh`.
* corrected changedetector function name
* Apply suggestions from code review
* adjusted `debug-fetchmail` script
It is absolutely fine to source `setup-stack.sh` because sourcing the
script does not execute a single function (by desing of the script).
This way, we retain functionality.
* praise be ShellCheck
* added `log.sh` to `debug-fetchmail` as a dependency
* final cleanup
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* `update-check.sh` now uses the new log
* refactored `setup-stack.sh`
The changes are:
1. Replaced `""` wiht `''` where possible (reasoning: Bash is very
implicit and I'd like to use `''` where possible to indicate no
variables are expanded here)
2. `> /file` -> `>/file` according to our style guide
3. Some log adjustments for messages where I deemed it appropriate
4. Then, an error message from a Dovecot setup was also prevented (by
adding a check whether the directory is present before a `: >...`
command would create a file in this directory).
These are all small, miscellaneous changes that I wanted to combine into
one commit and ultimately one PR because I see no point in opening a PR
for every small change here. I hope this is fine.
* added a small `sleep` to the `_shutdown` function
This ensure the last log message is actually logged before Supervisor
logs the message that it received a SIGTERM. This makes reading the log
easier because now the causal relationship is shown (we are terminating
Supervisor, and not someone else and we're just logging it).
I forgot to replace `""` with `''` in `update-check.sh`, so I included
it here because this is the last commit before PR review.
* re-add exit on successful update (only)
* re-added date information to update-check log messages
* added `_log_with_date` function
The new function will log a message with a proper timestamp. This is all
handled in `log.sh`, we therefore not need to source other files too.
This will be used in the future by `check-for-changes.sh` as well :)
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* refactored scripts located under `target/bin/`
The scripts under `target/bin/` now use the new log and I replaced some
`""` with `''` on the way. The functionality stays the same, this mostly
style and log.
* corrected fail2ban (script and tests)
* corrected OpenDKIM log output in tests
* reverted (some) changes to `sedfile`
Moreover, a few messages for BATS were streamlined and a regression in
the linting script reverted.
* apple PR feedback
* improve log output from `fail2ban` script
The new output has a single, clear message with the '[ ERROR ] '
prefix, and then output that explains the error afterwards. This is
coherent with the logging style which should be used while providing
more information than just a single line about IPTables not functioning.
* simplified `setquota` script
* consistently named the `__usage` function
Before, scripts located under `target/bin/` were using `usage` or
`__usage`. Now, they're using `__usage` as they should.
* improved `sedfile`
With `sedfile`, we cannot use the helper functions in a nice way because
it is used early in the Dockerfile at a stage where the helper scripts
are not yet copied. The script has been adjusted to be canonical with
all the other scripts under `target/bin/`.
* fixed tests
* removed `__usage` from places where it does not belong
`__usage` is to be used on wrong user input, not on other failures as
well. This was fixed in `delquota` and `setquota`.
* apply PR review feedback
* added new `_log` function
With `_log`, the `_notify` method wa rendered obsolete. `_notify` was
not completely removed due to test failures in `check-for-changes.sh`.
The new `_log` function properly uses log levels such as `trace`,
`debug`, `info`, `warn` and `error`. It provides a cleaner solution
and renders `DMS_DEBUG` obsolete too (as only `_notify` depends on it).
* converted all helper script to new `_log` function
* converted all startup stacks to new `log` function
* `start-mailserver.sh` now uses new `_log` function
* final test and misc small script adjustments
* updated documentation
The new setup will now set env variables on one place and on one place
only. The old setup used two separate places wich is not DRY and
confusing.
Some default values changed:
1. PFLOGSUMM_TRIGGER: logrotate => none
2. REPORT_SENDER: mailserver-report@HOSTNAME => mailserver-report@DOMAIN
3. REPORT_RECIPIENT: "0" => POSTMASTER_ADDRESS
One env variable was renamed: REPORT_INTERVAL => LOGROTATE_INTERVAL
I believe these defaults to be more sensible, especially the REPORT_RECIPIENT
address. The PFLOGSUMM_TRIGGER value was changed to `none` because otherwise
people would start getting daily Postfix log summary reports automatically.
Now, this is opt-in, and reports are sent only when enabled properly.
Some of the variables changed were marked as deprecated. I removed the note,
as the variables now bear some (sane) defaults again for other variables
(i.e.) REPORT_RECIPIENT is now default for other recipient addresses.
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* get rid of subshell + exec
The new way of executing `sha512sum` should work as well as the old way
but without the clutter and possible problems the usage of subshells +
exec incurs.
Moreover, there was a misconception about array expansion. Using `""`
around an expanding array (`${ARRAY[@]}`) is quite fine (and actually
the preffered way), not because it makes the expansion _one_ string
(this would be `${ARRAY[*]}`), but it makes sure when elements are
expanded, each element has `""` around them so to speak, i.e. there is
no re-splitting of these elements.
* removed old concerns in comments
* increase test and check for changes sleep duration
The `DYNAMIC_FILES` var was quote wrapped, treating all filepaths to create checksums for as a single string that would be ignored instead of processed individually.
Removed the quotes, and changed the for loop to an array which accomplishes the same goal.
* fix: Prevent unnecessary change detection event
`acme.json` change would extract new cert files, which would then be hashed after restarting services and considered a change event, running through the logic again and restarting services once more when that was not required.
The checksum entries for those cert files are now replaced with new entries containing updated checksum hashes, after `acme.json` extraction.
Removes duplicate logic from `check-for-changes.sh` that is used/maintained elsewhere to avoid risk of problems, as this code is already starting to diverge / rot.
---
Previously the change detection support has had code added for rebuilding config upon change detection which is the same as code run during startup scripts. Unfortunately over time this has fallen out of sync. Mostly the startup scripts would get maintenance and the contributor and reviewers may not have been aware of the duplicate code handled by `check-for-changes.sh`.
That code was starting to diverge in addition to some changes in structure (_eg: relay host logic seems interleaved here vs separated out in startup scripts_). I wanted to address this before it risks becoming a much bigger headache.
Rather than bloat `helper-functions.sh` further, I've added a `helpers/` folder extracting relevant common logic between startup scripts and `changedetector`. If you want to follow that process I've kept scoped commits to make those diffs easier. Some minor changes/improvements were added but nothing significant.
---
- chore: Extract relay host logic to new `relay.sh` helper
- chore: Extract `/etc/postfix/sasl_passwd` logic to new `sasl.sh` helper
- chore: Extract `postfix-accounts.cf` logic to new `accounts.sh` helper
- chore: Extract `/etc/aliases` logic to new `aliases.sh` helper
- chore: Extract `/etc/postfix/vhost` logic to new `postfix.sh` helper
- chore: Add inline docs for Postfix configs
> These are possibly more verbose than needed and can be reduced at a later stage.
> They are helpful during this refactor process while investigating that everything is handled correctly.
`accounts.sh`:
- Add note regarding potential bug for bare domain setups with `/etc/postfix/vhost` and `mydestination` sharing same domain value.
`relay.sh`:
- Remove the tabs for a single space delimiter, revised associated comment.
- Add PR reference for original `_populate_relayhost_map` implementation which has some useful details.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* chore: Normalize container setup
Easier to grok what is different between configurations.
- Container name usage replaced with variable
- Volumes defined earlier and redeclared when relevant (only real difference is `VOLUME_LETSENCRYPT`)
- Contextual comment about the `acme.json` copy.
- Quoting `SSL_TYPE`, `SSL_DOMAIN` and `-h` values for syntax highlighting.
- Moved `-t` and `${NAME}` to separate line.
- Consistent indentation.
* chore: DRY test logic
Extracts out repeated test logic into methods
* chore: Scope configs to individual test cases (1/3)
- Preparation step for shifting out the container configs to their own scoped test cases. Split into multiple commits to ease reviewing by diffs for this change.
- Re-arrange the hostname and domain configs to match the expected order of the new test cases.
- Shuffle the hostname and domainname grouped tests into tests per container config scope.
- Collapse the `acme.json` test cases into single test case.
* chore: Scope configs to individual test cases (2/3)
- Shifts the hostname and domainname container configs into their respective scoped test cases.
- Moving the `acme.json` container config produces a less favorable diff, so is deferred to a follow-up commit.
- Test cases updated to refer to their `${CONTAINER_NAME}` var instead of the hard-coded string name.
* chore: Scope configs to individual test cases (3/3)
Final commit to shift out the container configs.
- Common vars are exported in `setup_file()` for the test cases to use without needing to repeat the declaration in each test case.
- `teardown_file()` shifts container removal at end of scoped test case.
* chore: Adapt to `common_container_setup` template
- `CONTAINER_NAME` becomes `TEST_NAME` (`common.bash` helper via `init_with_defaults`).
- `docker run ...` and related configuration is now outsourced to the `common.bash` helper, only extra args that the default template does not cover are defined in the test case.
- `TARGET_DOMAIN`establishes the domain folder name for `/etc/letsencrypt/live`.
- `_should*` methods no longer manage a `CONTAINER_NAME` arg, instead using the `TEST_NAME` global that should be valid as test is run as a sequence of test cases.
- `PRIVATE_CONFIG` and the `private_config_path ...` are now using the global `TEST_TMP_CONFIG` initialized at the start of each test case, slightly different as not locally defined/scoped like `PRIVATE_CONFIG` would be within the test case, hence the explicit choice of a different name for context.
* chore: Minor tweaks
- Test case comment descriptions.
- DRY: `docker rm -f` lines moved to `teardown()`
- Use `wait_for_service` helper instead of checking the `changedetector` script itself is running.
- There is a startup delay before the `changedetector` begins monitoring, wait until it ready event is logged.
- Added a helper to query logs for a service (useful later).
- `/bin/sh` commands reduced to `sh`.
- Change the config check to match and compare output, not number of lines returned. Provides better failure output by bats to debug against.
* chore: Add more test functions for `acme.json`
This just extracts out existing logic from the test case to functions to make the test case itself more readable/terse.
* chore: Housekeeping
No changes, just moving logic around and grouping into inline functions, with some added comments.
* chore: Switch to `example.test` certs
This also required copying the source files to match the expected letsencrypt file structure expected in the test/container usage.
* chore: Delete `test/config/letsencrypt/`
No longer necessary, using the `example.test/` certs instead.
These letsencrypt certs weren't for the domains they were used for, and of course long expired.
* chore: Housekeeping
Add more maintainer comments, rename some functions.
* tests: Expand `acme.json` extraction coverage
Finally able to add more test coverage! :)
- Two new methods to validate expected success/failure of extraction for a given FQDN.
- Added an RSA test prior to the wildcard to test a renewal simulation (just with different cert type).
- Added extra method to make sure we're detecting multiple successful change events, not just a previous logged success (false positive).
* tests: Refactor the negotiate_tls functionality
Covers all ports (except POP) and correctly tests against expected verification status with new `example.test` certs.
The `FQDN` var will be put to use in a follow-up commit.
* tests: Verify the certs contain the expected FQDNs
* chore: Extract TLS test methods into a separate helper script
Can be useful for other TLS tests to utilize.
* chore: Housekeeping
* chore: Fix test typo
There was a mismatch between the output and expected output between these two files "find key for" and "find key & cert for". Changed to "find key and/or cert for" to make the warning more clear that it's issued for either or both failure conditions.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Better logical flow, handling and inline documentation.
Despite the verbosity, it's better to make this visible here for maintenance and debugging purposes than trying to dig through issue/PR or commit history for it.
* fix: Panic when HOSTNAME is misconfigured
* chore: Add more comment docs for maintainers
* tests(fix): Use `--domainname` not ENV `DOMAINNAME`
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Previously we only monitored for `$HOSTNAME` in `/etc/letsencrypt/live` and only for hard-coded `.pem` filenames.
This ensures we check the locations of other locations that may not match `$HOSTNAME`, which we also support. Ideally in future at least the directory to look in would be better known in advance..
Split into scoped commits with messages if further details are needed, view those via the associated PR :)
**Commit Summary:**
**`check-for-changes.sh`**
- Prevent `SSL_DOMAIN` silently skipping when value has wildcard prefix `*.` (_at least this was known as a bugfix when originally committed in linked PR_).
- Improved inlined docs for maintainers.
- Additional logging for debugging.
**`helper-functions.sh:_extract_certs_from_acme`**:
- Fail if the input arg (_`$CERT_DOMAIN`, aka the FQDN_) provided for extraction is empty.
- Use `$CERT_DOMAIN` in place of `$HOSTNAME` and `$1` for a consistent value (_previously could mismatch, eg with `SSL_DOMAIN` defined_).
- The conditional is now only for handling extraction failure (_key or cert value is missing from extraction_).
- Log an actual warning or success (debug) based on outcome.
- Don't use `SSL_DOMAIN` with wildcard value for the `mkdir` letsencrypt directory name (_wildcard prefix `*.` is first stripped instead_).
**`acme_extract`** (_new python utility for `acme.json` handling_):
- Extracted out into a python script that can be treated as a utility in the `$PATH` like other helper scripts. It can now be used and optionally tested directly instead of via `helper-functions.sh`.
-Made compatible with Python 3, as Python 2 is EOL and no longer in newer versions of Debian.
Mostly cleans up the code and documents it better, although there are some minor fixes for handling `SSL_DOMAIN` ENV and additional logging added for spotting issues related to it in future when troubleshooting.
Commits are scoped with context messages for easing review if necessary. Overview of changes:
Traefik specific:
- Logic extracted out into it's own function.
- Conditional reworked to assist with debugging.
- `SSL_DOMAIN` must not be empty when attempting to extract.
- Added additional notes.
`SSL_TYPE=letsencrypt` case:
- Revised top note block.
- Correct handling for `SSL_DOMAIN`.
- Removed some unnecessary nesting.
- Less repetitive error message for `LETSENCRYPT_DOMAIN`.
- Added use of panics where appropriate (kept `return 1` so failures still exit functionality early).
- Improved inline docs.
Dovecot quota support would log auth failures when Postfix validated incoming mail to accept/reject and the `check_policy_service` for `quota-status` was queried with a recipient that was an account alias.
When Dovecot is not aware of the user account, it will not be able to check a quota and inform Postfix that everything is fine, Postfix will accept the mail and send it to Dovecot, where if the quota is exceeded will result in a bounce back to the sender. This is considered "backscatter" and can be abused by spammers forging the sender address which can get your server blacklisted.
The solution is to either disable quota support `ENABLE_QUOTAS=0`, or as a workaround, add dummy accounts to Dovecot userdb for aliases in `postfix-virtual.cf` (not `postfix-aliases.cf`), these dummy accounts will map to the real user account mailbox (real users are defined in `postfix-accounts.cf`).
The workaround is naive, in that we only check for basic 1-to-1 alias mapping to real accounts. This will still be an issue for aliases that map to another alias or multiple addresses (real or alias). Unfortunately Postfix will not expand aliases until accepting mail where this would be too late.
A better solution is to proxy the `check_policy_service` from Dovecot `quota-status` that Postfix queries in `main.cf:smtpd_recipient_restrictions`, however this requires a fair amount more of additional work and still requires an implementation to recursively query aliases for nested or multiple address mappings, which can then be forwarded to the `quota-status` service configured by Dovecot in `/etc/dovecot/conf.d/90-quota.conf`.
LDAP users are unaffected as quota support is not supported/implemented with `docker-mailserver` at this time, it is always considered disabled when using LDAP.
---
Additionally Dovecot configuration for `passdb` has been fixed to use the correct password hash scheme of `SHA512-CRYPT`.
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
This was all introduced by the original project author early on, no explanation for it.
- None of the paths they use as `argv` values exist.
- `uucp` doesn't seem relevant to the project. No justification for it, no issues or PRs in project history or codebase.
See associated PR for further details and linked resources.
* function _defunc removed
* _shutdown is better than just notify in that cases
* PANIC_TYPE 'fail-init' introduced
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
