[management] Prevent deleting custom domains used by services (#7515)

Deleting a custom domain released its name while services still pointed
at it, leaving them on a namespace the account no longer held.

Deletion now refuses with 412 when a service in the same account uses the
domain or a subdomain, including disabled ones. Service writes revalidate
authorization inside their transaction and hold a shared lock on the
matching registrations, so a delete racing a create cannot strand either.
The dependency lookup is account-scoped: registrations are unique by name,
so another account can hold team.example.com under example.com and its
services are authorized by its own registration.

management/server/store/sql_store_custom_domain.go

@@ -4,13 +4,16 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/rs/xid"
 	log "github.com/sirupsen/logrus"
 	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
 
 	"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
+	nbdomain "github.com/netbirdio/netbird/shared/management/domain"
 	"github.com/netbirdio/netbird/shared/management/status"
 )
 
@@ -131,16 +134,45 @@ func (s *SqlStore) UpdateCustomDomain(ctx context.Context, accountID string, d *
 	return d, nil
 }
 
-func (s *SqlStore) DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error {
-	result := s.db.Delete(domain.Domain{}, accountAndIDQueryCondition, accountID, domainID)
-	if result.Error != nil {
-		log.WithContext(ctx).Errorf("failed to delete reverse proxy custom domain from store: %v", result.Error)
-		return status.Errorf(status.Internal, "failed to delete reverse proxy custom domain from store")
+// LockCustomDomains holds shared locks on registrations covering a service until commit.
+func (s *SqlStore) LockCustomDomains(ctx context.Context, accountID string, serviceDomain nbdomain.Domain) ([]*domain.Domain, error) {
+	var names []string
+	for name := serviceDomain.PunycodeString(); name != ""; {
+		names = append(names, name)
+		_, name, _ = strings.Cut(name, ".")
 	}
 
-	if result.RowsAffected == 0 {
-		return status.Errorf(status.NotFound, "reverse proxy custom domain %s not found", domainID)
+	var domains []*domain.Domain
+	if err := s.db.WithContext(ctx).Clauses(clause.Locking{Strength: string(LockingStrengthShare)}).
+		Where(accountIDCondition, accountID).Where("domain IN ?", names).
+		Order("id").Find(&domains).Error; err != nil {
+		return nil, fmt.Errorf("lock custom domains: %w", err)
 	}
+	return domains, nil
+}
 
-	return nil
+// DeleteCustomDomain removes a registration only when no service uses its namespace.
+func (s *SqlStore) DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error {
+	return s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+		var d domain.Domain
+		// Service writes hold a shared lock on this row through commit, so neither
+		// operation can proceed against the other's outdated view of the domain.
+		if err := tx.Clauses(clause.Locking{Strength: string(LockingStrengthUpdate)}).
+			Take(&d, accountAndIDQueryCondition, accountID, domainID).Error; err != nil {
+			if errors.Is(err, gorm.ErrRecordNotFound) {
+				return status.Errorf(status.NotFound, "custom domain not found")
+			}
+			return fmt.Errorf("lock custom domain for deletion: %w", err)
+		}
+
+		result := tx.Where(accountAndIDQueryCondition, accountID, domainID).
+			Where("NOT EXISTS (?)", customDomainServices(tx, &d).Select("1")).Delete(&domain.Domain{})
+		if result.Error != nil {
+			return fmt.Errorf("delete custom domain: %w", result.Error)
+		}
+		if result.RowsAffected == 0 {
+			return status.Errorf(status.PreconditionFailed, "custom domain has dependent services; delete or move them before deleting the domain")
+		}
+		return nil
+	})
 }

management/server/store/sql_store_custom_domain_test.go

@@ -0,0 +1,182 @@
+package store
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
+	"github.com/netbirdio/netbird/management/server/types"
+	nbdomain "github.com/netbirdio/netbird/shared/management/domain"
+	"github.com/netbirdio/netbird/shared/management/status"
+)
+
+func TestLockCustomDomains_ConcurrentServices(t *testing.T) {
+	runTestForAllEngines(t, "", func(t *testing.T, store Store) {
+		if store.GetStoreEngine() == types.SqliteStoreEngine {
+			t.Skip("SQLite serializes transactions on one connection")
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+		defer cancel()
+		require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", "")))
+		_, err := store.CreateCustomDomain(ctx, "owner", "one.example.com", "cluster", true)
+		require.NoError(t, err)
+		_, err = store.CreateCustomDomain(ctx, "owner", "two.example.com", "cluster", true)
+		require.NoError(t, err)
+
+		locked := make(chan error, 1)
+		release := make(chan struct{})
+		done := make(chan error, 1)
+		go func() {
+			done <- store.ExecuteInTransaction(ctx, func(tx Store) error {
+				_, err := tx.LockCustomDomains(ctx, "owner", "app.one.example.com")
+				locked <- err
+				if err != nil {
+					return err
+				}
+				select {
+				case <-release:
+					return nil
+				case <-ctx.Done():
+					return ctx.Err()
+				}
+			})
+		}()
+		var lockErr error
+		select {
+		case lockErr = <-locked:
+		case err := <-done:
+			t.Fatalf("transaction ended before locking: %v", err)
+		}
+		writeCtx, writeCancel := context.WithTimeout(ctx, 3*time.Second)
+		defer writeCancel()
+		var writeErr error
+		for _, name := range []nbdomain.Domain{"app.one.example.com", "app.two.example.com"} {
+			writeErr = store.ExecuteInTransaction(writeCtx, func(tx Store) error {
+				if _, err := tx.LockCustomDomains(writeCtx, "owner", name); err != nil {
+					return err
+				}
+				return tx.CreateService(writeCtx, &rpservice.Service{
+					ID: name.PunycodeString(), AccountID: "owner", Domain: name.PunycodeString(),
+				})
+			})
+			if writeErr != nil {
+				break
+			}
+		}
+		close(release)
+		require.NoError(t, <-done)
+		require.NoError(t, lockErr)
+		require.NoError(t, writeErr, "domain authorization locks must allow concurrent service writes")
+		services, err := store.GetAccountServices(ctx, LockingStrengthNone, "owner")
+		require.NoError(t, err)
+		assert.Len(t, services, 2, "both services must commit while the first domain is locked")
+	})
+}
+
+func TestDeleteCustomDomain_ServiceDependencies(t *testing.T) {
+	runTestForAllEngines(t, "", func(t *testing.T, store Store) {
+		ctx := context.Background()
+		require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", "")))
+		d, err := store.CreateCustomDomain(ctx, "owner", "example.com", "cluster", true)
+		require.NoError(t, err)
+		svc := &rpservice.Service{ID: "service", AccountID: "owner", Domain: "APP.EXAMPLE.COM."}
+		require.NoError(t, store.CreateService(ctx, svc))
+
+		err = store.DeleteCustomDomain(ctx, "other", d.ID)
+		require.Error(t, err)
+		sErr, ok := status.FromError(err)
+		require.True(t, ok, "cross-account deletion must return a typed error")
+		assert.Equal(t, status.NotFound, sErr.Type(), "cross-account deletion must not reveal dependencies")
+
+		err = store.DeleteCustomDomain(ctx, "owner", d.ID)
+		require.Error(t, err)
+		sErr, ok = status.FromError(err)
+		require.True(t, ok, "dependent services must return a typed error")
+		assert.Equal(t, status.PreconditionFailed, sErr.Type(), "deletion must fail until services are removed")
+		stored, err := store.GetCustomDomain(ctx, "owner", d.ID)
+		require.NoError(t, err)
+		assert.True(t, stored.Validated, "rejected deletion must preserve validation")
+
+		require.NoError(t, store.DeleteService(ctx, "owner", svc.ID))
+		require.NoError(t, store.DeleteCustomDomain(ctx, "owner", d.ID))
+		_, err = store.GetCustomDomain(ctx, "owner", d.ID)
+		require.Error(t, err, "the registration must be gone after successful deletion")
+	})
+}
+
+func TestDeleteCustomDomain_OtherAccountSubdomainIsNotADependency(t *testing.T) {
+	runTestForAllEngines(t, "", func(t *testing.T, store Store) {
+		ctx := context.Background()
+		require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", "")))
+		require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "other", "admin", "")))
+
+		// Registrations are unique by name, so a second account can hold a
+		// subdomain of the first account's registration and serve from it.
+		parent, err := store.CreateCustomDomain(ctx, "owner", "example.com", "cluster", true)
+		require.NoError(t, err)
+		_, err = store.CreateCustomDomain(ctx, "other", "team.example.com", "cluster", true)
+		require.NoError(t, err)
+		require.NoError(t, store.CreateService(ctx, &rpservice.Service{
+			ID: "service", AccountID: "other", Domain: "app.team.example.com",
+		}))
+
+		require.NoError(t, store.DeleteCustomDomain(ctx, "owner", parent.ID),
+			"another account's service must not hold the registration open")
+	})
+}
+
+func TestDeleteCustomDomain_ConcurrentServiceCreation(t *testing.T) {
+	runTestForAllEngines(t, "", func(t *testing.T, store Store) {
+		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		defer cancel()
+		require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", "")))
+		for i := range 10 {
+			d, err := store.CreateCustomDomain(ctx, "owner", fmt.Sprintf("app%d.example.com", i), "cluster", true)
+			require.NoError(t, err)
+			svc := &rpservice.Service{ID: fmt.Sprintf("service-%d", i), AccountID: "owner", Domain: "nested." + d.Domain}
+			start := make(chan struct{})
+			created := make(chan error, 1)
+			deleted := make(chan error, 1)
+			go func() {
+				<-start
+				created <- store.ExecuteInTransaction(ctx, func(tx Store) error {
+					domains, err := tx.LockCustomDomains(ctx, "owner", nbdomain.Domain(svc.Domain))
+					if err != nil {
+						return err
+					}
+					for _, candidate := range domains {
+						if candidate.ID == d.ID && candidate.Validated {
+							return tx.CreateService(ctx, svc)
+						}
+					}
+					return status.Errorf(status.PreconditionFailed, "registration was deleted")
+				})
+			}()
+			go func() {
+				<-start
+				deleted <- store.DeleteCustomDomain(ctx, "owner", d.ID)
+			}()
+			close(start)
+			createErr, deleteErr := <-created, <-deleted
+			require.True(t, createErr == nil || deleteErr == nil, "one operation must succeed: create=%v, delete=%v", createErr, deleteErr)
+			if createErr == nil {
+				require.Error(t, deleteErr, "a committed service must block deletion")
+				stored, err := store.GetCustomDomain(ctx, "owner", d.ID)
+				require.NoError(t, err)
+				assert.True(t, stored.Validated, "the service must retain its authorization")
+				require.NoError(t, store.DeleteService(ctx, "owner", svc.ID))
+				require.NoError(t, store.DeleteCustomDomain(ctx, "owner", d.ID))
+				continue
+			}
+			require.NoError(t, deleteErr)
+			services, err := store.GetAccountServices(ctx, LockingStrengthNone, "owner")
+			require.NoError(t, err)
+			assert.Empty(t, services, "a deleted registration must not leave a new service")
+		}
+	})
+}

