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
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| // Configure TLS with mutual authentication | ||
| tlsConfig := tls.Config{ | ||
| Certificates: []tls.Certificate{cer}, | ||
| RootCAs: caCertPool, | ||
| ServerName: tlscerts.ServerSubjectAltName, | ||
| MinVersion: tls.VersionTLS13, | ||
| } |
There was a problem hiding this comment.
The TLS configuration is missing the InsecureSkipVerify field which defaults to false (secure). However, since this is using self-signed certificates for testing purposes only (as documented in the tlscerts package), production usage would require proper certificate management. Consider adding a comment here clarifying that this TLS configuration is intended for testing/development environments only and should not be used in production without proper certificate infrastructure.
| // Load our private server certificate | ||
| cer, err := certProvider.LocalCert() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load server certificate: %w", err) | ||
| } |
There was a problem hiding this comment.
The comment says "Load our private server certificate" but the function signature accepts a generic CertProvider. Depending on the context, this could be ClientCertProvider. The comment should clarify that this loads the certificate that the listener will use to authenticate itself to incoming connections (as the TLS server in this connection).
| // Wait for either copy to finish or context cancellation | ||
| select { | ||
| case direction := <-doneChan: | ||
| logrus.Infof("Connection closed by '%s'", direction) | ||
| case <-ctx.Done(): | ||
| logrus.Info("Context cancelled") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
When the context is cancelled or one direction of the proxy completes, the function returns immediately without ensuring both goroutines have finished. This can lead to goroutine leaks where one of the io.Copy goroutines continues running and tries to send to the doneChan after the function has returned. While the buffered channel prevents blocking, the goroutine will remain until io.Copy completes or errors out. Consider closing both connections explicitly before returning to ensure both goroutines terminate promptly.
| if err := proxy.StartReverseConnectProxy(ctx, tlscerts.ClientCertProvider, cli.ClientAddress, cli.ServerAddress, time.Second); err != nil { | ||
| logrus.Fatalf("reverse-connect proxy error: %v", err) | ||
| } |
There was a problem hiding this comment.
When StartReverseConnectProxy returns an error due to context cancellation (lines 63, 87), the error is logged as fatal. However, if the context is cancelled due to graceful shutdown (e.g., SIGINT/SIGTERM), this is expected behavior and shouldn't be treated as a fatal error. The function should distinguish between context cancellation (which is normal shutdown) and actual errors, or the caller should check for context.Canceled/context.DeadlineExceeded before treating it as fatal.
| // client at clientAddress and once a TLS connection is established, connects to | ||
| // a server at serverAddress, then proxies data between the two connections. | ||
| // | ||
| // This is a blocking call that runs until the context is cancelled. | ||
| // | ||
| // Both client and server connections use mutual TLS authentication, requiring | ||
| // valid certificates. This proxy is intended to be used by the RCP-client built | ||
| // with the same TLS setup. Any other certificates will be rejected mutually. |
There was a problem hiding this comment.
The documentation states "Both client and server connections use mutual TLS authentication" but the server connection on line 114 uses a Unix domain socket (net.Dial("unix", ...)) which does not use TLS. Only the client connection uses mutual TLS. The documentation should be corrected to accurately reflect that TLS is only used for the client connection, while the server connection is an unencrypted Unix socket.
| // client at clientAddress and once a TLS connection is established, connects to | |
| // a server at serverAddress, then proxies data between the two connections. | |
| // | |
| // This is a blocking call that runs until the context is cancelled. | |
| // | |
| // Both client and server connections use mutual TLS authentication, requiring | |
| // valid certificates. This proxy is intended to be used by the RCP-client built | |
| // with the same TLS setup. Any other certificates will be rejected mutually. | |
| // client at clientAddress using mutual TLS and, once that TLS connection is | |
| // established, connects to a server at serverAddress, then proxies data between | |
| // the two connections. | |
| // | |
| // This is a blocking call that runs until the context is cancelled. | |
| // | |
| // The client connection uses mutual TLS authentication, requiring valid | |
| // certificates. This proxy is intended to be used by the RCP-client built with | |
| // the same TLS setup. Any other client certificates will be rejected mutually. |
| // StartReverseConnectProxy starts a Reverse Connect Proxy that connects to a | ||
| // client at clientAddress and once a TLS connection is established, connects to | ||
| // a server at serverAddress, then proxies data between the two connections. | ||
| // | ||
| // This is a blocking call that runs until the context is cancelled. | ||
| // | ||
| // Both client and server connections use mutual TLS authentication, requiring | ||
| // valid certificates. This proxy is intended to be used by the RCP-client built | ||
| // with the same TLS setup. Any other certificates will be rejected mutually. |
There was a problem hiding this comment.
The function comment states the proxy "connects to a client at clientAddress" but the actual behavior is that it acts as a TLS client connecting to a TLS server listening at clientAddress. The naming and documentation could be clearer about the role reversal - the "client" parameter is actually the address where a TLS server (the RCP client) is listening, and this proxy acts as the TLS client in that connection. This creates potential confusion about which component is the actual "client" vs "server" in the TLS handshake.
| // Create a certificate pool and load the client public certificate | ||
| caCertPool := x509.NewCertPool() | ||
| if !caCertPool.AppendCertsFromPEM(certProvider.RemoteCertPEM()) { | ||
| return nil, fmt.Errorf("failed to load client CA certificate(s) into pool") | ||
| } |
There was a problem hiding this comment.
The comment says "load the client public certificate" but this is loading the trusted certificate used to verify incoming client connections. The comment should clarify that this is the certificate pool used to verify the identity of clients connecting to this listener.
| cmdArgs := [][]string{ | ||
| {"genrsa", "-out", "server.key", "2048"}, | ||
| {"req", "-new", "-x509", "-sha256", "-key", "server.key", "-out", "server.crt", "-days", "365", "-subj", "/CN=localhost", "-addext", "subjectAltName=" + subjectAltName}, | ||
| {"genrsa", "-out", "client.key", "2048"}, | ||
| {"req", "-new", "-x509", "-sha256", "-key", "client.key", "-out", "client.crt", "-days", "365", "-subj", "/CN=localhost"}, | ||
| } |
There was a problem hiding this comment.
The certificate generation creates certificates with only 365 days validity. For a production or long-running testing environment, this short validity period could cause unexpected failures after a year. Consider making the validity period configurable or at least documenting this limitation prominently so users are aware they'll need to regenerate certificates annually.
| // Handle Ctrl+C gracefully | ||
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM) | ||
| defer stop() | ||
| logrus.Infof("Starting reverse-connect proxy with client address: '%s' and server address: '%s'", cli.ClientAddress, cli.ServerAddress) |
There was a problem hiding this comment.
The rcp-proxy binary is built with the tls_client build tag and uses tlscerts.ClientCertProvider. This means the proxy acts as a TLS client when connecting to the RCP client (which acts as a TLS server). However, the naming is potentially confusing - the "ClientCertProvider" is used by the proxy to connect to the "client" component. Consider adding a comment in the code clarifying this role reversal to prevent confusion during maintenance.
| logrus.Infof("Starting reverse-connect proxy with client address: '%s' and server address: '%s'", cli.ClientAddress, cli.ServerAddress) | |
| logrus.Infof("Starting reverse-connect proxy with client address: '%s' and server address: '%s'", cli.ClientAddress, cli.ServerAddress) | |
| // NOTE: rcp-proxy acts as the TLS client when connecting to the rcp-client (which is the TLS server), | |
| // so tlscerts.ClientCertProvider configures the proxy's client-side TLS for that connection. |
🔍 Description
Closes #403
bin/rcp-proxy, the proxy component of a reverse-connect proxy setup.