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()