From 96f2b9fc0dbddfcaac1c55e0e0bc9aed3bf6badb Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 9 Dec 2024 12:09:42 +0000 Subject: [PATCH] Redact common secrets in `elastic-agent inspect` output (#6224) (#6240) * Redact common secrets in elastic-agent inspect output * add changelog entry * test: update tests that check `inspect` output (cherry picked from commit f3aea4063f42ca2b738341950e730a954bc9ed19) Co-authored-by: Andrzej Stencel --- ...dact-common-secrets-in-inspect-output.yaml | 32 +++++++++++++++++++ internal/pkg/agent/cmd/inspect.go | 2 +- internal/pkg/diagnostics/diagnostics.go | 9 ++++-- testing/integration/endpoint_security_test.go | 22 ++++++------- testing/integration/inspect_test.go | 8 +++++ 5 files changed, 59 insertions(+), 14 deletions(-) create mode 100644 changelog/fragments/1733397457-redact-common-secrets-in-inspect-output.yaml diff --git a/changelog/fragments/1733397457-redact-common-secrets-in-inspect-output.yaml b/changelog/fragments/1733397457-redact-common-secrets-in-inspect-output.yaml new file mode 100644 index 00000000000..52c16241b5b --- /dev/null +++ b/changelog/fragments/1733397457-redact-common-secrets-in-inspect-output.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: enhancement + +# Change summary; a 80ish characters long description of the change. +summary: Redact common secrets like API keys and passwords in the output from `inspect` command + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +#description: + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: elastic-agent + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +#pr: https://github.com/owner/repo/1234 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +#issue: https://github.com/owner/repo/1234 diff --git a/internal/pkg/agent/cmd/inspect.go b/internal/pkg/agent/cmd/inspect.go index 600b4ba4985..c340a1fc8dc 100644 --- a/internal/pkg/agent/cmd/inspect.go +++ b/internal/pkg/agent/cmd/inspect.go @@ -236,7 +236,7 @@ func inspectConfig(ctx context.Context, cfgPath string, opts inspectConfigOpts, } func printMapStringConfig(mapStr map[string]interface{}, streams *cli.IOStreams) error { - data, err := yaml.Marshal(diagnostics.RedactSecretPaths(mapStr, streams.Err)) + data, err := yaml.Marshal(diagnostics.Redact(mapStr, streams.Err)) if err != nil { return errors.New(err, "could not marshal to YAML") } diff --git a/internal/pkg/diagnostics/diagnostics.go b/internal/pkg/diagnostics/diagnostics.go index be80811447f..1e7ec30c7d6 100644 --- a/internal/pkg/diagnostics/diagnostics.go +++ b/internal/pkg/diagnostics/diagnostics.go @@ -333,8 +333,8 @@ func writeRedacted(errOut, resultWriter io.Writer, fullFilePath string, fileResu // Best effort, output a warning but still include the file fmt.Fprintf(errOut, "[WARNING] Could not redact %s due to unmarshalling error: %s\n", fullFilePath, err) } else { - unmarshalled = RedactSecretPaths(unmarshalled, errOut) - redacted, err := yaml.Marshal(redactMap(errOut, unmarshalled)) + unmarshalled = Redact(unmarshalled, errOut) + redacted, err := yaml.Marshal(unmarshalled) if err != nil { // Best effort, output a warning but still include the file fmt.Fprintf(errOut, "[WARNING] Could not redact %s due to marshalling error: %s\n", fullFilePath, err) @@ -575,6 +575,11 @@ func saveLogs(name string, logPath string, zw *zip.Writer) error { return nil } +// Redact redacts sensitive values from the passed mapStr. +func Redact(mapStr map[string]any, errOut io.Writer) map[string]any { + return redactMap(errOut, RedactSecretPaths(mapStr, errOut)) +} + // RedactSecretPaths will check the passed mapStr input for a secret_paths attribute. // If found it will replace the value for every key in the paths list with and return the resulting map. // Any issues or errors will be written to the errOut writer. diff --git a/testing/integration/endpoint_security_test.go b/testing/integration/endpoint_security_test.go index b2b4526f20d..197fc7d9f5a 100644 --- a/testing/integration/endpoint_security_test.go +++ b/testing/integration/endpoint_security_test.go @@ -1063,8 +1063,8 @@ func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { require.NoErrorf(t, err, "error running inspect cmd") assert.Equal(t, proxyCLI.URL, got.Fleet.ProxyURL) - assert.Equal(t, mtlsCLI.clientCertPath, got.Fleet.Ssl.Certificate) - assert.Equal(t, mtlsCLI.clientCertKeyPath, got.Fleet.Ssl.Key) + assert.Equal(t, "", got.Fleet.Ssl.Certificate) + assert.Equal(t, "", got.Fleet.Ssl.Key) assert.Empty(t, got.Fleet.Ssl.KeyPassphrasePath, "policy should have removed key_passphrase_path as key isn't passphrase protected anymore") }, }, @@ -1083,9 +1083,9 @@ func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { require.NoErrorf(t, err, "error running inspect cmd") assert.Equal(t, proxyCLI.URL, got.Fleet.ProxyURL) - assert.Equal(t, mtlsCLI.clientCertPath, got.Fleet.Ssl.Certificate) - assert.Equal(t, mtlsCLI.clientCertKeyEncPath, got.Fleet.Ssl.Key) - assert.Equal(t, mtlsCLI.clientCertKeyPassPath, got.Fleet.Ssl.KeyPassphrasePath) + assert.Equal(t, "", got.Fleet.Ssl.Certificate) + assert.Equal(t, "", got.Fleet.Ssl.Key) + assert.Equal(t, "", got.Fleet.Ssl.KeyPassphrasePath) }, }, { @@ -1155,8 +1155,8 @@ func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { return } assert.Equal(t, proxyPolicymTLS.URL, got.Fleet.ProxyURL) - assert.Equal(t, mtlsPolicy.clientCertPath, got.Fleet.Ssl.Certificate) - assert.Equal(t, mtlsPolicy.clientCertKeyPath, got.Fleet.Ssl.Key) + assert.Equal(t, "", got.Fleet.Ssl.Certificate) + assert.Equal(t, "", got.Fleet.Ssl.Key) assert.Empty(t, got.Fleet.Ssl.KeyPassphrasePath, "policy should have removed key_passphrase_path as key isn't passphrase protected anymore") }, }, @@ -1193,8 +1193,8 @@ func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { } assert.Equal(t, proxyPolicymTLS.URL, got.Fleet.ProxyURL) - assert.Equal(t, mtlsPolicy.clientCertPath, got.Fleet.Ssl.Certificate) - assert.Equal(t, mtlsPolicy.clientCertKeyPath, got.Fleet.Ssl.Key) + assert.Equal(t, "", got.Fleet.Ssl.Certificate) + assert.Equal(t, "", got.Fleet.Ssl.Key) assert.Empty(t, got.Fleet.Ssl.KeyPassphrasePath, "policy should have removed key_passphrase_path as key isn't passphrase protected anymore") }, }, @@ -1225,8 +1225,8 @@ func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { } assert.Equal(t, proxyPolicymTLS.URL, got.Fleet.ProxyURL) - assert.Equal(t, mtlsPolicy.clientCertPath, got.Fleet.Ssl.Certificate) - assert.Equal(t, mtlsPolicy.clientCertKeyPath, got.Fleet.Ssl.Key) + assert.Equal(t, "", got.Fleet.Ssl.Certificate) + assert.Equal(t, "", got.Fleet.Ssl.Key) assert.Empty(t, got.Fleet.Ssl.KeyPassphrasePath, "key_passphrase_path was never set") }, }, diff --git a/testing/integration/inspect_test.go b/testing/integration/inspect_test.go index cc8aaeab699..4bbd252db5a 100644 --- a/testing/integration/inspect_test.go +++ b/testing/integration/inspect_test.go @@ -78,6 +78,12 @@ func TestInspect(t *testing.T) { require.NoErrorf(t, err, "Error when running inspect, output: %s", p) // Unmarshal into minimal object just to check if a secret has been redacted. var yObj struct { + Agent struct { + Protection struct { + SigningKey string `yaml:"signing_key"` + UninstallTokenHash string `yaml:"uninstall_token_hash"` + } `yaml:"protection"` + } `yaml:"agent"` SecretPaths []string `yaml:"secret_paths"` Inputs []struct { CustomAttr string `yaml:"custom_attr"` @@ -88,4 +94,6 @@ func TestInspect(t *testing.T) { assert.ElementsMatch(t, []string{"inputs.0.custom_attr"}, yObj.SecretPaths) require.Len(t, yObj.Inputs, 1) assert.Equalf(t, "", yObj.Inputs[0].CustomAttr, "inspect output: %s", p) + assert.Equalf(t, "", yObj.Agent.Protection.SigningKey, "`signing_key` is not redacted but it should be, because it contains `key`. inspect output: %s", p) + assert.Equalf(t, "", yObj.Agent.Protection.UninstallTokenHash, "`uninstall_token_hash` is not redacted but it should be, because it contains `token`. inspect output: %s", p) }