Conversation
garmr-ulfr
requested review from
Copilot and
myleshorton
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR introduces a new local HTTP proxy component (KindlingProxy) backed by the kindling HTTP client, starts it automatically as part of Radiance startup/shutdown, and registers it as an HTTP outbound option in the VPN (sing-box) configuration.
Changes:
- Added
kindling/proxy.goimplementing a local HTTP proxy server on127.0.0.1:14988. - Updated
radiance.goto create/start the proxy and close it during shutdown. - Updated
vpn/boxoptions.goto register the proxy as an HTTP outbound (kindling-proxy) pointing at127.0.0.1:${kindling.ProxyPort}.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| kindling/proxy.go | New Kindling-backed local HTTP proxy implementation. |
| radiance.go | Starts/stops the Kindling proxy as part of Radiance lifecycle. |
| vpn/boxoptions.go | Adds a new HTTP outbound option targeting the local Kindling proxy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| Type: C.TypeHTTP, | ||
| Tag: "kindling-proxy", | ||
| Options: &O.HTTPOutboundOptions{ | ||
| ServerOptions: O.ServerOptions{ | ||
| Server: "127.0.0.1", | ||
| ServerPort: kindling.ProxyPort, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This adds an HTTP outbound that points at the local Kindling proxy. For HTTPS/TLS traffic, sing-box (and most HTTP proxy clients) will use CONNECT tunneling, so the local proxy server must implement CONNECT correctly; otherwise selecting this outbound will break most traffic. Please ensure the proxy implementation supports CONNECT before wiring it in as an outbound option.
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Scheme == "" { | ||
| r.URL.Scheme = "https" | ||
| } | ||
| if r.URL.Host == "" { | ||
| r.URL.Host = r.Host | ||
| } | ||
|
|
||
| outReq, err := http.NewRequestWithContext(r.Context(), r.Method, r.URL.String(), r.Body) | ||
| if err != nil { |
There was a problem hiding this comment.
The proxy handler currently forwards all methods using http.Client.Do. This will not work for HTTP CONNECT requests (commonly used to tunnel TLS through an HTTP proxy): net/http will treat CONNECT as a normal request instead of hijacking and tunneling bytes, so HTTPS/proxied TCP traffic will fail. Add explicit CONNECT handling (hijack the client conn, dial r.Host, write a 200 response, then bidirectionally copy until close).
| } | ||
|
|
||
| func NewKindlingProxy(addr string) *KindlingProxy { | ||
| proxy := &KindlingProxy{ | ||
| server: &http.Server{ | ||
| Addr: addr, | ||
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Scheme == "" { | ||
| r.URL.Scheme = "https" | ||
| } | ||
| if r.URL.Host == "" { | ||
| r.URL.Host = r.Host | ||
| } | ||
|
|
||
| outReq, err := http.NewRequestWithContext(r.Context(), r.Method, r.URL.String(), r.Body) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadGateway) | ||
| return | ||
| } | ||
| outReq.Header = r.Header.Clone() | ||
|
|
||
| resp, err := HTTPClient().Do(outReq) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadGateway) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| for key, values := range resp.Header { | ||
| for _, value := range values { | ||
| w.Header().Add(key, value) | ||
| } | ||
| } | ||
| w.WriteHeader(resp.StatusCode) | ||
| io.Copy(w, resp.Body) | ||
| }), | ||
| }, |
There was a problem hiding this comment.
HTTPClient() is called for every proxied request. In this codebase HTTPClient() constructs a new kindling-backed http.Client each time, which can be expensive and may create many transports/connections under load. Store a single *http.Client on KindlingProxy (created once in NewKindlingProxy) and reuse it for Do calls.
| } | |
| func NewKindlingProxy(addr string) *KindlingProxy { | |
| proxy := &KindlingProxy{ | |
| server: &http.Server{ | |
| Addr: addr, | |
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.URL.Scheme == "" { | |
| r.URL.Scheme = "https" | |
| } | |
| if r.URL.Host == "" { | |
| r.URL.Host = r.Host | |
| } | |
| outReq, err := http.NewRequestWithContext(r.Context(), r.Method, r.URL.String(), r.Body) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusBadGateway) | |
| return | |
| } | |
| outReq.Header = r.Header.Clone() | |
| resp, err := HTTPClient().Do(outReq) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusBadGateway) | |
| return | |
| } | |
| defer resp.Body.Close() | |
| for key, values := range resp.Header { | |
| for _, value := range values { | |
| w.Header().Add(key, value) | |
| } | |
| } | |
| w.WriteHeader(resp.StatusCode) | |
| io.Copy(w, resp.Body) | |
| }), | |
| }, | |
| client *http.Client | |
| } | |
| func NewKindlingProxy(addr string) *KindlingProxy { | |
| proxy := &KindlingProxy{ | |
| client: HTTPClient(), | |
| } | |
| proxy.server = &http.Server{ | |
| Addr: addr, | |
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.URL.Scheme == "" { | |
| r.URL.Scheme = "https" | |
| } | |
| if r.URL.Host == "" { | |
| r.URL.Host = r.Host | |
| } | |
| outReq, err := http.NewRequestWithContext(r.Context(), r.Method, r.URL.String(), r.Body) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusBadGateway) | |
| return | |
| } | |
| outReq.Header = r.Header.Clone() | |
| resp, err := proxy.client.Do(outReq) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusBadGateway) | |
| return | |
| } | |
| defer resp.Body.Close() | |
| for key, values := range resp.Header { | |
| for _, value := range values { | |
| w.Header().Add(key, value) | |
| } | |
| } | |
| w.WriteHeader(resp.StatusCode) | |
| io.Copy(w, resp.Body) | |
| }), |
kindling/proxy.go
Outdated
| for key, values := range resp.Header { | ||
| for _, value := range values { | ||
| w.Header().Add(key, value) | ||
| } | ||
| } | ||
| w.WriteHeader(resp.StatusCode) | ||
| io.Copy(w, resp.Body) | ||
| }), |
There was a problem hiding this comment.
Response forwarding should filter hop-by-hop headers (e.g., Connection, Proxy-Connection, Keep-Alive, Transfer-Encoding, Upgrade, etc.) instead of copying all headers verbatim, otherwise clients can see incorrect connection semantics. Also io.Copy's error is ignored here; please handle it (and consider logging/aborting the request on copy failure).
| func NewKindlingProxy(addr string) *KindlingProxy { | ||
| proxy := &KindlingProxy{ | ||
| server: &http.Server{ | ||
| Addr: addr, | ||
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Scheme == "" { | ||
| r.URL.Scheme = "https" | ||
| } | ||
| if r.URL.Host == "" { | ||
| r.URL.Host = r.Host | ||
| } | ||
|
|
||
| outReq, err := http.NewRequestWithContext(r.Context(), r.Method, r.URL.String(), r.Body) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadGateway) | ||
| return | ||
| } | ||
| outReq.Header = r.Header.Clone() | ||
|
|
||
| resp, err := HTTPClient().Do(outReq) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadGateway) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| for key, values := range resp.Header { | ||
| for _, value := range values { | ||
| w.Header().Add(key, value) | ||
| } | ||
| } | ||
| w.WriteHeader(resp.StatusCode) | ||
| io.Copy(w, resp.Body) | ||
| }), | ||
| }, | ||
| } | ||
| return proxy | ||
| } |
There was a problem hiding this comment.
There are existing tests in kindling/ (e.g., client_test.go), but the new proxy has no coverage. Please add tests for the proxy behavior, especially CONNECT tunneling (and at least one plain HTTP request) to ensure the sing-box HTTP outbound integration won’t regress.
| go func() { | ||
| if err := r.kindlingProxy.ListenAndServe(); err != nil { | ||
| slog.Error("Kindling proxy stopped with error", "error", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
ListenAndServe returns http.ErrServerClosed on normal shutdown; logging that as an error will produce noisy/false error logs during clean app exits. Consider treating ErrServerClosed as expected (return without logging or log at debug level).
| slog.Error("Kindling proxy stopped with error", "error", err) | ||
| } | ||
| }() | ||
| r.addShutdownFunc(func(ctx context.Context) error { return r.kindlingProxy.Close() }) |
There was a problem hiding this comment.
Shutdown currently calls server.Close(), which aborts active connections immediately. If you intend a graceful shutdown (as described in the PR), prefer server.Shutdown(ctx) and pass through a context with a timeout from Radiance.Close so in-flight proxy requests can complete cleanly.
| r.addShutdownFunc(func(ctx context.Context) error { return r.kindlingProxy.Close() }) | |
| r.addShutdownFunc(func(ctx context.Context) error { return r.kindlingProxy.Shutdown(ctx) }) |
kindling/proxy.go
Outdated
| if r.URL.Scheme == "" { | ||
| r.URL.Scheme = "https" | ||
| } | ||
| if r.URL.Host == "" { | ||
| r.URL.Host = r.Host | ||
| } |
There was a problem hiding this comment.
Defaulting requests with an empty URL scheme to "https" can misroute origin-form proxy requests (which often omit scheme/host) and will break plain HTTP destinations. Consider deriving the scheme from the incoming request/headers (or default to "http"), and only rewrite when you can do so unambiguously.
| kindlingProxy: kindling.NewKindlingProxy(kindling.ProxyAddr), | ||
| shutdownFuncs: shutdownFuncs, | ||
| stopChan: make(chan struct{}), | ||
| closeOnce: sync.Once{}, | ||
| } | ||
| go func() { | ||
| if err := r.kindlingProxy.ListenAndServe(); err != nil { | ||
| slog.Error("Kindling proxy stopped with error", "error", err) | ||
| } | ||
| }() | ||
| r.addShutdownFunc(func(ctx context.Context) error { return r.kindlingProxy.Close() }) |
There was a problem hiding this comment.
The Kindling proxy is started asynchronously and any bind/startup error is only logged; NewRadiance still returns success. If the port is in use or the listener can’t start, the VPN outbound that points at 127.0.0.1:14988 will silently be broken. Consider creating the listener synchronously (net.Listen) and returning an error from NewRadiance on failure, or otherwise exposing readiness/failure to the caller.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func NewKindlingProxy(addr string) *KindlingProxy { | ||
| proxy := &KindlingProxy{ | ||
| server: &http.Server{ | ||
| Addr: addr, | ||
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodConnect { | ||
| http.Error(w, "only CONNECT method is supported", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| hijacker, ok := w.(http.Hijacker) | ||
| if !ok { | ||
| http.Error(w, "Hijacking not supported", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| clientConn, _, err := hijacker.Hijack() | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| defer clientConn.Close() | ||
|
|
||
| // Send 200 OK to client | ||
| _, err = clientConn.Write([]byte("HTTP/1.1 200 Connection Established\r\n\r\n")) | ||
| if err != nil { | ||
| return | ||
| } | ||
| req, err := http.ReadRequest(bufio.NewReader(clientConn)) | ||
| if err != nil { | ||
| writeErrorToConn(clientConn, http.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
| req.URL = &url.URL{ | ||
| Scheme: "http", | ||
| Host: r.Host, | ||
| Path: req.URL.Path, | ||
| RawQuery: req.URL.RawQuery, | ||
| } | ||
| req.RequestURI = "" | ||
| resp, err := HTTPClient().Do(req) | ||
| if err != nil { | ||
| writeErrorToConn(clientConn, http.StatusBadGateway, err.Error()) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| resp.Write(clientConn) | ||
| }), | ||
| }, | ||
| } | ||
| return proxy | ||
| } | ||
|
|
||
| func (p *KindlingProxy) ListenAndServe() error { | ||
| return p.server.ListenAndServe() | ||
| } |
There was a problem hiding this comment.
There are existing CONNECT-tunneling tests in bypass/bypass_test.go, but this new proxy implementation isn’t covered by tests. Adding unit tests for the proxy’s CONNECT behavior (successful tunnel, bad upstream dial, and ensuring bidirectional copy/close behavior) would help prevent regressions and confirm the proxy works with sing-box’s HTTP outbound expectations.
| Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodConnect { | ||
| http.Error(w, "only CONNECT method is supported", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| hijacker, ok := w.(http.Hijacker) | ||
| if !ok { | ||
| http.Error(w, "Hijacking not supported", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| clientConn, _, err := hijacker.Hijack() | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| defer clientConn.Close() | ||
|
|
||
| // Send 200 OK to client | ||
| _, err = clientConn.Write([]byte("HTTP/1.1 200 Connection Established\r\n\r\n")) | ||
| if err != nil { | ||
| return | ||
| } | ||
| req, err := http.ReadRequest(bufio.NewReader(clientConn)) | ||
| if err != nil { | ||
| writeErrorToConn(clientConn, http.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
| req.URL = &url.URL{ | ||
| Scheme: "http", | ||
| Host: r.Host, | ||
| Path: req.URL.Path, | ||
| RawQuery: req.URL.RawQuery, | ||
| } | ||
| req.RequestURI = "" | ||
| resp, err := HTTPClient().Do(req) | ||
| if err != nil { | ||
| writeErrorToConn(clientConn, http.StatusBadGateway, err.Error()) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| resp.Write(clientConn) | ||
| }), |
There was a problem hiding this comment.
The CONNECT handling here is not implementing an HTTP CONNECT tunnel. After sending "200 Connection Established" the proxy should treat the connection as a raw byte stream and relay traffic to the CONNECT target (r.Host), but this code instead calls http.ReadRequest on the hijacked conn and issues an HTTP request via HTTPClient(). That will hang/fail for typical CONNECT usage (e.g., TLS handshake bytes) and will also close the connection after a single request due to defer clientConn.Close(). Rework this handler to dial the target and bidirectionally copy bytes between clientConn and the upstream connection until either side closes.
| go func() { | ||
| if err := r.kindlingProxy.ListenAndServe(); err != nil { | ||
| if !errors.Is(err, http.ErrServerClosed) { | ||
| slog.Error("Kindling proxy stopped with error", "error", err) | ||
| } | ||
| } | ||
| }() | ||
| r.addShutdownFunc(func(ctx context.Context) error { return r.kindlingProxy.Close() }) | ||
|
|
There was a problem hiding this comment.
NewRadiance starts the Kindling proxy in a goroutine and only logs errors from ListenAndServe. If the port is already in use or bind fails, Radiance will still start successfully but the "kindling-proxy" outbound will be configured to point at a proxy that isn't running, causing hard-to-diagnose runtime failures. Consider binding the listener during startup (e.g., net.Listen) and returning an error if it fails, or otherwise making startup conditional/observable so callers can handle the failure.
This pull request introduces a new HTTP proxy component called
KindlingProxyand integrates it into the Radiance application. The proxy is started automatically with Radiance and is configured as an outbound option for VPN traffic. The changes are grouped into two main themes: new feature implementation and integration with the VPN system.New Feature: Kindling HTTP Proxy
kindling/proxy.goimplementing theKindlingProxy, which acts as an HTTP proxy server listening on127.0.0.1:14988. It forwards requests and responses between clients and remote servers.Integration with Radiance and VPN
Radiancestruct inradiance.goto include akindlingProxyfield, and modified the initialization logic to create, start, and close the proxy during application startup and shutdown. [1] [2]vpn/boxoptions.go), enabling VPN traffic to be routed through the proxy.kindlingpackage invpn/boxoptions.goto support the proxy integration.