From bf897af86e0bb1505c83ab46a68d8a3df28b9754 Mon Sep 17 00:00:00 2001 From: Kyle Wong <37189875+kyle-a-wong@users.noreply.github.com> Date: Wed, 8 Jan 2025 21:55:45 +0000 Subject: [PATCH] server: fix admin server Settings RPC redaction logic Previously admin.Settings only allowed admins to view all cluster settings without redaction. If the requester was not an admin, would use the isReportable field on settings to determine if the setting should be redacted or not. This API also had outdated logic, as users with the MODIFYCLUSTERSETTINGS should also be able to view all cluster settings (See #115356 for more discussions on this). This patch respects this new role, and no longer uses the `isReportable` setting flag to determine if a setting should be redacted. This is implemented by query `crdb_internal.cluster_settings` directly, allowing the sql layer to permission check. This commit also removes the `unredacted_values` from the request entity as well, since it is no longer necessary. Ultimately, this commit updates the Settings RPC to have the same redaction logic as querying `crdb_internal.cluster_settings` or using `SHOW CLUSTER SETTINGS`. Epic: None Fixes: #137698 Release note (ui change): The /_admin/v1/settings API (and therfore cluster settings console page) now returns cluster settings using the same redaction logic as querying `SHOW CLUSTER SETTINGS` and `crdb_internal.cluster_settings`. This means that only settings flagged as "sensitive" will be redacted, all other settings will be visible. The same authorization is required for this endpoint, meaning the user must be an admin or have MODIFYCLUSTERSETTINGS or VIEWCLUSTERSETTINGS roles to hit this API. The exception is that if the user has VIEWACTIVITY or VIEWACTIVITYREDACTED, they will see console only settings. --- docs/generated/http/full.md | 1 - pkg/cli/rpc_node_shutdown.go | 1 - pkg/server/BUILD.bazel | 1 + pkg/server/admin.go | 160 +++++------ pkg/server/application_api/BUILD.bazel | 3 + pkg/server/application_api/config_test.go | 323 ++++++++-------------- pkg/server/serverpb/admin.proto | 8 +- pkg/settings/common.go | 19 ++ pkg/settings/masked.go | 11 - 9 files changed, 215 insertions(+), 312 deletions(-) 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 -}