Add support for dehydrated devices #5239

uhoreg · 2020-09-18T22:02:26Z

Depends on matrix-org/matrix-js-sdk#1436

Implements matrix-org/matrix-spec-proposals#2697

…ther fixes

jryans

I think I am a little confused about how all the pieces fit together, but I'm sure we'll get there. 😅

One thing that I'd like advice on is, when the user enters a passphrase/key to rehydrate the device, it should automatically try to unlock SSSS using the same passphrase/key. But with the current patch, the user has to tell Element that they want to set up SSSS, then say that they want to use a passphrase, and then it will automatically unlock. I'm not sure how to make it not prompt the user, but instead to automatically try to unlock it.

I'm still a bit puzzled on how the dehydration key also functions as a 4S key, but anyway, I'll assume that's a reasonable things for the moment... You could call accessSecretStorage which would eventually rely on getSecretStorageKey to retrieve the 4S key, but I suppose you only want this to happen when dehydration exists and only to try that key silently... so perhaps in some reasonable spot just after login, if you know you have a dehydration key, you could call bootstrapCrossSigning and / or bootstrapSecretStorage directly, but that would still rely on the global getSecretStorageKey helper... maybe this works means it's time for bootstrapFoo functions to take getSecretStorageKey as a parameter, so you can pass a customised version just for the dehydration key case which doesn't use any dialogs?

src/Lifecycle.js

src/Login.js

src/MatrixClientPeg.ts

src/SecurityManager.js

uhoreg · 2020-09-30T05:27:16Z

Still have not managed to get it to not prompt to unlock SSSS if the dehydration key works, but would like a review of the current state anyways.

The test failure is probably because I'm still based on an old version of develop.

src/Lifecycle.js

jryans · 2020-09-30T13:38:21Z

src/Lifecycle.js

+        // If we just logged in, try to rehydrate a device instead of using a
+        // new device.  If it succeeds, we'll get a new device ID, so make sure
+        // we persist that ID to localStorage
+        const newDeviceId = await client.rehydrateDevice();


Is there a way to know that device ID came from rehydration? That would allow us to know the same information as freshLogin without needing an additional flag to track it.

Sorry, I don't really understand this comment. The freshLogin flag tells us whether we just logged in or not. And if so, then it tries to rehydrate the device. So at the point where the freshLogin flag is set, we haven't tried to rehydrate yet. We only want to try to rehydrate if we just logged in so that we don't replace a device that is already in use with the dehydrated device.

I guess I am trying to ask if there is any other way to know whether a rehydration is needed, instead of adding a new flag freshLogin. It would be nice to skip this work if it is not needed, for example.

I see. There's two different issues here. The first is whether there's a dehydrated device ready to be rehydrated. If there isn't a dehydrated device, the rehydrateDevice function will basically only make one call to the server and then return, so it shouldn't be much work.

The second issue is whether we just logged in, which is what the freshLogin flag checks for. If, for example, someone reloads Element, we don't want it to try to dehydrate a device even if there is one available. We want it to keep using the device it was already using. So we only check for a dehydrated device if the user just logged in.

Okay, I think I am following. As an alternative to storing mx_fresh_login in local storage when it is fresh, could we instead rely on knowing that the _restoreFromLocalStorage means we must not be a fresh login, and so it could pass something freshLogin: false through to _doSetLoggedIn without needing extra storage...?

Sorry if this feels a bit pedantic... 😅 I'm trying to finding chances to reduce complexity (even if a tiny amount), since login is already very complex.

could we instead rely on knowing that the _restoreFromLocalStorage means we must not be a fresh login

Unfortunately, we can't do that. When we do an SSO login, it saves the state to storage, and then reloads the page (why? I have no idea), so that means that it will _restoreFromLocalStorage even though it's a fresh login.

jryans · 2020-09-30T13:43:32Z

src/SecurityManager.js

