diff --git a/docs/custom-domain-validation.md b/docs/custom-domain-validation.md index a70388e4d..4b0f95ae3 100644 --- a/docs/custom-domain-validation.md +++ b/docs/custom-domain-validation.md @@ -26,3 +26,8 @@ window. Restarting management does not extend a previously assigned deadline. Registrations with existing services, including services using subdomains, are retained for operator review. Management logs their account and domain IDs so an operator can identify and resolve those dependencies before cleanup. + +Manual deletion is also refused while any service uses the domain or a subdomain, +including disabled services. Delete those services or move them to another domain +before removing the registration. A refused deletion returns HTTP 412 and leaves +the domain and its services unchanged; no deletion activity event is recorded. diff --git a/management/internals/modules/reverseproxy/domain/manager/deletion_test.go b/management/internals/modules/reverseproxy/domain/manager/deletion_test.go new file mode 100644 index 000000000..460e4dbdf --- /dev/null +++ b/management/internals/modules/reverseproxy/domain/manager/deletion_test.go @@ -0,0 +1,78 @@ +package manager + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" + "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/activity" + nbcontext "github.com/netbirdio/netbird/management/server/context" + nbstore "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/shared/auth" +) + +func TestDeleteDomain_ServiceDependencies(t *testing.T) { + for _, tt := range []struct { + name string + domainName string + serviceHost string + accountID string + enabled bool + protected bool + }{ + {"exact", "example.com", "example.com", accountA, true, true}, + {"subdomain", "example.com", "deep.app.example.com", accountA, true, true}, + {"disabled", "example.com", "app.example.com", accountA, false, true}, + {"other account", "example.com", "app.example.com", accountB, true, true}, + {"case and trailing dot", "example.com", "APP.EXAMPLE.COM.", accountA, true, true}, + {"suffix boundary", "example.com", "notexample.com", accountA, true, false}, + {"literal underscore", "a_b.example.com", "app.a_b.example.com", accountA, true, true}, + {"underscore wildcard", "a_b.example.com", "app.axb.example.com", accountA, true, false}, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + events := captureDomainEvents(env) + d, err := env.store.CreateCustomDomain(ctx, accountA, tt.domainName, testCluster, true) + require.NoError(t, err) + svc := &rpservice.Service{ + ID: "dependent", AccountID: tt.accountID, Domain: tt.serviceHost, + Enabled: tt.enabled, ProxyCluster: testCluster, + } + require.NoError(t, env.store.CreateService(ctx, svc)) + router := mux.NewRouter() + RegisterEndpoints(router, env.manager) + deleteDomain := func() *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodDelete, "/domains/"+d.ID, nil) + req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{AccountId: accountA, UserId: accountAUser}) + response := httptest.NewRecorder() + router.ServeHTTP(response, req) + return response + } + + response := deleteDomain() + if tt.protected { + require.Equal(t, http.StatusPreconditionFailed, response.Code, "dependent services must block deletion: %s", response.Body.String()) + assert.NotContains(t, response.Body.String(), tt.accountID, "the error must not reveal the service's account") + assert.NotNil(t, storedDomain(t, env.store, accountA, d.Domain), "the namespace must remain reserved") + assert.Empty(t, events.get(), "rejected deletion must not emit DomainDeleted") + stored, err := env.store.GetServiceByID(ctx, nbstore.LockingStrengthNone, tt.accountID, svc.ID) + require.NoError(t, err) + assert.Equal(t, svc.Enabled, stored.Enabled, "rejected deletion must preserve the service") + require.NoError(t, env.store.DeleteService(ctx, tt.accountID, svc.ID)) + response = deleteDomain() + } + require.Equal(t, http.StatusNoContent, response.Code, "deletion must succeed without dependencies: %s", response.Body.String()) + assert.Nil(t, storedDomain(t, env.store, accountA, d.Domain), "the registration must be deleted") + captured := events.get() + require.Len(t, captured, 1, "only successful deletion may emit an event") + assert.Equal(t, activity.DomainDeleted, captured[0].Activity, "the event must describe the successful deletion") + }) + } +} diff --git a/management/internals/modules/reverseproxy/domain/manager/manager.go b/management/internals/modules/reverseproxy/domain/manager/manager.go index c0fb12e9c..da07bb368 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager.go @@ -357,6 +357,22 @@ func (m Manager) DeriveClusterFromDomain(ctx context.Context, accountID, domain return "", fmt.Errorf("domain %s does not match any available proxy cluster", domain) } +// ValidateServiceDomain holds custom domain authorization through a service write transaction. +func (m Manager) ValidateServiceDomain(ctx context.Context, tx nbstore.Store, accountID, serviceDomain, cluster string) error { + if _, ok := ExtractClusterFromFreeDomain(serviceDomain, []string{cluster}); ok { + return nil + } + customDomains, err := tx.LockCustomDomains(ctx, accountID) + if err != nil { + return err + } + target, match := extractClusterFromCustomDomains(serviceDomain, customDomains) + if match != customDomainValidated || target != cluster { + return status.Errorf(status.PreconditionFailed, "custom domain authorization changed; retry the service operation") + } + return nil +} + func (m Manager) getClusterAllowList(ctx context.Context, accountID string) ([]string, error) { byopAddresses, err := m.proxyManager.GetActiveClusterAddressesForAccount(ctx, accountID) if err != nil { diff --git a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go index ccb955cd8..10bac864c 100644 --- a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go +++ b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go @@ -125,3 +125,53 @@ func TestUpdateService_RefusesMoveToUnvalidatedDomain(t *testing.T) { require.NoError(t, err) assert.Equal(t, "app.proven.example.com", stored.Domain, "the service must keep its original domain") } + +func TestCreateService_DomainDeletedBeforeWrite(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + + d, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true) + require.NoError(t, err) + svc := newTestService("app.proven.example.com") + require.NoError(t, mgr.initializeServiceForCreate(ctx, testAccountID, svc)) + + // Delete after the initial authorization check, before the service transaction starts. + require.NoError(t, testStore.DeleteCustomDomain(ctx, testAccountID, d.ID)) + err = mgr.persistNewService(ctx, testAccountID, svc) + require.Error(t, err, "an earlier validation result must not authorize a deleted registration") + sErr, ok := status.FromError(err) + require.True(t, ok, "the caller must receive a typed precondition error") + assert.Equal(t, status.PreconditionFailed, sErr.Type(), "the service must require current domain authorization") + services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID) + require.NoError(t, err) + assert.Empty(t, services, "the failed write must not leave a service") +} + +func TestUpdateService_DomainDeletedBeforeWrite(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + _, err := testStore.CreateCustomDomain(ctx, testAccountID, "original.example.com", validationTestCluster, true) + require.NoError(t, err) + d, err := testStore.CreateCustomDomain(ctx, testAccountID, "destination.example.com", validationTestCluster, true) + require.NoError(t, err) + svc, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.original.example.com")) + require.NoError(t, err) + moved := svc.Copy() + moved.Domain = "app.destination.example.com" + cluster, err := mgr.resolveEffectiveCluster(ctx, testAccountID, moved) + require.NoError(t, err) + + require.NoError(t, testStore.DeleteCustomDomain(ctx, testAccountID, d.ID)) + err = testStore.ExecuteInTransaction(ctx, func(tx store.Store) error { + return mgr.executeServiceUpdate(ctx, tx, testAccountID, moved, &serviceUpdateInfo{}, nil, cluster) + }) + require.Error(t, err, "a domain deleted after cluster resolution must reject the update") + sErr, ok := status.FromError(err) + require.True(t, ok, "the caller must receive a typed precondition error") + assert.Equal(t, status.PreconditionFailed, sErr.Type(), "the move must require current domain authorization") + stored, err := testStore.GetServiceByID(ctx, store.LockingStrengthNone, testAccountID, svc.ID) + require.NoError(t, err) + assert.Equal(t, svc.Domain, stored.Domain, "the service must retain its authorized domain") +} diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index 9c7f95eb4..62897c9ae 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -74,6 +74,7 @@ const unknownHostPlaceholder = "unknown" // ClusterDeriver derives the proxy cluster from a domain. type ClusterDeriver interface { DeriveClusterFromDomain(ctx context.Context, accountID, domain string) (string, error) + ValidateServiceDomain(ctx context.Context, tx store.Store, accountID, domain, cluster string) error GetClusterDomains() []string } @@ -332,6 +333,9 @@ func (m *Manager) persistNewService(ctx context.Context, accountID string, svc * } return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, svc, svc.ProxyCluster); err != nil { + return err + } if svc.Domain != "" { if err := m.checkDomainAvailable(ctx, transaction, svc.Domain, ""); err != nil { return err @@ -461,6 +465,9 @@ func (m *Manager) persistNewEphemeralService(ctx context.Context, accountID, pee } return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, svc, svc.ProxyCluster); err != nil { + return err + } if err := m.validateEphemeralPreconditions(ctx, transaction, accountID, peerID, svc); err != nil { return err } @@ -622,6 +629,9 @@ func (m *Manager) resolveEffectiveCluster(ctx context.Context, accountID string, } func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo, customPorts *bool, effectiveCluster string) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, service, effectiveCluster); err != nil { + return err + } existingService, err := transaction.GetServiceByID(ctx, store.LockingStrengthUpdate, accountID, service.ID) if err != nil { return err @@ -677,6 +687,13 @@ func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.St return nil } +func (m *Manager) validateServiceDomain(ctx context.Context, tx store.Store, accountID string, svc *service.Service, cluster string) error { + if m.clusterDeriver == nil { + return nil + } + return m.clusterDeriver.ValidateServiceDomain(ctx, tx, accountID, svc.Domain, cluster) +} + // validateL4PortDiffOnClusterDiff checks if custom L4 ports are configured and validates port changes across clusters. // It ensures no port changes if custom ports are unsupported for a given cluster and protocol mode. // Returns an error if validation fails, otherwise returns nil. diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index dd0edec60..9704796be 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -655,6 +655,10 @@ func (d *testClusterDeriver) GetClusterDomains() []string { return d.domains } +func (d *testClusterDeriver) ValidateServiceDomain(context.Context, store.Store, string, string, string) error { + return nil +} + const ( testAccountID = "test-account" testPeerID = "test-peer-1" diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 08ec45395..37617ef41 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -5757,20 +5757,6 @@ 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") - } - - if result.RowsAffected == 0 { - return status.Errorf(status.NotFound, "reverse proxy custom domain %s not found", domainID) - } - - return nil -} - // CreateAccessLog creates a new access log entry in the database func (s *SqlStore) CreateAccessLog(ctx context.Context, logEntry *accesslogs.AccessLogEntry) error { result := s.db.Create(logEntry) diff --git a/management/server/store/sql_store_domain_deletion.go b/management/server/store/sql_store_domain_deletion.go new file mode 100644 index 000000000..dcad69503 --- /dev/null +++ b/management/server/store/sql_store_domain_deletion.go @@ -0,0 +1,49 @@ +package store + +import ( + "context" + "errors" + "fmt" + + "gorm.io/gorm" + "gorm.io/gorm/clause" + + "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" + "github.com/netbirdio/netbird/shared/management/status" +) + +// LockCustomDomains locks an account's registrations until the caller's transaction ends. +func (s *SqlStore) LockCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error) { + var domains []*domain.Domain + if err := s.db.WithContext(ctx).Clauses(clause.Locking{Strength: string(LockingStrengthUpdate)}). + Where(accountIDCondition, accountID).Order("id").Find(&domains).Error; err != nil { + return nil, fmt.Errorf("lock custom domains: %w", err) + } + return domains, 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 take the same lock before checking validation, so neither + // operation can commit 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 + }) +} diff --git a/management/server/store/sql_store_domain_deletion_test.go b/management/server/store/sql_store_domain_deletion_test.go new file mode 100644 index 000000000..9f0638bea --- /dev/null +++ b/management/server/store/sql_store_domain_deletion_test.go @@ -0,0 +1,96 @@ +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/shared/management/status" +) + +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_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: 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") + 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") + } + }) +} diff --git a/management/server/store/store.go b/management/server/store/store.go index 6886536b9..fc050f3ba 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -302,6 +302,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) ([]*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) diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 04f79d30a..066e148f0 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3212,6 +3212,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) ([]*domain.Domain, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LockCustomDomains", ctx, accountID) + 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 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LockCustomDomains", reflect.TypeOf((*MockStore)(nil).LockCustomDomains), ctx, accountID) +} + // MarkAccountPrimary mocks base method. func (m *MockStore) MarkAccountPrimary(ctx context.Context, accountID string) error { m.ctrl.T.Helper() diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 5ad682c6b..c19df4fcf 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -13347,7 +13347,7 @@ paths: /api/reverse-proxies/domains/{domainId}: delete: summary: Delete a Custom domain - description: Delete an existing service custom domain + description: Delete an existing service custom domain after removing or moving all services that use it or its subdomains, including disabled services. tags: [ Services ] security: - BearerAuth: [ ] @@ -13370,6 +13370,9 @@ paths: "$ref": "#/components/responses/forbidden" '404': "$ref": "#/components/responses/not_found" + '412': + description: The domain or one of its subdomains is still used by a service + content: { } '500': "$ref": "#/components/responses/internal_error" /api/reverse-proxies/domains/{domainId}/validate: