[9.1] (backport #9560) Fix/5871 redact secrets in diagnostics (#9601)

* Fix/5871 redact secrets in diagnostics (#9560)

* fix(5871): added secret marker func

* fix(5871): added redaction marker test

* fix(5871): added secret marker in coordinator

* fix(5871): added secret marker func in coordinator unit tests

* fix(5871): update redactMap to handle redaction markers

* fix(5871): update redactmap tests

* fix(5871): update diagnostics integration test

* fix(5871): udpate inspect command to inject redaction markers

* fix(5871): add changelog fragment

(cherry picked from commit a44e87a)

# Conflicts:
#	internal/pkg/agent/application/coordinator/coordinator.go

* resolved merge conflicts

---------

Co-authored-by: Kaan Yalti <[email protected]>

Author: mergify[bot]

changelog/fragments/1756122869-update-secret-redaction-in-diagnostics.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: security
+
+# Change summary; a 80ish characters long description of the change.
+summary: redact secrets from pre-config, computed-config, components-expected, and components-actual files in diagnostics archive
+
+# 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/elastic/elastic-agent/pull/9560
+
+# 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

internal/pkg/agent/application/coordinator/coordinator.go

@@ -332,6 +332,9 @@ type Coordinator struct {
 	// run a ticker that checks to see if we have a new PID.
 	componentPIDTicker         *time.Ticker
 	componentPidRequiresUpdate *atomic.Bool
+
+	// Abstraction for diagnostics AddSecretMarkers function for testability
+	secretMarkerFunc func(*logger.Logger, *config.Config) error
 }
 
 // The channels Coordinator reads to receive updates from the various managers.
@@ -452,7 +455,8 @@ func New(
 		componentPIDTicker:         time.NewTicker(time.Second * 30),
 		componentPidRequiresUpdate: &atomic.Bool{},
 
-		fleetAcker: fleetAcker,
+		fleetAcker:       fleetAcker,
+		secretMarkerFunc: diagnostics.AddSecretMarkers,
 	}
 	// Setup communication channels for any non-nil components. This pattern
 	// lets us transparently accept nil managers / simulated events during
@@ -1371,6 +1375,10 @@ func (c *Coordinator) processConfigAgent(ctx context.Context, cfg *config.Config
 		span.End()
 	}()
 
+	if err = c.secretMarkerFunc(c.logger, cfg); err != nil {
+		c.logger.Errorf("failed to add secret markers: %v", err)
+	}
+
 	err = c.generateAST(cfg)
 	c.setConfigError(err)
 	if err != nil {

internal/pkg/agent/application/coordinator/coordinator_unit_test.go

@@ -41,10 +41,16 @@ import (
 	"github.com/elastic/elastic-agent/pkg/component"
 	"github.com/elastic/elastic-agent/pkg/component/runtime"
 	agentclient "github.com/elastic/elastic-agent/pkg/control/v2/client"
+	"github.com/elastic/elastic-agent/pkg/core/logger"
 	"github.com/elastic/elastic-agent/pkg/core/logger/loggertest"
 	"github.com/elastic/elastic-agent/pkg/utils/broadcaster"
 )
 
+var testSecretMarkerFunc = func(*logger.Logger, *config.Config) error {
+	// no-op secret marker function for testing
+	return nil
+}
+
 func TestVarsManagerError(t *testing.T) {
 	// Set a one-second timeout -- nothing here should block, but if it
 	// does let's report a failure instead of timing out the test runner.
@@ -475,6 +481,7 @@ func TestCoordinatorReportsInvalidPolicy(t *testing.T) {
 		vars:               emptyVars(t),
 		ast:                emptyAST(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Send an invalid config update and confirm that Coordinator reports
@@ -591,6 +598,7 @@ func TestCoordinatorReportsComponentModelError(t *testing.T) {
 		vars:               emptyVars(t),
 		ast:                emptyAST(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// This configuration produces a valid AST but its EQL condition is
@@ -659,7 +667,7 @@ func TestCoordinatorPolicyChangeUpdatesMonitorReloader(t *testing.T) {
 	// does let's report a failure instead of timing out the test runner.
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
 	defer cancel()
-	logger := logp.NewLogger("testing")
+	log := logp.NewLogger("testing")
 
 	configChan := make(chan ConfigChange, 1)
 
@@ -674,10 +682,16 @@ func TestCoordinatorPolicyChangeUpdatesMonitorReloader(t *testing.T) {
 	newServerFn := func(*monitoringCfg.MonitoringConfig) (reload.ServerController, error) {
 		return monitoringServer, nil
 	}
-	monitoringReloader := reload.NewServerReloader(newServerFn, logger, monitoringCfg.DefaultConfig())
+	monitoringReloader := reload.NewServerReloader(newServerFn, log, monitoringCfg.DefaultConfig())
+
+	secretMarkerCalled := false
+	mockSecretMarkerFunc := func(*logger.Logger, *config.Config) error {
+		secretMarkerCalled = true
+		return nil
+	}
 
 	coord := &Coordinator{
-		logger:           logger,
+		logger:           log,
 		agentInfo:        &info.AgentInfo{},
 		stateBroadcaster: broadcaster.New(State{}, 0, 0),
 		managerChans: managerChans{
@@ -687,6 +701,7 @@ func TestCoordinatorPolicyChangeUpdatesMonitorReloader(t *testing.T) {
 		otelMgr:            &fakeOTelManager{},
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   mockSecretMarkerFunc,
 	}
 	coord.RegisterMonitoringServer(monitoringReloader)
 
@@ -707,6 +722,8 @@ inputs:
 	coord.runLoopIteration(ctx)
 	assert.True(t, cfgChange.acked, "Coordinator should ACK a successful policy change")
 
+	assert.True(t, secretMarkerCalled, "secret marker should be called")
+
 	// server is started by default
 	assert.True(t, monitoringServer.startTriggered)
 	assert.True(t, monitoringServer.isRunning)
@@ -826,6 +843,7 @@ func TestCoordinatorPolicyChangeUpdatesRuntimeAndOTelManager(t *testing.T) {
 		otelMgr:            otelManager,
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Create a policy with one input and one output (no otel configuration)
@@ -998,6 +1016,7 @@ func TestCoordinatorPolicyChangeUpdatesRuntimeAndOTelManagerWithOtelComponents(t
 		specs:              specs,
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Create a policy with one input and one output (no otel configuration)
@@ -1096,6 +1115,7 @@ func TestCoordinatorReportsRuntimeManagerUpdateFailure(t *testing.T) {
 
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Send an empty policy which should forward an empty component model to
@@ -1157,6 +1177,7 @@ func TestCoordinatorReportsOTelManagerUpdateFailure(t *testing.T) {
 		otelMgr:            otelManager,
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Send an empty policy which should forward an empty component model to
@@ -1221,6 +1242,7 @@ func TestCoordinatorAppliesVarsToPolicy(t *testing.T) {
 		otelMgr:            &fakeOTelManager{},
 		vars:               emptyVars(t),
 		componentPIDTicker: time.NewTicker(time.Second * 30),
+		secretMarkerFunc:   testSecretMarkerFunc,
 	}
 
 	// Create a policy with one input and one output

internal/pkg/agent/cmd/inspect.go

@@ -145,6 +145,11 @@ func inspectConfig(ctx context.Context, cfgPath string, opts inspectConfigOpts,
 		if err != nil {
 			return fmt.Errorf("error loading agent config: %w", err)
 		}
+		// Ensure secret markers are injected based on secret_paths before redaction.
+		if err := diagnostics.AddSecretMarkers(l, fullCfg); err != nil {
+			fmt.Fprintf(streams.Err, "failed to add secret markers: %v\n", err)
+		}
+
 		err = printConfig(fullCfg, streams)
 		if err != nil {
 			return fmt.Errorf("error printing config: %w", err)
@@ -228,6 +233,16 @@ func inspectConfig(ctx context.Context, cfgPath string, opts inspectConfigOpts,
 
 	}
 
+	// Ensure secret markers are injected based on secret_paths before redaction.
+	rawCfg := config.MustNewConfigFrom(cfg)
+	if err := diagnostics.AddSecretMarkers(l, rawCfg); err != nil {
+		fmt.Fprintf(streams.Err, "failed to add secret markers: %v\n", err)
+	}
+	cfg, err = rawCfg.ToMapStr()
+	if err != nil {
+		return fmt.Errorf("failed to convert config with secret markers: %w", err)
+	}
+
 	return printMapStringConfig(cfg, streams)
 }