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

Key backup: add PUT /room_keys/version/{version} #1

Merged
merged 3 commits into from
Feb 8, 2019

Conversation

manuroe
Copy link

@manuroe manuroe commented Feb 6, 2019

to allow matrix clients to add signatures to an existing backup or update any information in the auth_data field.

…ients to add signatures to an existing backup
Copy link
Owner

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

A couple of minor issues, but overall looks pretty good. I'm wondering, though, if we should say that the server will ensure that the public key in the auth_data is the same as the previous value, though that means that the server must understand the different backup formats.


Body parameters:

- `algorithm` (string): Optional. Must be the same as in the body parameters for `GET
Copy link
Owner

Choose a reason for hiding this comment

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

AIUI, in REST, the contents of PUT are supposed to be the full object that is to be stored, so I think algorithm should be required.

@@ -177,6 +177,42 @@ Error codes:

- `M_NOT_FOUND`: No backup version has been created.

##### `PUT /room_keys/version/{version}`

Update information about the given version, or the current version if `{version}`
Copy link
Owner

Choose a reason for hiding this comment

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

Having the {version} optional seems like it makes it a bit too easy for a client to accidentally overwrite a new backup version with an old backup version. I think we should just make the version required.

@uhoreg
Copy link
Owner

uhoreg commented Feb 6, 2019

There's a race condition in this API for use with updating signatures. If the user is setting up backup on two devices A and B, they both fetch the backup version info, and then both sign the backup info, and then both send their updates to the server, then the server will only end up with one of the signatures. This might not be too bad in practice, but clients should do what they can to avoid this situation (e.g. re-fetching the backup info just before signing and sending, so that they have as much up-to-date information as they can get).

@manuroe
Copy link
Author

manuroe commented Feb 7, 2019

I'm wondering, though, if we should say that the server will ensure that the public key in the auth_data is the same as the previous value, though that means that the server must understand the different backup formats.

Given we specify m.megolm_backup.v1.curve25519-aes-sha2 in this spec, we could require the server to make additional check when encountering that format.

There's a race condition in this API for use with updating signatures

You are right. Client should manage it.

since they're marked as required
@uhoreg uhoreg merged commit e7f7926 into uhoreg:e2e_backup Feb 8, 2019
uhoreg pushed a commit that referenced this pull request Apr 16, 2020
uhoreg pushed a commit that referenced this pull request Nov 23, 2020
uhoreg pushed a commit that referenced this pull request Mar 12, 2021
Reinstate and fix schema validation files
uhoreg pushed a commit that referenced this pull request Nov 9, 2021
Reinstate and fix schema validation files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants