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

MSC1219: storing megolm keys serverside #1538

Merged
merged 36 commits into from
Oct 29, 2019
Merged

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Aug 18, 2018

proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
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
Copy link
Member

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.

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

Copy link
Member

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.

Copy link
Member Author

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.

proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
@NotAFile

This comment has been minimized.

@ara4n

This comment has been minimized.

@NotAFile

This comment has been minimized.

@uhoreg

This comment has been minimized.

@uhoreg uhoreg changed the title [WIP] storing megolm keys serverside storing megolm keys serverside Oct 30, 2018
@uhoreg uhoreg requested a review from a team October 30, 2018 04:43
@uhoreg
Copy link
Member Author

uhoreg commented Sep 10, 2019

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

@turt2live turt2live self-requested a review September 16, 2019 21:14
Copy link
Member

@turt2live turt2live left a 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)

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.

generally looks good to me, though I have a few nits.

proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
@uhoreg uhoreg requested a review from dbkr October 4, 2019 14:28
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Copy link
Member

@dbkr dbkr left a 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
Copy link
Member

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.

proposals/1219-storing-megolm-keys-serverside.md Outdated Show resolved Hide resolved
@uhoreg uhoreg requested review from dbkr and richvdh October 10, 2019 19:07
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!

@dbkr dbkr removed their request for review October 22, 2019 09:27
@ahmedcharles
Copy link

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.

@uhoreg
Copy link
Member Author

uhoreg commented Oct 23, 2019

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 <23707194+aditsachde@users.noreply.github.com>
@turt2live
Copy link
Member

MSC has passed FCP - merging.

@turt2live turt2live merged commit ce286da into matrix-org:master Oct 29, 2019
@joepie91
Copy link

Am I correct in observing that part of this MSC hasn't actually made it into the spec yet (specifically the m.megolm_backup.v1 account data type, possibly other stuff also), even though SSSS is now merged and therefore that is unblocked?

Not sure if this is on the radar already, considering the issue tags.

@uhoreg
Copy link
Member Author

uhoreg commented Jul 26, 2020

Am I correct in observing that part of this MSC hasn't actually made it into the spec yet (specifically the m.megolm_backup.v1 account data type, possibly other stuff also), even though SSSS is now merged and therefore that is unblocked?

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.

@richvdh richvdh mentioned this pull request Jan 18, 2022
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023
…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
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.