From f7956428a12cabd3190bf251f576fc82dccc5517 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 3 Nov 2021 19:55:43 +0000 Subject: [PATCH 1/7] Catch federation request failures in `/register` --- sydent/http/servlets/registerservlet.py | 41 +++++++++++++++++-------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/sydent/http/servlets/registerservlet.py b/sydent/http/servlets/registerservlet.py index f87ef2cd..1f96959a 100644 --- a/sydent/http/servlets/registerservlet.py +++ b/sydent/http/servlets/registerservlet.py @@ -14,8 +14,11 @@ import logging import urllib +from http import HTTPStatus from typing import TYPE_CHECKING +from twisted.internet.error import ConnectError, DNSLookupError +from twisted.web.client import ResponseFailed from twisted.web.resource import Resource from twisted.web.server import Request @@ -56,22 +59,34 @@ async def render_POST(self, request: Request) -> JsonDict: "error": "matrix_server_name must be a valid Matrix server name (IP address or hostname)", } - result = await self.client.get_json( - "matrix://%s/_matrix/federation/v1/openid/userinfo?access_token=%s" - % ( - matrix_server, - urllib.parse.quote(args["access_token"]), - ), - 1024 * 5, - ) + try: + result = await self.client.get_json( + "matrix://%s/_matrix/federation/v1/openid/userinfo?access_token=%s" + % ( + matrix_server, + urllib.parse.quote(args["access_token"]), + ), + 1024 * 5, + ) + except (DNSLookupError, ConnectError, ResponseFailed) as e: + logger.warning("Unable to contact %s: %s", matrix_server, e) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) + return { + "errcode": "M_UNKNOWN", + "error": f"Unable to contact the Matrix homeserver ({type(e).__name__})", + } if "sub" not in result: - raise Exception("Invalid response from homeserver") + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) + return { + "errcode": "M_UNKNOWN", + "error": "The Matrix homeserver did not include 'sub' in its response", + } user_id = result["sub"] if not isinstance(user_id, str): - request.setResponseCode(500) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) return { "errcode": "M_UNKNOWN", "error": "The Matrix homeserver returned a malformed reply", @@ -81,7 +96,7 @@ async def render_POST(self, request: Request) -> JsonDict: # Ensure there's a localpart and domain in the returned user ID. if len(user_id_components) != 2: - request.setResponseCode(500) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) return { "errcode": "M_UNKNOWN", "error": "The Matrix homeserver returned an invalid MXID", @@ -90,14 +105,14 @@ async def render_POST(self, request: Request) -> JsonDict: user_id_server = user_id_components[1] if not is_valid_matrix_server_name(user_id_server): - request.setResponseCode(500) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) return { "errcode": "M_UNKNOWN", "error": "The Matrix homeserver returned an invalid MXID", } if user_id_server != matrix_server: - request.setResponseCode(500) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) return { "errcode": "M_UNKNOWN", "error": "The Matrix homeserver returned a MXID belonging to another homeserver", From 4210907da43b974b1e5ed873ab69e4e50ef45222 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 3 Nov 2021 19:56:22 +0000 Subject: [PATCH 2/7] Test cases to check we handle these cases properly --- .github/workflows/pipeline.yml | 2 +- setup.py | 1 + tests/test_register.py | 43 ++++++++++++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index eed54929..5d903262 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -31,7 +31,7 @@ jobs: - uses: actions/setup-python@v2 with: python-version: ${{ matrix.python-version }} - - run: python -m pip install -e . + - run: python -m pip install -e .[dev] - run: trial tests run-matrix-is-tests: diff --git a/setup.py b/setup.py index 590042ac..246e7fb6 100644 --- a/setup.py +++ b/setup.py @@ -56,6 +56,7 @@ def read(fname): ], extras_require={ "dev": [ + "parameterized==0.8.1", "flake8==3.9.2", "flake8-pyi==20.10.0", "black==21.6b0", diff --git a/tests/test_register.py b/tests/test_register.py index 16109050..335c2aa9 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -11,7 +11,12 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from http import HTTPStatus +from unittest.mock import patch +import twisted.internet.error +import twisted.web.client +from parameterized import parameterized from twisted.trial import unittest from tests.utils import make_request, make_sydent @@ -20,11 +25,11 @@ class RegisterTestCase(unittest.TestCase): """Tests Sydent's register servlet""" - def setUp(self): + def setUp(self) -> None: # Create a new sydent self.sydent = make_sydent() - def test_sydent_rejects_invalid_hostname(self): + def test_sydent_rejects_invalid_hostname(self) -> None: """Tests that the /register endpoint rejects an invalid hostname passed as matrix_server_name""" self.sydent.run() @@ -40,3 +45,37 @@ def test_sydent_rejects_invalid_hostname(self): request.render(self.sydent.servlets.registerServlet) self.assertEqual(channel.code, 400) + + @parameterized.expand( + [ + (twisted.internet.error.DNSLookupError(),), + (twisted.internet.error.TimeoutError(),), + (twisted.internet.error.ConnectionRefusedError(),), + # Naughty: strictly we're supposed to initialise a ResponseNeverReceived + # with a list of 1 or more failures. + (twisted.web.client.ResponseNeverReceived([]),), + ] + ) + def test_connection_failure(self, exc: Exception) -> None: + self.sydent.run() + request, channel = make_request( + self.sydent.reactor, + "POST", + "/_matrix/identity/v2/account/register", + content={ + "matrix_server_name": "matrix.alice.com", + "access_token": "back_in_wonderland", + }, + ) + servlet = self.sydent.servlets.registerServlet + + def mock_get_json(*args: object, **kwargs: object) -> None: + raise exc + + with patch.object(servlet.client, "get_json", mock_get_json): + request.render(servlet) + self.assertEqual(channel.code, HTTPStatus.INTERNAL_SERVER_ERROR) + self.assertEqual(channel.json_body["errcode"], "M_UNKNOWN") + # Check that we haven't just returned the generic error message in asyncjsonwrap + self.assertNotEqual(channel.json_body["error"], "Internal Server Error") + self.assertIn("contact", channel.json_body["error"]) From c1c6067bb4e10858fbc3819af3a48f9f0d2b008a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 3 Nov 2021 19:56:51 +0000 Subject: [PATCH 3/7] Stubs --- stubs/twisted/internet/error.pyi | 2 ++ stubs/twisted/web/client.pyi | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/stubs/twisted/internet/error.pyi b/stubs/twisted/internet/error.pyi index 30aa9b9a..145f440f 100644 --- a/stubs/twisted/internet/error.pyi +++ b/stubs/twisted/internet/error.pyi @@ -2,3 +2,5 @@ from typing import Any, Optional class ConnectError(Exception): def __init__(self, osError: Optional[Any] = ..., string: str = ...): ... + +class DNSLookupError(IOError): ... diff --git a/stubs/twisted/web/client.pyi b/stubs/twisted/web/client.pyi index 0c47572f..9c1cce33 100644 --- a/stubs/twisted/web/client.pyi +++ b/stubs/twisted/web/client.pyi @@ -1,8 +1,9 @@ -from typing import BinaryIO, Optional, Type, TypeVar +from typing import BinaryIO, Optional, Sequence, Type, TypeVar from twisted.internet.defer import Deferred from twisted.internet.interfaces import IConsumer, IProtocol from twisted.internet.task import Cooperator +from twisted.python.failure import Failure from twisted.web.http_headers import Headers from twisted.web.iweb import ( IAgent, @@ -15,6 +16,11 @@ from zope.interface import implementer _C = TypeVar("_C") +class ResponseFailed(Exception): + def __init__( + self, reasons: Sequence[Failure], response: Optional[Response] = ... + ): ... + class HTTPConnectionPool: persistent: bool maxPersistentPerHost: int From 5c8b16ee747862e68ce703a65c4b3ecc0d99b087 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 3 Nov 2021 19:59:42 +0000 Subject: [PATCH 4/7] Changelog for good measure --- changelog.d/456.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/456.misc diff --git a/changelog.d/456.misc b/changelog.d/456.misc new file mode 100644 index 00000000..c29e9770 --- /dev/null +++ b/changelog.d/456.misc @@ -0,0 +1 @@ +Handle federation request failures in `/request` explicitly, to reduce Sentry noise. \ No newline at end of file From 8281c4a827153d0199b683af80c3e96f1b26fda9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 5 Nov 2021 14:05:37 +0000 Subject: [PATCH 5/7] Use `side_effect` arg when mocking --- tests/test_register.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/test_register.py b/tests/test_register.py index 335c2aa9..972b638a 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -69,10 +69,7 @@ def test_connection_failure(self, exc: Exception) -> None: ) servlet = self.sydent.servlets.registerServlet - def mock_get_json(*args: object, **kwargs: object) -> None: - raise exc - - with patch.object(servlet.client, "get_json", mock_get_json): + with patch.object(servlet.client, "get_json", side_effect=exc): request.render(servlet) self.assertEqual(channel.code, HTTPStatus.INTERNAL_SERVER_ERROR) self.assertEqual(channel.json_body["errcode"], "M_UNKNOWN") From 0d202e544936573beb2c8b25399c72bd290a6d56 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 5 Nov 2021 14:21:26 +0000 Subject: [PATCH 6/7] Log a warning in all cases where we return 500 No more exceptions. Let's log something so it's not silent. --- sydent/http/servlets/registerservlet.py | 57 +++++++++++-------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/sydent/http/servlets/registerservlet.py b/sydent/http/servlets/registerservlet.py index 1f96959a..072c7e0a 100644 --- a/sydent/http/servlets/registerservlet.py +++ b/sydent/http/servlets/registerservlet.py @@ -59,6 +59,14 @@ async def render_POST(self, request: Request) -> JsonDict: "error": "matrix_server_name must be a valid Matrix server name (IP address or hostname)", } + def federation_request_problem(error: str): + logger.warning(error) + request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) + return { + "errcode": "M_UNKNOWN", + "error": error, + } + try: result = await self.client.get_json( "matrix://%s/_matrix/federation/v1/openid/userinfo?access_token=%s" @@ -69,54 +77,41 @@ async def render_POST(self, request: Request) -> JsonDict: 1024 * 5, ) except (DNSLookupError, ConnectError, ResponseFailed) as e: - logger.warning("Unable to contact %s: %s", matrix_server, e) - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": f"Unable to contact the Matrix homeserver ({type(e).__name__})", - } + return federation_request_problem( + f"Unable to contact the Matrix homeserver ({type(e).__name__})" + ) if "sub" not in result: - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": "The Matrix homeserver did not include 'sub' in its response", - } + return federation_request_problem( + "The Matrix homeserver did not include 'sub' in its response", + ) user_id = result["sub"] if not isinstance(user_id, str): - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": "The Matrix homeserver returned a malformed reply", - } + return federation_request_problem( + "The Matrix homeserver returned a malformed reply" + ) user_id_components = user_id.split(":", 1) # Ensure there's a localpart and domain in the returned user ID. if len(user_id_components) != 2: - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": "The Matrix homeserver returned an invalid MXID", - } + return federation_request_problem( + "The Matrix homeserver returned an invalid MXID" + ) user_id_server = user_id_components[1] if not is_valid_matrix_server_name(user_id_server): - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": "The Matrix homeserver returned an invalid MXID", - } + return federation_request_problem( + "The Matrix homeserver returned an invalid MXID" + ) if user_id_server != matrix_server: - request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) - return { - "errcode": "M_UNKNOWN", - "error": "The Matrix homeserver returned a MXID belonging to another homeserver", - } + return federation_request_problem( + "The Matrix homeserver returned a MXID belonging to another homeserver" + ) tok = issueToken(self.sydent, user_id) From c23fc626c2288cedfadc8c484fe3ec43a34ce5ed Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 5 Nov 2021 15:54:22 +0000 Subject: [PATCH 7/7] Whoops, appease mypy --- sydent/http/servlets/registerservlet.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sydent/http/servlets/registerservlet.py b/sydent/http/servlets/registerservlet.py index 072c7e0a..9a852aa4 100644 --- a/sydent/http/servlets/registerservlet.py +++ b/sydent/http/servlets/registerservlet.py @@ -15,7 +15,7 @@ import logging import urllib from http import HTTPStatus -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Dict from twisted.internet.error import ConnectError, DNSLookupError from twisted.web.client import ResponseFailed @@ -59,7 +59,7 @@ async def render_POST(self, request: Request) -> JsonDict: "error": "matrix_server_name must be a valid Matrix server name (IP address or hostname)", } - def federation_request_problem(error: str): + def federation_request_problem(error: str) -> Dict[str, str]: logger.warning(error) request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR) return {