[PATCH] Add GnuPG encryption to email notifications

[richard@…]

Here's a modification to add ​GnuPG encryption to outgoing notifications in a mod_poython setup. It's my first python hack, so its pretty crude. There's no error checking, and constants appear magically. Here we go:

Add an import:

from subprocess import *

In the init section of class NotifyEmail replace lines 192-196 of the original source with:

        # ..and the gpg key if the user has one
        self.email_map = {}
        emails = []
        for username, name, email in self.env.get_known_users(self.db):
            if email:
                self.email_map[username] = email
                emails.append(email)
        # ..and the gpg key if the user has one
        self.gpg_map = {}
        output = Popen(["c:\Program Files\GNU\GnuPG\gpg.exe", "--homedir",
                "C:\data\gpg", "--batch", "--with-colons", "--list-keys"],
                stdout=PIPE).communicate()[0]
        lines = re.split("\n", output)
        skip = 0
        gpgkeyid = ""
        for line in lines:
            data = re.split(':', line)
            if data[0] == "pub":
                gpgkeyid = data[4]
                skip = 0
            if gpgkeyid and skip == 0 and data[0] == "uid":
                temp = data[9].rsplit('<', 1)
                tmail = temp[1].rstrip('>')
                for email in emails:
                    if email == tmail:
                        self.gpg_map[email] = gpgkeyid
                        skip = 1
                        break

In the send method replace line 397 of the original source with:

        # lets see if we need to encrypt body to one or multiple recipients
        gpgkeys = [];
        for recipient in recipients:
            if recipient in self.gpg_map:
                temp = '-r ' + self.gpg_map[recipient]
                gpgkeys.append(temp)
        value = body
        if len(gpgkeys):
            args = ["c:\Program Files\GNU\GnuPG\gpg.exe", "--homedir", "C:\data\gpg", '-e',
                                 '--batch', '--armor']
            args.extend(gpgkeys)
            proc = Popen(args, stdout=PIPE, stdin=PIPE)
            value = proc.communicate(body)[0]
        # msg = MIMEText(body, 'plain')
        msg = MIMEText(value, 'plain')

Now setup your public keys using "—homedir c:\data\gpg —import", assign ultimate trust to the keys. And that's that.

[Christian Boos]

Now you may want to retry with a proper patch and factor out the constants to a few TracIni settings (e.g. [notification] gpg_path).

But I question a bit the usefulness of such a feature… what would be the use case?

(gpg signing, I can understand, but why encrypting?)

[anonymous]

Well, lets say that an organization uses PGP encryption to secure all email because of the sensitive content of the email, or because of the hostile location of the email recipients. Or, just because the sysadmin hates the idea of dropping clear text readable postcards into the world wide email system. When such a person uses snail mail, he probably encloses it in an envelope with the idea that only the recipient will read it.

Speaking for myself, we've discovered that trac is really great for running our small company development and operations departments. In fact, trac has become the central focus of much of our daily activities. Thanks for a great product! Our organization is also not centralized, so trac notifications keep us all in touch. Our server is behind a VPN, and we use PGP for email. Since a lot of our IP has been transferred from email to trac, we want to protect it from both well meaning and hostile eyes.

Since trac is free, I also wanted to return something to the community. This is a pretty valuable hack since PGP Corp sells a server to do this for $$$$, and has deliberately crippled their $100+ desktop product to prevent the ability to generate encrypted notifications without a logged on user. I found it unacceptable to extort money for something that should be inexpensive, and only needed 40 lines of code to make free.

I'm not fully convinced that anything more needs to be done other than to have this particular hack-a-trac for trac 11.3 mod_python users located where it can be found, but, as you say, it would be fairly easy to get the constants from ini, and it would be a good exercise for me to do the patch.

Like everyone else, I'm pretty busy, so we'll see..

[ebray]

