Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions .github/workflows/package.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ on:
- "scripts/verify_release_assets.py"
- "scripts/validate_changelog.py"
- "scripts/validate_release_ref.py"
- "scripts/validate_release_provenance.py"
- "CHANGELOG.md"
- "examples/**"
- "compatibility/**"
- "docs/releasing.md"
- "tests/test_package_workflow.py"
- "tests/test_verify_release_assets.py"
- "tests/test_validate_release_provenance.py"
- "requirements/release.in"
- "requirements/release.txt"
- ".github/workflows/package.yml"
Expand Down Expand Up @@ -235,9 +237,31 @@ jobs:
- name: Check installed dependency consistency
run: python -m pip check

provenance:
name: Verify reviewed release provenance
needs: [build, smoke]
if: ${{ (github.event_name == 'push' && github.ref_type == 'tag') || github.event_name == 'workflow_dispatch' }}
runs-on: ubuntu-latest
timeout-minutes: 10
permissions:
contents: read
steps:
- name: Check out source
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
fetch-depth: 0

- name: Fetch trusted main history
run: git fetch --no-tags --prune origin "refs/heads/main:refs/remotes/origin/main"

- name: Verify release provenance
env:
PUBLISH_TARGET: ${{ inputs.publish_target || '' }}
run: python scripts/validate_release_provenance.py

publish:
name: Publish reviewed distribution
needs: [build, smoke]
needs: [build, smoke, provenance]
if: ${{ (github.event_name == 'push' && github.ref_type == 'tag') || github.event_name == 'workflow_dispatch' }}
runs-on: ubuntu-latest
timeout-minutes: 10
Expand Down Expand Up @@ -269,7 +293,7 @@ jobs:

attest:
name: Attest reviewed release
needs: [build, smoke]
needs: [build, smoke, provenance]
if: ${{ (github.event_name == 'push' && github.ref_type == 'tag') || github.event_name == 'workflow_dispatch' }}
runs-on: ubuntu-latest
timeout-minutes: 10
Expand Down Expand Up @@ -303,7 +327,7 @@ jobs:

