Skip to content

fix(launchpad): scope tracked-sensitive-files to what the cohort owns - #1983

Closed
tucktuck101 wants to merge 2 commits into
launchpadfrom
audit/1965-upstream-owned-fixtures
Closed

fix(launchpad): scope tracked-sensitive-files to what the cohort owns#1983
tucktuck101 wants to merge 2 commits into
launchpadfrom
audit/1965-upstream-owned-fixtures

Conversation

@tucktuck101

Copy link
Copy Markdown
Collaborator

Summary

audit failed on launchpad trunk, and so on every branch cut after 2026-08-28, over six files nobody in this cohort wrote. The check is not wrong about them — four genuinely contain private key material — but they are byte-for-byte upstream's APNs test fixtures, and this fork cannot fix them. This narrows the check by ownership rather than adding an exemption, because the check deliberately has no exemption mechanism and an earlier one caused a real miss.

Feature

N/A - single-issue PR

Related issue

Closes #1965

Issue type

Bug


Agent provenance

Field Value
Harness / provider omp
Model claude-opus-5
Session reference N/A
Initiating human @tucktuck101

Objective

A tracked-sensitive-files check that passes on launchpad trunk while still failing on any sensitive-shaped file this cohort added or modified.

Impacted components

launchpad/scripts/security_audit_classifier.py              additive: fetch_upstream_blobs, local_blob, divergence
launchpad/scripts/security_audit_tracked_files_check.py     partitions hits by ownership
launchpad/scripts/test_security_audit_tracked_files_check.py  4 new controls, existing ones taken off the network

Approach and rejected alternatives

The six files, classified by content — headers only, no key material printed:

apns-test-cert-only.pem              CERTIFICATE
apns-test-encrypted-identity.pem     ENCRYPTED PRIVATE KEY, CERTIFICATE
apns-test-identity.pem               PRIVATE KEY, CERTIFICATE
apns-test-key-only.pem               PRIVATE KEY
apns-test-mismatched-identity.pem    PRIVATE KEY, CERTIFICATE
apple-app-attestation-root.pem       CERTIFICATE

They arrived in c432a111c ("feat(mobile): push notifications MVP (block#6269)"), an ancestor of block/buzz@main, and are byte-for-byte identical here (git rev-parse on fork and upstream returns the same OIDs). block/buzz is public, so they have always been public — consistent with deliberately generated APNs test fixtures.

Rejected: an allowlist or path exemption. This was my first instinct and it is wrong. _matches_sensitive_shape has no exemption mechanism on purpose: a *.example suffix exemption was removed after it let a tracked seed/authorized_keys.example through, which is exactly the shape #68 documents a real past incident for. Re-adding the mechanism for these six would undo a control that was hardened deliberately.

Rejected: match on content instead of filename. Would not help — four of the six really do contain PRIVATE KEY blocks, so a content rule still fires.

Rejected: untrack them, or generate them at test time. Both edit upstream-owned test code, which this fork does not do.

Rejected: raise it upstream only. Arguably correct owner, and worth doing separately, but it leaves trunk red indefinitely.

Chosen: scope by ownership. A fork that operates the upstream product is accountable for what it wrote, not for what it inherited unchanged. divergence() returns fork-added, inherited-modified, inherited-identical or indeterminate, and only inherited-identical licenses a skip.

Identity, not origin — this is the part that matters. classify() already answered "did this path exist upstream", and that is not sufficient: an inherited filename is precisely where cohort-added key material could hide. divergence() compares blob OIDs, so a path present upstream but modified here still fails. The OID comes from the same single git fetch the classifier already ran — git ls-tree without --name-only is one column wider and costs no extra network.

fetch_upstream_paths and classify are untouched, so the change is additive. Incidentally this is the classifier's first caller: it was written for #62 and never wired to anything.

Unreachable upstream reports INDETERMINATE, naming the paths. Neither PASS nor FAIL is honest when ownership cannot be established, and INDETERMINATE is visibly not-green so it cannot be read as clean. Upstream-identical hits are named in the PASS detail — a skip nobody can see is indistinguishable from a check that stopped looking.

Verification

Command run:

python3 -m unittest discover -s launchpad/scripts -p "test_security_audit*.py"
# the changed check, against this repository
python3 -c "from security_audit_tracked_files_check import run; print(run(Path('.')))"
# negative control: a cohort-owned key, force-added because *.pem is gitignored,
# committed, checked, then reset away

Raw output:

--- all security-audit controls
........................................................................................
Ran 88 tests in 3.634s
OK

