scripts: improve DKIM path scanning in Rspamd setup - #4201
Conversation
Sorry, something went wrong.
The new version uses `rspamadm configdump` to more efficiently scan the `dkim_signing` section for paths. We strip paths that contain "$" and only print the key path by using "awk".
do not copy otherwise
|
I'm happy to approve the change, but I do recall suggesting an alternative back in Dec 2023: #3702 (comment) I've not refreshed myself on the subject, but I did document a way forward here: #3778 If you have the time to have a quick look over that and let me know if that's potentially a better approach, and if it's acceptable for v15 or should be delayed, that'd be good. It may be a bit of a breaking change / inconvenience, but might be worth considering before switching rspamd to the default (which I assume may also be blocked on adopting Valkey to minimize the wider impact of anyone that has been relying on the internal Redis service?). EDIT: Looks like I added another revision in the related unified DKIM key generation task, that mentions the current rspamd logic extracting path from dkim_signing.conf and how it would differ with the suggested selector_map + path approach (no need to mess with path in scripts like is done presently, so our logic/maintenance should be simpler): #3630 (comment) |
Sorry, something went wrong.
polarathene
left a comment
There was a problem hiding this comment.
LGTM. Go with this fix for now 🚀
I'd still appreciate your opinion on adopting selector_map in future before rspamd becomes default. That'll probably need some doc changes and migration advice in changelog for existing users, which I know is a hassle 😅 (but getting it in before the release that defaults to rspamd would be good for testing/feedback)
Sorry, something went wrong.
| if find "$(realpath -L "${KEY_FILE}")" \( -user _rspamd -or -group _rspamd -or -perm -o=r \) \ | ||
| -exec false {} +; then |
There was a problem hiding this comment.
Any reason for the line split? Was it just width?
The concern handled here would be a non-issue if the keys were copied to an internal path right?
Sorry, something went wrong.
There was a problem hiding this comment.
Any reason for the line split? Was it just width?
Yes :)
The concern handled here would be a non-issue if the keys were copied to an internal path right?
Indeed!
Sorry, something went wrong.
| __rspamd__log 'warn' "Rspamd DKIM private key file '${KEY_FILE}' is configured for usage, but does not appear to exist" | ||
| fi | ||
| done | ||
| done < <(rspamadm configdump dkim_signing | grep 'path =' | grep -v -F '$' | awk '{print $3}' | tr -d ';"') |
There was a problem hiding this comment.
So the config has multiple path values to extract and check. It seems with the mentioned selector_map config, none of that would be necessary. All keys would stored with predictable hierarchy:
selector = "mail"; # These paths should technically be internal locations instead, # with data sourced from the users config volume (/tmp/docker-mailserver): path = "/tmp/docker-mailserver/rspamd/dkim/keys/$domain/$selector.private"; selector_map = "/tmp/docker-mailserver/rspamd/dkim/selector_map";
The selector_map file then maps domain with a selector:
example.com dkim-example another-domain.com another-selector
So I think the selector_map config approach would make all of this logic redundant 🤔
Sorry, something went wrong.
There was a problem hiding this comment.
I would need to have another look at your proposed, in-depth, but I guess you are right, your approach would simplify and unify our setup.
Sorry, something went wrong.
Just a reminder: this PR is a fix for #4172 only, not a rewrite of the DKIM functionality :)
I think you are absolutely right; when I got some time in the future, I will tackle your proposals. |
Sorry, something went wrong.
Description
This PR improves the algorithm to scan for DKIM paths in Rspamd'd configuration. Moreover, it fixes a small inconvenience: cp would output "Nothing to copy" because the directory contained no contents; this has been fixed as well.
Fixes #4172
Type of change
Checklist