Skip to content

added plausibility checks for milter insertion#3111

Closed
georglauterbach wants to merge 3 commits intomasterfrom
fix/milter-insertions
Closed

added plausibility checks for milter insertion#3111
georglauterbach wants to merge 3 commits intomasterfrom
fix/milter-insertions

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

Description

Makes the insertion of milters more robust.

Fixes #3109

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

@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 23, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Feb 23, 2023
@georglauterbach georglauterbach self-assigned this Feb 23, 2023
Comment thread target/scripts/helpers/setup-rspamd.sh
Comment on lines +41 to +47
if grep -q -E '^smtpd_milters.*=.*rspamd_milter' /etc/postfix/main.cf
then
_log 'warn' "'smtpd_milters' already contains Rspamd milter, it will not be added twice - likely an inconsistency (did you run docker compose down properly?)"
else
# shellcheck disable=SC2016
sed -i -E 's|^(smtpd_milters =.*)|\1 \$rspamd_milter|g' /etc/postfix/main.cf
fi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A warning like that doesn't seem necessary to communicate?

Ignore, postconf is a smarter approach

The DB helper has a method that could be re-used for checking duplicates I think?:

# Internal method for: _db_operation
function __db_list_already_contains_value
{
# Avoids accidentally matching a substring (case-insensitive acceptable):
# 1. Extract the current value of the entry (`\1`),
# 2. If a value list, split into separate lines (`\n`+`g`) at V_DELIMITER,
# 3. Check each line for an exact match of the target VALUE
sed -e "s/^${KEY_LOOKUP}\(.*\)/\1/" \
-e "s/${V_DELIMITER}/\n/g" \
"${DATABASE}" | grep -qi "^${_VALUE_}$"
}

but we could also do this:

# Method to update settings while avoiding adding duplicates
function _maincf_append_value_to_key_if_missing()
{
  local KEY=$1
  local VALUE_TO_APPEND=$2

  # Extract value of key from main.cf:
  local VALUE=$(sed -nE "s/^${KEY} = (.*)/\1/p" /etc/postfix/main.cf)

  if ! grep -q -F "${VALUE_TO_APPEND}" <<< "${VALUE}"
  then
    postconf "${KEY} = ${VALUE} ${VALUE_TO_APPEND}"
  fi
}

# ....

_maincf_append_value_to_key_if_missing 'smtpd_milters' '$rspamd_milter'
_maincf_append_value_to_key_if_missing 'smtpd_milters' '$dmarc_milter'
_maincf_append_value_to_key_if_missing 'smtpd_milters' '$dkim_milter'
_maincf_append_value_to_key_if_missing 'non_smtpd_milters' '$dkim_milter'

Names of things could perhaps be better, but simple enough to re-use:

  • Give it a key and a value to append
  • If there's a match for that key, then append the value via postconf 😎 (it'll ensure consistent white-space formatting between key and values)

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Feb 24, 2023

I've actually committed to going a different (a bit more invasive approach). The insert-and-check functionality is not needed then. I will provide a new PR for this in a short while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] mail from client to server slow, perhaps opendkim causing delays

2 participants