Uh oh!
There was an error while loading. Please reload this page.
feat(apex): establish authenticated runtime trust boundary - #3
feat(apex): establish authenticated runtime trust boundary#3kratosboom wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b7d33bd824
ℹ️ 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".
| EvidenceProvenance::TrustedExecution { receipt } => { | ||
| matches!(kind, EvidenceKind::Test | EvidenceKind::Ci) | ||
| && receipt.outcome() == ExecutionOutcome::Pass |
There was a problem hiding this comment.
A caller can run any successful local command—even /bin/true—and submit the receipt as EvidenceKind::Ci, because every passing TrustedExecutionReceipt is treated as authoritative for both Test and CI. This lets the Ready gate pass without authenticated CI, contradicting the new runtime-trust contract that external CI remains unavailable; restrict LocalCommand receipts to Test evidence or bind receipts to an authenticated CI executor/provider.
Useful? React with 👍 / 👎.
| Ok(TrustedExecutionReceipt { | ||
| executor: ExecutorKind::LocalCommand, | ||
| task_id: task_id.to_owned(), | ||
| task_revision, | ||
| source_revision: source.source_revision().to_owned(), |
There was a problem hiding this comment.
When the command or another process changes the worktree after the single pre-spawn is_current() check, the command can execute against content other than the verified commit while this code still emits a Pass receipt naming the old SHA. A command can even restore the files before exiting, defeating a simple post-run cleanliness check, so trusted execution should use an immutable checkout/snapshot rather than the mutable verified worktree.
Useful? React with 👍 / 👎.
| pub fn submit_trusted_execution_evidence( | ||
| &mut self, | ||
| principal: impl Into<String>, | ||
| receipt: &TrustedExecutionReceipt, | ||
| subject: &EvidenceSubject, | ||
| kind: EvidenceKind, | ||
| status: EvidenceStatus, | ||
| detail: impl Into<String>, | ||
| ) -> Result<(), OrchestrationError> { | ||
| self.require_subject(subject)?; | ||
| let role = match kind { | ||
| EvidenceKind::Ci => EvidenceRole::Ci, | ||
| _ => { | ||
| return Err(OrchestrationError::ProvenanceRequired); | ||
| } | ||
| }; | ||
| let principal = principal.into(); | ||
| if principal.is_empty() { | ||
| return Err(OrchestrationError::ProvenanceRequired); | ||
| } | ||
| let record = EvidenceRecord::with_provenance( | ||
| kind, | ||
| status, | ||
| subject.clone(), | ||
| detail, | ||
| EvidenceProvenance::TrustedSystem { | ||
| id: principal, | ||
| role, | ||
| }, | ||
| ) | ||
| .map_err(|_| OrchestrationError::ProvenanceRequired)?; | ||
| let record = EvidenceRecord::from_trusted_execution(kind, subject.clone(), detail, receipt) |
There was a problem hiding this comment.
The cross-crate agent/release flow introduced here has only per-module unit tests, leaving the full behavior untested: authenticate and restore a task snapshot, verify its Git source, execute a trusted test, submit its receipt, and confirm release decisions change when the source or receipt becomes invalid. Add an integration test covering that end-to-end boundary as required for agent-logic changes.
AGENTS.md reference: AGENTS.md:L112-L120
Useful? React with 👍 / 👎.
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
This newly introduced crate defines its new test module inline in the implementation file. Move it to a descriptive sibling *_tests.rs file and reference it with an explicit #[path = "..."] attribute.
AGENTS.md reference: AGENTS.md:L169-L178
Useful? React with 👍 / 👎.
| "apex-task-state", | ||
| "apex-evidence", | ||
| "apex-orchestrator", | ||
| "apex-runtime-trust", |
There was a problem hiding this comment.
This non-mechanical security change totals 1,445 changed lines and combines authenticated snapshot restoration, the Git/command trust crate, evidence provenance, and orchestrator wiring. Split it by landing the apex-task-state authenticated snapshot API and dependencies first, followed by apex-runtime-trust, evidence integration, and orchestrator wiring, so each trust invariant can be reviewed independently.
AGENTS.md reference: AGENTS.md:L125-L131
Useful? React with 👍 / 👎.
| let git_root = self.git(repository_root, &["rev-parse", "--show-toplevel"])?; | ||
| let git_root = | ||
| std::fs::canonicalize(git_root.trim()).map_err(|_| TrustError::NotGitRepository)?; |
There was a problem hiding this comment.
Converting git rev-parse --show-toplevel output through a lossy UTF-8 String and then calling trim() rejects valid Unix repositories whose paths contain non-UTF-8 bytes or end in whitespace/newlines. Handle the path as raw OS bytes or avoid round-tripping it through command output so valid platform paths remain verifiable.
AGENTS.md reference: AGENTS.md:L317-L322
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
| if !self | ||
| .git( | ||
| repository_root, | ||
| &["status", "--porcelain", "--untracked-files=all"], | ||
| )? |
There was a problem hiding this comment.
A clean git status does not prove that tracked on-disk bytes equal HEAD: skip-worktree or assume-unchanged index flags, sparse checkouts, and ignored submodule changes can hide modified or absent tracked content from this command. The verifier can therefore mint a VerifiedSource while the runner reads bytes not represented by source_revision; compare or hash the checked-out tracked content against the commit tree rather than relying on status alone.
Useful? React with 👍 / 👎.
| if let Some(status) = child | ||
| .try_wait() | ||
| .map_err(|error| TrustError::ExecutionFailed(error.to_string()))? | ||
| { | ||
| let outcome = if status.success() { |
There was a problem hiding this comment.
The polling loop checks try_wait() before checking the deadline, so a process that exits after its timeout but before the next 10 ms poll is reported as Pass rather than Unavailable; scheduler delays can widen this overrun further. Use a timeout-aware wait or otherwise ensure completion after the deadline cannot mint a successful trusted receipt.
Useful? React with 👍 / 👎.
| if let Some(status) = child | ||
| .try_wait() | ||
| .map_err(|error| TrustError::ExecutionFailed(error.to_string()))? | ||
| { |
There was a problem hiding this comment.
If try_wait() returns an OS error, execute propagates immediately, but dropping std::process::Child neither terminates nor waits for the process. The supposedly bounded command can therefore continue running after ExecutionFailed is returned; use a cleanup guard that kills and reaps the child on every error after a successful spawn.
Useful? React with 👍 / 👎.
STACKED ON PR #2
CLOSES RESIDUAL SECURITY FINDINGS #4#7#8
NO CODEX CORE INTEGRATION
NO PRODUCTION CHANGE
NO MERGE