docs(spec-minimal-verify): minimal verify: consumer-side SLSA provenance verification for cache-pulled artifacts - #460
Conversation
📝 WalkthroughWalkthrough
ChangesConnection Handshake Error Handling Refactor
minimal-verify Specification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/minimald/src/connection.rs (1)
138-154:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPanic on handshake failure creates denial-of-service vulnerability.
russh::server::run_streamreturns aResultbecause SSH handshakes can fail (malformed packets, protocol violations, incompatible algorithms). Using.unwrap()converts these recoverable errors into panics that propagate to the caller.In
server.rs, this is awaited directly in the accept loop without spawn isolation, so any connecting client that sends a malformed SSH handshake will crash the entire daemon.Recommend keeping the
Resultreturn type and handling failures at call sites, or at minimum isolating the handshake in a spawned task before the setup completes.Suggested fix: restore Result-based error propagation
pub(crate) async fn from_stream<S>( s: S, c: Arc<RuConfig>, serv: ServerStateHandle, is_local: bool, - ) -> (ConnectionHandle, RunningSession<ConnectionHandler>) + ) -> Result<(ConnectionHandle, RunningSession<ConnectionHandler>), ConnectionError> where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, { let h = ConnectionHandle(Arc::new(Mutex::new(Self { auth: if is_local { Auth::Local } else { Auth::Pending }, ssh_username: None, channels: BTreeMap::new(), serv, }))); - ( + Ok(( h.clone(), russh::server::run_stream(c, s, ConnectionHandler(h)) - .await - .unwrap(), - ) + .await?, + )) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/connection.rs` around lines 138 - 154, The `.unwrap()` call on `russh::server::run_stream` in the method that creates a ConnectionHandle will panic if the SSH handshake fails, crashing the entire daemon. Remove the `.unwrap() call and change the return type of this method from `(ConnectionHandle, RunningSession<ConnectionHandler>)` to `Result<(ConnectionHandle, RunningSession<ConnectionHandler>), Error>` (or appropriate error type). Propagate the Result returned by `russh::server::run_stream` instead of unwrapping it, allowing the caller in server.rs to handle handshake failures gracefully rather than panicking.crates/minimald/src/server.rs (1)
206-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winServer loop loses fault tolerance for handshake failures.
The previous implementation caught handshake errors, logged them, and continued accepting connections. Now, if
from_streampanics (which it will on anyrun_streamfailure), the panic occurs at line 208 before the spawned task, crashing the entire accept loop.The comment on line 209-210 about logging session errors only covers the spawned
session_fut—it does not protect against handshake-phase failures.If keeping the panic behavior in
from_stream, consider spawning the entire setup includingfrom_stream:Alternative: isolate handshake in spawned task
let (stream, peer) = listener.accept().await?; tracing::info!(?peer, transport = L::TRANSPORT, "accepted connection"); - let (_conn_hnd, session_fut) = - Connection::from_stream(stream, russh_config.clone(), state.clone(), L::IS_LOCAL) - .await; - // Log session errors instead of silently dropping the spawned - // future, so a failed handshake is visible on any transport. - session_set.spawn(async move { - if let Err(e) = session_fut.await { - tracing::warn!(error = %e, transport = L::TRANSPORT, "session ended with error"); + let russh_config = russh_config.clone(); + let state = state.clone(); + session_set.spawn(async move { + let (_conn_hnd, session_fut) = match Connection::from_stream( + stream, + russh_config, + state, + L::IS_LOCAL, + ) + .await + { + Ok(v) => v, + Err(e) => { + tracing::warn!(error = %e, transport = L::TRANSPORT, "handshake failed"); + return; + } + }; + if let Err(e) = session_fut.await { + tracing::warn!(error = %e, transport = L::TRANSPORT, "session ended with error"); } });This requires
from_streamto returnResultagain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/server.rs` around lines 206 - 215, The call to Connection::from_stream on line 208 can panic during handshake failures, crashing the entire accept loop before the spawned error handler task is created. Move the entire Connection::from_stream setup and session_fut handling into the spawned task that session_set.spawn creates, so that any handshake failures or panics are caught and logged without crashing the server loop. This ensures the accept loop remains fault-tolerant and continues accepting new connections even when individual handshakes fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/minimald/src/connection.rs`:
- Around line 138-154: The `.unwrap()` call on `russh::server::run_stream` in
the method that creates a ConnectionHandle will panic if the SSH handshake
fails, crashing the entire daemon. Remove the `.unwrap() call and change the
return type of this method from `(ConnectionHandle,
RunningSession<ConnectionHandler>)` to `Result<(ConnectionHandle,
RunningSession<ConnectionHandler>), Error>` (or appropriate error type).
Propagate the Result returned by `russh::server::run_stream` instead of
unwrapping it, allowing the caller in server.rs to handle handshake failures
gracefully rather than panicking.
In `@crates/minimald/src/server.rs`:
- Around line 206-215: The call to Connection::from_stream on line 208 can panic
during handshake failures, crashing the entire accept loop before the spawned
error handler task is created. Move the entire Connection::from_stream setup and
session_fut handling into the spawned task that session_set.spawn creates, so
that any handshake failures or panics are caught and logged without crashing the
server loop. This ensures the accept loop remains fault-tolerant and continues
accepting new connections even when individual handshakes fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 420310da-e443-4c7a-bd06-1f305b1df301
📒 Files selected for processing (5)
crates/minimald/src/connection.rscrates/minimald/src/guest.rscrates/minimald/src/server.rscrates/minimald/src/test_harness.rsdocs/specs/04-spec-minimal-verify/04-spec-minimal-verify.md
💤 Files with no reviewable changes (1)
- crates/minimald/src/guest.rs
Spec Validation CompleteBoundary: Spec (PR adds Gate Set Applied: Spec gates (acceptance criteria testable, no implementation leakage, assumptions explicit, proof artifacts present and behavioral) ✅ Gate 1: Acceptance Criteria Testable - PASSAll 30 requirements (R1.1–R6.5) across 6 demoable units are testable with observable pass/fail outcomes. Each R-ID specifies a checkable property:
|
| Gate | Result | Findings |
|---|---|---|
| 1. Acceptance criteria testable | ✅ PASS | 0 |
| 2. No implementation leakage | 6 instances | |
| 3. Assumptions explicit | ✅ PASS | 0 |
| 4. Proof artifacts present | ✅ PASS | 0 |
Overall Result: Spec validation complete with 1 Warning finding. The spec is well-structured with testable requirements, explicit assumptions, and comprehensive proof artifacts. The implementation leakage (file names, struct names, library choices) is pervasive but contextually justified for a cryptographic verification system where security properties depend on specific implementation choices.
Generated by sdd-validate for issue #460 · sonnet45 1.4M · ◷
This specification defines a new
minimal-verifycrate that implements SLSA provenance verification for cache-pulled artifacts, closing the L0 (envelope authenticity) verification gap.Summary
The spec establishes consumer-side cryptographic verification of SLSA Provenance v1 attestations with dual-signature support (ECDSA-P256 + post-quantum ML-DSA-65) using aws-lc-rs. It is the blocking prerequisite for transparency layers (L1 timestamps, L2 log inclusion) since those layers depend on envelope authenticity being verified first.
Demoable Units
.intoto.jsonlenvelopes and recompute PAE byte-exactlyminimal verifysubcommand with expectations, policy flags, output formatsKey Decisions
.intoto.jsonlfails verification.well-knownrequires meta-key firstNext Steps
Merging this pull request advances the tracking issue gominimal/inbox#348 from the spec phase into the architecture and triage phase. The architecture phase will detail the crypto library selection rationale, module boundaries, and upgrade safety mechanisms.
gominimal/inbox#348
Closes #459
Summary by CodeRabbit
Documentation
Refactor