Yeah, it's not a bad idea, but it won't of use to much anyone who doesn't have GPG installed, much less at c:\Program Files\GNU\GnuPG\gpg.exe. (My gpg, for example, is /usr/bin/gpg).

[Noah Kantrowitz]

Seems like something that would make a very nice plugin once we have a notification system flexible enough to allow it.

[Remy Blank]

Replying to nkantrowitz:

Seems like something that would make a very nice plugin once we have a notification system flexible enough to allow it.

This could probably already be done on trunk by implementing an IEmailSender that pipes through GPG and delegates to SmtpEmailSender to actually send the message.

[rgent <richard@…>]

Had to upgrade the hack for 11.7 and that was no fun. The big change was moving mail server logon to happen after gpg encryption to prevent dropped connections due to mail server timeouts. Since 2.0 seems pretty far off, now looking into preparing a proper patch for 0.12 trunk as per comment:5.

[rgent <richard@…>]

IEmailSender has Smtp and Sendmail implementations (and could have more), so implementing a GpgEmailSender sender isn't elegant. Better to have an IMsgEncrypt with a GPG implementation (potential others being PGPCorp, Pkware, etc). The encryption part of IMsgEncrypt could be added directly to NotifyEmail.send. 0.12 moves Notify.begin_send directly into SmtpEmailSender.send so server timeouts due to encryption processing are eliminated (and thanks for that!). NotifyEmail.__init__ would call through IMsgEncrypt.init to implement the key caching logic from the OP. Then add constants to trac.ini similar to IEmailSender/Sendmail implementation, which solves the problem raised in comment:3.

Only basic usability problem is that new users won't get into the key cache without a server restart, but that's also a problem for NotifyEmail.email_map, so if that is already resolved, or gets fixed eventually, IMsgEncrypt's keys will also be refreshed.

A fundamental restriction would be that only one type of encryption would be allowed per project. It would be complicated to implement GPG encryption for some users in a project and, say, Pkware encryption for others in the same project.

Should have a patch for GPG encryption soon.

[Remy Blank]

How about using SendmailEmailSender and using a wrapper script around sendmail that encrypts the message before passing it to sendmail?

The notification architecture is currently being redesigned (see TracDev/Proposals/Announcer), so you might want to join the discussion there (and on trac-dev).

[rgent <richard@…>]

Sounds good, but I'm using SmtpEmailSender, so I'd need to adjust the code anyway. Thanks for the suggestion about TracDev.

[rgent <richard@…>]

Looking at the Announcer UML shows that the proper place for IMsgEncrypt would be in the decorator loop of Addl.Decorator of the preferred formatter loop, so the UML seems to handle the complexity of multiple encryption types in the same project.. and that is good. But looking at the trunk source shows IAnnouncementEmailDecorator requires weighted decorators since the order of decoration would be critical. Once encrypted, further decorations would be pointless.

Looking into the equivalent of the 0.12 Notifier.py, ​distributors/mail.py, shows that the same logic from comment:8 would allow encrypted emails without enforcing weighted decorators at the expense of multiple encryption types for the same project. Frankly, multiple encryption types for the same project seems presently irrelevant, so the proposal in comment:8 would apply to both 0.12 and the Announcer plugin. If a use case evolves where people using the Announcer plugin require multiple encryption types for the same project, weighted decorators could be explored.

[rgent <richard@…>]

Oops.. looks like someone else is already working on ​MessageEncryption for Announcer (see ​ticket 6773 over at trac-hacks). Given the proposal at TracDev/Proposals/Announcer to replace the existing notification system with Announcer, my patch for 0.12 seems unrequired.

[rgent <richard@…>]

Decided to complete the patch for 0.12 anyway. Diff is attached.

Here's my trac.ini settings from the notification section:

email_encrypt = true
email_encrypter = GpgMsgEncrypter
email_sender = SmtpEmailSender
gpg_binary = C:\Program Files\GNU\GnuPG\gpg.exe
gpg_home = C:\Data\GnuPG

