-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
1 parent
ae2c24b
commit e8f3727
Showing
4 changed files
with
195 additions
and
91 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, "<redacted>", 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, "<redacted>", 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) | ||
| }) | ||
| }) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters