From 117155b60565dae29aa48acb6558931004514d42 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 8 Oct 2024 15:20:24 -0700 Subject: [PATCH 1/4] Diagnostics file writes use RedactSecretPaths --- ...ostics-files-will-redact-secret_paths.yaml | 34 ++++++ internal/pkg/diagnostics/diagnostics.go | 3 +- internal/pkg/diagnostics/diagnostics_test.go | 100 +++++++++++++++++- 3 files changed, 135 insertions(+), 2 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..a4a45acd4d0 --- /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/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/diagnostics/diagnostics.go b/internal/pkg/diagnostics/diagnostics.go index 2f918c06b0e..3d385beaecd 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 diff --git a/internal/pkg/diagnostics/diagnostics_test.go b/internal/pkg/diagnostics/diagnostics_test.go index cd066754697..8f349cd814d 100644 --- a/internal/pkg/diagnostics/diagnostics_test.go +++ b/internal/pkg/diagnostics/diagnostics_test.go @@ -84,6 +84,7 @@ i4EFZLWrFRsAAAARYWxleGtAZ3JlbWluLm5lc3QBAg== -----END OPENSSH PRIVATE KEY-----` exampleConfig := mapstr.M{ + "secret_paths": []string{}, "root": mapstr.M{ "passphrase": "unredacted", "nested1": mapstr.M{ @@ -140,6 +141,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{ @@ -162,7 +260,7 @@ func TestUnitAndStateMapping(t *testing.T) { err = writeRedacted(&errOut, &outWriter, "test/path", res) require.NoError(t, err) - require.Empty(t, errOut.String()) + require.Equal(t, "No output redaction: secret_paths attribute not found.\n", errOut.String()) } type zippedItem struct { From 8a7225df74f1ee50697fa58ef8fffbe02a781b00 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Wed, 9 Oct 2024 10:05:33 -0700 Subject: [PATCH 2/4] Add integration test --- ...ostics-files-will-redact-secret_paths.yaml | 2 +- internal/pkg/diagnostics/diagnostics.go | 1 - internal/pkg/diagnostics/diagnostics_test.go | 3 +- 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, 96 insertions(+), 11 deletions(-) diff --git a/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml b/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml index a4a45acd4d0..38b69e892f1 100644 --- a/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml +++ b/changelog/fragments/1728425752-Diagnostics-files-will-redact-secret_paths.yaml @@ -27,7 +27,7 @@ component: # 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 +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. diff --git a/internal/pkg/diagnostics/diagnostics.go b/internal/pkg/diagnostics/diagnostics.go index 3d385beaecd..ceaa8f0296a 100644 --- a/internal/pkg/diagnostics/diagnostics.go +++ b/internal/pkg/diagnostics/diagnostics.go @@ -580,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 8f349cd814d..ec21e4a525b 100644 --- a/internal/pkg/diagnostics/diagnostics_test.go +++ b/internal/pkg/diagnostics/diagnostics_test.go @@ -84,7 +84,6 @@ i4EFZLWrFRsAAAARYWxleGtAZ3JlbWluLm5lc3QBAg== -----END OPENSSH PRIVATE KEY-----` exampleConfig := mapstr.M{ - "secret_paths": []string{}, "root": mapstr.M{ "passphrase": "unredacted", "nested1": mapstr.M{ @@ -260,7 +259,7 @@ func TestUnitAndStateMapping(t *testing.T) { err = writeRedacted(&errOut, &outWriter, "test/path", res) require.NoError(t, err) - require.Equal(t, "No output redaction: secret_paths attribute not found.\n", errOut.String()) + require.Empty(t, errOut.String()) } type zippedItem struct { 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..cf12924a2c8 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.v2" "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) } From 279aec449cda10342bb99844ec3add0119443a05 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Wed, 9 Oct 2024 14:14:54 -0700 Subject: [PATCH 3/4] Change to yaml.v3 in integration tests --- testing/integration/diagnostics_test.go | 2 +- testing/integration/inspect_test.go | 2 +- testing/integration/package_version_test.go | 2 +- testing/integration/pkgversion_common_test.go | 2 +- testing/integration/proxy_url_test.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/testing/integration/diagnostics_test.go b/testing/integration/diagnostics_test.go index cf12924a2c8..26e53c3d68e 100644 --- a/testing/integration/diagnostics_test.go +++ b/testing/integration/diagnostics_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/pkg/control/v2/client" integrationtest "github.com/elastic/elastic-agent/pkg/testing" diff --git a/testing/integration/inspect_test.go b/testing/integration/inspect_test.go index cc8aaeab699..95dbf452639 100644 --- a/testing/integration/inspect_test.go +++ b/testing/integration/inspect_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" integrationtest "github.com/elastic/elastic-agent/pkg/testing" "github.com/elastic/elastic-agent/pkg/testing/define" diff --git a/testing/integration/package_version_test.go b/testing/integration/package_version_test.go index b3a7816bf3c..851aa02b7dc 100644 --- a/testing/integration/package_version_test.go +++ b/testing/integration/package_version_test.go @@ -22,7 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/pkg/control/v2/client" atesting "github.com/elastic/elastic-agent/pkg/testing" diff --git a/testing/integration/pkgversion_common_test.go b/testing/integration/pkgversion_common_test.go index ec2ad6d03e4..63c07dcb165 100644 --- a/testing/integration/pkgversion_common_test.go +++ b/testing/integration/pkgversion_common_test.go @@ -16,7 +16,7 @@ import ( "sync" "testing" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/testing/integration/proxy_url_test.go b/testing/integration/proxy_url_test.go index c4e067831c7..5de7dddebb6 100644 --- a/testing/integration/proxy_url_test.go +++ b/testing/integration/proxy_url_test.go @@ -22,7 +22,7 @@ import ( "github.com/gofrs/uuid/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent-libs/testing/certutil" integrationtest "github.com/elastic/elastic-agent/pkg/testing" From 0cb68e558be5eaf26c4548923d01d5df4a029ad3 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 15 Oct 2024 09:23:13 -0700 Subject: [PATCH 4/4] revert update to yaml.v3 across other testing files --- testing/integration/inspect_test.go | 2 +- testing/integration/package_version_test.go | 2 +- testing/integration/pkgversion_common_test.go | 2 +- testing/integration/proxy_url_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/integration/inspect_test.go b/testing/integration/inspect_test.go index 95dbf452639..cc8aaeab699 100644 --- a/testing/integration/inspect_test.go +++ b/testing/integration/inspect_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" + "gopkg.in/yaml.v2" integrationtest "github.com/elastic/elastic-agent/pkg/testing" "github.com/elastic/elastic-agent/pkg/testing/define" diff --git a/testing/integration/package_version_test.go b/testing/integration/package_version_test.go index 851aa02b7dc..b3a7816bf3c 100644 --- a/testing/integration/package_version_test.go +++ b/testing/integration/package_version_test.go @@ -22,7 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" + "gopkg.in/yaml.v2" "github.com/elastic/elastic-agent/pkg/control/v2/client" atesting "github.com/elastic/elastic-agent/pkg/testing" diff --git a/testing/integration/pkgversion_common_test.go b/testing/integration/pkgversion_common_test.go index 63c07dcb165..ec2ad6d03e4 100644 --- a/testing/integration/pkgversion_common_test.go +++ b/testing/integration/pkgversion_common_test.go @@ -16,7 +16,7 @@ import ( "sync" "testing" - "gopkg.in/yaml.v3" + "gopkg.in/yaml.v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/testing/integration/proxy_url_test.go b/testing/integration/proxy_url_test.go index 5de7dddebb6..c4e067831c7 100644 --- a/testing/integration/proxy_url_test.go +++ b/testing/integration/proxy_url_test.go @@ -22,7 +22,7 @@ import ( "github.com/gofrs/uuid/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" + "gopkg.in/yaml.v2" "github.com/elastic/elastic-agent-libs/testing/certutil" integrationtest "github.com/elastic/elastic-agent/pkg/testing"