Description

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:

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.

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.

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.

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.

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.

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.

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.

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.

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?

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.

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.

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()​).

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?

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.

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.

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.

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.

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:

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.

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.

assignThe owner will be changed from eblot to anonymous. Next status will be 'assigned'.

Add Comment

This ticket has been modified since you started editing. You should review the
other modifications which have been appended above,
and any conflicts shown in the preview below.
You can nevertheless proceed and submit your changes if you wish so.