e2e: ensure we have both master and self-signing key

[TheJJ]

it seems to be possible that only one of them ends up to be cached.
when this was the case, the missing one was not fetched via federation,
and clients then failed to validate cross-signed devices.

[TheJJ]

Ah, I should explain more why I think this patch helps:
It's the result of debugging issue element-hq/element-web#13997.
Element can't verify the user, since his self-signing key is not delivered via /client/r0/keys/query, but his master-key is.

I'm not sure if this is the correct solution to the problem, but you'll have to guide me there :)

[TheJJ]

All checks are passing now, please advice if this key syncing method is correct, or if I should fix it another way!

[richvdh]

@uhoreg I think you wrote most of this code - do you have any recollection, and does this seem reasonable?

[richvdh]

This looks generally plausible to me, though I have some initial thoughts below.

@uhoreg : I think this was your code originally. Do you have any insights?

[uhoreg]

Looks reasonable. Regarding #8455 (comment) I would guess that it's because if we have neither key, then we assume that the user doesn't have cross-signing set up at all. But it may be worthwhile to check anyways.

[TheJJ]

Ok, it will now always handle a user as "not cached" when any of master or self-signing key is not cached. If no crossigningkeys are there by intention, federation will always be queried. Not sure if this is a good behavior? :)

Apart from this patch, maybe we should investigate how my original problem arised, i.e. that only the master key is cached, but not the self-signing key.

[clokep]

@TheJJ Looks like this is failing some tests, I suspect it is from this branch since it is related to device keys.

[TheJJ]

I don't think it's related since the test failure is about device display names, which I didn't touch. I've rebased the branch to current develop, let's see.

[TheJJ]

Ok, you were right, my change broke the test. It works if I check for (has master key xor has self signing key). If we check for any of master/self signing key is missing, the test fails. I guess the test fails since all non-crossigning-devices tests will issue a federation request, which is unexpected.

[richvdh]

lgtm otherwise

[TheJJ]

Ok, I've updated the comment, and all tests are passing.

[TheJJ]

anything else missing?

[richvdh]

@TheJJ please could you undo the force-push? I can't see what's changed since the previous review.

[TheJJ]

I rebased because some unrelated tests were failling again. The changes after the review:
git diff e9097e5..e3e5c23 synapse/handlers/e2e_keys.py

diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py
index 5d1e0d2d6..929752150 100644
--- a/synapse/handlers/e2e_keys.py
+++ b/synapse/handlers/e2e_keys.py
@@ -167,10 +167,12 @@ class E2eKeysHandler:
                     user_id in cross_signing_keys["self_signing_keys"]
                 )
 
-                # check if only one of the cross-signing master and
-                # self-signing key are cached.
-                # for each user we want the master _and_ the self-signing key,
-                # so we fetch those keys from federation
+                # check if we are missing only one of cross-signing master or
+                # self-signing key, but the other one is cached.
+                # as we need both, this will issue a federation request.
+                # if we don't have any of the keys, either the user doesn't have
+                # cross-signing set up, or the cached device list
+                # is not (yet) updated.
                 if cached_cross_master ^ cached_cross_selfsigning:
                     user_ids_not_in_cache.add(user_id)

[richvdh]

Unfortunately I have no way to confirm what you are telling me without a lot of git-fu.

Could you undo the force-push and add the patch as a separate commit, please? Feel free to merge develop into your branch.

[richvdh]

(cf https://github.com/matrix-org/synapse/blob/develop/CONTRIBUTING.md#updating-your-pull-request)

[TheJJ]

That's why I linked the commit hashes to the github ui, but ok well. Do we rebase to have clean patches after things are approved?
Anyway, I've pushed the old ref again and added a new commit.

[richvdh]

That's why I linked the commit hashes to the github ui, but ok well.

you linked to a massive diff, https://github.com/matrix-org/synapse/compare/e9097e5..e3e5c23?

Please don't rebase. We will squash the branch into a single commit when merging.

[richvdh]

lgtm, once ci completes

[TheJJ]

yay! nice :)

[richvdh]

thank you :)