From 28748df18f302bdfe1dc6a0f54846eee2a1995d8 Mon Sep 17 00:00:00 2001 From: Kyle Wong <37189875+kyle-a-wong@users.noreply.github.com> Date: Wed, 8 Jan 2025 16:55:45 -0500 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 (general change): The /_admin/v1/settings API 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 | 159 +++++++++++++++----------------- pkg/server/admin_test.go | 118 ++++++++++++++++++++++++ pkg/server/serverpb/admin.proto | 8 +- 6 files changed, 195 insertions(+), 93 deletions(-) create mode 100644 pkg/server/admin_test.go diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md index d6c26b5ec253..5630828bdcb3 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 e2767abcaa9f..bdd678a3cee2 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -427,6 +427,7 @@ go_test( name = "server_test", size = "enormous", srcs = [ + "admin_test.go", "api_v2_databases_metadata_test.go", "api_v2_grants_test.go", "api_v2_ranges_test.go", diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 035c386a890d..ee0370dc8614 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -12,7 +12,6 @@ import ( "fmt" "io" "net/http" - "slices" "sort" "strconv" "strings" @@ -1933,74 +1932,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 + keyFilter := make(map[string]bool) + for _, key := range req.Keys { + keyFilter[key] = true } - - // 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 - } - } - // 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. @@ -2026,36 +1966,87 @@ 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) + 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 err := scanner.ScanAll( + row, + &responseValue.Name, + &responseValue.Value, + &responseValue.Type, + &responseValue.Description, + &responseValue.Public); err != nil { + return nil, srverrors.ServerError(ctx, err) } - if !ok { - continue + internalKey, found, _ := settings.NameToKey(settings.SettingName(responseValue.Name)) + + if found && filterKey(responseValue.Name, keyFilter) { + 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 len(respSettings) == 0 { + 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 + } } - 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, + 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 && + filterKey(string(consoleSetting.Name()), keyFilter) { + 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 } +func filterKey(key string, filter map[string]bool) bool { + if len(filter) == 0 { + return true + } + + _, ok := filter[key] + return ok +} + // Cluster returns cluster metadata. func (s *adminServer) Cluster( _ context.Context, req *serverpb.ClusterRequest, diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go new file mode 100644 index 000000000000..e791280db876 --- /dev/null +++ b/pkg/server/admin_test.go @@ -0,0 +1,118 @@ +// Copyright 2025 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package server + +import ( + "context" + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/server/authserver" + "github.com/cockroachdb/cockroach/pkg/server/serverpb" + "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +type user struct { + userName string + consoleOnly bool + redactableSettings bool + grantRole string +} + +func Test_adminServer_Settings(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + 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]user{ + "admin_user": {userName: "admin_user", redactableSettings: false, grantRole: "ADMIN"}, + "modify_settings_user": {userName: "modify_settings_user", redactableSettings: false, grantRole: "SYSTEM MODIFYCLUSTERSETTING"}, + "view_settings_user": {userName: "view_settings_user", redactableSettings: true, grantRole: "SYSTEM VIEWCLUSTERSETTING"}, + "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}, + } + ts := serverutils.StartServerOnly(t, base.TestServerArgs{}) + defer ts.Stopper().Stop(ctx) + conn := sqlutils.MakeSQLRunner(ts.ApplicationLayer().SQLConn(t)) + 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)) + } + + 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) + if u.consoleOnly { + consoleKeys := settings.ConsoleKeys() + for _, key := range consoleKeys { + require.Containsf(t, resp.KeyValues, string(key), "expected key %s", key) + } + } else { + require.Contains(t, resp.KeyValues, settingName) + if redactSensitive && u.redactableSettings { + require.Equal(t, "", resp.KeyValues[settingName].Value) + // Should be greater than one to account for new setting registered for this test + require.Greater(t, len(resp.KeyValues[settingName].Value), 1) + } else { + for _, val := range resp.KeyValues { + require.NotEqualf(t, "", val.Value, "Setting should not be redacted. %+v", val) + } + } + } + + }) + } + + 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()) + + }) + + t.Run("filter keys", func(t *testing.T) { + authCtx := authserver.ForwardHTTPAuthInfoToRPCCalls(authserver.ContextWithHTTPAuthInfo(ctx, testUsers["admin_user"].userName, 1), nil) + resp, err := ts.GetAdminClient(t).Settings(authCtx, &serverpb.SettingsRequest{Keys: []string{settingName}}) + require.NoError(t, err) + require.Contains(t, resp.KeyValues, settingName) + require.Len(t, resp.KeyValues, 1) + }) + }) +} diff --git a/pkg/server/serverpb/admin.proto b/pkg/server/serverpb/admin.proto index 2a320cabb54d..36db4e726332 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.