management/server/store/sql_store_domain_expiration.go

@@ -53,7 +53,10 @@ func customDomainServices(db *gorm.DB, d *domain.Domain) *gorm.DB {
 	// Shared domain validation permits underscores, and older rows may contain
 	// other LIKE metacharacters.
 	escaped := strings.NewReplacer("!", "!!", "%", "!%", "_", "!_").Replace(name)
-	return db.Model(&rpservice.Service{}).Where(
+	// Registrations are unique by name, so another account can hold a subdomain
+	// of this one and serve from it. Its services derive their cluster from that
+	// account's own registration and are not dependents of this one.
+	return db.Model(&rpservice.Service{}).Where(accountIDCondition, d.AccountID).Where(
 		"LOWER(domain) IN ? OR LOWER(domain) LIKE ? ESCAPE '!' OR LOWER(domain) LIKE ? ESCAPE '!'",
 		[]string{name, name + "."}, "%."+escaped, "%."+escaped+".",
 	)

management/server/store/store.go

@@ -36,6 +36,7 @@ import (
 	"github.com/netbirdio/netbird/management/server/telemetry"
 	"github.com/netbirdio/netbird/management/server/testutil"
 	"github.com/netbirdio/netbird/management/server/types"
+	nbdomain "github.com/netbirdio/netbird/shared/management/domain"
 	"github.com/netbirdio/netbird/util"
 	"github.com/netbirdio/netbird/util/crypt"
 
@@ -307,6 +308,7 @@ type Store interface {
 	GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error)
 	ListFreeDomains(ctx context.Context, accountID string) ([]string, error)
 	ListCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error)
+	LockCustomDomains(ctx context.Context, accountID string, serviceDomain nbdomain.Domain) ([]*domain.Domain, error)
 	GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error)
 	CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error)
 	UpdateCustomDomain(ctx context.Context, accountID string, d *domain.Domain) (*domain.Domain, error)

