419203 - Use tokens to authenticate email senders

Status: RESOLVED WONTFIX

(Bugzilla :: Incoming Email, enhancement)

Description

[Frédéric Buclin]

email_in.pl cannot trust email senders based on the "From:" header only as this header can easily be altered. Not everybody can sign their emails, and till such a feature is implemented, we should have an easier solution to authenticate the user:

userprefs.cgi has a button to generate a token for incoming emails. Everytime you click the button, a new token is generated (e.g. if you suspect someone had access to an older one and you want a new one) and an email is sent to your email address with the token in it. The token would be displayed in userprefs.cgi so that you can still copy it (or to remember which one is the current one).

If you already generated a token once, you should be able to generate a new one using email_in.pl, as email_in.pl could authenticate you for sure thanks to the (not yet old) passed token. Here again, the new token would be sent to you per email.

The "Generate New Token" button and the token read-only field would not be displayed if Email::Reply and Email::MIME::Attachment::Stripper are not installed, for consistency with t/Support/Files.pm.

I asked justdave if he agrees to take it for 3.2, following our discussion about bug 419188, and his answer is 'yes':

(17:54:23) LpSolit: justdave: and for 3.2, would you accept my token implementation?
(17:54:39) justdave: yup, we haven't hit hard freeze yet that I know of
(17:54:47) justdave: and that was a planned feature anyway

Personally, I would even take it for 3.0.x.

Comment 1

[Max Kanat-Alexander]

No, this is a major enhancement and is not approved for 3.2. I am heavily vetoing this, as release manager. We *have* hit a hard freeze, as of February 4th.

Comment 2

[Max Kanat-Alexander]

Also, this is not the correct way to handle this situation. Email users should *not* have to access the web interface to use email. Ever. There might be email users who can't even *get* to the web interface, in some situations.

The correct solution is to implement both of the following, as I'm sure I described somewhere else already:

1) Bounce-back confirmations. This is where we send an email back to the "From"
   address and say, "Reply to this message to have your edits take place."

2) GPG-signed messages and the user puts their GPG key ID into their 
   preferences.

Then, we can make a userpreference for whether or not to *require* GPG on all incoming messages (that is, disallow bounce-back confirmation).

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

Comment 4

[Frédéric Buclin]

You only have to use the web once, as when you create your user account. Reopening!

Comment 5

[Dave Miller [:justdave]]

I was under the impression you were trying to use that as part of the fix for bug 419188.  Within that context it seemed to make sense.  If we have indeed frozen already (I thought we were still in the soft freeze, probably I'm not paying close enough attention and you didn't correct me) then it's probably appropriate to make this wait.  WONTFIX probably isn't appropriate though.  I can see this as a usable method alongside the other choices listed above.

Comment 6

[Max Kanat-Alexander]

It's totally unnecessary to provide three mechanisms when the two I specified above will work. Three mechanisms is one too many to have to maintain.

I am still of the opinion that this should be WONTFIX.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> It's totally unnecessary to provide three mechanisms when the two I specified
> above will work. Three mechanisms is one too many to have to maintain.

The one I propose here is better than your option 1). I would hate to have to reply everytime to have my submissions accepted. With my implementation, you add your token in your email and that's it.

In case you didn't realize, your token remains valid as long as you want. So that's much efficient.

Comment 8

[Max Kanat-Alexander]

Hmm, I think that's a little too insecure. For example, you could CC somebody and then they'd have your token and be able to act as you. Anybody else seeing your email would suddenly be able to impersonate you.

Comment 9

[Frédéric Buclin]

Well, that's how e.g. majordomo works. We have to pass our (admin) password every time we do a change per email, or even to request some information (e.g. when managing the qa@ mailing-list).

If you realize you CC'ed someone and your token has been seen, all you have to do is to request a new token *per email*:

@email_token = <old_email_token_string>
@generate_new_email_token = 1

