Skip to content

Commit

Permalink
settings: redact all string settings for diagnostics
Browse files Browse the repository at this point in the history
Previously, the redaction logic for `Sensitive` settings in
the diagnotics payload was conditional on the value of the
`"server.redact_sensitive_settings.enabled"` cluster setting.

This commit modifies the behavior of `RedactedValue` used to render
modified cluster settings by the `diagnostics` package to always
fully redact the values of string settings and any sensitive or non-
reportable settings.

Because the `MaskedSetting` struct is now in use by code in the `SHOW
CLUSTER SETTING` code path, we no longer rely on it for redaction
behavior of string settings.

Resolves: CRDB-43457
Epic: None

Release note (security update): all cluster settings that accept
strings are now fully redacted when transmitted as part of our
diagnostics telemetry. This payload includes a record of modified
cluster settings and their values when they are not strings.
Customers who previously applied the mitigations in Technical
Advisory 133479 can safely set the value of cluster setting
`server.redact_sensitive_settings.enabled` to false and turn on
diagnostic reporting via the `diagnostics.reporting.enabled` cluster
setting without leaking sensitive cluster settings values.
  • Loading branch information
dhartunian committed Nov 1, 2024
1 parent f23716f commit c149620
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 4 deletions.
18 changes: 17 additions & 1 deletion pkg/ccl/serverccl/diagnosticsccl/reporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,19 @@ func TestServerReport(t *testing.T) {
})
}

// We want to ensure that non-reportable settings, sensitive
// settings, and all string settings are redacted. Below we override
// one of each.
settingOverrides := []string{
`SET CLUSTER SETTING server.oidc_authentication.client_id = 'sensitive-client-id'`, // Sensitive setting.
`SET CLUSTER SETTING sql.log.user_audit = 'test_role NONE'`, // Non-reportable setting.
`SET CLUSTER SETTING changefeed.node_throttle_config = '{"message_rate": 0.5}'`, // String setting.
}
for _, s := range settingOverrides {
_, err := rt.serverDB.Exec(s)
require.NoError(t, err)
}

expectedUsageReports := 0

clusterSecret := sql.ClusterSecret.Get(&rt.settings.SV)
Expand Down Expand Up @@ -195,7 +208,7 @@ func TestServerReport(t *testing.T) {
// 3 + 3 = 6: set 3 initially and org is set mid-test for 3 altered settings,
// plus version, reporting and secret settings are set in startup
// migrations.
expected, actual := 6, len(last.AlteredSettings)
expected, actual := 6+len(settingOverrides), len(last.AlteredSettings)
require.Equal(t, expected, actual, "expected %d changed settings, got %d: %v", expected, actual, last.AlteredSettings)

for key, expected := range map[string]string{
Expand All @@ -204,6 +217,9 @@ func TestServerReport(t *testing.T) {
"server.time_until_store_dead": "1m30s",
"version": clusterversion.TestingBinaryVersion.String(),
"cluster.secret": "<redacted>",
"server.oidc_authentication.client_id": "<redacted>",
"sql.log.user_audit": "<redacted>",
"changefeed.node_throttle_config": "<redacted>",
} {
got, ok := last.AlteredSettings[key]
require.True(t, ok, "expected report of altered setting %q", key)
Expand Down
10 changes: 7 additions & 3 deletions pkg/settings/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,15 @@ var ReadableTypes = map[string]string{
}

// RedactedValue returns:
// - a string representation of the value, if the setting is reportable (or it
// is a string setting with an empty value);
// - "<redacted>" if the setting is not reportable;
// - a string representation of the value, if the setting is reportable;
// - "<redacted>" if the setting is not reportable, or a string;
// - "<unknown>" if there is no setting with this name.
func RedactedValue(name string, values *Values, forSystemTenant bool) string {
if k, ok := registry[name]; ok {
if k.Typ() == "s" || !k.isReportable() {
return "<redacted>"
}
}
if setting, ok := LookupForReporting(name, forSystemTenant); ok {
return setting.String(values)
}
Expand Down

0 comments on commit c149620

Please sign in to comment.