diff --git a/bin/git-server b/bin/git-server index efe23c2..22962e1 100755 --- a/bin/git-server +++ b/bin/git-server @@ -91,6 +91,7 @@ from pathlib import Path from types import ModuleType from urllib.parse import urlsplit +from scm import Secret from scm.git.grant import sign_grant from scm.git.relay import GitRelay from scm.git.server import GitServer @@ -220,7 +221,7 @@ def _require(**values: str | None) -> None: def make_git_server( backend: Backend, - secret: str, + secret: Secret, ca_bundle: str | None, get_token: GetToken | None = None, repository: Repository | None = None, @@ -250,7 +251,7 @@ def make_git_server( ) -def grant_for(secret: str, operation: GitOperation, repository_id: RepositoryId) -> str: +def grant_for(secret: Secret, operation: GitOperation, repository_id: RepositoryId) -> str: grant = Grant( organization_id=ORGANIZATION_ID, referrer=REFERRER, @@ -564,7 +565,7 @@ def main() -> None: creds = load_credentials() args.repository = resolve("SCM_GIT_REPOSITORY", args.repository, creds) # The server prints grants for its repository, so nothing else needs to know a secret it makes up. - args.secret = resolve("SCM_GIT_GRANT_SECRET", args.secret, creds) or secrets.token_hex(32) + args.secret = Secret(resolve("SCM_GIT_GRANT_SECRET", args.secret, creds) or secrets.token_hex(32)) args.ca_bundle = resolve("SCM_GIT_CA_BUNDLE", args.ca_bundle, creds) _require(SCM_GIT_REPOSITORY=args.repository) path = args.repository.strip("/").removesuffix(".git") diff --git a/bin/github-client b/bin/github-client index 38ce0da..6a36f21 100755 --- a/bin/github-client +++ b/bin/github-client @@ -95,6 +95,7 @@ import pathlib import sys from datetime import date +from scm import Secret from scm.manager import SourceCodeManager from scm.types import ( CheckRunOutput, @@ -522,7 +523,7 @@ def main() -> None: args.org_id, ("github", args.repo), base_url=args.base_url if args.base_url else "http://127.0.0.1:8080", - signing_secret=args.secret if args.secret else "secret", + signing_secret=Secret(args.secret if args.secret else "secret"), ) if args.command == "get-pull-request": diff --git a/bin/github-server b/bin/github-server index a24ff6a..1f1cf0f 100755 --- a/bin/github-server +++ b/bin/github-server @@ -53,6 +53,7 @@ from typing import Any import jwt import requests +from scm import Secret from scm.providers.github.provider import GitHubProvider from scm.rpc.server import RpcServer from scm.types import CredentialsSet, Provider, Repository, RepositoryId @@ -298,7 +299,7 @@ def main(): app_id = resolve("GITHUB_APP_ID", args.app_id, creds) private_key_path = resolve("GITHUB_PRIVATE_KEY_PATH", args.private_key, creds) installation_id = resolve("GITHUB_INSTALLATION_ID", args.installation_id, creds) - secret = resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret" + secret = Secret(resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret") base_url = resolve("GITHUB_BASE_URL", args.base_url, creds) or GITHUB_API_BASE web_base_url = resolve("GITHUB_WEB_BASE_URL", args.web_base_url, creds) or _derive_web_base_url(base_url) provider_name = resolve("GITHUB_PROVIDER_NAME", args.provider_name, creds) or _derive_provider_name(base_url) diff --git a/bin/gitlab-client b/bin/gitlab-client index 4a5a52f..60a0f1c 100755 --- a/bin/gitlab-client +++ b/bin/gitlab-client @@ -89,6 +89,7 @@ import pathlib import sys from datetime import date +from scm import Secret from scm.manager import SourceCodeManager from scm.types import ( CheckRunOutput, @@ -415,7 +416,7 @@ def main(): kwargs = { "base_url": args.base_url if args.base_url else "http://127.0.0.1:8080", - "signing_secret": args.secret if args.secret else "secret", + "signing_secret": Secret(args.secret if args.secret else "secret"), } scm = SourceCodeManager.make_proxy_client( diff --git a/bin/gitlab-server b/bin/gitlab-server index 9a6f871..c2266ba 100755 --- a/bin/gitlab-server +++ b/bin/gitlab-server @@ -34,6 +34,7 @@ from urllib.parse import urlparse import requests +from scm import Secret from scm.providers.gitlab.provider import API_VERSION, GitLabProvider from scm.rpc.server import RpcServer from scm.types import CredentialsSet, Provider, Repository, RepositoryId @@ -192,7 +193,7 @@ def main(): access_token = resolve("GITLAB_ACCESS_TOKEN", args.access_token, creds) base_url = resolve("GITLAB_BASE_URL", args.base_url, creds) or "https://gitlab.com" - secret = resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret" + secret = Secret(resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret") if not access_token: logger.error("Error: missing required config: GITLAB_ACCESS_TOKEN") diff --git a/src/scm/__init__.py b/src/scm/__init__.py index e69de29..aaed280 100644 --- a/src/scm/__init__.py +++ b/src/scm/__init__.py @@ -0,0 +1,3 @@ +from scm.secret import Secret + +__all__ = ("Secret",) diff --git a/src/scm/git/grant.py b/src/scm/git/grant.py index 89586d2..20b5470 100644 --- a/src/scm/git/grant.py +++ b/src/scm/git/grant.py @@ -9,6 +9,7 @@ import hashlib import time +from collections.abc import Sequence from typing import Any import jwt @@ -16,6 +17,7 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.types import GRANT_MAX_LENGTH, Grant +from scm.secret import Secret GRANT_AUDIENCE = "scm-git-grant" _ALGORITHM = "HS256" @@ -23,7 +25,7 @@ _JWT_CLAIMS = frozenset({"aud", "exp", "iat"}) -def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: +def sign_grant(secret: Secret, grant: Grant, *, now: float | None = None) -> str: # HMAC accepts an empty key, so an empty secret, say from an unset setting, would let anyone sign a grant. if not secret: raise ValueError("A git grant secret is empty.") @@ -31,7 +33,7 @@ def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: claims["exp"] = claims.pop("expires_at") claims["iat"] = int(time.time() if now is None else now) claims["aud"] = GRANT_AUDIENCE - token = jwt.encode(claims, secret, algorithm=_ALGORITHM, headers={"kid": _key_id(secret)}) + token = jwt.encode(claims, secret.reveal(), algorithm=_ALGORITHM, headers={"kid": _key_id(secret)}) # Refused here, the signer's tests catch a grant that is too long, rather than the sandbox router in production. if len(token) > GRANT_MAX_LENGTH: raise ValueError( @@ -41,7 +43,7 @@ def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: return token -def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> Grant: +def verify_grant(secrets: Sequence[Secret], token: str, *, now: float | None = None) -> Grant: """Return the grant if one of ``secrets`` signed it and it has not expired. Verification takes a list of secrets, as ``RpcServer`` does, so a new secret can be added before signers @@ -64,7 +66,7 @@ def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> try: claims = jwt.decode( token, - secret, + secret.reveal(), algorithms=[_ALGORITHM], audience=GRANT_AUDIENCE, # exp is checked below against an injectable clock, so tests can pin it. PyJWT checks iat, which it @@ -97,6 +99,6 @@ def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> return grant -def _key_id(secret: str) -> str: +def _key_id(secret: Secret) -> str: # The first 8 hex characters of the secret's SHA-256, the same key id Sentry's viewer contexts use. - return hashlib.sha256(secret.encode()).hexdigest()[:8] + return hashlib.sha256(secret.reveal().encode()).hexdigest()[:8] diff --git a/src/scm/git/server.py b/src/scm/git/server.py index ba7a851..c8bdd0c 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -1,14 +1,13 @@ -from collections.abc import Callable, Mapping +from collections.abc import Callable, Mapping, Sequence from urllib.parse import urlsplit -from requests.structures import CaseInsensitiveDict - from scm.errors import ErrorCode, GitProviderNotSupported, GitTokenRejected, GitTokenUnavailable, SCMCodedError from scm.git.grant import verify_grant from scm.git.relay import GitRelay, ResponseBody, authorize_request from scm.git.types import GRANT_HEADER, GetToken, Readable from scm.helpers import initialize_provider from scm.rpc.types import StreamResponse +from scm.secret import Secret, require_secrets from scm.types import GetGitRemoteProtocol, Provider, Repository, RepositoryId # The status of each refusal GitServer answers itself. None is a 401: on a 401 git asks the user for credentials, @@ -75,7 +74,7 @@ class GitServer: def __init__( self, - secrets: list[str], + secrets: Sequence[Secret], fetch_repository: Callable[[int, RepositoryId], Repository | None], fetch_provider: Callable[[int, Repository], Provider | None], get_token: GetToken, @@ -86,7 +85,7 @@ def __init__( # Fail when the server is built rather than on every request, because no grant could ever verify. if not secrets or not all(secrets): raise ValueError("GitServer needs at least one secret, and none may be empty.") - self.secrets = secrets + self.secrets = require_secrets(secrets, parameter="secrets") self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.get_token = get_token @@ -112,11 +111,14 @@ def handle( body as empty, and git sends a push over about 1 MiB chunked, so a Django view needs to pass a stream that has the whole body. """ + # Rebound without the grant, so it shows as [Filtered] in the frames of a reported error rather than in + # ``headers``, here and in the relay, which never sends it upstream anyway. + grant_token, headers = _take_grant(headers) counted_body = _CountingReader(body) if body is not None else None context = {"repository_id": str(repository_id)} referrer_tag: dict[str, str] = {} try: - grant = verify_grant(self.secrets, CaseInsensitiveDict(headers).get(GRANT_HEADER, "")) + grant = verify_grant(self.secrets, grant_token.reveal()) context["organization_id"] = str(grant.organization_id) referrer_tag["referrer"] = context["referrer"] = grant.referrer operation = authorize_request(grant, repository_id, method, route, query) @@ -181,6 +183,18 @@ def finish(failure: str | None, bytes_sent: int) -> None: ) +def _take_grant(headers: Mapping[str, str]) -> tuple[Secret, dict[str, str]]: + """Split the grant, empty if there is none, from the other headers. Header names are case-insensitive.""" + grant = Secret("") + rest: dict[str, str] = {} + for name, value in headers.items(): + if name.lower() == GRANT_HEADER.lower(): + grant = Secret(value) + else: + rest[name] = value + return grant, rest + + class _CountingReader: """The client's request body, counting the bytes the relay reads from it.""" diff --git a/src/scm/manager.py b/src/scm/manager.py index 1c882fd..014550c 100644 --- a/src/scm/manager.py +++ b/src/scm/manager.py @@ -10,6 +10,7 @@ ) from scm.rpc.client import fetch_provider as fetch_proxy_provider from scm.rpc.client import fetch_repository as fetch_proxy_repository +from scm.secret import Secret from scm.types import Provider, Referrer, Repository, RepositoryId @@ -44,7 +45,7 @@ def make_proxy_client( *, referrer: Referrer = "shared", base_url: str, - signing_secret: str, + signing_secret: Secret, record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, retry: RetryConfig | None = None, ): diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index 831a880..f67b9b5 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -17,6 +17,7 @@ from scm.providers.gitlab.provider import GitLabProvider from scm.rpc.helpers import deserialize_repository, sign_get, sign_post from scm.rpc.types import ActionAttributes, ActionRequest, ErrorResponse +from scm.secret import Secret from scm.types import ApiClient, CredentialsSet, Provider, Repository, RepositoryId SCM_API_URL = "{base_url}/api/0/internal/scm-rpc/" @@ -73,7 +74,7 @@ def post(self, url: str, data: bytes, headers: dict[str, str], stream: bool = Fa def fetch_repository( url: str, - signing_secret: str, + signing_secret: Secret, organization_id: int, repository_id: RepositoryId, session: Callable[[], Session] = lambda: RequestsSession(), @@ -154,7 +155,7 @@ class RpcApiClient(ApiClient): def __init__( self, full_url: str, - signing_secret: str, + signing_secret: Secret, organization_id: int, referrer: str, repository_id: RepositoryId, diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index 33f5b44..c91793f 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -1,35 +1,39 @@ import hashlib import hmac +from collections.abc import Sequence from typing import Literal import msgspec from scm.errors import RepositoryCouldNotBeDeserialized from scm.rpc.types import JsonApiData, JsonApiPayload, RepositoryAttributes +from scm.secret import Secret from scm.types import Repository, RepositoryId -def sign_get(secret: str, organization_id: int, repository_id: RepositoryId) -> str: +def sign_get(secret: Secret, organization_id: int, repository_id: RepositoryId) -> str: return sign(secret, f"{organization_id}{repository_id}".encode()) -def sign_post(secret: str, serialized_body: bytes) -> str: +def sign_post(secret: Secret, serialized_body: bytes) -> str: return sign(secret, serialized_body) -def sign(secret: str, message: bytes) -> str: - return f"rpc0:{hmac.new(secret.encode(), message, hashlib.sha256).hexdigest()}" +def sign(secret: Secret, message: bytes) -> str: + return f"rpc0:{hmac.new(secret.reveal().encode(), message, hashlib.sha256).hexdigest()}" -def verify_get(secrets: list[str], organization_id: int, repository_id: RepositoryId, received_signature: str) -> bool: +def verify_get( + secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: str +) -> bool: return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature) -def verify_post(secrets: list[str], serialized_body: bytes, received_signature: str) -> bool: +def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: str) -> bool: return verify(secrets, serialized_body, received_signature) -def verify(secrets: list[str], message: bytes, received_signature: str) -> bool: +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) diff --git a/src/scm/rpc/server.py b/src/scm/rpc/server.py index f7b9a78..38168ba 100644 --- a/src/scm/rpc/server.py +++ b/src/scm/rpc/server.py @@ -1,4 +1,4 @@ -from collections.abc import Callable, Iterator, Mapping, MutableMapping +from collections.abc import Callable, Iterator, Mapping, MutableMapping, Sequence from urllib.parse import urlparse import msgspec @@ -17,6 +17,7 @@ from scm.rpc.errors import serialize_error from scm.rpc.helpers import serialize_repository, verify_get, verify_post from scm.rpc.types import ActionRequest, Response, StreamResponse +from scm.secret import Secret, require_secrets from scm.types import Provider, Repository, RepositoryId TEN_MEGABYTES = 10 * 1024 * 1024 @@ -26,13 +27,13 @@ class RpcServer: def __init__( self, - secrets: list[str], + secrets: Sequence[Secret], fetch_repository: Callable[[int, RepositoryId], Repository | None], fetch_provider: Callable[[int, Repository], Provider | None], record_count: Callable[[str, int, dict[str, str]], None], emit_error: Callable[[Exception], None], ) -> None: - self.secrets = secrets + self.secrets = require_secrets(secrets, parameter="secrets") self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.record_count = record_count diff --git a/src/scm/secret.py b/src/scm/secret.py new file mode 100644 index 0000000..5ce9fb9 --- /dev/null +++ b/src/scm/secret.py @@ -0,0 +1,53 @@ +from collections.abc import Sequence + + +class Secret: + """A string that must not reach a log line or an error report, such as a signing secret. + + Sentry's SDK sends the local variables of every frame in a reported traceback, and its default scrubbing only + replaces variables with a few exact names (``secret`` is one, ``secrets`` and ``signing_secret`` are not), never + looking inside a list or a dict. Held in a ``Secret``, the value shows as ``[Filtered]`` wherever it is: the SDK + calls ``__sentry_repr__`` first, and ``repr()`` and ``str()`` hide it too. + + ``reveal()`` returns the plain string, which is then in the frame that called it. Call it where the value is used, + inline, rather than keeping the result in a variable. + """ + + __slots__ = ("_value",) + + def __init__(self, value: str) -> None: + self._value = value + + def reveal(self) -> str: + return self._value + + def __repr__(self) -> str: + return "Secret('[Filtered]')" + + def __str__(self) -> str: + return "[Filtered]" + + def __sentry_repr__(self) -> str: + return "[Filtered]" + + def __bool__(self) -> bool: + # False for an empty secret, as for an empty string, so a check such as ``if not secret`` still refuses one. + return bool(self._value) + + +def require_secrets(values: Sequence[object], *, parameter: str) -> list[Secret]: + """Return ``values`` as a list, if each is a ``Secret``. + + For the servers, which hold their secrets for every request. A plain string that got past the type checker, say + from untyped code, would make every request fail inside verification, and each failure's error report would + carry the strings in its frames. Refused here, the server fails once, when it is built. + """ + secrets: list[Secret] = [] + for value in values: + if not isinstance(value, Secret): + raise TypeError( + f"{parameter} must be scm.Secret objects, not {type(value).__name__}. " + 'Wrap each in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).' + ) + secrets.append(value) + return secrets diff --git a/tests/unit/error_reports.py b/tests/unit/error_reports.py new file mode 100644 index 0000000..1bb905e --- /dev/null +++ b/tests/unit/error_reports.py @@ -0,0 +1,12 @@ +def assert_not_in_frames(error: BaseException, value: str) -> None: + """Fail if any frame of the error's traceback holds ``value`` where an error report would show it. + + Sentry's SDK sends every frame's local variables, using ``__sentry_repr__`` or else ``repr()`` for each, and its + default scrubbing only replaces variables with a few exact names, such as ``secret``. + """ + 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}" + tb = tb.tb_next diff --git a/tests/unit/test_git_grant.py b/tests/unit/test_git_grant.py index cea9fd3..db308f0 100644 --- a/tests/unit/test_git_grant.py +++ b/tests/unit/test_git_grant.py @@ -9,18 +9,20 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.grant import GRANT_AUDIENCE, sign_grant, verify_grant from scm.git.types import GRANT_MAX_LENGTH, Grant +from scm.secret import Secret from tests.unit.git_builders import FAR_FUTURE, make_grant # Before the real time: PyJWT checks iat against the real clock, so a grant must not be issued after it. NOW = 1_700_000_000 # HS256 secrets of at least 32 bytes, as PyJWT recommends. -SECRET = "test-secret-0000000000000000000000" -OLD_SECRET = "test-secret-old-000000000000000000" -NEW_SECRET = "test-secret-new-000000000000000000" +SECRET = Secret("test-secret-0000000000000000000000") +OLD_SECRET = Secret("test-secret-old-000000000000000000") +NEW_SECRET = Secret("test-secret-new-000000000000000000") +OTHER_SECRET = Secret("other-secret-0000000000000000000000") -def key_id(secret: str) -> str: - return hashlib.sha256(secret.encode()).hexdigest()[:8] +def key_id(secret: Secret) -> str: + return hashlib.sha256(secret.reveal().encode()).hexdigest()[:8] def claims_of(grant: Grant) -> dict: @@ -29,12 +31,12 @@ def claims_of(grant: Grant) -> dict: return claims | {"iat": NOW, "aud": GRANT_AUDIENCE} -def sign_claims(claims: dict, secret: str = SECRET, algorithm: str = "HS256", kid: str | None = None) -> str: +def sign_claims(claims: dict, secret: Secret = SECRET, algorithm: str = "HS256", kid: str | None = None) -> str: """Sign arbitrary claims the way sign_grant does, to reach the checks that run after the signature.""" - return jwt.encode(claims, secret, algorithm=algorithm, headers={"kid": kid or key_id(secret)}) + return jwt.encode(claims, secret.reveal(), algorithm=algorithm, headers={"kid": kid or key_id(secret)}) -def assert_invalid(token: str, detail: str, secrets: list[str] | None = None) -> None: +def assert_invalid(token: str, detail: str, secrets: list[Secret] | None = None) -> None: with pytest.raises(GitInvalidGrant) as exc_info: verify_grant(secrets or [SECRET], token, now=NOW) assert detail in exc_info.value.detail @@ -75,7 +77,7 @@ def test_expires_at_its_expiry_time(self): def test_forged_grant_is_invalid_even_when_expired(self): # Expiry is checked only once the signature matches, so an expired grant is always a genuine one. - token = sign_grant("other-secret-0000000000000000000000", make_grant(expires_at=NOW - 1), now=NOW) + token = sign_grant(OTHER_SECRET, make_grant(expires_at=NOW - 1), now=NOW) assert_invalid(token, "not signed with a known secret") def test_valid_until_its_expiry_time(self): @@ -118,12 +120,10 @@ def test_changed_signature(self): assert_invalid(f"{header}.{payload}.{resigned}", "signature does not match") def test_unknown_secret(self): - assert_invalid( - sign_grant("other-secret-0000000000000000000000", make_grant(), now=NOW), "not signed with a known" - ) + assert_invalid(sign_grant(OTHER_SECRET, make_grant(), now=NOW), "not signed with a known") def test_other_secret_claiming_a_known_key_id(self): - token = sign_claims(claims_of(make_grant()), secret="other-secret-0000000000000000000000", kid=key_id(SECRET)) + token = sign_claims(claims_of(make_grant()), secret=OTHER_SECRET, kid=key_id(SECRET)) assert_invalid(token, "signature does not match") def test_no_secrets(self): @@ -179,13 +179,13 @@ class TestSecrets: # PyJWT signs with an empty key, so an empty secret would let anyone sign grants. def test_refuses_to_sign_with_an_empty_secret(self): with pytest.raises(ValueError, match="empty"): - sign_grant("", make_grant(), now=NOW) + sign_grant(Secret(""), make_grant(), now=NOW) @pytest.mark.filterwarnings("ignore::jwt.warnings.InsecureKeyLengthWarning") def test_refuses_to_verify_with_an_empty_secret(self): - token = jwt.encode(claims_of(make_grant()), "", algorithm="HS256", headers={"kid": key_id("")}) + token = jwt.encode(claims_of(make_grant()), "", algorithm="HS256", headers={"kid": key_id(Secret(""))}) with pytest.raises(ValueError, match="empty"): - verify_grant([SECRET, ""], token, now=NOW) + verify_grant([SECRET, Secret("")], token, now=NOW) class TestRotatedSecrets: diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 332221c..bdf6747 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -13,11 +13,13 @@ from scm.git.server import _STATUS, GitServer from scm.git.types import GRANT_HEADER, GitOperation, Readable from scm.rpc.types import StreamResponse +from scm.secret import Secret from scm.types import GitRemote, Provider, Repository, RepositoryId +from tests.unit.error_reports import assert_not_in_frames from tests.unit.git_builders import make_grant, make_provider, make_repository from tests.unit.git_upstream import ClosingAdapter, basic_auth, running_upstream -SECRET = "test-secret-0000000000000000000000" +SECRET = Secret("test-secret-0000000000000000000000") TOKEN = "ghs_secrettoken" REFERRER = make_grant().referrer @@ -138,7 +140,7 @@ def test_requires_a_relay(): ) -@pytest.mark.parametrize("secrets", [[], [SECRET, ""]]) +@pytest.mark.parametrize("secrets", [[], [SECRET, Secret("")]]) def test_requires_non_empty_secrets(secrets): with pytest.raises(ValueError, match="secret"): GitServer( @@ -152,6 +154,19 @@ def test_requires_non_empty_secrets(secrets): ) +def test_requires_secret_objects(): + with pytest.raises(TypeError, match=r"^secrets must be scm\.Secret objects, not str\."): + GitServer( + secrets=[SECRET.reveal()], # type: ignore[list-item] + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + get_token=lambda repository, operation: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error, context: None, + relay=MagicMock(spec=GitRelay), + ) + + class TestRefusedBeforeTheProvider: """Nothing the grant does not allow may look up a repository or a token.""" @@ -161,7 +176,7 @@ class TestRefusedBeforeTheProvider: (None, "missing", "git_invalid_grant"), ("garbage", "malformed", "git_invalid_grant"), ( - sign_grant("other-secret-0000000000000000000000", make_grant()), + sign_grant(Secret("other-secret-0000000000000000000000"), make_grant()), "not signed with a known secret", "git_invalid_grant", ), @@ -272,8 +287,7 @@ def test_token_asked_for_the_grants_operation(self): def test_forwards_with_remote_and_token(self): recorder = Recorder() server = make_server(recorder) - grant = signed_grant() - response = request(server, grant, headers={"Git-Protocol": "version=2"}) + response = request(server, signed_grant(), headers={"Git-Protocol": "version=2"}) assert b"".join(response.content) == b"refs" server.relay.forward.assert_called_once_with( # type: ignore[attr-defined] GitRemote(url="https://github.com/getsentry/sentry.git", basic_auth_username="x-access-token"), @@ -281,7 +295,8 @@ def test_forwards_with_remote_and_token(self): method="GET", route="info/refs", query="service=git-upload-pack", - headers={GRANT_HEADER: grant, "Git-Protocol": "version=2"}, + # Without the grant, which the relay has no use for and would otherwise hold in its frame. + headers={"Git-Protocol": "version=2"}, body=None, ) assert recorder.counts == [ @@ -508,3 +523,27 @@ def test_relays_end_to_end(self): assert received.path == "/getsentry/sentry.git/info/refs?service=git-upload-pack" assert received.headers["Authorization"] == basic_auth("x-access-token", TOKEN) assert GRANT_HEADER not in received.headers + + +class TestKeptOutOfErrorReports: + """What GitServer passes to emit_error is reported to Sentry, with every frame's local variables.""" + + def test_refused_grant_leaves_out_the_signing_secret(self): + recorder = Recorder() + request(make_server(recorder), sign_grant(Secret("other-secret-0000000000000000000000"), make_grant())) + [(error, _)] = recorder.errors + assert error.code == "git_invalid_grant" # type: ignore[attr-defined] + assert_not_in_frames(error, SECRET.reveal()) + + @pytest.mark.parametrize("header", [GRANT_HEADER, GRANT_HEADER.lower()]) + def test_relay_failure_leaves_out_the_grant_and_the_signing_secret(self, header): + recorder = Recorder( + repository=make_repository(provider_name="github_enterprise", web_base_url="https://127.0.0.1:1") + ) + grant = signed_grant() + request(make_server(recorder, GitRelay(timeout=(1, 1))), None, headers={header: grant}) + [(error, _)] = recorder.errors + # Failed after the grant check, in the relay. + assert error.code != "git_invalid_grant" # type: ignore[attr-defined] + assert_not_in_frames(error, grant) + assert_not_in_frames(error, SECRET.reveal()) diff --git a/tests/unit/test_rpc_client.py b/tests/unit/test_rpc_client.py index 94b4e9f..3e4c394 100644 --- a/tests/unit/test_rpc_client.py +++ b/tests/unit/test_rpc_client.py @@ -18,6 +18,7 @@ ) from scm.rpc.server import serialize_repository from scm.rpc.types import Error, ErrorResponse +from scm.secret import Secret def make_repository(**overrides): @@ -62,7 +63,7 @@ class TestFetchRepository: @patch("scm.rpc.client.requests.get") def test_success(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - repo = fetch_repository("http://base", "secret", 1, 1) + repo = fetch_repository("http://base", Secret("secret"), 1, 1) assert repo["name"] == "org/repo" assert repo["provider_name"] == "github" @@ -70,7 +71,7 @@ def test_success(self, mock_get): def test_single_error_raises_coded_error(self, mock_get): mock_get.return_value = MagicMock(status_code=404, content=make_error_response("repository_not_found")) with pytest.raises(SCMCodedError) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) assert exc_info.value.code == "repository_not_found" @patch("scm.rpc.client.requests.get") @@ -80,7 +81,7 @@ def test_multiple_errors_raises_exception_group(self, mock_get): content=make_error_response("repository_not_found", "rpc_invalid_grant"), ) with pytest.raises(ExceptionGroup) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) codes = {e.code for e in exc_info.value.exceptions} assert codes == {"repository_not_found", "rpc_invalid_grant"} @@ -89,13 +90,13 @@ def test_multiple_errors_raises_exception_group(self, mock_get): def test_undeserializable_error_response(self, mock_get): mock_get.return_value = MagicMock(status_code=500, content=b"not valid json") with pytest.raises(SCMCodedError) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) assert exc_info.value.code == "rpc_errors_could_not_be_deserialized" @patch("scm.rpc.client.requests.get") def test_signs_get_request_headers(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) call_kwargs = mock_get.call_args headers = call_kwargs.kwargs["headers"] @@ -105,7 +106,7 @@ def test_signs_get_request_headers(self, mock_get): @patch("scm.rpc.client.requests.get") def test_tuple_repository_id(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - fetch_repository("http://base", "secret", 1, ("github", "ext-123")) + fetch_repository("http://base", Secret("secret"), 1, ("github", "ext-123")) headers = mock_get.call_args.kwargs["headers"] assert headers["X-Repository-Id"] == '["github","ext-123"]' @@ -183,7 +184,7 @@ class TestRpcApiClient: def test_request_encodes_action_and_signs(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="test-referrer", repository_id=1, @@ -224,7 +225,7 @@ def test_request_encodes_action_and_signs(self): def test_request_with_tuple_repository_id(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=("github", "ext-123"), @@ -240,7 +241,7 @@ def test_request_with_tuple_repository_id(self): def test_request_forwards_stream_opt_out(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -269,7 +270,7 @@ def _make_client( # Retries are off by default in the library; these tests opt in to exercise the retry path. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -409,7 +410,7 @@ def test_no_retries_by_default(self, mock_sleep): # and no transport_retry metric fires. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -459,7 +460,7 @@ def test_connection_error_intercept_defaults_off_when_omitted(self, mock_sleep): # retry_connection_errors is optional; omitting it leaves connection errors untouched. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, diff --git a/tests/unit/test_rpc_integration.py b/tests/unit/test_rpc_integration.py index d17c05b..770e91e 100644 --- a/tests/unit/test_rpc_integration.py +++ b/tests/unit/test_rpc_integration.py @@ -17,6 +17,7 @@ from scm.rpc.errors import deserialize_error from scm.rpc.helpers import sign_get from scm.rpc.server import RpcServer +from scm.secret import Secret from scm.test_fixtures import ( make_github_assignee, make_github_branch, @@ -83,7 +84,7 @@ def make_github_api_response( def make_rpc_server(repository: Repository, server_provider: MagicMock) -> RpcServer: return RpcServer( - secrets=[SIGNING_SECRET], + secrets=[Secret(SIGNING_SECRET)], fetch_repository=lambda org_id, repo_id: repository, fetch_provider=lambda org_id, repo: server_provider, record_count=lambda name, value, tags: None, @@ -129,7 +130,7 @@ def make_client_scm(organization_id, repository_id, server): # This is a real AoiClient but we'll client = RpcApiClient( full_url=BASE_URL, - signing_secret=SIGNING_SECRET, + signing_secret=Secret(SIGNING_SECRET), organization_id=organization_id, referrer="shared", repository_id=repository_id, @@ -141,7 +142,9 @@ def make_client_scm(organization_id, repository_id, server): scm = SourceCodeManager.make_client( organization_id, repository_id, - fetch_repository=lambda oid, rid: fetch_proxy_repository(BASE_URL, SIGNING_SECRET, oid, rid, client.session), + fetch_repository=lambda oid, rid: fetch_proxy_repository( + BASE_URL, Secret(SIGNING_SECRET), oid, rid, client.session + ), fetch_provider=lambda oid, repo: fetch_proxy_provider(client, oid, repo), record_count=lambda a, b, c: None, ) diff --git a/tests/unit/test_rpc_server.py b/tests/unit/test_rpc_server.py index 6dd0149..7a8f7eb 100644 --- a/tests/unit/test_rpc_server.py +++ b/tests/unit/test_rpc_server.py @@ -7,10 +7,11 @@ from scm.errors import 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.secret import Secret from scm.test_fixtures import BaseTestProvider from scm.types import Repository -TEST_SECRET = "test-secret" +TEST_SECRET = Secret("test-secret") def make_repository(**overrides) -> Repository: diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py new file mode 100644 index 0000000..731769d --- /dev/null +++ b/tests/unit/test_secret.py @@ -0,0 +1,109 @@ +import hashlib +import hmac + +import pytest +import requests + +from scm import Secret as ExportedSecret +from scm.manager import SourceCodeManager +from scm.rpc import helpers +from scm.rpc.client import RpcApiClient +from scm.rpc.helpers import sign_get +from scm.rpc.server import RpcServer +from scm.secret import Secret +from tests.unit.error_reports import assert_not_in_frames + +SECRET = "rpc-signing-secret-0000000000000000" + + +class TestSecret: + def test_reveal(self): + assert Secret(SECRET).reveal() == SECRET + + def test_hidden_from_repr_str_and_formatting(self): + secret = Secret(SECRET) + for shown in (repr(secret), str(secret), f"{secret}", f"{secret!r}", repr([secret]), repr({"key": secret})): + assert SECRET not in shown + assert repr(secret) == "Secret('[Filtered]')" + + def test_hidden_from_sentry(self): + assert Secret(SECRET).__sentry_repr__() == "[Filtered]" + + def test_has_no_attributes_to_look_into(self): + # With __slots__ there is no __dict__ for a serializer to walk. + assert not hasattr(Secret(SECRET), "__dict__") + + def test_false_when_empty(self): + # So an empty-secret check such as ``if not secret`` works on a Secret as on a string. + assert not Secret("") + assert Secret(SECRET) + + def test_exported_from_the_package(self): + assert ExportedSecret is Secret + + def test_signs_with_the_revealed_value(self): + assert sign_get(Secret(SECRET), 1, 5) == "rpc0:" + hmac.new(SECRET.encode(), b"15", hashlib.sha256).hexdigest() + + +# A revealed value kept in a variable, rather than used inline, would put the secret back in a frame. +class TestKeptOutOfErrorReports: + def test_proxy_client_that_cannot_reach_sentry(self, monkeypatch): + def unreachable(*args, **kwargs): + raise requests.ConnectionError("Sentry is unreachable") + + monkeypatch.setattr(requests, "get", unreachable) + with pytest.raises(requests.ConnectionError) as exc_info: + SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=Secret(SECRET)) + assert_not_in_frames(exc_info.value, SECRET) + + def test_api_client_that_cannot_reach_sentry(self): + class Unreachable: + def post(self, url, data, headers, stream=False): + raise requests.ConnectionError("Sentry is unreachable") + + client = RpcApiClient( + full_url="https://sentry.example/api/0/internal/scm-rpc/", + signing_secret=Secret(SECRET), + organization_id=1, + referrer="shared", + repository_id=5, + session=Unreachable, + ) + with pytest.raises(requests.ConnectionError) as exc_info: + client.request("GET", "/repos/getsentry/sentry") + assert_not_in_frames(exc_info.value, SECRET) + + def test_server_failing_while_it_verifies(self, monkeypatch): + def failing_compare(a, b): + raise RuntimeError("compare failed") + + monkeypatch.setattr(helpers.hmac, "compare_digest", failing_compare) + server = RpcServer( + secrets=[Secret(SECRET)], + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error: None, + ) + headers = {"Authorization": "rpcsignature rpc0:abc", "X-Organization-Id": "1", "X-Repository-Id": "5"} + with pytest.raises(RuntimeError) as exc_info: + server.get(headers) + assert_not_in_frames(exc_info.value, SECRET) + + +def make_server(secrets): + return RpcServer( + secrets=secrets, + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error: None, + ) + + +class TestServerRequiresSecret: + def test_refuses_a_plain_string(self): + # Built with one, the server would fail every request inside verification, with the strings in its frames. + with pytest.raises(TypeError, match=r"^secrets must be scm\.Secret objects, not str\.") as exc_info: + make_server([Secret(SECRET), SECRET]) + assert SECRET not in str(exc_info.value)