From 4473231e27323b78fd624992f4a59d37d216e97f Mon Sep 17 00:00:00 2001 From: Thomas Hardy Date: Tue, 20 Jun 2023 17:00:41 -0400 Subject: [PATCH] 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: https://github.com/cockroachdb/cockroach/issues/33316 #Epic: CRDB-8035 (sql change): #Release note (ui change): #Release note (security update): #Release note (general change): (performance improvement): #Release note (bug fix): --- pkg/ccl/auditloggingccl/BUILD.bazel | 1 - pkg/ccl/auditloggingccl/audit_log_config.go | 7 +----- pkg/ccl/auditloggingccl/audit_logging_test.go | 19 ++++++++------ pkg/server/server_sql.go | 9 +++---- pkg/sql/auditlogging/BUILD.bazel | 2 +- pkg/sql/auditlogging/audit_log.go | 25 ++++++++++++++++--- 6 files changed, 39 insertions(+), 24 deletions(-) diff --git a/pkg/ccl/auditloggingccl/BUILD.bazel b/pkg/ccl/auditloggingccl/BUILD.bazel index bec3acbb1481..ab6a5b7a44c5 100644 --- 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 index f6f28a3b296f..b218c090902e 100644 --- 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/ccl/auditloggingccl/audit_logging_test.go b/pkg/ccl/auditloggingccl/audit_logging_test.go index 1c34fbca642d..3ac7c458b311 100644 --- a/pkg/ccl/auditloggingccl/audit_logging_test.go +++ b/pkg/ccl/auditloggingccl/audit_logging_test.go @@ -32,22 +32,27 @@ import ( "github.com/stretchr/testify/require" ) -func TestRoleBasedAuditEnterpriseGated(t *testing.T) { +func TestRoleBasedAuditEnterpriseEnabled(t *testing.T) { defer leaktest.AfterTest(t)() - + testQuery := `SET CLUSTER SETTING sql.log.user_audit = 'ALL ALL'` s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) rootRunner := sqlutils.MakeSQLRunner(sqlDB) defer s.Stopper().Stop(context.Background()) + // Test that we can change the cluster setting when enterprise is enabled. + rootRunner.Exec(t, testQuery) +} +func TestRoleBasedAuditEnterpriseDisabled(t *testing.T) { + defer leaktest.AfterTest(t)() testQuery := `SET CLUSTER SETTING sql.log.user_audit = 'ALL ALL'` + // Disable enterprise. + utilccl.TestingDisableEnterprise() + s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) + rootRunner := sqlutils.MakeSQLRunner(sqlDB) + defer s.Stopper().Stop(context.Background()) // Test that we cannot change the cluster setting when enterprise is disabled. - enableEnterprise := utilccl.TestingDisableEnterprise() rootRunner.ExpectErr(t, "role-based audit logging requires enterprise license", testQuery) - // Enable enterprise. - enableEnterprise() - // Test that we can change the cluster setting when enterprise is enabled. - rootRunner.Exec(t, testQuery) } func TestSingleRoleAuditLogging(t *testing.T) { diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 196bb1482af2..693769dbd91b 100644 --- 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 index 63aba65e9349..8513536fbad4 100644 --- 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 index 74f6352d64fd..8719e32d78fd 100644 --- 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.