Rspamd: add check for DKIM private key files' permissions

[georglauterbach]

Description

The newly added function __rspamd__check_dkim_permissions performs a check on DKIM private key files. This is useful to prevent issues like #3621 in the future. The function is deliberately kept simple and may not catch every single misconfiguration in terms of permissions and ownership, but it should be quite accurate.

Please note that the Rspamd setup does NOT change at all, and the checks will not abort the setup in case they fail. A simple warning is emmited.

Reviewing commit-by-commit is advised.

Fixes #3621

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

[polarathene]

Just some suggestions.

The reference to the DKIM TODO issue and changes related to that aren't necessary to implement here, just conveying a different perspective / improvement.

[polarathene]

Might be better handled in future by new proposed approach with DKIM files generated having a complimentary rspamd.k or rspamd.yaml config beside the keypair files (although the proposed file layout covers a fair amount of that information itself):

• [TODO]: Consider revising DKIM generation #3630
• misc: final Rspamd adjustments for v13 #3599 (comment)
• misc: final Rspamd adjustments for v13 #3599 (comment)
• https://github.com/orgs/kcl-lang/discussions/804#discussioncomment-7430551

Example fetches the separate configs and produces output YAML (dynamic on-demand generation for rspamd config instead of manually editing/updating the domain section):

$ docker run --rm --pull=always -v "/tmp/config:/config:ro" kcllang/kcl:main bash -c \
  'find /config/dkim/keys -type f -name "rspamd.k" | xargs kcl /config/main.k'

main: Pulling from kcllang/kcl
Digest: sha256:094150067c1c1079e320dc1f6a1c2ed68dcf92d09d1f428e8dd760c96f619ab6
Status: Image is up to date for kcllang/kcl:main

domain:
  example.com:
    selectors:
    - path: /config/dkim/keys/example.com/mail-ec/private.pem
      selector: mail-ec
    - path: /config/dkim/keys/example.com/mail-rsa/private.pem
      selector: mail-rsa
  example.org:
    selectors:
    - path: /config/dkim/keys/example.org/mail-rsa/private.pem
      selector: mail-rsa

Alternative example fetches the file paths of the separate configs to generate a YAML list as config:

# Find the `rspamd.k` files and output a YAML list (filepath prefixed with `- `) as input into `yq`:
# NOTE: Just pipe to `yq` with nothing else after to get a pure YAML list from the stdin input
$ find /config/dkim/keys -type f -name "rspamd.k" -exec printf '- %s\n' {} + \
  | yq '{ "kcl_cli_configs": { "files": . } }'

kcl_cli_configs:
  files:
    - /config/dkim/keys/example.com/mail-ec/rspamd.k
    - /config/dkim/keys/example.com/mail-rsa/rspamd.k
    - /config/dkim/keys/example.org/mail-rsa/rspamd.k

Easy to generate and iterate over in bash similar to what you're doing above if needed (the .k / KCL files can be ignored and stick to plain YAML, unless you want config features like immutability).

[georglauterbach]

Just as a note for me: will be deferred to a future PR where DKIM is reworked.

[polarathene]

Probably a better way to handle that check than running cat like this? @casperklein would probably have good input here.

Presumably using stat to check the expected ownership/permissions is what you expect is best (and more informative to maintainers?).

If #3630 was implemented (at least the proposed filesystem layout for keypair files), you could have the setup scripts copy those over to an internal location and this whole concern would be a non-issue making this check redundant?

check-for-changes.sh could additionally monitor the config volume should the keypair change, copying over the update and restarting/reloading the opendkim/rspamd service if needed?

[georglauterbach]

Probably a better way to handle that check than running cat like this? @casperklein would probably have good input here.

Presumably using stat to check the expected ownership/permissions is what you expect is best (and more informative to maintainers?).

This could indeed be more elaborate; but as I said, it ought to be simple for now. We could use stat indeed, but then we'd need logic to check all possible cases - remember, we just want the _rspamd user to be able to read these files. Matching against permissions is more complicated here. As you also said yourself:

If #3630 was implemented (at least the proposed filesystem layout for keypair files), you could have the setup scripts copy those over to an internal location and this whole concern would be a non-issue making this check redundant?

