Enforce validity period on server_keys for fed requests. by richvdh · Pull Request #5321 · matrix-org/synapse

richvdh · 2019-06-03T12:41:36Z

When handling incoming federation requests, make sure that we have an
up-to-date copy of the signing key.

We do not yet enforce the validity period for event signatures.

When handling incoming federation requests, make sure that we have an up-to-date copy of the signing key. We do not yet enforce the validity period for event signatures.

no factory here

codecov · 2019-06-03T12:57:58Z

Codecov Report

Merging #5321 into develop will decrease coverage by 0.64%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##           develop    #5321      +/-   ##
===========================================
- Coverage    62.99%   62.35%   -0.65%     
===========================================
  Files          341      341              
  Lines        35607    35625      +18     
  Branches      5827     5831       +4     
===========================================
- Hits         22432    22214     -218     
- Misses       11605    11811     +206     
- Partials      1570     1600      +30

codecov · 2019-06-03T12:57:58Z

Codecov Report

Merging #5321 into develop will increase coverage by 0.02%.
The diff coverage is 90.32%.

@@             Coverage Diff             @@
##           develop    #5321      +/-   ##
===========================================
+ Coverage    62.99%   63.02%   +0.02%     
===========================================
  Files          341      341              
  Lines        35607    35623      +16     
  Branches      5827     5830       +3     
===========================================
+ Hits         22432    22452      +20     
+ Misses       11605    11603       -2     
+ Partials      1570     1568       -2

erikjohnston

Other than clarification I think this works.

erikjohnston · 2019-06-03T12:58:08Z

+            for key_id in verify_request.key_ids:
+                current_min_ts = keys_for_server.get(key_id, -1)
+                if current_min_ts < verify_request.minimum_valid_until_ts:
+                    keys_for_server[key_id] = verify_request.minimum_valid_until_ts


Can haz comment pls? I'm struggling to follow the logic here. We're taking the maximum minimum_valid_untl_ts? Maybe this can be written as:

for key_id in verify_request.key_ids: current_min_ts = keys_for_server.get(key_id, -1) keys_for_server[key_id] = max(keys_for_server[key_id], current_min_ts)

?

Does the remote server respond with the key with the latest valid_until_ts even if that is less than the requested if it can't find a later one? If not will that cause problems where the key may have been valid for some of the key requests?

Does the remote server respond with the key with the latest valid_until_ts even if that is less than the requested if it can't find a later one? If not will that cause problems where the key may have been valid for some of the key requests?

Hum, apparently a notary server will not respond with such a key. And yes, it probably will. I'll try and get that changed on the notary server impl before landing this.

Can haz comment pls?

done

Maybe this can be written as:

I'm not entirely convinced it's clearer, but have tweaked it anyway.

Hum, apparently a notary server will not respond with such a key.

This is apparently incorrect, as tested by matrix-org/sytest#620.

