Repository: matrix-org/synapse

Issue:
crypto.keyring: TypeError '<' not supported between instances of 'NoneType' and 'int'
### Description

From the logs:

```
2019-06-07_11:31:30.41626     return g.throw(self.type, self.value, self.tb)
2019-06-07_11:31:30.41626   File "/var/local/matrix/venv/lib/python3.7/site-packages/synapse/crypto/keyring.py", line 350, in do_iterations
2019-06-07_11:31:30.41626     yield self._attempt_key_fetches_with_fetcher(f, remaining_requests)
2019-06-07_11:31:30.41627   File "/var/local/matrix/venv/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
2019-06-07_11:31:30.41627     result = g.send(result)
2019-06-07_11:31:30.41627   File "/var/local/matrix/venv/lib/python3.7/site-packages/synapse/crypto/keyring.py", line 425, in _attempt_key_fetches_with_fetcher
2019-06-07_11:31:30.41627     < verify_request.minimum_valid_until_ts
2019-06-07_11:31:30.41628 TypeError: '<' not supported between instances of 'NoneType' and 'int'
```


### Steps to reproduce

- Start matrix 1.0.0rc1
- Check the logs
- Sometimes this will show up

### Version information

- **Version**: 1.0.0rc1

- **Install method**: pip

### Patch

It looks like issue was introduced here:
https://github.com/matrix-org/synapse/blob/8dc1eb66972a2d2aa98d5c852f667a0854932980/synapse/crypto/keyring.py#L419

Where the expectation is: "either `fetch_key_result = result_keys.get(key_id)` results in a `None` var, or in real values  and it is not being the case, with `fetch_key_result.valid_until_ts` being `None`

Either Fetcher is to be fixed or L419 should check that *both* `fetch_key_result` and `fetch_key_result.valid_until_ts` have a valid value.

FWIW, the second option can be: replacing L419 with
`if not (fetch_key_result and fetch_key_result.valid_until_ts):`

