diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md index 95d39c4cb5f7..ad837abf98bf 100644 --- a/docs/generated/http/full.md +++ b/docs/generated/http/full.md @@ -6402,7 +6402,6 @@ SettingsRequest inquires what are the current settings in the cluster. | Field | Type | Label | Description | Support status | | ----- | ---- | ----- | ----------- | -------------- | | keys | [string](#cockroach.server.serverpb.SettingsRequest-string) | repeated | The array of setting keys or names to retrieve. An empty keys array means "all". | [reserved](#support-status) | -| unredacted_values | [bool](#cockroach.server.serverpb.SettingsRequest-bool) | | Indicate whether to see unredacted setting values. This is opt-in so that a previous version `cockroach zip` does not start reporting values when this becomes active. For good security, the server only obeys this after it checks that the logger-in user has admin privilege. | [reserved](#support-status) | diff --git a/pkg/cli/rpc_node_shutdown.go b/pkg/cli/rpc_node_shutdown.go index cce1d459a4d6..8f11ee2239a1 100644 --- a/pkg/cli/rpc_node_shutdown.go +++ b/pkg/cli/rpc_node_shutdown.go @@ -69,7 +69,6 @@ func doDrain( string(server.QueryShutdownTimeout.InternalKey()), string(kvserver.LeaseTransferPerIterationTimeout.InternalKey()), }, - UnredactedValues: true, }) if err != nil { return err diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index d52149bddf58..7f8bb505525e 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -242,6 +242,7 @@ go_library( "//pkg/sql/parser", "//pkg/sql/parser/statements", "//pkg/sql/pgwire", + "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "//pkg/sql/pgwire/pgwirecancel", "//pkg/sql/physicalplan", diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 7d1dbe14dd6d..4bef00350dfd 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -12,7 +12,6 @@ import ( "fmt" "io" "net/http" - "slices" "sort" "strconv" "strings" @@ -50,6 +49,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/parser" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/ts/catalog" @@ -1934,74 +1936,15 @@ func (s *adminServer) GetUIData( func (s *adminServer) Settings( ctx context.Context, req *serverpb.SettingsRequest, ) (*serverpb.SettingsResponse, error) { - ctx = s.AnnotateCtx(ctx) - - _, isAdmin, err := s.privilegeChecker.GetUserAndRole(ctx) + userName, err := authserver.UserFromIncomingRPCContext(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) - // and `cockroach zip` to redact the values (telemetry). - if req.UnredactedValues { - redactValues = false - } - } else { - // Non-root access cannot see the values. - // Exception: users with VIEWACTIVITY and VIEWACTIVITYREDACTED can see cluster - // settings used by the UI Console. - if err := s.privilegeChecker.RequireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil { - if err2 := s.privilegeChecker.RequireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil { - // The check for VIEWACTIVITY or VIEWATIVITYREDACTED is a special case so cluster settings from - // the console can be returned, but if the user doesn't have them (i.e. err2 != nil), we don't want - // to share this error message, so only return `err`. - return nil, err - } - consoleSettingsOnly = true - } - } - - showSystem := s.sqlServer.execCfg.Codec.ForSystemTenant() - target := settings.ForVirtualCluster - if showSystem { - target = settings.ForSystemTenant - } - - // settingsKeys is the list of setting keys to retrieve. - settingsKeys := make([]settings.InternalKey, 0, len(req.Keys)) - for _, desiredSetting := range req.Keys { - // The API client can pass either names or internal keys through the API. - key, ok, _ := settings.NameToKey(settings.SettingName(desiredSetting)) - if ok { - settingsKeys = append(settingsKeys, key) - } else { - settingsKeys = append(settingsKeys, settings.InternalKey(desiredSetting)) - } - } - if !consoleSettingsOnly { - if len(settingsKeys) == 0 { - settingsKeys = settings.Keys(target) - } - } else { - if len(settingsKeys) == 0 { - settingsKeys = settings.ConsoleKeys() - } else { - newSettingsKeys := make([]settings.InternalKey, 0, len(settings.ConsoleKeys())) - for _, k := range settingsKeys { - if slices.Contains(settings.ConsoleKeys(), k) { - newSettingsKeys = append(newSettingsKeys, k) - } - } - settingsKeys = newSettingsKeys - } + keyFilter := make(map[string]bool) + for _, key := range req.Keys { + keyFilter[key] = true } - // Read the system.settings table to determine the settings for which we have // explicitly set values -- the in-memory SV has the set and default values // flattened for quick reads, but we'd only need the non-defaults for comparison. @@ -2027,33 +1970,80 @@ func (s *adminServer) Settings( } } + // Get cluster settings + it, err := s.internalExecutor.QueryIteratorEx( + ctx, "get-cluster-settings", nil, /* txn */ + sessiondata.InternalExecutorOverride{User: userName}, + "SELECT variable, value, type, description, public from crdb_internal.cluster_settings", + ) + + if err != nil { + return nil, srverrors.ServerError(ctx, err) + } + + scanner := makeResultScanner(it.Types()) resp := serverpb.SettingsResponse{KeyValues: make(map[string]serverpb.SettingsResponse_Value)} - for _, k := range settingsKeys { - var v settings.Setting - var ok bool - if redactValues { - v, ok = settings.LookupForReportingByKey(k, target) - } else { - v, ok = settings.LookupForLocalAccessByKey(k, target) - } - if !ok { - continue + respSettings := make(map[string]serverpb.SettingsResponse_Value) + var ok bool + for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) { + row := it.Cur() + var responseValue serverpb.SettingsResponse_Value + if scanErr := scanner.ScanAll( + row, + &responseValue.Name, + &responseValue.Value, + &responseValue.Type, + &responseValue.Description, + &responseValue.Public); scanErr != nil { + return nil, srverrors.ServerError(ctx, scanErr) + } + internalKey, found, _ := settings.NameToKey(settings.SettingName(responseValue.Name)) + + if found && (len(keyFilter) == 0 || keyFilter[string(internalKey)]) { + if lastUpdated, found := alteredSettings[internalKey]; found { + responseValue.LastUpdated = lastUpdated + } + respSettings[string(internalKey)] = responseValue } + } - var altered *time.Time - if val, ok := alteredSettings[k]; ok { - altered = val + // Users without MODIFYCLUSTERSETTINGS or VIEWCLUSTERSETTINGS access cannot see the values. + // Exception: users with VIEWACTIVITY and VIEWACTIVITYREDACTED can see cluster + // settings used by the UI Console. + if err != nil { + if pgerror.GetPGCode(err) != pgcode.InsufficientPrivilege { + return nil, srverrors.ServerError(ctx, err) } - resp.KeyValues[string(k)] = serverpb.SettingsResponse_Value{ - Type: v.Typ(), - Name: string(v.Name()), - // Note: v.String() redacts the values if the purpose is not "LocalAccess". - Value: v.String(&s.st.SV), - Description: v.Description(), - Public: v.Visibility() == settings.Public, - LastUpdated: altered, + if err2 := s.privilegeChecker.RequireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil { + // The check for VIEWACTIVITY or VIEWATIVITYREDACTED is a special case so cluster settings from + // the console can be returned, but if the user doesn't have them (i.e. err2 != nil), we don't want + // to share this error message. + return nil, grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s or %s system privileges", + privilege.VIEWCLUSTERSETTING.DisplayName(), privilege.MODIFYCLUSTERSETTING.DisplayName()) + } + consoleKeys := settings.ConsoleKeys() + for _, k := range consoleKeys { + if consoleSetting, ok := settings.LookupForLocalAccessByKey(k, s.sqlServer.execCfg.Codec.ForSystemTenant()); ok { + if internalKey, found, _ := settings.NameToKey(consoleSetting.Name()); found && + (len(keyFilter) == 0 || keyFilter[string(internalKey)]) { + var responseValue serverpb.SettingsResponse_Value + responseValue.Name = string(consoleSetting.Name()) + responseValue.Value = consoleSetting.String(&s.st.SV) + responseValue.Type = consoleSetting.Typ() + responseValue.Description = consoleSetting.Description() + responseValue.Public = consoleSetting.Visibility() == settings.Public + if lastUpdated, found := alteredSettings[internalKey]; found { + responseValue.LastUpdated = lastUpdated + } + respSettings[string(internalKey)] = responseValue + } + } } + } + + resp.KeyValues = respSettings return &resp, nil } diff --git a/pkg/server/application_api/BUILD.bazel b/pkg/server/application_api/BUILD.bazel index f38ce22758d7..91b2a04ae203 100644 --- a/pkg/server/application_api/BUILD.bazel +++ b/pkg/server/application_api/BUILD.bazel @@ -54,6 +54,7 @@ go_test( "//pkg/security/username", "//pkg/server", "//pkg/server/apiconstants", + "//pkg/server/authserver", "//pkg/server/diagnostics", "//pkg/server/diagnostics/diagnosticspb", "//pkg/server/license", @@ -99,6 +100,8 @@ go_test( "@com_github_prometheus_client_model//go", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//status", "@org_golang_x_sync//errgroup", ], ) diff --git a/pkg/server/application_api/config_test.go b/pkg/server/application_api/config_test.go index 84db02750918..c2fc4b040a1a 100644 --- a/pkg/server/application_api/config_test.go +++ b/pkg/server/application_api/config_test.go @@ -8,17 +8,15 @@ package application_api_test import ( "context" "fmt" - "net/url" "reflect" - "slices" - "sort" "testing" + "time" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl" // To ensure the streaming replication cluster setting is defined. _ "github.com/cockroachdb/cockroach/pkg/crosscluster" - "github.com/cockroachdb/cockroach/pkg/server/apiconstants" + "github.com/cockroachdb/cockroach/pkg/server/authserver" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/srvtestutils" "github.com/cockroachdb/cockroach/pkg/settings" @@ -27,227 +25,138 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/safesql" "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func TestAdminAPISettings(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - - srv, conn, _ := serverutils.StartServer(t, base.TestServerArgs{}) - defer srv.Stopper().Stop(context.Background()) - s := srv.ApplicationLayer() - - // Any bool that defaults to true will work here. - const settingKey = "sql.metrics.statement_details.enabled" - st := s.ClusterSettings() - target := settings.ForSystemTenant - if srv.TenantController().StartedDefaultTestTenant() { - target = settings.ForVirtualCluster - } - allKeys := settings.Keys(target) - - checkSetting := func(t *testing.T, k settings.InternalKey, v serverpb.SettingsResponse_Value) { - ref, ok := settings.LookupForReportingByKey(k, target) - if !ok { - t.Fatalf("%s: not found after initial lookup", k) - } - typ := ref.Typ() - - if !settings.TestingIsReportable(ref) { - if v.Value != "" && v.Value != "" { - t.Errorf("%s: expected redacted value for %v, got %s", k, ref, v.Value) - } - } else { - if ref.String(&st.SV) != v.Value { - t.Errorf("%s: expected value %v, got %s", k, ref, v.Value) - } - } - - if expectedPublic := ref.Visibility() == settings.Public; expectedPublic != v.Public { - t.Errorf("%s: expected public %v, got %v", k, expectedPublic, v.Public) - } - - if desc := ref.Description(); desc != v.Description { - t.Errorf("%s: expected description %s, got %s", k, desc, v.Description) - } - if typ != v.Type { - t.Errorf("%s: expected type %s, got %s", k, typ, v.Type) - } - if v.LastUpdated != nil { - db := sqlutils.MakeSQLRunner(conn) - q := safesql.NewQuery() - q.Append(`SELECT name, "lastUpdated" FROM system.settings WHERE name=$`, k) - rows := db.Query( - t, - q.String(), - q.QueryArguments()..., - ) - defer rows.Close() - if rows.Next() == false { - t.Errorf("missing sql row for %s", k) - } - } + ctx := context.Background() + + // Test setup + settingName := "some.sensitive.setting" + settings.RegisterStringSetting( + settings.SystemVisible, + settings.InternalKey(settingName), + "sensitiveSetting", + "Im sensitive!", + settings.WithPublic, + settings.WithReportable(false), + settings.Sensitive, + ) + testUsers := map[string]struct { + userName string + consoleOnly bool + redactableSettings bool + grantRole string + }{ + // Admin users should be able to se all cluster settings without redaction. + "admin_user": {userName: "admin_user", redactableSettings: false, grantRole: "ADMIN"}, + // Users with MODIFYCLUSTERSETTING should be able to see all cluster + // settings without redaction + "modify_settings_user": {userName: "modify_settings_user", redactableSettings: false, grantRole: "SYSTEM MODIFYCLUSTERSETTING"}, + // Users with VIEWCLUSTERSETTING should be able to see all cluster + // settings, but sensitive settings are redacted + "view_settings_user": {userName: "view_settings_user", redactableSettings: true, grantRole: "SYSTEM VIEWCLUSTERSETTING"}, + // Users with VIEWACTIVITY and VIEWACTIVITYREDACTED should only be able to + // see console specific settings. + "view_activity_user": {userName: "view_activity_user", redactableSettings: true, grantRole: "SYSTEM VIEWACTIVITY", consoleOnly: true}, + "view_activity_redacted_user": {userName: "view_activity_redacted_user", redactableSettings: true, grantRole: "SYSTEM VIEWACTIVITYREDACTED", consoleOnly: true}, } - - t.Run("all", func(t *testing.T) { - var resp serverpb.SettingsResponse - - if err := srvtestutils.GetAdminJSONProto(s, "settings", &resp); err != nil { + ts := serverutils.StartServerOnly(t, base.TestServerArgs{}) + defer ts.Stopper().Stop(ctx) + forSystemTenant := ts.ApplicationLayer().Codec().ForSystemTenant() + conn := sqlutils.MakeSQLRunner(ts.ApplicationLayer().SQLConn(t)) + settingsLastUpdated := make(map[string]*time.Time) + rows := conn.Query(t, `SELECT name, "lastUpdated" FROM system.settings`) + defer rows.Close() + for rows.Next() { + var name string + var lastUpdated *time.Time + + if err := rows.Scan(&name, &lastUpdated); err != nil { t.Fatal(err) } + settingsLastUpdated[name] = lastUpdated + } - // Check that all expected keys were returned. - if len(allKeys) != len(resp.KeyValues) { - t.Fatalf("expected %d keys, got %d", len(allKeys), len(resp.KeyValues)) - } - for _, k := range allKeys { - if _, ok := resp.KeyValues[string(k)]; !ok { - t.Fatalf("expected key %s not found in response", k) - } - } + for _, u := range testUsers { + conn.Exec(t, fmt.Sprintf("CREATE USER IF NOT EXISTS %s", u.userName)) + conn.Exec(t, fmt.Sprintf("GRANT %s TO %s", u.grantRole, u.userName)) + } - // Check that the test key is listed and the values come indeed - // from the settings package unchanged. - seenRef := false - for k, v := range resp.KeyValues { - if k == settingKey { - seenRef = true - if v.Value != "true" { - t.Errorf("%s: expected true, got %s", k, v.Value) + // Runs test cases on each user in testUsers twice, once with redact_sensitive_settings + // enabled and once with it disabled. + testutils.RunTrueAndFalse(t, "redact sensitive", func(t *testing.T, redactSensitive bool) { + if redactSensitive { + conn.Exec(t, "SET CLUSTER SETTING server.redact_sensitive_settings.enabled=t") + } else { + conn.Exec(t, "SET CLUSTER SETTING server.redact_sensitive_settings.enabled=f") + } + for _, u := range testUsers { + t.Run(u.userName, func(t *testing.T) { + authCtx := authserver.ForwardHTTPAuthInfoToRPCCalls(authserver.ContextWithHTTPAuthInfo(ctx, u.userName, 1), nil) + resp, err := ts.GetAdminClient(t).Settings(authCtx, &serverpb.SettingsRequest{}) + require.NoError(t, err) + var keys []settings.InternalKey + // users with consoleOnly flag are expected to only be able to see + // console specific settings + if u.consoleOnly { + keys = settings.ConsoleKeys() + } else { + keys = settings.Keys(forSystemTenant) + require.Contains(t, resp.KeyValues, settingName) } - } - - checkSetting(t, settings.InternalKey(k), v) - } - - if !seenRef { - t.Fatalf("failed to observe test setting %s, got %+v", settingKey, resp.KeyValues) - } - }) - - t.Run("one-by-one", func(t *testing.T) { - var resp serverpb.SettingsResponse - - // All the settings keys must be retrievable, and their - // type and description must match. - for _, k := range allKeys { - q := make(url.Values) - q.Add("keys", string(k)) - url := "settings?" + q.Encode() - if err := srvtestutils.GetAdminJSONProto(s, url, &resp); err != nil { - t.Fatalf("%s: %v", k, err) - } - if len(resp.KeyValues) != 1 { - t.Fatalf("%s: expected 1 response, got %d", k, len(resp.KeyValues)) - } - v, ok := resp.KeyValues[string(k)] - if !ok { - t.Fatalf("%s: response does not contain key", k) - } - - checkSetting(t, k, v) - } - }) - - t.Run("different-permissions", func(t *testing.T) { - var resp serverpb.SettingsResponse - nonAdminUser := apiconstants.TestingUserNameNoAdmin().Normalized() - var consoleKeys []settings.InternalKey - for _, k := range settings.ConsoleKeys() { - if _, ok := settings.LookupForLocalAccessByKey(k, target); !ok { - continue - } - consoleKeys = append(consoleKeys, k) - } - sort.Slice(consoleKeys, func(i, j int) bool { return consoleKeys[i] < consoleKeys[j] }) - - // 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) + for _, internalKey := range keys { + keyAsString := string(internalKey) + setting, ok := settings.LookupForLocalAccessByKey(internalKey, forSystemTenant) + require.True(t, ok) + settingResponse := resp.KeyValues[keyAsString] + settingVal := settingResponse.Value + // If the setting is "sensitive", the setting is not empty, the + // redact_sensitive_settings is true, and the user being tested + // is not allowed to see sensitive settings, the value should + // be redacted + if settings.TestingIsSensitive(setting) && settingVal != "" && redactSensitive && u.redactableSettings { + require.Equalf(t, "", settingVal, "Expected %s to be , but got %s", keyAsString, settingVal) + } else { + require.NotEqualf(t, "", settingVal, "Expected %s to be %s, but got ", keyAsString, settingVal) + } + require.Equal(t, setting.Description(), settingResponse.Description) + require.Equal(t, setting.Typ(), settingResponse.Type) + require.Equal(t, string(setting.Name()), settingResponse.Name) + if lu, ok := settingsLastUpdated[keyAsString]; ok { + require.Equal(t, lu.UTC(), *settingResponse.LastUpdated) + } + } + }) } - 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) - } + t.Run("no permission", func(t *testing.T) { + userName := "no_permission" + conn.Exec(t, fmt.Sprintf("CREATE USER IF NOT EXISTS %s", userName)) + authCtx := authserver.ForwardHTTPAuthInfoToRPCCalls(authserver.ContextWithHTTPAuthInfo(ctx, userName, 1), nil) + _, err := ts.GetAdminClient(t).Settings(authCtx, &serverpb.SettingsRequest{}) + require.Error(t, err) + grpcStatus, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.PermissionDenied, grpcStatus.Code()) - gotKeys := make([]string, 0, len(resp.KeyValues)) - for k := range resp.KeyValues { - gotKeys = append(gotKeys, k) - } - sort.Strings(gotKeys) - require.Equal(t, len(consoleKeys), len(resp.KeyValues), "found:\n%+v\nexpected:\n%+v", gotKeys, consoleKeys) - for k := range resp.KeyValues { - require.True(t, slices.Contains(consoleKeys, settings.InternalKey(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) + t.Run("filter keys", func(t *testing.T) { + resp, err := ts.GetAdminClient(t).Settings(ctx, &serverpb.SettingsRequest{Keys: []string{settingName}}) + require.NoError(t, err) + require.Contains(t, resp.KeyValues, settingName) + require.Len(t, 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) + resp, err = ts.GetAdminClient(t).Settings(ctx, &serverpb.SettingsRequest{Keys: []string{"random.key"}}) + require.NoError(t, err) + require.Empty(t, resp.KeyValues) + }) }) } diff --git a/pkg/server/serverpb/admin.proto b/pkg/server/serverpb/admin.proto index a9e068884e0e..06112467541c 100644 --- a/pkg/server/serverpb/admin.proto +++ b/pkg/server/serverpb/admin.proto @@ -596,13 +596,7 @@ message SettingsRequest { // The array of setting keys or names to retrieve. // An empty keys array means "all". repeated string keys = 1; - - // Indicate whether to see unredacted setting values. - // This is opt-in so that a previous version `cockroach zip` - // does not start reporting values when this becomes active. - // For good security, the server only obeys this after it checks - // that the logger-in user has admin privilege. - bool unredacted_values = 2; + reserved 2; } // SettingsResponse is the response to SettingsRequest. diff --git a/pkg/settings/common.go b/pkg/settings/common.go index f2126bf01834..52343a687508 100644 --- a/pkg/settings/common.go +++ b/pkg/settings/common.go @@ -172,3 +172,22 @@ type internalSetting interface { // are not run against the decoded value. decodeAndSetDefaultOverride(ctx context.Context, sv *Values, encoded string) error } + +// TestingIsReportable is used in testing for reportability. +func TestingIsReportable(s Setting) bool { + if _, ok := s.(*MaskedSetting); ok { + return false + } + if e, ok := s.(internalSetting); ok { + return e.isReportable() + } + return true +} + +// TestingIsSensitive is used in testing for sensitivity. +func TestingIsSensitive(s Setting) bool { + if e, ok := s.(internalSetting); ok { + return e.isSensitive() + } + return false +} diff --git a/pkg/settings/masked.go b/pkg/settings/masked.go index d9f9e5bb7e05..8776cdb089f5 100644 --- a/pkg/settings/masked.go +++ b/pkg/settings/masked.go @@ -90,14 +90,3 @@ func (s *MaskedSetting) ValueOrigin(ctx context.Context, sv *Values) ValueOrigin func (s *MaskedSetting) IsUnsafe() bool { return s.setting.IsUnsafe() } - -// TestingIsReportable is used in testing for reportability. -func TestingIsReportable(s Setting) bool { - if _, ok := s.(*MaskedSetting); ok { - return false - } - if e, ok := s.(internalSetting); ok { - return e.isReportable() - } - return true -}