1594855 - Check in starting point for OpenPGP integration, based on stripped/rebranded Enigmail

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Kai Engert (:KaiE:)]

For the OpenPGP integration work, instead of starting from scratch, we could use the Enigmail code as a starting point. Then we can remove/adjust as necessary.

Using Enigmail version 2.1.3, I stripped code that we clearly don't want, and reduced it to a minimal set of code that still worked (as an external addon with TB 68).

I'll attach the result as an initial, large patch. That patch will simply add files, without enabling them.

Patrick: It's probably difficult to review this large patch. To simplify it, I'll send you a small HG repository by email, which contains 27 incremental commits, which shows the modifications I made. I'll ask you to review the patch, but the intention is to get your approval that this is a reasonable starting point for landing into Thunderbird.

I removed the Enigmail branding, to ensure testers won't be confused.

Then I'll attach a second, much smaller patch, which adds the prefs, and build rules to enable those files. Also some initial modifications, that attempt to convert the previous Add-on style loading of the code to the new integrated code.

A new pref temp.openpgp.engine is used, set to 0 by default, which disables loading of this code. Even if enabled, it doesn't work yet, I get string bundle errors, the user interface overlaying doesn't work.

Also, there's some initial experimental code to access the RNP library with ctypes.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: enigmail-subset-rebranded.patch

Comment 2

[Kai Engert (:KaiE:)]

Attachment: enigmail-initial-integration.patch

Comment 3

[Kai Engert (:KaiE:)]

Magnus, do you agree that we land this code as a starting point?

Comment 4

[Jorg K (CEST = GMT+2)]

Personally I don't think it's a good idea.

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Jorg K (GMT+2) from comment #4)

Personally I don't think it's a good idea.

Why? This approach allows to avoid permanent merging, and avoids that cleanup work done by others will also include this code.

Comment 6

[Jorg K (CEST = GMT+2)]

Is this meant as an add-on on or is it integrated? I see some left over AutoCrypt and even pEp code. Typically we review code before committing it to the code base, that is, a Thunderbird peer looks at it. That's a big job for 130+K lines of code. Have you run this through linting/prettier? Does it work? How about the connection to GnuPG? How about the translations you're proposing to land? What about the maintenance burden maintaining all that XUL code that's on the way out? We're committing another overlay loader to the code base when we're busily working on removing the one we have? Are there any tests?

I thought we're going to build "PGP" support "from the ground up", integrating it into the UI like we have S/MIME. Here you bolting an "ex-add-on" onto the side to reach into core code with its tentacles in a mostly not maintainable fashion.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to Jorg K (GMT+2) from comment #6)

Is this meant as an add-on on or is it integrated?

Integrated.

I see some left over AutoCrypt and even pEp code.

I don't claim this code is already perfect. It's a starting point, and the intention is to remove everything that we don't need as we go ahead.

Typically we review code before committing it to the code base, that is, a Thunderbird peer looks at it.

I know. However, this code is explicitly disabled. It also lives strictly separate from all the other code, in its own separate subdirectory. So we can be aware that code in that directory can be considered unreviewed.

That's a big job for 130+K lines of code.

I don't think we should review this code at this time. It doesn't make sense to review code that we won't keep. A lot of this code will be removed or rewritten.

Have you run this through linting/prettier?

No, it's too early. It's a starting point for collaborating on new feature development, not code that is ready for being used.

Does it work?

It doesn't work yet in this integrated state.

How about the connection to GnuPG?

Because some people have complained that OpenPGP smartcards will no longer work when using an OpenPGP engine other than GnuPG, we're exploring potential strategies for keeping that support. A potential approach could be to use a dual-engine strategy. That is, by default, for the majority of users, an engine that is shipped with Thunderbird could be used. However, as an advanced user configuration, we might allow users to switch to use the external GnuPG engine, thereby allowing the use of OpenPGP smartcards.

It's not decided if that's what we'll do. But it seems useful to keep the existing GnuPG binding code for another while.

How about the translations you're proposing to land?

What problem do you see with that? If it's uncommon to land non-english into comm-central, I'm happy to have those strings be landed elsewhere.

What about the maintenance burden maintaining all that XUL code that's on the way out? We're committing another overlay loader to the code base when we're busily working on removing the one we have?

Clearly all OpenPGP UI code needs to follow the restrictions that current Thunderbird has. However, if we land this code here, then conversion efforts can more easily cover this code, too.

Are there any tests?

No, not yet.

I thought we're going to build "PGP" support "from the ground up", integrating it into the UI like we have S/MIME. Here you bolting an "ex-add-on" onto the side to reach into core code with its tentacles in a mostly not maintainable fashion.

We said that we want to reuse as much existing code from Enigmail as possible. Instead of moving in piece by piece, it seems easier to check in what we have, then adjust/remove and add new code as necessary, in incremental steps.

