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

Add SpamAssassin KAM #2418

Merged
merged 11 commits into from Feb 21, 2022
Merged

Add SpamAssassin KAM #2418

merged 11 commits into from Feb 21, 2022

Conversation

georglauterbach
Copy link
Member

Description

Adds KAM (https://mcgrail.com/template/projects#KAM1) to SpamAssassin (SA) if SA is enabled. If not, there is no effect.

Closes #2412

Type of change

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

If SpamAssassin is enabled, we can also enable KAM with a new
environment variable (`ENABLE_SPAMASSASSIN_KAM`). This is a simple
addition to the mail server.
@georglauterbach georglauterbach added kind/new feature A new feature is requested in this issue or implemeted with this PR priority/low area/features service/security/spamassassin labels Feb 18, 2022
@georglauterbach georglauterbach requested a review from a team February 18, 2022 11:45
@georglauterbach georglauterbach self-assigned this Feb 18, 2022
@casperklein
Copy link
Member

I can only speak for myself, but I never heared about KAM before the mentioned issue. So a quick explanation what KAM is or at least a link to the homepage/documentation should be in environment.md and 'mailserver.env.

@georglauterbach
Copy link
Member Author

I can only speak for myself, but I never heared about KAM before the mentioned issue. So a quick explanation what KAM is or at least a link to the homepage/documentation should be in environment.md and 'mailserver.env.

I will add this. I can, if you like, change the default to 0 too, if you think this is more appropriate?

docs/content/config/environment.md Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
@casperklein
Copy link
Member

I will add this.

Oh, I didn't see that you answered meanwhile 😉

@casperklein
Copy link
Member

casperklein commented Feb 18, 2022

I will add this. I can, if you like, change the default to 0 too, if you think this is more appropriate?

I think that's a good idea. I don't use KAM and don't see a reason to do so at the moment. I am very satisfied, how SA performs in the current state. Enabling KAM per default might lead to new problems / more false positives for new users.

georglauterbach and others added 2 commits February 18, 2022 13:46
Co-authored-by: Casper <casperklein@users.noreply.github.com>
The Dockerfile was lacking a `&&` and CI failed because of this.
mailserver.env Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
georglauterbach and others added 2 commits February 19, 2022 10:22
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
casperklein
casperklein previously approved these changes Feb 19, 2022
target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 1fa81ef

@georglauterbach georglauterbach merged commit 2927cc4 into master Feb 21, 2022
@georglauterbach georglauterbach deleted the feature/add-SA-KAM branch February 21, 2022 09:48
@ghnp5
Copy link
Contributor

ghnp5 commented Feb 22, 2022

Hey @georglauterbach
Thanks for this!

Did you test sa-update on this?

For some reason, --gpgkey 24C063D8 doesn't work for me.
When there is an actual update, sa-update it fails with:

 The update downloaded successfully, but it was not signed with a trusted GPG
 key.  Instead, it was signed with the following keys:
     24C063D8 

So I had to change the command to --nogpg.

sa-update --nogpg --channel kam.sa-channels.mcgrail.com --gpghomedir /var/lib/spamassassin/sa-update-keys -v 2>&1

@georglauterbach
Copy link
Member Author

georglauterbach commented Feb 22, 2022

When there is an actual update, sa-update it fails with:

 The update downloaded successfully, but it was not signed with a trusted GPG

 key.  Instead, it was signed with the following keys:

     24C063D8 

So I had to change the command to --nogpg.

sa-update --nogpg --channel kam.sa-channels.mcgrail.com --gpghomedir /var/lib/spamassassin/sa-update-keys -v 2>&1

I experienced a similar issue, but (as I did with this PR) I did not use --nogpg, but I removed the --gpghomedir /var/lib/spamassassin/sa-update-keys option which was messing with the command. Without this option, all should work well. This PR makes use of that as well.

@ghnp5
Copy link
Contributor

ghnp5 commented Feb 22, 2022

ah nice one! Thanks very much. 👍👍

cat >"${SPAMASSASSIN_KAM_CRON_FILE}" <<"EOM"
#! /bin/bash

sa-update --gpgkey 24C063D8 --channel kam.sa-channels.mcgrail.com && \
Copy link
Member

Choose a reason for hiding this comment

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

the GPG key is a bit small, could a full one be used ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you find the complete key, please, feel free to open a PR :D

Copy link
Member

Choose a reason for hiding this comment

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

https://mcgrail.com/template/kam.cf_channel
I will let you do it if you want ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

The link you posted shows the very same key this PR uses...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but for some reason they do use a short version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this to be fine :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/features kind/new feature A new feature is requested in this issue or implemeted with this PR priority/low service/security/spamassassin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SpamAssassin - KAM channel auto-update
5 participants