-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC1219: storing megolm keys serverside #1538
Conversation
On 403 error when trying to `PUT` keys: | ||
|
||
1. get the current version | ||
2. notify the user that there is a new backup version, and display relevant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be exposing multiple backup versions to the user: i.e. "Online backup started by device at ", to help protect the user against a malicious attacker starting a new backup to a) overwrite their actual keys or b) steal their keys. We could also let the user delete their online backups (after doing UI auth) to help them control their keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have an opinion on what other metadata we should be exposing to the user, but I think we can put it in a future MSC if we think it's worthwhile. Clients already check that a backup is trusted before storing keys in a backup, so I think the risk of an attacker creating a new backup to steal keys is low.
|
||
¹: cross-signing (when that is completed) can be used to verify the device | ||
that signed the key. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be very useful to spell out the sort of flows we expose via settings as per the original proposal: i.e. the ability to explicitly recover keys from a backup; change (rotate) the recovery key; delete the backup(s); and start a (new) backup. Particularly in terms of how much we should re-auth the user before each operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been mentioned, but doesn't go into much detail. I don't think we need to go into too much detail, since I don't think we should be mandating too much UI-wise in the spec.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think this one is finally ready. This is one of the weird ones where the MSC issue is different from the MSC PR, but I think I call for FCP on the PR? @mscbot fcp merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clearly this is working well in practice, and I'm not a crypto expert at all. API structure looks sane though.
(and yes - this is a legacy MSC so the FCP needs to be called on the issue, which it seems like you've done)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good to me, though I have a few nits.
Co-Authored-By: Richard van der Hoff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm generally but some comments outstanding - particularly rich's
[MSC1687](https://github.com/matrix-org/matrix-doc/issues/1687)), or both. If | ||
the key is saved directly by the user, then the code is constructed as follows: | ||
|
||
1. The 256-bit curve25519 private key is prepended by the bytes `0x8B` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitcoin uses a similar prefix and also uses base58 encoding, so picking something similar and not-colliding seemed polite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Just curious, has there been any consideration around placing this data into generic account data? (Or even storing account data as room state?) Having a single good api which can store arbitrary json values seems better than having many different apis for storing very specific json values. Room state is effectively a distributed key/value store and account data (along with lots of other very specific apis) is just key/value data. This would allow servers and clients to communicate in generic ways, which would increase consistency. |
Yes, I've thought about using account data, but key backup has some extra functionality that doesn't fit well into the current account data model (e.g. determining whether one version of a key is better than another), and that doesn't seem useful to any other uses of account data. Same with room state. |
Co-Authored-By: aditsachde <[email protected]>
MSC has passed FCP - merging. |
Am I correct in observing that part of this MSC hasn't actually made it into the spec yet (specifically the Not sure if this is on the radar already, considering the issue tags. |
That's correct. I was going to wait until cross-signing was merged, and then merge the SSSS changes for both at the same time. Though since SSSS is now merged, I guess it makes sense to merge the change for this, and then throw the SSSS change for cross-signing straight into the cross-signing PR. |
…around releases (matrix-org#1538) * Add a spec release checklist issue template because I'm tired of copy/paste * Document a chunk of our release approach This should probably go elsewhere, but here is fine for now as a SCT-referenced doc/content. * changelog * Brief clarifications
proposal document for #1219
Rendered: https://github.com/uhoreg/matrix-doc/blob/e2e_backup/proposals/1219-storing-megolm-keys-serverside.md