From 090b07e835d3c1927cee22fab508c08f647565db Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 08:38:16 +0100 Subject: [PATCH 1/3] feat(git)!: Tell a bad git grant key apart from a bad grant Porting Sentry and Seer to the Ed25519 grants showed that neither could tell its own misconfiguration from a bad or oversized grant. A bad key raised plain ValueError or TypeError from the same calls that parse untrusted grants. So Sentry called require_public_keys before every verify_grant and caught both types, and Seer called public_key_of only to check its key at startup. GitGrantKeyError now covers every bad-key case on both sides. It's a ValueError, so existing handlers still catch it, but not an SCMCodedError: a bad key is the server's fault, and a coded error would be answered as a refused grant. The oversized-grant error stays a plain ValueError. require_private_key gives signers the startup check that verifiers already had, and generate_private_key replaces the cryptography calls that bin/git-server, the tests and both consumers' tests each copied. The key-material tests now check each base64 line on its own. Before, they joined the lines into one string, which never matched a multi-line key (RSA, EC, Ed448), so those cases passed without checking anything. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- bin/git-server | 11 +-- src/scm/errors.py | 11 +++ src/scm/git/grant.py | 72 +++++++++++++----- src/scm/git/server.py | 4 +- tests/unit/git_builders.py | 10 ++- tests/unit/test_errors.py | 9 +++ tests/unit/test_git_grant.py | 133 +++++++++++++++++++++++++++------- tests/unit/test_git_server.py | 19 ++--- 9 files changed, 204 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index f756974..36cbaf8 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ The platform exposes four subsystems: - **Actions** — outbound SCM operations initiated by Sentry code. The `SourceCodeManager` class provides 70+ methods covering comments, reactions, pull requests, branches, git objects, reviews, and check runs. With more actions planned to be added as we port more use cases. - **Actions RPC** - outbound SCM commands exposed over the network. The `SourceCodeManager` is fully available over the network. This enables usage of the SCM for services outside the monolith. -- **Git relay** - `git` fetches and pushes for services that hold no provider credentials. Seer signs a grant naming the organization, operation and repositories with `scm.git.grant.sign_grant` and an Ed25519 private key only Seer holds. `scm.git.server.GitServer` verifies the grant against the matching public keys, so whoever verifies grants cannot mint one, and checks each git smart-HTTP request against it before relaying the request to the provider with a token, streaming both bodies. +- **Git relay** - `git` fetches and pushes for services that hold no provider credentials. Seer signs a grant naming the organization, operation and repositories with `scm.git.grant.sign_grant` and an Ed25519 private key only Seer holds. `scm.git.server.GitServer` verifies the grant against the matching public keys, so whoever verifies grants cannot mint one, and checks each git smart-HTTP request against it before relaying the request to the provider with a token, streaming both bodies. A missing or unusable key on either side raises `scm.errors.GitGrantKeyError`, the server's misconfiguration rather than a refused grant. `require_private_key` and `require_public_keys` check the keys at startup, and `generate_private_key` makes a key for tests and local tools. - **Event Stream** — inbound webhook processing. SCM service-providers push events which are deserialized into typed, provider-neutral dataclasses (`CheckRunEvent`, `CommentEvent`, `PullRequestEvent`) which are then dispatched to registered listener functions. ### Why diff --git a/bin/git-server b/bin/git-server index 819b3ef..6096ef2 100755 --- a/bin/git-server +++ b/bin/git-server @@ -95,11 +95,9 @@ from pathlib import Path from types import ModuleType from urllib.parse import urlsplit -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey -from cryptography.hazmat.primitives.serialization import Encoding, NoEncryption, PrivateFormat - from scm import Secret -from scm.git.grant import public_key_of, sign_grant +from scm.errors import GitGrantKeyError +from scm.git.grant import generate_private_key, public_key_of, sign_grant from scm.git.relay import GitRelay from scm.git.server import GitServer from scm.git.types import GRANT_HEADER, GetToken, GitOperation, Grant, Readable @@ -233,8 +231,7 @@ def grant_keys(signing_key_path: str | None) -> tuple[Secret, str]: its repository, so nothing else needs to know it. """ if not signing_key_path: - key = Ed25519PrivateKey.generate() - private_key = Secret(key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption()).decode()) + private_key = generate_private_key() return private_key, public_key_of(private_key) try: # Bytes that are not UTF-8, as in a DER file, become U+FFFD, so the error says the file is not PEM rather @@ -243,7 +240,7 @@ def grant_keys(signing_key_path: str | None) -> tuple[Secret, str]: return private_key, public_key_of(private_key) except OSError as e: logger.error(f"Error: cannot read {signing_key_path}: {e.strerror or e}.") - except ValueError as e: + except GitGrantKeyError as e: # scm.git.grant's messages name no key material. logger.error(f"Error: {signing_key_path}: {e}") sys.exit(1) diff --git a/src/scm/errors.py b/src/scm/errors.py index 59a9e33..bfd674a 100644 --- a/src/scm/errors.py +++ b/src/scm/errors.py @@ -147,6 +147,17 @@ def from_code(cls, code: ErrorCode, detail: str | None = None) -> "SCMCodedError return error_cls(detail=detail) +class GitGrantKeyError(SCMError, ValueError): + """A git grant key is missing or unusable: the signer's private key, or one of the verifier's public keys. + + The key is the server's configuration, so a bad one is the server's fault rather than the client's. Unlike the + git endpoint's refusals it is not an :class:`SCMCodedError`: ``verify_grant`` raises it instead of refusing the + grant, and ``GitServer`` raises it when it is built instead of answering requests with it. It is a + ``ValueError``, so code that catches one still catches it. Its message names a key by its role or by its place in + the list, and never quotes it. + """ + + class GitSCMCodedError(SCMCodedError): """Base class for the errors of the git endpoint (``scm.git``), so a caller can catch them all at once.""" diff --git a/src/scm/git/grant.py b/src/scm/git/grant.py index 86266b6..400ee2a 100644 --- a/src/scm/git/grant.py +++ b/src/scm/git/grant.py @@ -8,8 +8,12 @@ they rotate. Keys are PEM strings, as for objectstore's tokens: the private key as PKCS#8 (``-----BEGIN PRIVATE KEY-----``), -public keys as SubjectPublicKeyInfo (``-----BEGIN PUBLIC KEY-----``). ``openssl genpkey -algorithm ed25519`` writes -a private key, and ``openssl pkey -pubout`` or ``public_key_of`` gives its public key. +public keys as SubjectPublicKeyInfo (``-----BEGIN PUBLIC KEY-----``). ``openssl genpkey -algorithm ed25519`` or +``generate_private_key`` makes a private key, and ``openssl pkey -pubout`` or ``public_key_of`` gives its public key. + +A key that is missing or unusable raises ``GitGrantKeyError`` from every function that takes it, whichever side it +is on. ``require_private_key`` and ``require_public_keys`` check the keys on their own, so a signer or a verifier +can refuse to start with a bad one. """ import hashlib @@ -23,12 +27,14 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey, Ed25519PublicKey from cryptography.hazmat.primitives.serialization import ( Encoding, + NoEncryption, + PrivateFormat, PublicFormat, load_pem_private_key, load_pem_public_key, ) -from scm.errors import GitGrantExpired, GitInvalidGrant +from scm.errors import GitGrantExpired, GitGrantKeyError, GitInvalidGrant from scm.git.types import GRANT_MAX_LENGTH, Grant from scm.secret import Secret @@ -43,7 +49,9 @@ def sign_grant(private_key: Secret, grant: Grant, *, now: float | None = None) -> str: """Sign ``grant`` with ``private_key``, an Ed25519 private key in PEM. - The ``kid`` header is derived from the key, so a signer needs no key id setting next to it. + The ``kid`` header is derived from the key, so a signer needs no key id setting next to it. Raises + GitGrantKeyError if ``private_key`` is not an unencrypted Ed25519 private key in PEM, and a plain ValueError if + the signed grant is longer than GRANT_MAX_LENGTH. """ key = _load_private_key(private_key) claims: dict[str, Any] = msgspec.to_builtins(grant) @@ -64,19 +72,40 @@ def sign_grant(private_key: Secret, grant: Grant, *, now: float | None = None) - def public_key_of(private_key: Secret) -> str: """Return the public key, in PEM, that verifies the grants ``private_key`` signs. - It goes in the verifier's list of public keys. Raises ValueError, as ``sign_grant`` does, if ``private_key`` is - not an unencrypted Ed25519 private key in PEM. + It goes in the verifier's list of public keys. Raises GitGrantKeyError, as ``sign_grant`` does, if + ``private_key`` is not an unencrypted Ed25519 private key in PEM. """ public_key = _load_private_key(private_key).public_key() return public_key.public_bytes(Encoding.PEM, PublicFormat.SubjectPublicKeyInfo).decode() +def generate_private_key() -> Secret: + """Return a new Ed25519 private key to sign grants with, in PEM (PKCS#8). + + For tests and local tools that sign their own grants. ``public_key_of`` gives the public key that verifies them. + """ + key = Ed25519PrivateKey.generate() + # Wrapped inline, so no frame holds the PEM outside its Secret. + return Secret(key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption()).decode()) + + +def require_private_key(private_key: Secret) -> Secret: + """Return ``private_key`` if it is an unencrypted Ed25519 private key in PEM, and raise GitGrantKeyError if not. + + For a signer to check its key when it starts, as ``require_public_keys`` does for a verifier, rather than when it + signs its first grant. + """ + _load_private_key(private_key) + return private_key + + def verify_grant(public_keys: Sequence[str], token: str, *, now: float | None = None) -> Grant: """Return the grant if the private key of one of ``public_keys`` signed it and it has not expired. Verification takes a list of Ed25519 public keys in PEM, so a new key can be added before the signer switches to - it and the old one removed after its grants expire. Raises ValueError, rather than refusing the grant, if the - list is empty or a key is not one, since no grant could verify against it. + it and the old one removed after its grants expire. Raises GitGrantKeyError, rather than refusing the grant, if + the list is empty or a key is not one, since no grant could verify against it. Every other failure is a + GitSCMCodedError that refuses the grant. """ keys = _public_keys_by_id(public_keys) now = time.time() if now is None else now @@ -131,32 +160,39 @@ def verify_grant(public_keys: Sequence[str], token: str, *, now: float | None = def require_public_keys(public_keys: Sequence[str]) -> list[str]: """Return ``public_keys`` as a list, if there is at least one and each is an Ed25519 public key in PEM. - For GitServer, which verifies a grant against its keys on every request. A key that does not parse would make - every request fail, so the server fails once, when it is built. + Raises GitGrantKeyError if not. For GitServer, which verifies a grant against its keys on every request. A key + that does not parse would make every request fail, so the server fails once, when it is built. """ _public_keys_by_id(public_keys) return list(public_keys) def _load_private_key(private_key: Secret) -> Ed25519PrivateKey: + if not isinstance(private_key, Secret): + # A plain string, from code the type checker does not see, would otherwise fail on reveal() with an + # AttributeError that says nothing about the key. + raise GitGrantKeyError( + f"The git grant signing key must be an scm.Secret, not {type(private_key).__name__}. Wrap the key in " + "scm.Secret where you read it." + ) if not private_key: - raise ValueError("The git grant signing key is empty.") + raise GitGrantKeyError("The git grant signing key is empty.") try: # Revealed inline, so no frame holds the PEM outside its Secret. key = load_pem_private_key(private_key.reveal().encode(), password=None) except (ValueError, TypeError, UnsupportedAlgorithm) as e: # cryptography's reason, chained, names no key material. - raise ValueError( + raise GitGrantKeyError( "The git grant signing key is not an unencrypted private key in PEM (-----BEGIN PRIVATE KEY-----)." ) from e if not isinstance(key, Ed25519PrivateKey): - raise ValueError(f"The git grant signing key must be an Ed25519 key, not {type(key).__name__}.") + raise GitGrantKeyError(f"The git grant signing key must be an Ed25519 key, not {type(key).__name__}.") return key def _public_keys_by_id(public_keys: Sequence[str]) -> dict[str, Ed25519PublicKey]: if not public_keys: - raise ValueError("There are no git grant public keys, so no grant could verify.") + raise GitGrantKeyError("There are no git grant public keys, so no grant could verify.") keys: dict[str, Ed25519PublicKey] = {} for index, pem in enumerate(public_keys): key = _load_public_key(pem, index) @@ -167,20 +203,20 @@ def _public_keys_by_id(public_keys: Sequence[str]) -> dict[str, Ed25519PublicKey def _load_public_key(pem: str, index: int) -> Ed25519PublicKey: # Each message names the key by its place in the list rather than quoting it. if not isinstance(pem, str): - raise TypeError(f"public_keys[{index}] must be a PEM string, not {type(pem).__name__}.") + raise GitGrantKeyError(f"public_keys[{index}] must be a PEM string, not {type(pem).__name__}.") # load_pem_public_key refuses a private key too, but saying what it is tells the operator to move it away from # where verifiers read their keys. if "PRIVATE KEY" in pem: - raise ValueError( + raise GitGrantKeyError( f"public_keys[{index}] is a private key. Verifiers take public keys only; the private key stays with the " "signer." ) try: key = load_pem_public_key(pem.encode()) except (ValueError, UnsupportedAlgorithm) as e: - raise ValueError(f"public_keys[{index}] is not a public key in PEM (-----BEGIN PUBLIC KEY-----).") from e + raise GitGrantKeyError(f"public_keys[{index}] is not a public key in PEM (-----BEGIN PUBLIC KEY-----).") from e if not isinstance(key, Ed25519PublicKey): - raise ValueError(f"public_keys[{index}] must be an Ed25519 key, not {type(key).__name__}.") + raise GitGrantKeyError(f"public_keys[{index}] must be an Ed25519 key, not {type(key).__name__}.") return key diff --git a/src/scm/git/server.py b/src/scm/git/server.py index 2071560..a463baa 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -47,7 +47,9 @@ class GitServer: """Serves a git endpoint: checks each request against its grant, then relays it with a provider token. Grants are verified against ``public_keys``, the Ed25519 public keys in PEM whose private keys sign them; see - ``scm.git.grant``. + ``scm.git.grant``. The keys are checked when the server is built: an empty list, or one holding anything but + Ed25519 public keys in PEM, raises ``GitGrantKeyError`` from the constructor, since no grant could verify against + it. That is the server's misconfiguration rather than a refusal, so the server never answers a request with it. The order of the checks matters. The grant is verified, and the request's route and repository checked against it, before the repository and its provider are looked up or a token fetched, so a request the grant diff --git a/tests/unit/git_builders.py b/tests/unit/git_builders.py index 7b6236b..fe1418c 100644 --- a/tests/unit/git_builders.py +++ b/tests/unit/git_builders.py @@ -4,8 +4,9 @@ from unittest.mock import MagicMock from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey -from cryptography.hazmat.primitives.serialization import Encoding, NoEncryption, PrivateFormat, PublicFormat +from cryptography.hazmat.primitives.serialization import Encoding, PublicFormat, load_pem_private_key +from scm.git.grant import generate_private_key from scm.git.types import Grant from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider @@ -70,9 +71,12 @@ class KeyPair(NamedTuple): def make_key_pair() -> KeyPair: """A new Ed25519 key pair for signing grants. Tests make their own, so no private key is committed.""" - key = Ed25519PrivateKey.generate() + private = generate_private_key() + key = load_pem_private_key(private.reveal().encode(), password=None) + assert isinstance(key, Ed25519PrivateKey) return KeyPair( key=key, - private=Secret(key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption()).decode()), + private=private, + # Serialized here rather than by public_key_of, so the tests check public_key_of against cryptography. public=key.public_key().public_bytes(Encoding.PEM, PublicFormat.SubjectPublicKeyInfo).decode(), ) diff --git a/tests/unit/test_errors.py b/tests/unit/test_errors.py index f9bdf51..c5e758c 100644 --- a/tests/unit/test_errors.py +++ b/tests/unit/test_errors.py @@ -7,6 +7,7 @@ ERROR_CODES, ClientClosedRequest, ErrorCode, + GitGrantKeyError, RateLimitExceeded, RepositoryNotFound, ResourceBadGateway, @@ -84,6 +85,14 @@ def test_can_be_caught_as_base_type(self): raise RpcInvalidGrant() +def test_git_grant_key_error_is_not_a_refusal(): + # A bad key is the server's mistake. As a coded error, GitServer would answer it as a refused grant, blaming the + # client; as a ValueError, code written before it existed still catches it. + assert not issubclass(GitGrantKeyError, SCMCodedError) + assert issubclass(GitGrantKeyError, SCMError) + assert issubclass(GitGrantKeyError, ValueError) + + class TestErrorClassForStatus: @pytest.mark.parametrize( ("status_code", "expected_type"), diff --git a/tests/unit/test_git_grant.py b/tests/unit/test_git_grant.py index 64fdb8c..3c0f8da 100644 --- a/tests/unit/test_git_grant.py +++ b/tests/unit/test_git_grant.py @@ -9,17 +9,27 @@ import msgspec import pytest from cryptography.hazmat.primitives.asymmetric import ec, ed448, rsa +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from cryptography.hazmat.primitives.serialization import ( BestAvailableEncryption, Encoding, NoEncryption, PrivateFormat, PublicFormat, + load_pem_private_key, load_pem_public_key, ) -from scm.errors import GitGrantExpired, GitInvalidGrant -from scm.git.grant import GRANT_AUDIENCE, public_key_of, require_public_keys, sign_grant, verify_grant +from scm.errors import GitGrantExpired, GitGrantKeyError, GitInvalidGrant +from scm.git.grant import ( + GRANT_AUDIENCE, + generate_private_key, + public_key_of, + require_private_key, + require_public_keys, + sign_grant, + verify_grant, +) from scm.git.types import GRANT_MAX_LENGTH, Grant from scm.secret import Secret from tests.unit.error_reports import assert_not_in_frames @@ -69,9 +79,31 @@ def assert_invalid(token: str, detail: str, public_keys: list[str] | None = None assert detail in exc_info.value.detail +def key_lines(pem: str) -> list[str]: + """The lines of base64 between a PEM's delimiters: the key material an error must not echo. + + Checked one by one, since an error could quote a key that spans several lines with or without its line breaks. + """ + return [line for line in pem.splitlines() if line and not line.startswith("-----")] + + def pem_body(pem: str) -> str: - """The base64 between a PEM's delimiters: the key material an error must not echo.""" - return "".join(line for line in pem.splitlines() if not line.startswith("-----")) + """The base64 between a PEM's delimiters, as one string.""" + return "".join(key_lines(pem)) + + +def assert_quotes_no_key(error: BaseException, pem: str) -> None: + """Neither the error's message nor that of the cryptography error it chains quotes the key.""" + for message in (str(error), str(error.__cause__)): + for line in key_lines(pem): + assert line not in message + + +def assert_key_not_in_frames(error: BaseException, private_key: Secret) -> None: + """No frame of the error's traceback shows the key. The caller's frame is in the traceback too, so it must hold the + key only as a Secret; the lines are revealed here, in a frame that is not.""" + for line in key_lines(private_key.reveal()): + assert_not_in_frames(error, line) class TestRoundTrip: @@ -113,6 +145,22 @@ def test_public_key_with_surrounding_whitespace(self): assert verify_grant([f"\n {KEY.public}\n\n"], token, now=NOW) == make_grant() +class TestGeneratePrivateKey: + def test_signs_grants_its_public_key_verifies(self): + private_key = generate_private_key() + token = sign_grant(private_key, make_grant(), now=NOW) + assert verify_grant([public_key_of(private_key)], token, now=NOW) == make_grant() + + def test_ed25519_private_key_in_pkcs8_pem(self): + # What openssl genpkey writes, so a generated key can go wherever a signer reads its key from. + pem = generate_private_key().reveal() + assert pem.startswith("-----BEGIN PRIVATE KEY-----\n") + assert isinstance(load_pem_private_key(pem.encode(), password=None), Ed25519PrivateKey) + + def test_new_key_each_time(self): + assert generate_private_key().reveal() != generate_private_key().reveal() + + class TestExpiry: def test_expired(self): with pytest.raises(GitGrantExpired, match="expired"): @@ -226,8 +274,10 @@ class TestLength: def test_too_long_to_sign(self): with pytest.raises(ValueError, match=f"over GRANT_MAX_LENGTH \\({GRANT_MAX_LENGTH}\\)") as exc_info: sign_grant(KEY.private, grant_with_repositories(100), now=NOW) + # The grant is at fault rather than the key, so the signer can tell the two apart. + assert not isinstance(exc_info.value, GitGrantKeyError) # The signer reports this error with its frames, which hold the private key only as a Secret and a key object. - assert_not_in_frames(exc_info.value, pem_body(KEY.private.reveal())) + assert_key_not_in_frames(exc_info.value, KEY.private) def test_longest_grant_that_fits_signs(self): # Add repositories until one more would not fit. @@ -266,42 +316,57 @@ def public_pem(key) -> str: class TestPrivateKey: - @pytest.fixture(params=[sign_grant, public_key_of], ids=["sign_grant", "public_key_of"]) + """A signing key that is not one is a mistake in the signer's configuration, raised as GitGrantKeyError.""" + + @pytest.fixture( + params=[sign_grant, public_key_of, require_private_key], ids=["sign_grant", "public_key_of", "require"] + ) def load(self, request) -> Callable[[Secret], object]: if request.param is sign_grant: return lambda private_key: sign_grant(private_key, make_grant(), now=NOW) - return public_key_of + return request.param + + def test_require_private_key_returns_it(self): + assert require_private_key(KEY.private) is KEY.private def test_empty(self, load): - with pytest.raises(ValueError, match="empty"): + with pytest.raises(GitGrantKeyError, match="^The git grant signing key is empty\\.$"): load(Secret("")) + def test_not_a_secret(self, load): + pem = KEY.private.reveal() + expected = r"^The git grant signing key must be an scm\.Secret, not str\." + with pytest.raises(GitGrantKeyError, match=expected) as exc_info: + load(pem) + assert_quotes_no_key(exc_info.value, pem) + @pytest.mark.parametrize( "pem", - [ - "not a key", - KEY.public, - KEY.key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption())[:80].decode(), - ], - ids=["garbage", "public-key", "truncated"], + ["not a key", KEY.public, KEY.private.reveal()[:80], pem_body(KEY.private.reveal())], + ids=["garbage", "public-key", "truncated", "no-delimiters"], ) def test_not_a_private_key(self, load, pem): - with pytest.raises(ValueError, match=r"not an unencrypted private key in PEM \(-----BEGIN PRIVATE KEY-----\)"): + expected = ( + r"^The git grant signing key is not an unencrypted private key in PEM \(-----BEGIN PRIVATE KEY-----\)" + ) + with pytest.raises(GitGrantKeyError, match=expected) as exc_info: load(Secret(pem)) + assert_quotes_no_key(exc_info.value, pem) def test_encrypted(self, load): pem = KEY.key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, BestAvailableEncryption(b"test-only")).decode() - with pytest.raises(ValueError, match="not an unencrypted private key") as exc_info: + with pytest.raises(GitGrantKeyError, match="not an unencrypted private key") as exc_info: load(Secret(pem)) assert "encrypted" in str(exc_info.value.__cause__) + assert_quotes_no_key(exc_info.value, pem) @pytest.mark.parametrize("key", other_key_types(), ids=["rsa", "ec", "ed448"]) def test_other_key_type(self, load, key): - pem = private_pem(key) - with pytest.raises(ValueError, match=f"must be an Ed25519 key, not {type(key).__name__}") as exc_info: - load(Secret(pem)) - assert pem_body(pem) not in str(exc_info.value) - assert_not_in_frames(exc_info.value, pem_body(pem)) + private_key = Secret(private_pem(key)) + with pytest.raises(GitGrantKeyError, match=f"must be an Ed25519 key, not {type(key).__name__}") as exc_info: + load(private_key) + assert_quotes_no_key(exc_info.value, private_key.reveal()) + assert_key_not_in_frames(exc_info.value, private_key) class TestPublicKeys: @@ -318,31 +383,43 @@ def test_require_public_keys_returns_them(self): assert require_public_keys((KEY.public, OTHER_KEY.public)) == [KEY.public, OTHER_KEY.public] def test_empty_list(self, load): - with pytest.raises(ValueError, match="no git grant public keys"): + # Under HS256, verify_grant refused every grant against an empty list; now the caller learns it has no keys. + with pytest.raises(GitGrantKeyError, match="no git grant public keys"): load([]) def test_private_key(self, load): pem = KEY.private.reveal() - with pytest.raises(ValueError, match=r"^public_keys\[1\] is a private key\.") as exc_info: + with pytest.raises(GitGrantKeyError, match=r"^public_keys\[1\] is a private key\.") as exc_info: load([OTHER_KEY.public, pem]) - assert pem_body(pem) not in str(exc_info.value) + assert_quotes_no_key(exc_info.value, pem) def test_private_key_as_a_secret(self, load): - with pytest.raises(TypeError, match=r"^public_keys\[0\] must be a PEM string, not Secret\."): + with pytest.raises(GitGrantKeyError, match=r"^public_keys\[0\] must be a PEM string, not Secret\."): load([KEY.private]) - @pytest.mark.parametrize("pem", ["", "not a key", KEY.public[:60]], ids=["empty", "garbage", "truncated"]) + def test_empty_key(self, load): + # As a list read from a setting with a trailing separator may have. + with pytest.raises(GitGrantKeyError, match=r"^public_keys\[1\] is not a public key in PEM"): + load([KEY.public, ""]) + + @pytest.mark.parametrize( + "pem", + ["not a key", OTHER_KEY.public[:60], pem_body(OTHER_KEY.public)], + ids=["garbage", "truncated", "no-delimiters"], + ) def test_not_a_public_key(self, load, pem): # One bad key fails the whole list rather than being skipped, so it is noticed before it is needed. - with pytest.raises(ValueError, match=r"^public_keys\[1\] is not a public key in PEM"): + with pytest.raises(GitGrantKeyError, match=r"^public_keys\[1\] is not a public key in PEM") as exc_info: load([KEY.public, pem]) + assert_quotes_no_key(exc_info.value, pem) @pytest.mark.parametrize("key", other_key_types(), ids=["rsa", "ec", "ed448"]) def test_other_key_type(self, load, key): pem = public_pem(key) expected = f"^public_keys\\[0\\] must be an Ed25519 key, not {type(key.public_key()).__name__}" - with pytest.raises(ValueError, match=expected): + with pytest.raises(GitGrantKeyError, match=expected) as exc_info: load([pem]) + assert_quotes_no_key(exc_info.value, pem) class TestRotatedKeys: diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 220cf47..1523931 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, GitRemoteRefused, RateLimitExceeded +from scm.errors import ClientClosedRequest, GitGrantKeyError, GitRemoteRefused, RateLimitExceeded from scm.git.grant import sign_grant from scm.git.relay import GitRelay, RelayResponse, ResponseBody from scm.git.server import _STATUS, GitServer @@ -141,18 +141,19 @@ def test_requires_a_relay(): @pytest.mark.parametrize( - "public_keys,error,message", + "public_keys,message", [ - ([], ValueError, "no git grant public keys"), - ([KEY.public, "not a key"], ValueError, r"^public_keys\[1\] is not a public key in PEM"), - ([KEY.private.reveal()], ValueError, r"^public_keys\[0\] is a private key\."), - ([KEY.private], TypeError, r"^public_keys\[0\] must be a PEM string, not Secret\."), + ([], "no git grant public keys"), + ([KEY.public, "not a key"], r"^public_keys\[1\] is not a public key in PEM"), + ([KEY.private.reveal()], r"^public_keys\[0\] is a private key\."), + ([KEY.private], r"^public_keys\[0\] must be a PEM string, not Secret\."), ], ids=["empty", "not-a-key", "private-key", "secret"], ) -def test_requires_public_keys(public_keys, error, message): - # Checked when the server is built, since no grant could verify against them. - with pytest.raises(error, match=message): +def test_requires_public_keys(public_keys, message): + # Checked when the server is built, since no grant could verify against them. Not a coded error, so a caller + # that answers those as refusals does not blame the client for the server's keys. + with pytest.raises(GitGrantKeyError, match=message): GitServer( public_keys=public_keys, fetch_repository=lambda organization_id, repository_id: None, From 7806db6aba92bee8792c0ea8826e5fe97c5778fd Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 09:13:52 +0100 Subject: [PATCH 2/3] fix(git): Answer permanent git failures with a 500, not a 502 A remote that isn't https, a host the relay's adapter refuses, and a TLS certificate Sentry doesn't trust are misconfigurations of the integration or the host, and no retry fixes them. As 502s, Seer's checkout retried each one for about a minute before failing, because it retries a 502 or a 503. As 500s, it fails at once, like the other misconfigurations already mapped to 500. The 502s that stay mean the provider failed or Sentry couldn't get or use a token. 503 and 504 still mean a retry can succeed. GitServer already answered a coded error from get_token by its code, but nothing said so. Sentry now relies on it to turn an unreachable control silo into a 503 that Seer retries, so GetToken and GitServer document it and a test pins it. Co-Authored-By: Claude Opus 5.5 --- src/scm/git/server.py | 20 ++++++----- src/scm/git/types.py | 6 +++- tests/unit/test_git_server.py | 62 ++++++++++++++++++++++++++++++++--- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/scm/git/server.py b/src/scm/git/server.py index a463baa..ce56cb2 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -28,16 +28,18 @@ "provider_not_found": 404, "git_provider_not_supported": 404, "client_closed_request": 499, - # Sentry's record of the repository is incomplete. + # Misconfigured: Sentry's record of the repository is incomplete, or the provider's host is one the relay does + # not send a token to. No retry can succeed, so these are not a 502 or a 503, which a client retries. "malformed_external_id": 500, "missing_repository_path": 500, - # The provider cannot be reached safely, or failed. - "git_remote_not_https": 502, - "git_remote_refused": 502, + "git_remote_not_https": 500, + "git_remote_refused": 500, + "resource_certificate_invalid": 500, + # The provider failed, or Sentry could not get or use a token for it. "git_token_rejected": 502, "git_token_unavailable": 502, "resource_bad_gateway": 502, - "resource_certificate_invalid": 502, + # Temporary, so a retry can succeed. "resource_service_unavailable": 503, "resource_gateway_timeout": 504, } @@ -64,9 +66,11 @@ class GitServer: message leaves out where the request was going, so ``emit_error`` also gets what the request had resolved 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 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. + before it was known. A client closing its request is not reported, since there is nothing to fix. A coded error + that ``get_token`` raises is answered the same way, so the token supplier can say how it failed: a + ``ResourceServiceUnavailable`` becomes a 503, which the client retries. Any other exception, such as an error of + the token supplier that is not coded, or one 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/src/scm/git/types.py b/src/scm/git/types.py index fccfd38..66239d0 100644 --- a/src/scm/git/types.py +++ b/src/scm/git/types.py @@ -53,7 +53,11 @@ class Grant(msgspec.Struct, frozen=True, kw_only=True, forbid_unknown_fields=Tru """Return a provider token that can run ``operation`` on ``repository``, or None if there is none. Wrap the token in a ``Secret`` where you read it. GitServer and the relay hold it in their frames' local variables, -which an error report sends.""" +which an error report sends. + +It may raise an ``SCMCodedError`` to say why there is no token. GitServer answers it as it answers its own +refusals, with the status for its code, or a 500 for a code it has none for. A ``ResourceServiceUnavailable`` for a +failure that may pass becomes a 503, which the client retries. Any other exception reaches GitServer's caller.""" class Readable(Protocol): diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 1523931..b6a5e79 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -7,7 +7,13 @@ import pytest -from scm.errors import ClientClosedRequest, GitGrantKeyError, GitRemoteRefused, RateLimitExceeded +from scm.errors import ( + ClientClosedRequest, + GitGrantKeyError, + GitRemoteRefused, + RateLimitExceeded, + ResourceServiceUnavailable, +) from scm.git.grant import sign_grant from scm.git.relay import GitRelay, RelayResponse, ResponseBody from scm.git.server import _STATUS, GitServer @@ -29,9 +35,12 @@ def signed_grant(**overrides) -> str: class Recorder: - def __init__(self, repository: Repository | None = None, token: Secret | None = TOKEN) -> None: + def __init__( + self, repository: Repository | None = None, token: Secret | None = TOKEN, token_error: Exception | None = None + ) -> None: self.repository = make_repository() if repository is None else repository self.token = token + self.token_error = token_error self.repository_lookups: list[tuple[int, RepositoryId]] = [] self.provider_lookups: list[tuple[int, Repository]] = [] self.token_lookups: list[tuple[Repository, GitOperation]] = [] @@ -49,6 +58,8 @@ def fetch_provider(self, organization_id: int, repository: Repository) -> Provid def get_token(self, repository: Repository, operation: GitOperation) -> Secret | None: self.token_lookups.append((repository, operation)) + if self.token_error is not None: + raise self.token_error return self.token def record_count(self, name: str, value: int, tags: dict[str, str]) -> None: @@ -389,7 +400,8 @@ def test_plain_http_remote(self): repository=make_repository(provider_name="github_enterprise", web_base_url="http://127.0.0.1:1") ) response = request(make_server(recorder, GitRelay()), signed_grant()) - assert_refused(response, 502, "not an https URL") + # Retrying cannot change the remote's scheme, so this is the server's error rather than a 502. + assert_refused(response, 500, "not an https URL") assert recorder.counts == [ ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "git_remote_not_https"}) ] @@ -404,12 +416,54 @@ def test_adapter_refusing_the_host(self): 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_refused(response, 500, "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_untrusted_certificate(self): + # A self-managed instance whose certificate Sentry does not trust. Retrying cannot fix it. + with running_upstream() as upstream: + recorder = Recorder( + repository=make_repository(provider_name="github_enterprise", web_base_url=upstream.url) + ) + response = request(make_server(recorder, GitRelay()), signed_grant()) + assert_refused(response, 500, "TLS certificate") + assert recorder.counts == [ + ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "resource_certificate_invalid"}) + ] + assert upstream.requests == [] + + def test_token_supplier_raising_a_coded_error(self): + # e.g. Sentry failing to reach the control silo for a token: the supplier says the failure may pass, and + # GitServer answers with a 503, which the client retries. + recorder = Recorder(token_error=ResourceServiceUnavailable()) + server = make_server(recorder) + assert_refused(request(server, signed_grant()), 503, "temporarily unavailable") + server.relay.forward.assert_not_called() # type: ignore[attr-defined] + assert recorder.counts == [ + ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "resource_service_unavailable"}) + ] + assert recorder.error_codes == ["resource_service_unavailable"] + assert recorder.errors[0][1] == { + "repository_id": "5", + "organization_id": "1", + "referrer": REFERRER, + "operation": "fetch", + "provider": "github", + "remote_host": "github.com", + } + + def test_token_supplier_raising_another_error(self): + recorder = Recorder(token_error=ConnectionError("control silo unreachable")) + with pytest.raises(ConnectionError): + request(make_server(recorder), signed_grant()) + assert recorder.counts == [ + ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "unhandled_exception"}) + ] + assert recorder.errors == [] + def test_provider_rejecting_the_token(self): # Passed on, a 401 would make git ask the sandboxed client for credentials. adapters: list[ClosingAdapter] = [] From 296e79cc6b6c6fb3052526d12d236dce3e604745 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 09:36:22 +0100 Subject: [PATCH 3/3] docs(git): Say GitServer reports a coded error from get_token Right after "a client closing its request is not reported", "answered the same way" read as "not reported either". GitServer does report it, and the test checks that. Also drops a test comment about the HS256 behavior, which the 3.0.0 release notes record. Co-Authored-By: Claude Opus 5.5 --- src/scm/git/server.py | 2 +- tests/unit/test_git_grant.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/scm/git/server.py b/src/scm/git/server.py index ce56cb2..101a6bf 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -67,7 +67,7 @@ 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. A coded error - that ``get_token`` raises is answered the same way, so the token supplier can say how it failed: a + that ``get_token`` raises is answered and reported like a refusal, so the token supplier can say how it failed: a ``ResourceServiceUnavailable`` becomes a 503, which the client retries. Any other exception, such as an error of the token supplier that is not coded, or one 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. diff --git a/tests/unit/test_git_grant.py b/tests/unit/test_git_grant.py index 3c0f8da..f939281 100644 --- a/tests/unit/test_git_grant.py +++ b/tests/unit/test_git_grant.py @@ -383,7 +383,6 @@ def test_require_public_keys_returns_them(self): assert require_public_keys((KEY.public, OTHER_KEY.public)) == [KEY.public, OTHER_KEY.public] def test_empty_list(self, load): - # Under HS256, verify_grant refused every grant against an empty list; now the caller learns it has no keys. with pytest.raises(GitGrantKeyError, match="no git grant public keys"): load([])