Conversation
Code ReviewOverall: Well-structured Rust replacement for the Python launcher with good test coverage and security hardening (read-only FS, no-new-privileges, typed digest validation). A few items to address: Issues
Minor / Non-blocking
✅ Approved — the |
bd8a122 to
32d01bb
Compare
Add a Rust implementation of the TEE launcher for running MPC nodes inside TDX CVMs. The launcher validates MPC image hashes against the contract's approved list, extends RTMR3, writes MPC node config to shared volume, and launches the node via Docker Compose. Pin reqwest to 0.12 with bundled webpki-roots for reproducible builds (reqwest 0.13 uses rustls-platform-verifier which requires system CA certs not present in the minimal launcher Docker container). Deployment configs, CI, test assets, and scripts will follow in separate PRs.
32d01bb to
5201ba0
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new Rust-based tee-launcher binary crate intended to replace the legacy Python TEE launcher for running MPC nodes in TDX CVMs, including image-hash validation and TEE attestation event emission.
Changes:
- Introduces
crates/tee-launcher/with CLI/env parsing, TOML config loading, registry manifest resolution, digest validation, and docker-compose launch logic. - Adds templates for non-TEE and TEE docker-compose manifests rendered at runtime.
- Registers the crate in the workspace and locks dependencies (including a pinned
reqwest0.12 + rustls-tls).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tee-launcher/src/main.rs | Core launcher flow: config load, hash selection/validation, RTMR3 event emission, docker-compose rendering and launch |
| crates/tee-launcher/src/types.rs | CLI + TOML config types and unit tests |
| crates/tee-launcher/src/error.rs | Error types for launcher and digest validation |
| crates/tee-launcher/src/docker_types.rs | Registry API response types and unit tests |
| crates/tee-launcher/src/constants.rs | Runtime file/socket/container constants |
| crates/tee-launcher/mpc-node-docker-compose*.template.yml | docker-compose templates for TEE vs non-TEE |
| crates/tee-launcher/README.md | Operator/developer documentation for config and usage |
| crates/tee-launcher/Cargo.toml | New crate manifest (incl. pinned reqwest + feature gate for external tests) |
| Cargo.toml | Adds crates/tee-launcher to workspace members |
| Cargo.lock | Adds lock entries for the new crate and its dependency graph |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6fd38b3 to
45f12c8
Compare
- Only fall back to DEFAULT_IMAGE_DIGEST on NotFound; return error on other I/O failures (permission denied, disk errors) since they could indicate a security issue - Replace .expect() with error return on docker inspect parse failure - Add timeout to registry auth token request - Only retry transient errors (timeouts, connect, 5xx); fail fast on 4xx - Fix error messages: "docker run" → "docker compose up" - Fix README: [mpc_config] → [mpc_node_config], wrong feature flag, mark fields as required (no serde defaults in code) - Fix field doc comments: remove stale env var references - Fix test comment: mpc_config → mpc_node_config
45f12c8 to
522758a
Compare
gilcu3
left a comment
There was a problem hiding this comment.
Shallow first pass of reviews.
A few of the comments might be blockers, like using reqwest 0.12 for example
crates/tee-launcher/Cargo.toml
Outdated
| # Pin reqwest 0.12 with bundled webpki-roots for reproducible builds. | ||
| # The workspace uses reqwest 0.13 which defaults to rustls-platform-verifier | ||
| # (loads CA certs from the system), but the launcher Docker image is a minimal | ||
| # container without system CA certs. Using rustls-tls bundles Mozilla's root | ||
| # certs into the binary, making TLS work without any system dependencies. | ||
| reqwest = { version = "0.12.28", default-features = false, features = ["rustls-tls", "json"] } |
There was a problem hiding this comment.
I don't see why depending on system tls is a problem for reproducibility. All TLS certificates eventually are rejected anyway. Therefore I think we must use 0.13 (recently upgraded in the node), with some feature change if needed
There was a problem hiding this comment.
when I used 0.13 I got a panic at launcher startup:
thread 'main' panicked at
/usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/reqwest-0.13.2/src/async_impl/client.rs:2478:38:
Client::new(): reqwest::Error { kind: Builder, source: General("No CA certificates were loaded from the system") }
This happened because reqwest 0.13's default TLS backend (rustls-platform-verifier) calls into the OS to find CA
certificates. Inside the minimal Docker container there are none, so it panics when trying to construct the HTTP
client — before it even makes any request.
If I add the ca-certificates package contents it may vary across builds - not sure this we can support reproducible builds
There was a problem hiding this comment.
I believe it can (achieve reproducibility with CA certificates in the OS), we use it for the mpc-node for example. Also we don't need to use reqwest default tls backend, they offer several
There was a problem hiding this comment.
I'm not sure about this, will need to look into it a bit more. is it ok by you to create a follow-up issue for this?
There was a problem hiding this comment.
I will give it a try tomorrow and merge here if I manage to fix this
There was a problem hiding this comment.
just pushed a fix for this in d41dc42 It includes a new Dockerfile which should make this solution testable, let me know if it works
- Validate image_name contains only safe characters [a-zA-Z0-9/_.-] to prevent YAML injection when substituted into compose templates - Redact bearer token in DockerTokenResponse Debug impl to prevent accidental leakage to logs
- Move compose templates to assets/ folder - Match specific errors in tests instead of broad Err(_) - Use test-release profile in README test commands
84514ff to
6befaf0
Compare
crates/tee-launcher/assets/mpc-node-docker-compose.tee.template.yml
Outdated
Show resolved
Hide resolved
crates/tee-launcher/assets/mpc-node-docker-compose.template.yml
Outdated
Show resolved
Hide resolved
| | Variable | Required | Description | | ||
| |----------|----------|-------------| | ||
| | `PLATFORM` | Yes | `TEE` or `NONTEE` | | ||
| | `DOCKER_CONTENT_TRUST` | Yes | Must be `1` | |
There was a problem hiding this comment.
this one is a bit weird. Why does the user need to set it if it is always 1? The other are user choices, this one is a requirement so technically should be in the section where we explain how to run this
There was a problem hiding this comment.
maybe it is not explained well enough those are value the launcher sets, not the user
There was a problem hiding this comment.
ahh I thought the user sets them as well. For example, the PLATFORM is user set no?
There was a problem hiding this comment.
or rather,
DOCKER_CONTENT_TRUST - should not be touched,
but PLATFORM and DEFAULT_IMAGE_DIGEST can be changed.
There was a problem hiding this comment.
this is an example Dockerfile I added that should contain everything needed and be reproducible.
| // the expected config digest. On native Linux, `.ID` returns the config digest | ||
| // (sha256 of the image config blob), but on macOS, Docker Desktop's containerd | ||
| // image store returns the manifest digest instead, causing a spurious mismatch. | ||
| #[cfg(target_os = "linux")] |
There was a problem hiding this comment.
indeed, really painful difference
There was a problem hiding this comment.
hhm, we may need to add this to the operator guide.
netrome
left a comment
There was a problem hiding this comment.
Shallow review. Mostly nits, created follow-ups so we can tackle these separately.
crates/tee-launcher/src/main.rs
Outdated
| let rendered = template | ||
| .replace("{{IMAGE_NAME}}", image_name) | ||
| .replace("{{IMAGE}}", &manifest_digest.to_string()) |
There was a problem hiding this comment.
This feels very hacky. Why don't we use a proper templating engine like Askama? This would give us better type safety when rendering the templates https://docs.rs/askama/latest/askama/
Summary
crates/tee-launcher/— Rust binary that replaces the Python launcher for running MPC nodes in TDX CVMsThe Python launcher in
tee_launcher/is preserved and will be removed separately in #2615.Deployment configs, CI, test assets, and localnet scripts will follow in separate PRs.
Closes #2622
Split from #2618.
Test plan
cargo check --locked -p tee-launchercompiles