-
-
Notifications
You must be signed in to change notification settings - Fork 2k
misc: final Rspamd adjustments for v13 #3599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f2312da
aa86e6e
6a69d00
46129ca
5a251e9
9f6e023
81501dc
c798c1c
752b183
a914675
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,10 @@ source /etc/dms-settings | |||||
| # usage with DMS_HOSTNAME, which should remove the need to call this: | ||||||
| _obtain_hostname_and_domainname | ||||||
|
|
||||||
| # This is a helper to properly set all Rspamd-related environment variables | ||||||
| # correctly and in one place. | ||||||
| _rspamd_get_envs | ||||||
|
|
||||||
| # verify checksum file exists; must be prepared by start-mailserver.sh | ||||||
| if [[ ! -f ${CHKSUM_FILE} ]]; then | ||||||
| _exit_with_error "'${CHKSUM_FILE}' is missing" 0 | ||||||
|
|
@@ -49,6 +53,7 @@ function _check_for_changes() { | |||||
| # Handle any changes | ||||||
| _ssl_changes | ||||||
| _postfix_dovecot_changes | ||||||
| _rspamd_changes | ||||||
|
|
||||||
| _log_with_date 'debug' 'Reloading services due to detected changes' | ||||||
|
|
||||||
|
|
@@ -174,6 +179,33 @@ function _ssl_changes() { | |||||
| # They presently have no special handling other than to trigger a change that will restart Postfix/Dovecot. | ||||||
| } | ||||||
|
|
||||||
| function _rspamd_changes() { | ||||||
| # RSPAMD_DMS_D='/tmp/docker-mailserver/rspamd' | ||||||
| if [[ ${CHANGED} =~ ${RSPAMD_DMS_D}/.* ]]; then | ||||||
|
|
||||||
| # "${RSPAMD_DMS_D}/override.d" | ||||||
| if [[ ${CHANGED} =~ ${RSPAMD_DMS_OVERRIDE_D}/.* ]]; then | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regex not necessary?
Suggested change
Also think about the very likely case, when someone mounts something like There are other places where normal string comparison is done with Regardless of this, the rest of the PR looks good 👍
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your example wouldn't work. When you match via
Can you elaborate please? I don't see why this would be very likely :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is not. That was just a joke 😆
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, I see 😆 I have always had a hard time telling irony in texts :D |
||||||
| _log_with_date 'trace' 'Rspamd - Copying configuration overrides' | ||||||
| rm "${RSPAMD_OVERRIDE_D}"/* | ||||||
| cp "${RSPAMD_DMS_OVERRIDE_D}"/* "${RSPAMD_OVERRIDE_D}" | ||||||
| fi | ||||||
|
|
||||||
| # "${RSPAMD_DMS_D}/custom-commands.conf" | ||||||
| if [[ ${CHANGED} =~ ${RSPAMD_DMS_CUSTOM_COMMANDS_F} ]]; then | ||||||
| _log_with_date 'trace' 'Rspamd - Generating new configuration from custom commands' | ||||||
| _rspamd_handle_user_modules_adjustments | ||||||
| fi | ||||||
|
|
||||||
| # "${RSPAMD_DMS_D}/dkim" | ||||||
| if [[ ${CHANGED} =~ ${RSPAMD_DMS_DKIM_D} ]]; then | ||||||
| _log_with_date 'trace' 'Rspamd - DKIM files updated' | ||||||
| fi | ||||||
|
|
||||||
| _log_with_date 'debug' 'Rspamd configuration has changed - restarting service' | ||||||
| supervisorctl restart rspamd | ||||||
| fi | ||||||
| } | ||||||
|
|
||||||
| while true; do | ||||||
| _check_for_changes | ||||||
| sleep 2 | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| #! /bin/bash | ||
|
|
||
| # shellcheck disable=SC2034 # VAR appears unused. | ||
|
|
||
| function _rspamd_get_envs() { | ||
| readonly RSPAMD_LOCAL_D='/etc/rspamd/local.d' | ||
| readonly RSPAMD_OVERRIDE_D='/etc/rspamd/override.d' | ||
|
|
||
| readonly RSPAMD_DMS_D='/tmp/docker-mailserver/rspamd' | ||
| readonly RSPAMD_DMS_DKIM_D="${RSPAMD_DMS_D}/dkim" | ||
| readonly RSPAMD_DMS_OVERRIDE_D="${RSPAMD_DMS_D}/override.d" | ||
|
|
||
| readonly RSPAMD_DMS_CUSTOM_COMMANDS_F="${RSPAMD_DMS_D}/custom-commands.conf" | ||
| } | ||
|
|
||
| # Parses `RSPAMD_DMS_CUSTOM_COMMANDS_F` and executed the directives given by the file. | ||
| # To get a detailed explanation of the commands and how the file works, visit | ||
| # https://docker-mailserver.github.io/docker-mailserver/latest/config/security/rspamd/#with-the-help-of-a-custom-file | ||
| function _rspamd_handle_user_modules_adjustments() { | ||
| # Adds an option with a corresponding value to a module, or, in case the option | ||
| # is already present, overwrites it. | ||
| # | ||
| # @param ${1} = file name in ${RSPAMD_OVERRIDE_D}/ | ||
| # @param ${2} = module name as it should appear in the log | ||
| # @param ${3} = option name in the module | ||
| # @param ${4} = value of the option | ||
| # | ||
| # ## Note | ||
| # | ||
| # While this function is currently bound to the scope of `_rspamd_handle_user_modules_adjustments`, | ||
| # it is written in a versatile way (taking 4 arguments instead of assuming `ARGUMENT2` / `ARGUMENT3` | ||
| # are set) so that it may be used elsewhere if needed. | ||
| function __add_or_replace() { | ||
| local MODULE_FILE=${1:?Module file name must be provided} | ||
| local MODULE_LOG_NAME=${2:?Module log name must be provided} | ||
| local OPTION=${3:?Option name must be provided} | ||
| local VALUE=${4:?Value belonging to an option must be provided} | ||
| # remove possible whitespace at the end (e.g., in case ${ARGUMENT3} is empty) | ||
| VALUE=${VALUE% } | ||
| local FILE="${RSPAMD_OVERRIDE_D}/${MODULE_FILE}" | ||
|
|
||
| readonly MODULE_FILE MODULE_LOG_NAME OPTION VALUE FILE | ||
|
|
||
| [[ -f ${FILE} ]] || touch "${FILE}" | ||
|
|
||
| if grep -q -E "${OPTION}.*=.*" "${FILE}"; then | ||
| __rspamd__log 'trace' "Overwriting option '${OPTION}' with value '${VALUE}' for ${MODULE_LOG_NAME}" | ||
| sed -i -E "s|([[:space:]]*${OPTION}).*|\1 = ${VALUE};|g" "${FILE}" | ||
| else | ||
| __rspamd__log 'trace' "Setting option '${OPTION}' for ${MODULE_LOG_NAME} to '${VALUE}'" | ||
| echo "${OPTION} = ${VALUE};" >>"${FILE}" | ||
| fi | ||
| } | ||
|
|
||
| # We check for usage of the previous location of the commands file. | ||
| # TODO This can be removed after the release of v14.0.0. | ||
| local RSPAMD_DMS_CUSTOM_COMMANDS_F_OLD="${RSPAMD_DMS_D}-modules.conf" | ||
| readonly RSPAMD_DMS_CUSTOM_COMMANDS_F_OLD | ||
| if [[ -f ${RSPAMD_DMS_CUSTOM_COMMANDS_F_OLD} ]]; then | ||
| _dms_panic__general "Old custom command file location '${RSPAMD_DMS_CUSTOM_COMMANDS_F_OLD}' is deprecated (use '${RSPAMD_DMS_CUSTOM_COMMANDS_F}' now)" 'Rspamd setup' | ||
| fi | ||
|
|
||
| if [[ -f "${RSPAMD_DMS_CUSTOM_COMMANDS_F}" ]]; then | ||
| __rspamd__log 'debug' "Found file '${RSPAMD_DMS_CUSTOM_COMMANDS_F}' - parsing and applying it" | ||
|
|
||
| local COMMAND ARGUMENT1 ARGUMENT2 ARGUMENT3 | ||
| while read -r COMMAND ARGUMENT1 ARGUMENT2 ARGUMENT3; do | ||
| case "${COMMAND}" in | ||
| ('disable-module') | ||
| __rspamd__helper__enable_disable_module "${ARGUMENT1}" 'false' 'override' | ||
| ;; | ||
|
|
||
| ('enable-module') | ||
| __rspamd__helper__enable_disable_module "${ARGUMENT1}" 'true' 'override' | ||
| ;; | ||
|
|
||
| ('set-option-for-module') | ||
| __add_or_replace "${ARGUMENT1}.conf" "module '${ARGUMENT1}'" "${ARGUMENT2}" "${ARGUMENT3}" | ||
| ;; | ||
|
|
||
| ('set-option-for-controller') | ||
| __add_or_replace 'worker-controller.inc' 'controller worker' "${ARGUMENT1}" "${ARGUMENT2} ${ARGUMENT3}" | ||
| ;; | ||
|
|
||
| ('set-option-for-proxy') | ||
| __add_or_replace 'worker-proxy.inc' 'proxy worker' "${ARGUMENT1}" "${ARGUMENT2} ${ARGUMENT3}" | ||
| ;; | ||
|
|
||
| ('set-common-option') | ||
| __add_or_replace 'options.inc' 'common options' "${ARGUMENT1}" "${ARGUMENT2} ${ARGUMENT3}" | ||
| ;; | ||
|
|
||
| ('add-line') | ||
| __rspamd__log 'trace' "Adding complete line to '${ARGUMENT1}'" | ||
| echo "${ARGUMENT2}${ARGUMENT3+ ${ARGUMENT3}}" >>"${RSPAMD_OVERRIDE_D}/${ARGUMENT1}" | ||
| ;; | ||
|
|
||
| (*) | ||
| __rspamd__log 'warn' "Command '${COMMAND}' is invalid" | ||
| continue | ||
| ;; | ||
| esac | ||
| done < <(_get_valid_lines_from_file "${RSPAMD_DMS_CUSTOM_COMMANDS_F}") | ||
| fi | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still potentially trigger a change detection right? As it compares to it's before/after checksums in the config volume?
It seems that DKIM keys aren't copied over to an internal location like the certs are? I'm not too concerned about that I guess 🤔
FWIW, the rspamd docs mention rspamadm template for generating configs from jinja templates if you wanted to take advantage of that 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UCL can also be JSON (easier to manipulate)
Or.. since the UCL format also supports a JSON representation, you could also just concat your default config base as yaml:
with a loop over domain blocks (which could be generated as separate files if that's better):
eg:
Then with
yqwe can take this YAML and generate a single JSON document:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistent input format transformed to rspamd single vs multi selector
We can be more consistent with the generation too, changing our
domains.yamlinto:Then we can produce the same output as above by filtering for
selectorswith only1element, and rewriting that domain entry intopath+selectorfields:Now you could handle multiple domains like the open-dkim generator supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better unified internal storage layout for dkim keys
Although due to multiple key types and selectors, a better structure might be:
The DNS
TXTrecord can be generated from thepublic.pem, assumingpublic.pemis actually just the public key. The rspamd generator like opendkim outputs the zone file record, but additionally creates another file that extracts the record value into a single line (which other DNS providers are more compatible with).With the above structure, both opendkim and rspamd should be compatible to share the source and you'd have fairly predictable/semantic paths to work with.
That is beneficial for the
dkim_signing.confdynamic generation since you can loop through it withyqto create the config:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough of dynamic
dkim_signing.confgeneration withyqIf each keypair location had an associated YAML snippet like above, you could then scan the directory tree for the
yamlfiles and merge them all together:Grab all the YAML snippets:
Now pass that into
yqand merge under thedomainfield/object:If rspamd requires single selectors to avoid arrays, additionally handle that:
Finally, set JSON as output and prepend the
base.yamlconfig:No comments support/retained due to JSON, but that is not an issue if relying on this as internal config generated at container runtime.
The
base.yamlwith comments could still exist in the container and optionally be replaced by users config volume override 🤷♂️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, UCL supports an
.include "path/to/file.conf"macro.It can handle merging of duplicates too, but not logic for the domain being a map of
path+selectoror an array of such maps underselectorsfield AFAIK?Might not be an issue if you can always use the
selectorsarray, including for single elements (I assume you can). Then you can disregard all theyq+ YAML dance I guess and just.includethe domains config(s) as separate files similar to the above approach? 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STAGING_FILESare crated by scanningRSPAMD_DMS_D(i.e./tmp/docker-mailserver/rspamd), so AFAIK it should not trigger another event (since thecpcopies to a location not checked andchowndoes not change SHA sum IIRC.Yes. This could be changed in the future if we encounter problems, but it seems to be absolutely fine for now.
Have not looked intot that yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the Rspamd DKIM proposals: wow, that was very interesting! Funnily enough, I think we should first try the
.include ...macros before dancing around withyq(even though I found theyqstuff cool). Thank you for sharing your ideas here. I will make sure to remember this location; the reason I didn't touch DKIM for Rspamd here any more than the--forceoption is that the PR is already pretty big. The--forceoption was due to a recent issue I stumbled upon. I know you'd like to merge the OpenDKIM and Rspamd DKIM scripts, and I can tackle this in the future; for now, I'd like this PR to stay as-is to not inflate it any more. For a future PR that tackles DKIM again, we back-reference to your comments here!Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah I didn't mean to update this PR to apply the review comments here, definitely a separate PR 👍
I agree the include macro would be wiser. I'd have suggested that before typing up all the
yqstuff if I had read the rest of the UCL readme first.... 😓UPDATE: I got around to trying KCL out, and while it has some nice benefits this was a scenario that it didn't handle too well.
For example, it might be nice for managing ENV / internal config with doc generation + type validation/constraints, even has linting and formatting tools 😎
KCL is primarily for defining configuration in a more featureful way, and then outputting YAML it seems. It can ingest YAML (kinda) too.
UPDATE2: KCL discussion got some feedback since and they have some fixes in a future release that improves viability of KCL for this: https://github.com/orgs/kcl-lang/discussions/804#discussioncomment-7430551