And that's it. A new token is sent to you per email, invalidating the old one. It's fast and secure.

Comment 10

[Max Kanat-Alexander]

Okay. First, I'd like to publicly apologize to LpSolit for WONTFIX'ing this bug without talking to him about it first. That was rude and inconsiderate of me, as well as being technically a bad decision.

Secondly, I'd like to say that with a few modifications, I've decided that I'm OK with this approach:

1. I'd like a parameter that expires these tokens after a certain amount of time. There should be at least one choice that expires them immediately, and another choice that never expires them. If you just want a radio-button with those two choices, I'd be fine with that. Some people might want them to expire after X days, so a third choice (or just a numeric text entry) might also be OK.

2. When I talked to LpSolit, I said that one thing I want in the future is a way to use this system entirely through email, without ever interacting with a web interface. However, one thing I don't necessarily want to have to do is to maintain two interfaces for creating tokens--one web interface and one email interface. I suspect that once we have an email interface, the web interface won't be necessary at all. Email users will want to interact with the system primarily through email, without having to go to the web interface just to generate a token. One of the most common complaints I've heard about email interfaces in other systems is "Why are you making me open my web browser?" It's just like how we use bugbot to search Bugzilla on IRC instead of opening our browser--it's much more convenient.

  So, perhaps it would be a good idea to split this into "backend/frontend" tasks. That is, implement a system in email_in for validating tokens, and then an interface via email for creating these tokens. We can check them both in at once, but they can be reviewed as separate items. Basically what I'd like to see is a system where email_in sends you a token back to the "From" address to validate that you really are *you*, and then you have to reply with that token. Once you have the token, it can stay valid according to the parameter in #1.

  I think I'd like to see the token somehow integrated into the subject line, if possible, so that people have an easy way to use it, in addition to using @email_token.

  If we do want the ability to invalidate an old token via email, I think the field should probably just be called @new_token, which is shorter. The existence of the field, no matter what its value, should trigger the action, unless that would complicate the code of email_in.pl.

  Of course, we have to make sure that "disabled" accounts can't get tokens or use email_in.pl, but I'm pretty sure that's already the case. (Otherwise somebody could generate a token to send to a mailing list, and that'd be dangerous. We should document that risk, though, so people know to disable mailing list accounts.)

Comment 11

[Frédéric Buclin]

With the new token infrastructure we implemented for security bugs, it should be pretty easy to generate and validate tokens from email_in.pl. This bug is #2 on my TODO list (after bug 415541) and should be ready on time for 3.6.

Comment 12

[Frédéric Buclin]

I have a slightly different proposal to suggest:

Rather than forcing the user to write @email_token = foo in the body of the message, Bugzilla should put the token in the From: header, so that when you reply to the message, email_in.pl will be able to parse

 "token" <[e-mail]>

in the To: header, and authenticate the user this way. If the token is missing in the To: header, then it will look at @email_token, and reject the email if it's also missing.

I think this proposal is better than putting the token in the Subject header, because it may confuse some email clients and break threads. Also, parsing the Subject line is not ideal, because we would probably choose something like

 [Bug NNN] $token $bug_summary

which is annoying because the token is meaningless to the user, and so wastes some space when reading the email subject in email clients.

Also, the token should be a combination of the bug ID + user ID (or login) + Bugzilla secret key, i.e would be different for each bug, to limit impersonation/abuse. We shouldn't include delta_ts, because you would be unable to reply if you didn't get the very last message (though it would be cool to catch midair collisions).

For new bugs, the user would simply write @email_token = foo in the body of the message. Or even better: get a confirmation email (which would contain the token in the From: header), so that we never use the @email_token syntax *at all*. In that case, we would need a parameter which controls whether a token is required to accept emails or not; and we could later add another option to this parameter once GPG is supported (incoming_email_check: none/token/GPG/token-or-PGP).

