feat: Config Templates for LDAP support #3524

polarathene · 2023-09-04T01:25:53Z

Description

Implements the proposed change "LDAP config override support (config files + ENV)".

This is a rather large diff to review, but the feature is basically two new helper methods with supporting template files:

Config generation / merging:
- A template file .tmpl has supported config keys with a ${KEY_NAME} as the variable to be replaced by the helper function.
- There's an additional .base template with static default values or ENV (sharing a common prefix LDAP_), that has subsequent configs (templates and optional user-provided config) with their own overrides or new settings layered over the base (via cat).
Cleanup (final output):
- Only lines with keys that are actually set with non-empty values are kept.
- Any earlier duplicate keys are removed (they were overwritten by a newer layer).

Still WIP:

Waiting on maintainer review feedback to be positive before updating relevant docs.
Open to this approach being rejected if there is a better way. I've tried to make it easy to grok and maintenance should be low.

Highlights:

Feat: New approach / feature for using within our scripts. Possibly making our need/use of _replace_by_env_in_file() helper redundant.
Fix: A bug where all LDAP configs enabled by default is unexpected. User only needed the ldap-users.cf and ldap-groups.cf Postfix LDAP tables configured, but DMS had virtual aliases and domains also configured, which was messing with their deployment. They presently workaround with an empty query_filter to opt-out. There was checks to enable based on the config existing in /etc/postfix, but at some point configs were moved into this location internally by default making those toggles ineffective.
Improvement: over the _replace_by_env_in_file() helper via a layered config with folding/merge approach. Users can provide their configs without having to copy/paste in each file the common connection settings, and still use ENV overrides without requiring the setting to be in their custom configs that we copy over internally.
Fix: Additionally, ENV that have fallbacks/defaults when unset won't accidentally override user-provided configs.
Feat: A new /tmp/docker-mailserver/ldap/ location has been added to source the new custom LDAP config support for Dovecot and Saslauthd, providing parity to the Postfix support.
Breaking:
- While defaults have been preserved for the most part, as part of the Postfix query_filter fix, these are now unset.
- Postfix related LDAP ENV support now uses the POSTFIX_ prefix, and individual configs now use the query config type (eg: ldap-users.cf => POSTFIX_USERS), instead of the prior support which only handled LDAP_QUERY_FILTER by appending a suffix that was not consistent with the query config type in the filename (eg: ldap-users.cf => LDAP_QUERY_FILTER_USER).
- The internal location for ldap-*.cf files is now /etc/postfix/ldap/*.cf. The custom config from users config volume has been left as-is for now to minimize breaking changes.

New dependencies:

zenv (350KB):
- Flexible on changing away from this, but zenv was a handy small dotenv program to properly load the .env file format as ENV.
- The scope of the ENV loaded only applies to the subprocess command it runs (envsubst) using Command::envs(). This was needed as simply sourcing the grep + sed filtered ENV would need extra work to be valid for source (eg: proper quote wrap handling of values, which may already have quotes), I didn't really want to think about any other potential gotchas 😅
  - Unlike envbed, this program properly splits at the first = using splitn(). Quote wrapped values or the lack of any is also handled correctly.
GNU envsubst (package 150KB, program only 35KB):
- Very basic functionality, we could technically source/run a script with a HereDoc to do the same transform and take that output instead.
I had considered alternatives:
- envsubst:
  - A popular Golang implementation for some extra features, but it had problems with nested parameter expansion.
  - Maintenance/development also seems stale, and binary was 2 MB.
- teller:
  - A popular Golang environment manager focused on secrets management.
  - I initially thought it's features could work some magic, but like the Golang envsubst, it struggled with variables for defaults. Since the use-case of this PR isn't related to secrets management, teller isn't likely to address the drawbacks I experienced with my experiments.
  - Maintenance/development also seems stale, and binary was 10 MB.
- envbed:
  - A rust CLI that can take a file template, take .env input (although I haven't tried it, it looks like it'd flop when value has =) or filter to an ENV prefix (but not remove it, which is needed for a common Postfix template at least).
  - Looked convenient, but nor popular or actively developed. Has a binary weight of 1.2 MB.
- dotenv-linter:
  - Not looked into this much. It's a 1.6MB lint tool with a fix feature that's meant to format .env properly, but a bit weighty for that.
  - They did split out the .env parser into a separate crate though (dotenv-lookup), if ever writing a program that needs that functionality in rust.
  - Maintenance wise looks ok, but they're not pushing out releases (9 months since last release, despite all the commits since).
- Write my own:
  - I technically have configomat-rs that could be polished up and adapted for this template instead of replacement approach. I cannot justify the time or future maintenance responsibility right now for writing and releasing my own tool 😅
  - I was especially surprised how there didn't seem to be much out there CLI-wise for filtering ENV, teller can do that but doesn't handle prefix filtering a group of ENV well and too big for only that purpose. env -i with zenv (and grep + sed) will have to do? 🤷‍♂️
- The DMS script to export ENV to file.
  - Bit awkward to leverage and as mentioned may be prone to error if value contains quotes or other gotchas?

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

polarathene · 2023-09-04T07:21:52Z

I was quite confused about this test failure, but after fixing some lint issues another lint issue was raised about the line endings of the new template files... they're using CRLF line endings as I'm presently on a Windows system 🤦‍♂️ (yay for tests and linters!)

#   `_should_have_matching_setting "${LDAP_SETTING}" /etc/postfix/ldap/users.cf' failed
#
# -- output differs --
# expected : server_host = ldap://ldap.example.test
# actual   : server_host = ldap://ldap.example.test
# --

georglauterbach

Nice changes, I have a few notes that I'd like to be resolved first :)

target/scripts/build/packages.sh

target/scripts/helpers/utils.sh

target/scripts/startup/setup.d/ldap.sh

target/scripts/helpers/utils.sh

georglauterbach

LGTM 👍🏼

georglauterbach · 2023-09-05T14:14:19Z

Serial tests time out; I just stopped them.

polarathene · 2023-09-06T10:14:35Z

target/scripts/startup/setup.d/ldap.sh

+ # Opt-out of generated config if `query_filter` was not configured:
+ if ! grep -q '^query_filter =' "${LDAP_CONFIG_FILE}"; then
+ _log 'warn' "'${LDAP_CONFIG_FILE}' is missing the 'query_filter' setting - disabling"
+
+ sed -i "s/$(_escape_for_sed "${LDAP_CONFIG_FILE}")//" /etc/postfix/main.cf
+ fi


This won't handle smtpd_sender_login_maps in spoofing.sh.

Technically we could add our own variable to main.cf for this like we've done with milters and restrictions, so that we could disable that.

Not sure if removing the file from the main.cf setting like this is the correct call either, but the alternative would be restoring the conditionals for retaining the original file based lookups that may not exist with ACCOUNT_PROVISIONER=LDAP, so perhaps this opt-out approach is the way to go?

Gotta be honst, your call here :D I am not sure as I lack a bit of context (due to a lack of time).

The LDAP support isn't as complicated as originally thought:

LDAP service has the data and receives a search query with 1 or more keys (attributes) with some value or pattern to match.

The query is like a conditional statement in that sense and supports & (aka &&) or | (aka (||). That's good enough for DMS at least.

Postfix has the ldap:/ lookup table type. Same config for each separate file that we assign in main.cf, they mostly only differ by the query_filter (search query).

Technical details aside, they function and work the same as our text file counterparts, the query just makes the content a bit more dynamic instead of the left-side column lookup in text:/ tables.

lack a bit of context (due to a lack of time)

No worries, I understand 👍

I'll be running out of time soon enough myself, just trying to avoid the LDAP support rotting away :)

polarathene · 2023-09-06T10:18:40Z

target/scripts/startup/setup.d/security/spoofing.sh

@@ -6,9 +6,9 @@ function _setup_spoof_protection() {

 if [[ ${ACCOUNT_PROVISIONER} == 'LDAP' ]]; then
 if [[ -z ${LDAP_QUERY_FILTER_SENDERS} ]]; then
- postconf 'smtpd_sender_login_maps = ldap:/etc/postfix/ldap-users.cf ldap:/etc/postfix/ldap-aliases.cf ldap:/etc/postfix/ldap-groups.cf'
+ postconf 'smtpd_sender_login_maps = ldap:/etc/postfix/ldap/users.cf ldap:/etc/postfix/ldap/aliases.cf ldap:/etc/postfix/ldap/groups.cf'


I'm also not sure about this in general. I think it was the original approach before senders.cf was contributed, and was provided as a compatibility fallback. I can check.

The result_attribute for these configs isn't necessarily going to be correct for this feature like it should be for senders.cf.

A future PR can address that though.

casperklein · 2023-09-10T09:56:07Z

fyi: I just did a quick look.. A detailed review will follow.

I think, a pure bash alternative for zenv is:

#!/bin/bash

[ $# -lt 2 ] && echo "Usage: $0 <env file> <binary> [args]" && exit 1

# Mark variables which are modified or created for export.
set -a

# source env file
source "$1"

# run binary with arguments
shift
exec "$@"

polarathene · 2023-09-11T07:46:23Z

fyi: I just did a quick look.. A detailed review will follow.

Thanks! ❤️

I wanted to give you time to have a chance to glance over it. I'm also juggling a bit elsewhere, but one other blocker for this PR will be a docs rewrite that I have as WIP and will push separately.

Once the docs PR is ready this is good to merge (a small change for senders.cf also still needs to be sorted out, as per my recent review comment).

I think, a pure bash alternative for zenv is:

# source env file
source "$1"

No, I had something like this prior to zenv.

You'd potentially have a problem with spaces/quotes to account for I think? (at least with how I'm creating the temporary .env file):

VAR_NAME=some value that isn't quote-wrapped

Additionally, since we're calling an executable (envsubst), I think the sourced ENV would need to be exported too? Thus extra code to add export before each ENV key.

These was mentioned in my PR description under the zenv dependency notes:

So zenv was chosen to not have to think about those kinda problems (present or future related changes) 😅

Just to clarify the generated .env approach:

Filtered ENV group by prefix, with the prefix stripped (defining new temporary ENV for envsubst)
env command is removing everything else. It can define new ENV as args, but you'd probably have a similar quoting concern?
zenv is used to read the .env created (technically just cat redirected as if it were a file), and exports those ENV into the scope of the envsubst command to be run.

casperklein · 2023-09-24T21:27:05Z

Sorry for the late update. Just want to let you know, that I can't review before October 😞 Maybe next weekend.

polarathene · 2023-09-25T00:50:20Z

I can't review before October 😞 Maybe next weekend.

No worries. Did you want to hold off merging until then?

I've been busy elsewhere and still need to get the associated docs update PR ready, so no rush if you'd like to review this before it's merged. I can also walk you through the PR diff if it helps.

georglauterbach · 2023-09-25T21:54:52Z

I will have a bit of time from Friday to Sunday and I can review this PR.

georglauterbach · 2023-12-04T09:24:07Z

I think there is actually no issue in releasing v14. The base image update warrants that anyway :) So merging this should be fine at all times.

williamdes · 2023-12-09T10:23:04Z

This looks quite interesting !

From a inline config via HereDoc to using a `.tmpl` file that has of all supported keys for SASLAuthd LDAP config. This additionally supports layering the ENV `.tmpl` generated config over a user-provided config. With a utility method that will ensure earlier duplicate keys are removed. The two new utilities are documented well enough to grok. `Dockerfile` and `packages.sh` updated to bring in new dependencies and provide the `.tmpl` file. `log_level` is not documented as a LDAP config key. Original PR did not explain why this key and value chosen were added.

This avoids the ENV overriding user-provided settings due to the ENV having hard-coded fallback defaults when not explicitly set.

Similar to the prior commit, this ensures default ENV fallback doesn't accidentally override explicit user-provided config settings.

Same process as described by earlier commits for SASLAuthd. To avoid introducing potential breakage, the ENV fallback convenience for `DOVECOT_PASS_FILTER` is retained.

- `postfix.base` defaults are now potentially breaking: - The `query_filter` default is common between `ldap-users.cf` and `ldap-senders.cf`, but the `mailEnabled` attribute locks it in to requiring the `postfix-book` OpenLDAP schema. Like the `result_attribute` setting, this is only set as a convenient default but not as broadly useful like the `bind` + `version` settings. - `version = 3` is required as unlike SASLAuthd and Dovecot, the default for Postfix is `2`. - `bind = yes` because we only support configuring for this in DMS? - `ldap-senders.cf` originally differed with it's `result_attribute` setting, but that default chosen looks to be more of a workaround introduced and should be more explicit? - The Postfix `.base` template does not include the four common attributes (_that Dovecot and SASLAuthd base configs do_), as the `LDAP_` prefix is presently the same (no`POSTFIX_` prefix), thus would override user config regardless.. - `sender_login_maps.ldap` doesn't exist and isn't relevant to LDAP queries (seems to be accidentally included here). `ldap-senders.cf` provides this functionality. - `ldap-senders.cf` was not supporting copying over a user-provided config, it does now. - Internal location for these Postfix generated configs is now `/etc/postfix/ldap/`.

Remove the prefix in favor of moving these files into a subdirectory: `/etc/postfix/ldap/`. Original LDAP config files provided via `Dockerfile` are now removed as the new `.tmpl` generation makes them redundant.

Generic approach to configuring Postfix with the LDAP tables in `main.cf`. Instead of opt-in when file exists (always did), opt-out when the `query_filter` is missing. This fixes a bug reported when deployment does not require LDAP queries for a lookup type, like groups. Avoids introducing misconfiguration by default.

Previously only `query_filter` had this support via an inconsistent `_${QUERY_KIND}` ENV suffix. This has been shifted to the left under the new `POSTFIX_` prefix, so that it can easily leverage the ENV prefix with config templates, layering after the generic `POSTFIX_` template. Naming is now consistent with `${QUERY_KIND}` (upper-cased). This also enables using the common `LDAP_` prefix in the Postfix `.base` template. As a part of the previous commit toggling based on presence of `query_filter`, this is now dropped from the Postfix `.base` template.

- `packages.sh` + `utils.sh` lint fix. - `.tmpl` + `.base` files column aligned. - `mail_with_ldap.bats` updated to ignore white-space between key/value entries being checked.

These files were created on Windows, linter caught the discrepancy. Now they're `LF`.

Inverse the assert to process the config file entry lookup to reduce the white-space between key and value to a consistent ` = ` which can then be compared directly to the `KEY_VALUE` input (_instead of the KV isolation dance used previously_).

Co-authored-by: Georg Lauterbach <[email protected]>

polarathene · 2024-01-03T08:10:54Z

Just a rebase with some conflicts resolved.

georglauterbach · 2024-01-03T11:14:28Z

Shall I already review this?

polarathene · 2024-01-03T11:26:32Z

Shall I already review this?

Last I recall reviews were already done and I had a bit more to address.

Main blocker has been on me providing a rewrite of the LDAP docs page to update to changes here. Probably not wise to merge this until that's handled 😅 I don't want this going stale so I'll try to address it by mid-jan. There's not much else on DMS that should need my time prioritized after the OAuth PR AFAIK.

georglauterbach · 2024-01-20T11:50:26Z

This is marked for v14.0.0. Will it be addresses for v14.0.0, or should we immediately postpone to v14.1.0?

polarathene · 2024-01-20T12:51:59Z

should we immediately postpone to v14.1.0?

It's a breaking change, must be a major release. Push out the base image update, this can wait as I chip away at the docs rewrite as time allows.

polarathene added this to the v13.0.0 milestone Sep 4, 2023

polarathene requested review from casperklein and georglauterbach September 4, 2023 01:25

polarathene self-assigned this Sep 4, 2023

polarathene force-pushed the feat/config-templates branch from 105b393 to 198b452 Compare September 4, 2023 07:12

This was referenced Sep 4, 2023

refactor: Helper function _replace_by_env_in_file() #3526

Open

fix: Ensure files are committed with eol=lf via .gitattributes #3527

Merged

georglauterbach requested changes Sep 5, 2023

View reviewed changes

polarathene commented Sep 5, 2023

View reviewed changes

target/scripts/helpers/utils.sh Show resolved Hide resolved

target/scripts/helpers/utils.sh Outdated Show resolved Hide resolved

georglauterbach previously approved these changes Sep 5, 2023

View reviewed changes

polarathene mentioned this pull request Sep 5, 2023

[TODO]: Revise LDAP support #3517

Open

polarathene commented Sep 6, 2023

View reviewed changes

polarathene mentioned this pull request Sep 16, 2023

bug report: receiving address checked against LDAP #3538

Closed

1 task

georglauterbach self-requested a review September 25, 2023 21:58

polarathene dismissed casperklein’s stale review via 7810508 December 4, 2023 01:11

polarathene mentioned this pull request Dec 9, 2023

feat: start implementing replication - Add DOVECOT_ITERATE_ATTRS/FILTER ENVs #3426

Closed

11 tasks

georglauterbach added meta/feature freeze On hold due to upcoming release process and removed meta/feature freeze On hold due to upcoming release process labels Dec 23, 2023

polarathene and others added 15 commits January 3, 2024 21:08

chore: saslauthd.conf generation - Use a base config for defaults

54eebb9

This avoids the ENV overriding user-provided settings due to the ENV having hard-coded fallback defaults when not explicitly set.

chore: saslauthd.conf generation - Use common LDAP_ ENV in base

4cc300f

Similar to the prior commit, this ensures default ENV fallback doesn't accidentally override explicit user-provided config settings.

chore: Adapt Dovecot LDAP config generation to use Config Template

b5edba6

Same process as described by earlier commits for SASLAuthd. To avoid introducing potential breakage, the ENV fallback convenience for `DOVECOT_PASS_FILTER` is retained.

chore: ldap-*cf to ldap/*.cf

067341a

Remove the prefix in favor of moving these files into a subdirectory: `/etc/postfix/ldap/`. Original LDAP config files provided via `Dockerfile` are now removed as the new `.tmpl` generation makes them redundant.

tests: Fix white-space difference

4968f4a

- `packages.sh` + `utils.sh` lint fix. - `.tmpl` + `.base` files column aligned. - `mail_with_ldap.bats` updated to ignore white-space between key/value entries being checked.

fix: New template files accidentally committed with CRLF line endings

6bc4a45

These files were created on Windows, linter caught the discrepancy. Now they're `LF`.

chore: Add new file extensions to .gitattributes

d6ae1a8

tests: Adjust KV match logic

4b02933

Inverse the assert to process the config file entry lookup to reduce the white-space between key and value to a consistent ` = ` which can then be compared directly to the `KEY_VALUE` input (_instead of the KV isolation dance used previously_).

tests: Fix LDAP test for Postfix start_tls = no

24d3ba3

Apply suggestions from code review

b007533

Co-authored-by: Georg Lauterbach <[email protected]>

Apply suggestions from code review

c811e6c

polarathene force-pushed the feat/config-templates branch from 7810508 to c811e6c Compare January 3, 2024 08:09

georglauterbach modified the milestones: v14.0.0, v15.0.0 Jan 21, 2024

georglauterbach added the meta/feature freeze On hold due to upcoming release process label May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Config Templates for LDAP support #3524

feat: Config Templates for LDAP support #3524

polarathene commented Sep 4, 2023 •

edited

polarathene commented Sep 4, 2023

georglauterbach left a comment

georglauterbach left a comment

georglauterbach commented Sep 5, 2023

polarathene Sep 6, 2023

georglauterbach Sep 7, 2023

polarathene Sep 7, 2023

polarathene Sep 6, 2023

georglauterbach Sep 7, 2023

casperklein commented Sep 10, 2023

polarathene commented Sep 11, 2023

casperklein commented Sep 24, 2023

polarathene commented Sep 25, 2023

georglauterbach commented Sep 25, 2023 •

edited

georglauterbach commented Dec 4, 2023 •

edited

williamdes commented Dec 9, 2023

polarathene commented Jan 3, 2024

georglauterbach commented Jan 3, 2024

polarathene commented Jan 3, 2024

georglauterbach commented Jan 20, 2024

polarathene commented Jan 20, 2024

feat: Config Templates for LDAP support #3524

Are you sure you want to change the base?

feat: Config Templates for LDAP support #3524

Conversation

polarathene commented Sep 4, 2023 • edited

Description

Type of change

Checklist:

polarathene commented Sep 4, 2023

georglauterbach left a comment

Choose a reason for hiding this comment

georglauterbach left a comment

Choose a reason for hiding this comment

georglauterbach commented Sep 5, 2023

polarathene Sep 6, 2023

Choose a reason for hiding this comment

georglauterbach Sep 7, 2023

Choose a reason for hiding this comment

polarathene Sep 7, 2023

Choose a reason for hiding this comment

polarathene Sep 6, 2023

Choose a reason for hiding this comment

georglauterbach Sep 7, 2023

Choose a reason for hiding this comment

casperklein commented Sep 10, 2023

polarathene commented Sep 11, 2023

casperklein commented Sep 24, 2023

polarathene commented Sep 25, 2023

georglauterbach commented Sep 25, 2023 • edited

georglauterbach commented Dec 4, 2023 • edited

williamdes commented Dec 9, 2023

polarathene commented Jan 3, 2024

georglauterbach commented Jan 3, 2024

polarathene commented Jan 3, 2024

georglauterbach commented Jan 20, 2024

polarathene commented Jan 20, 2024

polarathene commented Sep 4, 2023 •

edited

georglauterbach commented Sep 25, 2023 •

edited

georglauterbach commented Dec 4, 2023 •

edited