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

Fix bug where we kept re-requesting a remote server's key repeatedly. #16257

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Fix bug where we kept re-requesting a remote server's key repeatedly. #16257

merged 2 commits into from
Sep 5, 2023

Conversation

erikjohnston
Copy link
Member

We have two tables:

  1. server_keys_json which stores the raw JSON that we got, and is unique on (server_name, key_json, from_server).
  2. server_signature_keys which stores the JSON fields as columns, and is unique on (server_name, key_json)

When fetching keys from the store in Keyring, we used the first table. However, the fetch function didn't correctly account for multiple rows per server / key ID, and so would return whichever row happened to be returned last by Postgres. (This is often the most recent row, but also often not).

The fix here is: a) fix the read path to return the most recently added key JSON, and b) use the server_signature_keys instead for Keyring as we don't need the full JSON anyway.

@erikjohnston erikjohnston marked this pull request as ready for review September 5, 2023 16:08
@erikjohnston erikjohnston requested a review from a team as a code owner September 5, 2023 16:08
clokep
clokep suggested changes Sep 5, 2023
View reviewed changes
synapse/crypto/keyring.py Outdated
@@ -511,7 +511,7 @@ async def _fetch_keys(
for key_id in queue_value.key_ids
)

res = await self.store.get_server_keys_json(key_ids_to_fetch)
res = await self.store.get_server_signature_keys(key_ids_to_fetch)
Copy link
Member

Choose a reason for hiding this comment

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

This essentially reverts #15417 and will cause large amounts of federation traffic. See #15171.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh boo, for some reason I thought that the both perspective and direct key fetcher persisted in there 🤦

Copy link
Member

Choose a reason for hiding this comment

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

Opposite, see #15171 (comment) they both store into server_keys_json.

@clokep
Copy link
Member

clokep commented Sep 5, 2023

See also #15463.

@clokep clokep mentioned this pull request Sep 5, 2023
Open
@erikjohnston erikjohnston force-pushed the erikj/fix_key_fetching branch from d07cbc8 to e8a217f Compare September 5, 2023 18:19
@erikjohnston
Copy link
Member Author

@clokep I've reverted the second set change. I've rebased (sorry about that) to only be the basic fix

@erikjohnston erikjohnston requested a review from clokep September 5, 2023 18:19
clokep
clokep approved these changes Sep 5, 2023
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems OK.

@erikjohnston erikjohnston merged commit c9cec2d into develop Sep 5, 2023
37 checks passed
@erikjohnston erikjohnston deleted the erikj/fix_key_fetching branch September 5, 2023 19:27
yingziwu added a commit to yingziwu/synapse that referenced this pull request Oct 3, 2023
@yingziwu
Merge tag 'v1.93.0' into develop
e159835 
No significant changes since 1.93.0rc1.

The following issues are fixed in 1.93.0 (and RCs).

- [GHSA-4f74-84v3-j9q5](GHSA-4f74-84v3-j9q5) / [CVE-2023-41335](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-41335) — Low Severity

  Temporary storage of plaintext passwords during password changes.

- [GHSA-7565-cq32-vx2x](GHSA-7565-cq32-vx2x) / [CVE-2023-42453](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-42453) — Low Severity

  Improper validation of receipts allows forged read receipts.

See the advisories for more details. If you have any questions, email [email protected].