Haven't tried with sendmail, but should work. Haven't tried on unix, but changing gpg_binary and gpg_home in the trac.ini should work. Couldn't find any unit tests for notification, but the patch worked for me with clear and encrypted text recipients.

[rgent <richard@…>]

Found a bug.. a break in the GpgMsgEncrypter init method would have caused only one decryption key to be identified.

Also, patch was not unified diff.

[Christian Boos]

Milestone 2.0 deleted

[Remy Blank]

The patch looks nice. Is it really necessary to cache the key IDs, i.e. is the time taken by this operation really significant compared to e.g. the time taken to encrypt the message? If not, I'd rather leave the caching out.

I wonder if we could generalize the encryption process as an INotificationMessageProcessor "pipe" for messages, where a message is passed to each processor in a list, which can return a list of processed messages that are passed to the next processor (to handle users for which the encryption key is not known, as you do in your patch), and finally, all resulting messages are passed to the IEmailSender.

Also, considering there were talks about integrating ​th:AnnouncerPlugin, I'd like to hear from other developers: is it worth integrating this patch for 0.13? I'm +1, as a nice patch is available and announcer hasn't shown progress lately, and I volunteer for the integration.

[rgent <richard@…>]

Key ID cache is not required for the implementation. The cache was created to decrease resource use and prevent repetitive processing.

From the subprocess class documentation we get: "On Windows: the Popen class uses CreateProcess() to execute the child program," and CreateProcess is known to be resource intensive. While the parse of the key file is not singularly expensive, its a repetitious loop that returns the same result on each ticket change. Caching key IDs resolves both issues.

If key caching is a serious problem, perhaps an enable\disable switch could be added to the config file.

Concerning your suggestion for an

INotificationMessageProcessor "pipe" for messages, where a message is passed to each processor in a list, which can return a list of processed messages that are passed to the next processor..

TracDev/Proposals/Announcer suggests the term decorators in its UML proposal, and your processor loop would be a partial implementation of the UML's decorator loop. I agree with the UML (provided weighted decorators are included - see comment:11), and I agree with your design suggestion.

[Remy Blank]

Replying to rgent <richard@…>:

While the parse of the key file is not singularly expensive, its a repetitious loop that returns the same result on each ticket change.

Except when it doesn't :) From comment:8:

Only basic usability problem is that new users won't get into the key cache without a server restart

(snip)

If key caching is a serious problem, perhaps an enable\disable switch could be added to the config file.

It's not a problem per se. It introduces an additional issue (having to restart the server when the key list changes) and more complexity in the code, and I just want to make sure the performance advantage is important enough to compensate for that. If it doesn't, we better leave it out.

[hasienda <hoff.st@…>]

I'm the author of GPG support for ​AnnouncerPlugin, so I'll certainly take a look at your patch. It's a pity, that I've not seen this before. I'd have certainly liked to hear from your effort earlier, but it's not too late. If you do care, get in contact with me, please (see my trac-hacks author page for details).

I'm about to revive the aforementioned proposal with some code to actually do the integration. I'd appreciate help in any form. Maybe you're interested to work with us?

[rgent <richard@…>]

Replying to rblank:

Replying to rgent <richard@…>:

While the parse of the key file is not singularly expensive, its a repetitious loop that returns the same result on each ticket change.

Except when it doesn't :) From comment:8:

Only basic usability problem is that new users won't get into the key cache without a server restart

Continuing comment:8

..but that's also a problem for NotifyEmail.email_map

Which is still the same in 0.13. So if we're caching the email_map for known users, without manipulation somewhere, then new users won't get into the notification queue without a server restart either. So if email_map is adjusted someplace else, or NotifyEmail.__init__ gets re-called when a new user email becomes available, the key cache can\will be refilled. That restricts encryption to the user base, not all emails that might appear in the ticket - which makes sense since we'd have to add the user's key to a local key file, leaving anonymous encryption as a much bigger task.

(snip)