release:
name: Create GitHub Release
needs: [build, smoke, publish, attest]
needs: [build, smoke, provenance, publish, attest]
if: ${{ github.event_name == 'push' && github.ref_type == 'tag' }}
runs-on: ubuntu-latest
timeout-minutes: 10
Expand Down
29 changes: 24 additions & 5 deletions docs/releasing.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,15 @@ policy](https://github.com/basefoundry/base/blob/main/docs/ecosystem-policy.md).
the wheel and sdist metadata, and `base_cli.__version__` reports the same value
from a source checkout or from installed distribution metadata.

Production releases use a matching annotated-style tag such as `v0.1.0`.
The Package workflow rejects a tag that does not exactly match `v${VERSION}`.
Production releases use a matching annotated tag such as `v0.1.0`, created from
`main`. Lightweight tags, tags pointing at a commit outside `main`, forced tag
updates, and tags that do not exactly match `v${VERSION}` are rejected before
publication. The workflow fetches the complete trusted `main` history so an
ancestor check fails closed instead of relying on shallow checkout state.
The active default-branch ruleset also requires one approving pull-request
review, approval from someone other than the last pusher, strict up-to-date
status checks, and the policy, quality, runtime, and consumer checks listed in
the repository ruleset. Deletion and non-fast-forward updates are disabled.

## Validation workflow

Expand All @@ -43,6 +50,13 @@ runs, GitHub's OIDC-backed `actions/attest` job records both build provenance
and an SBOM attestation for the exact artifact digests; no PyPI token or other
long-lived publish secret is used.

The provenance job runs after build and smoke validation and before any
publication, attestation, or GitHub Release write. It verifies the full source
SHA, annotated tag object, exact tag target, trusted `origin/main` ancestry,
non-shallow history, and push-event force/deletion flags. A TestPyPI dispatch
from a branch remains available for rehearsal, but still requires full history
and a full source SHA.

For a version tag, the same Package workflow creates a GitHub Release after
the protected PyPI publication and attestations succeed. The release attaches
the exact reviewed wheel, sdist, `SHA256SUMS`, `SBOM.spdx.json`, and
Expand Down Expand Up @@ -174,9 +188,14 @@ publishing for this repository and workflow before the dispatch can upload.
'import base_cli; import importlib.metadata as m; assert base_cli.__version__ == m.version("base-cli"); print(base_cli.__version__)'
```

The `pypi` GitHub environment must require approval and be configured with the
PyPI trusted publisher for `.github/workflows/package.yml`. No long-lived PyPI
token is stored in the repository.
The `pypi` GitHub environment requires approval, rejects self-review, disallows
administrator bypass, and is configured with the PyPI trusted publisher for
`.github/workflows/package.yml`. No long-lived PyPI token is stored in the
repository. Production publication therefore waits for an independent
reviewer until maintainer-capacity work in [#252](https://github.com/basefoundry/base-cli/issues/252)
adds one. If a temporary solo-maintainer exception is ever needed, it must be
time-bounded and record an owner, expiry, audit trail, and link to #252 before
an administrator changes the environment policy.

## Recovery

Expand Down
141 changes: 141 additions & 0 deletions scripts/validate_release_provenance.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
#!/usr/bin/env python3
"""Fail closed unless a publication comes from a reviewed main-line commit."""

from __future__ import annotations

import argparse
import json
import os
import re
import subprocess
import sys
from pathlib import Path

FULL_SHA = re.compile(r"^[0-9a-f]{40}$")
VALID_PUBLISH_TARGETS = {"", "testpypi", "pypi"}


def validate_release_provenance(
*,
event_name: str,
ref_type: str,
tag: str,
publish_target: str,
source_commit: str,
tag_type: str,
resolved_tag_commit: str,
main_reachable: bool,
repository_shallow: bool,
forced: bool = False,
deleted: bool = False,
) -> list[str]:
"""Return violations for the source that is about to be published."""
errors: list[str] = []
if publish_target not in VALID_PUBLISH_TARGETS:
errors.append(f"unsupported publication target {publish_target!r}")
if not FULL_SHA.fullmatch(source_commit):
errors.append("reviewed source commit must be a full 40-character commit SHA")
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"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

tag_release = ref_type == "tag"
if production_release and ref_type != "tag":
errors.append("PyPI publication requires a version tag, not a branch or pull request ref")

if tag_release:
if not tag.startswith("v") or tag == "v":
errors.append(f"release tag must be a v-prefixed version, got {tag!r}")
if tag_type != "tag":
errors.append("release tag must be an annotated tag; lightweight tags are rejected")
if not FULL_SHA.fullmatch(resolved_tag_commit):
errors.append("release tag must resolve to a full commit SHA")
elif resolved_tag_commit != source_commit:
errors.append(
f"release tag does not resolve to the reviewed source commit ({resolved_tag_commit} != {source_commit})"
)
if not main_reachable:
errors.append("release tag commit is not reachable from the trusted origin/main history")
if forced:
errors.append("forced tag updates are rejected for release publication")
if deleted:
errors.append("deleted tag events are rejected for release publication")

return errors


def _git(*args: str) -> tuple[int, str]:
completed = subprocess.run(
["git", *args],
check=False,
capture_output=True,
text=True,
)
return completed.returncode, completed.stdout.strip()


def _git_output(*args: str) -> str:
returncode, output = _git(*args)
return output if returncode == 0 else ""


def _read_event_flags(event_path: Path) -> tuple[bool, bool, list[str]]:
try:
payload = json.loads(event_path.read_text(encoding="utf-8"))
except (OSError, json.JSONDecodeError) as exc:
return False, False, [f"could not read the GitHub event payload: {exc}"]
return bool(payload.get("forced")), bool(payload.get("deleted")), []


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

parser.add_argument("--ref-type", default=os.environ.get("GITHUB_REF_TYPE", ""))
parser.add_argument("--tag", default=os.environ.get("GITHUB_REF_NAME", ""))
parser.add_argument("--publish-target", default=os.environ.get("PUBLISH_TARGET", ""))
parser.add_argument("--source-commit", default=os.environ.get("GITHUB_SHA", ""))
parser.add_argument("--main-ref", default="refs/remotes/origin/main")
args = parser.parse_args()

tag_type = ""
resolved_tag_commit = ""
main_reachable = False
if args.ref_type == "tag":
tag_ref = f"refs/tags/{args.tag}"
tag_type = _git_output("cat-file", "-t", tag_ref)
resolved_tag_commit = _git_output("rev-parse", "--verify", f"{tag_ref}^{{}}")
if resolved_tag_commit:
main_reachable = _git("merge-base", "--is-ancestor", resolved_tag_commit, args.main_ref)[0] == 0

forced = False
deleted = False
event_errors: list[str] = []
if args.ref_type == "tag" or args.publish_target == "pypi":
if not args.event_path:
event_errors.append("GitHub event payload is required for release provenance validation")
else:
forced, deleted, event_errors = _read_event_flags(Path(args.event_path))

errors = event_errors + validate_release_provenance(
event_name=args.event_name,
ref_type=args.ref_type,
tag=args.tag,
publish_target=args.publish_target,
source_commit=args.source_commit,
tag_type=tag_type,
resolved_tag_commit=resolved_tag_commit,
main_reachable=main_reachable,
repository_shallow=_git_output("rev-parse", "--is-shallow-repository") == "true",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

forced=forced,
deleted=deleted,
)
if errors:
for error in errors:
print(f"release provenance validation failed: {error}", file=sys.stderr)
raise SystemExit(1)
print(f"Validated release provenance for {args.source_commit}")


if __name__ == "__main__":
main()
10 changes: 10 additions & 0 deletions tests/test_package_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,13 @@ def test_package_workflow_does_not_replace_published_release_assets() -> None:
assert '--source-digest "$GITHUB_SHA"' in workflow
assert '--source-ref "$GITHUB_REF"' in workflow
assert "--clobber" not in workflow


def test_package_workflow_gates_writes_on_release_provenance() -> None:
workflow = (Path(__file__).resolve().parents[1] / ".github/workflows/package.yml").read_text(encoding="utf-8")

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

assert "needs: [build, smoke, provenance, publish, attest]" in workflow
74 changes: 74 additions & 0 deletions tests/test_validate_release_provenance.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
from __future__ import annotations

import sys
import unittest
from pathlib import Path

sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
from scripts.validate_release_provenance import validate_release_provenance

SOURCE = "a" * 40
TAG_COMMIT = SOURCE


def valid(**overrides: object) -> list[str]:
values: dict[str, object] = {
"event_name": "push",
"ref_type": "tag",
"tag": "v1.0.0",
"publish_target": "",
"source_commit": SOURCE,
"tag_type": "tag",
"resolved_tag_commit": TAG_COMMIT,
"main_reachable": True,
"repository_shallow": False,
}
values.update(overrides)
return validate_release_provenance(**values) # type: ignore[arg-type]


class ReleaseProvenanceValidationTests(unittest.TestCase):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

def test_accepts_annotated_tag_on_main(self) -> None:
self.assertEqual(valid(), [])

def test_rejects_lightweight_tag(self) -> None:
errors = valid(tag_type="commit")
self.assertTrue(any("annotated tag" in error for error in errors))

def test_rejects_unmerged_commit(self) -> None:
errors = valid(main_reachable=False)
self.assertTrue(any("not reachable" in error for error in errors))

def test_rejects_moved_or_mismatched_tag(self) -> None:
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:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

errors = valid(forced=True)
self.assertTrue(any("forced tag" in error for error in errors))

def test_rejects_shallow_history(self) -> None:
errors = valid(repository_shallow=True)
self.assertTrue(any("shallow" in error for error in errors))

def test_rejects_pypi_dispatch_from_a_branch(self) -> None:
errors = valid(event_name="workflow_dispatch", ref_type="branch", publish_target="pypi")
self.assertTrue(any("requires a version tag" in error for error in errors))

def test_allows_testpypi_branch_rehearsal_with_full_history(self) -> None:
self.assertEqual(
valid(
event_name="workflow_dispatch",
ref_type="branch",
tag="",
publish_target="testpypi",
tag_type="",
resolved_tag_commit="",
main_reachable=False,
),
[],
)


if __name__ == "__main__":
unittest.main()
Loading