Without knowing the details of Fetcher it's hard for me to know which way a fix should go.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/matrix-org/synapse/pull/5415?src=pr&el=h1) Report
> Merging [#5415](https://codecov.io/gh/matrix-org/synapse/pull/5415?src=pr&el=desc) into [develop](https://codecov.io/gh/matrix-org/synapse/commit/843dd714cb4b26c835f7b9a8da4cd0bf57255587?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

```diff
@@             Coverage Diff             @@
##           develop    #5415      +/-   ##
===========================================
+ Coverage    62.56%   62.56%   +<.01%     
===========================================
  Files          326      326              
  Lines        35649    35651       +2     
  Branches      5848     5849       +1     
===========================================
+ Hits         22304    22306       +2     
  Misses       11793    11793              
  Partials      1552     1552
```


Patch:
diff --git a/.github/FUNDING.yml b/.github/FUNDING.yml
deleted file mode 100644
index 1a57677a0..000000000
--- a/.github/FUNDING.yml
+++ /dev/null
@@ -1,4 +0,0 @@
-# One username per supported platform and one custom link
-patreon: matrixdotorg
-liberapay: matrixdotorg
-custom: https://paypal.me/matrixdotorg
diff --git a/changelog.d/5325.bugfix b/changelog.d/5325.bugfix
deleted file mode 100644
index b9413388f..000000000
--- a/changelog.d/5325.bugfix
+++ /dev/null
@@ -1,1 +0,0 @@
-Fix a bug where running synapse_port_db would cause the account validity feature to fail because it didn't set the type of the email_sent column to boolean.
diff --git a/changelog.d/5363.feature b/changelog.d/5363.feature
deleted file mode 100644
index 803fe3fc3..000000000
--- a/changelog.d/5363.feature
+++ /dev/null
@@ -1,1 +0,0 @@
-Allow expired user to trigger renewal email sending manually.
diff --git a/changelog.d/5382.misc b/changelog.d/5382.misc
deleted file mode 100644
index 060cbba2a..000000000
--- a/changelog.d/5382.misc
+++ /dev/null
@@ -1,1 +0,0 @@
-Add a sponsor button to the repo.
diff --git a/changelog.d/5386.misc b/changelog.d/5386.misc
deleted file mode 100644
index 060cbba2a..000000000
--- a/changelog.d/5386.misc
+++ /dev/null
@@ -1,1 +0,0 @@
-Add a sponsor button to the repo.
diff --git a/changelog.d/5412.feature b/changelog.d/5412.feature
deleted file mode 100644
index ec1503860..000000000
--- a/changelog.d/5412.feature
+++ /dev/null
@@ -1,1 +0,0 @@
-Add --no-daemonize option to run synapse in the foreground, per issue #4130. Contributed by Soham Gumaste.
\ No newline at end of file
diff --git a/changelog.d/5415.bugfix b/changelog.d/5415.bugfix
new file mode 100644
index 000000000..83629e193
--- /dev/null
+++ b/changelog.d/5415.bugfix
@@ -0,0 +1,1 @@
+Fix bug where old keys stored in the database with a null valid until timestamp caused all verification requests for that key to fail.
diff --git a/scripts/synapse_port_db b/scripts/synapse_port_db
index b6ba19c77..41be9c922 100755
--- a/scripts/synapse_port_db
+++ b/scripts/synapse_port_db
@@ -54,7 +54,6 @@ BOOLEAN_COLUMNS = {
     "group_roles": ["is_public"],
     "local_group_membership": ["is_publicised", "is_admin"],
     "e2e_room_keys": ["is_verified"],
-    "account_validity": ["email_sent"],
 }
 
 
diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 79e2808dc..0c6c93a87 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -184,22 +184,11 @@ class Auth(object):
         return event_auth.get_public_keys(invite_event)
 
     @defer.inlineCallbacks
-    def get_user_by_req(
-        self,
-        request,
-        allow_guest=False,
-        rights="access",
-        allow_expired=False,
-    ):
+    def get_user_by_req(self, request, allow_guest=False, rights="access"):
         """ Get a registered user's ID.
 
         Args:
             request - An HTTP request with an access_token query parameter.
-            allow_expired - Whether to allow the request through even if the account is
-                expired. If true, Synapse will still require an access token to be
-                provided but won't check if the account it belongs to has expired. This
-                works thanks to /login delivering access tokens regardless of accounts'
-                expiration.
         Returns:
             defer.Deferred: resolves to a ``synapse.types.Requester`` object
         Raises:
@@ -240,7 +229,7 @@ class Auth(object):
             is_guest = user_info["is_guest"]
 
             # Deny the request if the user account has expired.
-            if self._account_validity.enabled and not allow_expired:
+            if self._account_validity.enabled:
                 user_id = user.to_string()
                 expiration_ts = yield self.store.get_expiration_ts_for_user(user_id)
                 if expiration_ts is not None and self.clock.time_msec() >= expiration_ts:
diff --git a/synapse/rest/client/v2_alpha/account_validity.py b/synapse/rest/client/v2_alpha/account_validity.py
index 63bdc3356..55c4ed566 100644
--- a/synapse/rest/client/v2_alpha/account_validity.py
+++ b/synapse/rest/client/v2_alpha/account_validity.py
@@ -79,7 +79,7 @@ class AccountValiditySendMailServlet(RestServlet):
         if not self.account_validity.renew_by_email_enabled:
             raise AuthError(403, "Account renewal via email is disabled on this server.")
 
-        requester = yield self.auth.get_user_by_req(request, allow_expired=True)
+        requester = yield self.auth.get_user_by_req(request)
         user_id = requester.user.to_string()
         yield self.account_activity_handler.send_renewal_email_to_user(user_id)
 
diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py
index 5300720db..e3655ad8d 100644
--- a/synapse/storage/keys.py
+++ b/synapse/storage/keys.py
@@ -80,6 +80,14 @@ class KeyStore(SQLBaseStore):
 
             for row in txn:
                 server_name, key_id, key_bytes, ts_valid_until_ms = row
+
+                if ts_valid_until_ms is None:
+                    # Old keys may be stored with a ts_valid_until_ms of null,
+                    # in which case we treat this as if it was set to `0`, i.e.
+                    # it won't match key requests that define a minimum
+                    # `ts_valid_until_ms`.
+                    ts_valid_until_ms = 0
+
                 res = FetchKeyResult(
                     verify_key=decode_verify_key_bytes(key_id, bytes(key_bytes)),
                     valid_until_ts=ts_valid_until_ms,
diff --git a/synctl b/synctl
index 30d751236..07a68e6d8 100755
--- a/synctl
+++ b/synctl
@@ -69,14 +69,10 @@ def abort(message, colour=RED, stream=sys.stderr):
     sys.exit(1)
 
 
-def start(configfile, daemonize=True):
+def start(configfile):
     write("Starting ...")
     args = SYNAPSE
-
-    if daemonize:
-        args.extend(["--daemonize", "-c", configfile])
-    else:
-        args.extend(["-c", configfile])
+    args.extend(["--daemonize", "-c", configfile])
 
     try:
         subprocess.check_call(args)
@@ -147,21 +143,12 @@ def main():
         help="start or stop all the workers in the given directory"
         " and the main synapse process",
     )
-    parser.add_argument(
-        "--no-daemonize",
-        action="store_false",
-        help="Run synapse in the foreground for debugging. "
-        "Will work only if the daemonize option is not set in the config."
-    )
 
     options = parser.parse_args()
 
     if options.worker and options.all_processes:
         write('Cannot use "--worker" with "--all-processes"', stream=sys.stderr)
         sys.exit(1)
-    if options.no_daemonize and options.all_processes:
-        write('Cannot use "--no-daemonize" with "--all-processes"', stream=sys.stderr)
-        sys.exit(1)
 
     configfile = options.configfile
 
@@ -289,7 +276,7 @@ def main():
             # Check if synapse is already running
             if os.path.exists(pidfile) and pid_running(int(open(pidfile).read())):
                 abort("synapse.app.homeserver already running")
-            start(configfile, bool(options.no_daemonize))
+            start(configfile)
 
         for worker in workers:
             env = os.environ.copy()


Test patch:
diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py
index 4b1901ce3..5a355f00c 100644
--- a/tests/crypto/test_keyring.py
+++ b/tests/crypto/test_keyring.py
@@ -25,7 +25,11 @@ from twisted.internet import defer
 
 from synapse.api.errors import SynapseError
 from synapse.crypto import keyring
-from synapse.crypto.keyring import PerspectivesKeyFetcher, ServerKeyFetcher
+from synapse.crypto.keyring import (
+    PerspectivesKeyFetcher,
+    ServerKeyFetcher,
+    StoreKeyFetcher,
+)
 from synapse.storage.keys import FetchKeyResult
 from synapse.util import logcontext
 from synapse.util.logcontext import LoggingContext
@@ -219,6 +223,50 @@ class KeyringTestCase(unittest.HomeserverTestCase):
         # self.assertFalse(d.called)
         self.get_success(d)
 
+    def test_verify_json_for_server_with_null_valid_until_ms(self):
+        """Tests that we correctly handle key requests for keys we've stored
+        with a null `ts_valid_until_ms`
+        """
+        mock_fetcher = keyring.KeyFetcher()
+        mock_fetcher.get_keys = Mock(return_value=defer.succeed({}))
+
+        kr = keyring.Keyring(
+            self.hs, key_fetchers=(StoreKeyFetcher(self.hs), mock_fetcher)
+        )
+
+        key1 = signedjson.key.generate_signing_key(1)
+        r = self.hs.datastore.store_server_verify_keys(
+            "server9",
+            time.time() * 1000,
+            [("server9", get_key_id(key1), FetchKeyResult(get_verify_key(key1), None))],
+        )
+        self.get_success(r)
+
+        json1 = {}
+        signedjson.sign.sign_json(json1, "server9", key1)
+
+        # should fail immediately on an unsigned object
+        d = _verify_json_for_server(kr, "server9", {}, 0, "test unsigned")
+        self.failureResultOf(d, SynapseError)
+
+        # should fail on a signed object with a non-zero minimum_valid_until_ms,
+        # as it tries to refetch the keys and fails.
+        d = _verify_json_for_server(
+            kr, "server9", json1, 500, "test signed non-zero min"
+        )
+        self.get_failure(d, SynapseError)
+
+        # We expect the keyring tried to refetch the key once.
+        mock_fetcher.get_keys.assert_called_once_with(
+            {"server9": {get_key_id(key1): 500}}
+        )
+
+        # should succeed on a signed object with a 0 minimum_valid_until_ms
+        d = _verify_json_for_server(
+            kr, "server9", json1, 0, "test signed with zero min"
+        )
+        self.get_success(d)
+
     def test_verify_json_dedupes_key_requests(self):
         """Two requests for the same key should be deduped."""
         key1 = signedjson.key.generate_signing_key(1)
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 1628db501..0cb6a363d 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -427,41 +427,6 @@ class AccountValidityRenewalByEmailTestCase(unittest.HomeserverTestCase):
 
         self.assertEqual(len(self.email_attempts), 1)
 
-    def test_manual_email_send_expired_account(self):
-        user_id = self.register_user("kermit", "monkey")
-        tok = self.login("kermit", "monkey")
-
-        # We need to manually add an email address otherwise the handler will do
-        # nothing.
-        now = self.hs.clock.time_msec()
-        self.get_success(
-            self.store.user_add_threepid(
-                user_id=user_id,
-                medium="email",
-                address="kermit@example.com",
-                validated_at=now,
-                added_at=now,
-            )
-        )
-
-        # Make the account expire.
-        self.reactor.advance(datetime.timedelta(days=8).total_seconds())
-
-        # Ignore all emails sent by the automatic background task and only focus on the
-        # ones sent manually.
-        self.email_attempts = []
-
-        # Test that we're still able to manually trigger a mail to be sent.
-        request, channel = self.make_request(
-            b"POST",
-            "/_matrix/client/unstable/account_validity/send_mail",
-            access_token=tok,
-        )
-        self.render(request)
-        self.assertEquals(channel.result["code"], b"200", channel.result)
-
-        self.assertEqual(len(self.email_attempts), 1)
-
 
 class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):