+    if (dehydrationInfo.key) {
+        try {
+            const key = dehydrationInfo.key;
+            if (await MatrixClientPeg.get().checkSecretStorageKey(key, keyInfo)) {


For this to work, doesn't it mean there would need to be a step that attempts creating 4S with the dehydration key as the 4S key? Is that future work, or does some change here cause that to happen and I just haven't spotted it yet...? 🤔

For now, we're not going to create 4S using the dehydration key, but we will unlock 4S using the dehydration key if it matches.

Okay, but then... why do we want to do that, since we know that at the moment, it will always fail since 4S wasn't created with the dehydration key?

It seems like we would only want to start trying dehydration as a 4S key when there's some reason to believe it would work.

Sorry, I missed this question earlier.

Even though we don't create 4S using the dehydration key, we do dehydrate the device using the 4S key, so they keys will be the same.

src/SecurityManager.js

uhoreg · 2020-10-02T04:17:41Z

Not sure why the test is failing. The failing tests seem unrelated to code that I've changed. I'll try to debug more tomorrow, but I think this is otherwise ready for re-review.

src/settings/Settings.ts

jryans · 2020-10-02T17:04:25Z

src/Lifecycle.js

+        // If we just logged in, try to rehydrate a device instead of using a
+        // new device.  If it succeeds, we'll get a new device ID, so make sure
+        // we persist that ID to localStorage
+        const newDeviceId = await client.rehydrateDevice();


I guess I am trying to ask if there is any other way to know whether a rehydration is needed, instead of adding a new flag freshLogin. It would be nice to skip this work if it is not needed, for example.

jryans

A few more changes I think, especially to ensure the feature flag guards the new code paths. It's especially important to be a bit more conservative for changes in the login path like this.

jryans · 2020-10-02T17:17:35Z

src/SecurityManager.js

-function cacheSecretStorageKey(keyId, key) {
+export async function getDehydrationKey(keyInfo, checkFunc) {
+    const inputToKey = makeInputToKey(keyInfo);
+    const { finished } = Modal.createTrackedDialog("Access Secret Storage dialog", "",


This will ask for the dehydration key using UI that talks about Secure Backup, which worries me that users will get lost in yet another key... If we are not yet taking steps to unify 4S and dehydration key (so the dehydration key may exist and be different), then it seems like we need fresh designs for the UI that asks for it, so as to avoid confusion.

I suppose since there's a feature flag now, we wouldn't need to block merging on those designs, but it feels like a concern to me that would block enabling it later.

src/SecurityManager.js

jryans

Since we have a disabled feature flag, this at least seems okay to me from a risk perspective.

However, there are still two points that I am unsure of that would be good to at least get your comments on:

I'm curious if we can simplify the fresh logic detection with less storage and less conditionals, see my comment today on that thread
I still don't understand why we want to try the dehydration key as the 4S key (see thread), so more details on the motivation or why it makes sense would help

jryans · 2020-10-05T16:43:53Z

src/Lifecycle.js

+        // If we just logged in, try to rehydrate a device instead of using a
+        // new device.  If it succeeds, we'll get a new device ID, so make sure
+        // we persist that ID to localStorage
+        const newDeviceId = await client.rehydrateDevice();


Okay, I think I am following. As an alternative to storing mx_fresh_login in local storage when it is fresh, could we instead rely on knowing that the _restoreFromLocalStorage means we must not be a fresh login, and so it could pass something freshLogin: false through to _doSetLoggedIn without needing extra storage...?

Sorry if this feels a bit pedantic... 😅 I'm trying to finding chances to reduce complexity (even if a tiny amount), since login is already very complex.

uhoreg added 5 commits August 31, 2020 14:40

initial version of device rehydration support

1c2e05e

support setting up dehydration from blank account, SSO support, and o…

4398f1e

…ther fixes

Merge branch 'develop' into dehydration

add36ca

fix lint and merge issues

b1b7215

doc fixes and minor code improvement

4e2397a

uhoreg requested a review from a team September 19, 2020 01:27

uhoreg marked this pull request as ready for review September 19, 2020 01:27

uhoreg mentioned this pull request Sep 19, 2020

add support for dehydrated devices #5230

Closed

jryans suggested changes Sep 24, 2020

View reviewed changes

src/Lifecycle.js Outdated Show resolved Hide resolved

src/Login.js Outdated Show resolved Hide resolved

src/MatrixClientPeg.ts Outdated Show resolved Hide resolved

src/MatrixClientPeg.ts Outdated Show resolved Hide resolved

src/SecurityManager.js Outdated Show resolved Hide resolved

dbkr mentioned this pull request Sep 25, 2020

Warn users about e2e when logging out of their last device element-hq/element-web#10242

Closed

update to latest js-sdk changes

744f464

uhoreg mentioned this pull request Sep 30, 2020

Dehydrate and rehydrate devices matrix-org/matrix-js-sdk#1436

Merged

uhoreg requested a review from jryans September 30, 2020 05:27

jryans suggested changes Sep 30, 2020

View reviewed changes

uhoreg added 4 commits October 1, 2020 21:40

try to unlock secret storage with dehydration key

503f329

add a feature flag for dehydration

0db81e4

Merge remote-tracking branch 'upstream/develop' into dehydration

dd57e94

oh right, I need to run the i18n script too

bd9cebe

uhoreg requested a review from jryans October 2, 2020 04:16

jryans reviewed Oct 2, 2020

View reviewed changes

jryans suggested changes Oct 2, 2020

View reviewed changes

uhoreg added 2 commits October 2, 2020 17:43

apply changes from review

c1765c8

Oh right. Run i18n again.

49cc62c

uhoreg requested a review from jryans October 5, 2020 16:05

jryans approved these changes Oct 5, 2020

View reviewed changes

uhoreg merged commit f5f0686 into matrix-org:develop Oct 5, 2020

uhoreg mentioned this pull request Oct 7, 2020

Users in short-lived incognito tabs may never be able to share megolm keys, unless they can dehydrate/rehydrate their device somehow. element-hq/element-web#3825

Closed

nmscode mentioned this pull request Jul 26, 2023

Custom emotes #9240

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for dehydrated devices #5239

Add support for dehydrated devices #5239

uhoreg commented Sep 18, 2020 •

edited

Loading

jryans left a comment

uhoreg commented Sep 30, 2020

jryans Sep 30, 2020

uhoreg Oct 2, 2020

jryans Oct 2, 2020

uhoreg Oct 2, 2020

jryans Oct 5, 2020

uhoreg Oct 5, 2020

jryans Sep 30, 2020

uhoreg Oct 2, 2020

jryans Oct 2, 2020

uhoreg Oct 5, 2020

uhoreg commented Oct 2, 2020

jryans Oct 2, 2020

jryans left a comment

jryans Oct 2, 2020

jryans Oct 2, 2020

jryans left a comment

jryans Oct 5, 2020

Add support for dehydrated devices #5239

Add support for dehydrated devices #5239

Conversation

uhoreg commented Sep 18, 2020 • edited Loading

jryans left a comment

Choose a reason for hiding this comment

uhoreg commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Oct 2, 2020

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Sep 18, 2020 •

edited

Loading