sql: fix race/deadlock condition on audit logging CCL hook

Release note (bug fix): This change fixes a race/deadlock condition that can occur when multiple SQLServers are created simulatenously, causing simultaneous write to an unprotected global variable used to configure a CCL audit logging feature. https://cockroachlabs.atlassian.net/browse/DOC-1355 #Informs: cockroachdb#33316 #Epic: CRDB-8035 (sql change): #Release note (ui change): #Release note (security update): #Release note (general change): (performance improvement): #Release note (bug fix):
THardy98 · Jun 20, 2023 · cfa79e7 · cfa79e7
1 parent 9633594
commit cfa79e7
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 17 deletions.
diff --git a/pkg/ccl/auditloggingccl/BUILD.bazel b/pkg/ccl/auditloggingccl/BUILD.bazel
@@ -6,7 +6,6 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/auditloggingccl",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/ccl/utilccl",
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql/auditlogging",

diff --git a/pkg/ccl/auditloggingccl/audit_log_config.go b/pkg/ccl/auditloggingccl/audit_log_config.go
@@ -11,7 +11,6 @@ package auditloggingccl
 import (
 	"context"
 
-	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/auditlogging"
@@ -54,11 +53,7 @@ func validateAuditLogConfig(_ *settings.Values, input string) error {
 		// Empty config
 		return nil
 	}
-	st, clusterID, err := auditlogging.UserAuditEnterpriseParamsHook()
-	if err != nil {
-		return err
-	}
-	enterpriseCheckErr := utilccl.CheckEnterpriseEnabled(st, clusterID, "role-based audit logging")
+	_, enterpriseCheckErr := auditlogging.ReadEnterpriseParamsHook()
 	if enterpriseCheckErr != nil {
 		return pgerror.Wrap(enterpriseCheckErr,
 			pgcode.InsufficientPrivilege, "role-based audit logging requires enterprise license")

diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go
@@ -1380,11 +1380,10 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 	vmoduleSetting.SetOnChange(&cfg.Settings.SV, fn)
 	fn(ctx)
 
-	auditlogging.UserAuditEnterpriseParamsHook = func(st *cluster.Settings, clusterID uuid.UUID) func() (*cluster.Settings, uuid.UUID, error) {
-		return func() (*cluster.Settings, uuid.UUID, error) {
-			return st, clusterID, nil
-		}
-	}(execCfg.Settings, cfg.ClusterIDContainer.Get())
+	initialized, _ := auditlogging.ReadEnterpriseParamsHook()
+	if !initialized {
+		auditlogging.WriteEnterpriseParamsHook(ctx, base.CheckEnterpriseEnabled(execCfg.Settings, cfg.ClusterIDContainer.Get(), "role-based auditing"))
+	}
 
 	auditlogging.ConfigureRoleBasedAuditClusterSettings(ctx, execCfg.SessionInitCache.AuditConfig, execCfg.Settings, &execCfg.Settings.SV)
 

diff --git a/pkg/sql/auditlogging/BUILD.bazel b/pkg/sql/auditlogging/BUILD.bazel
@@ -17,10 +17,10 @@ go_library(
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/tree",
+        "//pkg/util/log",
         "//pkg/util/log/eventpb",
         "//pkg/util/log/logpb",
         "//pkg/util/syncutil",
-        "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_olekukonko_tablewriter//:tablewriter",
     ],

diff --git a/pkg/sql/auditlogging/audit_log.go b/pkg/sql/auditlogging/audit_log.go
@@ -20,11 +20,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
-	"github.com/cockroachdb/cockroach/pkg/util/uuid"
-	"github.com/cockroachdb/errors"
 	"github.com/olekukonko/tablewriter"
 )
 
@@ -43,8 +42,26 @@ var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
 	return false
 }
 
-var UserAuditEnterpriseParamsHook = func() (*cluster.Settings, uuid.UUID, error) {
-	return nil, uuid.Nil, errors.New("Cannot validate log config, enterprise parameters not initialized yet")
+var UserAuditEnterpriseParamsHook = struct {
+	syncutil.RWMutex
+	Initialized   bool
+	EnterpriseErr error
+}{}
+
+var ReadEnterpriseParamsHook = func() (initialized bool, enterpriseErr error) {
+	UserAuditEnterpriseParamsHook.RLock()
+	defer UserAuditEnterpriseParamsHook.RUnlock()
+	return UserAuditEnterpriseParamsHook.Initialized, UserAuditEnterpriseParamsHook.EnterpriseErr
+}
+
+var WriteEnterpriseParamsHook = func(ctx context.Context, enterpriseErr error) {
+	UserAuditEnterpriseParamsHook.Lock()
+	defer UserAuditEnterpriseParamsHook.Unlock()
+	UserAuditEnterpriseParamsHook.Initialized = true
+	UserAuditEnterpriseParamsHook.EnterpriseErr = enterpriseErr
+	if enterpriseErr != nil {
+		log.Errorf(ctx, "role-based auditing disabled, enterprise check failed: %v", enterpriseErr)
+	}
 }
 
 // Auditor is an interface used to check and build different audit events.