fix(security): 2 improvements across 2 files#2616
fix(security): 2 improvements across 2 files#2616tomaioo wants to merge 2 commits intoe2b-dev:mainfrom
Conversation
- Security: Insecure gRPC Transport Credentials in Node Manager Client - Security: Insecure gRPC Transport Credentials in Instance Client (Conditional) Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Insecure gRPC Transport Credentials in Node Manager Client - Security: Insecure gRPC Transport Credentials in Instance Client (Conditional) Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have @tomaioo on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53972aa35a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !endpointTLS { | ||
| return nil, fmt.Errorf("TLS is required for instance client connections") | ||
| } |
There was a problem hiding this comment.
This change makes createClient fail whenever endpointTLS is false, but local-cluster instance creation intentionally passes false (newLocalCluster calls newInstance(..., false) in packages/api/internal/clusters/cluster.go), so every local instance now fails at client construction instead of connecting over the direct node IP as before. In environments using the local cluster path, this prevents instance registration/sync and breaks routing to local orchestrators.
Useful? React with 👍 / 👎.
| func NewClient(tracerProvider trace.TracerProvider, meterProvider metric.MeterProvider, host string) (*clusters.GRPCClient, error) { | ||
| conn, err := grpc.NewClient(host, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})), |
There was a problem hiding this comment.
Switching this dial option to TLS breaks node-manager connectivity because discovered orchestrator targets are raw IP:port addresses and the orchestrator gRPC server is started without TLS credentials (grpc.NewServer via packages/shared/pkg/grpc/server.go), so this client now attempts a TLS handshake against a plaintext listener and cannot connect to nodes. That makes nodemanager.New fail to initialize node clients in normal local-orchestrator discovery flows.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The insecure import in packages/api/internal/clusters/instance_client.go is now unused and will cause a compilation error. Enforcing TLS by returning an error when endpointTLS is false is a breaking change that may cause failures in environments relying on insecure connections. The gRPC client in packages/api/internal/orchestrator/nodemanager/client.go is missing the WithAuthority option, which can lead to connection failures if the host does not match the certificate's server name.
| "go.opentelemetry.io/otel/trace" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/credentials/insecure" | ||
| "google.golang.org/grpc/credentials" |
There was a problem hiding this comment.
The removal of the insecure import in this file is correct, but the same import remains in packages/api/internal/clusters/instance_client.go where it is now unused. Because Go does not allow unused imports, this will cause a compilation error and break the build.
| if !endpointTLS { | ||
| return nil, fmt.Errorf("TLS is required for instance client connections") | ||
| } |
There was a problem hiding this comment.
Enforcing TLS by returning an error when endpointTLS is false is a breaking change that will cause failures in any environment or caller currently relying on insecure connections. If this enforcement is intentional, ensure that all callers and development environments have been updated to support TLS.
| func NewClient(tracerProvider trace.TracerProvider, meterProvider metric.MeterProvider, host string) (*clusters.GRPCClient, error) { | ||
| conn, err := grpc.NewClient(host, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})), |
There was a problem hiding this comment.
The TLS configuration is missing the WithAuthority option used in the instance_client.go reference. This inconsistency can lead to connection failures if the host string is an IP address or does not match the certificate's expected server name.
grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})),
grpc.WithAuthority(host),2 participants
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:packages/api/internal/orchestrator/nodemanager/client.go:L41The nodemanager client uses insecure.NewCredentials() for gRPC connections, which disables TLS encryption. This could allow man-in-the-middle attacks and expose sensitive sandbox metadata in transit. While this might be acceptable for internal cluster communication, it should be reviewed against security requirements.
Solution
If this is for internal cluster communication within a trusted network, document this decision. Otherwise, implement TLS with proper certificate validation. Consider using the same TLS configuration as in instance_client.go which uses TLS 1.2+ with proper certificate handling.
Changes
packages/api/internal/orchestrator/nodemanager/client.go(modified)packages/api/internal/clusters/instance_client.go(modified)