change pgp encryption from CAST-5 to AES

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: glob, Assigned: alex_johnson)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

7.32 KB, patch
jfearn
: review-
dylan
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/SecureMail/Extension.pm#L600

# We use the CAST5 cipher because the Rijndael (AES) module doesn't
# like us for some reason I don't have time to debug fully.
# ("key must be an untainted string scalar")
Yep, that was me. Fair enough :-)

gerv
(Assignee)

Comment 2

4 years ago
What about IDEA encryption, will it work if you do:

my $encrypted = $pgp->encrypt(
        Compat     => 'PGP2',
        Data       => $text,
        Recipients => "@",
        Armour     => 0
    );
Flags: needinfo?(glob)
(Assignee)

Comment 3

4 years ago
(In reply to Alex Johnson(:alex_johnson) from comment #2)
> What about IDEA encryption, will it work if you do:
> 
> my $encrypted = $pgp->encrypt(
>         Compat     => 'PGP2',
>         Data       => $text,
>         Recipients => "@",
>         Armour     => 0
>     );

my $encrypted = $pgp->encrypt(
         Compat     => 'PGP2',
         Data       => $text,
         Recipients => "@",
         Armour     => 0
     );

Never mind, it's less secure than CAST5.  I would be interested in figuring out why we can't use AES though.
Flags: needinfo?(glob)
(Assignee)

Comment 4

4 years ago
Maybe using GnuPG::Interface might help.
(Reporter)

Updated

4 years ago
Assignee: nobody → mrjohnsonalex
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
I tried switching to GnuPG::Interface, but I've been having trouble base64 encoding the result even if I used the same Crypt::OpenPGP::Armour->armor() funciton.
Attachment #8687697 - Flags: review?(dylan)
(In reply to Alex Johnson(:alex_johnson) from comment #5)
> Created attachment 8687697 [details] [diff] [review]
> Tried switching to GnuPG::Interface.
> 
> I tried switching to GnuPG::Interface, but I've been having trouble base64
> encoding the result even if I used the same Crypt::OpenPGP::Armour->armor()
> funciton.

I'm a bit confused by this comment -- is the patch for review in a working state?
Flags: needinfo?(mrjohnsonalex)
(Assignee)

Comment 7

4 years ago
(In reply to Dylan William Hardison [:dylan] from comment #6)
> (In reply to Alex Johnson(:alex_johnson) from comment #5)
> > Created attachment 8687697 [details] [diff] [review]
> > Tried switching to GnuPG::Interface.
> > 
> > I tried switching to GnuPG::Interface, but I've been having trouble base64
> > encoding the result even if I used the same Crypt::OpenPGP::Armour->armor()
> > funciton.
> 
> I'm a bit confused by this comment -- is the patch for review in a working
> state?

It actually isn't.  I'll keep working on it.  Please ignore the review request.
Flags: needinfo?(mrjohnsonalex)
(Reporter)

Updated

4 years ago
Attachment #8687697 - Flags: review?(dylan)
(Assignee)

Comment 8

4 years ago
Sorry about the previous review request.  I think this one works though.  Please take a look.
Attachment #8687697 - Attachment is obsolete: true
Attachment #8688807 - Flags: review?(dylan)
(Assignee)

Updated

4 years ago
Attachment #8688807 - Attachment description: Switched to GnuPG::Interface and MIME::Entity and it looks like it works. → Part 1: Switched to GnuPG::Interface and MIME::Entity
(Assignee)

Comment 9

4 years ago
Attachment #8689538 - Flags: review?(dylan)
(Assignee)

Updated

4 years ago
Attachment #8689538 - Attachment description: Added AES256 cipher-algo → Part 2: Added AES256 cipher-algo
Comment on attachment 8688807 [details] [diff] [review]
Part 1: Switched to GnuPG::Interface and MIME::Entity

Review of attachment 8688807 [details] [diff] [review]:
-----------------------------------------------------------------

This is really great, but I'd like the file-scoped lexicals to be removed. 
Also there are lots of extra white space at the ends of lines that it would be nice to clean up.

::: Bugzilla/GPGMail.pm
@@ +28,5 @@
> +my $homedir = File::Temp->newdir();
> +my $keyring;
> +my $imported;
> +my $comment;
> +

I have concerns for these package-scoped lexicals. I'd rather see them as instance members of $self.
$self->{_bz_homedir}, $self->{_bz_keyring} etc should be enough to avoid any collisions with the parent class.

Do we need to worry about a shared homedir between multiple calls to gpg?

::: extensions/SecureMail/Config.pm
@@ +28,5 @@
> +	{
> +        package => 'Mail::GPG',
> +        module  => 'Mail::GPG',
> +        version => 0,
> +    },

I think we need to explicitly depend on Try::Tiny here. I know it's pulled in anyway, but I believe that's our general guideline.
Attachment #8688807 - Flags: review?(dylan) → review-
Comment on attachment 8688807 [details] [diff] [review]
Part 1: Switched to GnuPG::Interface and MIME::Entity

Review of attachment 8688807 [details] [diff] [review]:
-----------------------------------------------------------------

there were quite a few "style issues" as well -- those can be taken care of with perltidy (for the most part). If you have issues with that get with me on IRC.
Also the GPGMail.pm file will need to go into extensions/SecureMail/lib and its package name will change to Bugzilla::Extension::SecureMail::GPGMail (also already mentioned on irc).
(Assignee)

Comment 14

3 years ago
Attachment #8688807 - Attachment is obsolete: true
Attachment #8689538 - Attachment is obsolete: true
Attachment #8692020 - Attachment is obsolete: true
Attachment #8694840 - Attachment is obsolete: true
Attachment #8689538 - Flags: review?(dylan)
Attachment #8692020 - Flags: review?(dylan)
Attachment #8694840 - Flags: review?(dylan)
Attachment #8695020 - Flags: review?(dylan)
Comment on attachment 8695020 [details] [diff] [review]
All the commits folded into one with pertidy run on all the changed files.

Review of attachment 8695020 [details] [diff] [review]:
-----------------------------------------------------------------

Please do not run perltidy on existing code and instead adhere to whatever coding style is around the areas you are editing. This makes the reviewer's job much more difficult and time consuming. Adding entirely new files or cleaning just the block of code you are adding, perltidy is not a bad idea but we would still like the new code to also use similar style to the rest of the Bugzilla code. More info (good reference but slightly out of date) can be found at https://www.bugzilla.org/docs/developer.html

dkl
Attachment #8695020 - Flags: review-
(Assignee)

Comment 16

3 years ago
Sorry about the bad perltidy from the previous patch.  I have fixed that here.
Attachment #8695020 - Attachment is obsolete: true
Attachment #8695020 - Flags: review?(dylan)
Attachment #8695461 - Flags: review?(dkl)
(Assignee)

Updated

3 years ago
Attachment #8695461 - Flags: review?(dylan)
(Assignee)

Comment 17

3 years ago
I just realized that I'm not using Try::Tiny anywhere.
Attachment #8695461 - Attachment is obsolete: true
Attachment #8695461 - Flags: review?(dylan)
Attachment #8695461 - Flags: review?(dkl)
Attachment #8695500 - Flags: review?(dkl)
(Assignee)

Updated

3 years ago
Attachment #8695500 - Flags: review?(dylan)
I will get to this but it might not be until Saturday. It's quite a lot to read and test.
(Assignee)

Comment 19

3 years ago
Attachment #8695547 - Flags: review?(dylan)
(Reporter)

Comment 20

3 years ago
Comment on attachment 8695500 [details] [diff] [review]
Switched to GnuPG with fixed PerlTidy.

this change doesn't need two reviews.
Attachment #8695500 - Flags: review?(dkl)
(Reporter)

Comment 21

3 years ago
(In reply to Alex Johnson(:alex_johnson) from comment #19)
> Created attachment 8695547 [details] [diff] [review]
> Fixup for multipart HTML messages

when fixing up, please attach a new patch which encompasses all the changes including the fixes instead of a patch that just carries the fix.
(Assignee)

Comment 22

3 years ago
Attachment #8695500 - Attachment is obsolete: true
Attachment #8695547 - Attachment is obsolete: true
Attachment #8695500 - Flags: review?(dylan)
Attachment #8695547 - Flags: review?(dylan)
Attachment #8695671 - Flags: review?(dylan)
Comment on attachment 8695671 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity.

Review of attachment 8695671 [details] [diff] [review]:
-----------------------------------------------------------------

When using perltidy, it's important to use the perltidyrc file that BMO uses, make sure to copy the .perltidyrc from the BMO repo into your $HOME,
or otherwise instruct your editor to use it. The GPGMail.pm file had tabs (instead of spaces) in it, which shouldn't happen.

::: extensions/SecureMail/Extension.pm
@@ +31,3 @@
>  use Bugzilla::Util qw(correct_urlbase trim trick_taint is_7bit_clean);
>  use Bugzilla::Error;
>  use Bugzilla::Mailer;

Because of the weird way extensions are loaded, it would be better to turn this into "require Bugzilla::Extension::SecureMail::GPGMail" and move it into _pgp_encrypt()

::: extensions/SecureMail/lib/GPGMail.pm
@@ +16,5 @@
> +# Portions created by Mozilla are Copyright (C) 2008 Mozilla Foundation.
> +# All Rights Reserved.
> +#
> +# Contributor(s): Alex Johnson <mrjohnsonalex@gmail.com>
> +

This is the old copyright header, don't use this one. It should look like:
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
#
# This Source Code Form is "Incompatible With Secondary Licenses", as
# defined by the Mozilla Public License, v. 2.0.
Attachment #8695671 - Flags: review?(dylan) → review-
(Assignee)

Comment 24

3 years ago
Moved .perltidyrc to $HOME and ran perltidy again from vim, changed the use to a require and moved it to _pgp_encrypt() and changed the copyright.
Attachment #8695671 - Attachment is obsolete: true
Attachment #8705398 - Flags: review?(dylan)
(Assignee)

Comment 25

3 years ago
There has been no momentum on this in over 4 months.  Are we still planning on doing this?
I have not had the time to test it in an environment that would closely match production. We are currently undergoing three very large infrastructure changes (four if you include the AWS migration) as well.

One of these changes will make specifying new CPAN modules far easier, so I suspect that this will be a great help.
Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

Review of attachment 8705398 [details] [diff] [review]:
-----------------------------------------------------------------

I still do not have the time to *test* this but the code looks good. So it gets f+.
This also uncovered some errors in how extensions work and I'm very happy about that.

::: extensions/SecureMail/lib/GPGMail.pm
@@ +7,5 @@
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +#
> +# Contributor(s): Alex Johnson <me@alex-johnson.net>
> +

We use MPL 2 style copyright headers, so this would need to be changed. Minor nit.
Attachment #8705398 - Flags: feedback+
Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

You have a patch that is doing something like this, right? That you've been able to test? At the time this work was done I had no easy way of testing it and it may have bit-rotten, but getting it into SecureMail would be awesome.
Attachment #8705398 - Flags: review?(dylan) → review?(jfearn)

Comment 29

2 years ago
Yeah I have it working on RH Beta Bugzilla, you can test it yourself if you have an account on there, it's a dump from production from about February or March IIRC.

I changed the code in Extension.pm, I didn't add any extra files.

If you have a PGP key with sub keys and your encryption key isn't the first sub key, say your first sub key is a signing key and the second is an encryption key, then you can test it properly. Otherwise you are just testing what it can already do. It has been tested with some pretty complex key structures.

Mail out is disabled, but I added a hacky preference to bypass that for crazy people, like us. The check box with the big red danger message next to it.

https://beta-bugzilla.redhat.com/userprefs.cgi?tab=securemail

Just add/change your PGP key and click the check box, then submit, and it will send you the test mail.

You might want to disable that preference shortly after!

If you tell me the repo and branch you'd like a patch against I can diff it for you.

FWIW I also have the corresponding changes for email_in.pl, it makes PGP signing (not encryption) mandatory for incoming mail, you can also test this on the beta as mail-in is enabled. It will silently swallow mail that doesn't map to a real account or isn't signed, or where the from address and PGP don't match, or if the PGP key isn't already in Bugzilla, etc, etc.

Comment 30

2 years ago
Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

Nacking this because it isn't similar to what I've given to Dylan and I'm not in a position to validate this version fully.

Couple of notes, the hard coded values (paths, exe, etc) should be parameters (e.g. I prefer gpg2), using key servers should either be disabled or made optional. There is no authentication on key servers, anyone can add a key, limiting keys to those entered in to Bugzilla limits exposure to some degree.
Attachment #8705398 - Flags: review?(jfearn) → review-
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Depends on: 1413328
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.