Thoughts?

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> Rather than forcing the user to write @email_token = foo in the body of the
> message, Bugzilla should put the token in the From: header, so that when you
> reply to the message, email_in.pl will be able to parse
> 
>  "token" <[e-mail]>

Hum, probably better is to parse the Message-ID header. Bug 600230 added a randomly generated Message-ID token, but we could replace it by the one I suggested in my previous comment. This way, the From: field wouldn't look funny with a token in it, and the In-Reply-To header would contain the original Message-ID, which email_in.pl can parse and validate.

We would do something similar for new bugs created by email: the incoming email is stored in a DB table (which one? a new one?), and a confirmation email is sent back to the user. The new bug would expire if not confirmed within a few hours.

Sounds good?

Comment 14

[Max Kanat-Alexander]

Yeah, I think we could just check the In-Reply-To header, that's a pretty good idea, provided that it is indeed a secure hash. Then people could even forward the content of emails safely to each other, as long as they didn't forward the whole message as an attachment.

Comment 15

[Frédéric Buclin]

Attachment: patch ,v2

Comment 16

[Reed Loden [:reed]]

You already support post-tokens for new bugs... Why can't you also support post-tokens for bug changes so that a lower expiration time (as in, not infinite) can be set while still allowing users to authenticate their updates/comments/whatever?

Comment 17

[Frédéric Buclin]