--- the changed check against this repository
status : pass
detail : no cohort-owned tracked file matches a sensitive shape (5718 tracked files checked);
         6 sensitive-shaped file(s) are byte-for-byte upstream's and not cohort-owned:
         crates/buzz-push-gateway/tests/fixtures/apns-test-cert-only.pem;
         crates/buzz-push-gateway/tests/fixtures/apns-test-encrypted-identity.pem;
         crates/buzz-push-gateway/tests/fixtures/apns-test-identity.pem;
         crates/buzz-push-gateway/tests/fixtures/apns-test-key-only.pem;
         crates/buzz-push-gateway/tests/fixtures/apns-test-mismatched-identity.pem;
         crates/buzz-push-gateway/tests/fixtures/apple-app-attestation-root.pem

--- negative control: cohort-owned key committed to THIS repo, then reverted
status : fail
detail : 1 cohort-owned tracked file(s) match a sensitive shape: launchpad/deploy/negative-control.pem

The negative control is the acceptance criterion demonstrated rather than asserted: with the six upstream fixtures present and correctly excluded, one cohort-added key still fails the run.

  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

Not verified

  • The full security_audit.py harness was not run to completion locally. It downloads and checksum-verifies a pinned gitleaks binary and exceeded a 600s timeout on this machine. Only the check I changed was run against the repository, plus all 88 unit controls. CI on this PR is the first end-to-end run.
  • The CI path through fetch_upstream_blobs is unproven. The workflow already uses fetch-depth: 0 because the classifier fetches block/buzz, but no job has ever exercised that fetch — this is the classifier's first caller, so its behaviour on a runner is untested.
  • Whether those four private keys are genuinely throwaway fixtures is upstream's claim, not a verified fact. They are named apns-test-*, live in a test fixtures directory, and are public in upstream — but nothing here proves they are unused elsewhere.
  • Rename/move is not tracked. A file moved upstream to a new path reads as fork-added at its old path and would fail. No such case exists today.
  • git ls-tree parsing assumes the standard <mode> <type> <oid>\t<path> shape. Malformed entries are skipped rather than raising, which is a silent-ish failure mode.

Authority

N/A - no approval or merge performed by an agent; this PR is submitted for human review.

Deferred blockers

none

Security implications

This narrows a security control, so it deserves the scrutiny. What it gives up: a sensitive-shaped file that is byte-for-byte identical to upstream's no longer fails. What it keeps: everything the cohort added (fork-added), everything the cohort changed (inherited-modified), and everything whose ownership cannot be established (indeterminate, which is not-green). The negative control above proves a cohort-added key still fails with the upstream fixtures present.

The alternative was an exemption list, which would have been a permanent hole matched by name rather than by content identity — strictly weaker than this, and against the documented intent of the function it would have modified.

One genuine new dependency: correctness now rests on git fetch of a public upstream reaching the network. Failure is handled as INDETERMINATE rather than PASS, so a network outage cannot silently turn the check green.

Escalations

  • Upstream arguably should not ship private-key-shaped test fixtures, even generated ones, and this PR does not tell them so. Worth a separate upstream report; I have not filed one because that is a decision about how this cohort talks to block/buzz, not a code fix.
  • I recommended the allowlist approach first and it was wrong. The check's own comments document why exemptions were removed, and I proposed re-adding the mechanism before reading them. Recording it because the reasoning matters more than the outcome: a security control's history is part of its specification.
  • Four of the six files really do contain private keys. If anyone believes one of them could be a real credential rather than a fixture, this stops being a CI problem and becomes an upstream security report, and this PR should be held.

`audit` failed on `launchpad` trunk, and therefore on every branch cut after
2026-08-28, over six files nobody here wrote.

The check is not wrong about them. Four of the six genuinely contain private key
material:

    apns-test-cert-only.pem              CERTIFICATE
    apns-test-encrypted-identity.pem     ENCRYPTED PRIVATE KEY, CERTIFICATE
    apns-test-identity.pem               PRIVATE KEY, CERTIFICATE
    apns-test-key-only.pem               PRIVATE KEY
    apns-test-mismatched-identity.pem    PRIVATE KEY, CERTIFICATE
    apple-app-attestation-root.pem       CERTIFICATE

They arrived in `c432a111c` ("feat(mobile): push notifications MVP (block#6269)"),
which is an ancestor of `block/buzz@main`, and they are byte-for-byte identical
here. They are APNs test fixtures in upstream's public repository. This fork
operates the upstream product rather than developing it: it cannot fix them, and
it does not move or rename upstream paths.

