refactor: Share a common helper (vhost builder) for sourcing domains by polarathene · Pull Request #2620 · docker-mailserver/docker-mailserver

polarathene · 2022-06-05T06:25:15Z

Description

bin/open-dkim and helpers/accounts.sh + helpers/aliases.sh have roughly duplicate code for collecting domains to process. This code is now in sync by updating methods in helpers/postfix.sh.

Commit messages (first to last) for quick reference

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?

We use this functionality for populating /etc/postfix/vhost used by Postfix main.cf:virtual_mailbox_domains setting. bin/open-dkim leverages it to acquire domains relevant to Postfix.
This PR will additionally benefit check-for-changes.sh to generate vhost config when helpers/accounts.sh or helpers/aliases.sh is not affected by the changed file. That support will be covered in refactor: Revise check-for-changes.sh #2615

Relevant history - `bin/open-dkim` arg `DOMAINS`

Additional info regarding bin/open-dkim (most of this isn't useful in the context of this PR, but helps understand the background of the alternative support via DOMAINS arg):

It's alternative DOMAINS arg support was added in Jan 2021 to support the needs of LDAP users (earlier issue from 2017 for LDAP users).
- The contributor shares in their original issue a command for their setup to retrieve the domains that they'd supply as DOMAINS.
- The DOMAINS arg replaced the need for a separate command that provided similar functionality (originally for a user with DKIM sourced via SQL, implemented Sep 2017 in Fixes #619 by adding a new command: generate-dkim-domain #690 ).
  - Allow generating DKIM key for a virtual mailbox domain #619 notes the intention was to configure DKIM for equivalent domains available in main.cf:virtual_mailbox_domains (/etc/postfix/vhost), although LDAP setups also add a 2nd table value: ldap:/etc/postfix/ldap-domains.cf (could possible source via postconf query? Unless bin/open-dkim is not meant to be dependent upon a running/configured Postfix instance).
- That PR introduced an unrelated change to the default DKIM keylength size which was bumped up from 2048-bit to 4096-bit (with little justification).
  - AFAIK 4096-bit RSA is excessive for DKIM, it's not for encryption and not damaging to past transactions? You want some strength to avoid forgery, but it's not as valuable as a root CA key.
  - Prior to that, a March 2018 PR which added support to adjust keylength size references two issues about services lacking support for key lengths larger than 1024-bit.
    - In 2022, OpenDKIM docs still document 1024-bit as the default size.
    - Debian apparently has defaulted to 2048-bit since 2017.
    - The NameCheap provider has since updated support to 2048-bit, unclear if other vendors improved support.

Type of change

Improvement (non-breaking change that does improve existing functionality)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes

polarathene · 2022-06-05T06:25:59Z

helpers/aliases.sh has a maintenance comment (which I carried over into helpers/postfix.sh):

# the `to` is important, don't delete it

Originally this FROM TO syntax was introduced in an Oct 2015 PR #26, but no explanation for to var usage. Later in April 2016, this was duplicated into opendkim support: bin/generate-dkim-config, albeit no PR (direct commit by original project author).

The maintainer comment instructing the importance of TO was added by @georglauterbach in Sep 2020 large refactor PR, no additional context in the PR discussion or commit messages.

Can we better clarify why it's important here?

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

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

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

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

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.

polarathene

Good catch with those two lint lines! 😀

No longer applicable with the switch to `_`

polarathene added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged labels Jun 5, 2022

polarathene added this to the v11.1.0 milestone Jun 5, 2022

polarathene self-assigned this Jun 5, 2022

polarathene mentioned this pull request Jun 5, 2022

refactor: Revise check-for-changes.sh #2615

Merged

5 tasks

polarathene force-pushed the refactor/share-common-vhost-builder-helper branch 2 times, most recently from b25d6d5 to 12d7921 Compare June 5, 2022 10:03

This comment was marked as outdated.

Sign in to view

polarathene force-pushed the refactor/share-common-vhost-builder-helper branch 2 times, most recently from ea1b434 to 5df0e26 Compare June 7, 2022 00:17

polarathene removed the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Jun 7, 2022

polarathene marked this pull request as ready for review June 7, 2022 00:18

polarathene marked this pull request as draft June 7, 2022 01:53

polarathene added 4 commits June 7, 2022 14:02

chore: Split vhost helper method and use filepath vars

1c277e1

- 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

0c89840

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

polarathene force-pushed the refactor/share-common-vhost-builder-helper branch from 5df0e26 to 3c44ed1 Compare June 7, 2022 02:03

polarathene marked this pull request as ready for review June 7, 2022 03:03

polarathene requested review from casperklein and georglauterbach June 7, 2022 03:03

This comment was marked as resolved.

Sign in to view

polarathene commented Jun 7, 2022

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh Outdated

Comment thread target/scripts/helpers/postfix.sh Outdated

polarathene commented Jun 7, 2022

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh Outdated

chore: Set the 2nd value as blank _

8fbc3fa

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.

georglauterbach reviewed Jun 7, 2022

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh Outdated

georglauterbach reviewed Jun 7, 2022

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh Outdated

polarathene commented Jun 7, 2022

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh Outdated

Comment thread target/scripts/helpers/postfix.sh Outdated

chore: Remove shellcheck disable lines

5feb925

No longer applicable with the switch to `_`

georglauterbach approved these changes Jun 7, 2022

View reviewed changes

Merge branch 'master' into refactor/share-common-vhost-builder-helper

aa83701

polarathene requested a review from a team June 7, 2022 23:20

Merge branch 'master' into refactor/share-common-vhost-builder-helper

f35f136

casperklein approved these changes Jun 9, 2022

View reviewed changes

polarathene merged commit e3cc627 into docker-mailserver:master Jun 9, 2022

polarathene mentioned this pull request Oct 9, 2022

[TODO]: SPOOF_PROTECTION=1 feature and Alias support improvements #2819

Open

8 tasks

polarathene mentioned this pull request Feb 5, 2023

tests(refactor): open_dkim.bats #3060

Merged

5 tasks

polarathene mentioned this pull request Aug 28, 2023

chore: Change setup config dkim default key size to 2048 (open-dkim) #3508

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Share a common helper (vhost builder) for sourcing domains#2620

refactor: Share a common helper (vhost builder) for sourcing domains#2620
polarathene merged 8 commits intodocker-mailserver:masterfrom
polarathene:refactor/share-common-vhost-builder-helper

polarathene commented Jun 5, 2022 •

edited

Loading

Uh oh!

polarathene commented Jun 5, 2022 •

edited

Loading

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

polarathene commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Relevant history - bin/open-dkim arg DOMAINS

Type of change

Checklist:

Uh oh!

polarathene commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

polarathene commented Jun 5, 2022 •

edited

Loading

Relevant history - `bin/open-dkim` arg `DOMAINS`

polarathene commented Jun 5, 2022 •

edited

Loading