From ea1703ea42261be501ec09d86fe6bef6d9722e22 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 28 Oct 2026 00:02:01 +0100 Subject: [PATCH] fix(rpc): Keep request signatures out of error reports RpcServer held a request's signature as a plain string, in `headers` and in `authorization`, and RpcServer.post reports every coded error, provider failures included. Sentry's SDK sends every frame's local variables, and Sentry's SDK configuration turns off the scrubber, so each such report carried the signature next to the body it signs. A signature has no timestamp or nonce, so that pair can be replayed. The server now takes Authorization out of the headers on entry, in any case, and holds it as an scm.Secret. verify_get and verify_post take a Secret and still accept a plain string. RpcApiClient.request holds its signature as a Secret and reveals it only in the call to the session. ResourceServiceUnavailable no longer chains the connection error, whose frames in the session and in requests hold the header. A raw requests error still carries it in those frames, which this library can't mask. --- src/scm/rpc/client.py | 20 ++++++-- src/scm/rpc/helpers.py | 13 +++-- src/scm/rpc/server.py | 40 ++++++++++----- tests/unit/error_reports.py | 17 ++++++- tests/unit/test_rpc_client.py | 42 ++++++++++++++++ tests/unit/test_rpc_server.py | 91 ++++++++++++++++++++++++++++++----- 6 files changed, 187 insertions(+), 36 deletions(-) diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index f67b9b5..151bcc2 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -150,6 +150,11 @@ class RpcApiClient(ApiClient): regardless of request origin (i.e. within Sentry or without). Access controls are handling on the Sentry-side with required scoping metadata specified in the request's headers. + + A request's signature, which could be replayed, is held in a ``Secret`` and revealed only in the call to the + session. The session gets it in plain text in its headers, as does requests below it, so a requests error that + reaches the caller carries it in those frames. The ``ResourceServiceUnavailable`` raised instead for a connection + error, with ``retry_connection_errors``, does not. """ def __init__( @@ -201,8 +206,8 @@ def request( ) ) + authorization = Secret(f"rpcsignature {sign_post(self.signing_secret, body)}") request_headers = { - "Authorization": f"rpcsignature {sign_post(self.signing_secret, body)}", "Content-Type": "application/json", "X-Organization-Id": str(self.organization_id), "X-Referrer": self.referrer, @@ -226,7 +231,12 @@ def request( last_reason = "" # trigger of the most recent retry; tags the recovery metric on success while True: try: - response = self.session.post(url=self.full_url, data=body, headers=request_headers, stream=stream) + response = self.session.post( + url=self.full_url, + data=body, + headers={"Authorization": authorization.reveal(), **request_headers}, + stream=stream, + ) except _RETRIABLE_TRANSPORT_ERRORS as exc: # Only intercept connection errors when the caller opted in, and only for reads: a # write that died at the transport layer may already have landed upstream, so it is @@ -244,8 +254,10 @@ def request( if attempt: self._record_retry_outcome("exhausted", method, "connection_error") # Surface a typed, RPC-serializable error instead of an opaque ConnectionError that - # would not survive the proxy boundary — independent of whether we retried. - raise ResourceServiceUnavailable(detail=f"{type(exc).__name__}: {exc}") from exc + # would not survive the proxy boundary — independent of whether we retried. "from None" drops the + # connection error, which is reported with its frames, and those of the session and of requests hold + # the Authorization header. + raise ResourceServiceUnavailable(detail=f"{type(exc).__name__}: {exc}") from None if idempotent and response.status_code in status_codes: if attempt < max_retries: diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index c91793f..5fdeab8 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -24,17 +24,22 @@ def sign(secret: Secret, message: bytes) -> str: def verify_get( - secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: str + secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: Secret | str ) -> bool: return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature) -def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: str) -> bool: +def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: Secret | str) -> bool: return verify(secrets, serialized_body, received_signature) -def verify(secrets: Sequence[Secret], message: bytes, received_signature: str) -> bool: - return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets) +def verify(secrets: Sequence[Secret], message: bytes, received_signature: Secret | str) -> bool: + """Whether ``received_signature`` signs ``message`` with one of ``secrets``. + + A signature with its message can be replayed, so pass it as a ``Secret``. A plain string is still accepted. + """ + received = received_signature if isinstance(received_signature, Secret) else Secret(received_signature) + return any(hmac.compare_digest(received.reveal(), sign(secret, message)) for secret in secrets) def deserialize_repository(content: bytes) -> Repository: diff --git a/src/scm/rpc/server.py b/src/scm/rpc/server.py index 38168ba..efae5bf 100644 --- a/src/scm/rpc/server.py +++ b/src/scm/rpc/server.py @@ -41,9 +41,12 @@ def __init__( def get(self, headers: dict[str, str]): try: - authorization, organization_id, repository_id = self._extract_headers(headers) + # Rebound without the signature, so it shows as [Filtered] in the frames of a reported error rather than in + # ``headers``. + signature, headers = _take_signature(headers) + organization_id, repository_id = self._extract_headers(headers) - if not verify_get(self.secrets, organization_id, repository_id, authorization): + if not verify_get(self.secrets, organization_id, repository_id, signature): raise RpcInvalidGrant() scm = SourceCodeManager.make_client( @@ -73,7 +76,9 @@ def get(self, headers: dict[str, str]): def post(self, data: bytes, headers: dict[str, str]) -> StreamResponse: try: - result = self._post(data, headers) + # Rebound without the signature, which would otherwise be reported next to ``data``, the body it signs. + signature, headers = _take_signature(headers) + result = self._post(data, signature, headers) self.record_count("sentry.scm.rpc.request.post.succeeded", 1, {}) return result except SCMCodedError as e: @@ -90,10 +95,10 @@ def post(self, data: bytes, headers: dict[str, str]) -> StreamResponse: content=iter([error_data]), ) - def _post(self, data: bytes, headers: dict[str, str]) -> StreamResponse: - authorization, organization_id, repository_id = self._extract_headers(headers) + def _post(self, data: bytes, signature: Secret, headers: dict[str, str]) -> StreamResponse: + organization_id, repository_id = self._extract_headers(headers) - if not verify_post(self.secrets, data, authorization): + if not verify_post(self.secrets, data, signature): raise RpcInvalidGrant() if len(data) >= TEN_MEGABYTES: raise RpcRequestTooLarge() @@ -140,12 +145,7 @@ def _post(self, data: bytes, headers: dict[str, str]) -> StreamResponse: content=iter_response(response), ) - def _extract_headers(self, headers: Mapping[str, str]) -> tuple[str, int, RepositoryId]: - try: - authorization = headers["Authorization"].removeprefix("rpcsignature ") - except KeyError as e: - raise RpcMalformedRequestHeaders(detail="Could not find Authorization header") from e - + def _extract_headers(self, headers: Mapping[str, str]) -> tuple[int, RepositoryId]: try: raw = headers["X-Organization-Id"] organization_id = int(raw) @@ -161,7 +161,21 @@ def _extract_headers(self, headers: Mapping[str, str]) -> tuple[str, int, Reposi except msgspec.DecodeError as e: raise RpcMalformedRequestHeaders(detail="Could not parse X-Repository-Id header") from e - return authorization, organization_id, repository_id + return organization_id, repository_id + + +def _take_signature(headers: Mapping[str, str]) -> tuple[Secret, dict[str, str]]: + """Split the request's signature from the other headers. Header names are case-insensitive.""" + signature: Secret | None = None + rest: dict[str, str] = {} + for name, value in headers.items(): + if name.lower() == "authorization": + signature = Secret(value.removeprefix("rpcsignature ")) + else: + rest[name] = value + if signature is None: + raise RpcMalformedRequestHeaders(detail="Could not find Authorization header") + return signature, rest def iter_response(response: requests.Response) -> Iterator[bytes]: diff --git a/tests/unit/error_reports.py b/tests/unit/error_reports.py index 1bb905e..af4a846 100644 --- a/tests/unit/error_reports.py +++ b/tests/unit/error_reports.py @@ -7,6 +7,19 @@ def assert_not_in_frames(error: BaseException, value: str) -> None: tb = error.__traceback__ while tb is not None: for name, local in tb.tb_frame.f_locals.items(): - shown = local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local) - assert value not in shown, f"{tb.tb_frame.f_code.co_name} holds it in {name}" + assert value not in _shown(local), f"{tb.tb_frame.f_code.co_name} holds it in {name}" tb = tb.tb_next + + +def reported_local(error: BaseException, function: str, name: str) -> str: + """How an error report shows the local variable ``name`` of ``function``'s frame in the error's traceback.""" + tb = error.__traceback__ + while tb is not None: + if tb.tb_frame.f_code.co_name == function: + return _shown(tb.tb_frame.f_locals[name]) + tb = tb.tb_next + raise AssertionError(f"The traceback has no frame of {function}.") + + +def _shown(local: object) -> str: + return local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local) diff --git a/tests/unit/test_rpc_client.py b/tests/unit/test_rpc_client.py index 3e4c394..41830e3 100644 --- a/tests/unit/test_rpc_client.py +++ b/tests/unit/test_rpc_client.py @@ -3,6 +3,7 @@ import msgspec import pytest +import requests from requests.exceptions import ConnectionError as RequestsConnectionError from scm.errors import ErrorCode, ResourceServiceUnavailable, SCMCodedError @@ -19,6 +20,7 @@ from scm.rpc.server import serialize_repository from scm.rpc.types import Error, ErrorResponse from scm.secret import Secret +from tests.unit.error_reports import assert_not_in_frames, reported_local def make_repository(**overrides): @@ -532,3 +534,43 @@ def test_does_not_retry_status_code_outside_config(self, mock_sleep): assert client.session.post.call_count == 1 mock_sleep.assert_not_called() assert self._metric_names(client) == [] + + +class UnreachableSession: + """Keeps the Authorization header it is sent, then fails as if Sentry were unreachable.""" + + def __init__(self) -> None: + self.authorization = "" + + def get(self, url: str, headers: dict[str, str]) -> requests.Response: + raise NotImplementedError + + def post(self, url: str, data: bytes, headers: dict[str, str], stream: bool = False) -> requests.Response: + self.authorization = headers["Authorization"] + raise RequestsConnectionError("connection reset") + + +class TestKeptOutOfErrorReports: + """An error is reported to Sentry with every frame's local variables, and a signature in one could be replayed.""" + + def test_exhausted_connection_retries_leave_out_the_signature(self): + session = UnreachableSession() + client = RpcApiClient( + full_url="http://base/api/0/internal/scm-rpc", + signing_secret=Secret("secret"), + organization_id=1, + referrer="shared", + repository_id=1, + session=lambda: session, + retry={"max_retries": 1, "backoff_seconds": 0, "status_codes": (), "retry_connection_errors": True}, + ) + + with pytest.raises(ResourceServiceUnavailable) as exc_info: + client.request(method="GET", path="/repos/org/repo") + + error = exc_info.value + assert_not_in_frames(error, session.authorization.removeprefix("rpcsignature ")) + assert reported_local(error, "request", "authorization") == "[Filtered]" + # A chained exception is reported with its own frames, and the session's holds the headers it was sent. + assert error.__cause__ is None + assert error.__suppress_context__ diff --git a/tests/unit/test_rpc_server.py b/tests/unit/test_rpc_server.py index 7a8f7eb..f71b368 100644 --- a/tests/unit/test_rpc_server.py +++ b/tests/unit/test_rpc_server.py @@ -4,12 +4,20 @@ import msgspec import pytest -from scm.errors import SCMCodedError +from scm.errors import ResourceServiceUnavailable, SCMCodedError from scm.rpc.helpers import deserialize_repository, sign_get, sign_post -from scm.rpc.server import RpcServer, is_safe_path, iter_response, normalize_headers, serialize_repository +from scm.rpc.server import ( + RpcServer, + _take_signature, + is_safe_path, + iter_response, + normalize_headers, + serialize_repository, +) from scm.secret import Secret from scm.test_fixtures import BaseTestProvider from scm.types import Repository +from tests.unit.error_reports import assert_not_in_frames, reported_local TEST_SECRET = Secret("test-secret") @@ -55,28 +63,38 @@ def make_headers(**overrides) -> dict[str, str]: return defaults +class TestTakeSignature: + def test_takes_the_signature_out_of_the_headers(self): + signature, headers = _take_signature(make_headers()) + assert signature.reveal() == "rpc0:abc" + assert headers == {"X-Organization-Id": "1", "X-Repository-Id": "1"} + + def test_header_name_is_case_insensitive(self): + signature, headers = _take_signature({"authorization": "rpcsignature rpc0:abc", "X-Organization-Id": "1"}) + assert signature.reveal() == "rpc0:abc" + assert headers == {"X-Organization-Id": "1"} + + def test_missing_authorization_raises(self): + headers = make_headers() + del headers["Authorization"] + with pytest.raises(SCMCodedError) as exc_info: + _take_signature(headers) + assert exc_info.value.code == "rpc_malformed_request_headers" + + class TestExtractHeaders: def test_extracts_int_repository_id(self): server = make_server() - auth, org_id, repo_id = server._extract_headers(make_headers()) - assert auth == "rpc0:abc" + org_id, repo_id = server._extract_headers(make_headers()) assert org_id == 1 assert repo_id == 1 def test_extracts_tuple_repository_id(self): server = make_server() headers = make_headers(**{"X-Repository-Id": '["github","ext-123"]'}) - _, _, repo_id = server._extract_headers(headers) + _, repo_id = server._extract_headers(headers) assert repo_id == ("github", "ext-123") - def test_missing_authorization_raises(self): - server = make_server() - headers = make_headers() - del headers["Authorization"] - with pytest.raises(SCMCodedError) as exc_info: - server._extract_headers(headers) - assert exc_info.value.code == "rpc_malformed_request_headers" - def test_missing_organization_id_raises(self): server = make_server() headers = make_headers() @@ -290,6 +308,53 @@ def test_successful_post_streams_response(self): assert b'{"ref": "main"}' in chunks +class UnavailableProvider(BaseTestProvider): + def request(self, *args, **kwargs): # type: ignore[override] + raise ResourceServiceUnavailable() + + +class TestKeptOutOfErrorReports: + """An error is reported to Sentry with every frame's local variables, and a signature in one could be replayed.""" + + @pytest.mark.parametrize("header", ["Authorization", "authorization"]) + def test_post_reporting_a_provider_failure_leaves_out_the_signature(self, header): + errors: list[Exception] = [] + server = make_server(fetch_provider=lambda org_id, repo: UnavailableProvider(), emit_error=errors.append) + body = msgspec.json.encode( + { + "type": "action", + "data": { + "method": "GET", + "path": "/repos/org/repo", + "headers": None, + "data": None, + "params": None, + "allow_redirects": None, + "stream": None, + }, + } + ) + signature = sign_post(TEST_SECRET, body) + + server.post(body, {header: f"rpcsignature {signature}", "X-Organization-Id": "1", "X-Repository-Id": "1"}) + + [error] = errors + assert error.code == "resource_service_unavailable" # type: ignore[attr-defined] + assert_not_in_frames(error, signature) + assert reported_local(error, "post", "signature") == "[Filtered]" + + def test_get_failing_to_fetch_the_repository_leaves_out_the_signature(self): + def failing_fetch(organization_id, repository_id): + raise RuntimeError("database down") + + server = make_server(fetch_repository=failing_fetch) + # Not a coded error, so it reaches the caller, whose error reporting sees every frame here. + with pytest.raises(RuntimeError) as exc_info: + server.get(make_headers(Authorization=f"rpcsignature {sign_get(TEST_SECRET, 1, 1)}")) + assert_not_in_frames(exc_info.value, sign_get(TEST_SECRET, 1, 1)) + assert reported_local(exc_info.value, "get", "signature") == "[Filtered]" + + class TestSerializeRepository: def test_serializes_all_fields(self): repo = make_repository()