I want this piece of code to be easy so that we can throw it away once #3630 is implemented. Until then, I actually think this addition is worthwhile.

check-for-changes.sh could additionally monitor the config volume should the keypair change, copying over the update and restarting/reloading the opendkim/rspamd service if needed?

I like this idea! 🚀

[polarathene]

We could use stat indeed, but then we'd need logic to check all possible cases - remember, we just want the _rspamd user to be able to read these files. Matching against permissions is more complicated here.

Earlier suggestions

Check all possible cases?

If you want to ensure the file has specific octal permission (%a), or user (%U) / group (%G):

if [[ $(stat --format '%a %U %G' "${FILE}" 2>/dev/null) != '775 _rspamd docker' ]]; then
  ...
fi

That could work if any of those would be reliable values to match (easier when we have an internal copy since we can ensure it's copied over with ownership/permissions that we'd expect anyway).

---

Alternatively, we could do the check that Postfix has with postfix check (as noted in a recent PR review comment) for file permissions with find:

if [[ $(find "${FILE}" -perm -ugo=r 2>/dev/null) ]]; then
  ...
fi

This uses symbolic notation instead of octal, bit easier to grok, but -444 would be equivalent AFAIK. The - prefix for the value ensures that the file has these baseline permission for all user/group/other ownership (minimum perms, rather than for any ownership or exact permissions):

-perm MASK      At least one mask bit (+MASK), all bits (-MASK),
                        or exactly MASK bits are set in file's mode

That would still better communicate the expectation of the file being readable. Although I get that your expectation is laxed, the file could have user or group ownership for _rspamd and thus not need to be readable by everybody, but that would also be valid...and yet you can't use +ugo=r (at least one is readable) since that may not apply to _rspamd 🤷‍♂️

---

This should technically be valid:

# Internally _rspamd would only be valid as user/group,
# otherwise require read permission for `other` perms:
if [[ $(find "${FILE}" -user _rspamd -or -group _rspamd -or -perm -o=r 2>/dev/null) ]]; then
  ...
fi

This could indeed be more elaborate; but as I said, it ought to be simple for now.
I want this piece of code to be easy so that we can throw it away once #3630 is implemented.

If that's likely to be addressed before the end of the year, that seems fine 👍

Otherwise the find approach seems simple enough and conveys expectation a bit better?

Suggested change

	if __do_as_rspamd_user cat "${FILE}" &>/dev/null; then
	__rspamd__log 'trace' "DKIM file '${FILE}' permissions and ownership appear correct"
	if [[ $(find "${FILE}" -user _rspamd -or -group _rspamd -or -perm -o=r 2>/dev/null) ]]; then
	__rspamd__log 'trace' "DKIM file '${FILE}' permissions and ownership appear correct"

[georglauterbach]

You proposal works on the (quite awful) Bash logic in which if [[ ${EMPTY} ]]; then evaluates to false and if [[ ${NOT_EMPTY} ]]; then evaluates to true, but I like the concept, so I changed it:

# See https://serverfault.com/a/829314 for an explanation on `-exec false {} +`
# We additionally resolve symbolic links to check the permissions of the actual files
if find "$(realpath -eL "${FILE}")" -user _rspamd -or -group _rspamd -or -perm -o=r -exec false {} +; then
  __rspamd__log 'warn' "Rspamd DKIM private key file '${FILE}' does not appear to have correct permissions/ownership for Rspamd to use it"
else
  __rspamd__log 'trace' "DKIM file '${FILE}' permissions and ownership appear correct"
fi

PS: Looking at both approaches, I don't know which of the two is worse 😆

[polarathene]

LGTM 👍

Only one optional suggestion that uses find on the files to check if user/group is _rspamd or permissions for other users has read access. Simple enough and communicates intent a bit better.

Suggestion is optional since in the near future the logic will be discarded in favor of an internal copy where we can adjust ownership/permissions as we see fit, and leverage check-for-changes.sh support.

[georglauterbach]

I applied changes similar to your suggestion, see #3627 (comment). We should be set now 🚀

[polarathene]

LGTM 👍 (sorry for the delay!)