- Add automatic purge after all users have forgotten a room. ([\matrix-org#15488](matrix-org#15488))
- Restore room purge/shutdown after a Synapse restart. ([\matrix-org#15488](matrix-org#15488))
- Support resolving homeservers using `matrix-fed` DNS SRV records from [MSC4040](matrix-org/matrix-spec-proposals#4040). ([\matrix-org#16137](matrix-org#16137))
- Add the ability to use `G` (GiB) and `T` (TiB) suffixes in configuration options that refer to numbers of bytes. ([\matrix-org#16219](matrix-org#16219))
- Add span information to requests sent to appservices. Contributed by MTRNord. ([\matrix-org#16227](matrix-org#16227))
- Add the ability to enable/disable registrations when using CAS. Contributed by Aurélien Grimpard. ([\matrix-org#16262](matrix-org#16262))
- Allow the `/notifications` endpoint to be routed to workers. ([\matrix-org#16265](matrix-org#16265))
- Enable users to easily unsubscribe to notifications emails via the `List-Unsubscribe` header. ([\matrix-org#16274](matrix-org#16274))
- Report whether a user is `locked` in the [List Accounts admin API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#list-accounts), and exclude locked users by default. ([\matrix-org#16328](matrix-org#16328))

- Fix a long-standing bug where multi-device accounts could cause high load due to presence. ([\matrix-org#16066](matrix-org#16066), [\matrix-org#16170](matrix-org#16170), [\matrix-org#16171](matrix-org#16171), [\matrix-org#16172](matrix-org#16172), [\matrix-org#16174](matrix-org#16174))
- Fix a long-standing bug where appservices using [MSC2409](matrix-org/matrix-spec-proposals#2409) to receive `to_device` messages would only get messages for one user. ([\matrix-org#16251](matrix-org#16251))
- Fix bug when using workers where Synapse could end up re-requesting the same remote device repeatedly. ([\matrix-org#16252](matrix-org#16252))
- Fix long-standing bug where we kept re-requesting a remote server's key repeatedly, potentially causing delays in receiving events over federation. ([\matrix-org#16257](matrix-org#16257))
- Avoid temporary storage of sensitive information. ([\matrix-org#16272](matrix-org#16272))
- Fix bug introduced in Synapse 1.49.0 when using dehydrated devices ([MSC2697](matrix-org/matrix-spec-proposals#2697)) and refresh tokens. Contributed by Hanadi. ([\matrix-org#16288](matrix-org#16288))
- Fix a long-standing bug where invalid receipts would be accepted. ([\matrix-org#16327](matrix-org#16327))
- Use standard name for UTF-8 charset in emails. ([\matrix-org#16329](matrix-org#16329))
- Don't try refetching device lists for users on remote hosts that are marked as "down". ([\matrix-org#16298](matrix-org#16298))

- Fix typos in the documentation. ([\matrix-org#16282](matrix-org#16282))
- Link to the Alpine Linux community package for Synapse. ([\matrix-org#16304](matrix-org#16304))
- Use string for `federation_client_minimum_tls_version` documentation examples. Contributed by @jcgruenhage. ([\matrix-org#16353](matrix-org#16353))

- Allow modules to delete rooms. ([\matrix-org#15997](matrix-org#15997))
- Add GCC and GNU Make to the Nix flake development environment so that `ruff` can be compiled. ([\matrix-org#16090](matrix-org#16090), [\matrix-org#16263](matrix-org#16263))
- Fix type checking when using the new version of Twisted. ([\matrix-org#16235](matrix-org#16235))
- Delete device messages asynchronously and in staged batches using the task scheduler. ([\matrix-org#16240](matrix-org#16240), [\matrix-org#16311](matrix-org#16311), [\matrix-org#16312](matrix-org#16312), [\matrix-org#16313](matrix-org#16313))
- Bump minimum supported Rust version to 1.61.0. ([\matrix-org#16248](matrix-org#16248))
- Update rust to version 1.71.1 in the nix development environment. ([\matrix-org#16260](matrix-org#16260))
- Simplify server key storage. ([\matrix-org#16261](matrix-org#16261))
- Reduce CPU overhead of change password endpoint. ([\matrix-org#16264](matrix-org#16264))
- Stop purging from tables slated for removal. ([\matrix-org#16273](matrix-org#16273))
- Improve type hints. ([\matrix-org#16276](matrix-org#16276), [\matrix-org#16301](matrix-org#16301), [\matrix-org#16325](matrix-org#16325), [\matrix-org#16326](matrix-org#16326))
- Raise `setuptools_rust` version cap to 1.7.0. ([\matrix-org#16277](matrix-org#16277))
- Fix using the new task scheduler causing lots of CPU to be used. ([\matrix-org#16278](matrix-org#16278))
- Upgrade CI run of Python 3.12 from rc1 to rc2. ([\matrix-org#16280](matrix-org#16280))
- Include values in SQL debug when using `execute_values` with Postgres. ([\matrix-org#16281](matrix-org#16281))
- Enable additional linting checks. ([\matrix-org#16283](matrix-org#16283))
- Refactor `receipts_graph` Postgres transactions to stop error messages. ([\matrix-org#16299](matrix-org#16299))
- Small improvements to logging in replication code. ([\matrix-org#16309](matrix-org#16309))
- Remove a reference cycle in background processes. ([\matrix-org#16314](matrix-org#16314))
- Only use literal strings for background process names. ([\matrix-org#16315](matrix-org#16315))
- Refactor `get_user_by_id`. ([\matrix-org#16316](matrix-org#16316))
- Speed up task to delete to-device messages. ([\matrix-org#16318](matrix-org#16318))
- Avoid patching code in tests. ([\matrix-org#16349](matrix-org#16349))
- Test against PostgreSQL 16. ([\matrix-org#16351](matrix-org#16351))

* Bump mypy from 1.4.1 to 1.5.1. ([\matrix-org#16300](matrix-org#16300))
* Bump black from 23.7.0 to 23.9.1. ([\matrix-org#16295](matrix-org#16295))
* Bump docker/build-push-action from 4 to 5. ([\matrix-org#16336](matrix-org#16336))
* Bump docker/login-action from 2 to 3. ([\matrix-org#16339](matrix-org#16339))
* Bump docker/metadata-action from 4 to 5. ([\matrix-org#16337](matrix-org#16337))
* Bump docker/setup-qemu-action from 2 to 3. ([\matrix-org#16338](matrix-org#16338))
* Bump furo from 2023.8.19 to 2023.9.10. ([\matrix-org#16340](matrix-org#16340))
* Bump gitpython from 3.1.32 to 3.1.35. ([\matrix-org#16267](matrix-org#16267), [\matrix-org#16279](matrix-org#16279))
* Bump mypy-zope from 1.0.0 to 1.0.1. ([\matrix-org#16291](matrix-org#16291))
* Bump pillow from 10.0.0 to 10.0.1. ([\matrix-org#16344](matrix-org#16344))
* Bump regex from 1.9.4 to 1.9.5. ([\matrix-org#16233](matrix-org#16233))
* Bump ruff from 0.0.286 to 0.0.290. ([\matrix-org#16342](matrix-org#16342))
* Bump serde_json from 1.0.105 to 1.0.107. ([\matrix-org#16296](matrix-org#16296), [\matrix-org#16345](matrix-org#16345))
* Bump twisted from 22.10.0 to 23.8.0. ([\matrix-org#16235](matrix-org#16235))
* Bump types-pillow from 10.0.0.2 to 10.0.0.3. ([\matrix-org#16293](matrix-org#16293))
* Bump types-setuptools from 68.0.0.3 to 68.2.0.0. ([\matrix-org#16292](matrix-org#16292))
* Bump typing-extensions from 4.7.1 to 4.8.0. ([\matrix-org#16341](matrix-org#16341))
Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request Oct 27, 2023
@Fizzadar
Merge tag 'v1.93.0' into merge-1.93
fef9b3a 
No significant changes since 1.93.0rc1.

The following issues are fixed in 1.93.0 (and RCs).

- [GHSA-4f74-84v3-j9q5](GHSA-4f74-84v3-j9q5) / [CVE-2023-41335](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-41335) — Low Severity

  Temporary storage of plaintext passwords during password changes.

- [GHSA-7565-cq32-vx2x](GHSA-7565-cq32-vx2x) / [CVE-2023-42453](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-42453) — Low Severity

  Improper validation of receipts allows forged read receipts.

See the advisories for more details. If you have any questions, email [email protected].

- Add automatic purge after all users have forgotten a room. ([\matrix-org#15488](matrix-org#15488))
- Restore room purge/shutdown after a Synapse restart. ([\matrix-org#15488](matrix-org#15488))
- Support resolving homeservers using `matrix-fed` DNS SRV records from [MSC4040](matrix-org/matrix-spec-proposals#4040). ([\matrix-org#16137](matrix-org#16137))
- Add the ability to use `G` (GiB) and `T` (TiB) suffixes in configuration options that refer to numbers of bytes. ([\matrix-org#16219](matrix-org#16219))
- Add span information to requests sent to appservices. Contributed by MTRNord. ([\matrix-org#16227](matrix-org#16227))
- Add the ability to enable/disable registrations when using CAS. Contributed by Aurélien Grimpard. ([\matrix-org#16262](matrix-org#16262))
- Allow the `/notifications` endpoint to be routed to workers. ([\matrix-org#16265](matrix-org#16265))
- Enable users to easily unsubscribe to notifications emails via the `List-Unsubscribe` header. ([\matrix-org#16274](matrix-org#16274))
- Report whether a user is `locked` in the [List Accounts admin API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#list-accounts), and exclude locked users by default. ([\matrix-org#16328](matrix-org#16328))

- Fix a long-standing bug where multi-device accounts could cause high load due to presence. ([\matrix-org#16066](matrix-org#16066), [\matrix-org#16170](matrix-org#16170), [\matrix-org#16171](matrix-org#16171), [\matrix-org#16172](matrix-org#16172), [\matrix-org#16174](matrix-org#16174))
- Fix a long-standing bug where appservices using [MSC2409](matrix-org/matrix-spec-proposals#2409) to receive `to_device` messages would only get messages for one user. ([\matrix-org#16251](matrix-org#16251))
- Fix bug when using workers where Synapse could end up re-requesting the same remote device repeatedly. ([\matrix-org#16252](matrix-org#16252))
- Fix long-standing bug where we kept re-requesting a remote server's key repeatedly, potentially causing delays in receiving events over federation. ([\matrix-org#16257](matrix-org#16257))
- Avoid temporary storage of sensitive information. ([\matrix-org#16272](matrix-org#16272))
- Fix bug introduced in Synapse 1.49.0 when using dehydrated devices ([MSC2697](matrix-org/matrix-spec-proposals#2697)) and refresh tokens. Contributed by Hanadi. ([\matrix-org#16288](matrix-org#16288))
- Fix a long-standing bug where invalid receipts would be accepted. ([\matrix-org#16327](matrix-org#16327))
- Use standard name for UTF-8 charset in emails. ([\matrix-org#16329](matrix-org#16329))
- Don't try refetching device lists for users on remote hosts that are marked as "down". ([\matrix-org#16298](matrix-org#16298))

- Fix typos in the documentation. ([\matrix-org#16282](matrix-org#16282))
- Link to the Alpine Linux community package for Synapse. ([\matrix-org#16304](matrix-org#16304))
- Use string for `federation_client_minimum_tls_version` documentation examples. Contributed by @jcgruenhage. ([\matrix-org#16353](matrix-org#16353))

- Allow modules to delete rooms. ([\matrix-org#15997](matrix-org#15997))
- Add GCC and GNU Make to the Nix flake development environment so that `ruff` can be compiled. ([\matrix-org#16090](matrix-org#16090), [\matrix-org#16263](matrix-org#16263))
- Fix type checking when using the new version of Twisted. ([\matrix-org#16235](matrix-org#16235))
- Delete device messages asynchronously and in staged batches using the task scheduler. ([\matrix-org#16240](matrix-org#16240), [\matrix-org#16311](matrix-org#16311), [\matrix-org#16312](matrix-org#16312), [\matrix-org#16313](matrix-org#16313))
- Bump minimum supported Rust version to 1.61.0. ([\matrix-org#16248](matrix-org#16248))
- Update rust to version 1.71.1 in the nix development environment. ([\matrix-org#16260](matrix-org#16260))
- Simplify server key storage. ([\matrix-org#16261](matrix-org#16261))
- Reduce CPU overhead of change password endpoint. ([\matrix-org#16264](matrix-org#16264))
- Stop purging from tables slated for removal. ([\matrix-org#16273](matrix-org#16273))
- Improve type hints. ([\matrix-org#16276](matrix-org#16276), [\matrix-org#16301](matrix-org#16301), [\matrix-org#16325](matrix-org#16325), [\matrix-org#16326](matrix-org#16326))
- Raise `setuptools_rust` version cap to 1.7.0. ([\matrix-org#16277](matrix-org#16277))
- Fix using the new task scheduler causing lots of CPU to be used. ([\matrix-org#16278](matrix-org#16278))
- Upgrade CI run of Python 3.12 from rc1 to rc2. ([\matrix-org#16280](matrix-org#16280))
- Include values in SQL debug when using `execute_values` with Postgres. ([\matrix-org#16281](matrix-org#16281))
- Enable additional linting checks. ([\matrix-org#16283](matrix-org#16283))
- Refactor `receipts_graph` Postgres transactions to stop error messages. ([\matrix-org#16299](matrix-org#16299))
- Small improvements to logging in replication code. ([\matrix-org#16309](matrix-org#16309))
- Remove a reference cycle in background processes. ([\matrix-org#16314](matrix-org#16314))
- Only use literal strings for background process names. ([\matrix-org#16315](matrix-org#16315))
- Refactor `get_user_by_id`. ([\matrix-org#16316](matrix-org#16316))
- Speed up task to delete to-device messages. ([\matrix-org#16318](matrix-org#16318))
- Avoid patching code in tests. ([\matrix-org#16349](matrix-org#16349))
- Test against PostgreSQL 16. ([\matrix-org#16351](matrix-org#16351))

* Bump mypy from 1.4.1 to 1.5.1. ([\matrix-org#16300](matrix-org#16300))
* Bump black from 23.7.0 to 23.9.1. ([\matrix-org#16295](matrix-org#16295))
* Bump docker/build-push-action from 4 to 5. ([\matrix-org#16336](matrix-org#16336))
* Bump docker/login-action from 2 to 3. ([\matrix-org#16339](matrix-org#16339))
* Bump docker/metadata-action from 4 to 5. ([\matrix-org#16337](matrix-org#16337))
* Bump docker/setup-qemu-action from 2 to 3. ([\matrix-org#16338](matrix-org#16338))
* Bump furo from 2023.8.19 to 2023.9.10. ([\matrix-org#16340](matrix-org#16340))
* Bump gitpython from 3.1.32 to 3.1.35. ([\matrix-org#16267](matrix-org#16267), [\matrix-org#16279](matrix-org#16279))
* Bump mypy-zope from 1.0.0 to 1.0.1. ([\matrix-org#16291](matrix-org#16291))
* Bump pillow from 10.0.0 to 10.0.1. ([\matrix-org#16344](matrix-org#16344))
* Bump regex from 1.9.4 to 1.9.5. ([\matrix-org#16233](matrix-org#16233))
* Bump ruff from 0.0.286 to 0.0.290. ([\matrix-org#16342](matrix-org#16342))
* Bump serde_json from 1.0.105 to 1.0.107. ([\matrix-org#16296](matrix-org#16296), [\matrix-org#16345](matrix-org#16345))
* Bump twisted from 22.10.0 to 23.8.0. ([\matrix-org#16235](matrix-org#16235))
* Bump types-pillow from 10.0.0.2 to 10.0.0.3. ([\matrix-org#16293](matrix-org#16293))
* Bump types-setuptools from 68.0.0.3 to 68.2.0.0. ([\matrix-org#16292](matrix-org#16292))
* Bump typing-extensions from 4.7.1 to 4.8.0. ([\matrix-org#16341](matrix-org#16341))

# -----BEGIN PGP SIGNATURE-----
#
# iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmUS8iEQHGVyaWtAbWF0
# cml4Lm9yZwAKCRClQuTtGw+sCXFgB/912+T+BydS290UECCXp9kpRB5xo3aWe8mX
# NCx9Oor1TRLBpLhlQWk786gP1Q9JAQpmA4z6kovjKaLG1b4oLbZNjbPG4hEYc8ow
# /rVzGor52pfyS7uS5GW+rRmapcw4AYND6hA9XGELupf2joC8LXioSCEVG4cxwD8E
# IgIbLc87C7KpaUkNbDEz3jzZ3/BVRGcIYyhF3zTK2ZApvH2qsegq8wKYx4EYJnfh
# 87DXtTCNwA+bW6XZYPtUwPKjZ+TGB11IizxmQySGLbAxvH+GUan8X8TizGyxaqaA
# FDk3yMBbUo0R7ljDgL5YsZXT6qsZz+IBz/bsMzSbZ39f/yEUqHak
# =1/pL
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue Sep 26 16:00:49 2023 BST
# gpg:                using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09
# gpg:                issuer "[email protected]"
# gpg: Can't check signature: No public key

# Conflicts:
#	.github/workflows/docker.yml
#	.github/workflows/push_complement_image.yml
#	.github/workflows/release-artifacts.yml
#	.github/workflows/tests.yml
#	poetry.lock
#	synapse/appservice/scheduler.py
#	synapse/handlers/pagination.py
#	synapse/handlers/room.py
#	synapse/rest/client/account_data.py
#	tests/rest/client/test_receipts.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erikjohnston @clokep