Proposals: * [MSC2076](#2076) * [MSC2077](#2077) Implementation references: * matrix-org/synapse@00bf99f * matrix-org/synapse#5354 * matrix-org/synapse#5321 No known differences from the proposals are included here - alterations are accidental.

@dnaf

Synapse 1.0.0rc1 (2019-06-07) ============================= Features -------- - Synapse now more efficiently collates room statistics. ([\#4338](#4338), [\#5260](#5260), [\#5324](#5324)) - Add experimental support for relations (aka reactions and edits). ([\#5220](#5220)) - Ability to configure default room version. ([\#5223](#5223), [\#5249](#5249)) - Allow configuring a range for the account validity startup job. ([\#5276](#5276)) - CAS login will now hit the r0 API, not the deprecated v1 one. ([\#5286](#5286)) - Validate federation server TLS certificates by default (implements [MSC1711](https://github.com/matrix-org/matrix-doc/blob/master/proposals/1711-x509-for-federation.md)). ([\#5359](#5359)) - Update /_matrix/client/versions to reference support for r0.5.0. ([\#5360](#5360)) - Add a script to generate new signing-key files. ([\#5361](#5361)) - Update upgrade and installation guides ahead of 1.0. ([\#5371](#5371)) - Replace the `perspectives` configuration section with `trusted_key_servers`, and make validating the signatures on responses optional (since TLS will do this job for us). ([\#5374](#5374)) - Add ability to perform password reset via email without trusting the identity server. ([\#5377](#5377)) - Set default room version to v4. ([\#5379](#5379)) Bugfixes -------- - Fixes client-server API not sending "m.heroes" to lazy-load /sync requests when a rooms name or its canonical alias are empty. Thanks to @dnaf for this work! ([\#5089](#5089)) - Prevent federation device list updates breaking when processing multiple updates at once. ([\#5156](#5156)) - Fix worker registration bug caused by ClientReaderSlavedStore being unable to see get_profileinfo. ([\#5200](#5200)) - Fix race when backfilling in rooms with worker mode. ([\#5221](#5221)) - Fix appservice timestamp massaging. ([\#5233](#5233)) - Ensure that server_keys fetched via a notary server are correctly signed. ([\#5251](#5251)) - Show the correct error when logging out and access token is missing. ([\#5256](#5256)) - Fix error code when there is an invalid parameter on /_matrix/client/r0/publicRooms ([\#5257](#5257)) - Fix error when downloading thumbnail with missing width/height parameter. ([\#5258](#5258)) - Fix schema update for account validity. ([\#5268](#5268)) - Fix bug where we leaked extremities when we soft failed events, leading to performance degradation. ([\#5274](#5274), [\#5278](#5278), [\#5291](#5291)) - Fix "db txn 'update_presence' from sentinel context" log messages. ([\#5275](#5275)) - Fix dropped logcontexts during high outbound traffic. ([\#5277](#5277)) - Fix a bug where it is not possible to get events in the federation format with the request `GET /_matrix/client/r0/rooms/{roomId}/messages`. ([\#5293](#5293)) - Fix performance problems with the rooms stats background update. ([\#5294](#5294)) - Fix noisy 'no key for server' logs. ([\#5300](#5300)) - Fix bug where a notary server would sometimes forget old keys. ([\#5307](#5307)) - Prevent users from setting huge displaynames and avatar URLs. ([\#5309](#5309)) - Fix handling of failures when processing incoming events where calling `/event_auth` on remote server fails. ([\#5317](#5317)) - Ensure that we have an up-to-date copy of the signing key when validating incoming federation requests. ([\#5321](#5321)) - Fix various problems which made the signing-key notary server time out for some requests. ([\#5333](#5333)) - Fix bug which would make certain operations (such as room joins) block for 20 minutes while attemoting to fetch verification keys. ([\#5334](#5334)) - Fix a bug where we could rapidly mark a server as unreachable even though it was only down for a few minutes. ([\#5335](#5335), [\#5340](#5340)) - Fix a bug where account validity renewal emails could only be sent when email notifs were enabled. ([\#5341](#5341)) - Fix failure when fetching batches of events during backfill, etc. ([\#5342](#5342)) - Add a new room version where the timestamps on events are checked against the validity periods on signing keys. ([\#5348](#5348), [\#5354](#5354)) - Fix room stats and presence background updates to correctly handle missing events. ([\#5352](#5352)) - Include left members in room summaries' heroes. ([\#5355](#5355)) - Fix `federation_custom_ca_list` configuration option. ([\#5362](#5362)) - Fix missing logcontext warnings on shutdown. ([\#5369](#5369)) Improved Documentation ---------------------- - Fix docs on resetting the user directory. ([\#5282](#5282)) - Fix notes about ACME in the MSC1711 faq. ([\#5357](#5357)) Internal Changes ---------------- - Synapse will now serve the experimental "room complexity" API endpoint. ([\#5216](#5216)) - The base classes for the v1 and v2_alpha REST APIs have been unified. ([\#5226](#5226), [\#5328](#5328)) - Simplifications and comments in do_auth. ([\#5227](#5227)) - Remove urllib3 pin as requests 2.22.0 has been released supporting urllib3 1.25.2. ([\#5230](#5230)) - Preparatory work for key-validity features. ([\#5232](#5232), [\#5234](#5234), [\#5235](#5235), [\#5236](#5236), [\#5237](#5237), [\#5244](#5244), [\#5250](#5250), [\#5296](#5296), [\#5299](#5299), [\#5343](#5343), [\#5347](#5347), [\#5356](#5356)) - Specify the type of reCAPTCHA key to use. ([\#5283](#5283)) - Improve sample config for monthly active user blocking. ([\#5284](#5284)) - Remove spurious debug from MatrixFederationHttpClient.get_json. ([\#5287](#5287)) - Improve logging for logcontext leaks. ([\#5288](#5288)) - Clarify that the admin change password API logs the user out. ([\#5303](#5303)) - New installs will now use the v54 full schema, rather than the full schema v14 and applying incremental updates to v54. ([\#5320](#5320)) - Improve docstrings on MatrixFederationClient. ([\#5332](#5332)) - Clean up FederationClient.get_events for clarity. ([\#5344](#5344)) - Various improvements to debug logging. ([\#5353](#5353)) - Don't run CI build checks until sample config check has passed. ([\#5370](#5370)) - Automatically retry buildkite builds (max twice) when an agent is lost. ([\#5380](#5380))

Enforce validity period on server_keys for fed requests.

ee5494b

When handling incoming federation requests, make sure that we have an up-to-date copy of the signing key. We do not yet enforce the validity period for event signatures.

richvdh requested a review from a team June 3, 2019 12:41

Fix compat with attr 17.4

40eeda8

no factory here

erikjohnston approved these changes Jun 3, 2019

View reviewed changes

Tweaks and comments

5abc340

richvdh merged commit fec2dcb into develop Jun 3, 2019

turt2live mentioned this pull request Jun 5, 2019

Spec v5 rooms: Key validity matrix-org/matrix-spec-proposals#2080

Merged

richvdh deleted the rav/server_keys/08-enforce-validity-for-incoming-requests branch December 1, 2020 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enforce validity period on server_keys for fed requests.#5321

Enforce validity period on server_keys for fed requests.#5321
richvdh merged 3 commits into
developfrom
rav/server_keys/08-enforce-validity-for-incoming-requests

richvdh commented Jun 3, 2019

Uh oh!

codecov Bot commented Jun 3, 2019

Uh oh!

codecov Bot commented Jun 3, 2019 •

edited

Loading

Uh oh!

erikjohnston left a comment

Uh oh!

erikjohnston Jun 3, 2019

Uh oh!

erikjohnston Jun 3, 2019

Uh oh!

richvdh Jun 3, 2019

Uh oh!

richvdh Jun 3, 2019

Uh oh!

richvdh Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

richvdh commented Jun 3, 2019

Uh oh!

codecov Bot commented Jun 3, 2019

Codecov Report

Uh oh!

codecov Bot commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

erikjohnston Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

erikjohnston Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jun 3, 2019 •

edited

Loading