diff --git a/src/scm/errors.py b/src/scm/errors.py index cb40781..59a9e33 100644 --- a/src/scm/errors.py +++ b/src/scm/errors.py @@ -6,6 +6,7 @@ "git_operation_not_granted", "git_provider_not_supported", "git_remote_not_https", + "git_remote_refused", "git_repository_not_granted", "git_route_not_supported", "git_token_rejected", @@ -53,6 +54,7 @@ "git_operation_not_granted": "The git grant does not allow this operation.", "git_provider_not_supported": "The repository's provider has no git remote.", "git_remote_not_https": "The repository's git remote is not an https URL.", + "git_remote_refused": "The repository's git remote is at an address Sentry does not connect to.", "git_repository_not_granted": "The git grant does not cover this repository.", "git_route_not_supported": "The request is not a git fetch or push.", "git_token_rejected": "The provider rejected Sentry's credentials for the repository.", @@ -169,6 +171,10 @@ class GitRemoteNotHttps(GitSCMCodedError): code = "git_remote_not_https" +class GitRemoteRefused(GitSCMCodedError): + code = "git_remote_refused" + + class GitRepositoryNotGranted(GitSCMCodedError): code = "git_repository_not_granted" diff --git a/src/scm/git/relay.py b/src/scm/git/relay.py index eab0d79..aa8d785 100644 --- a/src/scm/git/relay.py +++ b/src/scm/git/relay.py @@ -18,6 +18,7 @@ ResourceGatewayTimeout, ResourceServiceUnavailable, RpcMalformedRequestHeaders, + SCMCodedError, ) from scm.git.types import GitOperation, Grant, Readable from scm.rpc.types import StreamResponse @@ -107,12 +108,18 @@ class GitRelay: ``adapter`` builds the adapter that sends each upstream request. Pass one that refuses internal addresses: a GitHub Enterprise or self-managed GitLab host is whatever the customer configured, and the relay connects wherever it resolves. Sentry's ``BlacklistAdapter`` does this for its API clients, and the class itself can be - passed. An error the adapter raises for a refused host reaches the caller unchanged, unless it is a requests or - urllib3 exception, which the relay replaces like any other so that it cannot carry the token. One that reaches - the caller does carry it: an adapter refusing the host as it connects raises through urllib3's frames, which hold - the Authorization header, so report such an error without its frames' local variables. The adapter must - not retry, because a retry cannot resend a body that has already been streamed. Without one, the relay uses a plain - ``HTTPAdapter``, which checks nothing. + passed. The adapter must not retry, because a retry cannot resend a body that has already been streamed. Without + one, the relay uses a plain ``HTTPAdapter``, which checks nothing. + + The relay replaces a requests or urllib3 exception from the upstream request with a coded error, so that it + cannot carry the token. ``map_error`` gets any other exception, such as the adapter's own for a refused host, + and returns either a coded error, which the relay raises in its place without the original, or None to let the + original reach the caller unchanged. An error that reaches the caller this way does carry the token: an adapter + refusing the host as it connects raises through urllib3's frames, which hold the Authorization header, so report + such an error without its frames' local variables. GitServer answers a coded error like its other refusals. For + an adapter that refuses a host with ``HostRefused``:: + + GitRelay(adapter=..., map_error=lambda e: GitRemoteRefused() if isinstance(e, HostRefused) else None) The relay takes no settings from the environment. Through a proxy from ``HTTPS_PROXY``, the adapter would check the proxy's address rather than the provider's. ``ca_bundle`` names a CA file to trust instead of the @@ -123,11 +130,13 @@ def __init__( self, *, adapter: Callable[[], HTTPAdapter] = HTTPAdapter, + map_error: Callable[[Exception], SCMCodedError | None] | None = None, ca_bundle: str | None = None, timeout: tuple[float, float] = DEFAULT_TIMEOUT, chunk_size: int = DEFAULT_CHUNK_SIZE, ) -> None: self.adapter = adapter + self.map_error = map_error self.ca_bundle = ca_bundle self.timeout = timeout self.chunk_size = chunk_size @@ -219,6 +228,16 @@ def forward( # Any other failure of the request, e.g. a malformed response. A requests exception, like those above, # holds the request. raise ResourceBadGateway() from None + # The relay's own, such as the client's connection failing while its body is sent. + except SCMCodedError: + raise + except Exception as error: + mapped = self.map_error(error) if self.map_error is not None else None + if mapped is None: + raise + # "from None" drops the original. Raised as the adapter connects, e.g. for a refused host, its traceback + # runs through urllib3's frames, which hold the Authorization header. + raise mapped from None finally: # Once a response arrives, _stream closes the session when the response ends. if upstream is None: diff --git a/src/scm/git/server.py b/src/scm/git/server.py index 6f054cc..295198e 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -33,6 +33,7 @@ "missing_repository_path": 500, # The provider cannot be reached safely, or failed. "git_remote_not_https": 502, + "git_remote_refused": 502, "git_token_rejected": 502, "git_token_unavailable": 502, "resource_bad_gateway": 502, @@ -59,8 +60,8 @@ class GitServer: when it failed, as tags: ``repository_id`` as requested, then ``organization_id`` and ``referrer`` once the grant is verified, ``operation``, ``provider`` and ``remote_host``. A tag is missing when the request failed before it was known. A client closing its request is not reported, since there is nothing to fix. Any other - exception, such as an adapter refusing a host or the token supplier failing, reaches the caller unchanged, so - the caller can answer it the way it answers the same error elsewhere. + exception, such as the token supplier failing, or an error of the relay's adapter that its ``map_error`` leaves + alone, reaches the caller unchanged, so the caller can answer it the way it answers the same error elsewhere. Every count is tagged with the grant's ``referrer`` once the grant is verified, so Sentry can tell a coding job's clone from Seer's own fetch or push. diff --git a/tests/unit/git_upstream.py b/tests/unit/git_upstream.py index 71cdb30..38addcb 100644 --- a/tests/unit/git_upstream.py +++ b/tests/unit/git_upstream.py @@ -8,6 +8,7 @@ import datetime import functools import ipaddress +import socket import ssl import sys import tempfile @@ -23,6 +24,8 @@ from cryptography.x509.oid import NameOID from requests.adapters import HTTPAdapter from requests.structures import CaseInsensitiveDict +from urllib3 import HTTPSConnectionPool +from urllib3.connection import HTTPSConnection @dataclass @@ -138,6 +141,27 @@ def close(self) -> None: super().close() +class HostRefused(Exception): + """Stands in for Sentry's ``RestrictedIPAddress``, which is neither a requests nor a urllib3 exception.""" + + +class _RefusingConnection(HTTPSConnection): + def _new_conn(self) -> socket.socket: + raise HostRefused(f"{self.host} is an internal address") + + +class _RefusingPool(HTTPSConnectionPool): + ConnectionCls = _RefusingConnection + + +class RefusingAdapter(HTTPAdapter): + """Refuses every host as it connects, from inside urllib3 the way Sentry's ``BlacklistAdapter`` does.""" + + def init_poolmanager(self, *args, **kwargs) -> None: + super().init_poolmanager(*args, **kwargs) + self.poolmanager.pool_classes_by_scheme = {"https": _RefusingPool} + + def echo(handler: UpstreamHandler) -> None: body = b"".join(handler.body_chunks()) handler.record(body) diff --git a/tests/unit/test_git_relay.py b/tests/unit/test_git_relay.py index 8a853a2..db2ec1a 100644 --- a/tests/unit/test_git_relay.py +++ b/tests/unit/test_git_relay.py @@ -3,7 +3,7 @@ import socket import threading import time -from collections.abc import Iterator +from collections.abc import Callable, Iterator from unittest.mock import MagicMock import pytest @@ -14,6 +14,7 @@ from scm.errors import ( GitOperationNotGranted, GitRemoteNotHttps, + GitRemoteRefused, GitRepositoryNotGranted, GitRouteNotSupported, ResourceBadGateway, @@ -27,7 +28,15 @@ from scm.types import GitRemote from tests.unit.error_reports import assert_not_in_frames, reported_local from tests.unit.git_builders import make_grant -from tests.unit.git_upstream import ClosingAdapter, UpstreamHandler, basic_auth, ca_bundle, running_upstream +from tests.unit.git_upstream import ( + ClosingAdapter, + HostRefused, + RefusingAdapter, + UpstreamHandler, + basic_auth, + ca_bundle, + running_upstream, +) TOKEN = Secret("ghs_secrettoken") # Long enough that a relay that buffers fails the test instead of hanging it. @@ -414,16 +423,70 @@ def handler(h: UpstreamHandler) -> None: class TestAdapter: def test_uses_the_adapter_it_is_given(self, upstream): # Sentry passes an adapter that refuses internal addresses; the relay must send through it. - class HostRefused(Exception): - pass + with pytest.raises(HostRefused, match="internal address"): + fetch_refs(GitRelay(adapter=RefusingAdapter, ca_bundle=ca_bundle()), upstream) + assert upstream.connections == 0 + + +def refusal_mapper(seen: list[Exception]) -> Callable[[Exception], SCMCodedError | None]: + """A map_error that records each error it gets and maps HostRefused, the way Sentry maps its refusal.""" + + def map_error(error: Exception) -> SCMCodedError | None: + seen.append(error) + return GitRemoteRefused() if isinstance(error, HostRefused) else None + + return map_error + - class RefusingAdapter(HTTPAdapter): +class TestMapError: + def test_mapped_error_is_raised_in_place_of_the_original(self, upstream): + seen: list[Exception] = [] + relay = GitRelay(adapter=RefusingAdapter, map_error=refusal_mapper(seen), ca_bundle=ca_bundle()) + with pytest.raises(GitRemoteRefused) as exc_info: + fetch_refs(relay, upstream) + assert [type(error) for error in seen] == [HostRefused] + # The refusal passes through urllib3's frames, which hold the Authorization header. + assert_token_not_in(exc_info.value) + # The refusal names the address; the client has no use for it. + assert "127.0.0.1" not in exc_info.value.detail + + @pytest.mark.parametrize("map_error", [None, lambda error: None], ids=["no_mapper", "unmapped"]) + def test_unmapped_error_reaches_the_caller_unchanged(self, upstream, map_error): + with pytest.raises(HostRefused, match="internal address"): + fetch_refs(GitRelay(adapter=RefusingAdapter, map_error=map_error, ca_bundle=ca_bundle()), upstream) + + @pytest.mark.parametrize( + ("make_error", "expected"), + [ + pytest.param( + lambda request: requests.ConnectionError("refused", request=request), + ResourceServiceUnavailable, + id="requests", + ), + pytest.param( + lambda request: urllib3.exceptions.HTTPError("bad response"), ResourceBadGateway, id="urllib3" + ), + ], + ) + def test_requests_and_urllib3_errors_never_reach_the_mapper(self, upstream, make_error, expected): + # A requests exception holds the request and its Authorization header. + class FailingAdapter(HTTPAdapter): def send(self, request, *args, **kwargs): - raise HostRefused(request.url) + raise make_error(request) - with pytest.raises(HostRefused): - fetch_refs(GitRelay(adapter=RefusingAdapter, ca_bundle=ca_bundle()), upstream) - assert upstream.requests == [] + seen: list[Exception] = [] + relay = GitRelay(adapter=FailingAdapter, map_error=refusal_mapper(seen), ca_bundle=ca_bundle()) + with pytest.raises(expected): + fetch_refs(relay, upstream) + assert seen == [] + + def test_the_relays_own_errors_never_reach_the_mapper(self, upstream): + seen: list[Exception] = [] + relay = GitRelay(map_error=refusal_mapper(seen), ca_bundle=ca_bundle()) + with pytest.raises(SCMCodedError) as exc_info: + upload_pack(relay, upstream, io.BytesIO(b"00"), headers={"Content-Length": "4"}) + assert exc_info.value.code == "client_closed_request" + assert seen == [] class TestEnvironment: diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 84d6652..1602ee8 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -7,7 +7,7 @@ import pytest -from scm.errors import ClientClosedRequest, RateLimitExceeded +from scm.errors import ClientClosedRequest, GitRemoteRefused, RateLimitExceeded from scm.git.grant import sign_grant from scm.git.relay import GitRelay, RelayResponse, ResponseBody from scm.git.server import _STATUS, GitServer @@ -17,7 +17,7 @@ from scm.types import GitRemote, Provider, Repository, RepositoryId from tests.unit.error_reports import assert_not_in_frames, reported_local from tests.unit.git_builders import make_grant, make_provider, make_repository -from tests.unit.git_upstream import ClosingAdapter, basic_auth, running_upstream +from tests.unit.git_upstream import ClosingAdapter, HostRefused, RefusingAdapter, basic_auth, running_upstream SECRET = Secret("test-secret-0000000000000000000000") TOKEN = Secret("ghs_secrettoken") @@ -400,6 +400,22 @@ def test_plain_http_remote(self): ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "git_remote_not_https"}) ] + def test_adapter_refusing_the_host(self): + # e.g. Sentry's BlacklistAdapter refusing a GitHub Enterprise host that resolves to an internal address. + recorder = Recorder( + repository=make_repository(provider_name="github_enterprise", web_base_url="https://127.0.0.1:1") + ) + relay = GitRelay( + adapter=RefusingAdapter, + map_error=lambda error: GitRemoteRefused() if isinstance(error, HostRefused) else None, + ) + response = request(make_server(recorder, relay), signed_grant()) + assert_refused(response, 502, "at an address Sentry does not connect to") + assert recorder.counts == [ + ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "git_remote_refused"}) + ] + assert recorder.error_codes == ["git_remote_refused"] + def test_provider_rejecting_the_token(self): # Passed on, a 401 would make git ask the sandboxed client for credentials. adapters: list[ClosingAdapter] = [] @@ -474,10 +490,8 @@ def test_no_refusal_is_a_401(self): assert 401 not in _STATUS.values() def test_other_errors_reach_the_caller(self): - # Sentry's view answers these the way it answers them elsewhere, e.g. a refused host as ApiHostError. - class HostRefused(Exception): - pass - + # Sentry's view answers these the way it answers them elsewhere, e.g. an adapter's error that the relay's + # map_error leaves alone. relay = MagicMock(spec=GitRelay) relay.forward.side_effect = HostRefused() recorder = Recorder()