security: gate releases on reviewed main provenance - #375
codeforester merged 1 commit into
Conversation
|
Validation refresh (2026-09-30):
The repository-side #271 controls are now also complete: |
|
This looks good. |
| def main() -> None: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument("--event-name", default=os.environ.get("GITHUB_EVENT_NAME", "")) | ||
| parser.add_argument("--event-path", type=Path, default=os.environ.get("GITHUB_EVENT_PATH", "")) |
There was a problem hiding this comment.
Correctness: default=os.environ.get("GITHUB_EVENT_PATH", "") combined with type=Path makes the guard below dead code. argparse applies type to a string default, so when GITHUB_EVENT_PATH is unset the default becomes Path("") → PosixPath('.'), which is always truthy (Path has no __bool__). Reproduced directly: bool(Path('')) is True, so if not args.event_path: (line 115) never fires.
Failure scenario: GITHUB_EVENT_PATH unset and --event-path not passed (local/manual invocation, or a future workflow change that drops the env var) with ref_type=="tag". The intended clear error ("GitHub event payload is required...") never fires; instead _read_event_flags(Path('.')) calls .read_text() on a directory, raises IsADirectoryError (an OSError subclass), and gets caught into a confusing "could not read the GitHub event payload: [Errno 21] Is a directory" message. It still fails closed today only by accident via that except clause — the intended guard itself is unreachable, so a future tweak to that except clause could silently flip this to fail-open. Suggest defaulting to None (not Path-typed) and checking args.event_path is None.
| tag_type=tag_type, | ||
| resolved_tag_commit=resolved_tag_commit, | ||
| main_reachable=main_reachable, | ||
| repository_shallow=_git_output("rev-parse", "--is-shallow-repository") == "true", |
There was a problem hiding this comment.
Correctness (fail-open, inconsistent with main_reachable's handling of the same failure mode): repository_shallow=_git_output("rev-parse", "--is-shallow-repository") == "true". _git_output swallows any non-zero git exit into "". For main_reachable (lines 103/109), that same swallowing defaults to False, which correctly fails the tag-release gate (fail closed). But for repository_shallow, "" == "true" is also False, and False here means "not shallow" — i.e. the check passes.
Failure scenario: any git-command failure in this step (corrupted checkout, an unsupported flag on a future git version, a transient runner issue) silently waves through the shallow-history gate instead of raising an error, even though this module's own docstring promises "fail closed unless a publication comes from a reviewed main-line commit." Consider treating a non-zero exit here as an error (e.g. _git(...) returning a failure tuple should itself append a violation) rather than coercing it to a boolean that happens to mean "safe."
| return validate_release_provenance(**values) # type: ignore[arg-type] | ||
|
|
||
|
|
||
| class ReleaseProvenanceValidationTests(unittest.TestCase): |
There was a problem hiding this comment.
Test gap: every test in this file calls validate_release_provenance() directly with hand-built kwargs — none exercise main(), --event-path argparse wiring, _read_event_flags(), or the _git/_git_output subprocess wrappers. Both of the bugs I flagged on validate_release_provenance.py (the Path('') dead-code guard at line 93/115, and the fail-open shallow-repo check at line 129) live entirely in this untested glue code, so the full test suite gives no signal for either. Worth adding at least one test that drives main() end-to-end (e.g. via subprocess or by refactoring the argparse/env wiring into a small testable seam) to cover the actual production entrypoint, not just the pure validation function.
| errors = valid(resolved_tag_commit="b" * 40) | ||
| self.assertTrue(any("does not resolve" in error for error in errors)) | ||
|
|
||
| def test_rejects_forced_tag_update(self) -> None: |
There was a problem hiding this comment.
Test gap (asymmetric coverage): there's a test_rejects_forced_tag_update (forced=True) but no counterpart for deleted=True, even though validate_release_provenance() treats them symmetrically (if forced: ... / if deleted: ..., scripts/validate_release_provenance.py lines ~57-58). A regression on the deleted branch specifically (typo'd variable, inverted condition, accidentally removed check) would ship green. Suggest adding a test_rejects_deleted_tag_event mirroring the forced-tag test.
| assert "name: Verify reviewed release provenance" in workflow | ||
| assert 'git fetch --no-tags --prune origin "refs/heads/main:refs/remotes/origin/main"' in workflow | ||
| assert "python scripts/validate_release_provenance.py" in workflow | ||
| assert "needs: [build, smoke, provenance]" in workflow |
There was a problem hiding this comment.
Test gap (weak assertion on a security-critical gate): assert "needs: [build, smoke, provenance]" in workflow is a plain substring check. In the actual workflow, both the publish job and the attest job declare needs: [build, smoke, provenance] verbatim — this assertion can't distinguish "both jobs have the gate" from "only one does." If attest's needs: were ever reverted to [build, smoke] (silently dropping the provenance gate on the attestation step) while publish kept the full list, this assertion would still pass because the substring still occurs once, via publish. Consider asserting on each job's needs: line individually (e.g. locate the attest: job block and assert its own needs: line), so a regression on either job is caught independently.
| if repository_shallow: | ||
| errors.append("release provenance cannot be verified from a shallow repository") | ||
|
|
||
| production_release = (event_name == "push" and ref_type == "tag") or publish_target == "pypi" |
There was a problem hiding this comment.
Cleanup (minor): production_release = (event_name == "push" and ref_type == "tag") or publish_target == "pypi" is only used once, at if production_release and ref_type != "tag": (next line) — which is logically equivalent to if publish_target == "pypi" and ref_type != "tag":, since the first disjunct of production_release can never make ref_type != "tag" true (it requires ref_type == "tag"). Not a functional bug, just dead weight that could confuse a future maintainer editing this gate into thinking event_name/the first disjunct matters here.
|
Additional finding (file not touched by this PR's diff, so not postable as an inline comment):
|
2 participants
Summary
Repository controls
Validation
Fixes #271