(In reply to comment #16)
> You already support post-tokens for new bugs...

What is a post-token?

 Why can't you also support
> post-tokens for bug changes so that a lower expiration time (as in, not
> infinite) can be set while still allowing users to authenticate their
> updates/comments/whatever?

I don't want any expiration time to reply to an existing bug. If a bug has low activity, there could be months between two emails.

Comment 18

[Ben Bucksch (:BenB)]

Very good idea!

Comment 19

[Ben Bucksch (:BenB)]

(I would allow users to opt-out of it, though, to satisfy users who consider this to be too insecure.)

Comment 20

[Frédéric Buclin]

(In reply to comment #19)
> (I would allow users to opt-out of it, though, to satisfy users who consider
> this to be too insecure.)

I thought about that too, but I will keep this user pref for a separate bug. :)

Comment 21

[Max Kanat-Alexander]

Comment on attachment 500595 [details] [diff] [review]
patch ,v2

>Index: Bugzilla/DB/Schema.pm
>+    email_inbound => {
>+        FIELDS => [
>+            token   => {TYPE => 'varchar(16)', NOTNULL => 1, PRIMARYKEY => 1,
>+                        REFERENCES => {TABLE  => 'tokens',
>+                                       COLUMN => 'token',
>+                                       DELETE => 'CASCADE'}},
>+            content => {TYPE => 'LONGBLOB', NOTNULL => 1},
>+        ],
>+        INDEXES => [
>+            email_inbound_token_idx => ['token'],
>+        ],

  You don't need that index--making it a PK already gives it an index.

>Index: email_in.pl
>+    my @token_data;
>+    if (Bugzilla->params->{'incoming_email_check'} eq 'token') {

  It'd be nice to make this whole block into a separate subroutine, like _check_token or something.

>+        if ($identifier) {
>+            @token_data = $identifier =~ /^<bug-(\d+)-(\d+)-(\w+)(?:-|@)/;

  It might be nicer to do something like "my ($bug_id, $user_id, $token) = " instead of @token_data.

>@@ -380,6 +413,30 @@ sub die_handler {
>+    $template->process("email/email_in_confirmation.txt.tmpl", $vars, \$msg)
>+      || ThrowTemplateError($template->error());

  Hmm. Do you maybe want to use Email::Reply along with this, since we have it? That way you don't have to pass in $mail_text to the template (which contains all the headers and everything, in raw format, from the email).

>@@ -411,6 +468,44 @@ if (my $suffix = Bugzilla->params->{'ema
>+if (Bugzilla->params->{'incoming_email_check'} eq 'token') {
>+    my $token = delete $mail_fields->{'token'};

  This block should have its own subroutine, too.

>+        if ($bug_id) {
>+            $token = issue_hash_token([$bug_id], EXPIRATION_DATE, $user);

  I think that should be check_hash_token?


  And also, the changes that we talked about on IRC.

Comment 22

[Frédéric Buclin]

(In reply to comment #21)
>   I think that should be check_hash_token?

No, because check_hash_token() is for the web interface, and will offer you to submit you changes anyway, or go back to index.cgi. When you get this message per email, it doesn't really make sense. :)

Comment 23

[Max Kanat-Alexander]

(In reply to comment #22)
> No, because check_hash_token() is for the web interface, and will offer you to
> submit you changes anyway, or go back to index.cgi. When you get this message
> per email, it doesn't really make sense. :)

  Oh, true. How about just modifying the behavior of check_hash_token in user-error when we're in USAGE_MODE_EMAIL, then?

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
>   Oh, true. How about just modifying the behavior of check_hash_token in
> user-error when we're in USAGE_MODE_EMAIL, then?

I suppose that's doable.

Comment 25

[Ben Bucksch (:BenB)]

>+        if ($bug_id) {
>+            $token = issue_hash_token([$bug_id], EXPIRATION_DATE, $user);

We wanted a very different expiration date for web submission and email response, too. I don't think it's worth trying to share these few lines.

Comment 26

[Frédéric Buclin]

(In reply to comment #21)
> >+    $template->process("email/email_in_confirmation.txt.tmpl", $vars, \$msg)
> >+      || ThrowTemplateError($template->error());
> 
>   Hmm. Do you maybe want to use Email::Reply along with this, since we have it?

How do I set the Message-ID with the token in it in that case? I also need to add some additional text to explain why the sender got his email back, so I still have to use a template.

Comment 27

[Max Kanat-Alexander]

(In reply to comment #26)
> How do I set the Message-ID with the token in it in that case?

  Email::Reply generates an Email::Simple object, so you can just add the header yourself.

> I also need to
> add some additional text to explain why the sender got his email back, so I
> still have to use a template.

  Ahh, true, but you could just specify that as the reply body.

Comment 28

[Frédéric Buclin]

Note that to correctly authenticate a user, I need to add a hash token to the Message-ID header. As a consequence, I'm going to add this hash token to the In-Reply-To header as well, which means bugmails will no longer be correctly threaded by some email clients for old threads as the new In-Reply-To header won't match the old Message-ID header. I hope we are fine with this.

Comment 29

[Max Kanat-Alexander]

  Hmm. I was hoping we wouldn't have to break existing threads. Is there any way to avoid doing so?

Comment 30

[Frédéric Buclin]

(In reply to comment #29)
>   Hmm. I was hoping we wouldn't have to break existing threads. Is there any
> way to avoid doing so?

The Message-ID header must contain the token, so that In-Reply-To passes it back to us. As the Message-ID header must be unique across the world, it's fine to change the way it's built. About In-Reply-To, I'm going to make it match the new Message-ID header, for consistency and because that's what more and more bugmails will have.

To not break existing threads, what I can do is to add both the old and new syntaxes in the References header, as RFC 2822 section 3.6.4 says that

"the "References:" field may be used to identify a "thread" of conversation. [...] The "References:" field will contain the contents of the parent's "References:" field (if any) followed by the contents of the parent's "Message-ID:" field (if any)."

So we can have more than one identifier in this header and so it will be able to detect either one or the other of the two syntaxes. Technically, I could do the same for the In-Reply-To header, if we want to.

Comment 31

[Max Kanat-Alexander]

Yeah, that could possibly work. Or maybe for simplicity's sake we should just break the threads.

Comment 32

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 33

[Frédéric Buclin]

I spent a lot of time thinking about how to implement this correctly, and I couldn't come to a good solution for new bugs. I'm going to close this bug as WONTFIX in favor of bug 943052.