From 1f3ade3e32938750f10126bf33df13e850249ddd Mon Sep 17 00:00:00 2001 From: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> Date: Tue, 15 Oct 2024 13:20:36 -0700 Subject: [PATCH] Diagnostics file writes use RedactSecretPaths (#5745) * Diagnostics file writes use RedactSecretPaths * Add integration test * Change to yaml.v3 in integration tests * revert update to yaml.v3 across other testing files --- ...ostics-files-will-redact-secret_paths.yaml | 34 +++++++ internal/pkg/diagnostics/diagnostics.go | 4 +- internal/pkg/diagnostics/diagnostics_test.go | 97 +++++++++++++++++++ testing/fleetservertest/checkin.go | 4 +- testing/fleetservertest/fleetserver_test.go | 4 +- testing/integration/diagnostics_test.go | 87 +++++++++++++++++ testing/integration/inspect_test.go | 6 +- 7 files changed, 227 insertions(+), 9 deletions(-) create mode 100644 changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml diff --git a/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml b/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml new file mode 100644 index 00000000000..38b69e892f1 --- /dev/null +++ b/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml @@ -0,0 +1,34 @@ +# 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: Diagnostics files will redact secret_paths + +# 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: | + The elastic-agent will use redact secret paths in files written in diagnostics bundles. + Secret paths are expected to be specified as a top-level attribute in yaml data being written. + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: + +# 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/elastic/elastic-agent/pull/5745 + +# 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/diagnostics/diagnostics.go b/internal/pkg/diagnostics/diagnostics.go index 2f918c06b0e..ceaa8f0296a 100644 --- a/internal/pkg/diagnostics/diagnostics.go +++ b/internal/pkg/diagnostics/diagnostics.go @@ -327,12 +327,13 @@ func writeRedacted(errOut, resultWriter io.Writer, fullFilePath string, fileResu // Should we support json too? if fileResult.ContentType == "application/yaml" { - unmarshalled := map[interface{}]interface{}{} + unmarshalled := map[string]interface{}{} err := yaml.Unmarshal(fileResult.Content, &unmarshalled) if err != nil { // 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)) if err != nil { // Best effort, output a warning but still include the file @@ -579,7 +580,6 @@ func saveLogs(name string, logPath string, zw *zip.Writer) error { func RedactSecretPaths(mapStr map[string]any, errOut io.Writer) map[string]any { v, ok := mapStr["secret_paths"] if !ok { - fmt.Fprintln(errOut, "No output redaction: secret_paths attribute not found.") return mapStr } arr, ok := v.([]interface{}) diff --git a/internal/pkg/diagnostics/diagnostics_test.go b/internal/pkg/diagnostics/diagnostics_test.go index cd066754697..ec21e4a525b 100644 --- a/internal/pkg/diagnostics/diagnostics_test.go +++ b/internal/pkg/diagnostics/diagnostics_test.go @@ -140,6 +140,103 @@ mapping: require.Empty(t, errOut.String()) } +func TestRedactWithSecretPaths(t *testing.T) { + tests := []struct { + name string + input []byte + expect string + }{{ + name: "no secret paths", + input: []byte(`id: test-policy +inputs: + - type: test_input + redactKey: secretValue +outputs: + default: + type: elasticsearch + api_key: secretKey + redactOtherKey: secretOutputValue +`), + expect: `id: test-policy +inputs: + - redactKey: secretValue + type: test_input +outputs: + default: + api_key: + redactOtherKey: secretOutputValue + type: elasticsearch +`, + }, { + name: "secret_paths are redacted", + input: []byte(`id: test-policy +secret_paths: + - inputs.0.redactKey + - outputs.default.redactOtherKey +inputs: + - type: test_input + redactKey: secretValue +outputs: + default: + type: elasticsearch + api_key: secretKey + redactOtherKey: secretOutputValue +`), + expect: `id: test-policy +inputs: + - redactKey: + type: test_input +outputs: + default: + api_key: + redactOtherKey: + type: elasticsearch +secret_paths: + - inputs.0.redactKey + - outputs.default.redactOtherKey +`, + }, { + name: "secret_paths contains extra keys", + input: []byte(`id: test-policy +secret_paths: + - inputs.0.redactKey + - inputs.1.missingKey + - outputs.default.redactOtherKey +inputs: + - type: test_input + redactKey: secretValue +outputs: + default: + type: elasticsearch + api_key: secretKey +`), + expect: `id: test-policy +inputs: + - redactKey: + type: test_input +outputs: + default: + api_key: + type: elasticsearch +secret_paths: + - inputs.0.redactKey + - inputs.1.missingKey + - outputs.default.redactOtherKey +`, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + file := client.DiagnosticFileResult{Content: tc.input, ContentType: "application/yaml"} + var out bytes.Buffer + err := writeRedacted(io.Discard, &out, "testPath", file) + require.NoError(t, err) + + assert.Equal(t, tc.expect, out.String()) + }) + } +} + func TestUnitAndStateMapping(t *testing.T) { // this structure causes problems due to the compound agentruntime.ComponentUnitKey map key exampleState := agentruntime.ComponentState{ diff --git a/testing/fleetservertest/checkin.go b/testing/fleetservertest/checkin.go index e0200f20719..adb83058cdc 100644 --- a/testing/fleetservertest/checkin.go +++ b/testing/fleetservertest/checkin.go @@ -174,11 +174,11 @@ const ( "hosts": {{ toJson .FleetHosts }} }, "id": "{{.PolicyID}}", - "secret_paths": ["inputs.0.secret_key"], + "secret_paths": ["inputs.0.custom_attr"], "inputs": [ { "id": "fake-input", - "secret_key": "secretValue", + "custom_attr": "secretValue", "revision": 1, "name": "fake-input", "type": "fake-input", diff --git a/testing/fleetservertest/fleetserver_test.go b/testing/fleetservertest/fleetserver_test.go index 979104f6e78..79a6cdcbf88 100644 --- a/testing/fleetservertest/fleetserver_test.go +++ b/testing/fleetservertest/fleetserver_test.go @@ -258,6 +258,7 @@ func ExampleNewServer_checkin_fleetConnectionParams() { // "id": "", // "inputs": [ // { + // "custom_attr": "secretValue", // "data_stream": { // "namespace": "default" // }, @@ -271,7 +272,6 @@ func ExampleNewServer_checkin_fleetConnectionParams() { // "name": "fake-input", // "package_policy_id": "", // "revision": 1, - // "secret_key": "secretValue", // "streams": [], // "type": "fake-input", // "use_output": "default" @@ -289,7 +289,7 @@ func ExampleNewServer_checkin_fleetConnectionParams() { // }, // "revision": 2, // "secret_paths": [ - // "inputs.0.secret_key" + // "inputs.0.custom_attr" // ], // "secret_references": [], // "signed": { diff --git a/testing/integration/diagnostics_test.go b/testing/integration/diagnostics_test.go index 04193210497..26e53c3d68e 100644 --- a/testing/integration/diagnostics_test.go +++ b/testing/integration/diagnostics_test.go @@ -20,11 +20,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/pkg/control/v2/client" integrationtest "github.com/elastic/elastic-agent/pkg/testing" "github.com/elastic/elastic-agent/pkg/testing/define" + "github.com/elastic/elastic-agent/pkg/testing/tools/check" "github.com/elastic/elastic-agent/pkg/testing/tools/testcontext" + "github.com/elastic/elastic-agent/testing/fleetservertest" ) const diagnosticsArchiveGlobPattern = "elastic-agent-diagnostics-*.zip" @@ -200,6 +203,90 @@ func TestIsolatedUnitsDiagnosticsCommand(t *testing.T) { assert.NoError(t, err) } +func TestRedactFleetSecretPathsDiagnostics(t *testing.T) { + _ = define.Require(t, define.Requirements{ + Group: Fleet, + Local: false, + Sudo: true, + }) + + ctx, cancel := testcontext.WithTimeout(t, context.Background(), time.Minute*10) + defer cancel() + + t.Log("Setup fake fleet-server") + apiKey, policy := createBasicFleetPolicyData(t, "http://fleet-server:8220") + checkinWithAcker := fleetservertest.NewCheckinActionsWithAcker() + fleet := fleetservertest.NewServerWithHandlers( + apiKey, + "enrollmentToken", + policy.AgentID, + policy.PolicyID, + checkinWithAcker.ActionsGenerator(), + checkinWithAcker.Acker(), + fleetservertest.WithRequestLog(t.Logf), + ) + defer fleet.Close() + policyChangeAction, err := fleetservertest.NewActionPolicyChangeWithFakeComponent("test-policy-change", fleetservertest.TmplPolicy{ + AgentID: policy.AgentID, + PolicyID: policy.PolicyID, + FleetHosts: []string{fleet.LocalhostURL}, + }) + require.NoError(t, err) + checkinWithAcker.AddCheckin("token", 0, policyChangeAction) + + t.Log("Enroll agent in fake fleet-server") + fixture, err := define.NewFixtureFromLocalBuild(t, + define.Version(), + integrationtest.WithAllowErrors(), + integrationtest.WithLogOutput()) + require.NoError(t, err, "SetupTest: NewFixtureFromLocalBuild failed") + err = fixture.EnsurePrepared(ctx) + require.NoError(t, err, "SetupTest: fixture.Prepare failed") + + out, err := fixture.Install( + ctx, + &integrationtest.InstallOpts{ + Force: true, + NonInteractive: true, + Insecure: true, + Privileged: false, + EnrollOpts: integrationtest.EnrollOpts{ + URL: fleet.LocalhostURL, + EnrollmentToken: "anythingWillDO", + }}) + require.NoErrorf(t, err, "Error when installing agent, output: %s", out) + check.ConnectedToFleet(ctx, t, fixture, 5*time.Minute) + + t.Log("Gather diagnostics.") + diagZip, err := fixture.ExecDiagnostics(ctx) + require.NoError(t, err, "error when gathering diagnostics") + stat, err := os.Stat(diagZip) + require.NoErrorf(t, err, "stat file %q failed", diagZip) + require.Greaterf(t, stat.Size(), int64(0), "file %s has incorrect size", diagZip) + + t.Log("Check if config files have been redacted.") + extractionDir := t.TempDir() + extractZipArchive(t, diagZip, extractionDir) + path := filepath.Join(extractionDir, "computed-config.yaml") + stat, err = os.Stat(path) + require.NoErrorf(t, err, "stat file %q failed", path) + require.Greaterf(t, stat.Size(), int64(0), "file %s has incorrect size", path) + f, err := os.Open(path) + require.NoErrorf(t, err, "open file %q failed", path) + defer f.Close() + var yObj struct { + SecretPaths []string `yaml:"secret_paths"` + Inputs []struct { + CustomAttr string `yaml:"custom_attr"` + } `yaml:"inputs"` + } + err = yaml.NewDecoder(f).Decode(&yObj) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"inputs.0.custom_attr"}, yObj.SecretPaths) + require.Len(t, yObj.Inputs, 1) + assert.Equal(t, "", yObj.Inputs[0].CustomAttr) +} + func testDiagnosticsFactory(t *testing.T, compSetup map[string]integrationtest.ComponentState, diagFiles []string, diagCompFiles []string, fix *integrationtest.Fixture, cmd []string) func(ctx context.Context) error { return func(ctx context.Context) error { diagZip, err := fix.ExecDiagnostics(ctx, cmd...) diff --git a/testing/integration/inspect_test.go b/testing/integration/inspect_test.go index 74866ff8460..cc8aaeab699 100644 --- a/testing/integration/inspect_test.go +++ b/testing/integration/inspect_test.go @@ -80,12 +80,12 @@ func TestInspect(t *testing.T) { var yObj struct { SecretPaths []string `yaml:"secret_paths"` Inputs []struct { - SecretKey string `yaml:"secret_key"` + CustomAttr string `yaml:"custom_attr"` } `yaml:"inputs"` } err = yaml.Unmarshal(p, &yObj) require.NoError(t, err) - assert.ElementsMatch(t, []string{"inputs.0.secret_key"}, yObj.SecretPaths) + assert.ElementsMatch(t, []string{"inputs.0.custom_attr"}, yObj.SecretPaths) require.Len(t, yObj.Inputs, 1) - assert.Equalf(t, "", yObj.Inputs[0].SecretKey, "inspect output: %s", p) + assert.Equalf(t, "", yObj.Inputs[0].CustomAttr, "inspect output: %s", p) }