From 7d8f4fa31c2c8a3ebd40f041065ddbae6055d5f3 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Fri, 18 Sep 2026 18:21:34 +0300 Subject: [PATCH 1/3] [management] Handle empty trusted peer (#7589) --- .../getting-started-enterprise.sh | 5 ++-- infrastructure_files/getting-started.sh | 6 ++--- management/internals/server/boot.go | 24 +++++++++++-------- management/internals/server/realip_test.go | 7 +++--- 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/infrastructure_files/getting-started-enterprise.sh b/infrastructure_files/getting-started-enterprise.sh index 701598a60..e88436e84 100755 --- a/infrastructure_files/getting-started-enterprise.sh +++ b/infrastructure_files/getting-started-enterprise.sh @@ -808,9 +808,8 @@ server: # Trust X-Forwarded-* only from the Traefik container's static address. Both # keys must stay in step with the ipv4_address pinned in docker-compose.yml: - # trustedPeers decides whether forwarded headers are read at all. Leaving it - # unset trusts nothing and records Traefik's own address as every peer's - # connection IP. + # trustedPeers restricts which sources may supply forwarded headers. Leaving + # it unset trusts all IPv4 and IPv6 sources. reverseProxy: trustedPeers: - "${TRAEFIK_IP}/32" diff --git a/infrastructure_files/getting-started.sh b/infrastructure_files/getting-started.sh index afbc5c282..d5c6d9dc9 100755 --- a/infrastructure_files/getting-started.sh +++ b/infrastructure_files/getting-started.sh @@ -586,9 +586,9 @@ configure_reverse_proxy() { TRUSTED_PEERS="${NETBIRD_TRUSTED_PEERS:-}" if [[ -z "$TRUSTED_PEERS" ]]; then echo "" > /dev/stderr - echo "Note: reverseProxy.trustedPeers is unset, so NetBird will use the address your" > /dev/stderr - echo "proxy connects from as each peer's connection IP. To record real client IPs," > /dev/stderr - echo "set NETBIRD_TRUSTED_PEERS to your proxy's address (e.g. 172.20.0.5/32) and re-run." > /dev/stderr + echo "Warning: reverseProxy.trustedPeers is unset, so all IPv4 and IPv6 sources" > /dev/stderr + echo "are trusted to provide forwarded client-IP headers. Set NETBIRD_TRUSTED_PEERS" > /dev/stderr + echo "to your proxy's address (e.g. 172.20.0.5/32) and re-run." > /dev/stderr echo "" > /dev/stderr fi fi diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index cd3fb9a62..ea999d82b 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -366,20 +366,24 @@ func streamInterceptor( return handler(srv, wrapped) } -// realIPOptions builds the real-IP middleware options from the reverse proxy config. +// realIPOptions builds the real-IP middleware options. // -// TrustedPeers controls which transport peers are allowed to supply forwarded-IP -// headers. If empty, forwarded headers are ignored and the transport peer address -// is used directly. Operators terminating connections at a reverse proxy should -// configure TrustedPeers with that proxy's address or network. +// Empty TrustedPeers trusts all IPv4 and IPv6 sources. Configure TrustedPeers +// with the reverse proxy address or network. // -// X-Forwarded-For is consulted first. X-Real-IP is read when X-Forwarded-For is -// absent or has no entries left after TrustedHTTPProxiesCount is applied. +// X-Forwarded-For takes precedence over X-Real-IP. func realIPOptions(cfg nbconfig.ReverseProxy) []realip.Option { - if idx := slices.IndexFunc(cfg.TrustedPeers, func(p netip.Prefix) bool { return p.Bits() == 0 }); idx >= 0 { + trustedPeers := cfg.TrustedPeers + if len(trustedPeers) == 0 { + trustedPeers = []netip.Prefix{ + netip.MustParsePrefix("0.0.0.0/0"), + netip.MustParsePrefix("::/0"), + } + } + if idx := slices.IndexFunc(trustedPeers, func(p netip.Prefix) bool { return p.Bits() == 0 }); idx >= 0 { log.WithContext(context.Background()).Warnf("TrustedPeers contains the default route %s, which trusts "+ "X-Forwarded-For from every client and allows connection IP spoofing. Set TrustedPeers to the address "+ - "of your reverse proxy, or leave it empty to use the connection's source address.", cfg.TrustedPeers[idx]) + "of your reverse proxy.", trustedPeers[idx]) } if cfg.TrustedHTTPProxiesCount > 0 { log.WithContext(context.Background()).Warn( @@ -389,7 +393,7 @@ func realIPOptions(cfg nbconfig.ReverseProxy) []realip.Option { } return []realip.Option{ - realip.WithTrustedPeers(cfg.TrustedPeers), + realip.WithTrustedPeers(trustedPeers), realip.WithTrustedProxies(cfg.TrustedHTTPProxies), realip.WithTrustedProxiesCount(cfg.TrustedHTTPProxiesCount), realip.WithHeaders([]string{realip.XForwardedFor, realip.XRealIp}), diff --git a/management/internals/server/realip_test.go b/management/internals/server/realip_test.go index 661cbc94a..ef6b3123d 100644 --- a/management/internals/server/realip_test.go +++ b/management/internals/server/realip_test.go @@ -9,13 +9,14 @@ import ( "time" "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/realip" - nbconfig "github.com/netbirdio/netbird/management/internals/server/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/metadata" "google.golang.org/protobuf/types/known/emptypb" + + nbconfig "github.com/netbirdio/netbird/management/internals/server/config" ) const ( @@ -135,8 +136,8 @@ func assertRealIP(t *testing.T, cfg nbconfig.ReverseProxy, want string, kv ...st }) } -func TestRealIPDefaultIgnoresClientForwardedHeaders(t *testing.T) { - assertRealIP(t, nbconfig.ReverseProxy{}, "127.0.0.1", +func TestRealIPDefaultTrustsForwardedHeaders(t *testing.T) { + assertRealIP(t, nbconfig.ReverseProxy{}, "203.0.113.44", realip.XForwardedFor, "203.0.113.44", realip.XRealIp, "203.0.113.44", ) From 3073d18039a040800b690bd41f4a05ddcce35282 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Sun, 20 Sep 2026 20:24:01 +0200 Subject: [PATCH 2/3] [proxy] Close the client connection on private service denials (#7590) A client that hits a private service before its peer joins the overlay gets a 403 from the tunnel-peer check. After it connects to NetBird, the browser reuses the warm socket to the public listener, so the request never traverses the tunnel and keeps failing until the 120s idle timeout closes it. Private service denials now set Connection: close and Cache-Control: no-store before the 403, both at the tunnel-peer check and at IP restriction denials on a private domain. Go's HTTP/1.1 server closes after the response; its HTTP/2 server turns the exact lowercase close token into a GOAWAY, which retires the stale connection for h2 clients. Public services and allowed private traffic keep their keep-alive behaviour. Tests cover HTTP/1.1 and HTTP/2 denials over a real listener (retry lands on a new connection), public denials and allowed private requests (connection reused), and both IP restriction paths. --- proxy/internal/auth/middleware.go | 26 ++- proxy/internal/auth/private_deny_test.go | 272 +++++++++++++++++++++++ 2 files changed, 295 insertions(+), 3 deletions(-) create mode 100644 proxy/internal/auth/private_deny_test.go diff --git a/proxy/internal/auth/middleware.go b/proxy/internal/auth/middleware.go index 1d46fd824..25ff68010 100644 --- a/proxy/internal/auth/middleware.go +++ b/proxy/internal/auth/middleware.go @@ -133,7 +133,7 @@ func (mw *Middleware) Protect(next http.Handler) http.Handler { if mw.forwardWithTunnelPeer(w, r, host, config, next) { return } - http.Error(w, "Forbidden", http.StatusForbidden) + denyPrivate(w) return } @@ -228,7 +228,7 @@ func (mw *Middleware) checkIPRestrictions(w http.ResponseWriter, r *http.Request clientIP := mw.resolveClientIP(r) if !clientIP.IsValid() { mw.logger.Debugf("IP restriction: cannot resolve client address for %q, denying", r.RemoteAddr) - http.Error(w, "Forbidden", http.StatusForbidden) + denyForbidden(w, config) return false } @@ -263,10 +263,30 @@ func (mw *Middleware) checkIPRestrictions(w http.ResponseWriter, r *http.Request reason := verdict.String() mw.blockIPRestriction(r, reason) - http.Error(w, "Forbidden", http.StatusForbidden) + denyForbidden(w, config) return false } +// denyForbidden writes a 403, dropping the client connection when the +// domain is private so a later retry cannot reuse it. +func denyForbidden(w http.ResponseWriter, config DomainConfig) { + if config.Private { + denyPrivate(w) + return + } + http.Error(w, "Forbidden", http.StatusForbidden) +} + +// denyPrivate writes a 403 and closes the connection, so a client refused +// before joining the overlay cannot keep retrying on the same warm socket. +// Go's HTTP/2 server turns the exact lowercase "close" token into a GOAWAY. +func denyPrivate(w http.ResponseWriter) { + h := w.Header() + h.Set("Connection", "close") + h.Set("Cache-Control", "no-store") + http.Error(w, "Forbidden", http.StatusForbidden) +} + // resolveClientIP extracts the real client IP from CapturedData, falling back to r.RemoteAddr. func (mw *Middleware) resolveClientIP(r *http.Request) netip.Addr { if cd := proxy.CapturedDataFromContext(r.Context()); cd != nil { diff --git a/proxy/internal/auth/private_deny_test.go b/proxy/internal/auth/private_deny_test.go new file mode 100644 index 000000000..5bec67168 --- /dev/null +++ b/proxy/internal/auth/private_deny_test.go @@ -0,0 +1,272 @@ +package auth + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "net/http/httptrace" + "net/netip" + "sync" + "testing" + "time" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + + "github.com/netbirdio/netbird/proxy/internal/proxy" + "github.com/netbirdio/netbird/proxy/internal/restrict" + "github.com/netbirdio/netbird/shared/management/proto" +) + +// switchableTunnelValidator flips the ValidateTunnelPeer verdict between requests. +type switchableTunnelValidator struct { + mu sync.Mutex + valid bool +} + +func (s *switchableTunnelValidator) setValid(v bool) { + s.mu.Lock() + defer s.mu.Unlock() + s.valid = v +} + +func (s *switchableTunnelValidator) ValidateSession(context.Context, *proto.ValidateSessionRequest, ...grpc.CallOption) (*proto.ValidateSessionResponse, error) { + return nil, errors.New("not used in this test") +} + +func (s *switchableTunnelValidator) ValidateTunnelPeer(context.Context, *proto.ValidateTunnelPeerRequest, ...grpc.CallOption) (*proto.ValidateTunnelPeerResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + if !s.valid { + return &proto.ValidateTunnelPeerResponse{Valid: false, DeniedReason: "not_in_group"}, nil + } + return &proto.ValidateTunnelPeerResponse{ + Valid: true, + UserId: "user-1", + SessionToken: "tunnel-session-token", + }, nil +} + +// testServerHost is the domain key Protect derives from the httptest listener. +const testServerHost = "127.0.0.1" + +var testTunnelIP = netip.MustParseAddr("100.90.1.14") + +// startProtectedServer serves mw.Protect and stamps requests as overlay traffic. +func startProtectedServer(t *testing.T, mw *Middleware, clientIP netip.Addr, lookup TunnelLookupFunc, h2 bool) *httptest.Server { + t.Helper() + protected := mw.Protect(newPassthroughHandler()) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cd := proxy.NewCapturedData("") + cd.SetClientIP(clientIP) + ctx := proxy.WithCapturedData(r.Context(), cd) + ctx = WithTunnelLookup(ctx, lookup) + protected.ServeHTTP(w, r.WithContext(ctx)) + }) + + srv := httptest.NewUnstartedServer(handler) + if h2 { + srv.EnableHTTP2 = true + srv.StartTLS() + } else { + srv.Start() + } + t.Cleanup(srv.Close) + return srv +} + +// tracedResponse is what a test observes from one client round trip. +type tracedResponse struct { + status int + protoMajor int + close bool + connection string + cacheControl string + reused bool +} + +// doTraced GETs url and reports whether the connection that served it was reused. +func doTraced(t *testing.T, client *http.Client, url string) tracedResponse { + t.Helper() + var reused bool + trace := &httptrace.ClientTrace{ + GotConn: func(info httptrace.GotConnInfo) { reused = info.Reused }, + } + req, err := http.NewRequestWithContext(httptrace.WithClientTrace(context.Background(), trace), http.MethodGet, url, nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + _, err = io.Copy(io.Discard, resp.Body) + require.NoError(t, err) + return tracedResponse{ + status: resp.StatusCode, + protoMajor: resp.ProtoMajor, + close: resp.Close, + connection: resp.Header.Get("Connection"), + cacheControl: resp.Header.Get("Cache-Control"), + reused: reused, + } +} + +func acceptAllLookup(_ netip.Addr) (PeerIdentity, bool) { + return PeerIdentity{TunnelIP: testTunnelIP}, true +} + +func newPrivateMiddleware(t *testing.T, validator SessionValidator, ipRestrictions *restrict.Filter) *Middleware { + t.Helper() + mw := NewMiddleware(log.StandardLogger(), validator, nil) + kp := generateTestKeyPair(t) + require.NoError(t, mw.AddDomain(testServerHost, nil, kp.PublicKey, time.Hour, "acct-1", "svc-1", ipRestrictions, true, nil)) + return mw +} + +// A rejected tunnel peer must emit the exact lowercase "close" token h2 matches on. +func TestProtect_PrivateService_DeniedSetsCloseHeaders(t *testing.T) { + mw := newPrivateMiddleware(t, &switchableTunnelValidator{}, nil) + handler := mw.Protect(newPassthroughHandler()) + + cd := proxy.NewCapturedData("") + cd.SetClientIP(testTunnelIP) + req := httptest.NewRequest(http.MethodGet, "http://"+testServerHost+"/", nil) + req.RemoteAddr = testTunnelIP.String() + ":5000" + req = req.WithContext(WithTunnelLookup(proxy.WithCapturedData(req.Context(), cd), acceptAllLookup)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Equal(t, "close", rec.Header().Get("Connection"), "private denial must ask the client to drop the connection") + assert.Equal(t, "no-store", rec.Header().Get("Cache-Control"), "private denial must not be cacheable") +} + +// A denied client must not keep reusing the warm socket after joining the overlay. +func TestPrivateDeny_HTTP1_ClosesConnection(t *testing.T) { + validator := &switchableTunnelValidator{} + mw := newPrivateMiddleware(t, validator, nil) + srv := startProtectedServer(t, mw, testTunnelIP, acceptAllLookup, false) + client := srv.Client() + + resp := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusForbidden, resp.status) + assert.Equal(t, 1, resp.protoMajor, "plain httptest server must speak HTTP/1.1") + // The Go client folds "Connection: close" into resp.close and drops the header. + assert.True(t, resp.close, "private denial must make the client mark the connection as not reusable") + assert.Equal(t, "no-store", resp.cacheControl, "private denial must not be cacheable") + + validator.setValid(true) + resp2 := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusOK, resp2.status, "the retry must reach the upstream once the peer is valid") + assert.False(t, resp2.reused, "the retry must open a new connection") +} + +// On HTTP/2 the header becomes a GOAWAY and the retry must use a new connection. +func TestPrivateDeny_HTTP2_SendsGoAway(t *testing.T) { + validator := &switchableTunnelValidator{} + mw := newPrivateMiddleware(t, validator, nil) + srv := startProtectedServer(t, mw, testTunnelIP, acceptAllLookup, true) + client := srv.Client() + + resp := doTraced(t, client, srv.URL) + require.Equal(t, 2, resp.protoMajor, "test client must negotiate HTTP/2") + assert.Equal(t, http.StatusForbidden, resp.status) + assert.Empty(t, resp.connection, "HTTP/2 must not carry a Connection header on the wire") + assert.Equal(t, "no-store", resp.cacheControl) + + validator.setValid(true) + resp2 := doTraced(t, client, srv.URL) + assert.Equal(t, 2, resp2.protoMajor) + assert.Equal(t, http.StatusOK, resp2.status, "the retry must reach the upstream once the peer is valid") + assert.False(t, resp2.reused, "GOAWAY must retire the connection so the retry opens a new one") +} + +// Legitimate private traffic keeps its keep-alive connection. +func TestPrivateAllow_KeepsConnection(t *testing.T) { + validator := &switchableTunnelValidator{valid: true} + mw := newPrivateMiddleware(t, validator, nil) + srv := startProtectedServer(t, mw, testTunnelIP, acceptAllLookup, false) + client := srv.Client() + + resp := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusOK, resp.status) + assert.Empty(t, resp.connection, "an allowed private request must not close the connection") + + resp2 := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusOK, resp2.status) + assert.True(t, resp2.reused, "allowed private traffic must keep reusing the connection") +} + +// Public denials keep the connection open; only private services change. +func TestPublicDeny_KeepsConnection(t *testing.T) { + mw := NewMiddleware(log.StandardLogger(), nil, nil) + filter := restrict.ParseFilter(restrict.FilterConfig{AllowedCIDRs: []string{"10.0.0.0/8"}}) + require.NoError(t, mw.AddDomain(testServerHost, nil, "", 0, "acct-1", "svc-1", filter, false, nil)) + srv := startProtectedServer(t, mw, netip.MustParseAddr("192.168.1.1"), nil, false) + client := srv.Client() + + resp := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusForbidden, resp.status) + assert.Empty(t, resp.connection, "public denial must not close the connection") + assert.Empty(t, resp.cacheControl, "public denial must not gain cache headers") + + resp2 := doTraced(t, client, srv.URL) + assert.Equal(t, http.StatusForbidden, resp2.status) + assert.True(t, resp2.reused, "public denials must keep reusing the connection") +} + +// IP restriction denials on a private service must close the connection too. +func TestCheckIPRestrictions_PrivateDenialClosesConnection(t *testing.T) { + filter := restrict.ParseFilter(restrict.FilterConfig{AllowedCIDRs: []string{"100.64.0.0/16"}}) + mw := newPrivateMiddleware(t, &switchableTunnelValidator{valid: true}, filter) + handler := mw.Protect(newPassthroughHandler()) + + tests := []struct { + name string + remoteAddr string + }{ + {"denied by CIDR", "100.65.5.6:5000"}, + {"unresolvable client address", "not-an-ip:1234"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "http://"+testServerHost+"/", nil) + req.RemoteAddr = tt.remoteAddr + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Equal(t, "close", rec.Header().Get("Connection"), "private IP-restriction denial must close the connection") + assert.Equal(t, "no-store", rec.Header().Get("Cache-Control"), "private IP-restriction denial must not be cacheable") + }) + } +} + +func TestCheckIPRestrictions_PublicDenialKeepsConnection(t *testing.T) { + mw := NewMiddleware(log.StandardLogger(), nil, nil) + filter := restrict.ParseFilter(restrict.FilterConfig{AllowedCIDRs: []string{"10.0.0.0/8"}}) + require.NoError(t, mw.AddDomain(testServerHost, nil, "", 0, "acct-1", "svc-1", filter, false, nil)) + handler := mw.Protect(newPassthroughHandler()) + + tests := []struct { + name string + remoteAddr string + }{ + {"denied by CIDR", "192.168.1.1:5000"}, + {"unresolvable client address", "not-an-ip:1234"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "http://"+testServerHost+"/", nil) + req.RemoteAddr = tt.remoteAddr + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Empty(t, rec.Header().Get("Connection"), "public IP-restriction denial must not close the connection") + assert.Empty(t, rec.Header().Get("Cache-Control"), "public IP-restriction denial must not gain cache headers") + }) + } +} From 314d88252d7d0f2549714b3cc25640bd612b7a88 Mon Sep 17 00:00:00 2001 From: Eduard Gert Date: Mon, 21 Sep 2026 10:34:04 +0200 Subject: [PATCH 3/3] [management] Name the account owner in the pending approval error (#7533) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Name the account owner in the pending approval error A user refused because their account is pending approval had no way to learn who could approve them. The refusal now carries the account owner's address, masked, so a caller can name someone to contact without being handed the address itself. Resolving the owner is best effort: a lookup failure, or an account predating the stored email, falls back to the refusal as it was. * Name only the caller's own owner in the pending approval error The refusal is raised before ValidateAccountAccess has established that the caller belongs to the account the request asked about, and the user is loaded by ID alone. Resolving the owner of the requested account therefore disclosed that owner's address to a pending user with no claim to it, reachable through any handler that takes an account ID from the caller — DELETE /api/accounts/{accountId} passes one straight through. The owner who can approve a pending user is the owner of their own account, so resolve that one. The requested account is never read. * Mask short local parts whole in MaskedEmail Keeping the first two characters and the last hides nothing until the local part is four long: at three or fewer they are the whole of it, so "abc@example.com" masked to "ab****c@example.com" and a pending user could recover the owner's address in full from what is meant to conceal it. Short local parts are now replaced entirely. * Name the owner from GetCurrentUserInfo instead of the permission gate The gate could only read the stored user row, which carries no address when an external IdP owns the identities — the usual case — so it named no one in practice. It also had no way to reach the IdP without being handed the account manager, which meant restoring bootstrap wiring that a refactor had dropped. GetCurrentUserInfo already holds that account manager, so it answers for a pending user itself and reuses GetOwnerInfo, the same lookup /msp uses to resolve an owner's address. The gate returns to exactly what it was, and with it goes the risk of naming the owner of an account the caller only asked about. MaskedEmail becomes MaskEmail: with a UserInfo in hand there is no stored row to hang it off. * [management] Cover the pending approval refusal in GetCurrentUserInfo The branch that names the owner had no coverage at the manager level, so neither the named refusal nor the fallback for an owner without a resolvable address was pinned down. * [management] Cover the failed owner lookup in the pending approval refusal The generic fallback has two ways in: no address on the resolved owner, and no owner to resolve at all. Only the first was pinned down. * [management] Pin the owner lookup to the caller's own account A mismatched account claim must not steer which owner the refusal names, and a blocked user is still answered before the claim is validated. Both are load bearing and neither was covered. --- management/server/types/user.go | 19 ++++ management/server/types/user_test.go | 141 +++++++++++++++++++++++++++ management/server/user.go | 27 +++++ management/server/user_test.go | 64 ++++++++++++ shared/management/status/error.go | 5 + 5 files changed, 256 insertions(+) diff --git a/management/server/types/user.go b/management/server/types/user.go index 02358ebc2..bebc20ea4 100644 --- a/management/server/types/user.go +++ b/management/server/types/user.go @@ -285,6 +285,25 @@ func (u *User) EncryptSensitiveData(enc *crypt.FieldEncrypt) error { return nil } +func MaskEmail(email string) string { + local, domain, found := strings.Cut(email, "@") + if !found || local == "" || domain == "" { + return "" + } + + // Runes, not bytes, so a non-ASCII local part is not cut mid-character. + runes := []rune(local) + + // Keeping the first two and the last needs a local part of at least four to + // hide anything at all: at three or fewer those are the whole of it, and the + // address would be recoverable in full from what is meant to conceal it. + if len(runes) < 4 { + return "****@" + domain + } + + return string(runes[:2]) + "****" + string(runes[len(runes)-1]) + "@" + domain +} + // DecryptSensitiveData decrypts the user's sensitive fields (Email and Name) in place. func (u *User) DecryptSensitiveData(enc *crypt.FieldEncrypt) error { if enc == nil { diff --git a/management/server/types/user_test.go b/management/server/types/user_test.go index e11df96aa..1b3ce7ce6 100644 --- a/management/server/types/user_test.go +++ b/management/server/types/user_test.go @@ -296,3 +296,144 @@ func TestUser_EncryptDecryptRoundTrip(t *testing.T) { }) } } + +func TestMaskEmail(t *testing.T) { + testCases := []struct { + name string + email string + expected string + }{ + { + name: "ordinary address keeps the first two, the last, and the domain", + email: "admin@example.com", + expected: "ad****n@example.com", + }, + { + name: "four characters is the shortest local part that reveals anything", + email: "abcd@example.com", + expected: "ab****d@example.com", + }, + { + name: "three character local part is masked whole, since a lead and tail would be all of it", + email: "abc@example.com", + expected: "****@example.com", + }, + { + name: "two character local part is masked whole", + email: "ab@example.com", + expected: "****@example.com", + }, + { + name: "single character local part is masked whole", + email: "a@b.co", + expected: "****@b.co", + }, + { + name: "mask width does not report the length it stands in for", + email: "a.very.long.local.part@example.com", + expected: "a.****t@example.com", + }, + { + name: "a local part far longer than the mask is still reduced to three characters", + email: "finance.department.notifications.owner.account@example.com", + expected: "fi****t@example.com", + }, + { + name: "plus addressing is masked along with the rest of the local part", + email: "admin+netbird@example.com", + expected: "ad****d@example.com", + }, + { + name: "separators inside the local part are not treated specially", + email: "first.last-name_x@example.com", + expected: "fi****x@example.com", + }, + { + name: "case is preserved rather than normalised", + email: "Admin@Example.COM", + expected: "Ad****n@Example.COM", + }, + { + name: "subdomains stay intact", + email: "owner@mail.corp.example.com", + expected: "ow****r@mail.corp.example.com", + }, + { + name: "german umlauts count as single characters", + email: "müller@example.de", + expected: "mü****r@example.de", + }, + { + name: "cyrillic local part is cut on runes", + email: "иванов@example.ru", + expected: "ив****в@example.ru", + }, + { + name: "cjk local part of three runes is masked whole, counted in runes not bytes", + email: "用户名@example.cn", + expected: "****@example.cn", + }, + { + name: "cjk local part of four runes reveals the first two and the last", + email: "用户名字@example.cn", + expected: "用户****字@example.cn", + }, + { + name: "arabic local part is cut on runes", + email: "مستخدم@example.sa", + expected: "مس****م@example.sa", + }, + { + name: "two rune non-ascii local part is masked whole", + email: "ää@example.de", + expected: "****@example.de", + }, + { + name: "astral plane runes are not split into surrogates", + email: "a🎉bc@example.com", + expected: "a🎉****c@example.com", + }, + { + name: "a non-ascii domain is left alone", + email: "admin@münchen.example", + expected: "ad****n@münchen.example", + }, + { + name: "only the first separator splits, so a second stays in the domain", + email: "a@b@example.com", + expected: "****@b@example.com", + }, + { + name: "empty email has nothing to mask", + email: "", + expected: "", + }, + { + name: "value without a separator is not an address", + email: "not-an-email", + expected: "", + }, + { + name: "missing local part is not an address", + email: "@example.com", + expected: "", + }, + { + name: "missing domain is not an address", + email: "admin@", + expected: "", + }, + { + name: "a bare separator is not an address", + email: "@", + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, MaskEmail(tc.email)) + }) + } + +} diff --git a/management/server/user.go b/management/server/user.go index 823c1b2e4..3510a624b 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -1448,6 +1448,25 @@ func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountI return updateAccountPeers, nil } +// pendingApprovalError refuses a user awaiting approval, naming the owner who +// can approve them when their address resolves. Failing to resolve one is not a +// reason to withhold the refusal, so the lookup is best effort. +func (am *DefaultAccountManager) pendingApprovalError(ctx context.Context, accountID string) error { + owner, err := am.GetOwnerInfo(ctx, accountID) + if err != nil { + log.WithContext(ctx).Debugf("pending approval refusal: owner of account %s did not resolve: %v", accountID, err) + return status.NewUserPendingApprovalError() + } + + masked := types.MaskEmail(owner.Email) + if masked == "" { + log.WithContext(ctx).Debugf("pending approval refusal: no address found for the owner of account %s", accountID) + return status.NewUserPendingApprovalError() + } + + return status.NewUserPendingApprovalByOwnerError(masked) +} + // GetOwnerInfo retrieves the owner information for a given account ID. func (am *DefaultAccountManager) GetOwnerInfo(ctx context.Context, accountID string) (*types.UserInfo, error) { owner, err := am.Store.GetAccountOwner(ctx, store.LockingStrengthNone, accountID) @@ -1505,6 +1524,14 @@ func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, userAut return nil, err } + // A user pending approval is blocked too, and the dashboard needs to tell + // the two apart: one is a dead end, the other resolves by itself once the + // owner acts. Naming that owner needs the address the IdP holds, which is + // why this is answered here rather than in the permission gate. + if user.IsBlocked() && user.PendingApproval { + return nil, am.pendingApprovalError(ctx, user.AccountID) + } + if user.IsBlocked() { return nil, status.NewUserBlockedError() } diff --git a/management/server/user_test.go b/management/server/user_test.go index ec0bbc54e..2d1a5f1e9 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1779,6 +1779,42 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { } require.NoError(t, store.SaveAccount(context.Background(), account2)) + account3 := newAccountWithId(context.Background(), "account3", "account3Owner", "", "owner@example.com", "", false) + account3.Users["pending-user"] = &types.User{ + Id: "pending-user", + AccountID: account3.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account3)) + + // The owner has no address to name, so the refusal falls back to the generic one. + account4 := newAccountWithId(context.Background(), "account4", "account4Owner", "", "", "", false) + account4.Users["pending-user-without-owner-email"] = &types.User{ + Id: "pending-user-without-owner-email", + AccountID: account4.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account4)) + + // No user holds the owner role, so the owner lookup itself fails. + account5 := newAccountWithId(context.Background(), "account5", "account5Admin", "", "", "", false) + account5.Users["account5Admin"].Role = types.UserRoleAdmin + account5.Users["pending-user-without-owner"] = &types.User{ + Id: "pending-user-without-owner", + AccountID: account5.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account5)) + + account6 := newAccountWithId(context.Background(), "account6", "account6Owner", "", "stranger@example.com", "", false) + require.NoError(t, store.SaveAccount(context.Background(), account6)) + permissionsManager := permissions.NewManager(store) am := DefaultAccountManager{ Store: store, @@ -1812,6 +1848,34 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { userAuth: auth.UserAuth{AccountId: account1.Id, UserId: "service-user"}, expectedErr: status.NewPermissionDeniedError(), }, + { + name: "pending approval names the owner", + userAuth: auth.UserAuth{AccountId: account3.Id, UserId: "pending-user"}, + expectedErr: status.NewUserPendingApprovalByOwnerError("ow****r@example.com"), + }, + { + name: "pending approval without an owner address", + userAuth: auth.UserAuth{AccountId: account4.Id, UserId: "pending-user-without-owner-email"}, + expectedErr: status.NewUserPendingApprovalError(), + }, + { + name: "pending approval without an owner", + userAuth: auth.UserAuth{AccountId: account5.Id, UserId: "pending-user-without-owner"}, + expectedErr: status.NewUserPendingApprovalError(), + }, + { + // The account claim points at an account the caller is not in. The + // owner named has to be the one of the account holding the caller's + // own record, never the one the claim asks for. + name: "pending approval ignores a mismatched account claim", + userAuth: auth.UserAuth{AccountId: account6.Id, UserId: "pending-user"}, + expectedErr: status.NewUserPendingApprovalByOwnerError("ow****r@example.com"), + }, + { + name: "blocked user answers before the account claim is validated", + userAuth: auth.UserAuth{AccountId: account6.Id, UserId: "blocked-user"}, + expectedErr: status.NewUserBlockedError(), + }, { name: "owner user", userAuth: auth.UserAuth{AccountId: account1.Id, UserId: "account1Owner"}, diff --git a/shared/management/status/error.go b/shared/management/status/error.go index e31663450..4249f27de 100644 --- a/shared/management/status/error.go +++ b/shared/management/status/error.go @@ -135,6 +135,11 @@ func NewUserPendingApprovalError() error { return Errorf(PermissionDenied, "user is pending approval") } +// NewUserPendingApprovalByOwnerError creates a new Error with PermissionDenied type for a blocked user pending approval, naming the masked address of the owner who can approve them +func NewUserPendingApprovalByOwnerError(ownerEmail string) error { + return Errorf(PermissionDenied, "user is pending approval by owner %s", ownerEmail) +} + // NewPeerNotRegisteredError creates a new Error with Unauthenticated type unregistered peer func NewPeerNotRegisteredError() error { return Errorf(Unauthenticated, "peer is not registered")