Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add support for dehydrated devices #5239

Merged
merged 12 commits into from
Oct 5, 2020
Merged

Add support for dehydrated devices #5239

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1c2e05e
initial version of device rehydration support
uhoreg Aug 31, 2020
4398f1e
support setting up dehydration from blank account, SSO support, and o…
uhoreg Sep 3, 2020
add36ca
Merge branch 'develop' into dehydration
uhoreg Sep 18, 2020
b1b7215
fix lint and merge issues
uhoreg Sep 18, 2020
4e2397a
doc fixes and minor code improvement
uhoreg Sep 19, 2020
744f464
update to latest js-sdk changes
uhoreg Sep 30, 2020
503f329
try to unlock secret storage with dehydration key
uhoreg Oct 2, 2020
0db81e4
add a feature flag for dehydration
uhoreg Oct 2, 2020
dd57e94
Merge remote-tracking branch 'upstream/develop' into dehydration
uhoreg Oct 2, 2020
bd9cebe
oh right, I need to run the i18n script too
uhoreg Oct 2, 2020
c1765c8
apply changes from review
uhoreg Oct 2, 2020
49cc62c
Oh right. Run i18n again.
uhoreg Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/Lifecycle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ export function attemptTokenLogin(queryParams, defaultDeviceDisplayName) {
console.log("Logged in with token");
return _clearStorage().then(() => {
_persistCredentialsToLocalStorage(creds);
// remember that we just logged in
sessionStorage.setItem("mx_fresh_login", true);
return true;
});
}).catch((err) => {
Expand Down Expand Up @@ -312,6 +314,9 @@ async function _restoreFromLocalStorage(opts) {
console.log("No pickle key available");
}

const freshLogin = sessionStorage.getItem("mx_fresh_login");
sessionStorage.removeItem("mx_fresh_login");

console.log(`Restoring session for ${userId}`);
await _doSetLoggedIn({
userId: userId,
Expand All @@ -321,6 +326,7 @@ async function _restoreFromLocalStorage(opts) {
identityServerUrl: isUrl,
guest: isGuest,
pickleKey: pickleKey,
freshLogin: freshLogin,
}, false);
return true;
} else {
Expand Down Expand Up @@ -364,6 +370,7 @@ async function _handleLoadSessionFailure(e) {
* @returns {Promise} promise which resolves to the new MatrixClient once it has been started
*/
export async function setLoggedIn(credentials) {
credentials.freshLogin = true;
stopMatrixClient();
const pickleKey = credentials.userId && credentials.deviceId
? await PlatformPeg.get().createPickleKey(credentials.userId, credentials.deviceId)
Expand Down Expand Up @@ -429,6 +436,7 @@ async function _doSetLoggedIn(credentials, clearStorage) {
" guest: " + credentials.guest +
" hs: " + credentials.homeserverUrl +
" softLogout: " + softLogout,
" freshLogin: " + credentials.freshLogin,
);

// This is dispatched to indicate that the user is still in the process of logging in
Expand Down Expand Up @@ -462,10 +470,28 @@ async function _doSetLoggedIn(credentials, clearStorage) {

Analytics.setLoggedIn(credentials.guest, credentials.homeserverUrl);

MatrixClientPeg.replaceUsingCreds(credentials);
const client = MatrixClientPeg.get();

if (credentials.freshLogin) {
jryans marked this conversation as resolved.
Show resolved Hide resolved
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (newDeviceId) {
credentials.deviceId = newDeviceId;
}

delete credentials.freshLogin;
}

if (localStorage) {
try {
_persistCredentialsToLocalStorage(credentials);

// make sure we don't think that it's a fresh login any more
sessionStorage.removeItem("mx_fresh_login");

// The user registered as a PWLU (PassWord-Less User), the generated password
// is cached here such that the user can change it at a later time.
if (credentials.password) {
Expand All @@ -482,12 +508,10 @@ async function _doSetLoggedIn(credentials, clearStorage) {
console.warn("No local storage available: can't persist session!");
}

MatrixClientPeg.replaceUsingCreds(credentials);

dis.dispatch({ action: 'on_logged_in' });

await startMatrixClient(/*startSyncing=*/!softLogout);
return MatrixClientPeg.get();
return client;
}

function _showStorageEvictedDialog() {
Expand Down
1 change: 1 addition & 0 deletions src/MatrixClientPeg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export interface IMatrixClientCreds {
accessToken: string;
guest: boolean;
pickleKey?: string;
freshLogin?: boolean;
}

// TODO: Move this to the js-sdk
Expand Down
109 changes: 96 additions & 13 deletions src/SecurityManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,19 @@ import RestoreKeyBackupDialog from './components/views/dialogs/security/RestoreK
// single secret storage operation, as it will clear the cached keys once the
// operation ends.
let secretStorageKeys = {};
let secretStorageKeyInfo = {};
let secretStorageBeingAccessed = false;

let dehydrationInfo = {};

export function cacheDehydrationKey(key, keyInfo = {}) {
dehydrationInfo = {key, keyInfo};
}

export function getDehydrationKeyCache() {
return dehydrationInfo;
}

function isCachingAllowed() {
return secretStorageBeingAccessed;
}
Expand Down Expand Up @@ -66,6 +77,20 @@ async function confirmToDismiss() {
return !sure;
}

function makeInputToKey(keyInfo) {
return async ({ passphrase, recoveryKey }) => {
if (passphrase) {
return deriveKey(
passphrase,
keyInfo.passphrase.salt,
keyInfo.passphrase.iterations,
);
} else {
return decodeRecoveryKey(recoveryKey);
}
};
}

async function getSecretStorageKey({ keys: keyInfos }, ssssItemName) {
uhoreg marked this conversation as resolved.
Show resolved Hide resolved
const keyInfoEntries = Object.entries(keyInfos);
if (keyInfoEntries.length > 1) {
Expand All @@ -78,17 +103,21 @@ async function getSecretStorageKey({ keys: keyInfos }, ssssItemName) {
return [keyId, secretStorageKeys[keyId]];
}

const inputToKey = async ({ passphrase, recoveryKey }) => {
if (passphrase) {
return deriveKey(
passphrase,
keyInfo.passphrase.salt,
keyInfo.passphrase.iterations,
);
} else {
return decodeRecoveryKey(recoveryKey);
}
};
// if we dehydrated a device, see if that key works for SSSS
if (dehydrationInfo.key) {
try {
const key = dehydrationInfo.key;
if (await MatrixClientPeg.get().checkSecretStorageKey(key, keyInfo)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Save to cache to avoid future prompts in the current session
cacheSecretStorageKey(keyId, key, keyInfo);
dehydrationInfo = {};
return [name, key];
}
} catch {}
dehydrationInfo = {};
}

const inputToKey = makeInputToKey(keyInfo);
const { finished } = Modal.createTrackedDialog("Access Secret Storage dialog", "",
AccessSecretStorageDialog,
/* props= */
Expand Down Expand Up @@ -118,14 +147,54 @@ async function getSecretStorageKey({ keys: keyInfos }, ssssItemName) {
const key = await inputToKey(input);

// Save to cache to avoid future prompts in the current session
cacheSecretStorageKey(keyId, key);
cacheSecretStorageKey(keyId, key, keyInfo);

return [keyId, key];
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

AccessSecretStorageDialog,
/* props= */
{
keyInfo,
checkPrivateKey: async (input) => {
const key = await inputToKey(input);
try {
checkFunc(key);
return true;
} catch (e) {
return false;
}
},
},
/* className= */ null,
/* isPriorityModal= */ false,
/* isStaticModal= */ false,
/* options= */ {
onBeforeClose: async (reason) => {
if (reason === "backgroundClick") {
return confirmToDismiss();
}
return true;
},
},
);
const [input] = await finished;
if (!input) {
throw new AccessCancelledError();
}
const key = await inputToKey(input);
// need to copy the key because rehydration (unpickling) will clobber it
cacheDehydrationKey(key, keyInfo);
return key;
}

function cacheSecretStorageKey(keyId, key, keyInfo) {
if (isCachingAllowed()) {
secretStorageKeys[keyId] = key;
secretStorageKeyInfo[keyId] = keyInfo;
}
}

Expand Down Expand Up @@ -176,6 +245,7 @@ export const crossSigningCallbacks = {
getSecretStorageKey,
cacheSecretStorageKey,
onSecretRequested,
getDehydrationKey,
};

export async function promptForBackupPassphrase() {
Expand Down Expand Up @@ -262,6 +332,18 @@ export async function accessSecretStorage(func = async () => { }, forceReset = f
await cli.bootstrapSecretStorage({
getKeyBackupPassphrase: promptForBackupPassphrase,
});

const keyId = Object.keys(secretStorageKeys)[0];
if (keyId) {
const dehydrationKeyInfo =
secretStorageKeyInfo[keyId] && secretStorageKeyInfo[keyId].passphrase
? {passphrase: secretStorageKeyInfo[keyId].passphrase}
: {};
console.log("Setting dehydration key");
await cli.setDehydrationKey(secretStorageKeys[keyId], dehydrationKeyInfo);
} else {
console.log("Not setting dehydration key: no SSSS key found");
}
}

// `return await` needed here to ensure `finally` block runs after the
Expand All @@ -272,6 +354,7 @@ export async function accessSecretStorage(func = async () => { }, forceReset = f
secretStorageBeingAccessed = false;
if (!isCachingAllowed()) {
secretStorageKeys = {};
secretStorageKeyInfo = {};
}
}
}