Repository: matrix-org/synapse

Issue:
sometimes the key notary endpoint doesn't give back the keys you asked for
Example: https://matrix.org/_matrix/key/v2/query/sw1v.org/ed25519:a_BPNW: it returns `sw1v.org/ed25519:auto`, and not the key I want.

Discussion:

Patch:
diff --git a/changelog.d/5276.feature b/changelog.d/5276.feature
deleted file mode 100644
index 403dee086..000000000
--- a/changelog.d/5276.feature
+++ /dev/null
@@ -1,1 +0,0 @@
-Allow configuring a range for the account validity startup job.
diff --git a/changelog.d/5307.bugfix b/changelog.d/5307.bugfix
new file mode 100644
index 000000000..6b152f485
--- /dev/null
+++ b/changelog.d/5307.bugfix
@@ -0,0 +1,1 @@
+Fix bug where a notary server would sometimes forget old keys.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 493ea9ee9..edfde05a2 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -763,9 +763,7 @@ uploads_path: "DATADIR/uploads"
 # This means that, if a validity period is set, and Synapse is restarted (it will
 # then derive an expiration date from the current validity period), and some time
 # after that the validity period changes and Synapse is restarted, the users'
-# expiration dates won't be updated unless their account is manually renewed. This
-# date will be randomly selected within a range [now + period - d ; now + period],
-# where d is equal to 10% of the validity period.
+# expiration dates won't be updated unless their account is manually renewed.
 #
 #account_validity:
 #  enabled: True
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index aad340081..693288f93 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -39,8 +39,6 @@ class AccountValidityConfig(Config):
             else:
                 self.renew_email_subject = "Renew your %(app)s account"
 
-            self.startup_job_max_delta = self.period * 10. / 100.
-
         if self.renew_by_email_enabled and "public_baseurl" not in synapse_config:
             raise ConfigError("Can't send renewal emails without 'public_baseurl'")
 
@@ -131,9 +129,7 @@ class RegistrationConfig(Config):
         # This means that, if a validity period is set, and Synapse is restarted (it will
         # then derive an expiration date from the current validity period), and some time
         # after that the validity period changes and Synapse is restarted, the users'
-        # expiration dates won't be updated unless their account is manually renewed. This
-        # date will be randomly selected within a range [now + period - d ; now + period],
-        # where d is equal to 10%% of the validity period.
+        # expiration dates won't be updated unless their account is manually renewed.
         #
         #account_validity:
         #  enabled: True
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index 5756478ad..8f47469a1 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -394,7 +394,7 @@ class BaseV2KeyFetcher(object):
 
     @defer.inlineCallbacks
     def process_v2_response(
-        self, from_server, response_json, time_added_ms, requested_ids=[]
+        self, from_server, response_json, time_added_ms
     ):
         """Parse a 'Server Keys' structure from the result of a /key request
 
@@ -417,10 +417,6 @@ class BaseV2KeyFetcher(object):
 
             time_added_ms (int): the timestamp to record in server_keys_json
 
-            requested_ids (iterable[str]): a list of the key IDs that were requested.
-                We will store the json for these key ids as well as any that are
-                actually in the response
-
         Returns:
             Deferred[dict[str, FetchKeyResult]]: map from key_id to result object
         """
@@ -476,11 +472,6 @@ class BaseV2KeyFetcher(object):
 
         signed_key_json_bytes = encode_canonical_json(signed_key_json)
 
-        # for reasons I don't quite understand, we store this json for the key ids we
-        # requested, as well as those we got.
-        updated_key_ids = set(requested_ids)
-        updated_key_ids.update(verify_keys)
-
         yield logcontext.make_deferred_yieldable(
             defer.gatherResults(
                 [
@@ -493,7 +484,7 @@ class BaseV2KeyFetcher(object):
                         ts_expires_ms=ts_valid_until_ms,
                         key_json_bytes=signed_key_json_bytes,
                     )
-                    for key_id in updated_key_ids
+                    for key_id in verify_keys
                 ],
                 consumeErrors=True,
             ).addErrback(unwrapFirstError)
@@ -749,7 +740,6 @@ class ServerKeyFetcher(BaseV2KeyFetcher):
 
             response_keys = yield self.process_v2_response(
                 from_server=server_name,
-                requested_ids=[requested_key_id],
                 response_json=response,
                 time_added_ms=time_now_ms,
             )
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 52891bb9e..3fe827cd4 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -16,7 +16,6 @@
 # limitations under the License.
 import itertools
 import logging
-import random
 import sys
 import threading
 import time
@@ -248,8 +247,6 @@ class SQLBaseStore(object):
                 self._check_safe_to_upsert,
             )
 
-        self.rand = random.SystemRandom()
-
         if self._account_validity.enabled:
             self._clock.call_later(
                 0.0,
@@ -311,36 +308,21 @@ class SQLBaseStore(object):
             res = self.cursor_to_dict(txn)
             if res:
                 for user in res:
-                    self.set_expiration_date_for_user_txn(
-                        txn,
-                        user["name"],
-                        use_delta=True,
-                    )
+                    self.set_expiration_date_for_user_txn(txn, user["name"])
 
         yield self.runInteraction(
             "get_users_with_no_expiration_date",
             select_users_with_no_expiration_date_txn,
         )
 
-    def set_expiration_date_for_user_txn(self, txn, user_id, use_delta=False):
+    def set_expiration_date_for_user_txn(self, txn, user_id):
         """Sets an expiration date to the account with the given user ID.
 
         Args:
              user_id (str): User ID to set an expiration date for.
-             use_delta (bool): If set to False, the expiration date for the user will be
-                now + validity period. If set to True, this expiration date will be a
-                random value in the [now + period - d ; now + period] range, d being a
-                delta equal to 10% of the validity period.
         """
         now_ms = self._clock.time_msec()
         expiration_ts = now_ms + self._account_validity.period
-
-        if use_delta:
-            expiration_ts = self.rand.randrange(
-                expiration_ts - self._account_validity.startup_job_max_delta,
-                expiration_ts,
-            )
-
         self._simple_insert_txn(
             txn,
             "account_validity",


Test patch:
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 0cb6a363d..d4a1d4d50 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -436,7 +436,6 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
 
     def make_homeserver(self, reactor, clock):
         self.validity_period = 10
-        self.max_delta = self.validity_period * 10. / 100.
 
         config = self.default_config()
 
@@ -454,18 +453,14 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
 
     def test_background_job(self):
         """
-        Tests the same thing as test_background_job, except that it sets the
-        startup_job_max_delta parameter and checks that the expiration date is within the
-        allowed range.
+        Tests whether the account validity startup background job does the right thing,
+        which is sticking an expiration date to every account that doesn't already have
+        one.
         """
-        user_id = self.register_user("kermit_delta", "user")
-
-        self.hs.config.account_validity.startup_job_max_delta = self.max_delta
+        user_id = self.register_user("kermit", "user")
 
         now_ms = self.hs.clock.time_msec()
         self.get_success(self.store._set_expiration_date_when_missing())
 
         res = self.get_success(self.store.get_expiration_ts_for_user(user_id))
-
-        self.assertGreaterEqual(res, now_ms + self.validity_period - self.max_delta)
-        self.assertLessEqual(res, now_ms + self.validity_period)
+        self.assertEqual(res, now_ms + self.validity_period)