e2e: return newly fetched cross-signing keys #10912

TheJJ · 2021-09-24T14:53:09Z

without this patch, only the cached cross-signing keys are returned when
a client requests them, although a federation request is done
in case there's no cache entry.
this patch returns newly fetched entries directly.

This is a stripped-down version of #10668, which needs some discussion and/or root cause analysis.

without this patch, only the cached cross-signing keys are returned when a client requests them, although a federation request is done in case there's no cache entry. this patch returns newly fetched entries directly. Signed-off-by: Jonas Jelten <[email protected]>

clokep · 2021-09-30T12:00:26Z

synapse/handlers/e2e_keys.py

+                    self_sig_key = cross_signing_keys["self_signing_keys"].get(
+                        user_id, {}
                    )
+                    cached_cross_selfsigning = bool(self_sig_key)


I'm not sure if I'm missing something, but it looks like this logic is identical to the previous version?

+1. I found the original easier to understand (and has the advantage of not introducing yet another local variable to this massive function).

The logic is not quite identical: the proposed code would behave differently if user_id was present in the self-signing keys dict but set to a falsey value, like an empty dict.

I wonder if this might more clearly express intent:

Suggested change

self_sig_key = cross_signing_keys["self_signing_keys"].get(

user_id, {}

)

cached_cross_selfsigning = bool(self_sig_key)

cached_cross_selfsigning = bool(cross_signing_keys["self_signing_keys"].get(user_id)

The question then becomes: are there scenarios where this could happen, and would we be better off preventing those instead? (E.g., stripping falsey values from the dicts before testing for membership)

From looking at get_cross_signing_keys_from_cache(), it may be possible for a falsey value to end up in cross_signing_keys["self_signing_keys"].

There's a call tree that's basically:

get_cross_signing_keys_from_cache()

store.get_e2e_cross_signing_keys_bulk() (Docstring: "If a user's cross-signing keys were not found, either their user ID will not be in the dict, or their user ID will map to None.")

store._get_bare_e2e_cross_signing_keys_bulk_txn() (Docstring: "If a user's cross-signing keys were not found, their user ID will not be in the dict.")

So worth figuring out what's actually true. Or being defensive here.

(Also worth seeing if this code survived Erik's refactor in the related PR)

The same code is present in the current version of e2e_keys.py, so still worth digging into.

richvdh

Thanks for making this contribution! Sorry it's been sitting for a while - I think the problem is that nobody on the team understands the changes here.

I'm afraid I'm really struggling to understand what is changing here, and why. I'm not familiar with the existing code, and it's very hard to understand. (I think it was written by someone outside the synapse core team - we certainly wouldn't have a single function over 250 lines long normally).

In other words - we could really do with your help to understand what's changing here, and why. Any comments you can add to the PR to help us understand would be appreciated.

Also: please can you add a regression test to tests/handlers/test_e2e_keys.py demonstrating the problem?

richvdh · 2021-09-30T12:05:25Z

changelog.d/10912.bugfix

@@ -0,0 +1 @@
+return E2E cross signing keys fetched over federation when to a client.


This doesn't really make sense? Please can you expand it into something that will be meaningful to other synapse admins.

Also, do we know when this bug was introduced? Please include that info.

richvdh · 2021-09-30T12:08:05Z

synapse/handlers/e2e_keys.py

+                        # here, we fetch the user's device list
+                        # and cross signing keys,
+                        # and update our cache with them.


please
use
longer
lines

richvdh · 2021-09-30T12:12:30Z

synapse/handlers/e2e_keys.py

-                    # cross-signing set up, or the cached device list
-                    # is not (yet) updated.
+                    # cross-signing set up, or we did not fetch the
+                    # cross-signing keys yet since the device list is not (yet) updated.


I'm not really following this. Why would the device list not be updated, and why does it cause us to not fetch the cross-signing keys?

Also, is it relevant to the bug being fixed in this PR, or is it a separate correction you're doing at the same time? (It doesn't matter either way - I'm just trying to understand what this PR is fixing!)

richvdh · 2021-09-30T12:13:55Z

synapse/handlers/e2e_keys.py


-                # check for missing cross-signing keys.
-                for user_id in remote_queries.keys():


so we're now doing the following code just on the results of the query, rather than the devices we asked for. That may well be valid, but can you explain why we need to make this change?

richvdh · 2021-09-30T12:15:44Z

synapse/handlers/e2e_keys.py

-                    for device_id, device in devices.items():
-                        keys = device.get("keys", None)
-                        device_display_name = device.get("device_display_name", None)
-                        if keys:
-                            result = dict(keys)
-                            unsigned = result.setdefault("unsigned", {})
-                            if device_display_name:
-                                unsigned["device_display_name"] = device_display_name
-                            user_devices[device_id] = result


It looks like this code is moving further down. Can you explain why we need to do that?

richvdh · 2021-09-30T12:16:57Z

synapse/handlers/e2e_keys.py

@@ -247,6 +254,15 @@ async def do_remote_query(destination: str) -> None:
                        user_results = results.setdefault(user_id, {})
                        for device in user_devices:
                            user_results[device["device_id"]] = device["keys"]
+
+                        # update the result with the devicelist's cross-signing keys


can you expand this comment a bit to say why this is necessary?

richvdh · 2021-09-30T12:18:10Z

synapse/handlers/e2e_keys.py

+                        cross_signing_keys["master_keys"][user_id] = user_devices.get(
+                            "master_key"
+                        )
+                        cross_signing_keys["self_signing_keys"][
+                            user_id
+                        ] = user_devices.get("self_signing_key")


this will overwrite any existing values in cross_signing_keys. Is that correct behaviour?

richvdh · 2022-01-07T14:03:06Z

unfortunately this has now bitrotted. @TheJJ if you're still interested in contributing this then that would be wonderful, but in the meantime I'm going to go ahead and close it.

TheJJ force-pushed the send-fetched-crosssigning-keys branch from 7a64bb9 to 5ab2291 Compare September 24, 2021 14:53

TheJJ mentioned this pull request Sep 24, 2021

e2e: fetch cross-signing keys when device is signed but no key cached #10668

Closed

erikjohnston requested a review from a team September 24, 2021 15:01

clokep reviewed Sep 30, 2021

View reviewed changes

clokep requested a review from a team September 30, 2021 12:00

richvdh suggested changes Sep 30, 2021

View reviewed changes

erikjohnston mentioned this pull request Oct 29, 2021

Some device signatures are missing when doing key/query other federation #11159

Closed

erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Oct 29, 2021

erikjohnston mentioned this pull request Nov 2, 2021

Include cross-signing signatures when syncing remote devices for the first time #11234

Merged

richvdh closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: return newly fetched cross-signing keys #10912

e2e: return newly fetched cross-signing keys #10912

TheJJ commented Sep 24, 2021 •

edited

Loading

clokep Sep 30, 2021

richvdh Sep 30, 2021

callahad Jan 7, 2022

callahad Jan 7, 2022

callahad Jan 7, 2022

richvdh left a comment

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh Sep 30, 2021

richvdh commented Jan 7, 2022

		@@ -0,0 +1 @@
		return E2E cross signing keys fetched over federation when to a client.


		# check for missing cross-signing keys.
		for user_id in remote_queries.keys():

e2e: return newly fetched cross-signing keys #10912

e2e: return newly fetched cross-signing keys #10912

Conversation

TheJJ commented Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jan 7, 2022

TheJJ commented Sep 24, 2021 •

edited

Loading