[PR #6268] [MERGED] [management, client, proxy] Follow-up fixes for private reverse-proxy services #27884

Closed
opened 2026-08-05 07:09:21 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6268
Author: @mlsmaycon
Created: 5/26/2026
Status: Merged
Merged: 6/2/2026
Merged by: @mlsmaycon

Base: mainHead: follow-up-private-services


📝 Commits (10+)

  • e09d51c fix(proxy): gate tunnel-peer fast-path on inbound listener marker
  • 8783440 fix(proxy): harden inbound listener resource + startup-ctx handling
  • 6600f0d feat(proxy): short-circuit peer-own-target loops with 421
  • af416c6 fix(management): private-service validation + tunnel-IP lookup semantics
  • 86e24a6 fix(client): include offlinePeers in PeerStateByIP lookup
  • 43c0cb1 fix(rest): reject empty Delete path params in reverse-proxy clients
  • 924be21 chore(api,ci,docs,test): private-service schema, proto-check, fixups
  • 4f7c733 fix(proxy): background ctx for already-started AddPeer notification
  • 75c8fa7 Merge branch 'main' into follow-up-private-services
  • b9fb04c use the cmd context for roundtripper

📊 Changes

31 files changed (+711 additions, -105 deletions)

View changed files

📝 .github/workflows/proto-version-check.yml (+28 -11)
📝 client/internal/peer/status.go (+11 -2)
📝 client/internal/peer/status_test.go (+22 -0)
📝 management/internals/modules/reverseproxy/service/service.go (+5 -1)
📝 management/internals/modules/reverseproxy/service/service_test.go (+12 -2)
📝 management/internals/shared/grpc/validate_session_test.go (+26 -1)
📝 management/server/store/sql_store.go (+8 -1)
📝 management/server/store/sql_store_service_test.go (+1 -1)
📝 management/server/store/sql_store_test.go (+21 -0)
📝 proxy/cmd/proxy/cmd/root.go (+4 -4)
📝 proxy/inbound.go (+43 -23)
📝 proxy/inbound_test.go (+36 -3)
📝 proxy/internal/auth/middleware.go (+24 -16)
📝 proxy/internal/auth/middleware_test.go (+90 -0)
📝 proxy/internal/auth/tunnel_lookup_test.go (+37 -7)
📝 proxy/internal/debug/handler.go (+1 -1)
📝 proxy/internal/proxy/reverseproxy.go (+42 -0)
📝 proxy/internal/proxy/reverseproxy_test.go (+98 -0)
📝 proxy/internal/roundtrip/netbird.go (+34 -10)
📝 proxy/internal/roundtrip/netbird_test.go (+68 -7)

...and 11 more files

📄 Description

Describe your changes

Follow-up to the private reverse-proxy services work (#6226), addressing
review findings from lixmal and CodeRabbit plus one new hardening
feature. Scope is correctness, security, and resource-leak fixes around
the private-service / tunnel-peer path — no new user-facing capability
beyond the loop guard.

Total: 28 files, ~678 insertions / 86 deletions across 7 commits.

What's in here

1. fix(proxy): gate tunnel-peer fast-path on inbound listener marker (Critical)

forwardWithTunnelPeer previously accepted any RFC1918 / ULA / CGNAT
source IP, so a public client whose address fell in those ranges could
bypass the configured operator auth scheme by colliding with a known
tunnel IP. The fast-path is now gated on TunnelLookupFromContext being
present — that marker is attached only by the per-account inbound
(overlay) listener, so the host-facing listener never takes this branch.

2. fix(proxy): harden inbound listener resource + startup-ctx handling

  • Close the logrus ErrorLog PipeWriter on tearDown. WriterLevel
    returns an *io.PipeWriter backed by a pipe + scanner goroutine the
    caller owns; the two writers per account (https + plain) were never
    closed, leaking on every teardown.
  • Run the post-Start hooks (readyHandler, NotifyStatus) on
    context.Background() instead of the AddPeer caller's request-scoped
    ctx. A cancelled request could otherwise abort the inbound bring-up or
    fail the status notification. The tail is split into notifyClientReady
    so the contract is unit-testable.

3. feat(proxy): short-circuit peer-own-target loops with 421

When a peer that hosts a private service dials its own service URL, the
request used to loop back through the proxy and over WireGuard to the
same peer (double the WG round-trip, no signal to the caller). Overlay
requests whose source tunnel IP matches the resolved target host now get
421 Misdirected Request with an explanatory body. Scoped to
overlay-origin requests so public-listener traffic with a colliding
source IP is unaffected.

4. fix(management): private-service validation + tunnel-IP lookup semantics

  • Require an explicit port for L4 cluster targets. validateL4Target
    exempted TargetTypeCluster, but buildPathMappings serializes every
    L4 target via net.JoinHostPort(host, port)port=0 shipped a
    ":0" upstream.
  • GetPeerByIP returns NotFound on a tunnel-IP miss instead of mapping
    every error to Internal, so ValidateTunnelPeer's expected misses are
    distinguishable from real store failures.
  • Thread ctx into getClusterCapability's gorm query.

5. fix(client): include offlinePeers in PeerStateByIP lookup

ReplaceOfflinePeers moves peers into d.offlinePeers, but
PeerStateByIP only scanned d.peers. The DNS filter and
embed.Client.IdentityForIP were treating known-but-offline peers as
unknown. Both slices are now searched.

6. fix(rest): reject empty Delete path params in reverse-proxy clients

ReverseProxyClustersAPI.Delete / ReverseProxyTokensAPI.Delete passed
the path param into url.PathEscape with no empty check, collapsing the
URL onto the collection endpoint. Both now short-circuit with a typed
error.

7. chore(api,ci,docs,test): private-service schema, proto-check, fixups

  • OpenAPI: require non-empty access_groups and mode=http when
    private=true, on both Service and ServiceRequest, mirroring
    validatePrivateRequirements. mode stays optional-but-constrained
    (empty defaults to http server-side), matching runtime.
  • CI: proto-version-check now covers renamed .pb.go files (reads
    base via previous_filename) and matches protoc-gen-go-grpc headers
    (optional - prefix, -gen-go-grpc suffix) so grpc-generated files
    are in scope.
  • Docs/comments: Config defaults doc corrected (applied at
    Server.Start, not New); obsolete --private-inbound flag renamed
    to --private across comments and the proto doc.
  • Pre-existing test fixups: repaired the integration-tagged
    validate_session_test.go (SignToken signature growth + new Manager
    interface methods), fixed a CI-skip boolean precedence bug that skipped
    Windows unconditionally, and guarded a router.HTTPListener type
    assertion with comma-ok.

8. fix(proxy): background ctx for already-started AddPeer notification

Completes the startup-ctx fix from commit 2. When a service is added to
an already-started client, AddPeer called NotifyStatus with the
caller's request-scoped ctx — a cancelled request/stream could drop the
connected notification to management. Uses context.Background() here
too, matching notifyClientReady. The other ctx use in AddPeer
(createClientEntry) stays request-scoped, since a cancelled request
should abort client creation. Test extended to pass a pre-cancelled
caller ctx and assert the notification still runs on a non-cancelled
context.

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md.

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • New Features

    • Reverse proxy rejects self-targeting requests with HTTP 421.
    • Stronger tunnel-peer fast-path validation to prevent spoofing.
  • Bug Fixes / Improvements

    • Peer lookups resolve peers moved offline.
    • L4 cluster targets now require a non-zero port.
    • Prevented HTTP error-log writer goroutine/pipe leaks.
    • Client-side validation rejects empty reverse-proxy cluster/token deletes.
    • Unknown tunnel-IP lookups return NotFound (not generic error).
    • Embedded client startup and status notifications use background contexts.
  • Documentation

    • OpenAPI schema enforces private-service constraints.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6268 **Author:** [@mlsmaycon](https://github.com/mlsmaycon) **Created:** 5/26/2026 **Status:** ✅ Merged **Merged:** 6/2/2026 **Merged by:** [@mlsmaycon](https://github.com/mlsmaycon) **Base:** `main` ← **Head:** `follow-up-private-services` --- ### 📝 Commits (10+) - [`e09d51c`](https://github.com/netbirdio/netbird/commit/e09d51c1a83254eabec57b1bba6ba8bba319ce9a) fix(proxy): gate tunnel-peer fast-path on inbound listener marker - [`8783440`](https://github.com/netbirdio/netbird/commit/878344088f58fa46ae33b40fbbf1a3c2f3cc5f45) fix(proxy): harden inbound listener resource + startup-ctx handling - [`6600f0d`](https://github.com/netbirdio/netbird/commit/6600f0d45fd4604f06c637f29cccb69dbc981334) feat(proxy): short-circuit peer-own-target loops with 421 - [`af416c6`](https://github.com/netbirdio/netbird/commit/af416c656b90fb81984e531a8fba1207fbc462a6) fix(management): private-service validation + tunnel-IP lookup semantics - [`86e24a6`](https://github.com/netbirdio/netbird/commit/86e24a622d97415edbab6058adc7178e69ba19a5) fix(client): include offlinePeers in PeerStateByIP lookup - [`43c0cb1`](https://github.com/netbirdio/netbird/commit/43c0cb1dc2490feef5fb4798221264139bb7daa8) fix(rest): reject empty Delete path params in reverse-proxy clients - [`924be21`](https://github.com/netbirdio/netbird/commit/924be2116be7cdd2598a6b29cedf9cbd01e6c360) chore(api,ci,docs,test): private-service schema, proto-check, fixups - [`4f7c733`](https://github.com/netbirdio/netbird/commit/4f7c73369b1754a15f2e4afc273dca331f3d92db) fix(proxy): background ctx for already-started AddPeer notification - [`75c8fa7`](https://github.com/netbirdio/netbird/commit/75c8fa78e283c8958d58659d42f9d3d93b8fcefd) Merge branch 'main' into follow-up-private-services - [`b9fb04c`](https://github.com/netbirdio/netbird/commit/b9fb04c302bc4e7ef3d2177c5611cb72d30b3549) use the cmd context for roundtripper ### 📊 Changes **31 files changed** (+711 additions, -105 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/proto-version-check.yml` (+28 -11) 📝 `client/internal/peer/status.go` (+11 -2) 📝 `client/internal/peer/status_test.go` (+22 -0) 📝 `management/internals/modules/reverseproxy/service/service.go` (+5 -1) 📝 `management/internals/modules/reverseproxy/service/service_test.go` (+12 -2) 📝 `management/internals/shared/grpc/validate_session_test.go` (+26 -1) 📝 `management/server/store/sql_store.go` (+8 -1) 📝 `management/server/store/sql_store_service_test.go` (+1 -1) 📝 `management/server/store/sql_store_test.go` (+21 -0) 📝 `proxy/cmd/proxy/cmd/root.go` (+4 -4) 📝 `proxy/inbound.go` (+43 -23) 📝 `proxy/inbound_test.go` (+36 -3) 📝 `proxy/internal/auth/middleware.go` (+24 -16) 📝 `proxy/internal/auth/middleware_test.go` (+90 -0) 📝 `proxy/internal/auth/tunnel_lookup_test.go` (+37 -7) 📝 `proxy/internal/debug/handler.go` (+1 -1) 📝 `proxy/internal/proxy/reverseproxy.go` (+42 -0) 📝 `proxy/internal/proxy/reverseproxy_test.go` (+98 -0) 📝 `proxy/internal/roundtrip/netbird.go` (+34 -10) 📝 `proxy/internal/roundtrip/netbird_test.go` (+68 -7) _...and 11 more files_ </details> ### 📄 Description ## Describe your changes Follow-up to the private reverse-proxy services work (#6226), addressing review findings from lixmal and CodeRabbit plus one new hardening feature. Scope is correctness, security, and resource-leak fixes around the private-service / tunnel-peer path — no new user-facing capability beyond the loop guard. Total: 28 files, ~678 insertions / 86 deletions across 7 commits. ### What's in here #### 1. `fix(proxy): gate tunnel-peer fast-path on inbound listener marker` *(Critical)* `forwardWithTunnelPeer` previously accepted any RFC1918 / ULA / CGNAT source IP, so a public client whose address fell in those ranges could bypass the configured operator auth scheme by colliding with a known tunnel IP. The fast-path is now gated on `TunnelLookupFromContext` being present — that marker is attached only by the per-account inbound (overlay) listener, so the host-facing listener never takes this branch. #### 2. `fix(proxy): harden inbound listener resource + startup-ctx handling` - Close the logrus `ErrorLog` `PipeWriter` on tearDown. `WriterLevel` returns an `*io.PipeWriter` backed by a pipe + scanner goroutine the caller owns; the two writers per account (https + plain) were never closed, leaking on every teardown. - Run the post-`Start` hooks (`readyHandler`, `NotifyStatus`) on `context.Background()` instead of the `AddPeer` caller's request-scoped ctx. A cancelled request could otherwise abort the inbound bring-up or fail the status notification. The tail is split into `notifyClientReady` so the contract is unit-testable. #### 3. `feat(proxy): short-circuit peer-own-target loops with 421` When a peer that hosts a private service dials its own service URL, the request used to loop back through the proxy and over WireGuard to the same peer (double the WG round-trip, no signal to the caller). Overlay requests whose source tunnel IP matches the resolved target host now get `421 Misdirected Request` with an explanatory body. Scoped to overlay-origin requests so public-listener traffic with a colliding source IP is unaffected. #### 4. `fix(management): private-service validation + tunnel-IP lookup semantics` - Require an explicit port for L4 cluster targets. `validateL4Target` exempted `TargetTypeCluster`, but `buildPathMappings` serializes every L4 target via `net.JoinHostPort(host, port)` — `port=0` shipped a `":0"` upstream. - `GetPeerByIP` returns `NotFound` on a tunnel-IP miss instead of mapping every error to `Internal`, so `ValidateTunnelPeer`'s expected misses are distinguishable from real store failures. - Thread `ctx` into `getClusterCapability`'s gorm query. #### 5. `fix(client): include offlinePeers in PeerStateByIP lookup` `ReplaceOfflinePeers` moves peers into `d.offlinePeers`, but `PeerStateByIP` only scanned `d.peers`. The DNS filter and `embed.Client.IdentityForIP` were treating known-but-offline peers as unknown. Both slices are now searched. #### 6. `fix(rest): reject empty Delete path params in reverse-proxy clients` `ReverseProxyClustersAPI.Delete` / `ReverseProxyTokensAPI.Delete` passed the path param into `url.PathEscape` with no empty check, collapsing the URL onto the collection endpoint. Both now short-circuit with a typed error. #### 7. `chore(api,ci,docs,test): private-service schema, proto-check, fixups` - **OpenAPI:** require non-empty `access_groups` and `mode=http` when `private=true`, on both `Service` and `ServiceRequest`, mirroring `validatePrivateRequirements`. `mode` stays optional-but-constrained (empty defaults to http server-side), matching runtime. - **CI:** proto-version-check now covers renamed `.pb.go` files (reads base via `previous_filename`) and matches `protoc-gen-go-grpc` headers (optional `- ` prefix, `-gen-go-grpc` suffix) so grpc-generated files are in scope. - **Docs/comments:** Config defaults doc corrected (applied at `Server.Start`, not `New`); obsolete `--private-inbound` flag renamed to `--private` across comments and the proto doc. - **Pre-existing test fixups:** repaired the integration-tagged `validate_session_test.go` (SignToken signature growth + new Manager interface methods), fixed a CI-skip boolean precedence bug that skipped Windows unconditionally, and guarded a `router.HTTPListener` type assertion with comma-ok. #### 8. `fix(proxy): background ctx for already-started AddPeer notification` Completes the startup-ctx fix from commit 2. When a service is added to an already-started client, `AddPeer` called `NotifyStatus` with the caller's request-scoped ctx — a cancelled request/stream could drop the connected notification to management. Uses `context.Background()` here too, matching `notifyClientReady`. The other `ctx` use in `AddPeer` (`createClientEntry`) stays request-scoped, since a cancelled request *should* abort client creation. Test extended to pass a pre-cancelled caller ctx and assert the notification still runs on a non-cancelled context. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [x] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Reverse proxy rejects self-targeting requests with HTTP 421. * Stronger tunnel-peer fast-path validation to prevent spoofing. * **Bug Fixes / Improvements** * Peer lookups resolve peers moved offline. * L4 cluster targets now require a non-zero port. * Prevented HTTP error-log writer goroutine/pipe leaks. * Client-side validation rejects empty reverse-proxy cluster/token deletes. * Unknown tunnel-IP lookups return NotFound (not generic error). * Embedded client startup and status notifications use background contexts. * **Documentation** * OpenAPI schema enforces private-service constraints. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 07:09:21 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#27884