Diagnostics file writes use RedactSecretPaths #5745

michel-laterman · 2024-10-08T22:34:31Z

What does this PR do?

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.

Why is it important?

Secrets that the fleet-server injects into policies can appear in diagnostics bundles.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

Disruptive User Impact

Secrets in diagnostics bundles will be redacted, as our public documentation discloses.

How to test this PR locally

Enrol in a fleet policy with secrets and collect a diagnostics bundle.

mergify · 2024-10-08T22:35:16Z

This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-10-08T22:35:17Z

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

michel-laterman · 2024-10-08T22:35:46Z

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{}{}


I think unmarshaling to map[interface{}]interface{} was a restriction with yaml.v2 which is not a part of yaml.v3, see go-yaml/yaml#139 (comment)

michel-laterman · 2024-10-09T19:30:48Z

internal/pkg/diagnostics/diagnostics.go

@@ -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.")


When a diagnostics action is ran secrets in the component related files are not redacted (structure does not match, and secret_paths is not passed)
This line causes extra noise in the output

michel-laterman · 2024-10-09T19:31:38Z

testing/fleetservertest/checkin.go

          "inputs": [
            {
              "id": "fake-input",
-              "secret_key": "secretValue",
+              "custom_attr": "secretValue",


I've renamed to custom_attr to ensurue redactKey does not match the attribute key

elasticmachine · 2024-10-09T19:32:08Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

blakerouse · 2024-10-09T20:31:50Z

testing/integration/diagnostics_test.go

@@ -20,11 +20,14 @@ import (

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v2"


Above you where talking about yaml.v3 but here you import yaml.v2? Which one is being used here? We should unify our yaml usage to a single library.

I've updated this and all the integration test uses, and have made #5750 to do the rest

blakerouse

Thanks for updating to yaml.v3. This looks good!

swiatekm

LGTM

swiatekm · 2024-10-10T10:38:01Z

internal/pkg/diagnostics/diagnostics_test.go

+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: <REDACTED>
+        redactOtherKey: secretOutputValue
+        type: elasticsearch


I think these would be more readable if inputs and outputs used the same indentation.

jlind23 · 2024-10-14T11:43:56Z

Looks like tests are panicking, weirdly enough this seems to be related to the yaml.V3 bump.

--- FAIL: TestInstallWithBasePath (27.97s)
panic: interface conversion: interface {} is map[string]interface {}, not map[interface {}]interface {} [recovered]
	panic: interface conversion: interface {} is map[string]interface {}, not map[interface {}]interface {}

goroutine 84 [running]:
testing.tRunner.func1.2({0x4368f40, 0xc0006af6e0})
	/usr/local/go/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1634 +0x377
panic({0x4368f40?, 0xc0006af6e0?})
	/usr/local/go/src/runtime/panic.go:770 +0x132
github.com/elastic/elastic-agent/testing/integration.unmarshalVersionOutput(0xc00052e000, {0xc000855400, 0x81, 0x400}, {0x480c968, 0x6})
	/home/ubuntu/agent/testing/integration/pkgversion_common_test.go:106 +0x1d8
github.com/elastic/elastic-agent/testing/integration.TestInstallWithBasePath.testAgentPackageVersion.func1(0xc00052e000)
	/home/ubuntu/agent/testing/integration/pkgversion_common_test.go:48 +0x27a
testing.tRunner(0xc00052e000, 0xc0006aef30)
	/usr/local/go/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 76
	/usr/local/go/src/testing/testing.go:1742 +0x390

michel-laterman · 2024-10-15T16:13:49Z

I'm going to roll back some of the updates to yaml.v3

elastic-sonarqube · 2024-10-15T18:28:39Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

ycombinator · 2024-10-15T20:20:31Z

Not sure why SonarQube is reporting 0% coverage on new code. The unit test TestRedactWithSecretPaths was added and it exercises code that was changed in the writeRedacted function. Force merging.

* Diagnostics file writes use RedactSecretPaths * Add integration test * Change to yaml.v3 in integration tests * revert update to yaml.v3 across other testing files (cherry picked from commit 1f3ade3)

* Diagnostics file writes use RedactSecretPaths * Add integration test * Change to yaml.v3 in integration tests * revert update to yaml.v3 across other testing files (cherry picked from commit 1f3ade3) Co-authored-by: Michel Laterman <[email protected]>

michel-laterman added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Oct 8, 2024

mergify bot assigned michel-laterman Oct 8, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 8, 2024

michel-laterman commented Oct 8, 2024

View reviewed changes

michel-laterman commented Oct 9, 2024

View reviewed changes

michel-laterman marked this pull request as ready for review October 9, 2024 19:32

michel-laterman requested a review from a team as a code owner October 9, 2024 19:32

michel-laterman requested review from blakerouse and swiatekm October 9, 2024 19:32

blakerouse reviewed Oct 9, 2024

View reviewed changes

blakerouse approved these changes Oct 9, 2024

View reviewed changes

swiatekm approved these changes Oct 10, 2024

View reviewed changes

michel-laterman added 4 commits October 15, 2024 10:57

Diagnostics file writes use RedactSecretPaths

117155b

Add integration test

8a7225d

Change to yaml.v3 in integration tests

279aec4

revert update to yaml.v3 across other testing files

0cb68e5

ycombinator force-pushed the redact-secret-paths-diag branch from e0b0651 to 0cb68e5 Compare October 15, 2024 17:57

ycombinator merged commit 1f3ade3 into elastic:main Oct 15, 2024
13 of 14 checks passed

mergify bot mentioned this pull request Oct 15, 2024

[8.x](backport #5745) Diagnostics file writes use RedactSecretPaths #5798

Merged

5 tasks

michel-laterman deleted the redact-secret-paths-diag branch October 15, 2024 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diagnostics file writes use RedactSecretPaths #5745

Diagnostics file writes use RedactSecretPaths #5745

michel-laterman commented Oct 8, 2024 •

edited

Loading

mergify bot commented Oct 8, 2024

mergify bot commented Oct 8, 2024

michel-laterman Oct 8, 2024

michel-laterman Oct 9, 2024

michel-laterman Oct 9, 2024

elasticmachine commented Oct 9, 2024

blakerouse Oct 9, 2024

michel-laterman Oct 9, 2024

blakerouse left a comment

swiatekm left a comment

swiatekm Oct 10, 2024

jlind23 commented Oct 14, 2024

michel-laterman commented Oct 15, 2024

elastic-sonarqube bot commented Oct 15, 2024

ycombinator commented Oct 15, 2024

Diagnostics file writes use RedactSecretPaths #5745

Diagnostics file writes use RedactSecretPaths #5745

Conversation

michel-laterman commented Oct 8, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Disruptive User Impact

How to test this PR locally

mergify bot commented Oct 8, 2024

mergify bot commented Oct 8, 2024

michel-laterman Oct 8, 2024

Choose a reason for hiding this comment

michel-laterman Oct 9, 2024

Choose a reason for hiding this comment

michel-laterman Oct 9, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 9, 2024

blakerouse Oct 9, 2024

Choose a reason for hiding this comment

michel-laterman Oct 9, 2024

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

swiatekm Oct 10, 2024

Choose a reason for hiding this comment

jlind23 commented Oct 14, 2024

michel-laterman commented Oct 15, 2024

elastic-sonarqube bot commented Oct 15, 2024

Quality Gate failed

ycombinator commented Oct 15, 2024

michel-laterman commented Oct 8, 2024 •

edited

Loading