diff --git a/proxy/internal/proxy/reverseproxy.go b/proxy/internal/proxy/reverseproxy.go index e437e78a7..da0bf6552 100644 --- a/proxy/internal/proxy/reverseproxy.go +++ b/proxy/internal/proxy/reverseproxy.go @@ -66,6 +66,22 @@ func (p *ReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Loop guard for private services: a peer that hosts the target + // dialing its own service URL would round-trip its own traffic + // through the proxy and back over WG to itself. Refuse the request + // with 421 (Misdirected Request) so the caller sees an explicit + // error instead of silently doubling tunnel traffic. + if p.isSelfTargetLoop(r, result.target.URL) { + if cd := CapturedDataFromContext(r.Context()); cd != nil { + cd.SetOrigin(OriginNoRoute) + } + requestID := getRequestID(r) + web.ServeErrorPage(w, r, http.StatusMisdirectedRequest, "Loop Detected", + "This peer is the target of the requested service. Reach the backend directly instead of dialing the public service URL from the same machine.", + requestID, web.ErrorStatus{Proxy: true, Destination: false}) + return + } + ctx := r.Context() // Set the account ID in the context for the roundtripper to use. ctx = roundtrip.WithAccountID(ctx, result.accountID) @@ -107,6 +123,32 @@ func (p *ReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { rp.ServeHTTP(w, r.WithContext(ctx)) } +// isSelfTargetLoop reports whether an overlay-origin request is about to +// be forwarded back to the very peer that initiated it. The detection +// is intentionally narrow: it only fires when the request arrived on +// the per-account inbound (overlay) listener (so we're confident the +// source address is the caller's tunnel IP), and only when the resolved +// target host matches that tunnel IP. Catching this here returns 421 to +// the caller instead of letting the proxy round-trip its own traffic +// over WG twice. +func (p *ReverseProxy) isSelfTargetLoop(r *http.Request, target *url.URL) bool { + if target == nil { + return false + } + if !types.IsOverlayOrigin(r.Context()) { + return false + } + srcIP := extractHostIP(r.RemoteAddr) + if !srcIP.IsValid() { + return false + } + targetIP, err := netip.ParseAddr(target.Hostname()) + if err != nil { + return false + } + return srcIP.Unmap() == targetIP.Unmap() +} + // rewriteFunc returns a Rewrite function for httputil.ReverseProxy that rewrites // inbound requests to target the backend service while setting security-relevant // forwarding headers and stripping proxy authentication credentials. diff --git a/proxy/internal/proxy/reverseproxy_test.go b/proxy/internal/proxy/reverseproxy_test.go index d5158a6cc..a8244fa56 100644 --- a/proxy/internal/proxy/reverseproxy_test.go +++ b/proxy/internal/proxy/reverseproxy_test.go @@ -20,6 +20,7 @@ import ( "github.com/netbirdio/netbird/proxy/auth" "github.com/netbirdio/netbird/proxy/internal/roundtrip" + "github.com/netbirdio/netbird/proxy/internal/types" "github.com/netbirdio/netbird/proxy/web" ) @@ -1285,6 +1286,103 @@ func TestStampNetBirdIdentity_OmitsGroupsHeaderWhenAllInvalid(t *testing.T) { "X-NetBird-Groups must not be set when every group label is rejected") } +// nopOKTransport returns 200 for every request without dialing — used +// by the self-target-loop tests so the non-loop cases don't pay a real +// TCP-dial timeout. +type nopOKTransport struct{} + +func (nopOKTransport) RoundTrip(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Header: http.Header{}}, nil +} + +// TestServeHTTP_SelfTargetLoopReturns421 covers the loop guard for +// private services: when a peer dials a service whose only target is +// the peer itself, the proxy must refuse with 421 (Misdirected +// Request) rather than round-tripping the request back over WG to +// the same peer. +func TestServeHTTP_SelfTargetLoopReturns421(t *testing.T) { + rp := NewReverseProxy(nopOKTransport{}, "auto", nil, nil) + rp.AddMapping(Mapping{ + ID: "svc-1", + AccountID: "acct-1", + Host: "private.svc", + Paths: map[string]*PathTarget{ + "/": { + URL: &url.URL{Scheme: "http", Host: "100.64.0.5:8080"}, + }, + }, + }) + + req := httptest.NewRequest(http.MethodGet, "http://private.svc/", nil) + req.Host = "private.svc" + req.RemoteAddr = "100.64.0.5:55555" + req = req.WithContext(types.WithOverlayOrigin(req.Context())) + rec := httptest.NewRecorder() + + rp.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusMisdirectedRequest, rec.Code, + "a peer dialing a service whose target is itself must get 421") +} + +// TestServeHTTP_SelfTargetLoop_NonOverlayRequestPassesThrough verifies +// the guard is scoped to overlay-origin requests. A public-listener +// request that happens to share a source IP with the target host must +// not be misinterpreted as a loop — the gating relies on the inbound +// marker being attached only by the per-account overlay listener. +func TestServeHTTP_SelfTargetLoop_NonOverlayRequestPassesThrough(t *testing.T) { + rp := NewReverseProxy(nopOKTransport{}, "auto", nil, nil) + rp.AddMapping(Mapping{ + ID: "svc-1", + AccountID: "acct-1", + Host: "public.svc", + Paths: map[string]*PathTarget{ + "/": { + URL: &url.URL{Scheme: "http", Host: "100.64.0.5:8080"}, + }, + }, + }) + + req := httptest.NewRequest(http.MethodGet, "http://public.svc/", nil) + req.Host = "public.svc" + req.RemoteAddr = "100.64.0.5:55555" + // No WithOverlayOrigin → the guard must not fire. + rec := httptest.NewRecorder() + + rp.ServeHTTP(rec, req) + + assert.NotEqual(t, http.StatusMisdirectedRequest, rec.Code, + "a non-overlay request with a colliding source IP must not be flagged as a loop") +} + +// TestServeHTTP_SelfTargetLoop_OverlayDifferentIPPassesThrough confirms +// that overlay-origin requests with a source IP that does *not* match +// the target host are forwarded normally. +func TestServeHTTP_SelfTargetLoop_OverlayDifferentIPPassesThrough(t *testing.T) { + rp := NewReverseProxy(nopOKTransport{}, "auto", nil, nil) + rp.AddMapping(Mapping{ + ID: "svc-1", + AccountID: "acct-1", + Host: "private.svc", + Paths: map[string]*PathTarget{ + "/": { + URL: &url.URL{Scheme: "http", Host: "100.64.0.5:8080"}, + }, + }, + }) + + req := httptest.NewRequest(http.MethodGet, "http://private.svc/", nil) + req.Host = "private.svc" + req.RemoteAddr = "100.64.0.99:55555" // different from the target + req = req.WithContext(types.WithOverlayOrigin(req.Context())) + rec := httptest.NewRecorder() + + rp.ServeHTTP(rec, req) + + assert.NotEqual(t, http.StatusMisdirectedRequest, rec.Code, + "overlay request with a non-matching source IP must not be flagged as a loop") +} + // TestStampNetBirdIdentity_CapturedDataPresentButEmpty covers requests // that carry CapturedData with no identity fields populated (e.g. the // auth middleware ran but the request didn't authenticate). Both