Cross-signing again [1/3] #4970
Conversation
uhoreg
force-pushed
the
uhoreg/e2e_cross-signing2
branch
from
7313284
to
df95f2d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4970 +/- ##
===========================================
- Coverage 60.57% 60.21% -0.36%
===========================================
Files 331 328 -3
Lines 34213 34480 +267
Branches 5655 5721 +66
===========================================
+ Hits 20723 20762 +39
- Misses 12012 12201 +189
- Partials 1478 1517 +39 |
Codecov Report
@@ Coverage Diff @@
## develop #4970 +/- ##
===========================================
- Coverage 63.48% 62.74% -0.74%
===========================================
Files 331 341 +10
Lines 36302 35760 -542
Branches 5990 5870 -120
===========================================
- Hits 23046 22439 -607
- Misses 11617 11720 +103
+ Partials 1639 1601 -38 |
|
(discussed in #synapse-dev) |
uhoreg
force-pushed
the
uhoreg/e2e_cross-signing2
branch
from
5727e5d
to
fc6dab7
Compare
There was a problem hiding this comment.
Sorry for sitting on this for so long.
Fundamentally the problem is that it's big and unwieldy and so a bit hard to follow and therefore hard to get started on.
I've done some review on the first commit, but I'm aware that it's only a small part of this PR. Would you be able to split this into separate PRs, so that we can land them incrementally and thus make better progress on it?
| */ | ||
|
|
||
| -- device signing keys for cross-signing | ||
| CREATE TABLE e2e_device_signing_keys ( |
There was a problem hiding this comment.
delta files need to be idempotent if possible. the easiest option for this is generally to add IF NOT EXISTS to CREATE TABLE/INDEX commands.
It's a bit harder for the ADD COLUMN and normally we don't bother (it's mitigated by making it the last thing in the delta file so that it's unlikely that the change will happen but the schema update not get recorded).
|
|
||
| -- device list needs to know which ones are "real" devices, and which ones are | ||
| -- just used to avoid collisions | ||
| ALTER TABLE devices ADD COLUMN hidden BOOLEAN DEFAULT FALSE; |
There was a problem hiding this comment.
this is going to require a rewrite of the table, which is non-trivial on matrix.org. Can we just make the column nullable?
There was a problem hiding this comment.
I have some queries that look for hidden = false (e.g. 8567ff6#diff-21f2f9be6a999959ddb0f9e3769006ccR53 ). Is there a way to make them still work if the column is nullable?
There was a problem hiding this comment.
You probably need custom sql rather than _simple_select_one. NULL is falsey in both sqlite and postgres so you can do select * from devices where not hidden.
There was a problem hiding this comment.
... no you can't. You need where hidden is not true. https://stackoverflow.com/a/46474204/637864
There was a problem hiding this comment.
... except that doesn't work in sqlite. I hate computers.
select * from devices where not coalesce(hidden, ?); and pass False as a parameter.
It might be easier to ignore hidden in the sql and do the filtering in the python instead.
synapse/storage/end_to_end_keys.py
Outdated
| # since signatures are identified by device ID. So add an entry to the | ||
| # device table to make sure that we don't have a collision with device | ||
| # IDs | ||
| for v in key["keys"].values(): |
There was a problem hiding this comment.
I think I would spell this as pubkey = next(iter(key["keys"].values())), but ymmv.
More helpfully: could you add a comment documenting the shape of key to show why this is the right thing to do?
synapse/storage/end_to_end_keys.py
Outdated
| @@ -281,3 +285,145 @@ def delete_e2e_keys_by_device_txn(txn): | |||
| return self.runInteraction( | |||
| "delete_e2e_keys_by_device", delete_e2e_keys_by_device_txn | |||
| ) | |||
|
|
|||
| def _set_e2e_device_signing_key_txn(self, txn, user_id, key_type, key): | |||
There was a problem hiding this comment.
should this be called _set_e2e_cross_signing_key_txn ?
uhoreg
force-pushed
the
uhoreg/e2e_cross-signing2
branch
from
4b29bcc
to
0a8d70e
Compare
|
I think I've addressed all the concerns. Note that the sytest is set up to run against this PR rather than the last PR, so it fails. I can fix this by either splitting the sytest PR into three, or changing the sytest PR to run against the last PR (which I think I need to do by creating a new sytest PR.) |
There was a problem hiding this comment.
I think this is broadly sane, modulo the nitpicking below.
It's still a bit of a monster, though, and breaking it up even more would be helpful to check we're not missing anything. As a starting point, could you factor out the addition of the hidden column, and the updates to the existing code to support it?
| @@ -0,0 +1 @@ | |||
| Add support for cross-signing. | |||
There was a problem hiding this comment.
| Add support for cross-signing. | |
| Add support for cross-signing devices for end-to-end encryption. |
or something
| if key: | ||
| master_keys[user_id] = key | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
please don't swallow all exceptions with no logging or anything: this will lead to very hard-to-debug problems.
| } | ||
| """ | ||
|
|
||
| PATTERNS = client_patterns("/keys/device_signing/upload$") |
There was a problem hiding this comment.
is this meant to be on r0 or unstable? please can it only be on one of them, anyway?
There was a problem hiding this comment.
I guess it goes on unstable until the MSC goes through?
Together with #5726 and #5727, implements the synapse side of matrix-org/matrix-spec-proposals#1756 (as of the current version)
fixes #4110
This PR implements uploading cross-signing keys. #5726 implements uploading signatures. #5727 implements the federation bits.