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.

Sign up for GitHub

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

Jump to bottom

initial version of spec for key backups #2387

Merged
merged 19 commits into from
Jun 2, 2020
Merged

initial version of spec for key backups #2387

merged 19 commits into from
Jun 2, 2020

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Dec 16, 2019 •
edited
Loading

Spec PR for #1219 / #1538 and #2526.

Does not have any mentions of SSSS or cross-signing keys, since those parts are not written up yet.

Rendered version of changes at https://12751-24998719-gh.circle-artifacts.com/0/scripts/gen/client_server/unstable.html#server-side-key-backups

@turt2live turt2live mentioned this pull request Dec 16, 2019
Closed
@uhoreg @ara4n
Apply suggestions from code review
a62c817 
Co-Authored-By: Matthew Hodgson <matthew@arasphere.net>
@ara4n
Copy link
Member

ara4n commented Jan 7, 2020

lgtm from medium-to-high level review :)

uhoreg and others added 3 commits February 7, 2020 15:28
@uhoreg @ara4n
Apply suggestions from code review
41072fc 
Co-Authored-By: Matthew Hodgson <matthew@arasphere.net>
@uhoreg
add note indicating similarity between key backup format and key exports
e67ba0c 
also copy description of forwarding_curve25519_key_chain from key exports, since
it's a better description
@uhoreg
remove extra space
30586ed
@uhoreg uhoreg requested a review from a team March 20, 2020 21:00
turt2live
turt2live requested changes Mar 27, 2020
View reviewed changes
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.

Generally looks fine and is definitely understandable - thank you! I mostly have stylistic concerns here (full stops at end of sentences, error responses, indentation, etc).

api/client-server/definitions/key_backup_data.yaml
@@ -0,0 +1,50 @@
# Copyright 2019 The Matrix.org Foundation C.I.C.
Copy link
Member

Choose a reason for hiding this comment

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

😢 that this has been stuck in the queue this long

@uhoreg uhoreg mentioned this pull request May 5, 2020
Merged
@uhoreg
Copy link
Member Author

uhoreg commented May 6, 2020 •
edited
Loading

Waiting for #2526, and I still need to check error codes.

@uhoreg uhoreg requested a review from turt2live May 8, 2020 20:55
turt2live
turt2live approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally looks sane. I haven't verified accuracy to the proposals in this review, but the explanations look sensible.

I might come by at a later point and review accuracy when time is more available.

Edit: LGTM modulo comments below.

uhoreg and others added 2 commits June 1, 2020 22:34
@uhoreg @turt2live
Apply suggestions from code review
40c50c8 
Co-authored-by: Travis Ralston <travpc@gmail.com>
@uhoreg
more clarifications and fixes
a362848
@uhoreg uhoreg merged commit cf37688 into matrix-org:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ara4n ara4n ara4n left review comments

@richvdh richvdh richvdh left review comments

@turt2live turt2live turt2live approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@uhoreg @ara4n @turt2live @richvdh