infra: Add RCP proxy binary and client library #449
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure for a reverse-connect proxy (RCP) system that enables secure communication between a proxy and client over TLS. The proxy component connects to a client listener, then forwards traffic between the client and a Unix socket server.
Key changes:
- Implements TLS certificate generation and embedding for mutual TLS authentication
- Adds
rcp-proxybinary that connects to an RCP client and forwards traffic to a Unix socket - Provides a client library for accepting reverse connections from the proxy
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
tools/pkg/rcp/tlscerts/generate.go |
Standalone tool to generate self-signed TLS certificates for testing |
tools/pkg/rcp/tlscerts/certs.go |
Common functions to access embedded certificate data |
tools/pkg/rcp/tlscerts/keyserver.go |
Server-side certificate loading (build tag: tls_server) |
tools/pkg/rcp/tlscerts/keyclient.go |
Client-side certificate loading (build tag: tls_client) |
tools/pkg/rcp/tlscerts/.gitignore |
Excludes generated certificate files from git |
tools/pkg/rcp/proxy/proxy.go |
Implements reverse-connect proxy logic with TLS and bidirectional traffic forwarding |
tools/pkg/rcp/client/listen.go |
Client library for accepting TLS connections from the proxy |
tools/pkg/rcp/harpoon.go |
Defines default Unix socket path constant |
tools/cmd/rcp-proxy/main.go |
CLI entry point for the rcp-proxy binary |
.vscode/settings.json |
Adds build tags for Go tooling support |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…to user/frhuelsz/grpc/rcp-proxy
| if os.IsNotExist(err) || stat.IsDir() { | ||
| logrus.Infof("File '%s' does not exist or is a directory", file) | ||
| return false | ||
| } |
There was a problem hiding this comment.
The checkAllFilesExist function may panic if os.Stat returns an error other than IsNotExist (e.g., permission denied). When stat returns an error that is not IsNotExist, the stat variable will be nil, but the code still attempts to call stat.IsDir() on line 88, which will cause a nil pointer dereference.
Consider checking if err is not nil before using the stat variable, similar to the pattern used in cleanCertFiles.
| if os.IsNotExist(err) || stat.IsDir() { | |
| logrus.Infof("File '%s' does not exist or is a directory", file) | |
| return false | |
| } | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| logrus.Infof("File '%s' does not exist", file) | |
| return false | |
| } | |
| logrus.WithError(err).Warnf("Failed to stat %q", file) | |
| return false | |
| } | |
| if stat.IsDir() { | |
| logrus.Infof("File '%s' is a directory", file) | |
| return false | |
| } |
| tlsConfig := tls.Config{ | ||
| Certificates: []tls.Certificate{cer}, | ||
| RootCAs: caCertPool, | ||
| ServerName: tlscerts.ServerSubjectAltName, |
There was a problem hiding this comment.
The TLS configuration does not set MinVersion, which means it may accept older, potentially insecure TLS versions. For security-sensitive applications involving mutual TLS authentication, it's recommended to explicitly set a minimum TLS version (e.g., tls.VersionTLS12 or tls.VersionTLS13) to prevent downgrade attacks.
| ServerName: tlscerts.ServerSubjectAltName, | |
| ServerName: tlscerts.ServerSubjectAltName, | |
| MinVersion: tls.VersionTLS12, |
| select { | ||
| case direction := <-doneChan: | ||
| logrus.Infof("Connection closed by '%s'", direction) | ||
| case <-ctx.Done(): | ||
| logrus.Info("Context cancelled") | ||
| } |
There was a problem hiding this comment.
The proxy establishes two goroutines for bidirectional copying but only waits for one to finish via a single channel receive. When one direction completes, the function returns immediately without waiting for the other goroutine to finish, which could lead to a goroutine leak. Additionally, both connections are closed via defer, but this happens immediately when the function returns, potentially interrupting the second goroutine while it's still copying data.
Consider using a WaitGroup or waiting for both goroutines to complete before returning, and ensure proper connection shutdown (e.g., using CloseWrite/CloseRead if needed) to signal the other goroutine to finish gracefully.
| select { | |
| case direction := <-doneChan: | |
| logrus.Infof("Connection closed by '%s'", direction) | |
| case <-ctx.Done(): | |
| logrus.Info("Context cancelled") | |
| } | |
| // Wait for both copies to finish or context cancellation | |
| completed := 0 | |
| for completed < 2 { | |
| select { | |
| case direction := <-doneChan: | |
| logrus.Infof("Connection closed by '%s'", direction) | |
| completed++ | |
| case <-ctx.Done(): | |
| logrus.Info("Context cancelled") | |
| return nil | |
| } |
| // This function blocks until the connection is closed or an error occurs. | ||
| func handleClientConnection(ctx context.Context, clientConn net.Conn, serverAddress string) error { | ||
| defer clientConn.Close() | ||
|
|
There was a problem hiding this comment.
The log message "Client connected from..." is duplicated here - it's already logged on line 93 in the caller function StartReverseConnectProxy before handleClientConnection is called. This creates redundant log entries for each client connection.
| case <-ctx.Done(): | ||
| // Context cancelled, exit loop | ||
| return ctx.Err() | ||
| } | ||
|
|
||
| continue |
There was a problem hiding this comment.
When a non-ECONNREFUSED error occurs during TLS dial, the error is logged but the retry logic continues without delay. This could cause a tight loop of connection attempts if there's a persistent error (e.g., TLS handshake failures, network issues). The continue statement on line 85 skips the retry delay that's only applied to ECONNREFUSED errors.
Consider adding a delay for all error cases, or at least for non-ECONNREFUSED errors, to prevent potential resource exhaustion from rapid retry loops.
| defer listener.Close() | ||
|
|
||
| logrus.Debugf("RCP-client listening on port %d", port) | ||
|
|
||
| // Create a sub-context to handle listener closure on context cancellation. | ||
| acceptCtx, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
|
|
||
| go func() { | ||
| // In the background, wait for context cancellation to close the | ||
| // listener. This is necessary because Accept() is a blocking call so we | ||
| // need to close the listener in parallel while the parent goroutine is | ||
| // waiting. Closing the listener will cause Accept() to return an error | ||
| // which we can handle appropriately. | ||
| <-acceptCtx.Done() | ||
| listener.Close() |
There was a problem hiding this comment.
The listener is closed twice: once in the goroutine when the context is cancelled (line 65), and once via the defer on line 50. If the context is cancelled, this will result in attempting to close an already-closed listener, which may cause a harmless but unnecessary error. While Go's Close() is generally idempotent, this pattern could be cleaner by using sync primitives to ensure single closure.
🔍 Description
Closes #403
bin/rcp-proxy, the proxy component of a reverse-connect proxy setup.