From 21b4a83cea05cb2a3a54d2357c875f85ec2dd10f Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Thu, 10 Sep 2026 11:57:14 +0200 Subject: [PATCH] [management] Refuse services on unvalidated custom domains (#7341) Require validated custom domains when creating or updating reverse proxy services. Propagate validation errors during updates and return HTTP 409 for duplicate domain claims. Add regression tests for domain validation, ownership, and service creation and updates. --- .../domain/manager/domain_test.go | 50 ++- .../reverseproxy/domain/manager/manager.go | 64 +++- .../domain/manager/manager_realstore_test.go | 326 ++++++++++++++++++ .../domain/manager/manager_test.go | 4 + .../service/manager/domain_validation_test.go | 127 +++++++ .../reverseproxy/service/manager/manager.go | 19 +- management/server/store/sql_store.go | 29 ++ management/server/store/store.go | 1 + management/server/store/store_mock.go | 15 + 9 files changed, 612 insertions(+), 23 deletions(-) create mode 100644 management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go create mode 100644 management/internals/modules/reverseproxy/service/manager/domain_validation_test.go diff --git a/management/internals/modules/reverseproxy/domain/manager/domain_test.go b/management/internals/modules/reverseproxy/domain/manager/domain_test.go index 523920a99..38d5a923b 100644 --- a/management/internals/modules/reverseproxy/domain/manager/domain_test.go +++ b/management/internals/modules/reverseproxy/domain/manager/domain_test.go @@ -66,8 +66,8 @@ func TestExtractClusterFromFreeDomain(t *testing.T) { func TestExtractClusterFromCustomDomains(t *testing.T) { customDomains := []*domain.Domain{ - {Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io"}, - {Domain: "proxy.corp.io", TargetCluster: "us1.proxy.netbird.io"}, + {Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io", Validated: true}, + {Domain: "proxy.corp.io", TargetCluster: "us1.proxy.netbird.io", Validated: true}, } tests := []struct { @@ -120,19 +120,49 @@ func TestExtractClusterFromCustomDomains(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - cluster, ok := extractClusterFromCustomDomains(tc.domain, customDomains) - assert.Equal(t, tc.wantOK, ok) - if ok { - assert.Equal(t, tc.wantVal, cluster) + cluster, match := extractClusterFromCustomDomains(tc.domain, customDomains) + if !tc.wantOK { + assert.Equal(t, customDomainNoMatch, match, "unrelated domain should not match any custom domain") + return } + assert.Equal(t, customDomainValidated, match, "validated custom domain should resolve a cluster") + assert.Equal(t, tc.wantVal, cluster) }) } } +// An unvalidated row must never yield a cluster: the account has not shown it +// controls the name, so no service may be bound to it. +func TestExtractClusterFromCustomDomains_UnvalidatedDomainRefused(t *testing.T) { + customDomains := []*domain.Domain{ + {Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io", Validated: false}, + } + + for _, serviceDomain := range []string{"example.com", "app.example.com"} { + t.Run(serviceDomain, func(t *testing.T) { + cluster, match := extractClusterFromCustomDomains(serviceDomain, customDomains) + assert.Equal(t, customDomainUnvalidated, match, "unvalidated row must be reported as such") + assert.Empty(t, cluster, "unvalidated row must not resolve a cluster") + }) + } +} + +// A more specific unvalidated row must not shadow a validated parent domain. +func TestExtractClusterFromCustomDomains_ValidatedParentWinsOverUnvalidatedChild(t *testing.T) { + customDomains := []*domain.Domain{ + {Domain: "example.com", TargetCluster: "cluster-generic", Validated: true}, + {Domain: "app.example.com", TargetCluster: "cluster-app", Validated: false}, + } + + cluster, match := extractClusterFromCustomDomains("app.example.com", customDomains) + assert.Equal(t, customDomainValidated, match) + assert.Equal(t, "cluster-generic", cluster, "validated parent domain should provide the cluster") +} + func TestExtractClusterFromCustomDomains_OverlappingDomains(t *testing.T) { customDomains := []*domain.Domain{ - {Domain: "example.com", TargetCluster: "cluster-generic"}, - {Domain: "app.example.com", TargetCluster: "cluster-app"}, + {Domain: "example.com", TargetCluster: "cluster-generic", Validated: true}, + {Domain: "app.example.com", TargetCluster: "cluster-app", Validated: true}, } tests := []struct { @@ -164,8 +194,8 @@ func TestExtractClusterFromCustomDomains_OverlappingDomains(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - cluster, ok := extractClusterFromCustomDomains(tc.domain, customDomains) - assert.True(t, ok) + cluster, match := extractClusterFromCustomDomains(tc.domain, customDomains) + assert.Equal(t, customDomainValidated, match) assert.Equal(t, tc.wantVal, cluster) }) } diff --git a/management/internals/modules/reverseproxy/domain/manager/manager.go b/management/internals/modules/reverseproxy/domain/manager/manager.go index a9774d0e9..46e4ced83 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager.go @@ -26,6 +26,7 @@ type store interface { GetAgentNetworkSettings(ctx context.Context, lockStrength nbstore.LockingStrength, accountID string) (*agentnetworkTypes.Settings, error) GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error) + GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) ListFreeDomains(ctx context.Context, accountID string) ([]string, error) ListCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error) CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error) @@ -150,6 +151,10 @@ func (m Manager) CreateDomain(ctx context.Context, accountID, userID, domainName return nil, fmt.Errorf("target cluster %s is not available", targetCluster) } + if err := m.checkDomainAvailable(ctx, domainName); err != nil { + return nil, err + } + // Attempt an initial validation against the specified cluster only var validated bool if m.validator.IsValid(ctx, domainName, []string{targetCluster}) { @@ -166,6 +171,23 @@ func (m Manager) CreateDomain(ctx context.Context, accountID, userID, domainName return d, nil } +// checkDomainAvailable reports whether the domain is free to claim. The unique +// index on the column is the real guard; this turns the violation into a +// conflict the caller can act on instead of a database error, and says nothing +// about which account holds the domain. +func (m Manager) checkDomainAvailable(ctx context.Context, domainName string) error { + _, err := m.store.GetCustomDomainByName(ctx, domainName) + if err == nil { + return status.Errorf(status.AlreadyExists, "domain %s is already registered", domainName) + } + + if sErr, ok := status.FromError(err); ok && sErr.Type() == status.NotFound { + return nil + } + + return fmt.Errorf("look up domain: %w", err) +} + func (m Manager) DeleteDomain(ctx context.Context, accountID, userID, domainID string) error { ok, ctx, err := m.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, modules.Services, operations.Delete) if err != nil { @@ -203,7 +225,9 @@ func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID log.WithFields(log.Fields{ "accountID": accountID, "domainID": domainID, - }).WithError(err).Error("validate domain") + "userID": userID, + }).Error("validate domain: permission denied") + return } log.WithFields(log.Fields{ @@ -298,9 +322,12 @@ func (m Manager) DeriveClusterFromDomain(ctx context.Context, accountID, domain return "", fmt.Errorf("list custom domains: %w", err) } - targetCluster, valid := extractClusterFromCustomDomains(domain, customDomains) - if valid { + targetCluster, match := extractClusterFromCustomDomains(domain, customDomains) + switch match { + case customDomainValidated: return targetCluster, nil + case customDomainUnvalidated: + return "", status.Errorf(status.PreconditionFailed, "domain %s is not validated", domain) } return "", fmt.Errorf("domain %s does not match any available proxy cluster", domain) @@ -363,19 +390,46 @@ func (m Manager) reservedGatewayAddress(ctx context.Context, accountID string) ( return settings.ProxyAddress, nil } -func extractClusterFromCustomDomains(serviceDomain string, customDomains []*domain.Domain) (string, bool) { +// customDomainMatch describes how a service domain relates to the account's +// custom domain rows. +type customDomainMatch int + +const ( + customDomainNoMatch customDomainMatch = iota + customDomainUnvalidated + customDomainValidated +) + +// extractClusterFromCustomDomains finds the longest custom domain covering the +// service domain and reports its target cluster. Only a validated row yields a +// cluster: until the CNAME check has passed the account has not shown it +// controls the name, so no traffic may be routed for it. +func extractClusterFromCustomDomains(serviceDomain string, customDomains []*domain.Domain) (string, customDomainMatch) { bestCluster := "" bestLen := -1 + matched := false for _, cd := range customDomains { if serviceDomain != cd.Domain && !strings.HasSuffix(serviceDomain, "."+cd.Domain) { continue } + matched = true + if !cd.Validated { + continue + } if l := len(cd.Domain); l > bestLen { bestLen = l bestCluster = cd.TargetCluster } } - return bestCluster, bestLen >= 0 + + switch { + case bestLen >= 0: + return bestCluster, customDomainValidated + case matched: + return "", customDomainUnvalidated + default: + return "", customDomainNoMatch + } } // ExtractClusterFromFreeDomain extracts the cluster address from a free domain. diff --git a/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go b/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go new file mode 100644 index 000000000..8a0b56171 --- /dev/null +++ b/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go @@ -0,0 +1,326 @@ +package manager + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/metric/noop" + + "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" + proxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy/manager" + "github.com/netbirdio/netbird/management/server/activity" + "github.com/netbirdio/netbird/management/server/mock_server" + "github.com/netbirdio/netbird/management/server/permissions" + nbstore "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/status" +) + +const ( + testCluster = "eu.proxy.test" + accountA = "account-a" + accountAUser = "account-a-admin" + accountB = "account-b" + accountBUser = "account-b-admin" + accountAMember = "account-a-member" +) + +// stubResolver answers CNAME lookups from a table the test controls, so a +// domain can point at the cluster or nowhere without touching a real resolver. +type stubResolver struct { + mu sync.Mutex + cnames map[string]string +} + +func (r *stubResolver) LookupCNAME(_ context.Context, host string) (string, error) { + r.mu.Lock() + defer r.mu.Unlock() + + cname, ok := r.cnames[host] + if !ok { + return "", fmt.Errorf("lookup %s: no such host", host) + } + return cname + ".", nil +} + +func (r *stubResolver) set(host, cname string) { + r.mu.Lock() + defer r.mu.Unlock() + r.cnames[host] = cname +} + +type domainTestEnv struct { + manager Manager + store nbstore.Store + resolver *stubResolver +} + +// setupDomainTest builds the domain manager on a real SQLite store with two +// accounts and one active public proxy cluster. +func setupDomainTest(t *testing.T) *domainTestEnv { + t.Helper() + + ctx := context.Background() + testStore, cleanup, err := nbstore.NewTestStoreFromSQL(ctx, "", t.TempDir()) + require.NoError(t, err) + t.Cleanup(cleanup) + + for accountID, userID := range map[string]string{accountA: accountAUser, accountB: accountBUser} { + users := map[string]*types.User{ + userID: { + Id: userID, + AccountID: accountID, + Role: types.UserRoleAdmin, + }, + } + if accountID == accountA { + // A real member of the account whose role denies Services:Create, so + // permission denial is exercised as ok=false rather than as a lookup + // error for a user who is not in the account at all. + users[accountAMember] = &types.User{ + Id: accountAMember, + AccountID: accountID, + Role: types.UserRoleUser, + } + } + + require.NoError(t, testStore.SaveAccount(ctx, &types.Account{ + Id: accountID, + CreatedBy: userID, + Settings: &types.Settings{}, + Users: users, + })) + } + + proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter("")) + require.NoError(t, err) + + _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", testCluster, "127.0.0.1", nil, nil) + require.NoError(t, err) + + resolver := &stubResolver{cnames: make(map[string]string)} + + mgr := Manager{ + store: testStore, + proxyManager: proxyMgr, + validator: domain.Validator{Resolver: resolver}, + permissionsManager: permissions.NewManager(testStore), + accountManager: &mock_server.MockAccountManager{ + StoreEventFunc: func(context.Context, string, string, string, activity.ActivityDescriber, map[string]any) {}, + }, + } + + return &domainTestEnv{manager: mgr, store: testStore, resolver: resolver} +} + +// storedDomain reads a domain row back through the store so assertions are made +// on what was persisted rather than on the value the manager returned. +func storedDomain(t *testing.T, s nbstore.Store, accountID, domainName string) *domain.Domain { + t.Helper() + + domains, err := s.ListCustomDomains(context.Background(), accountID) + require.NoError(t, err) + for _, d := range domains { + if d.Domain == domainName { + return d + } + } + return nil +} + +// A domain whose CNAME check fails is stored unvalidated and must not resolve a +// cluster, which is what service creation gates on. +func TestCreateDomain_FailedLookupIsNotServable(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "apps.example.com", testCluster) + require.NoError(t, err) + assert.False(t, created.Validated, "a domain whose CNAME lookup fails must not be created validated") + + stored := storedDomain(t, env.store, accountA, "apps.example.com") + require.NotNil(t, stored, "domain row should exist") + assert.False(t, stored.Validated, "persisted row must be unvalidated") + + cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "apps.example.com") + require.Error(t, err, "an unvalidated domain must not resolve a cluster") + assert.Empty(t, cluster) + assert.Contains(t, err.Error(), "not validated", "error should tell the caller what to fix") + + sErr, ok := status.FromError(err) + require.True(t, ok, "error should be a typed status error") + assert.Equal(t, status.PreconditionFailed, sErr.Type()) + + _, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "sub.apps.example.com") + assert.Error(t, err, "subdomains of an unvalidated custom domain are not servable either") +} + +// A second account claiming a registered domain gets a clean conflict, not a +// database error surfaced as a 500. +func TestCreateDomain_DuplicateIsAConflict(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + _, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "shared.example.com", testCluster) + require.NoError(t, err) + + _, err = env.manager.CreateDomain(ctx, accountB, accountBUser, "shared.example.com", testCluster) + require.Error(t, err) + + sErr, ok := status.FromError(err) + require.True(t, ok, "conflict must be a typed status error, not a raw database error") + assert.Equal(t, status.AlreadyExists, sErr.Type(), "conflict should map to 409, not 500") + assert.NotContains(t, sErr.Message, accountA, "the response must not reveal the holding account") + + assert.Nil(t, storedDomain(t, env.store, accountB, "shared.example.com"), "no row should be written on conflict") +} + +// The same account re-adding one of its own domains is a conflict too. +func TestCreateDomain_SameAccountDuplicateIsAConflict(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + _, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "dup.example.com", testCluster) + require.NoError(t, err) + + _, err = env.manager.CreateDomain(ctx, accountA, accountAUser, "dup.example.com", testCluster) + require.Error(t, err) + + sErr, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, status.AlreadyExists, sErr.Type()) +} + +// The negative control: a validated domain still derives its cluster, for the +// bare name and for subdomains, exactly as before. +func TestCreateDomain_ValidatedDomainDerivesCluster(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + env.resolver.set("validation.valid.example.com", testCluster) + + created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "valid.example.com", testCluster) + require.NoError(t, err) + require.True(t, created.Validated, "a matching CNAME should validate on create") + + cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "valid.example.com") + require.NoError(t, err) + assert.Equal(t, testCluster, cluster) + + cluster, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "app.valid.example.com") + require.NoError(t, err) + assert.Equal(t, testCluster, cluster, "subdomains of a validated custom domain resolve too") +} + +// Validating a domain flips the gate: the same lookup that failed before now +// resolves a cluster. +func TestValidateDomain_UnlocksClusterDerivation(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "later.example.com", testCluster) + require.NoError(t, err) + require.False(t, created.Validated) + + _, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "later.example.com") + require.Error(t, err) + + env.resolver.set("validation.later.example.com", testCluster) + env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID) + + require.True(t, storedDomain(t, env.store, accountA, "later.example.com").Validated) + + cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "later.example.com") + require.NoError(t, err) + assert.Equal(t, testCluster, cluster) +} + +// Free cluster domains are unaffected by the custom domain gate. +func TestDeriveClusterFromDomain_FreeDomainUnaffected(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "myapp.abc123."+testCluster) + require.NoError(t, err) + assert.Equal(t, testCluster, cluster) +} + +// The manager pre-check exists to turn a conflict into a 409, but the unique +// index on the column is what actually guarantees the domain is claimed once. +// +// Two requests can clear the pre-check concurrently and race to the insert. +// Inserting twice through the store reaches the same code path the loser of +// that race takes, without the nondeterminism of driving it from goroutines, +// and the loser must still see a conflict rather than an internal error. +func TestStore_DuplicateDomainRejectedByIndexAsConflict(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + _, err := env.store.CreateCustomDomain(ctx, accountA, "indexed.example.com", testCluster, false) + require.NoError(t, err) + + _, err = env.store.CreateCustomDomain(ctx, accountB, "indexed.example.com", testCluster, false) + require.Error(t, err, "the unique index must reject the same domain in a second account") + + sErr, ok := status.FromError(err) + require.True(t, ok, "the losing insert must return a typed status error") + assert.Equal(t, status.AlreadyExists, sErr.Type(), "a lost race is a 409, not a 500") +} + +// Validation is what decides whether a domain routes traffic, so a caller +// without permission to it must not be able to flip the flag. The check logged +// the denial and then carried on, which was inert while nothing read Validated +// and is not once cluster derivation gates on it. +func TestValidateDomain_PermissionDeniedDoesNotValidate(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "guarded.example.com", testCluster) + require.NoError(t, err) + require.False(t, created.Validated) + + // The CNAME is in place, so the only thing standing between this caller and + // a validated domain is the permission check. + env.resolver.set("validation.guarded.example.com", testCluster) + + env.manager.ValidateDomain(ctx, accountA, accountAMember, created.ID) + + stored := storedDomain(t, env.store, accountA, "guarded.example.com") + require.NotNil(t, stored) + assert.False(t, stored.Validated, "a caller without permission must not validate the domain") + + _, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "guarded.example.com") + assert.Error(t, err, "the domain must still be unservable") +} + +// Validation runs asynchronously, so it can finish after the domain was +// deleted and then write a stale row back. gorm's Save falls back to an insert +// when an update affects no rows, which would resurrect the domain as +// validated; UpdateCustomDomain avoids that by selecting explicit columns. +// This pins that behaviour, since dropping the Select would reintroduce it. +func TestUpdateCustomDomain_DoesNotResurrectDeletedDomain(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + + created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "racy.example.com", testCluster) + require.NoError(t, err) + + stale := storedDomain(t, env.store, accountA, "racy.example.com") + require.NotNil(t, stale) + + require.NoError(t, env.manager.DeleteDomain(ctx, accountA, accountAUser, created.ID)) + require.Nil(t, storedDomain(t, env.store, accountA, "racy.example.com"), "the domain should be gone") + + // What an in-flight validation would write once its CNAME check succeeded. + // The write has to succeed for the assertion below to mean anything: a + // rejected write would leave the domain absent for the wrong reason. + stale.Validated = true + _, err = env.store.UpdateCustomDomain(ctx, accountA, stale) + require.NoError(t, err, "the update itself must succeed, so absence is not just a failed write") + + assert.Nil(t, storedDomain(t, env.store, accountA, "racy.example.com"), + "a late validation write must not recreate a deleted domain") +} diff --git a/management/internals/modules/reverseproxy/domain/manager/manager_test.go b/management/internals/modules/reverseproxy/domain/manager/manager_test.go index 12281b447..519f5efeb 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager_test.go @@ -184,6 +184,10 @@ func (s *stubStore) GetCustomDomain(context.Context, string, string) (*domain.Do panic("not used in allow-list tests") } +func (s *stubStore) GetCustomDomainByName(context.Context, string) (*domain.Domain, error) { + panic("not used in allow-list tests") +} + func (s *stubStore) ListFreeDomains(context.Context, string) ([]string, error) { panic("not used in allow-list tests") } diff --git a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go new file mode 100644 index 000000000..ccb955cd8 --- /dev/null +++ b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go @@ -0,0 +1,127 @@ +package manager + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/metric/noop" + + domainmanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain/manager" + proxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy/manager" + rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" + "github.com/netbirdio/netbird/management/server/activity" + "github.com/netbirdio/netbird/management/server/mock_server" + "github.com/netbirdio/netbird/management/server/permissions" + "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/shared/management/status" +) + +const validationTestCluster = "eu.proxy.test" + +// withRealDomainManager swaps the stub cluster deriver for the real domain +// manager backed by the same store, so service creation is gated by the actual +// domain rows rather than by a test double that always agrees. +func withRealDomainManager(t *testing.T, mgr *Manager, testStore store.Store) { + t.Helper() + + ctx := context.Background() + proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter("")) + require.NoError(t, err) + + _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", validationTestCluster, "127.0.0.1", nil, nil) + require.NoError(t, err) + + accountMgr := &mock_server.MockAccountManager{ + StoreEventFunc: func(context.Context, string, string, string, activity.ActivityDescriber, map[string]any) {}, + } + mgr.clusterDeriver = domainmanager.NewManager(testStore, proxyMgr, permissions.NewManager(testStore), accountMgr) +} + +func newTestService(domain string) *rpservice.Service { + return &rpservice.Service{ + Name: "test-service", + Domain: domain, + Enabled: true, + Mode: rpservice.ModeHTTP, + Targets: []*rpservice.Target{{ + Host: "10.0.0.1", + Port: 8080, + Protocol: "http", + TargetId: testPeerID, + TargetType: "peer", + Enabled: true, + }}, + } +} + +// A service must not bind to a domain the account has not validated, and +// nothing may be persisted for the attempt. +func TestCreateService_RefusesUnvalidatedDomain(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + + _, err := testStore.CreateCustomDomain(ctx, testAccountID, "unproven.example.com", validationTestCluster, false) + require.NoError(t, err) + + _, err = mgr.CreateService(ctx, testAccountID, testUserID, newTestService("unproven.example.com")) + require.Error(t, err, "an unvalidated domain must not bind a service") + assert.Contains(t, err.Error(), "not validated", "the API error should name the actual problem") + + sErr, ok := status.FromError(err) + require.True(t, ok, "error should be a typed status error") + assert.Equal(t, status.PreconditionFailed, sErr.Type()) + + services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID) + require.NoError(t, err) + assert.Empty(t, services, "no service row should be written for a refused domain") +} + +// The negative control: a validated domain still binds a service and derives +// its cluster exactly as before. +func TestCreateService_ValidatedDomainBindsService(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + + _, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true) + require.NoError(t, err) + + created, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.proven.example.com")) + require.NoError(t, err) + assert.Equal(t, validationTestCluster, created.ProxyCluster, "service should bind to the domain's target cluster") + + services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID) + require.NoError(t, err) + require.Len(t, services, 1, "the service should be persisted") + assert.Equal(t, "app.proven.example.com", services[0].Domain) +} + +// An update must not be a way around the creation gate: moving a live service +// onto an unvalidated domain has to fail rather than silently keep the old +// cluster and start serving the new hostname. +func TestUpdateService_RefusesMoveToUnvalidatedDomain(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + + _, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true) + require.NoError(t, err) + _, err = testStore.CreateCustomDomain(ctx, testAccountID, "unproven.example.com", validationTestCluster, false) + require.NoError(t, err) + + created, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.proven.example.com")) + require.NoError(t, err) + + moved := *created + moved.Domain = "app.unproven.example.com" + _, err = mgr.UpdateService(ctx, testAccountID, testUserID, &moved) + require.Error(t, err, "moving to an unvalidated domain must fail") + assert.Contains(t, err.Error(), "not validated") + + stored, err := testStore.GetServiceByID(ctx, store.LockingStrengthNone, testAccountID, created.ID) + require.NoError(t, err) + assert.Equal(t, "app.proven.example.com", stored.Domain, "the service must keep its original domain") +} diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index 365fbab40..9c7f95eb4 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -606,16 +606,19 @@ func (m *Manager) resolveEffectiveCluster(ctx context.Context, accountID string, return existing.ProxyCluster, nil } - if m.clusterDeriver != nil { - derived, err := m.clusterDeriver.DeriveClusterFromDomain(ctx, accountID, svc.Domain) - if err != nil { - log.WithError(err).Warnf("could not derive cluster from domain %s", svc.Domain) - } else { - return derived, nil - } + if m.clusterDeriver == nil { + return existing.ProxyCluster, nil } - return existing.ProxyCluster, nil + // Falling back to the old cluster here would let an update move a service + // onto a domain the account has not validated, bypassing the check that + // creation makes. + derived, err := m.clusterDeriver.DeriveClusterFromDomain(ctx, accountID, svc.Domain) + if err != nil { + return "", status.Errorf(status.PreconditionFailed, "could not derive cluster from domain %s: %v", svc.Domain, err) + } + + return derived, nil } func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo, customPorts *bool, effectiveCluster string) error { diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 6337ebf1a..ef353ea83 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -5686,6 +5686,23 @@ func (s *SqlStore) ListCustomDomains(ctx context.Context, accountID string) ([]* return domains, nil } +// GetCustomDomainByName returns the custom domain row holding the given name, +// regardless of which account owns it. +func (s *SqlStore) GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) { + customDomain := &domain.Domain{} + result := s.db.Take(customDomain, "domain = ?", domainName) + if result.Error != nil { + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return nil, status.Errorf(status.NotFound, "custom domain %s not found", domainName) + } + + log.WithContext(ctx).Errorf("failed to get custom domain by name from store: %v", result.Error) + return nil, status.Errorf(status.Internal, "failed to get custom domain from store") + } + + return customDomain, nil +} + func (s *SqlStore) CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error) { newDomain := &domain.Domain{ ID: xid.New().String(), // Generate our own ID because gorm doesn't always configure the database to handle this for us. @@ -5697,6 +5714,18 @@ func (s *SqlStore) CreateCustomDomain(ctx context.Context, accountID string, dom } result := s.db.Create(newDomain) if result.Error != nil { + // The unique index is the last guard when two requests clear the + // manager's availability check at the same time. The one that loses the + // insert is a conflict, not an internal failure. + var count int64 + if err := s.db.Model(&domain.Domain{}).Where("domain = ?", domainName).Count(&count).Error; err == nil && count > 0 { + // The insert error is logged even on this path: the name being taken + // is what the caller has to act on, but if the insert also failed for + // an unrelated reason the operator still needs to see it. + log.WithContext(ctx).Warnf("create reverse proxy custom domain %s rejected, name already registered: %v", domainName, result.Error) + return nil, status.Errorf(status.AlreadyExists, "domain %s is already registered", domainName) + } + log.WithContext(ctx).Errorf("failed to create reverse proxy custom domain to store: %v", result.Error) return nil, status.Errorf(status.Internal, "failed to create reverse proxy custom domain to store") } diff --git a/management/server/store/store.go b/management/server/store/store.go index 7daeb28a9..da2b3c6e0 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) + 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) DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 70acb9f58..9bf49f076 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -1941,6 +1941,21 @@ func (mr *MockStoreMockRecorder) GetCustomDomain(ctx, accountID, domainID any) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCustomDomain", reflect.TypeOf((*MockStore)(nil).GetCustomDomain), ctx, accountID, domainID) } +// GetCustomDomainByName mocks base method. +func (m *MockStore) GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCustomDomainByName", ctx, domainName) + ret0, _ := ret[0].(*domain.Domain) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetCustomDomainByName indicates an expected call of GetCustomDomainByName. +func (mr *MockStoreMockRecorder) GetCustomDomainByName(ctx, domainName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCustomDomainByName", reflect.TypeOf((*MockStore)(nil).GetCustomDomainByName), ctx, domainName) +} + // GetCustomDomainsCounts mocks base method. func (m *MockStore) GetCustomDomainsCounts(ctx context.Context) (int64, int64, error) { m.ctrl.T.Helper()