management/server/store/store_mock.go

@@ -31,6 +31,7 @@ import (
 	posture "github.com/netbirdio/netbird/management/server/posture"
 	types3 "github.com/netbirdio/netbird/management/server/types"
 	route "github.com/netbirdio/netbird/route"
+	domain0 "github.com/netbirdio/netbird/shared/management/domain"
 	crypt "github.com/netbirdio/netbird/util/crypt"
 	gomock "go.uber.org/mock/gomock"
 )
@@ -3302,6 +3303,21 @@ func (mr *MockStoreMockRecorder) ListFreeDomains(ctx, accountID any) *gomock.Cal
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListFreeDomains", reflect.TypeOf((*MockStore)(nil).ListFreeDomains), ctx, accountID)
 }
 
+// LockCustomDomains mocks base method.
+func (m *MockStore) LockCustomDomains(ctx context.Context, accountID string, serviceDomain domain0.Domain) ([]*domain.Domain, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "LockCustomDomains", ctx, accountID, serviceDomain)
+	ret0, _ := ret[0].([]*domain.Domain)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// LockCustomDomains indicates an expected call of LockCustomDomains.
+func (mr *MockStoreMockRecorder) LockCustomDomains(ctx, accountID, serviceDomain any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LockCustomDomains", reflect.TypeOf((*MockStore)(nil).LockCustomDomains), ctx, accountID, serviceDomain)
+}
+
 // MarkAccountPrimary mocks base method.
 func (m *MockStore) MarkAccountPrimary(ctx context.Context, accountID string) error {
 	m.ctrl.T.Helper()