Closed Bug 1675342 Opened 4 years ago Closed 4 years ago

Support batch accepting OpenPGP keys at import time

Categories

(MailNews Core :: Security: OpenPGP, enhancement)

Product:
MailNews Core
Component:
Security: OpenPGP
Type:
enhancement

Tracking

(thunderbird_esr78 fixed, thunderbird84 fixed)

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- fixed
thunderbird84 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin
47 bytes, text/x-phabricator-request
wsmwk
: approval-comm-beta+
KaiE
: approval-comm-esr78+
Details | Review
import-accept.png
77.23 KB, image/png
Details

Currently we require that each correctponent's key is individually marked as accepted.

We have received a request to offer a mechanism that can be used to mark multiple keys as accepted at once, ideally in a way that works with Thunderbird 78.

We are very limited what new UI we can add on the stable 78 branch (mostly, because we don't want to introduce new user interface text), and also we want to avoid big code changes or big enhancements.

The question is, can we find a way to make this possible with a minimal amount of changes.
Here is a suggestion:

At the time we're importing a set of public keys (e.g. from a file), we currently start by reading the list of keys, and then we show a prompt, asking for import confirmation. Currently this dialog says "Import the following keys?" followed by a short or long list of names and email addresses.

We could extend this dialog by showing an additional check box - which is deselected by default. When checked, we could mark all imported keys as "accepted but unverified".

(This wouldn't change the acceptance state if a key is already present with an acceptance state of rejected or verified.)

The tricky question is, what text should be shown next to the checkbox.

Our choices are
(1) introduce a new english text, which wouldn't get localized, and be shown as english in all localized versions of Thunderbird
(2) use an existing text, which mostly describes what we're doing, which could be slightly inaccurate.

For (2), we could show a combination of the following existing strings:

  • above the checkbox, we'd show "Do you accept this key for verifying digital signatures and for encrypting messages?"
  • the checkbox label itself would say: "Yes, but I have not verified that it is the correct key."

Alessandro, Magnus, do you think we should use (2) with the above text?

If you're worried about people being misled (because the message is in singular, while we're processing multiple keys, and also because we don't show all the other explanation text usually found in the key details and acceptance configuration), then we'd have to go with (1).

I think (2) might be OK for users who have already seen the key details and acceptance dialog.

We could go on step further, and pref this additional feature off by default. In other words, by default we don't change anything in the UI. We introduce a new hidden pref like "mail.openpgp.enable_experimental_code", false by default. If true, we'd then offer the checkbox described above.

Note that the intention of this bug is to strictly start with an implementation that is suitable for backporting to the 78 branch, and thus needs to use a trade off. We can have a follow-up bug to make it nicer for Thunderbird 90 afterwards.

Implementation note for myself: The existing prompt uses EnigmailDialog.confirmDlg, which opens enigmailMsgBox.xhtml. That dialog already supports a checkbox. We could add another optional <description> above the checkbox, for the introduction text that we need here.

This dialog (the "Import the following keys?") has got to be one of the most terrible UI we have. I'd suggest not even trying to reuse that, but to take a small leap ahead. The text currently overflows outside the dialog if you have more then a few keys (try resizing the dialog), there's no formatting at all, etc.

It appears to me using the import dialog and wanting to set the key acceptances should be a fairly common usage; e.g. copying over your keys to another computer. That is, you'd want to also be able to set verified in case you know you verified on the other computer where you exported the key(s).

I would like the import to show a list of keys and allow to take action on them all or individually. Something like this, similar to how we now do import events from .ics files:

+------- Import Public Keys ---------------------------------------------------+

Found 2 keys to import:

 ----------------------------------------------------------------------------
User ID: John Doe <john@example.com>
Key ID: 0x1234567890123456
Acceptance: [ Yes, accept this key ]

                                                 [ Import Key ]
 ----------------------------------------------------------------------------
User ID: Jane Doe <jane@example.com>
Key ID: 0x2234567890123456
Acceptance: [ Not yet, I'll handle acceptance later ]

                                                 [ Import Key ]
  ---------------------------------------------------------------------------

Before using a a key you need to assign trust to it. You can choose to 
accept the keys later, or you can now specify how you want to handle the
keys you are importing.

Accept these keys for verifying digital signatures and for encrypting messages:
  x Yes, accept these keys.
  o Not yet, I'll handle acceptance later.
  o Yes, and mark verifed. These are fully verified keys from my other computer.
  
  [ Apply to All ]


                                                 [ Cancel ] [ Import All ]

+------------------------------------------------------------------------------+

Yes, this would mean new strings. I believe for Fluent strings you'd just get the English string if localized is not available (should verify that). The other (worse) option would be to put this new dialog behind a pref and leave the old behind (+remove from trunk)

The dialog to import private keys (e2e page -> Add Key -> Import an existing openpgp key) has a very good workflow with step by step screens to guide the user in selecting which key should be treated as personal.
At the end of the import process, a quick recap of all imported keys + a button to change the acceptance level is returned.

I'd recommend using the same dialog and allowing the import of public keys, and using the same interface to allow changing the acceptance level during import, and the trust level after import.

Are you sure you'd want to add such a big chunk of code to the 78 branch?

I think only some small parts of that can be reused. The key issue in this bug is setting the acceptance level for keys. The flow you mention doesn't really do anything for that (it's irrelevant for your private key).

I have another idea, which is much simpler.

We could add a new preference, to define the default acceptance setting when importing a new key.

Today, the default acceptance is "undecided".
We could allow the user to change the default to "accept imported keys, unverified".

In order to solve this bug, a user could temporarily change the setting, then import the file, then change the preference back.
This workaround wouldn't require any changes to the user interface.

When importing a file with this pref set to "automatically accept imported keys", and we're importing a key that we already have, but the key is currently in the "undecided" state, we'd also update its acceptance.

Today, we never import a key automatically.
Importing is done, only, if the user has requested it in some way, for example, by requesting the importing of a file, or by asking to discover a key.

Because of our requirement to manually request and confirm an import, some users could decide to permanently run with this pref set to "automatically accept every imported key as unverified". (Although I personally wouldn't recommend it.)

Magnus, does this make sense to you?

Flags: needinfo?(mkmelin+mozilla)

Because of our requirement to manually request and confirm an import, some users could decide to permanently run with this pref set to "automatically accept every imported key as unverified".

That's like the Enigmail option "trust all keys by default". It's a good choice for certain users who primarily want to evade mass surveillance.

Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #2)

This dialog (the "Import the following keys?") has got to be one of the most terrible UI we have.

don't disagree

I'd suggest not even trying to reuse that, but to take a small leap ahead. The text currently overflows outside the dialog if you have more then a few keys (try resizing the dialog), there's no formatting at all, etc.

That means implementing a new dialog - because the current one is based on a general-purpose minimal dialog.

It appears to me using the import dialog and wanting to set the key acceptances should be a fairly common usage; e.g. copying over your keys to another computer. That is, you'd want to also be able to set verified in case you know you verified on the other computer where you exported the key(s).

This migration scenario cannot be handled with a one-checkbox-for-everything decision. If you want to migrate, then we'll need a new export mechanism, that can copy the individual acceptance state together with each key.

You're asking for something that could be useful, but is outside the reduced scope of this bug.

I would like the import to show a list of keys and allow to take action on them all or individually. Something like this, similar to how we now do import events from .ics files:

I think we shouldn't try add this complex UI on 78.x.

Note that we already offer a way to control the acceptance for each imported key individuall. After importing, a large summary is shown. For each imported key, you're able to open the detail dialog.

This sequence of dialogs would no longer make sense, after having added your suggested UI, so it would probably have to be removed.

On the other hand, the existing UI is more powerful, because it allows you to view the complete details of each imported key. And those details could influence the user's decision to accept a key or not.

At the very least, your mock up lacks the following two very important details: The fingerprint, and the full list of names/email addresses. Because if a key contains two email addresses, where the second one is using a fake email, in an attempt to trick you, the user wouln't notice they are being tricked.

Also, I believe a dialog which can offer "either" import individually "or" treat all the same, will probably need a careful user interface design consideration. Something I'm not sure we should do as part of this bug - because time is limited to improve many details on 78.

What is being asked here is a hotfix, until we have something better. I'd like to hotfix to be as minimal as possible.

If we prefer to add UI, but a minimal UI, then we could do this:

  • enhance the current dialog to show all email addresses for each key, not just the first one as it's currently doing
  • include the fingerprint (not short ID)
  • add a single radio button choice at the bottom, which will be used for all keys
  • the radio button choice would be the same 4-way choice that we offer in the key details dialog: reject, undecided, unverified, verified

Sounds good.

I'm making progress with the better import confirmation dialog.

I've added the 4 radio buttons, but I'm not very happy with it.
I'm worried that people might accidentally select the "reject" choice for an import of many keys.

I think the import dialog shouldn't replace existing decisions - it should only replace the "undecided" state in my opinion.

If we allow the user to import 100 keys and mark all of them as rejected, that would be very cumbersome for the user to clean up.
Similarly, I feel like allowing the user to mark many keys as verified, in a very easy way, that people might do it without fully understanding what they are doing.
Because this new dialog will be about "many" keys, the wording from the key details dialog cannot really be reused.

I think we should do one of the following:

(a) Only offer the two choices for the acceptance state "undecided" and "accepted, unverified".

(b) Offer all 4 choices, including "rejected" and "verified". However, if the user selects either rejected or verified, we'll bring up an additional confirmation prompt, asking the user to confirm again, using new wording to remind the user of the consequences.

I'm leaning towards (a), because:

  • it can be fully implemented using existing strings
  • no new warning language required at this time
  • sufficient for the scope of this bug
  • we'll be motivated to improve this further in the future, to offer the additional choices with better UI

If we go with option (a), should we then add a footnote letting the user know that they will need to access the properties of those keys to verify them if they want to properly use them?

I agree in terms of implementing option (b) in a follow up bug, and do something similar to the Key Wizard, where we list the keys of the selected file and then we ask again to confirm the import before proceeding.
We could have that extra confirmation step only if the user selects rejected or verified and more than 1 key is currently being imported, in order to highlight the "severity" of this action.

Attachment #9188302 - Attachment description: Bug 1675342 - Add pref to automatically accept OpenPGP public key as accepted/unverified on import. r=PatrickBrunschwig → Bug 1675342 - Part 1, add RNP to auto accept keys at import time. r=mkmelin
Attached image import-accept.png —

The updated patch no longer introduces a pref.
Instead, it adds the new prompt to all scenarios in which we import public keys.

Magnus, can you please re-review?

Attachment #9188302 - Attachment is obsolete: true
Attachment #9188966 - Attachment description: Bug 1675342 - Part 2, improve public key import prompt, add UI to accept as unverified. r=mkmelin → Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin
Depends on: 1665145
Attachment #9188966 - Attachment is obsolete: true
Attachment #9188966 - Attachment is obsolete: false

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/2cc8e0ff3637
Improve public key import prompt, add UI to accept as unverified. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Comment on attachment 9188966 [details]
Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: feature unavailable
Testing completed (on c-c, etc.): landed on c-c
Risk to taking this patch (and alternatives if risky): low, shouldn't impact existing functionality

Attachment #9188966 - Flags: approval-comm-beta?

Comment on attachment 9188966 [details]
Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin

[Triage Comment]
Approved for beta

Attachment #9188966 - Flags: approval-comm-beta? → approval-comm-beta+

Could someone help to test that migration from Enigmail still works with this change? Because it touches import code.
Ideally by testing using a TB 78 build that contains this patch.

I can prepare such a build.

Samba, I know you were waiting for this - so feedback/testing from you would be appreciated.
I'll provide download links to the experimental build as soon as the above build progresses.

Comment on attachment 9188966 [details]
Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: Users have less comfort in accepting many keys when migrating to TB 78
Testing completed (on c-c, etc.): landed on c-c
Risk to taking this patch (and alternatives if risky): low, shouldn't impact existing functionality

Attachment #9188966 - Flags: approval-comm-esr78?

Comment on attachment 9188966 [details]
Bug 1675342 - Improve public key import prompt, add UI to accept as unverified. r=mkmelin

got a=wsmwk for 78.5.1 in chat

Attachment #9188966 - Flags: approval-comm-esr78? → approval-comm-esr78+

Thank you very much for following up with this and for the change!

From the test I made with a debian buster I was I was able to install tb 78.5.1
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/W2y39E9tQ6KO1pKZhjEXAw/runs/0/artifacts/public/build/target.tar.bz2

Then, after the account setup, I imported from URL these keys
https://keys.accessnow.org/000_all_accessnow.asc

And I've been prompted to accept all the key in bulk (see screenshot)
https://share.riseup.net/#p22KohD_kIRa_jBXQ5xv8Q

For test purpose I decided to press No and see if this option is available somewhere else, but I couldn't find anything in the menu or by selecting a multiple keys and press the mouse right button. Then I had to removed all the keys and imported them again in order to test the acceptance again.

Do you know if there a way to accept the keys in bulk once they are already imported ?

Apart this missing option, which maybe require another bug report, I think is ok to consider the bulk option is working fine when importing a sets of keys.

so thank you very much,
samba

Samba, thanks for the feedback.

Note that you don't need to delete the keys. You can just import them again, and the key data will be merged. If "accept" is chosen in the import dialog, then the acceptance for existing keys will be updated, if the current acceptance for existing keys is still "undecided".

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: