Repository navigation
security: verify published PyPI artifact bytes - #497
codeforester wants to merge 3 commits into
Conversation
4aed2bf to
a83d688
Compare
| python scripts/verify_pypi_metadata.py \ | ||
| --version "${{ needs.build.outputs.version }}" \ | ||
| --artifacts-dir dist \ | ||
| --evidence-output dist/PYPI-BYTE-VERIFICATION.json |
There was a problem hiding this comment.
Correctness: the new "Upload PyPI byte verification evidence" step has no if: always(), so Actions skips it whenever the preceding verification step fails — meaning the evidence artifact is never produced on exactly the failure path (a PyPI byte/hash mismatch) it exists to document. When scripts/verify_pypi_metadata.py detects a mismatch, it writes the evidence file and then raises SystemExit(1); that marks the step failed, and the next step (no if: condition) is skipped by default — losing the forensic evidence, contradicting this PR's stated goal, and inconsistent with this repo's own convention in .github/workflows/tests.yml (~lines 121-127, 194-200), which pairs if: always() with if-no-files-found: ignore for exactly this "keep evidence even on failure" case. Naively adding just if: always() isn't quite right either — if the script crashes before writing any evidence file (e.g. _fetch() exhausts retries), the upload step's current if-no-files-found: error would then surface a misleading second "no files found" failure masking the real root cause; pair it with if-no-files-found: ignore like the existing convention does.
| continue | ||
| filename = entry.get("filename") | ||
| digest = (entry.get("digests") or {}).get("sha256") if isinstance(entry.get("digests"), dict) else None | ||
| if not isinstance(filename, str) or not isinstance(digest, str): |
There was a problem hiding this comment.
Reuse: this reimplements SHA-256 file hashing inline (hashlib.sha256(path.read_bytes()).hexdigest()) instead of reusing release_metadata_helpers.sha256_file(), which every sibling release-integrity script already shares (via the same try/except-ImportError pattern used in generate_release_metadata.py, validate_release_metadata.py, verify_release_assets.py). The shared helper streams in 1MB chunks; this loads the whole artifact into memory — duplicate logic that would need a second fix if hashing ever changes, with strictly worse memory behavior for no benefit.
| checksums: dict[str, str] = {} | ||
| if not checksums_path.is_file(): | ||
| errors.append("reviewed SHA256SUMS is missing") | ||
| else: |
There was a problem hiding this comment.
Clarity bug: this error message swaps the semantic labels — it prints the locally-reviewed hash set as "expected" and PyPI's actual response as "observed", backwards from how a reader would interpret those words during a CI failure (expected {sorted(observed)!r}, observed {sorted(published)!r}, where observed actually holds the locally-built/reviewed artifact hashes and published holds PyPI's response). A maintainer debugging a genuine mismatch could misattribute which side is the trusted baseline.
| digest = (entry.get("digests") or {}).get("sha256") if isinstance(entry.get("digests"), dict) else None | ||
| if not isinstance(filename, str) or not isinstance(digest, str): | ||
| errors.append("PyPI file URL entry has no filename and SHA-256 digest") | ||
| continue |
There was a problem hiding this comment.
Robustness: when PyPI's JSON response has a malformed/missing urls field, this discards the already-computed local artifact hashes and returns an empty evidence dict — losing forensic detail in exactly the case most likely to indicate something's wrong with the PyPI response. The local (reviewed) hashes were already computed before this check; the written evidence file ends up {} instead of recording them for later audit.
|
Implemented the review fix in commit
|
|
Follow-up after the latest push (
Leaving these threads open since the underlying code is unchanged for 3 of 4, and the 4th is only partially fixed. |
|
Addressed the remaining evidence/hash-validation feedback in commit
|
Summary
SHA256SUMSfile in the same gatePYPI-BYTE-VERIFICATION.jsonevidence as a workflow artifactSecurity rationale
Version and README checks alone do not prove that users received the reviewed bytes. The PyPI verification job now fails on missing, extra, renamed, or hash-mismatched files and records the result.
Validation
UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev pytest -q tests/test_verify_pypi_metadata.py tests/test_package_workflow.pygit diff --checkpassRelates to #489.