chore: port node launcher to a rust binary

[DSharifi]

No description provided.

[DSharifi]

@claude review

[claude]

Code Review: chore: port node launcher to a rust binary

Issues Found

• Panicking on external input in validate_image_hash (main.rs ~line 384): The .expect("is valid digest") call parses the output of docker inspect — external process output that could be malformed, truncated, or empty. This violates the project's "Don't panic" standard as it's not a dead code path guaranteed by invariants. Should return an error instead:

  let pulled_digest: DockerSha256Digest = String::from_utf8_lossy(&inspect.stdout)
      .trim()
      .to_string()
      .parse()
      .map_err(|e| ImageDigestValidationFailed::DockerInspectFailed(
          format!("failed to parse inspect digest: {e}")
      ))?;

• Blocking sync I/O in async context (main.rs): validate_image_hash is an async fn but calls Command::new("docker").output() synchronously (docker pull + docker inspect), which blocks the tokio runtime thread. These should use tokio::process::Command instead, or be wrapped in tokio::task::spawn_blocking.

• Other expect calls in production paths (main.rs): While some are more defensible than the one above, the following expect calls are in production (non-test) code and could be replaced with proper error handling per project standards:

  • .expect("bearer token received from docker auth is a valid header value") — a malformed token from the registry could panic the launcher.
  • .expect("re-serializing a toml::Table always succeeds") — this one is arguably safe (exception 3 in CONTRIBUTING.md), but a map_err would be more consistent.

• Bearer token in Debug output (docker_types.rs): DockerTokenResponse derives Debug and Serialize, which means the bearer token could be exposed in logs (e.g., via tracing::debug!(?token_response, ...)). Consider using a wrapper type that redacts the token in Debug/Display, or at minimum removing Serialize from the struct since it's only used for deserialization.

Minor / Non-blocking

• The get_manifest_digest function is ~100 lines and mixes HTTP client setup, retry logic, and manifest resolution. Consider extracting the retry-wrapped HTTP call into a helper for readability (not blocking, just a suggestion).

• Config files (deployment/localnet/tee/frodo.toml, deployment/testnet/frodo.toml, etc.) contain secret_store_key_hex and backup_encryption_key_hex with placeholder/test values. These are clearly non-production configs committed for dev/test purposes, but worth noting they exist.

⚠️ Issues found — the expect on external docker output and blocking sync I/O in async context should be addressed before merge.

[claude]

Code Review: chore: port node launcher to a rust binary

The previous review flagged several issues. The code has evolved significantly since then (176 commits), but some of those issues persist in the final state and a few new ones are present.

Issues that should block merge

• Panicking on external docker output (main.rs:471-475): The .expect("is valid digest") on the output of docker inspect is still present. This parses external process output that could be malformed. Per CONTRIBUTING.md "Don't panic" — this should return an error instead.

• Blocking sync I/O in async context (main.rs:438-475, main.rs:538-548): validate_image_hash is async fn but calls std::process::Command::new("docker").output() synchronously (docker pull, docker inspect). launch_mpc_container similarly uses blocking Command calls and is invoked from the async run(). These block the tokio runtime thread. Use tokio::process::Command or wrap in spawn_blocking.

• .expect("bearer token received from docker auth is a valid header value") panics on malformed registry response (main.rs:305-307): A malicious or broken registry returning a token with non-ASCII bytes would crash the launcher. This parses external input and should use map_err to return LauncherError::RegistryAuthFailed.

• Bearer token exposed through Debug and Serialize on DockerTokenResponse (docker_types.rs:5-8): The struct derives both Debug and Serialize, meaning the bearer token could leak into logs via tracing::debug!(?token_response, ...) or serialization. Remove Serialize (it is only used for deserialization) and either remove Debug or wrap token in a redacting newtype.

• No timeout on auth token request (main.rs:284-288): The initial reqwest_client.get(token_url).send().await has no timeout, unlike the subsequent manifest requests which use config.rpc_request_timeout_secs. A hanging registry auth endpoint would block the launcher indefinitely. Add .timeout(...) to the token request.

⚠️ Issues found — the expect on external docker output, blocking sync I/O in async context, potential token leakage via Debug/Serialize, panic on malformed bearer token, and missing timeout on auth request should be addressed before merge.

[claude]

PR title type suggestion: This PR primarily involves restructuring the launcher implementation from Python to Rust (new crates/tee-launcher/ crate). Since the main changes are source code refactoring, the type prefix should probably be refactor: instead of chore:.

Suggested title: refactor: port node launcher to a rust binary

[claude]

PR title type suggestion: This PR ports the launcher from Python to Rust by creating new source code in crates/tee-launcher/src/. Since the behavior is being preserved while the code is restructured, the type prefix should probably be refactor: instead of chore:.

Suggested title: refactor: port node launcher to a rust binary

[netrome]

Started reviewing this. We should not define so much logic in main.rs. These files should be minimal imo.

[netrome]

This is way too much logic for a main.rs file. Ideally, main.rs should do minimal work and just call the main entrypoint defined in lib.rs to make sure it's easy to test and reuse the crate.