Conversation
ameba23
marked this pull request as draft
October 1, 2026 10:03
ameba23
changed the title
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The broad HTTP/CLI refactor and security-sensitive tunnel lifecycle require final human review.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds protocol-independent TCP forwarding over attested TLS, addressing issue #54 while sharing infrastructure with the existing HTTP proxy.
Changes:
- Adds TCP tunnel library APIs and client/server commands.
- Refactors shared TLS, target validation, CLI handling, and logging.
- Adds transport, gRPC, lifecycle, and CLI tests with usage documentation.
| File | Description |
|---|---|
| README.md | Documents tunnel commands and logging. |
| crates/attested-tls/src/lib.rs | Separates scoped IPv6 routing from TLS identity. |
| crates/attested-tls-proxy/tests/tcp_tunnel/tunnel.rs | Tests transport, trust, limits, and shutdown. |
| crates/attested-tls-proxy/tests/tcp_tunnel/main.rs | Registers tunnel test modules. |
| crates/attested-tls-proxy/tests/tcp_tunnel/grpc.rs | Tests gRPC streaming and cancellation. |
| crates/attested-tls-proxy/tests/tcp_tunnel/common.rs | Provides shared tunnel test helpers. |
| crates/attested-tls-proxy/tests/tcp_tunnel/cli.rs | Tests commands, resource exhaustion, and signals. |
| crates/attested-tls-proxy/tests/http/target.rs | Tests target validation and Host preservation. |
| crates/attested-tls-proxy/tests/http/main.rs | Registers HTTP integration tests. |
| crates/attested-tls-proxy/tests/http/attested_get_redirect.rs | Covers redirect isolation and rejection. |
| crates/attested-tls-proxy/TCP_TUNNEL.md | Explains configuration, trust, and lifecycle. |
| crates/attested-tls-proxy/src/tls.rs | Centralizes TLS configuration. |
| crates/attested-tls-proxy/src/tcp_tunnel/mod.rs | Implements attested byte-stream forwarding. |
| crates/attested-tls-proxy/src/target.rs | Validates and normalizes destinations. |
| crates/attested-tls-proxy/src/self_signed.rs | Migrates tests to shared TLS helpers. |
| crates/attested-tls-proxy/src/main.rs | Simplifies startup, logging, and runtime shutdown. |
| crates/attested-tls-proxy/src/lib.rs | Exposes tunnel and TLS APIs. |
| crates/attested-tls-proxy/src/http/mod.rs | Adopts shared TLS and target handling. |
| crates/attested-tls-proxy/src/http/attested_get.rs | Uses shared client TLS configuration. |
| crates/attested-tls-proxy/src/cli/tcp_tunnel.rs | Implements tunnel command configuration. |
| crates/attested-tls-proxy/src/cli/pem.rs | Extracts PEM loading and tests. |
| crates/attested-tls-proxy/src/cli/mod.rs | Centralizes command dispatch and validation. |
| crates/attested-tls-proxy/src/cli/http.rs | Extracts existing HTTP command handling. |
| crates/attested-tls-proxy/src/cli/attestation.rs | Shares attestation verification setup. |
| crates/attested-tls-proxy/Cargo.toml | Adopts workspace dependency declarations. |
| Cargo.toml | Centralizes shared dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+731
to
+732
| #[error("Invalid target: {0}")] | ||
| InvalidTarget(#[from] InvalidTarget), |
Collaborator
Author
There was a problem hiding this comment.
I only care about CLI breaking changes, not library breaking changes. No one is currently depending on this as a library crate
Comment on lines
+382
to
+384
| let (mut local, mut remote) = tokio::time::timeout( | ||
| options.setup_timeout, endpoint.setup(inbound, &target), | ||
| ).await.map_err(|_| TunnelError::SetupTimeout)??; |
Collaborator
Author
There was a problem hiding this comment.
Agree. Its annoying that quote generation is synchronous as this would be a lot simpler to fix. Worth noting, that the recent bump of tdx-attest means better timeouts on quote generation. So its unlikely that quote generation will hang for very long.
I think this is a more of a general issue not specific to this PR. I will put a fix in a separate PR.
Comment on lines
+13
to
+17
| pub fn client_config( | ||
| identity: Option<&TlsCertAndKey>, | ||
| remote_certificate: Option<CertificateDer<'static>>, | ||
| allow_self_signed: bool, | ||
| ) -> Result<ClientConfig, AttestedTlsError> { |
Collaborator
Author
There was a problem hiding this comment.
Again, I only care about CLI breaking changes, not library breaking changes. No one is currently depending on this as a library crate
ameba23
added this pull request to stack #185
October 2, 2026 08:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds an additional simpler version of the proxy which does not care about application protocol - it just provides a byte-stream over an attested TLS tunnel.
I ended up making quite a big refactor to integrate this, and there are some behavioral improvements also to the http proxy to keep things common.
This makes it a bit hard to review. The actual TCP tunnel logic (the new part) is here: https://github.com/flashbots/attested-tls-proxy/blob/peg/tcp-tunnel-2/crates/attested-tls-proxy/src/tcp_tunnel/mod.rs
Closes #54