Repository navigation
fix: close post-merge Codex provenance, recovery and OCI review debt - #71
Conversation
…ws and fence streams
… frozen overlay hashes
…nd enforce promotion gate
… and cover inputs
|
@codex review Review exact head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
AllfatherChess/scripts/verify-release-oci.py
Lines 263 to 266 in da7b516
Reject attestation descriptors as the BuildKit image digest
When containerimage.digest is erroneous or tampered to equal a top-level attestation-manifest descriptor, this allowlist accepts it because all index children are added indiscriminately. The verifier then reports a matching BuildKit digest even though the metadata did not identify the application image or the index; restrict accepted child descriptors to the selected linux/amd64 application descriptor.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accepted = {image["manifest_digest"], image["config_digest"]} | ||
| accepted.update(row["digest"] for row in image["layers"]) |
There was a problem hiding this comment.
The verifier treats the image config digest and every layer digest as valid in-toto subjects, so an SBOM or provenance statement describing only one layer can still set oci_attestation_subjects_verified and integrity_verified to true. When an attestation descriptor points at the application image but its statement names a config or layer, the image-level qualification is unsupported; each statement subject should be bound to the application manifest digest itself.
Useful? React with 👍 / 👎.
| not (r.get("path") in allowed_posts or | ||
| (isinstance(r.get("path"), str) and | ||
| r["path"].startswith(prefix) and | ||
| r["path"][len(prefix):] not in ("", "0000"))))] |
There was a problem hiding this comment.
If the packaged bridge submits a nonempty but illegal move such as /move/e7e5 on White's first turn, the fake server records status 400 without adding a forbidden attempt, yet this predicate does not put it in bad_http. If the bridge subsequently recovers and completes the game, ready can become true, and the aggregate verifier repeats the same permissive suffix check, so RELEASE-1 can qualify evidence containing an illegal POST; reconstruct these attempts against board/intent state or at minimum reject unsuccessful move responses.
Useful? React with 👍 / 👎.
… exact metadata digest
… malformed predicates
… failed move POSTs
…move from trace and journal
…e aggregate admission
…P evidence witnesses
|
@codex review Please review the new exact head The OCI export now carries an explicit local image tag; bounded diagnostic JSON is retained before strict verifier admission. RELEASE-1 host aggregate independently recomputes archive SHA-256 and rejects positive producer booleans without independently verified evidence. Please review OCI attestation subject/version semantics, source/trace trust boundaries, account-wide R10 cooldown and R02 lost acknowledgment, and whether new evidence schema can false-qualify. Check new adversarial tests and any remaining PR #66–#69 review debt. Do not authorize online play, public artifact distribution, or an engine strength claim. This PR remains DRAFT while exact-head full integration runs. |
PR #71 — Implementation update (draft; not merge-ready)Current exact head: Implemented:
Verified so far: RELEASE-1 fast contracts 51/51 passed at this exact head; controller shell validation success. Other exact-head workflows and final Codex code/security review remain pending. Keep draft, do not merge or promote artifacts. Non-grants remain unchanged: no OS/wheel lock approval, licensing authorization, destination host, real credentials, public Lichess transport, public combined artifact distribution, or strength claim. |
|
@codex review Final exact-head request for draft PR #71: RELEASE-1 fast contracts are 51/51 green at this head; heavy exact-head workflows are pending, not yet claimed green. PR remains draft; no live/public release authority. Please report all actionable defects before merge. |
…ot counterexamples
…fore executing them Run independent trusted workflow checks before dynamic imports, in addition to the aggregate's manifest-and-Git sealed-module snapshot.
…s and sealed-source workflow Extend PR71 finding ledger and operator documentation; assert that CI verifies verdict-bearing modules before import.
…A256 hashing Freeze upstream scanner commit d88056b4, its vendored Syft v1.42.3 and a single exact-source patch. Compile twice using pinned Go 1.25.8 and vendored deps, compare static binaries, and forbid promotion until generated identities are reviewed.
…eparate digest-approval gate Compile the exact-source scanner, double-build its scratch image, use a digest-addressed temporary local registry on the BuildKit network, and refuse promotion until independent frozen image/binary digests match.
…through local immutable registry Pin Go action, compile and verify scanner in both jobs, bind each BuildKit daemon to the isolated registry, require independent digest review after real SPDX/SLSA preflight and keep scanner artifacts private.
… local image identity Remove the SHA256-only public scanner reference, extend immutable stage and aggregate source inventories to all new scanner inputs, and test exact-head job usage.
…source-locked Go upgrade avoidance Explain v1.11.0 SHA1 producer fix, reproducible binary/image preflight, local registry confinement, reviewed digest pin and independent scanner tests.
… BuildKit Use the same controlled registry-connected builder for both independent scratch image exports with SOURCE_DATE_EPOCH and rewrite-timestamp=true, and retain safe pairwise digest evidence even if they differ.
…nned Ubuntu file bytes Independently extract OCI-attested Syft file checksums and compare against an actual regular package-owned dpkg executable, rejecting placeholder or schema-only hashes.
…tu executable Add independent host content-hash witness after real scanner preflight, attest the verifier source, retain only bounded verdicts and preserve separate scanner digest approval.
…nary and OCI manifest Bind approved scanner identities to exact-source real Syft+SLSA positive run 39130571069. Require future independent rebuild and real known-file SHA1/SHA256 proof before full RELEASE-1 qualification.
|
@codex review FINAL EXACT-HEAD boarding review — draft PR #71, SHA
Please perform a comprehensive security + code review of the whole merged-effect evidence graph (not just newly changed predicates), especially: source-verified dynamic imports and TOCTOU; scanner patch/Git/source/binary/manifest provenance, disposable registry and BuildKit image-reference identity; SPDX license grammar/SHA1/SLSA and file-hash authenticity; recovery HTTP request/response/actor/ply/SQLite evidence, contract status semantics, and publication-safe diagnostic reports; no source-closure or workflow-trigger regressions. Please reconcile the outstanding current inline P2 threads on this draft, distinguishing source-corrected findings from any surviving executable counterexamples. No automatic dismissal solely because a comment is old, and no implicit release/licensing/live bot authority. This is a review request, not authorization to merge or deploy. If nothing actionable remains, confirm the exact reviewed SHA. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
HMarcusWH
left a comment
HMarcusWH
left a comment
There was a problem hiding this comment.
Independent exact-head code/security review — CHANGES REQUIRED
Reviewed SHA: 18b5e1f2fd65f7d0cc2125f7a4e3b7129db12920 (PR #71; 70 changed files; 11/11 CI workflow groups green). I reviewed the PR diff, the executed source/OCI/SLSA/SPDX/recovery verifier paths, source-attestation order, historical actionable Codex comments, and genuine CI evidence. The prior P2s appear source-corrected, but I found the following new positive-verdict gaps, so I cannot approve or merge this head.
-
[P1 — full OCI provenance identity]
scripts/verify-release-oci.py:155-156,402-457,646-714:verify_attestation_subjectsadmits any structurally valid SLSA v0.2 or v1 and any syntactically valid builder URI. The finaloci_provenance_verifiedandimage_integrity_qualifiedcan therefore be true for a rehashed application OCI archive whose provenance is v0.2 or a different builder.qualification/release-image-v1.jsonrequires v1 and the exact workflow builder ID;scripts/check-release-scanner.pyenforces this only on a separate miniature scanner probe. Fix the full OCI admission to bind the actual exported artifact to that frozen version/ID, and add rehashedslsa_case="other-builder"andslsa_case="v02"full-archive negatives. -
[P1 — OCI verifier executes before source-admission]
.github/workflows/release-candidate.yml:263-319:scripts/verify-release-oci.pyand its dynamically loaded SPDX parser run beforeVerify every offline verdict module before aggregate execution. A worktree module modified after candidate staging could emit forged positive OCI evidence, be restored before the later Git-byte check, and escape the aggregate becausescripts/qualify-release-aggregate.pychecks the report booleans + archive digest rather than independently rerunning OCI verification from the sealed snapshot. Require exact Git-byte checks before this invocation, preferably execute sealed verified bytes and test an altered-then-restored verifier. -
[P2 — ONLINE-4B phase-log evidence substitution / traversal]
scripts/qualify-online-recovery-v2.py:473-488: the independent qualifier checks onlylen(logging)==len(phases)and caller-suppliedrow["path"]hashes. It does not enforcerow["phase"]==index+1, distinctbridge-phase-{phase}.lognames, or containment of resolved regular files under the case folder. A two-phase forged result can reuse one phase's log twice, or point at an absolute /..// symlink-escaped file and supply its digest. Bind exact producer-defined names/phase indices and reject path escapes; add duplicates/traversal/symlink negative tests. -
[P2 — ONLINE-4A replay symlink escape]
scripts/qualify-online-recovery.py:332-350: checking".." not in Path(path).partsis insufficient whencase_dir/replays, a manifest orroute.jsonis a symlink. Require non-symlink, regular files and resolved-path containment for replay manifests and routes; add external-symlink evidence mutations. -
[P2 — ONLINE-4A/4B source modules imported before byte verification]
scripts/qualify-online-recovery-v2.py:23-40,552-556,scripts/qualify-online-recovery.py:23-30,549-553, and the corresponding independent workflow jobs: local verifier helpers are imported and the v1 moduleexec_moduleruns beforeverify_committed_bytes. The workflow'sgit rev-parse HEADcheck does not detect working-tree mutations. Compare committed bytes before executing/importing source and load verified modules from an immutable/snapshotted closure; exercise post-checkout tampering.
Independent positive evidence: exact-head RELEASE-1 189/189 tests, reproduced pinned Syft scanner binary+OCI digest, genuine Ubuntu file SHA1/SHA256, OCI/smoke/SQLite/chess, LOCAL-1 and offline recovery workflows all passed; no evidence of actual compromise. These remaining gaps are adversarial acceptance paths not covered by the current passing fixtures. ONLINE-4A remains diagnostic-only, and all public release, distribution, licensing, host, network and operator permissions stay FALSE.
Decision: CHANGES REQUESTED; KEEP DRAFT / DO NOT MERGE. Repair these contracts, rerun 11/11 CI on one new exact head, and re-review the changed files. Do not resolve old threads merely because they are outdated.
…ed OCI image Validate every image provenance against the frozen release predicate and workflow builder, reject duplicates, and add fully rehashed alternative-builder/v0.2 regression fixtures.
…tained paths Reject duplicate/misindexed ONLINE-4B phase logs, absolute or symlink-escaped paths, and ONLINE-4A replay manifest/route symlink traversal; add hermetic negative controls.
…xact-commit Git worktrees Source admission now precedes Python imports: reconstruct all three qualifier entrypoints from immutable Git bytes after mutable build jobs, verify complete recovery import closures before execution, and run RELEASE-1 smoke/OCI/aggregate from isolated trusted source.
…review counterexamples Add isolated-worktree tamper oracle, workflow ordering and source-closure tests for ONLINE-4A/4B and RELEASE-1, plus historical repair ledger and operations notes.
… verifier checkout
HMarcusWH
left a comment
HMarcusWH
left a comment
There was a problem hiding this comment.
Independent follow-up code/security sign-off — exact SHA 90e98e64208c1d3fac172ca62f3ee9fd909974e5
Result: no surviving actionable finding from the five blockers reported in review #5564010246. I inspected the fixed full-image OCI SLSA version/builder and singleton gates, the unchanged public-release restrictions, independent verifier source-worktree execution, ONLINE-4A replay path containment, ONLINE-4B unique per-phase log binding, and negative regression tests. The prior 17 non-outdated Codex threads remain historical review metadata and have been checked against current source; their identified acceptance paths were addressed in the repair chain.
CI evidence: 11/11 pull-request workflow groups successful on this SHA, including RELEASE-1 193/193 fast tests, reproducible pinned scanner, independent file SHA1/SHA256, full OCI/SLSA/SPDX and four packaged smoke cases, ONLINE-4A/4B independent recovery qualification, LOCAL-1, LC0, engine baselines, merge and publication prohibition gates.
Code merge only. offline_candidate_qualified=false; OS/wheel provenance locks, controller licensing, external source review, destination host, live transport, publisher signature, artifact distribution and operator approval remain expressly blocked. ONLINE-4A unsafe observations are diagnostic and do not certify client recovery or playing strength.
I support merging this exact SHA into main, not deploying or publishing it. GitHub may record this as COMMENTED because the connected account is the PR author; this is an independently reconstructed source/CI assessment, not a third-party or Codex approval.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
…ndent lock freeze criteria Anchor next milestone to merged PR #71, actual 120/13 package observations and no-deployment authority boundaries, preserving historical roadmap rows.
1 participant
Manual security-review repairs — DRAFT / DO NOT MERGE
Current code head:
90e98e64208c1d3fac172ca62f3ee9fd909974e5. Five commits after18b5e1f2fd65address the five counterexamples identified in manual review #5564010246.Implemented repairs (exact-head qualification pending)
CI: All 11 workflow groups have been triggered at the current head. No prior-head green status transfers to this commit; the manual code/security review of the repaired implementation is outstanding. The PR must remain draft and unmerged until every exact-head workflow succeeds and its remaining actionable findings are reconciled.
Authority:
public_bot_release=false,artifact_distribution_authorized=false; OS/wheel locks, legal/licensing, source-availability, host, live transport and operator deployment approvals remain closed. Merging code is not live deployment approval.INDEPENDENT SOURCE/SECURITY REVIEW — CHANGES REQUIRED / DO NOT MERGE
Head reviewed:
18b5e1f2fd65f7d0cc2125f7a4e3b7129db12920; 11/11 exact-head CI workflows green; PR remains draft and offline-only.Manual review: exact-head review #5564010246. GitHub recorded it as COMMENTED (reviewing as the PR author's own account cannot submit REQUEST_CHANGES).
New merge-blocking findings:
Required next gate: implement destructive tests for these acceptance paths, correct the independent verifiers and source execution order, rerun all 11 workflows on the resulting exact head, then repeat independent review before making the PR merge-ready.
Authority:
offline_candidate_qualified=false, and no production bot, public distribution, host, operator, license, live transport or publisher-signature approval has been granted.FINAL BOARDING GATE — DRAFT / DO NOT MERGE
Current PR head:
18b5e1f2fd65f7d0cc2125f7a4e3b7129db12920(base449ff7eb1d4728b6d2a6ec8d6e84a85d3bf4b685).CI status: 11/11 exact-head workflows successful, no failed or pending checks. GitHub reports
mergeable_state=clean.Confirmed exact-head green
Remaining merge blocker — code/security review
18b5e1f2fd65f7d0cc2125f7a4e3b7129db12920. Last completed Codex review covered the obsolete16faccaba5, not the present head.Codex request: final exact-head request
Blocked: Codex quota refusal — code-review usage limit reached. Do not treat a requested or rate-limited review as a completed review.
Explicit authority boundaries
This PR is an offline evidence/OCI/source-assurance code change, not deployment approval. RELEASE-1 intentionally still reports
offline_candidate_qualified=falsewith OS and wheel locks, controller/source licensing, destination host, live transport and operator approval remaining closed.public_bot_release=false,artifact_distribution_authorized=false, and publisher-signature claims remain false.For the complete historical repair ledger and proofs, see docs/PR71_ASSURANCE_REPAIR_LEDGER.md.
Earlier implementation narrative (historical; superseded head/CI statuses)
RELEASE-1 source-locked SHA1 scanner — DRAFT / DO NOT MERGE
Exact head:
91458a74023eac25d4c739aab7008c1569b6d591(base449ff7eb1d4728b6d2a6ec8d6e84a85d3bf4b685).Status: Six additional commits replace the historically pinned SHA256-only scanner with a source-locked v1.11.0 BuildKit/Syft generator, patched to emit SHA1 and SHA256. The new real-scanner job compiles Go 1.25.8 using vendored dependencies, checks two binary builds and two local OCI manifests, and consumes a digest-addressed scanner from a disposable job-local registry. Full integration recreates the same generator independently on a separate runner.
Exact-head RELEASE-1 fast contracts: 186/186 GREEN. Real scanner preflight is running; full OCI/smoke not yet qualified. Scanner binary and manifest digests in
qualification/release-scanner-v1.jsonremain null until reproducible candidate digests are independently reviewed and frozen. A measured digest is not self-approval. All other workflow conclusions must be checked at this exact SHA, followed by fresh Codex code/security review. No merge before then.Authority: Offline test only. No real Lichess credentials, production host, approved controller licensing, publisher signature, artifact distribution, strength certification or live bot deployment. All public release flags remain false.
October 18 repair wave
docs/PR71_ASSURANCE_REPAIR_LEDGER.mdupdated.Explicit non-grants
No real chess credentials, public bot, production network transport, host certification, authenticated publisher signature, approved source distribution, controller licensing, artifact distribution, playing-strength certification or live deployment.
Current repairs after prior assurance consolidation
docs/PR71_ASSURANCE_REPAIR_LEDGER.mdrecords all 58 historical inline threads (16 non-outdated at audit), required counterexamples, CI and final authority boundaries. No thread should be closed solely because its diff became outdated.Implemented bottom-up trust boundaries
qualification/assurance-contract-v1.jsonlists positive verdicts, necessary evidence and non-granted authority.scripts/verify_assurance_contract.pychecks impossible Boolean combinations, required online4 recovery witnesses, and all four independently qualified packaged games. Completing OS/wheel locks alone no longer accidentally setsoffline_candidate_qualified=true.scripts/verify_source_closure.pyfollows repository-local AST imports from both recovery producer/verifier entrypoints, accounts for declared dynamic imports and offline build inputs, compares against each frozen policy path list, and independently checks actual source bytes againstgit show exact_sha:path. Both v1/v2 policies now include the inherited recovery/log/fault dependencies and assurance helper.qualification/http-evidence-contract-v1.jsonseparates canonical request syntax from successful statuses. GET and nonmove POST must succeed; fault response 0/429/504 is permitted only on the matching trace-correlated HTTP move request for its scenario. ONLINE-4A classifies unexpected outcomes as unsafe observations; ONLINE-4B rejects unsupported guarded outcomes.allfatherversusscripted_opponenton every authoritative turn. Existing path/request/ply/board/journal/PGN verification preserved.qualification/schemas.scripts/sanitize_qualification_evidence.pyprovides recursive, bounded, deny-by-default serialization for host RELEASE-1/ONLINE-4A/ONLINE-4B verdicts; freeform errors, producer paths, noncanonical UCIs and raw targets are redacted or sanitized.tests/release/test_assurance_contract.py, OCI/scanner negative controls, ONLINE-4 status, source/import/actor and deep canary report tests. Real BuildKit/SPDX remains the positive control.Exact-head required validation (pending)
fa8f457437b62e8a8ac043406de2a9330366ab52Explicit non-grants
No real chess credentials, live transport, public bot, destination host authority, publisher signature, legal/licensing approval, OS/wheel lock promotion, artifact distribution, Elo claims or deployment. Both the PR and image remain offline-only, draft, unmerged and release-blocked.