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: Create bare new test file `setup-cli.bats`
Bare minimum to setup a new test.
* chore: Transfer over relevant tests
* chore: `mail` container name to dynamic `${TEST_NAME}`
Only applied where it's relevant. Next commit will handle the config path correction.
* chore: Use `TEST_TMP_CONFIG` for referencing local config directory
Could technically use the existing function call. Some paths were using a hard-coded config location.
Both have been converted to `TEST_TMP_CONFIG` and related `grep` calls normalizing the quote mark usage, escaping doesn't seem necessary.
* tests(fix): Create container without providing extra args reference var
If a variable name (of an array) was not provided to reference, this would fail trying to reference `'`.
* chore: Remove `SYS_PTRACE` capability from docs and configs
* chore: Remove `SYS_PTRACE` capability from tests
Doesn't seem to be required. It was originally added when the original change detection feature PR apparently needed it to function.
* 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.
* tests(fix): Increase some timeouts
Running tests locally via a VM these tests would fail sometimes due to the time from being queued and Amavis actually processing being roughly around 30 seconds.
There should be no harm in raising this to 60 seconds, other than delaying a failure case which will ripple through other time sensitive tests.
It's better to pass when functionality is actually correct but just needs a bit longer to complete.
* tests(fix): Don't setup an invalid hostname
During container startup `helpers/dns.sh` would panic with `hostname -f` failing.
Dropping `--domainname` for this container is fine and does not affect the point of it's test.
---
It's unclear why this does not occur in CI. Possibly changes within the docker daemon since as CI runs docker on Ubuntu 20.04? (2020).
For clarity, this may be equivalent to setting a hostname of `domain.com.domain.com`, or `--hostname` value truncated the NIS domain (`--domainname`) of the same value.
IIRC, it would still fail with both options using different values if `--hostname` was multi-label. I believe I've documented how non-deterministic these options can be across different environments.
`--hostname` should be preferred. There doesn't seem to be any reason to actually need `--domainname` (which is NIS domain name, unrelated to the DNS domain name). We still need to properly investigate reworking our ENV support that `dns.sh` manages.
---
Containers were also not removing themselves after failures either (missing teardown). Which would cause problems when running tests again.
* chore: Normalize white-space
Sets a consistent indent size of 2 spaces. Previously this varied a fair bit, sometimes with tabs or mixed tabs and spaces.
Some formatting with blank lines.
Easier to review with white-space in diff ignored. Some minor edits besides blank lines, but no change in functionality.
* fix: `setup.sh` target container under test
Some of the `setup.sh` commands did not specify the container which was problematic if another `docker-mailserver` container was running, causing test failures.
This probably doesn't help with `test/no_container.bats`, but at least prevents `test/tests.bats` failing at this point.
* 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.
