Cross-signing [3/4] -- uploading signatures edition by uhoreg · Pull Request #5726 · matrix-org/synapse

uhoreg · 2019-07-19T21:56:34Z

DO NOT MERGE until uhoreg/e2e_cross-signing2 is merged, and the base of this PR changes to develop

Allows users to upload signatures using the new /keys/signatures/upload endpoint.

The upload_signatures_for_device_keys is really long. I'm not sure what's the best way to make it more manageable.

richvdh

I haven't done a full review here, just some ideas for upload_signatures_for_device_keys. I'm going to wait for #4970 to mature a bit more before proceeding.

synapse/handlers/e2e_keys.py

richvdh · 2019-07-23T12:07:53Z

synapse/handlers/e2e_keys.py

+                            device_id
+                        ] = _exception_to_failure(e)
+            except SynapseError as e:
+                failures[user_id] = {


kinda wondering under what circumstances we are likely to hit this case, and whether it is worth special handling rather than just failing the whole request.

I'm not sure. It's probably a spec discussion about whether the server can fail the whole request, and under what conditions.

…_upload

uhoreg · 2019-09-07T18:37:53Z

There seems to be an intermittent sytest is failure. b6e3dec failed with

not ok 268 Real non-joined user doesn't get events before room made world_readable
# Started: 2019-09-07 17:27:56.835
# Ended: 2019-09-07 17:28:06.949
# Timed out waiting for test at ./run-tests.pl line 727.
# 0.091850: Registered new user @anon-20190907_172406-230:localhost:8828
# 0.108166: Registered new user @anon-20190907_172406-231:localhost:8828
# 0.781076: room_id=!dtQHGsYefGEhtkVyUv:localhost:8828

but d3f2fbc (which only adds documentation) passed. I suspect that it is unrelated to my changes.

I think this is ready for review again.

richvdh · 2019-09-09T16:33:07Z

There seems to be an intermittent sytest is failure. b6e3dec failed with

Indeed. I've raised matrix-org/sytest#696.

richvdh · 2019-09-09T16:36:46Z

please could you link to the MSC or spec this is implementing?

Also: can this target develop rather than an intermediate branch?

richvdh · 2019-09-09T18:07:00Z

Also: can this target develop rather than an intermediate branch?

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

uhoreg · 2019-09-09T19:24:26Z

please could you link to the MSC or spec this is implementing?

matrix-org/matrix-spec-proposals#1756

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

@erikjohnston had suggested that I merge to a separate branch rather than develop. I can do either way, so the two of you can ~~fight it out~~ discuss it, and let me know what I should do.

richvdh · 2019-09-10T11:39:26Z

please could you link to the MSC or spec this is implementing?

matrix-org/matrix-doc#1756

thanks.

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

@erikjohnston had suggested that I merge to a separate branch rather than develop. I can do either way, so the two of you can ~~fight it out~~ discuss it, and let me know what I should do.

I've discussed this with erik a bit, and I think our position goes like this: we prefer to avoid feature branches (especially long-lived ones) because:

if things are in develop, it saves having to repeatedly merge develop into the branch (or run the risk of it bit-rotting)
having a feature branch makes it easy to walk into a trap where PRs can be of poor quality, or introduce bugs, etc, because it's "only a feature branch", which means you rapidly lose track of what is left to be done to clean it up. (Not an issue in this case, I hasten to point out: just a natural consequence.)
in general, it's preferable to think about how to break a large change up so that it can safely land incrementally, rather than (say) splitting it up by file where none of it can land until all of it does - which again makes it easy to lose track of what's left to be done (and in many ways isn't much of an improvement over a single huge PR, since you still end up with the cognitive load of having to hold the whole change in your head at once). Again a feature branch makes it easier to fall into this trap.

On the other hand:

if it's really impossible to land a large change incrementally, we certainly prefer a feature branch with a few PRs over a 1000-line diff (I'm unconvinced that this is a real thing, though).
If you're making DB changes, and there's likely to be more change down the line, that's an argument for keeping things on a feature branch, since if you discover that the original DB schema was a bit wrong, you have to write a migration if the first PR has landed on develop.

TL;DR: If there are no particular reasons why the changes so far can't land on develop, I'd like to see them merged. If there are such reasons, could you identify them?

richvdh

There's a lot of stuff here, and I'm struggling to hold it all in my head, so I'll take your word for it that it's doing about the right thing. A few requests though.

As a general principle: I'd like to see more validation of input (eg checking that things that should be dicts are in fact dicts), so that we return a 400 rather than a 500 in the case of malformed requests.

synapse/handlers/e2e_keys.py

uhoreg · 2019-09-25T01:50:42Z

Sytest failures seem to be new 3pid-related stuff, which will hopefully get fixed when I merge develop in (again). The synapse unit test failure seems to be my old friend test_replace_master_key which is the unit test failure from #5769 that I had thought that I had fixed by using stream ID generators.

Aside from that, though, I think this is ready for review again now. I can fix #5726 (comment) if what I've written there makes sense.

synapse/handlers/e2e_keys.py

richvdh · 2019-10-03T11:08:00Z

synapse/handlers/e2e_keys.py

+                        if (
+                            signing_device_id not in devices
+                            or signing_key_id
+                            not in devices[signing_device_id]["keys"]["keys"]


that sounds sensible, I think. I'd love it if it could be a preliminary PR rather than rolled into this one, though.

richvdh

Once again I'm afraid I've totally paged out most of the context on this PR, but I think it's probably sensible. As well as the nits above, and fixing whatever's wrong with the CI: can we target develop as per #5726 (comment)?

uhoreg · 2019-10-03T12:48:41Z

As well as the nits above, and fixing whatever's wrong with the CI: can we target develop as per #5726 (comment)?

Yes, I'll do that, although I want to try to fix the CI before I merge the previous PRs into develop, so that we don't have a broken CI in develop.

... again? How did you make it disappear, git?

uhoreg · 2019-10-18T22:09:53Z

I think I've addressed all the issues, and I've re-merged develop so the tests pass. I've also changed this PR so that it targets develop, now that the previous PRs have been merged in.

richvdh

Looks plausible!

@uhoreg

@uhoreg has confirmed these were both typos. They are only in comments and tests though, rather than anything critical. Introduced in: * #7157 * #5726

@uhoreg

@uhoreg has confirmed these were both typos. They are only in comments and tests though, rather than anything critical. Introduced in: * matrix-org#7157 * matrix-org#5726

* commit '276173163': Add changelog entry fix doc strings make isort happy add some comments drop some logger lines to debug make changes based on PR feedback add function docs use something that's the right type for user_id run black split out signature processing into separate functions avoid modifying input parameter fix test update with newer coding style add test make isort happy make black happy allow uploading signatures of master key signed by devices implement device signature uploading/fetching

uhoreg changed the title ~~Cross-signing -- uploading signatures edition~~ Cross-signing [2/3] -- uploading signatures edition Jul 19, 2019

uhoreg mentioned this pull request Jul 19, 2019

Cross-signing again [1/3] #4970

Closed

uhoreg force-pushed the uhoreg/e2e_cross-signing2-part2 branch from 9306355 to 9d38b5e Compare July 22, 2019 16:54

uhoreg requested a review from a team July 22, 2019 21:07

richvdh reviewed Jul 23, 2019

View reviewed changes

uhoreg added 10 commits September 4, 2019 20:02

implement device signature uploading/fetching

4bb4544

allow uploading signatures of master key signed by devices

ac4746a

make black happy

7d6c70f

make isort happy

9061b41

add test

5914fd0

update with newer coding style

c8dc740

fix test

e47af0f

avoid modifying input parameter

369462d

split out signature processing into separate functions

561cbba

run black

415d0a0

uhoreg force-pushed the uhoreg/e2e_cross-signing2-part2 branch from 47c5925 to 415d0a0 Compare September 6, 2019 20:55

uhoreg changed the base branch from uhoreg/e2e_cross-signing2 to uhoreg/e2e_cross-signing_merged September 6, 2019 20:55

uhoreg changed the title ~~Cross-signing [2/3] -- uploading signatures edition~~ Cross-signing [3/4] -- uploading signatures edition Sep 6, 2019

uhoreg added 4 commits September 6, 2019 17:52

use something that's the right type for user_id

ab729e3

Merge branch 'develop' into cross-signing_sig_upload

0d61d1d

Merge branch 'uhoreg/e2e_cross-signing_merged' into cross-signing_sig…

b6e3dec

…_upload

add function docs

d3f2fbc

uhoreg requested a review from a team September 7, 2019 18:38

richvdh suggested changes Sep 10, 2019

View reviewed changes

uhoreg added 4 commits September 24, 2019 14:12

make changes based on PR feedback

26113fb

drop some logger lines to debug

39864f4

add some comments

f4b6d43

make isort happy

c3635c9

uhoreg requested a review from a team September 25, 2019 01:51

richvdh reviewed Oct 3, 2019

View reviewed changes

uhoreg mentioned this pull request Oct 10, 2019

make storage layer in charge of interpreting the device key data #6193

Merged

uhoreg added 2 commits October 18, 2019 16:56

fix doc strings

125eb45

Merge branch 'develop' into cross-signing_sig_upload

36adfae

uhoreg changed the base branch from uhoreg/e2e_cross-signing_merged to develop October 18, 2019 17:36

Add changelog entry

6493ed5

... again? How did you make it disappear, git?

uhoreg requested a review from richvdh October 18, 2019 22:04

dbkr added the z-cross-signing-sprint (Deprecated Label) label Oct 22, 2019

richvdh approved these changes Oct 22, 2019

View reviewed changes

uhoreg merged commit 2761731 into develop Oct 22, 2019

richvdh mentioned this pull request Dec 3, 2019

Fix exception when a cross-signed device is deleted #6462

Merged

anoadragon453 mentioned this pull request Jun 4, 2020

Fix encryption algorithm typos in tests/comments #7637

Merged

anoadragon453 added a commit that referenced this pull request Jun 4, 2020

Fix encryption algorithm typos in tests/comments (#7637)

c389bfb

@uhoreg has confirmed these were both typos. They are only in comments and tests though, rather than anything critical. Introduced in: * #7157 * #5726

DMRobertson deleted the uhoreg/e2e_cross-signing2-part2 branch June 28, 2022 11:19

Uh oh!

Conversation

uhoreg commented Jul 19, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

richvdh Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

uhoreg Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

uhoreg commented Sep 7, 2019

Uh oh!

richvdh commented Sep 9, 2019

Uh oh!

richvdh commented Sep 9, 2019

Uh oh!

richvdh commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uhoreg commented Sep 9, 2019

Uh oh!

richvdh commented Sep 10, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uhoreg commented Sep 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

uhoreg commented Oct 3, 2019

Uh oh!

uhoreg commented Oct 18, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richvdh commented Sep 9, 2019 •

edited

Loading