Once we have arrived in a state that is usable, then we could start a code review effort for all the new OpenPGP code.

Comment 8

[Kai Engert (:KaiE:)]

I'd file a separate bug "review all code in mail/extensions/openpgp/ prior to enabling it by default, and prior to including it in a release version of TB.

Comment 9

[Jorg K (CEST = GMT+2)]

It doesn't work yet in this integrated state.

Well, then I see no point landing it. Also if we subject this code to "general mass replacements" porting M-C changes, we have no way of testing it.

Comment 10

[Patrick Brunschwig]

It was actually suggested by Magnus (in a private mail to Kai and me) that Kai should commit the code to the repository, such that changes like bug 1570954 and bug 1594331 will automatically be applied to the code.

Comment 11

[Kai Engert (:KaiE:)]

(In reply to Jorg K (GMT+2) from comment #9)

Well, then I see no point landing it.

The subject mentions the point: It's a starting point ;)

When the hidden pref is enabled, some parts work. For example, the mime handler is registered, and I can see decrypted email message content in the mail reader.

The UI doesn't work. Apparently the old xul loading fails in some way. I'm OK to just disable the XUL loading altogether. We can convert the UI pieces that we want to keep, step to step, to a new approach.

Also if we subject this code to "general mass replacements" porting M-C changes, we have no way of testing it.

That's fine. As long as this code is disabled and doesn't have automated testing, you can consider it my problem if things get broken by such changes. It still helps me, if I don't have to do such replacements myself.

Comment 12

[Patrick Brunschwig]

Comment on attachment 9107301 [details] [diff] [review]
enigmail-initial-integration.patch

As agreed I only browsed through the changes. I found nothing severely wrong.

enigmailPrivacyOverlay.* - can be removed entirely

Comment 13

[Patrick Brunschwig]

Comment on attachment 9107300 [details] [diff] [review]
enigmail-subset-rebranded.patch

As agreed, I only browsed through the changes. I found nothing severely wrong.

Comment 14

[Magnus Melin [:mkmelin]]

The idea was indeed to check it in to core so it can get part of the tree-wide changes we do. Doing it another way would be a lot more work. I hope we can get it into a workable state soon so most changes can be tested.

Getting it linted and running prettier over the code should be one of the first steps after landing.

I'd have to look more closely at the files, but I don't think at least the localizations should be landed. The English strings should be landed in a non-localizable location for now (like we did for OTR).

Comment 15

[Jorg K (CEST = GMT+2)]

If you cant to achieve that, please create a "disposable" project repository, see:
https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectRepositories

Comment 16

[Kai Engert (:KaiE:)]

I found that "eslint --fix" produces broken code. In one place, it removed two closing brackets, only added one, resulting in error "catch without try".

If "eslint --fix" comes with the risk of introducing semantic changes, I don't feel comfortable running that cleanup on all code, prior to committing a starting point.

I think it's better to start with adding that code to the eslint exclude list, and do the cleanup later, step by step.

Comment 17

[Kai Engert (:KaiE:)]

I don't think using a disposable project repo is helpful. I want the code that we're working on to be in tree, not in some other location. I don't want it to be in other branches that need to be constantly merged.

However, we could do the import in smaller steps.

Initially, the most important parts for me are the backend code, because that code is more likely to be reused than the UI code, it is easier to make the backend code work, and my initial work steps will be on backend code.

Here's an updated suggestion:
(a) for now, exclude all user interface code. In order to have a place for all files that are TODO, I've checked them in here: https://hg.mozilla.org/users/kaie_kuix.de/parking/file/tip/openpgp/not-yet-imported

(b) same as (a) for most locale files. In the initial step, only import the properties file that is references from the backend JS code.

(c) only import the backend JS code, however...

(d) exclude the overlay loading backend code, because it isn't working.

(e) exclude mail/extensions/openpgp from lint. I've filed bug 1595319 to track that. I promise to work on that as I proceed with that code. Given the risk that eslint introduces semantic regressions, it seems best to process a file after it has been touched by me, after I know I really need it, and after I know how a file is used. Then eslint can be applied for that one file, and testing can reveal if a regression / semantic change was introduced.

Patrick doesn't need to re-review the large attachment. It's just delaying some of the files, which are parked at my above user repo.

I'll attach a patch to exclude mail/extensions/openpgp from eslint, as proposed.

From directory "locale", the only surviving file is enigmail.properties, and I've moved it to content/strings, as suggested by Magnus, to exclude it from localizing.

I've fixed string loading (which I earlier said didn't work).

I'll attach updated glue code.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: enigmail-initial-integration-v2.patch

Comment 19

[Kai Engert (:KaiE:)]

Attachment: 1594855-eslint.patch

Comment 20

[Kai Engert (:KaiE:)]

Attachment: enigmail-subset-rebranded-v2.patch

smaller subset for initial landing, moved strings to content directory.
Other files moved to: https://hg.mozilla.org/users/kaie_kuix.de/parking/file/tip/openpgp/not-yet-imported

Comment 21

[Kai Engert (:KaiE:)]

Magnus, do you agree with the plan suggested in comment 17? If yes, please mark the attached patches as r+.

I don't ask you to review the code in detail at this time, but rather, I suggest that we allow working on this code, and postpone a real code review to a later time, until that code has been trimmed/processed/adjusted and found to work. I filed bug 1595325 to track that TODO.

FYI, the "enigmail-subset v2" patch is simply a subset of the files already r+'ed by Patrick.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9107681 [details] [diff] [review]
enigmail-subset-rebranded-v2.patch

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

::: mail/extensions/openpgp/content/modules/glodaUtils.jsm
@@ +19,5 @@
> +  // TB with omnijar
> +  GlodaUtils = ChromeUtils.import("resource:///modules/gloda/utils.js").GlodaUtils;
> +}
> +catch (ex) {
> +  // "old style" TB

How many of those crufty things have you got in your patch?

Comment 23

[Patrick Brunschwig]

There are still a few of them. That's the problem with an add-on that needs/needed to work on several versions of Thunderbird. But that one is really really old :-/

Comment 24

[Kai Engert (:KaiE:)]

Attachment: 1594855-rules.patch

Based on the offline conversations, we have the following situation:

• Jörg has raised his concerns, but nevertheless, Magnus, Patrick and I would like to use the suggested approach
• Jörg agreed to tolerate it

It might be useful to formalize and document our special rules for this work, which I'm documenting in a README file in this patch.

Jörg, is my understanding correct that you're willing to tolerate this approach, as stated in the README file?

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9108394 [details] [diff] [review]
1594855-rules.patch

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

You forgot to mention that you will push with DONTBUILD.

::: mail/extensions/openpgp/README.TXT
@@ +13,5 @@
> +- While in the work-in-progress state, owners of this code are:
> +  Kai Engert, Patrick Brunschwig, Magnus Melin
> +
> +- Prior to enabling this code, all code must be enabled for
> +  eslint and must be fully reviewd,  as tracked in:

double space.

Comment 26

[Kai Engert (:KaiE:)]

Attachment: 1594855-rules-v2.patch

Thanks Jörg, updated the rules to mention that.

Magnus?

Comment 27

[Kai Engert (:KaiE:)]

I've done first steps to bring in UI. I've learned that all overlay loading is deprecated, so we probably shouldn't bring in the dynamic overlay loader. I saw we have an existing overlay loader ./common/src/Overlays.jsm but IIUC, that one will also be deprecated soon, when all support for classic XUL Add-ons will be disabled, right?

Because we won't use all of the existing Enigmail UI, it might be less work to selectively integrated in only the pieces that we need, one by one. Looking at messenger.xul, it seems the current approach is using #include statements to load pieces that need to be overloaded.

While only parts of the UI are working consistently, we probably want to keep those UI elements disabled by default, so users of nightly and Beta aren't disturbed by them. (I think we'll need to do that at least for the next 2 months.)

I see two options for doing so.

(1) Add all UI elements to XUL files, but hide them by default. Have dynamic UI/JS code that checks the preference, and dynamically enables them. This requires additional JS code to check that pref and enable UI. That's a bit of unnecessary overhead, because eventually openpgp will no longer be an option, but enabled by default (and that dynamic code can be removed).

(2) Introduce a new build option, --enable-openpgp which defines #MOZ_OPENPGP. This way, we can use #ifdef statements in the main XUL files, and we don't need to dynamically check for a pref.

In both scenarios we need to touch the main TB files that get the integration.

It seems to me that (2) is easier.

Comment 28

[Jorg K (CEST = GMT+2)]

Thanks for looking into this. Yes, I don't think there is any point bringing in the UI bits which are currently add/injected into the existing TB UI using Enigmail's own overlay loader. That one should also not be brought in.

They should go where the S/MIME bits already are, like here:
https://searchfox.org/comm-central/search?q=smime&case=false&regexp=false&path=messengercompose.xul
Using an #ifdef sounds OK together with a build option.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: 1594855-build-config.patch

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 9108555 [details] [diff] [review]
1594855-build-config.patch

Thanks, but I keep out of build stuff if I can (unless I have to hack it).

Comment 31

[Kai Engert (:KaiE:)]

Attachment: 1594855-rules-v3.patch

Updated the rules to enable based on the build option (not the pref)

Comment 32

[Geoff Lankow (:darktrojan)]

