1985684 - Port bug 1974184: Enable AES support for encrypting logins with the SDR and migrate all existing logins

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, enhancement)

Description

[Geoff Lankow (:darktrojan)]

We've got a test failure I don't quite understand, and we should trigger this migration in the same way Firefox has. The ESR 140 login manager does not support AES so migrating would break all logins on a downgrade to 140.

The test in question is test_imapPasswordFailure.js, it installs premade key4.db and logins.json files, which work initially then when a login is updated they fail. I'm not sure why this is, surely there should be backwards compatibility here. I could make the test pass by creating new versions of the files, but I'm not sure that should be necessary.

Comment 1

[Geoff Lankow (:darktrojan)]

Oh and the SMTP, POP3, and NNTP versions of the test also fail.

Comment 2

[Geoff Lankow (:darktrojan)]

In a separate issue, if I port the migration changes, the migration fails with a rejection we can't catch, when there's no passwords to reencrypt:

FAIL test_migrateTryStartTLS - [test_migrateTryStartTLS : 266] A promise chain failed to handle a rejection: Need at least one ciphertext to decrypt - stack: decryptMany@resource://gre/modules/crypto-SDR.sys.mjs:226:24

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1985684 - Update logins files for tests. r=kaie

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1985684 - Migrate existing logins to use AES. r=kaie

This is a backwards-incompatible change. A profile that goes through this migration will not have
usable logins if downgraded to ESR 140.

Comment 5

[Kai Engert [:KaiE:]]

Despite the attached patches, we're trying to understand what is causing the failure, because the assumption is that it shouldn't fail.

I see we execute the test twice, once with maildir and once with mbox.

If I disable either one of the tests in mailnews/imap/test/moz.build then the failure is gone.

However, it doesn't appear to be a simple race, because enabling both and running the test with "test --sequential" we still fail.

With both tests enabled and --sequential, and some more logging, we still have 1 login saved after resetTest, but after performTest there are 0 logins saved. Same for both tests that are running one after the other.

I cannot yet explain.

Comment 6

[Kai Engert [:KaiE:]]

Explanation:

Our password testing, and password prompting simulation code, will call an async function for storing the known-good password:
Services.logins.addLoginAsync()

As soon as we reach that function, it apparently removes the old stored password.

Our test code continues immediately, and the new login data has not yet been stored. We're not waiting for it.
Waiting for addLoginAsync() would probably require to change a lot of our password prompting code to use async functions and await.
(functions promptPassword and promptUsernameAndPassword and call the callers)

In theory we could try to change our logic to call nsILoginManager.modifyLogin instead, because that's a synchronous function. But comments say that function is only retained for Thunderbird compatibility, not sure how long that function will continue to be available. So it wouldn't be a long term fix.

The easiest fix is to use the new files that Geoff provided, which already contain the new password, which means that during testing we don't need to re-encrypt/change the passwords, so we simply avoid that temporarily no password is stored.

Comment 7

[Kai Engert [:KaiE:]]

I'll attach a patch that uses another callback to report the promise for saving the login.
It seems a bit ugly, but it fixes the failures.
Do you want that, or maybe it inspires you to find a cleaner fix?

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Bug 1985684 - Wait for updated logins when testing IMAP passwords. r=darktrojan

Comment 9

[Kai Engert [:KaiE:]]

(In reply to Kai Engert [:KaiE:] from comment #7)

I'll attach a patch that uses another callback to report the promise for saving the login.
It seems a bit ugly, but it fixes the failures.
Do you want that, or maybe it inspires you to find a cleaner fix?

While that fixes the IMAP test, we're still failing in various other tests, which would require a similar fix.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c359a8c4d19e869aa998f4a001d048681fba007d

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b4ab2bdda63c
Update logins files for tests. r=kaie

Comment 11

[Malte Jürgens [:maltejur]]

(In reply to Geoff Lankow (:darktrojan) from comment #2)

In a separate issue, if I port the migration changes, the migration fails with a rejection we can't catch, when there's no passwords to reencrypt:

FAIL test_migrateTryStartTLS - [test_migrateTryStartTLS : 266] A promise chain failed to handle a rejection: Need at least one ciphertext to decrypt - stack: decryptMany@resource://gre/modules/crypto-SDR.sys.mjs:226:24

FYI, this is also a problem in Firefox. I am taking a look at it in Bug 1987673.

Comment 12

[Kai Engert [:KaiE:]]

Geoff, could you please r+ this one?
I made the change you asked for
https://phabricator.services.mozilla.com/D262952

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2edfc6ae9e4f
Migrate existing logins to use AES. r=kaie

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/35842fc1e059
Wait for updated logins when testing IMAP passwords. r=darktrojan

Comment 15

[Francesco]

This seems to have an incorrect Milestone since the 2025-10-22 was during the 146 cycle. It also shows up on
https://www.thunderbird.net/en-US/thunderbird/146.0beta/releasenotes/