[management] Return the reason when custom domain validation fails (#8122)

* [management] Return the reason when custom domain validation fails

The validate endpoint returned an empty 202 and ran the check in a
goroutine, so a failed verification only reached the management log.
Run the check in the request and return 204 or 412 with a classified
reason. Resolver errors are not forwarded.

* [management] Address review feedback on custom domain validation

Pass the request context to the lookup and store calls now that the
check runs in the request, compare the self-resolved name
case-insensitively, document the 412 like the other entries, and
check the returned error in the existing tests.

* [management] Match custom domain CNAME targets case-insensitively

DNS names are case-insensitive, so a CNAME pointing at the cluster in
different casing should validate instead of failing as a mismatch.

* [management] Add validation timeout

---------

Co-authored-by: DevBot NetBird <91386968+netbirddev@users.noreply.github.com>

management/internals/modules/reverseproxy/domain/manager/api.go

@@ -134,7 +134,10 @@ func (h *handler) triggerCustomDomainValidation(w http.ResponseWriter, r *http.R
 		return
 	}
 
-	go h.manager.ValidateDomain(r.Context(), userAuth.AccountId, userAuth.UserId, domainID)
+	if err := h.manager.ValidateDomain(r.Context(), userAuth.AccountId, userAuth.UserId, domainID); err != nil {
+		util.WriteError(r.Context(), err, w)
+		return
+	}
 
-	w.WriteHeader(http.StatusAccepted)
+	w.WriteHeader(http.StatusNoContent)
 }

management/internals/modules/reverseproxy/domain/manager/expiration_test.go

@@ -29,7 +29,7 @@ func TestValidateDomain_ExpiredRegistration(t *testing.T) {
 		Update("validation_expires_at", expiresAt).Error)
 	env.resolver.set("validation.expired.example.com", testCluster)
 
-	env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
+	assert.Error(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID), "an expired registration must be refused")
 
 	stored := storedDomain(t, env.store, accountA, d.Domain)
 	require.NotNil(t, stored)
@@ -47,7 +47,7 @@ func TestCreateDomain_ValidationDeadline(t *testing.T) {
 		assert.Equal(t, createdAt.Add(48*time.Hour), *d.ValidationExpiresAt, "new registrations get 48 hours")
 
 		time.Sleep(time.Hour)
-		env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
+		assert.Error(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID), "a missing CNAME must fail validation")
 		stored := storedDomain(t, env.store, accountA, d.Domain)
 		require.NotNil(t, stored)
 		require.NotNil(t, stored.ValidationExpiresAt)
@@ -198,9 +198,10 @@ func TestValidateDomain_DeadlinePassesDuringLookup(t *testing.T) {
 				resolver := blockingDomainResolver{started: make(chan struct{}), release: make(chan struct{})}
 				env.manager.validator.Resolver = resolver
 				done := make(chan struct{})
+				var validateErr error
 				go func() {
 					defer close(done)
-					env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
+					validateErr = env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
 				}()
 				<-resolver.started
 				time.Sleep(48 * time.Hour)
@@ -211,6 +212,7 @@ func TestValidateDomain_DeadlinePassesDuringLookup(t *testing.T) {
 				}
 				close(resolver.release)
 				<-done
+				assert.Error(t, validateErr, "a validation finishing after the deadline must be refused")
 				owner := accountA
 				if cleanup {
 					assert.Nil(t, storedDomain(t, env.store, accountA, d.Domain), "late validation must not restore the old claim")

management/internals/modules/reverseproxy/domain/manager/manager.go

@@ -227,22 +227,19 @@ func (m Manager) DeleteDomain(ctx context.Context, accountID, userID, domainID s
 	return nil
 }
 
-func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID string) {
-	ok, _, err := m.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, modules.Services, operations.Create)
+// validationTimeout bounds the DNS check so it ends, and reports why, before a proxy in
+// front of management times the request out.
+const validationTimeout = 10 * time.Second
+
+// ValidateDomain checks the domain's validation CNAME and marks it validated. A failure
+// the user can act on is returned as PreconditionFailed carrying the reason.
+func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID string) error {
+	ok, ctx, err := m.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, modules.Services, operations.Create)
 	if err != nil {
-		log.WithFields(log.Fields{
-			"accountID": accountID,
-			"domainID":  domainID,
-		}).WithError(err).Error("validate domain")
-		return
+		return status.NewPermissionValidationError(err)
 	}
 	if !ok {
-		log.WithFields(log.Fields{
-			"accountID": accountID,
-			"domainID":  domainID,
-			"userID":    userID,
-		}).Error("validate domain: permission denied")
-		return
+		return status.NewPermissionDeniedError()
 	}
 
 	log.WithFields(log.Fields{
@@ -250,32 +247,23 @@ func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID
 		"domainID":  domainID,
 	}).Info("starting domain validation")
 
-	d, err := m.store.GetCustomDomain(context.Background(), accountID, domainID)
+	d, err := m.store.GetCustomDomain(ctx, accountID, domainID)
 	if err != nil {
-		log.WithFields(log.Fields{
-			"accountID": accountID,
-			"domainID":  domainID,
-		}).WithError(err).Error("get custom domain from store")
-		return
+		return fmt.Errorf("get domain from store: %w", err)
 	}
 	if d.Validated {
-		return
+		return nil
 	}
 	if d.ValidationExpiresAt == nil || !time.Now().Before(*d.ValidationExpiresAt) {
-		log.WithFields(log.Fields{"accountID": accountID, "domainID": domainID}).
-			Debug("custom domain validation window has expired")
-		return
+		return validationFailed(domain.NewValidationError(domain.ValidationReasonExpired,
+			"custom domain %s validation window has expired; delete and add the domain again", d.Domain))
 	}
 
 	// Validate only against the domain's target cluster
 	targetCluster := d.TargetCluster
 	if targetCluster == "" {
-		log.WithFields(log.Fields{
-			"accountID": accountID,
-			"domainID":  domainID,
-			"domain":    d.Domain,
-		}).Warn("domain has no target cluster set, skipping validation")
-		return
+		return validationFailed(domain.NewValidationError(domain.ValidationReasonNoTargetCluster,
+			"custom domain %s has no target cluster", d.Domain))
 	}
 
 	log.WithFields(log.Fields{
@@ -285,32 +273,35 @@ func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID
 		"targetCluster": targetCluster,
 	}).Info("validating domain against target cluster")
 
