feat(git)!: Tell misconfiguration apart from bad grants and transient failures

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

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)

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."""
 

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
 
 

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,
 }
@@ -47,7 +49,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
@@ -62,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 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.
 
     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.

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

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

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"),