If key caching is a serious problem, perhaps an enable\disable switch could be added to the config file.

It's not a problem per se. It introduces an additional issue (having to restart the server when the key list changes) and more complexity in the code, and I just want to make sure the performance advantage is important enough to compensate for that. If it doesn't, we better leave it out.

Entirely your call.

[rgent <richard@…>]

Replying to hasienda <hoff.st@…>:

I'm the author of GPG support..

<snip>

As mentioned in comment:11, the patch would need minor changes to be applied to distributors\mail.py in the Announcer plugin. The patch was mostly a learning exercise with the added benefit of saving me the trouble of manually editing source code on every release of trac.

If you've got something specific in mind that I could help with, I'll gladly take a look, but there's a lot going on, so can't promise that anything will get done.

[Remy Blank]

Replying to rgent <richard@…>:

..but that's also a problem for NotifyEmail.email_map

Which is still the same in 0.13. So if we're caching the email_map for known users, without manipulation somewhere, then new users won't get into the notification queue without a server restart either.

I must be missing something. Where do you see NotifyEmail.email_map cached? Sure, it's created in the constructor, but NotifyEmail is not a Component, it's a "plain" class, and it gets instantiated for every e-mail that is sent (see e.g. TicketModule._do_save()​).

[rgent <richard@…>]

My bad. I moved encrypter init into a component, and forgot that NotifyEmail was not a singleton. Of course the cache can go. The read of the key file should probably stay in the c'tor, and the key file parse loop should be rewritten to be more efficient.

Is that something you would do at this stage, or do you want me to resubmit the patch?

[Remy Blank]

Replying to rgent <richard@…>:

Is that something you would do at this stage, or do you want me to resubmit the patch?

It would be great if you could submit an updated patch. OTOH, I'm still waiting for opinions whether this should be integrated as-is, or if Announcer is still meant to be integrated soon-ish, so you may want to wait for that decision before investing more time into it.

[rgent <richard@…>]

All done. Even better if patch does not get integrated because Announcer is integrated. I use the patch in production, so I'm committed either way.

[hasienda <hoff.st@…>]

Replying to rblank:

Replying to rgent <richard@…>:

Is that something you would do at this stage, or do you want me to resubmit the patch?

It would be great if you could submit an updated patch. OTOH, I'm still waiting for opinions whether this should be integrated as-is, or if Announcer is still meant to be integrated soon-ish, […]

Well, I'm already trying to contact the maintainer of Announcer about this. As the current implementation of GnuPG support for Announcer is out but not finished in my eyes (while already used in production as well), there is still much work to be done for both, actual integration of Announcer with Trac (replacing TracNotification) and improving the new crypto part. This is certainly not done within some days.

While the patch here certainly has anything needed to just do the mentioned use case, I'd certainly appreciate a more feature-full implementation of GnuPG support (managing multiple keys per user, allow for restriction to not send anything unencrypted, etc.). And as we're about to add an optional dependency (GnuPG binary) it can't hurt to have a ready-made wrapper as another one. I don't feel as reluctant as the OP to just interface to the GnuPG binary on my own but let a matured wrapper handle basic interaction including error checking (see ​related development documentation for more details).

I'm moving towards this stuff now, but given my Python skills and time both limiting the progress considerably I'll certainly need some help, especially with code review and testing. I.e. unit tests are something I've still to explore.

[n-roeser@…]

Adding myself to Cc. Without a comment, this change is rejected as potential spam. :-(

[rgent <richard@…>]

Replying to hasienda <hoff.st@…>:

<snip>

..but let a matured wrapper handle basic interaction including error checking (see ​related development documentation for more details).

I'm moving towards this stuff now, but given my Python skills and time both limiting the progress considerably I'll certainly need some help, especially with code review and testing. I.e. unit tests are something I've still to explore.

Most of the naming conventions and basic ideas were lifted from your encryption notes, so thanks for that. As for the help, I'll extend my development environment to include the Announcer plugin, and post Announcer specific message encryption notes in ​th:ticket:6773.

[rgent <richard@…>]

performance improvement in gpg key file parse

[rgent <richard@…>]

Ok, attachment:notification.0.13.3.diff​ is the latest (probably final) version. Last few diffs were performance improvements to the key file parse. According to ​gpg cvs "DETAILS", the number of fields in rows is not fixed, but in the case of uid, the documentation says:

1. Field:  Type of record
    ...
    uid = user id (only field 10 is used).

Also, added "—fixed-list-mode" flag to separate first uid from others.

[rgent <richard@…>]

Removed an unused var and replaced an if with elif. attachment:notification.0.13.4.diff​ is final candidate.

[Thijs Triemstra <lists@…>]

I'm not able to apply this patch using patch -p0, can you attach a file with the output of svn diff?

[rgent <richard@…>]

Try this with patch -p0.

[Thijs Triemstra]

Thanks.

[srevill@…]

Hi. I've created a Trac 0.12.2 install in a CentOS 5.5 VM to test this ​ notification.0.13.patch. I've got GnuPG installed with the pub keys for some test users and it appears to be working apart from one small detail: the body text in the encrypted notification email is empty. There's no encrypted stuff, just one or maybe two linefeeds.

Here's the output that may be relevant in the log file and it all looks OK to me:

2011-03-24 18:17:15,105 Trac[notification] INFO: Mapping GnuPG keys to emails using /usr/bin/gpg with home /home/trac/.gnupg
2011-03-24 18:17:15,135 Trac[notification] INFO: Encrypting email to: [u'srevill@endurancetech.co.uk', u'amoyler@endurancetech.co.uk', u'rnicoll@endurancetech.co.uk']
2011-03-24 18:17:15,136 Trac[notification] INFO: Preparing clear text for: []
2011-03-24 18:17:15,157 Trac[notification] INFO: Sending notification through SMTP at localhost:25 to [u'srevill@endurancetech.co.uk', u'amoyler@endurancetech.co.uk', u'rnicoll@endurancetech.co.uk']

Notes: I'm srevill and I assigned the test ticket to myself. rnicoll and amoyler are people who are CC'd on the ticket. All three of us have public keys in the gpg keyring.

Any ideas?

[anonymous]

Upon further investigation, I'm also seeing this in the log:

Trac[web_ui] ERROR: Failure sending notification on change to ticket #315: OSError: [Errno 32] Broken pipe

The email notifications were working before I added the extra stuff to the trac.ini to activate the patch:

email_encrypt = true
email_encrypter = GpgMsgEncrypter
email_sender = SmtpEmailSender
gpg_binary = /usr/bin/gpg
gpg_home = /home/trac/.gnupg

[srevill@…]

One last post: it seems that the empty body text notification email is sent for the first update on the ticket since starting that Trac environment. All subsequent ticket updates fail with the "Broken pipe" error and don't send any notification email at all. Restarting Trac seems to get back into the initial state.

[rgent <richard@…>]

Did you assign ultimate trust to the keys?

Like gpg --homedir /home/trac/.gnupg --edit-key srevill@endurancetech.co.uk trust.

[anonymous]

No, I assigned "full" trust. I'll give that a go and see what happens. Thanks.

[srevill@…]

Nice one! That was indeed the problem. I suppose gnupg was sitting there waiting for someone to press 'y' on the keyboard for it before! Oops.

[rgent <richard@…>]

trac 1.0.1 patched replacement for notification.py

[rgent <richard@…>]

I've upgraded my trac installation from 0.12 to 1.0.1. This required a new notification patch for GPG.

attachment:notification.1.0.1.py​ is an already patched replacement for version 1.0.1 trac/notification.py, as in not a diff\patch. You should edit your sites notification.py and completely replace the contents with notification.1.0.1.py.

If required, I can attach a patch for 1.1.x trunk.