-	if m.validator.IsValid(context.Background(), d.Domain, []string{targetCluster}) {
+	lookupCtx, cancel := context.WithTimeout(ctx, validationTimeout)
+	_, err = m.validator.Validate(lookupCtx, d.Domain, []string{targetCluster})
+	cancel()
+	if err == nil {
 		d.Validated = true
-		if _, err := m.store.UpdateCustomDomain(context.Background(), accountID, d); err != nil {
-			entry := log.WithFields(log.Fields{
-				"accountID": accountID,
-				"domainID":  domainID,
-			}).WithError(err)
-			if sErr, ok := status.FromError(err); ok && sErr.Type() == status.PreconditionFailed {
-				entry.Debug("custom domain registration is no longer pending validation")
-				return
+		if _, err := m.store.UpdateCustomDomain(ctx, accountID, d); err != nil {
+			// A concurrent request may have validated the domain first.
+			if current, getErr := m.store.GetCustomDomain(ctx, accountID, domainID); getErr == nil && current.Validated {
+				return nil
 			}
-			entry.Error("update custom domain in store")
-			return
+			return fmt.Errorf("update domain in store: %w", err)
 		}
 		log.WithFields(log.Fields{"accountID": accountID, "domainID": domainID}).
 			Info("custom domain validated successfully")
 
-		m.accountManager.StoreEvent(context.Background(), userID, domainID, accountID, activity.DomainValidated, d.EventMeta())
-	} else {
-		log.WithFields(log.Fields{
-			"accountID":     accountID,
-			"domainID":      domainID,
-			"domain":        d.Domain,
-			"targetCluster": targetCluster,
-		}).Warn("domain validation failed - CNAME does not match target cluster")
+		m.accountManager.StoreEvent(ctx, userID, domainID, accountID, activity.DomainValidated, d.EventMeta())
+		return nil
 	}
+
+	return validationFailed(err)
+}
+
+// validationFailed shows a classified failure to the user and keeps anything else internal.
+func validationFailed(err error) error {
+	var vErr *domain.ValidationError
+	if errors.As(err, &vErr) {
+		return status.Errorf(status.PreconditionFailed, "%s", vErr.Message)
+	}
+	return fmt.Errorf("validate domain: %w", err)
 }
 
 // GetClusterDomains returns a list of proxy cluster domains.

management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go

@@ -229,7 +229,7 @@ func TestValidateDomain_UnlocksClusterDerivation(t *testing.T) {
 	require.Error(t, err)
 
 	env.resolver.set("validation.later.example.com", testCluster)
-	env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID)
+	require.NoError(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID))
 
 	require.True(t, storedDomain(t, env.store, accountA, "later.example.com").Validated)
 
@@ -286,7 +286,7 @@ func TestValidateDomain_PermissionDeniedDoesNotValidate(t *testing.T) {
 	// a validated domain is the permission check.
 	env.resolver.set("validation.guarded.example.com", testCluster)
 
-	env.manager.ValidateDomain(ctx, accountA, accountAMember, created.ID)
+	assert.Error(t, env.manager.ValidateDomain(ctx, accountA, accountAMember, created.ID), "a caller without permission must be refused")
 
 	stored := storedDomain(t, env.store, accountA, "guarded.example.com")
 	require.NotNil(t, stored)

management/internals/modules/reverseproxy/domain/manager/normalization_test.go

@@ -52,7 +52,7 @@ func TestCreateDomain_NormalizedNameCanValidateLater(t *testing.T) {
 	require.False(t, created.Validated, "a missing CNAME must leave the normalized registration pending")
 
 	env.resolver.set("validation.apps.example.com", testCluster)
-	env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID)
+	require.NoError(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID))
 	stored, err := env.store.GetCustomDomain(ctx, accountA, created.ID)
 	require.NoError(t, err)
 	assert.Equal(t, "apps.example.com", stored.Domain, "retrying validation must retain the normalized name")

management/internals/modules/reverseproxy/domain/manager/validation_test.go

@@ -0,0 +1,151 @@
+package manager
+
+import (
+	"context"
+	"testing"
+	"testing/synctest"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
+	nbstore "github.com/netbirdio/netbird/management/server/store"
+	"github.com/netbirdio/netbird/shared/management/status"
+)
+
+// The validator tests cover each DNS outcome. These cover what the manager adds on top:
+// the status type the API turns into a response code, and the checks before the lookup.
+func TestValidateDomain_ReturnsReason(t *testing.T) {
+	ctx := context.Background()
+	tests := []struct {
+		name    string
+		setup   func(t *testing.T, env *domainTestEnv, d *domain.Domain)
+		user    string
+		want    status.Type
+		message string
+	}{
+		{
+			name: "cname mismatch",
+			setup: func(_ *testing.T, env *domainTestEnv, d *domain.Domain) {
+				env.resolver.set("validation."+d.Domain, "other.example.net")
+			},
+			want:    status.PreconditionFailed,
+			message: "CNAME record validation.test.example.com points to other.example.net; point it to " + testCluster,
+		},
+		{
+			name: "expired window",
+			setup: func(t *testing.T, env *domainTestEnv, d *domain.Domain) {
+				setDomainColumn(t, env, d, "validation_expires_at", time.Now().Add(-time.Second))
+			},
+			want:    status.PreconditionFailed,
+			message: "custom domain test.example.com validation window has expired; delete and add the domain again",
+		},
+		{
+			name: "no target cluster",
+			setup: func(t *testing.T, env *domainTestEnv, d *domain.Domain) {
+				setDomainColumn(t, env, d, "target_cluster", "")
+			},
+			want:    status.PreconditionFailed,
+			message: "custom domain test.example.com has no target cluster",
+		},
+		{
+			name: "permission denied",
+			setup: func(_ *testing.T, env *domainTestEnv, d *domain.Domain) {
+				env.resolver.set("validation."+d.Domain, testCluster)
+			},
+			user: accountAMember,
+			want: status.PermissionDenied,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			env := setupDomainTest(t)
+			d, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "test.example.com", testCluster)
+			require.NoError(t, err)
+			tt.setup(t, env, d)
+			user := accountAUser
+			if tt.user != "" {
+				user = tt.user
+			}
+
+			err = env.manager.ValidateDomain(ctx, accountA, user, d.ID)
+			sErr, ok := status.FromError(err)
+			require.True(t, ok, "expected a status error, got %v", err)
+			assert.Equal(t, tt.want, sErr.Type(), "status type")
+			if tt.message != "" {
+				assert.Equal(t, tt.message, sErr.Message, "user-facing message")
+			}
+			assert.False(t, storedDomain(t, env.store, accountA, d.Domain).Validated, "a failed check must not validate the domain")
+		})
+	}
+}
+
+func TestValidateDomain_SuccessIsIdempotent(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+	d, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "ok.example.com", testCluster)
+	require.NoError(t, err)
+	env.resolver.set("validation.ok.example.com", testCluster)
+
+	require.NoError(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID))
+	assert.True(t, storedDomain(t, env.store, accountA, d.Domain).Validated, "domain should be validated")
+	assert.NoError(t, env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID), "an already validated domain is a success")
+}
+
+func setDomainColumn(t *testing.T, env *domainTestEnv, d *domain.Domain, column string, value any) {
+	t.Helper()
+	db := env.store.(*nbstore.SqlStore).GetDB()
+	require.NoError(t, db.Model(&domain.Domain{}).Where("id = ?", d.ID).Update(column, value).Error)
+}
+
+// Two verify requests can both read the domain as pending. The one that loses the store
+// update must still report success when the other one validated the domain.
+func TestValidateDomain_ConcurrentValidationSucceeds(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+	d, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "twice.example.com", testCluster)
+	require.NoError(t, err)
+	resolver := blockingDomainResolver{started: make(chan struct{}), release: make(chan struct{})}
+	env.manager.validator.Resolver = resolver
+
+	done := make(chan error, 1)
+	go func() {
+		done <- env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
+	}()
+	<-resolver.started
+	// The other request completes while this one is still looking up the CNAME.
+	setDomainColumn(t, env, d, "validated", true)
+	close(resolver.release)
+
+	assert.NoError(t, <-done, "a domain validated by a concurrent request is a success")
+}
+
+// hangingResolver never answers, so only a deadline on the context ends the lookup.
+type hangingResolver struct{}
+
+func (hangingResolver) LookupCNAME(ctx context.Context, _ string) (string, error) {
+	<-ctx.Done()
+	return "", ctx.Err()
+}
+
+// A lookup that hangs must end on the validation timeout and still report the reason,
+// rather than leaving the request to whatever timeout sits in front of management.
+func TestValidateDomain_LookupTimeout(t *testing.T) {
+	env := setupClockDomainTest(t)
+	synctest.Test(t, func(t *testing.T) {
+		ctx := context.Background()
+		d, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "slow.example.com", testCluster)
+		require.NoError(t, err)
+		env.manager.validator.Resolver = hangingResolver{}
+
+		start := time.Now()
+		err = env.manager.ValidateDomain(ctx, accountA, accountAUser, d.ID)
+		sErr, ok := status.FromError(err)
+		require.True(t, ok, "expected a status error, got %v", err)
+		assert.Equal(t, status.PreconditionFailed, sErr.Type(), "status type")
+		assert.Equal(t, "DNS lookup for validation.slow.example.com failed; retry the validation", sErr.Message, "user-facing message")
+		assert.Equal(t, validationTimeout, time.Since(start), "the lookup must end on the validation timeout")
+	})
+}