jkraemer / mail-gpg

GPG/MIME extension for the Ruby Mail Library
MIT License
69 stars 24 forks source link

Why delivery_handler, not interceptor? #51

Open duckdalbe opened 6 years ago

duckdalbe commented 6 years ago

Is there a specific reason that mail-gpg has been implemented as a delivery_handler, instead of an interceptor?

The latter seems more appropriate to me. And it would also enable to chain it with other custom code that modifies messages, while with the delivery_handler I must implement my own code as delivery_handler that wraps mail-gpg.

Would it be an option to maybe rebuild mail-gpg as an interceptor?

Background: I want to handle specific headers in a certain way (remove them from the encrypted message) in order to protect headers as suggested by https://github.com/autocrypt/memoryhole

jkraemer commented 6 years ago

Actually I have a long-standing item on my todo list that says "look into action mailer interceptors". So if you want to give it a try, I'd be happy to get that done.

duckdalbe commented 6 years ago

Ok, I'll try.

duckdalbe commented 6 years ago

I have some working code now, but there is one big difference in behaviour of which I'm not sure if you're OK with: As an interceptor, mail-gpg must work on the very object, not a copy of it. That means, delievering a Mail::Message-object changes it:

mail = Mail.new { subject 'test'; body 'blabla'; from: 'me@example.org'; to 'you@example.org' }
mail.gpg encrypt: true
puts mail.body.to_s
=> "blabla"
mail.deliver
puts mail.body.to_s
=> "----- BEGIN PGP MESSAGE -----\n ..."

People who use mail-gpg in loops, e.g. to send a message to multiple recipients, would run into problems, because the lose the original message.

Working around that is simple, you e.g. only have to run mail.dup.deliver. But the behvaiour is rather surprising, I think.

On the other hand, mail-gpg changes some state during encryption already: it deletes :encrypt from the gpg-options (By the way: why's that?).

On the positive side: as an interceptor, mail-gpg doesn't have to copy headers.

What do you think about this?

duckdalbe commented 6 years ago

ping.

jkraemer commented 6 years ago

Sorry for the late reply.

In general I think if we made it clear in the changelog that the message object will be modified and make it a new major version such change in behavior would be ok. However there's a couple more things to consider.

I just found a (quite old) blog post which outlines another problem:

using deliver! [instead of deliver] will call any registered Mail Observers, but not Interceptors - meaning that your mail will be sent unaltered.

https://joshmcarthur.com/howto/2011/10/06/action-mailer-interceptors.html

I am not sure whats the reasoning behind this or if it maybe was just a bug in Rails that's already fixed now, but surely something to keep in mind. I wouldn't want to have such an inconsistency in behavior.

Another change for users will be that they will have to register the interceptor manually because the order multiple interceptors are registered in will matter. Will give the user more control (i.e. only enable it in production...) though.

Regarding the Memory Hole project - isn't what they advertise (putting the headers into the encrypted part) exactly what mail-gpg already does? All that is left to do in order to fully implement this would be formalizing a way to define what headers are sensitive and should be moved into the signed/encrypted part, and which probably should just be copied. The latter might make sense when signing only - leaving headers in the main message will help with clients unaware of the memory hole standard.

That would give you a way to move sensitive headers into the encrypted part, and it might also help with cases like #53 where people explicitly want no headers in there.

The header copying has been a problematic area of mail-gpg all the time, moving from a hard-coded list that had to be maintained to 'encrypt them all' which then made users ask why that is the case. I think implementing a flexible way that probably not only supports header names but also regexps to catch i.e. all X-Redmine-.+ Headers would be a good thing regardless of the Interceptor / DeliveryHandler issue.

What do you think?

duckdalbe commented 6 years ago

Alright, so I'll clean up the interceptor-code.

Regarding the other topics:

geor-g commented 6 years ago

@jkraemer Ping ... :)

duckdalbe commented 6 years ago

@ge-fa I think the ball is on my side. It's only that my pressing problem with this has vanished and I can't find the time to do this nonetheless.

It's still on my agenda, though.

mkamensky commented 5 years ago

The gem does not currently work with ActionMailer, as far as I can tell, since it assigns itself as the delivery_handler, and so Mail::Gpg::DeliveryHandler is not assigned. If I understand correctly, using the interceptor approach should solve it