1552177 - OTR chat: private key generation should be done in the background, remove status UI

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, enhancement)

Description

[Kai Engert (:KaiE:)]

Currently, when it's necessary to generate an OTR private key for a chat account, we'll show a status dialog.

Despite this action being very fast on most machines, and as a consequence, users are most likely not bothered much by this status dialog, Florian/Patrick complained that this kind of UI exists at all, that the UI should be removed, and instead have requested that all key generation should be performed in the background.

This will require us to rewrite the respective code.

Comment 1

[Kai Engert (:KaiE:)]

see also: https://github.com/arlolra/ctypes-otr/issues/20

Comment 2

[Kai Engert (:KaiE:)]

I measured on a 2.8 ghz quad core CPU, the key generation takes between 100 and 200ms. The consumed time varies. Key generation involves searching for random numbers, and (for example) testing if they are prime. It takes an unpredictable amount of time until good numbers are found.

(The time required might increase in the future, if it's found that the current key sizes we're using can be attacked too easily, then we'll change to use larger numbers.)

Our initial code (as checked in) is already able to generate the key in a background worker.
This is already being triggered, at the time a new account gets added, without showing any status UI.
(For the patch in bug 1550487, I'm also triggering background generation on prefs being opened, without bringing up any status UI.)

The open question is, when should keys for existing accounts get generated?
Florian has suggested to create all missing keys immediately after startup, in background threads.
I'm worried that approach is insufficient.

What if we relied completely on the background generation, and never blocked if a key is required immediately?

Let's imagine the following scenario:
Alice has 10 chat accounts configured in Thunderbird.
Alice upgrades to Thunderbird with OTR support.
Alice has a slower computer, which needs 1 second per generated key, so OTR will be fully available after 10 seconds uptime, at the earliest.

As soon as she starts up Thunderbird, a queued up message arrives, and the Thunderbird chat system wants to process it. That queued up message is from Bob, who has been using OTR software already. His message is in fact a request to start an encrypted OTR session.
Now that Thunderbird supports OTR, TB tries to process that message, and it wants to immediately start the OTR session. However, if the background generation isn't done yet, because the 10 seconds haven't passed yet, OTR processing will fail.

If it fails, Alice might be confused. Why does she get an error message about OTR not being supported, despite just having updated to an OTR capable client?

I'd prefer that scenario to work immediately. In order for that to work, we cannot rely completely on background processing. If the key isn't available yet, we should fall back to synchronous key generation.

With the current implementation, the above scenario works.
We display a status dialog "currently generating key", which is shown for a really short amount of time (e.g. less than 200ms on the test system I mentioned).

However, showing that dialog has a beneficial side effect. It temporarily blocks the processing of the incoming chat message. It delays it, until after the key generation has been completed. Then the dialog is automatically closed, and the processing of the incoming "OTR start" request can continue. And it can actually work, because Alice has her own key now!

(If you prefer, the fall back to synchronous generation could be done without showing the status dialog.)

Comment 3

[Kai Engert (:KaiE:)]

Attachment: experimental patch, remove key generation dialog, still generate synchronously if required

Comment 4

[Kai Engert (:KaiE:)]

In short, I'm OK to try to generate all missing OTR keys in the background, immediately after startup.

However, because of the explanation given in comment 2, I'd prefer to keep a fallback, that generates the synchronously, if it's required immediately. (Without or without showing a status dialog, up to you.)

Comment 5

[Florian Quèze [:florian]]

I don't mind a sync fallback if it's barely ever going to be encountered by users, but I'm afraid that means it'll receive very little testing. Is it possible to make the otr processing wait for the end of the async key generation? Sending or receiving messages is async by nature, so it seems this should be possible.

Comment 6

[Kai Engert (:KaiE:)]

OTR messages are regular messages. They must be processed in order with all other messages.

You'd require a mechanism in the TB chat that allows a message, which is currently being processed, to be moved back to a pending state, which other messages queueing up, until we're ready to continue. Then we'd have to process the message that we had pushed back, and the process any of the other queued up messages. Sounds complicated to me.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: 1552177-v2.patch

This seems to work.

After OTR is done with init, it sets a timeout for 3 seconds.
That will queue a list of all currently known accounts.

After a 1 second timeout, one element from the queue is popped, and checked, if it already has a private key.
If not, key generation is started on a worker thread.

Once the worker thread notifies that its done, we'll continue with the above (1 second timeout, check next account).

Once we've checked all accounts, no more timeout is set.

I've tested the above with a fresh setup (no keys yet), and starting an OTR conversation immediately (prior to the 3 second delay).

It worked, and logging told me, the check noticed that a key was already there for one of the accounts, and didn't create another one for it.

Note this patch is on top of the one from bug 1550487.

Comment 8

[Kai Engert (:KaiE:)]

Florian, do you prefer keeping or removing the status dialog for the sync fallback? I'd prefer to keep it, but your call.

Comment 9

[Florian Quèze [:florian]]

A dialog means an exchange of information with the user. If there's no action the user needs to take, it's a monolog and isn't something we should do.

Also, setTimeout isn't a good way to start background work without being in the user's way. ChromeUtils.idleDispatch is likely to help here.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1552177-v3.patch

Changed as requested.

This is ready to review, but I think you want to review bug 1550487 first. However, if you prefer to have a single review request for both bug 1550487 and this one, let me know, and I can merge it into the open review request at https://phabricator.services.mozilla.com/D32024

Comment 11

[Kai Engert (:KaiE:)]

Attachment: 1552177-v3.patch

Removed "eval" from the new code, it's unnecessary for main thread code.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: 1552177-v4.patch

Comment 13

[Kai Engert (:KaiE:)]

Attachment: 1552177-v5.patch

lint etc.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: otr-prefs

Bug 1552177

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Bug 1552177 - OTR chat: private key generation should be done in the background, remove status UI. r=florian

Depends on D32837

Comment 16

[Kai Engert (:KaiE:)]

Attachment: 1552177-ready.patch [checked in]

patch ready to land

This patch is on top of the patch from bug 1550487.

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 9069594 [details] [diff] [review]
1552177-ready.patch [checked in]

needed for OTR feature

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe88d825d3c6
OTR chat: private key generation should be done in the background, remove status UI. r=mkmelin

Comment 19

[Kai Engert (:KaiE:)]

Magnus, in the review, you had suggested to change

genNextMissingKey() {
if (!OTRUI.accountsToGenKey.length) {

to
genNextMissingKey() {
if (!OTRUI.accountsToGenKey) {

I had changed it, and that's what we have checked in.

The intention was to leave the function, if the object exists, but is empty.
That doesn't seem to work.

Today I notice that this change caused a JS exception a couple lines afterwards:
TypeError: acc is undefined OTRUI.jsm:205:14

We get this error, because we call OTRUI.accountsToGenKey.pop() on an empty array, which returns an undefined object.

Apparently your change doesn't accomplish that. I suggest we revert it to my earlier code.

You had said:
"both because we prefer falsy comparsisons, and because when comparing with length it would be nicer to compare > 0"

I think it's impossible that length would ever be negative.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: 1552177-empty-array-v1.patch

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9069944 [details] [diff] [review]
1552177-empty-array-v1.patch

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

::: chat/modules/OTRUI.jsm
@@ +196,5 @@
>  
>    accountsToGenKey: [],
>  
>    genNextMissingKey() {
> +    if (!OTRUI.accountsToGenKey.length) {

Ah sorry, didn't realize it was an array. 

Please use 
if (OTRUI.accountsToGenKey.length == 0) {

Comment 22

[Kai Engert (:KaiE:)]

Attachment: 1552177-empty-array-v2.patch

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/911d98783d65
Follow-up: Fix check for array length. r=mkmelin

Comment 24

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/bbe010237d94154eff7997ccfda9e0f5fbc92a25
https://hg.mozilla.org/releases/comm-beta/rev/713e61c38ff001a075807319261d5e76e5e2d42d