Skip to content
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

scripts: add DKIM helper script for Rspamd #3286

Merged
merged 8 commits into from May 3, 2023
Merged

Conversation

georglauterbach
Copy link
Member

@georglauterbach georglauterbach commented Apr 24, 2023

Description

DISCLAIMER: I am sure you think I must be joking looking at the diff size, but I am really not sure how to put this into multiple PRs, because all these changes additions really belong together (updating docs without adding the new script doesn't make sense; not adding docs but only the script would be incoherent; not adding tests would result in code being deployed that is not tested at all - you see the issue). If you think this PR can be split up into multiple PRs, please let me know.


This PR is the last PR before we can go into feature-freeze for v12.1.0 Nevermind, we also need adjust F2B (revert some of the recent changes, see #3288). It adds support for DKIM signing keys via a helper script for Rspamd. The mode of operation is mostly identical to the already present open-dkim script.

As a good programmer, I added tests that show my changes additions are effective. I also updated the documentation for DKIM signing with Rspamd.


@polarathene I have not assigned you as a reviewer because I think for the script additions (and this PR mostly is one big script addition including tests) @casperklein will do a good job when it comes to reviewing. When you want to have a look at the docs changes though (0ac887b), I would not mind :) The docs preview should help, because the docs-diffs are always a bit fuzzy.


Closes #3249

Type of change

  • New feature (non-breaking change which adds functionality)
  • 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

This is like `open-dkim`, but for Rspamd. The semantics are the same. I
also took care of some nice-to-have features (like printing the contents
of the DNS record, or supplying a default comfiguration if none is
given).
@polarathene
Copy link
Member

I likely won't have time to participate in review. Just had a quick peek and noticed you're maintaining both opendkim and rspamd methods of generating the DKIM keys.

There was an agnostic method I shared earlier. However, since you plan to drop opendkim eventually, perhaps that agnostic approach is not as useful.

@georglauterbach
Copy link
Member Author

I likely won't have time to participate in review.

Hence I didn't assign you in the first place :)

Just had a quick peek and noticed you're maintaining both opendkim and rspamd methods of generating the DKIM keys.

There was an agnostic method I shared earlier. However, since you plan to drop opendkim eventually, perhaps that agnostic approach is not as useful.

Exactly :)

@georglauterbach
Copy link
Member Author

georglauterbach commented Apr 25, 2023

@casperklein this PR is rather big, hence I wanted to ask you whether you actually have time to review this. If you're very short on time, I got to admit that I feel rather confident about these changes, so you could also do a very light review. They may not be perfect, but few changes really are. Just tell me so I get the rough picture.

@casperklein
Copy link
Member

I'll review this tonight.

@georglauterbach
Copy link
Member Author

I'll review this tonight.

Very nice 🚀👍🏼

Copy link
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

Nothing big. Some change requests and some comments you might aggree with and change the code or leave it as it is..

target/bin/open-dkim Outdated Show resolved Hide resolved
target/bin/rspamd-dkim Outdated Show resolved Hide resolved
target/bin/rspamd-dkim Show resolved Hide resolved
target/bin/rspamd-dkim Outdated Show resolved Hide resolved
target/bin/rspamd-dkim Outdated Show resolved Hide resolved
georglauterbach and others added 2 commits May 1, 2023 14:54
Co-authored-by: Casper <casperklein@users.noreply.github.com>
@casperklein
Copy link
Member

Looks good, but one test is failing.

@georglauterbach
Copy link
Member Author

georglauterbach commented May 2, 2023

My tests seem to work. I changed the wording on an error message and the tests check for the exact wording.. easily corrected.

UPDATE: Done in 057e203.

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

Documentation preview for this PR is ready! 🎉

Built with commit: be78d48

@georglauterbach georglauterbach merged commit bba72da into master May 3, 2023
9 checks passed
@georglauterbach georglauterbach deleted the rspamd/dkim branch May 3, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

tracking: Stabilization of Rspamd
3 participants