diff --git a/launchpad/scripts/security_audit_classifier.py b/launchpad/scripts/security_audit_classifier.py index e2684483d90..3011bfb577c 100644 --- a/launchpad/scripts/security_audit_classifier.py +++ b/launchpad/scripts/security_audit_classifier.py @@ -23,7 +23,7 @@ import re import subprocess from pathlib import Path, PurePosixPath -from typing import Optional, Set +from typing import Dict, Optional, Set #: Matches a Windows drive letter root ("C:/", "d:/") after backslash-to-slash #: normalization. PurePosixPath.is_absolute() only recognizes a leading "/" -- @@ -85,3 +85,91 @@ def classify(path: str, upstream_paths: Optional[Set[str]]) -> str: if PurePosixPath(normalized).is_absolute() or _WINDOWS_DRIVE_ROOT.match(normalized): return "indeterminate" return "inherited" if normalized in upstream_paths else "fork-added" + + +def fetch_upstream_blobs(repo_root: Path, timeout: int = 30) -> Optional[Dict[str, str]]: + """Map every upstream path to its blob OID, or None if upstream is unreachable. + + Same single fetch as `fetch_upstream_paths`, one column wider: `git ls-tree` + already carries the OID, so identity costs no extra network. Callers that + only need origin should keep using `fetch_upstream_paths`. + + Origin is not enough for a check that asks "is the cohort accountable for + this file". A path present upstream may still have been modified here, and a + modification is exactly where cohort-owned content could hide inside an + inherited filename. Comparing OIDs answers "is this byte-for-byte upstream's + file", which is the question that licenses skipping it. + """ + try: + subprocess.run( + ["git", "fetch", "--depth=1", UPSTREAM_URL, UPSTREAM_REF], + cwd=repo_root, + check=True, + capture_output=True, + timeout=timeout, + ) + result = subprocess.run( + # -z for the same quoting reason `fetch_upstream_paths` documents. + # Default (non---name-only) output is " \t". + ["git", "ls-tree", "-r", "-z", "FETCH_HEAD"], + cwd=repo_root, + check=True, + capture_output=True, + text=True, + timeout=timeout, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError): + return None + blobs: Dict[str, str] = {} + for entry in filter(None, result.stdout.split("\0")): + meta, _, path = entry.partition("\t") + parts = meta.split() + if not path or len(parts) < 3: + continue + blobs[path] = parts[2] + return blobs + + +def local_blob(repo_root: Path, path: str, timeout: int = 30) -> Optional[str]: + """The blob OID of `path` at HEAD, or None when it cannot be resolved.""" + try: + result = subprocess.run( + ["git", "rev-parse", f"HEAD:{path}"], + cwd=repo_root, + check=True, + capture_output=True, + text=True, + timeout=timeout, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError): + return None + return result.stdout.strip() or None + + +def divergence(path: str, repo_root: Path, upstream_blobs: Optional[Dict[str, str]]) -> str: + """Ownership of one path, as one of four answers. + + - `fork-added` — absent upstream. The cohort wrote it. + - `inherited-modified` — present upstream, different content here. + - `inherited-identical`— present upstream, byte-for-byte identical. + - `indeterminate` — upstream unreachable, or the local OID would not + resolve. Never guessed in either direction, for the + asymmetry this module's docstring sets out. + + Only `inherited-identical` licenses a caller to treat a file as none of the + cohort's business. The other three are all cohort-accountable or unknown. + """ + if upstream_blobs is None: + return "indeterminate" + normalized = path.replace("\\", "/") + if normalized.startswith("./"): + normalized = normalized[2:] + if PurePosixPath(normalized).is_absolute() or _WINDOWS_DRIVE_ROOT.match(normalized): + return "indeterminate" + upstream_oid = upstream_blobs.get(normalized) + if upstream_oid is None: + return "fork-added" + here = local_blob(repo_root, normalized) + if here is None: + return "indeterminate" + return "inherited-identical" if here == upstream_oid else "inherited-modified" diff --git a/launchpad/scripts/security_audit_tracked_files_check.py b/launchpad/scripts/security_audit_tracked_files_check.py index 40bbb78e315..bcf8149f990 100644 --- a/launchpad/scripts/security_audit_tracked_files_check.py +++ b/launchpad/scripts/security_audit_tracked_files_check.py @@ -28,6 +28,7 @@ from typing import List, Optional from security_audit_core import CheckResult, Status +from security_audit_classifier import divergence, fetch_upstream_blobs from security_audit_ignore_coverage_check import REQUIRED_COVERAGE NAME = "tracked-sensitive-files" @@ -149,6 +150,46 @@ def _check_newly_hidden_tracked_files(repo_root: Path, tracked: List[str]) -> Li return findings +def _partition_by_ownership( + repo_root: Path, hits: List[str] +) -> tuple[List[str], List[str], List[str]]: + """Split sensitive-shaped hits into (cohort, upstream_identical, unknown). + + WHY OWNERSHIP MATTERS HERE. This is a fork that operates the upstream + product rather than developing it. Upstream ships APNs test fixtures that + are genuinely private-key-shaped and genuinely private keys + (`crates/buzz-push-gateway/tests/fixtures/apns-test-*.pem`, from + `c432a111c`, an ancestor of `block/buzz@main`, byte-for-byte identical + here). This check flagged all six on every branch cut after 2026-08-28, so + `audit` was red on trunk and on every pull request regardless of content + (launchpad-26/buzz#1965). A deterministic check that fails for everyone + stops being read. + + The cohort cannot fix those files — it does not own them, and it does not + move or rename upstream paths. What it can do is be precise about what it is + accountable for. So this narrows by OWNERSHIP, not by adding an exemption + list: an allowlist would be a standing hole, and this check deliberately has + none (see `_matches_sensitive_shape`, where a suffix exemption was removed + after one let a tracked `seed/authorized_keys.example` through). + + Identity, not origin. A file present upstream but MODIFIED here still fails: + an inherited filename is exactly where cohort-added key material could hide. + """ + upstream_blobs = fetch_upstream_blobs(repo_root) + cohort: List[str] = [] + upstream_identical: List[str] = [] + unknown: List[str] = [] + for path in hits: + verdict = divergence(path, repo_root, upstream_blobs) + if verdict == "inherited-identical": + upstream_identical.append(path) + elif verdict == "indeterminate": + unknown.append(path) + else: # fork-added or inherited-modified — the cohort is accountable + cohort.append(path) + return cohort, upstream_identical, unknown + + def run(repo_root: Path) -> CheckResult: tracked = _tracked_paths(repo_root) if tracked is None: @@ -157,13 +198,31 @@ def run(repo_root: Path) -> CheckResult: sensitive_hits = sorted(p for p in tracked if _matches_sensitive_shape(p)) newly_hidden = _check_newly_hidden_tracked_files(repo_root, tracked) + cohort_hits: List[str] = [] + upstream_hits: List[str] = [] + unknown_hits: List[str] = [] if sensitive_hits: + cohort_hits, upstream_hits, unknown_hits = _partition_by_ownership( + repo_root, sensitive_hits + ) + + if cohort_hits: return CheckResult( NAME, Status.FAIL, - f"{len(sensitive_hits)} tracked file(s) match a sensitive shape: " - + "; ".join(sensitive_hits[:10]) - + (f", and {len(sensitive_hits) - 10} more" if len(sensitive_hits) > 10 else ""), + f"{len(cohort_hits)} cohort-owned tracked file(s) match a sensitive shape: " + + "; ".join(cohort_hits[:10]) + + (f", and {len(cohort_hits) - 10} more" if len(cohort_hits) > 10 else ""), + ) + if unknown_hits: + # Upstream unreachable: ownership is unknown, so neither failing nor + # passing is honest. INDETERMINATE is visibly not-green and cannot be + # mistaken for a clean result. + return CheckResult( + NAME, + Status.INDETERMINATE, + f"could not establish upstream ownership for {len(unknown_hits)} " + f"sensitive-shaped tracked file(s): " + "; ".join(unknown_hits[:10]), ) if newly_hidden: return CheckResult( @@ -172,8 +231,18 @@ def run(repo_root: Path) -> CheckResult: f"{len(newly_hidden)} newly-added ignore pattern(s) cover already-tracked content: " + "; ".join(newly_hidden), ) + # The upstream-identical hits are named, not silently dropped. A skip nobody + # can see is indistinguishable from a check that stopped looking. + suffix = "" + if upstream_hits: + suffix = ( + f"; {len(upstream_hits)} sensitive-shaped file(s) are byte-for-byte " + f"upstream's and not cohort-owned: " + "; ".join(upstream_hits[:10]) + + (f", and {len(upstream_hits) - 10} more" if len(upstream_hits) > 10 else "") + ) return CheckResult( NAME, Status.PASS, - f"no tracked file matches a sensitive shape ({len(tracked)} tracked files checked)", + f"no cohort-owned tracked file matches a sensitive shape " + f"({len(tracked)} tracked files checked){suffix}", ) diff --git a/launchpad/scripts/test_security_audit_tracked_files_check.py b/launchpad/scripts/test_security_audit_tracked_files_check.py index 7b337b76070..ef71ffae101 100644 --- a/launchpad/scripts/test_security_audit_tracked_files_check.py +++ b/launchpad/scripts/test_security_audit_tracked_files_check.py @@ -31,7 +31,42 @@ def _init_repo_with_files(root: Path, files: dict[str, str]) -> None: subprocess.run(["git", "commit", "-q", "-m", "test fixture"], cwd=root, check=True) -class TrackedSensitiveFilesTest(unittest.TestCase): +# FIXTURE CONTENT IS DELIBERATELY NOT KEY-SHAPED. The check under test matches on +# path shape, never on content, so a fixture never needs a real PEM header — and +# an earlier revision of this file that used one made `gitleaks-secret-scan` fail +# on two of these very lines. A control for a secret-detection suite must not +# itself be the thing it detects. + +def _oid(root: Path, rel_path: str) -> str: + """The committed blob OID of `rel_path`, for building a fake upstream tree.""" + result = subprocess.run( + ["git", "rev-parse", f"HEAD:{rel_path}"], + cwd=root, check=True, capture_output=True, text=True, + ) + return result.stdout.strip() + + +class _NoUpstreamMixin: + """Keep these tests off the network. + + `run()` now asks `security_audit_classifier.fetch_upstream_blobs` who owns a + sensitive-shaped hit, and that shells out to `git fetch` against block/buzz. + Unpatched, every case below would clone a real remote to decide something + the fixture already knows. An empty upstream tree is the honest stand-in for + "this fixture repo inherited nothing", which is what makes every + pre-existing expectation here still the right one. + """ + + def setUp(self): + patcher = mock.patch( + "security_audit_tracked_files_check.fetch_upstream_blobs", + return_value={}, + ) + self.fetch_upstream = patcher.start() + self.addCleanup(patcher.stop) + + +class TrackedSensitiveFilesTest(_NoUpstreamMixin, unittest.TestCase): def test_clean_repo_passes(self): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) @@ -103,7 +138,93 @@ def test_git_failure_is_indeterminate(self): self.assertEqual(result.status, Status.INDETERMINATE) -class NewlyHiddenTrackedFileTest(unittest.TestCase): +class UpstreamOwnershipTest(unittest.TestCase): + """Ownership scoping for #1965. + + Upstream ships genuinely private-key-shaped APNs test fixtures. This fork + operates the upstream product rather than developing it, cannot fix those + files, and was reporting FAIL on every branch for all six — so `audit` was + red on trunk and unreadable on every pull request. Scoping by ownership must + silence exactly that case and nothing else. + """ + + def test_upstream_identical_fixture_does_not_fail_and_is_named(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _init_repo_with_files( + root, {"crates/gw/tests/fixtures/apns-test-identity.pem": "fixture bytes, deliberately not key-shaped\n"} + ) + upstream = {"crates/gw/tests/fixtures/apns-test-identity.pem": + _oid(root, "crates/gw/tests/fixtures/apns-test-identity.pem")} + with mock.patch( + "security_audit_tracked_files_check.fetch_upstream_blobs", + return_value=upstream, + ): + result = run(root) + self.assertEqual(result.status, Status.PASS) + # Named, not silently dropped: a skip nobody can see is a check that stopped looking. + self.assertIn("apns-test-identity.pem", result.detail) + self.assertIn("upstream", result.detail) + + def test_a_cohort_added_key_still_fails_alongside_upstream_fixtures(self): + """The acceptance criterion: narrowing must not blind the check. + + An upstream-identical fixture and a cohort-added key in the same tree — + the second one must still fail the run. + """ + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _init_repo_with_files(root, { + "crates/gw/tests/fixtures/apns-test-identity.pem": "fixture bytes, deliberately not key-shaped\n", + "launchpad/deploy/relay.pem": "fixture bytes, deliberately not key-shaped\n", + }) + upstream = {"crates/gw/tests/fixtures/apns-test-identity.pem": + _oid(root, "crates/gw/tests/fixtures/apns-test-identity.pem")} + with mock.patch( + "security_audit_tracked_files_check.fetch_upstream_blobs", + return_value=upstream, + ): + result = run(root) + self.assertEqual(result.status, Status.FAIL) + self.assertIn("relay.pem", result.detail) + # The upstream fixture must not be counted against the cohort. + self.assertNotIn("apns-test-identity.pem", result.detail) + + def test_an_inherited_path_modified_here_still_fails(self): + """Identity, not origin. + + An inherited filename is exactly where cohort-added key material could + hide, so presence upstream is not enough — the bytes must match. + """ + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _init_repo_with_files( + root, {"crates/gw/tests/fixtures/apns-test-identity.pem": "fixture bytes, deliberately not key-shaped (ours)\n"} + ) + upstream = {"crates/gw/tests/fixtures/apns-test-identity.pem": "0" * 40} + with mock.patch( + "security_audit_tracked_files_check.fetch_upstream_blobs", + return_value=upstream, + ): + result = run(root) + self.assertEqual(result.status, Status.FAIL) + self.assertIn("apns-test-identity.pem", result.detail) + + def test_unreachable_upstream_is_indeterminate_not_pass(self): + """Ownership unknown is neither clean nor damning, and must not read as clean.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _init_repo_with_files(root, {"deploy/host.pem": "fixture bytes, deliberately not key-shaped\n"}) + with mock.patch( + "security_audit_tracked_files_check.fetch_upstream_blobs", + return_value=None, + ): + result = run(root) + self.assertEqual(result.status, Status.INDETERMINATE) + self.assertIn("host.pem", result.detail) + + +class NewlyHiddenTrackedFileTest(_NoUpstreamMixin, unittest.TestCase): """PR-mode only: a real two-commit history against a real 'origin' remote (a second bare repo, not a mock) -- proves the diff-against-base-ref path actually reads real git history, not a mocked stand-in for it.