Back out ill-advised notary server hackery (#6657)

* commit '573fee759':
  Back out ill-advised notary server hackery (#6657)

changelog.d/6657.bugfix

@@ -0,0 +1 @@
+Fix incorrect signing of responses from the key server implementation.

synapse/rest/key/v2/remote_key_resource.py

@@ -15,7 +15,6 @@
 import logging
 
 from canonicaljson import encode_canonical_json, json
-from signedjson.key import encode_verify_key_base64
 from signedjson.sign import sign_json
 
 from twisted.internet import defer
@@ -217,28 +216,15 @@ class RemoteKey(DirectServeResource):
         if cache_misses and query_remote_on_cache_miss:
             yield self.fetcher.get_keys(cache_misses)
             yield self.query_keys(request, query, query_remote_on_cache_miss=False)
-            return
-
-        signed_keys = []
-        for key_json in json_results:
-            key_json = json.loads(key_json)
-
-            # backwards-compatibility hack for #6596: if the requested key belongs
-            # to us, make sure that all of the signing keys appear in the
-            # "verify_keys" section.
-            if key_json["server_name"] == self.config.server_name:
-                verify_keys = key_json["verify_keys"]
+        else:
+            signed_keys = []
+            for key_json in json_results:
+                key_json = json.loads(key_json)
                 for signing_key in self.config.key_server_signing_keys:
-                    key_id = "%s:%s" % (signing_key.alg, signing_key.version)
-                    verify_keys[key_id] = {
-                        "key": encode_verify_key_base64(signing_key.verify_key)
-                    }
+                    key_json = sign_json(key_json, self.config.server_name, signing_key)
 
-            for signing_key in self.config.key_server_signing_keys:
-                key_json = sign_json(key_json, self.config.server_name, signing_key)
+                signed_keys.append(key_json)
 
-            signed_keys.append(key_json)
+            results = {"server_keys": signed_keys}
 
-        results = {"server_keys": signed_keys}
-
-        respond_with_json_bytes(request, 200, encode_canonical_json(results))
+            respond_with_json_bytes(request, 200, encode_canonical_json(results))

tests/rest/key/v2/test_remote_key_resource.py

@@ -13,25 +13,30 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import urllib.parse
-from io import BytesIO
+from io import BytesIO, StringIO
 
 from mock import Mock
 
 import signedjson.key
+from canonicaljson import encode_canonical_json
 from nacl.signing import SigningKey
 from signedjson.sign import sign_json
 
 from twisted.web.resource import NoResource
 
+from synapse.crypto.keyring import PerspectivesKeyFetcher
 from synapse.http.site import SynapseRequest
 from synapse.rest.key.v2 import KeyApiV2Resource
+from synapse.storage.keys import FetchKeyResult
 from synapse.util.httpresourcetree import create_resource_tree
+from synapse.util.stringutils import random_string
 
 from tests import unittest
 from tests.server import FakeChannel, wait_until_result
+from tests.utils import default_config
 
 
-class RemoteKeyResourceTestCase(unittest.HomeserverTestCase):
+class BaseRemoteKeyResourceTestCase(unittest.HomeserverTestCase):
     def make_homeserver(self, reactor, clock):
         self.http_client = Mock()
         return self.setup_test_homeserver(http_client=self.http_client)
@@ -73,6 +78,8 @@ class RemoteKeyResourceTestCase(unittest.HomeserverTestCase):
 
         self.http_client.get_json.side_effect = get_json
 
+
+class RemoteKeyResourceTestCase(BaseRemoteKeyResourceTestCase):
     def make_notary_request(self, server_name: str, key_id: str) -> dict:
         """Send a GET request to the test server requesting the given key.
 
@@ -125,6 +132,126 @@ class RemoteKeyResourceTestCase(unittest.HomeserverTestCase):
         oursigs = sigs[self.hs.hostname]
         self.assertEqual(len(oursigs), 2)
 
-        # and both keys should be present in the verify_keys section
+        # the requested key should be present in the verify_keys section
         self.assertIn("ed25519:ver1", keys[0]["verify_keys"])
-        self.assertIn("ed25519:a_lPym", keys[0]["verify_keys"])
+
+
+class EndToEndPerspectivesTests(BaseRemoteKeyResourceTestCase):
+    """End-to-end tests of the perspectives fetch case
+
+    The idea here is to actually wire up a PerspectivesKeyFetcher to the notary
+    endpoint, to check that the two implementations are compatible.
+    """
+
+    def default_config(self, *args, **kwargs):
+        config = super().default_config(*args, **kwargs)
+
+        # replace the signing key with our own
+        self.hs_signing_key = signedjson.key.generate_signing_key("kssk")
+        strm = StringIO()
+        signedjson.key.write_signing_keys(strm, [self.hs_signing_key])
+        config["signing_key"] = strm.getvalue()
+
+        return config
+
+    def prepare(self, reactor, clock, homeserver):
+        # make a second homeserver, configured to use the first one as a key notary
+        self.http_client2 = Mock()
+        config = default_config(name="keyclient")
+        config["trusted_key_servers"] = [
+            {
+                "server_name": self.hs.hostname,
+                "verify_keys": {
+                    "ed25519:%s"
+                    % (
+                        self.hs_signing_key.version,
+                    ): signedjson.key.encode_verify_key_base64(
+                        self.hs_signing_key.verify_key
+                    )
+                },
+            }
+        ]
+        self.hs2 = self.setup_test_homeserver(
+            http_client=self.http_client2, config=config
+        )
+
+        # wire up outbound POST /key/v2/query requests from hs2 so that they
+        # will be forwarded to hs1
+        def post_json(destination, path, data):
+            self.assertEqual(destination, self.hs.hostname)
+            self.assertEqual(
+                path, "/_matrix/key/v2/query",
+            )
+
+            channel = FakeChannel(self.site, self.reactor)
+            req = SynapseRequest(channel)
+            req.content = BytesIO(encode_canonical_json(data))
+
+            req.requestReceived(
+                b"POST", path.encode("utf-8"), b"1.1",
+            )
+            wait_until_result(self.reactor, req)
+            self.assertEqual(channel.code, 200)
+            resp = channel.json_body
+            return resp
+
+        self.http_client2.post_json.side_effect = post_json
+
+    def test_get_key(self):
+        """Fetch a key belonging to a random server"""
+        # make up a key to be fetched.
+        testkey = signedjson.key.generate_signing_key("abc")
+
+        # we expect hs1 to make a regular key request to the target server
+        self.expect_outgoing_key_request("targetserver", testkey)
+        keyid = "ed25519:%s" % (testkey.version,)
+
+        fetcher = PerspectivesKeyFetcher(self.hs2)
+        d = fetcher.get_keys({"targetserver": {keyid: 1000}})
+        res = self.get_success(d)
+        self.assertIn("targetserver", res)
+        keyres = res["targetserver"][keyid]
+        assert isinstance(keyres, FetchKeyResult)
+        self.assertEqual(
+            signedjson.key.encode_verify_key_base64(keyres.verify_key),
+            signedjson.key.encode_verify_key_base64(testkey.verify_key),
+        )
+
+    def test_get_notary_key(self):
+        """Fetch a key belonging to the notary server"""
+        # make up a key to be fetched. We randomise the keyid to try to get it to
+        # appear before the key server signing key sometimes (otherwise we bail out
+        # before fetching its signature)
+        testkey = signedjson.key.generate_signing_key(random_string(5))
+
+        # we expect hs1 to make a regular key request to itself
+        self.expect_outgoing_key_request(self.hs.hostname, testkey)
+        keyid = "ed25519:%s" % (testkey.version,)
+
+        fetcher = PerspectivesKeyFetcher(self.hs2)
+        d = fetcher.get_keys({self.hs.hostname: {keyid: 1000}})
+        res = self.get_success(d)
+        self.assertIn(self.hs.hostname, res)
+        keyres = res[self.hs.hostname][keyid]
+        assert isinstance(keyres, FetchKeyResult)
+        self.assertEqual(
+            signedjson.key.encode_verify_key_base64(keyres.verify_key),
+            signedjson.key.encode_verify_key_base64(testkey.verify_key),
+        )
+
+    def test_get_notary_keyserver_key(self):
+        """Fetch the notary's keyserver key"""
+        # we expect hs1 to make a regular key request to itself
+        self.expect_outgoing_key_request(self.hs.hostname, self.hs_signing_key)
+        keyid = "ed25519:%s" % (self.hs_signing_key.version,)
+
+        fetcher = PerspectivesKeyFetcher(self.hs2)
+        d = fetcher.get_keys({self.hs.hostname: {keyid: 1000}})
+        res = self.get_success(d)
+        self.assertIn(self.hs.hostname, res)
+        keyres = res[self.hs.hostname][keyid]
+        assert isinstance(keyres, FetchKeyResult)
+        self.assertEqual(
+            signedjson.key.encode_verify_key_base64(keyres.verify_key),
+            signedjson.key.encode_verify_key_base64(self.hs_signing_key.verify_key),
+        )