Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Cross-signing [2/4] - upload/download keys #5769

Merged

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Jul 25, 2019

This is just #4970, with #5759 split off.

Created a new PR so that the review comments don't disappear, since this would be a force-push.

uhoreg added 2 commits July 24, 2019 23:21
This is a prerequisite for cross-signing, as it allows us to create other things
that live within the device namespace, so they can be used for signatures.
@uhoreg uhoreg requested a review from a team July 25, 2019 15:18
synapse/storage/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/types.py Outdated Show resolved Hide resolved
@uhoreg
Copy link
Member Author

uhoreg commented Aug 2, 2019

There seems to be a race condition in the unit test, but I'm not sure how to fix it.

@uhoreg uhoreg requested a review from a team August 2, 2019 02:31
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 looking sane, a few nits

synapse/storage/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/storage/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
@@ -145,3 +147,66 @@ def test_claim_one_time_key(self):
"one_time_keys": {local_user: {device_id: {"alg1:k1": "key1"}}},
},
)

@defer.inlineCallbacks
def test_replace_master_key(self):
Copy link
Member

Choose a reason for hiding this comment

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

as a general note for future reference, we're moving away from this style of test towards ones which (a) inherit from HomeserverTestCase to instantiate a Homeserver with a mock reactor and (b) exercise the REST API rather than the mid-layer of the handlers.

@richvdh
Copy link
Member

richvdh commented Aug 13, 2019

There seems to be a race condition in the unit test, but I'm not sure how to fix it.

Yeah, odd. Can you reproduce it locally? If so, and if you turn the verbosity right up (SYNAPSE_TEST_LOG_LEVEL=DEBUG), do the logs (particularly the SQL queries) enlighten you?

@uhoreg
Copy link
Member Author

uhoreg commented Aug 21, 2019

OK, I think I know what's up with the unit test. When I fetch the cross-signing key, I order by timestamp and pick the first one. But the timestamp only has 1-second resolution, and the unit tests run too fast, so the two keys get stored with the timestamp. So when it fetches the keys, it basically picks one at random. (I guess the fact that it's never happened on my laptop means that my laptop is slow enough that the keys don't have the same timestamp. Or it's just extremely lucky.)

So, what's the best way of ensuring that the key that was last stored is the one that gets selected? Normally, I would use an autoincrement, but it seems that the synapse db doesn't have any autoincrement fields.

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (uhoreg/e2e_cross-signing_merged@72d296a). Click here to learn what that means.
The diff coverage is 49.38%.

@@                        Coverage Diff                         @@
##             uhoreg/e2e_cross-signing_merged    #5769   +/-   ##
==================================================================
  Coverage                                   ?   63.45%           
==================================================================
  Files                                      ?      331           
  Lines                                      ?    36668           
  Branches                                   ?     6068           
==================================================================
  Hits                                       ?    23267           
  Misses                                     ?    11729           
  Partials                                   ?     1672

@uhoreg uhoreg changed the base branch from uhoreg/e2e_cross-signing2-part0 to uhoreg/e2e_cross-signing_merged August 29, 2019 00:34
@uhoreg
Copy link
Member Author

uhoreg commented Aug 29, 2019

Switching to using stream ID generator seems to have fixed the tests.

The sytest failure I think is not-my-fault, since it's only failing in the worker sytest, and the sytest that's running against it isn't even calling any cross-signing functionality, but it's hard to tell because it doesn't seem to be copying the sytest artifacts properly.

hawkowl and others added 7 commits August 30, 2019 23:13
Adds a flag to `/versions`' `unstable_features` section indicating that this Synapse understands what an `id_access_token` is, as per #5927 (comment)

Fixes #5927
Python will return a tuple whether there are parentheses around the returned values or not.

I'm just sick of my editor complaining about this all over the place :)
* fix thumbnail storage location

Signed-off-by: Lorenz Steinert <[email protected]>

* Add changelog file.

Signed-off-by: Lorenz Steinert <[email protected]>

* Update Changelog

Signed-off-by: Lorenz Steinert <[email protected]>
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 now, modulo the build failure. Apparently @hawkowl has updated the build scripts to fix the artifact uploads, so could you merge latest develop in?

@richvdh
Copy link
Member

richvdh commented Sep 2, 2019

Sounds like you've fixed it, either way, but:

But the timestamp only has 1-second resolution,

1 millisecond, surely?

anoadragon453 and others added 15 commits September 2, 2019 18:29
These methods were part of the v1 C/S API. Remove them as they are no longer used by any code paths.
Trace device list changes.
Remove all the "double return" statements which were a result of us removing all the instances of

```
defer.returnValue(...)
return
```

statements when we switched to python3 fully.
* Ensure an auth instance is available to ListMediaInRoom

Fixes #5737

* Changelog
…D/GID (#5970)

Adjust su-exec to only be used if needed.

If UID == getuid() and GID == getgid() then we do not need to su-exec, and chmod will not work.
* Ensure the list media admin API is always available

This API is required for some external media repo implementations to operate (mostly for doing quarantine operations on a room).

* changelog
Previously the stats were not being correctly populated.
Removes the `bind_email` and `bind_msisdn` parameters from the `/register` C/S API endpoint as per [MSC2140: Terms of Service for ISes and IMs](https://github.com/matrix-org/matrix-doc/pull/2140/files#diff-c03a26de5ac40fb532de19cb7fc2aaf7R107).
@uhoreg uhoreg merged commit 19bb5c8 into uhoreg/e2e_cross-signing_merged Sep 4, 2019
@DMRobertson DMRobertson deleted the uhoreg/e2e_cross-signing2-part1 branch June 28, 2022 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.