From d7546ddc8c8e2b1a840a7731e88dfaf8b83426be Mon Sep 17 00:00:00 2001 From: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> Date: Mon, 7 Oct 2024 08:53:59 -0700 Subject: [PATCH] redact secret_paths from elastic-agent inspect output (#5621) redact secret_paths from elastic-agent inspect output. elastic-agent inspect will now redact the value for any key in the secret_paths array. secret_paths is expected to be part of the policy. --- ...nd-will-redact-secret_paths-in-policy.yaml | 34 +++ internal/pkg/agent/cmd/inspect.go | 3 +- internal/pkg/diagnostics/diagnostics.go | 38 ++++ internal/pkg/diagnostics/diagnostics_test.go | 194 ++++++++++++++++++ testing/fleetservertest/checkin.go | 2 + testing/fleetservertest/fleetserver_test.go | 4 + testing/integration/inspect_test.go | 91 ++++++++ 7 files changed, 365 insertions(+), 1 deletion(-) create mode 100644 changelog/fragments/1727453779-inspect-command-will-redact-secret_paths-in-policy.yaml create mode 100644 testing/integration/inspect_test.go diff --git a/changelog/fragments/1727453779-inspect-command-will-redact-secret_paths-in-policy.yaml b/changelog/fragments/1727453779-inspect-command-will-redact-secret_paths-in-policy.yaml new file mode 100644 index 00000000000..652c532f992 --- /dev/null +++ b/changelog/fragments/1727453779-inspect-command-will-redact-secret_paths-in-policy.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: inspect command will redact secret_paths in policy + +# 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 inspect command will now redact any secret values when displaying output. + The keys that are redacted are expected to be defined in the "secret_paths" attribute. + +# 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/agent/cmd/inspect.go b/internal/pkg/agent/cmd/inspect.go index c163360a01f..600b4ba4985 100644 --- a/internal/pkg/agent/cmd/inspect.go +++ b/internal/pkg/agent/cmd/inspect.go @@ -28,6 +28,7 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/cli" "github.com/elastic/elastic-agent/internal/pkg/config" "github.com/elastic/elastic-agent/internal/pkg/config/operations" + "github.com/elastic/elastic-agent/internal/pkg/diagnostics" "github.com/elastic/elastic-agent/pkg/component" "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/utils" @@ -235,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(mapStr) + data, err := yaml.Marshal(diagnostics.RedactSecretPaths(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 3abe90c6a1d..2f918c06b0e 100644 --- a/internal/pkg/diagnostics/diagnostics.go +++ b/internal/pkg/diagnostics/diagnostics.go @@ -20,10 +20,12 @@ import ( "time" "github.com/elastic/elastic-agent/pkg/control/v2/client" + "github.com/elastic/go-ucfg" "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" + "github.com/elastic/elastic-agent/internal/pkg/config" "github.com/elastic/elastic-agent/internal/pkg/release" "github.com/elastic/elastic-agent/pkg/component" "github.com/elastic/elastic-agent/version" @@ -570,3 +572,39 @@ func saveLogs(name string, logPath string, zw *zip.Writer) error { return nil } + +// 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. +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{}) + if !ok { + fmt.Fprintln(errOut, "No output redaction: secret_paths attribute is not a list.") + return mapStr + } + cfg := ucfg.MustNewFrom(mapStr) + for _, v := range arr { + key, ok := v.(string) + if !ok { + fmt.Fprintf(errOut, "No output redaction for %q: expected type string, is type %T.\n", v, v) + continue + } + + if ok, _ := cfg.Has(key, -1, ucfg.PathSep(".")); ok { + err := cfg.SetString(key, -1, REDACTED, ucfg.PathSep(".")) + if err != nil { + fmt.Fprintf(errOut, "No output redaction for %q: %v.\n", key, err) + } + } + } + result, err := config.MustNewConfigFrom(cfg).ToMapStr() + if err != nil { + return mapStr + } + return result +} diff --git a/internal/pkg/diagnostics/diagnostics_test.go b/internal/pkg/diagnostics/diagnostics_test.go index ee89121a1fe..cd066754697 100644 --- a/internal/pkg/diagnostics/diagnostics_test.go +++ b/internal/pkg/diagnostics/diagnostics_test.go @@ -340,3 +340,197 @@ func isPprof(input []byte) (bool, error) { } return true, nil } + +func TestRedactSecretPaths(t *testing.T) { + tests := []struct { + name string + input map[string]interface{} + expect map[string]interface{} + }{{ + name: "no secret_paths", + input: map[string]interface{}{ + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + expect: map[string]interface{}{ + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + }, { + name: "secret paths is not an array", + input: map[string]interface{}{ + "secret_paths": "inputs.0.secret,outputs.default.api_key", + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + expect: map[string]interface{}{ + "secret_paths": "inputs.0.secret,outputs.default.api_key", + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + }, { + name: "secret_paths are redacted", + input: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + expect: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": REDACTED, + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": REDACTED, + }, + }, + }, + }, { + name: "secret_paths contains extra keys", + input: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + "inputs.1.secret", + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + expect: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + "inputs.1.secret", + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": REDACTED, + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": REDACTED, + }, + }, + }, + }, { + name: "secret_paths contains non string key", + input: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + 2, + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": "apikeyvalue", + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": "secretvalue", + }, + }, + }, + expect: map[string]interface{}{ + "secret_paths": []interface{}{ + "inputs.0.secret", + "outputs.default.api_key", + uint64(2), // go-ucfg serializing/deserializing flattens types + }, + "outputs": map[string]interface{}{ + "default": map[string]interface{}{ + "type": "elasticsearch", + "api_key": REDACTED, + }, + }, + "inputs": []interface{}{ + map[string]interface{}{ + "type": "example", + "secret": REDACTED, + }, + }, + }, + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := RedactSecretPaths(tc.input, io.Discard) + assert.Equal(t, tc.expect, result) + }) + } +} diff --git a/testing/fleetservertest/checkin.go b/testing/fleetservertest/checkin.go index b160fb7bd14..e0200f20719 100644 --- a/testing/fleetservertest/checkin.go +++ b/testing/fleetservertest/checkin.go @@ -174,9 +174,11 @@ const ( "hosts": {{ toJson .FleetHosts }} }, "id": "{{.PolicyID}}", + "secret_paths": ["inputs.0.secret_key"], "inputs": [ { "id": "fake-input", + "secret_key": "secretValue", "revision": 1, "name": "fake-input", "type": "fake-input", diff --git a/testing/fleetservertest/fleetserver_test.go b/testing/fleetservertest/fleetserver_test.go index 92f44f1c6b8..979104f6e78 100644 --- a/testing/fleetservertest/fleetserver_test.go +++ b/testing/fleetservertest/fleetserver_test.go @@ -271,6 +271,7 @@ func ExampleNewServer_checkin_fleetConnectionParams() { // "name": "fake-input", // "package_policy_id": "", // "revision": 1, + // "secret_key": "secretValue", // "streams": [], // "type": "fake-input", // "use_output": "default" @@ -287,6 +288,9 @@ func ExampleNewServer_checkin_fleetConnectionParams() { // } // }, // "revision": 2, + // "secret_paths": [ + // "inputs.0.secret_key" + // ], // "secret_references": [], // "signed": { // "data": "eyJpZCI6IjI0ZTRkMDMwLWZmYTctMTFlZC1iMDQwLTlkZWJhYTVmZWNiOCIsImFnZW50Ijp7InByb3RlY3Rpb24iOnsiZW5hYmxlZCI6ZmFsc2UsInVuaW5zdGFsbF90b2tlbl9oYXNoIjoibE9SU2FESVFxNG5nbFVNSndXaktyd2V4ajRJRFJKQStGdG9RZWVxS0gvST0iLCJzaWduaW5nX2tleSI6Ik1Ga3dFd1lIS29aSXpqMENBUVlJS29aSXpqMERBUWNEUWdBRVE5QlBvSFVDeUx5RWxWcGZ3dktlRmRVdDZVOXdCYitRbFpOZjRjeTVlQXdLOVhoNEQ4ZkNsZ2NpUGVSczNqNjJpNklFZUd5dk9zOVUzK2ZFbHlVaWdnPT0ifX19", diff --git a/testing/integration/inspect_test.go b/testing/integration/inspect_test.go new file mode 100644 index 00000000000..74866ff8460 --- /dev/null +++ b/testing/integration/inspect_test.go @@ -0,0 +1,91 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. + +//go:build integration + +package integration + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v2" + + 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" +) + +func TestInspect(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() + + 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) + + 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) + + p, err := fixture.Exec(ctx, []string{"inspect"}) + 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 { + SecretPaths []string `yaml:"secret_paths"` + Inputs []struct { + SecretKey string `yaml:"secret_key"` + } `yaml:"inputs"` + } + err = yaml.Unmarshal(p, &yObj) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"inputs.0.secret_key"}, yObj.SecretPaths) + require.Len(t, yObj.Inputs, 1) + assert.Equalf(t, "", yObj.Inputs[0].SecretKey, "inspect output: %s", p) +}