Comment on attachment 9108555 [details] [diff] [review]
1594855-build-config.patch

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

I'm not overly familiar with how this file works but this looks good to me.

Comment 33

[Kai Engert (:KaiE:)]

Updated plan for the initial checkin:

• the rules attachment (eslint ignore and readme)

• build config attachment

• land a subset of attachment 9107300 [details] [diff] [review], with r=patrick,
  in particular the following files:

  • all content/modules JS files (from Enigmail)
  • all skin files (from Enigmail, without logo)
  • the prefs file (from Enigmail, plus the engine pref)
  • the initial, partial RNP bindings (3 files in content/modules)
  • 4 files from the content/ui directory, adding the key manager dialog
    and required JS files as the first UI to work on)
    (must port it to work with the RNP library)

• a replacement for attachment 9107301 [details] [diff] [review]
  (will ask Patrick to re-review)

• modifications to core messenger files that will enable the OpenPGP
  code based on the build time variable

Comment 34

[Kai Engert (:KaiE:)]

Attachment: enigmail-initial-integration-v3.patch

[deleted incorrect description]

Comment 35

[Kai Engert (:KaiE:)]

Comment on attachment 9108568 [details] [diff] [review]
enigmail-initial-integration-v3.patch

Sorry, wrong file, this is the patch for Patrick, updating the earlier patch.

Comment 36

[Kai Engert (:KaiE:)]

Attachment: core-messenger-integration.patch

Based on #ifdef MOZ_OPENPGP loads the JS code to start up the OpenPGP integration, and adds one menu entry to the Tools menu (I'm not claiming that this menu entry shall be like this in a release, however, having the ability to view the currently available keys will be a very important functionality during development.)

Comment 37

[Magnus Melin [:mkmelin]]

Comment on attachment 9108562 [details] [diff] [review]
1594855-rules-v3.patch

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

::: mail/extensions/openpgp/README.TXT
@@ +1,1 @@
> +This directory contains an incomplete OpenPGP email integration,

README.txt, or md perhaps?

@@ +12,5 @@
> +
> +- All commits will be done with DONTBUILD in the commit comment,
> +  to avoid unnecessary load on the infrastructure.
> +
> +- While in the work-in-progress state, owners of this code are:

instead of "owners of this code are", maybe "for questions or changes, consult"

Comment 38

[Kai Engert (:KaiE:)]

Attachment: 1594855-rules-v4.patch (r=mkmelin)

Comment 39

[Kai Engert (:KaiE:)]

Attachment: enigmail-initial-integration-v4.patch

This minor change makes decryption with RNP already work, if the decryption key is stored in the RNP keyring and has no password.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/415046cd77f0
Starting point for OpenPGP integration, define rules while work-in-progress. r=mkmelin DONTBUILD
https://hg.mozilla.org/comm-central/rev/e72aa5579de0
Add build option --enable-openpgp. r=darktrojan DONTBUILD
https://hg.mozilla.org/comm-central/rev/4d8319181ab2
Initial import of Enigmail backend modules, skin files, (renamed) prefs, strings. r=patrick DONTBUILD
https://hg.mozilla.org/comm-central/rev/c3f401cda821
Initial set of ctypes bindings to RNP library. r=patrick DONTBUILD
https://hg.mozilla.org/comm-central/rev/881bcd3d0f11
Import key manager UI and helper code from Enigmail. r=patrick DONTBUILD
https://hg.mozilla.org/comm-central/rev/86209ae8455a
initial integration of Enigmail backend code. r=patrick DONTBUILD
https://hg.mozilla.org/comm-central/rev/78130fcb0371
Initial OpenPGP messenger integration, conditional on MOZ_OPENPGP. r=mkmelin DONTBUILD

Comment 41

[Jorg K (CEST = GMT+2)]

DONTBUILD on the last changeset is sufficient :-)

Comment 42

[Kai Engert (:KaiE:)]

Comment on attachment 9107300 [details] [diff] [review]
enigmail-subset-rebranded.patch

Removing the "obsolete" flag from this attachment.

Patrick has r+'ed the import of all of this Enigmail code. We have already imported parts of it, and I'd like to import additional parts as needed. (However, I'll skip parts that we clearly don't need.)

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f0114d8e2104
Import additional parts of Enigmail. r=patrick DONTBUILD

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/388b8b71c9cf
Import JS needed for message reading, subset of reviewed patch. r=patrick DONTBUILD

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/377bb360e57c
Import additional files for key handling and composer, subset of the r+*ed patch. r=patrick DONTBUILD

Comment 46

[Kai Engert (:KaiE:)]

Attachment: gine-liam-0.2.1.3.xpi

Just for reference purposes, this xpi contains the forked, reduced and rebranded snapshot of Enigmail 2.1.3, which is the base for these imports.