diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 7ffb9cb9e678..788f9969ca48 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -367,6 +367,7 @@ go_library( "@org_golang_google_grpc//codes", "@org_golang_google_grpc//metadata", "@org_golang_google_grpc//status", + "@org_golang_x_exp//slices", ] + select({ "@io_bazel_rules_go//go/platform:aix": [ "@org_golang_x_sys//unix", diff --git a/pkg/server/admin.go b/pkg/server/admin.go index df7dd7ccf848..f3bed3c5c822 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -77,6 +77,7 @@ import ( "github.com/cockroachdb/errors" gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" gwutil "github.com/grpc-ecosystem/grpc-gateway/utilities" + "golang.org/x/exp/slices" "google.golang.org/grpc" "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" @@ -1986,17 +1987,15 @@ func (s *adminServer) Settings( ) (*serverpb.SettingsResponse, error) { ctx = s.AnnotateCtx(ctx) - keys := req.Keys - if len(keys) == 0 { - keys = settings.Keys(settings.ForSystemTenant) - } - _, isAdmin, err := s.privilegeChecker.GetUserAndRole(ctx) if err != nil { return nil, srverrors.ServerError(ctx, err) } redactValues := true + // Only returns non-sensitive settings that are required + // for features on DB Console. + consoleSettingsOnly := false if isAdmin { // Root accesses can customize the purpose. // This is used by the UI to see all values (local access) @@ -2007,7 +2006,32 @@ func (s *adminServer) Settings( } else { // Non-root access cannot see the values in any case. if err := s.privilegeChecker.RequireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil { - return nil, err + if err2 := s.privilegeChecker.RequireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil { + return nil, err + } + consoleSettingsOnly = true + log.Warningf(ctx, "only cluster settings used by DB Console are returned with "+ + "VIEWACTIVITY or VIEWACTIVITYREDACTED") + } + } + + var settingsKeys []string + if !consoleSettingsOnly { + settingsKeys = req.Keys + if len(settingsKeys) == 0 { + settingsKeys = settings.Keys(settings.ForSystemTenant) + } + } else { + + if len(req.Keys) == 0 { + settingsKeys = settings.ConsoleKeys() + } else { + settingsKeys = []string{} + for _, k := range req.Keys { + if slices.Contains(settings.ConsoleKeys(), k) { + settingsKeys = append(settingsKeys, k) + } + } } } @@ -2037,7 +2061,7 @@ func (s *adminServer) Settings( } resp := serverpb.SettingsResponse{KeyValues: make(map[string]serverpb.SettingsResponse_Value)} - for _, k := range keys { + for _, k := range settingsKeys { var v settings.Setting var ok bool if redactValues { diff --git a/pkg/server/application_api/BUILD.bazel b/pkg/server/application_api/BUILD.bazel index faf5f1ba6092..ffe71d038005 100644 --- a/pkg/server/application_api/BUILD.bazel +++ b/pkg/server/application_api/BUILD.bazel @@ -85,6 +85,7 @@ go_test( "@com_github_kr_pretty//:pretty", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_golang_x_exp//slices", "@org_golang_x_sync//errgroup", ], ) diff --git a/pkg/server/application_api/config_test.go b/pkg/server/application_api/config_test.go index e73131e965b2..f52d7b8915ec 100644 --- a/pkg/server/application_api/config_test.go +++ b/pkg/server/application_api/config_test.go @@ -12,11 +12,13 @@ package application_api_test import ( "context" + "fmt" "net/url" "reflect" "testing" "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/server/apiconstants" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/srvtestutils" "github.com/cockroachdb/cockroach/pkg/settings" @@ -29,6 +31,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/safesql" "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" ) func TestAdminAPISettings(t *testing.T) { @@ -149,6 +153,91 @@ func TestAdminAPISettings(t *testing.T) { checkSetting(t, k, v) } }) + + t.Run("different-permissions", func(t *testing.T) { + var resp serverpb.SettingsResponse + nonAdminUser := apiconstants.TestingUserNameNoAdmin().Normalized() + consoleKeys := settings.ConsoleKeys() + + // Admin should return all cluster settings + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, true); err != nil { + t.Fatal(err) + } + require.True(t, len(resp.KeyValues) == len(allKeys)) + + // Admin requesting specific cluster setting should return that cluster setting + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max", + &resp, true); err != nil { + t.Fatal(err) + } + require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"]) + require.True(t, len(resp.KeyValues) == 1) + + // Non-admin with no permission should return error message + err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false) + require.Error(t, err, "this operation requires the VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING system privileges") + + // Non-admin with VIEWCLUSTERSETTING permission should return all cluster settings + _, err = conn.Exec(fmt.Sprintf("ALTER USER %s VIEWCLUSTERSETTING", nonAdminUser)) + require.NoError(t, err) + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil { + t.Fatal(err) + } + require.True(t, len(resp.KeyValues) == len(allKeys)) + + // Non-admin with VIEWCLUSTERSETTING permission requesting specific cluster setting should return that cluster setting + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max", + &resp, false); err != nil { + t.Fatal(err) + } + require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"]) + require.True(t, len(resp.KeyValues) == 1) + + // Non-admin with VIEWCLUSTERSETTING and VIEWACTIVITY permission should return all cluster settings + _, err = conn.Exec(fmt.Sprintf("ALTER USER %s VIEWACTIVITY", nonAdminUser)) + require.NoError(t, err) + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil { + t.Fatal(err) + } + require.True(t, len(resp.KeyValues) == len(allKeys)) + + // Non-admin with VIEWCLUSTERSETTING and VIEWACTIVITY permission requesting specific cluster setting + // should return that cluster setting + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max", + &resp, false); err != nil { + t.Fatal(err) + } + require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"]) + require.True(t, len(resp.KeyValues) == 1) + + // Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING should only see console cluster settings + _, err = conn.Exec(fmt.Sprintf("ALTER USER %s NOVIEWCLUSTERSETTING", nonAdminUser)) + require.NoError(t, err) + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil { + t.Fatal(err) + } + require.True(t, len(resp.KeyValues) == len(consoleKeys)) + for k := range resp.KeyValues { + require.True(t, slices.Contains(consoleKeys, k)) + } + + // Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING permission requesting specific cluster setting + // from console should return that cluster setting + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=ui.display_timezone", + &resp, false); err != nil { + t.Fatal(err) + } + require.NotNil(t, resp.KeyValues["ui.display_timezone"]) + require.True(t, len(resp.KeyValues) == 1) + + // Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING permission requesting specific cluster setting + // that is not from console should not return that cluster setting + if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max", + &resp, false); err != nil { + t.Fatal(err) + } + require.True(t, len(resp.KeyValues) == 0) + }) } func TestClusterAPI(t *testing.T) { diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index 35cad8aa946c..9cba09ee8b5a 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -290,6 +290,29 @@ func Keys(forSystemTenant bool) (res []string) { return res } +// ConsoleKeys return an array with all cluster settings keys +// used by the UI Console. +// This list should only contain settings that have no sensitive +// information, because it will be returned on `settings` endpoint for +// users without VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING permission, +// but that have VIEWACTIVITY or VIEWACTIVITYREDACTED permissions. +func ConsoleKeys() (res []string) { + return []string{ + "cross_cluster_replication.enabled", + "keyvisualizer.enabled", + "keyvisualizer.sample_interval", + "sql.index_recommendation.drop_unused_duration", + "sql.insights.anomaly_detection.latency_threshold", + "sql.insights.high_retry_count.threshold", + "sql.insights.latency_threshold", + "sql.stats.automatic_collection.enabled", + "timeseries.storage.resolution_10s.ttl", + "timeseries.storage.resolution_30m.ttl", + "ui.display_timezone", + "version", + } +} + // LookupForLocalAccess returns a NonMaskedSetting by name. Used when a setting // is being retrieved for local processing within the cluster and not for // reporting; sensitive values are accessible.