Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for device dehydration v2 (Element R) #4062

Merged
merged 27 commits into from
Apr 11, 2024

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Feb 12, 2024

No description provided.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM other than a few suggestions which may or may not be helpful

spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
spec/integ/crypto/device-dehydration.spec.ts Outdated Show resolved Hide resolved
spec/integ/crypto/device-dehydration.spec.ts Show resolved Hide resolved
src/rust-crypto/dehydration.ts Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Apr 4, 2024

I'm a bit concerned that the API is moderately complicated, and that we're expecting each application to know when to call isDehydrationSupported, rehydrateDeviceIfAvailable, scheduleDeviceDehydration etc.

I've been thinking about this a bit more, and it's continuing to nag at me.

In particular: we already have a bit of a mess with APIs like isSecretStorageReady, bootstrapSecretStorage, isCrossSigningReady, bootstrapCrossSigning: they have unclear semantics, and it's unclear when the application is meant to call them, and in what order. This is mostly because they were defined back in 2018 when we thought it was "obvious" what they did and when you should call them, but then they evolved and now it's really unclear.

I'm really worried that we're compounding this situation by adding a bunch more methods that the application is supposed to call at some ill-defined time during startup.

For example:

  • Is it necessary that secret storage is correctly configured before calling the dehydration methods? What is the expected behaviour of these methods if it is not configured?
  • We are likely to see every possible combination of (new/existing session), (account with/without secret storage), (device with/without cached secret storage key), (account with/without cross-signing), (device verified/not verified), (device with/without access to master cross-signing keys). I'm not sure which, if any, of those are relevant, but I do feel like we should make any constraints absolutely explicit.

The React SDK isn't doing much with scheduleDeviceDehydration currently -- it never uses the delay parameter. This will come in a future iteration, where the application needs to store the time when a dehydrated device was last created, so that when you re-start, it will be able to re-schedule dehydration. I had considered making that part of the js-sdk, but the js-sdk doesn't seem to have a good place to store things like that. (In the past, I would have thrown it in the crypto store, but we don't have access to that any more, and the rest of the SDK doesn't seem to have a good place to easily persist random bits of information.)

A couple of things on this:

  • The MatrixClient is initialised with a Store. It's typically used for caching the last /sync response, but we could conceivably extend it to store this sort of information. The thing about Store is that it is a cache: if it is blown away, then we can resync from the server without loss of information. But that might be ok here?
  • If it's important, we can and should use the rust-side store for this sort of information. It probably means pushing more of this functionality down into the Rust layer, but that may not be a bad thing?

Comment on lines 543 to 544
* If the delay is omitted or 0, this function's promise will resolve after
* the first dehydrated device is created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If the delay is omitted or 0, this function's promise will resolve after
* the first dehydrated device is created.
* If the delay is omitted or 0, this function's promise will resolve after
* the first dehydrated device is created. Otherwise, it resolves immediately.

@uhoreg
Copy link
Member Author

uhoreg commented Apr 4, 2024

The React SDK isn't doing much with scheduleDeviceDehydration currently -- it never uses the delay parameter. This will come in a future iteration, where the application needs to store the time when a dehydrated device was last created, so that when you re-start, it will be able to re-schedule dehydration. I had considered making that part of the js-sdk, but the js-sdk doesn't seem to have a good place to store things like that. (In the past, I would have thrown it in the crypto store, but we don't have access to that any more, and the rest of the SDK doesn't seem to have a good place to easily persist random bits of information.)

A couple of things on this:

* The MatrixClient is initialised with a `Store`. It's typically used for caching the last `/sync` response, but we could conceivably extend it to store this sort of information. The thing about `Store` is that it is a _cache_: if it is blown away, then we can resync from the server without loss of information. But that might be ok here?

* If it's important, we can and should use the rust-side store for this sort of information. It probably means pushing more of this functionality down into the Rust layer, but that may not be a bad thing?

Having thought a bit more about this, I don't think it's critical that we need to store that information. If we replace the dehydrated device early, nothing bad will happen, so on restart, we can just create a new dehydrated device right away, and continue from there with the normal scheduling. So I can change the schedule... function to always create a new dehydrated device right away. I'll also give it a default period.

Regarding the larger API concerns, I'll give it some thought, about whether I can throw things into bootstrapCrossSigning

@uhoreg
Copy link
Member Author

uhoreg commented Apr 8, 2024

I've implemented a simpler API. Now, clients just need to check if dehydration is available, and then tell it to start dehydration, and it'll do everything necessary. Clients can re-start dehydration if it, e.g. needs to create a new dehydration key when it resets secret storage.

@uhoreg uhoreg requested a review from richvdh April 8, 2024 01:47
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Excellent, this seems better! Still a couple of concerns.

src/crypto-api.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/crypto-api.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM!

@uhoreg uhoreg added this pull request to the merge queue Apr 11, 2024
Merged via the queue into matrix-org:develop with commit 936e7c3 Apr 11, 2024
21 checks passed
@uhoreg uhoreg deleted the dehydration_v2 branch April 11, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants