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", ) 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/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") + }) + } +} 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")