enh: collector_mail_attach Decrypt GPG attachments #2623
Conversation
e3rd
force-pushed
the
mail-attach-encrypt
branch
2 times, most recently
from
57b27f8 to
b3c0b05
Compare
|
A separate OpenPGP parser (first variant) would strictly adhere to the rule "one bot one job", but the current code is not designed for nor tested with two parsers in a row. Forth variant makes less sense than the third. In third, the CSV parser could use an "OpenPGP Mixin". |
|
Great! Thanks for having investigated, sebix! :) I am then ready to be merged – or is there anything I could do better? |
There was a problem hiding this comment.
I agree with the recommendation from @sebix I like the change, it looks good - however I'd slightly suggest adding negative test to ensure proper handling GPG errors
|
Ah, unfortunately, you need to add license files for not code test files (see example https://github.com/certtools/intelmq/blob/develop/docs/static/images/Logo_Intel_MQ.png.license) |
e3rd
force-pushed
the
mail-attach-encrypt
branch
3 times, most recently
from
d1f5a25 to
22a7423
Compare
e3rd
force-pushed
the
mail-attach-encrypt
branch
from
22a7423 to
b86e313
Compare
|
tests added, license solved :) I'm done now |
|
|
||
| Files: intelmq/tests/bots/collectors/mail/gpg_ring/** | ||
| Copyright: 2025 Edvard Rejthar, CSIRT.cz <[email protected]> | ||
| License: AGPL-3.0-or-later |
| @@ -28,11 +31,19 @@ class MailAttachCollectorBot(MailCollectorBot): | |||
| mail_user: str = "<user>" | |||
| rate_limit: int = 60 | |||
| subject_regex: str = "<subject>" | |||
| decrypt: bool = False | |||
There was a problem hiding this comment.
As OpenPGP is not the only e-mail encryption method (S/MIME is also widespread), I suggest naming the parameter decrypt_openpgp or similar.
| decrypt: bool = False | |
| decrypt_openpgp: Optional[bool] = False |
|
|
||
| def init(self): | ||
| super().init() | ||
| if self.attach_regex is None: | ||
| raise InvalidArgument('attach_regex', expected='string') | ||
| raise InvalidArgument("attach_regex", expected="string") |
There was a problem hiding this comment.
I don't see how this and other formatting changes are related to the decryption. It makes the diff harder to read and the blames harder to interpret and use.
| try: | ||
| from gnupg import GPG | ||
| except ImportError: | ||
| raise MissingDependencyError("python-gnupg", ">=0.5") |
There was a problem hiding this comment.
I suggest doing the check in init (for quick user-feedback). If decyrption is activated, and the library is not installed, raise the exception.
sebix
added
feature
Our partner sends us a GPG-encrypted attachment. This PR solves this.
Before merging, let's discuss. Should it be part of a collector bot, or should it be rather a GPG parser?
Current pipeline (current PR)
Another possibility
Another possibility
Another possibility