So this narrows by OWNERSHIP, not by exemption. An allowlist would be a standing
hole, and this check deliberately has none — a suffix exemption was removed after
one let a tracked `seed/authorized_keys.example` through, which is the shape #68
documents a real past incident for. Adding the mechanism back for these six would
undo that.

IDENTITY, NOT ORIGIN. `security_audit_classifier.classify` already answered "did
this path exist upstream", and that is not enough: an inherited filename is
exactly where cohort-added key material could hide. `divergence()` compares blob
OIDs and returns one of `fork-added`, `inherited-modified`, `inherited-identical`
or `indeterminate`. Only `inherited-identical` licenses a skip; the other three
are cohort-accountable or unknown.

The OID comes from the same single `git fetch` the classifier already performed —
`git ls-tree` without `--name-only` is one column wider and costs no extra
network. `fetch_upstream_paths` and `classify` are untouched, so this is additive.
It is also the classifier's first caller: it was built for #62 and never wired.

Unreachable upstream reports INDETERMINATE, naming the paths — neither PASS nor
FAIL is honest when ownership cannot be established, and INDETERMINATE is visibly
not-green so it cannot be mistaken for clean. Upstream-identical hits are named
in the PASS detail: a skip nobody can see is indistinguishable from a check that
stopped looking.

The pre-existing controls now patch the upstream fetch rather than reaching the
network to decide something their own fixtures already know. Four controls added:
upstream-identical does not fail and is named; a cohort-added key still fails
alongside upstream fixtures; an inherited path modified here still fails;
unreachable upstream is INDETERMINATE, not PASS.

Verified against this repository:

  status : pass
  detail : no cohort-owned tracked file matches a sensitive shape (5718 tracked
           files checked); 6 sensitive-shaped file(s) are byte-for-byte
           upstream's and not cohort-owned: crates/buzz-push-gateway/tests/
           fixtures/apns-test-cert-only.pem; ... apple-app-attestation-root.pem

Tests: 88 security-audit tests pass (14 in this check's own suite, 4 new).

Closes #1965

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101 tucktuck101 added bug Something isn't working area:ci GitHub Actions, checks, automation by:agent Filed or authored by an AI agent, not a human labels Aug 31, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 4b6570ffa46e9407e4d3327dfae6f3eff7c018c7 against merge base 1ed55e980b0043f92d9c652e6a39a8e49345389c.

Incomplete

This review is INCOMPLETE and must not be read as a full pass:

  • no dimension was actually reviewed: the pipeline ran the 'default_reviewer' stub reviewer, which reports every dimension clean without reading it (a real dimension reviewer is #116)

Containment

No containment findings.

Fetched and empty: pr_issue_comments, pr_review_comments.

Automated containment covers the delimiter boundary and unambiguous injection tells only. It does not cover injection phrased as ordinary, unremarkable prose. The absence of a containment finding is not evidence that this pull request contains no injection attempt.

The previous revision used a real `-----BEGIN PRIVATE KEY-----` header as
fixture content, and `gitleaks-secret-scan` flagged two of those lines as
private-key findings — correctly. The check under test matches on path shape and
never reads content, so the header bought nothing.

A control for a secret-detection suite must not itself be the thing it detects.

Refs #1965

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101

Copy link
Copy Markdown
Collaborator Author

Superseded by #1984, which carries identical content as one clean commit.

Reason: this branch's first commit used a real -----BEGIN PRIVATE KEY----- header as fixture content, and gitleaks-secret-scan flagged two lines as private-key findings — correctly. security_audit_secrets_check.py scans --log-opts=<base>..HEAD, i.e. the commits a branch adds rather than its tip, so removing the header in commit 2 did not clear the finding. Force-pushing is barred by §6, so a fresh single-commit branch was the only convention-respecting remedy.

Worth recording: tracked-sensitive-files passed in CI on this branch before it was closed, which is the proof that the ownership scoping works on a runner and not just locally:

[ PASS] tracked-sensitive-files - no cohort-owned tracked file matches a sensitive shape
        (5718 tracked files checked); 6 sensitive-shaped file(s) are byte-for-byte
        upstream's and not cohort-owned: crates/buzz-push-gateway/tests/fixtures/...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci GitHub Actions, checks, automation bug Something isn't working by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

audit check fails on launchpad trunk: six tracked .pem fixtures from c432a111c break every branch cut after 2026-08-28

1 participant