From 37b8ad0b1ff66120af5b8722174223f7843dd8b7 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Mon, 16 Sep 2024 11:07:30 -0700 Subject: [PATCH 01/11] Add secret_paths attributes to policies sent to agents Add a new attribute "secret_paths" to the policy data sent to agents. This attribute is a list of keys where the fleet-server has repalced a reference with a secret value. The agent is expected to redact the values of these keys when outputting policy data. --- Makefile | 1 + ...paths-list-to-policies-sent-to-agents.yaml | 34 ++++++ internal/pkg/api/handleCheckin.go | 1 + internal/pkg/api/openapi.gen.go | 3 + internal/pkg/policy/parsed_policy.go | 25 ++-- internal/pkg/policy/secret.go | 110 +++++++++++++----- internal/pkg/policy/secret_test.go | 31 +++-- model/openapi.yml | 5 + pkg/api/types.gen.go | 3 + 9 files changed, 160 insertions(+), 53 deletions(-) create mode 100644 changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml diff --git a/Makefile b/Makefile index b944cb30a..bd358e4ae 100644 --- a/Makefile +++ b/Makefile @@ -101,6 +101,7 @@ generate: ## - Generate schema models env GOBIN=${GOBIN} go install github.com/deepmap/oapi-codegen/v2/cmd/oapi-codegen@v2.0.0 @printf "${CMD_COLOR_ON} Running go generate\n${CMD_COLOR_OFF}" env PATH="${GOBIN}:${PATH}" go generate ./... + @$(MAKE) check-headers .PHONY: check-ci check-ci: ## - Run all checks of the ci without linting, the linter is run through github action to have comments in the pull-request. diff --git a/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml b/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml new file mode 100644 index 000000000..9d9181b9c --- /dev/null +++ b/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.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: feature + +# Change summary; a 80ish characters long description of the change. +summary: Add secret paths list to policies sent to agents + +# 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: | + Add a secret_paths attribute as part of the policy response data. This attribute is a list of keys where secret substitution has occured. + The agent should redact the values of these keys when outputting them. + +# 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: diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index 3677ee438..674935ae4 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -846,6 +846,7 @@ func processPolicy(ctx context.Context, zlog zerolog.Logger, bulker bulk.Bulk, a if err != nil { return nil, err } + d.SecretPaths = &pp.SecretKeys ad := Action_Data{} err = ad.FromActionPolicyChange(ActionPolicyChange{d}) if err != nil { diff --git a/internal/pkg/api/openapi.gen.go b/internal/pkg/api/openapi.gen.go index 438aa6339..349d5b275 100644 --- a/internal/pkg/api/openapi.gen.go +++ b/internal/pkg/api/openapi.gen.go @@ -630,6 +630,9 @@ type PolicyData struct { // Revision The revision number of the policy. Should match revision_idx. Revision *int `json:"revision,omitempty"` + // SecretPaths A list of keys that reference secret values that have been injected into the policy. + SecretPaths *[]string `json:"secret_paths,omitempty"` + // Signed Optional action signing data. Signed *ActionSignature `json:"signed,omitempty" yaml:"signed"` } diff --git a/internal/pkg/policy/parsed_policy.go b/internal/pkg/policy/parsed_policy.go index 8d0948079..2f79357e0 100644 --- a/internal/pkg/policy/parsed_policy.go +++ b/internal/pkg/policy/parsed_policy.go @@ -10,10 +10,11 @@ import ( "errors" "fmt" + "go.elastic.co/apm/v2" + "github.com/elastic/fleet-server/v7/internal/pkg/bulk" "github.com/elastic/fleet-server/v7/internal/pkg/model" "github.com/elastic/fleet-server/v7/internal/pkg/smap" - "go.elastic.co/apm/v2" ) const ( @@ -44,16 +45,18 @@ type ParsedPolicyDefaults struct { } type ParsedPolicy struct { - Policy model.Policy - Roles RoleMapT - Outputs map[string]Output - Default ParsedPolicyDefaults - Inputs []map[string]interface{} - Links apm.SpanLink + Policy model.Policy + Roles RoleMapT + Outputs map[string]Output + Default ParsedPolicyDefaults + Inputs []map[string]interface{} + SecretKeys []string + Links apm.SpanLink } func NewParsedPolicy(ctx context.Context, bulker bulk.Bulk, p model.Policy) (*ParsedPolicy, error) { var err error + secretKeys := make([]string, 0) // Interpret the output permissions if available var roles map[string]RoleT if roles, err = parsePerms(p.Data.OutputPermissions); err != nil { @@ -65,7 +68,7 @@ func NewParsedPolicy(ctx context.Context, bulker bulk.Bulk, p model.Policy) (*Pa return nil, err } for _, policyOutput := range p.Data.Outputs { - err := ProcessOutputSecret(ctx, policyOutput, bulker) + err := ProcessOutputSecret(ctx, policyOutput, bulker) // TODO: Do we want to add output secret paths to SecretKeys? if err != nil { return nil, err } @@ -74,10 +77,11 @@ func NewParsedPolicy(ctx context.Context, bulker bulk.Bulk, p model.Policy) (*Pa if err != nil { return nil, err } - policyInputs, err := getPolicyInputsWithSecrets(ctx, p.Data, bulker) + policyInputs, keys, err := getPolicyInputsWithSecrets(ctx, p.Data, bulker) if err != nil { return nil, err } + secretKeys = append(secretKeys, keys...) // We are cool and the gang pp := &ParsedPolicy{ @@ -87,7 +91,8 @@ func NewParsedPolicy(ctx context.Context, bulker bulk.Bulk, p model.Policy) (*Pa Default: ParsedPolicyDefaults{ Name: defaultName, }, - Inputs: policyInputs, + Inputs: policyInputs, + SecretKeys: secretKeys, } if trace := apm.TransactionFromContext(ctx); trace != nil { // Pass current transaction link (should be a monitor transaction) to caller (likely a client request). diff --git a/internal/pkg/policy/secret.go b/internal/pkg/policy/secret.go index 5c98d346e..c3fc3c79d 100644 --- a/internal/pkg/policy/secret.go +++ b/internal/pkg/policy/secret.go @@ -6,6 +6,7 @@ package policy import ( "context" + "fmt" "regexp" "strings" @@ -39,57 +40,99 @@ func getSecretValues(ctx context.Context, secretRefs []model.SecretReferencesIte // read inputs and secret_references from agent policy // replace values of secret refs in inputs and input streams properties -func getPolicyInputsWithSecrets(ctx context.Context, data *model.PolicyData, bulker bulk.Bulk) ([]map[string]interface{}, error) { +func getPolicyInputsWithSecrets(ctx context.Context, data *model.PolicyData, bulker bulk.Bulk) ([]map[string]interface{}, []string, error) { if len(data.Inputs) == 0 { - return nil, nil + return nil, nil, nil } if len(data.SecretReferences) == 0 { - return data.Inputs, nil + return data.Inputs, nil, nil } secretValues, err := getSecretValues(ctx, data.SecretReferences, bulker) if err != nil { - return nil, err + return nil, nil, err } result := make([]map[string]interface{}, 0) - for _, input := range data.Inputs { - newInput := make(map[string]interface{}) - for k, v := range input { - newInput[k] = replaceAnyRef(v, secretValues) + keys := make([]string, 0) + for i, input := range data.Inputs { + newInput, ks := replaceMapRef(input, secretValues) + for _, key := range ks { + keys = append(keys, fmt.Sprintf("inputs[%d].%s", i, key)) } result = append(result, newInput) } data.SecretReferences = nil - return result, nil + return result, keys, nil } -// replaceAnyRef is a generic approach to replacing any secret references in the passed item. -// It will go through any slices or maps and replace any secret references. -// -// go's generic parameters are not a good fit for rewriting this method as the typeswitch will not work. -func replaceAnyRef(ref any, secrets map[string]string) any { +// replaceMapRef replaces all nested secret values in the passed input and returns the resulting input along with a list of keys where inputs have been replaced. +func replaceMapRef(input map[string]any, secrets map[string]string) (map[string]any, []string) { + keys := make([]string, 0) + result := make(map[string]any, len(input)) var r any - switch val := ref.(type) { - case string: - r = replaceStringRef(val, secrets) - case map[string]any: - obj := make(map[string]any) - for k, v := range val { - obj[k] = replaceAnyRef(v, secrets) + + for k, v := range input { + switch value := v.(type) { + case string: + ref, replaced := replaceStringRef(value, secrets) + if replaced { + keys = append(keys, k) + } + r = ref + case map[string]any: + ref, ks := replaceMapRef(value, secrets) + for _, key := range ks { + keys = append(keys, k+"."+key) + } + r = ref + case []any: + ref, ks := replaceSliceRef(value, secrets) + for _, key := range ks { + keys = append(keys, k+key) + } + r = ref + default: + r = v } - r = obj - case []any: - arr := make([]any, len(val)) - for i, v := range val { - arr[i] = replaceAnyRef(v, secrets) + result[k] = r + } + return result, keys +} + +// replaceSliceRef replaces all nested secrets within the passed slice and returns the resulting slice along with a list of keys that indicate where values have been replaced. +func replaceSliceRef(arr []any, secrets map[string]string) ([]any, []string) { + keys := make([]string, 0) + result := make([]any, len(arr)) + var r any + + for i, v := range arr { + switch value := v.(type) { + case string: + ref, replaced := replaceStringRef(value, secrets) + if replaced { + keys = append(keys, fmt.Sprintf("[%d]", i)) + } + r = ref + case map[string]any: + ref, ks := replaceMapRef(value, secrets) + for _, key := range ks { + keys = append(keys, fmt.Sprintf("[%d].%s", i, key)) + } + r = ref + case []any: + ref, ks := replaceSliceRef(value, secrets) + for _, key := range ks { + keys = append(keys, fmt.Sprintf("[%d]%s", i, key)) + } + r = ref + default: + r = v } - r = arr - default: - r = val + result[i] = r } - return r + return result, keys } type OutputSecret struct { @@ -178,16 +221,19 @@ func ProcessOutputSecret(ctx context.Context, output smap.Map, bulker bulk.Bulk) // replaceStringRef replaces values matching a secret ref regex, e.g. $co.elastic.secret{} -> // and does this for multiple matches -func replaceStringRef(ref string, secretValues map[string]string) string { +// returns the resulting string value, and if any replacements were made +func replaceStringRef(ref string, secretValues map[string]string) (string, bool) { + hasReplaced := false matches := secretRegex.FindStringSubmatch(ref) for len(matches) > 1 { secretRef := matches[1] if val, ok := secretValues[secretRef]; ok { + hasReplaced = true ref = strings.Replace(ref, matches[0], val, 1) matches = secretRegex.FindStringSubmatch(ref) continue } break } - return ref + return ref, hasReplaced } diff --git a/internal/pkg/policy/secret_test.go b/internal/pkg/policy/secret_test.go index 8ae433ff3..08de63a3c 100644 --- a/internal/pkg/policy/secret_test.go +++ b/internal/pkg/policy/secret_test.go @@ -22,24 +22,27 @@ func TestReplaceStringRef(t *testing.T) { secretRefs := map[string]string{ "abcd": "value1", } - val := replaceStringRef("$co.elastic.secret{abcd}", secretRefs) + val, replaced := replaceStringRef("$co.elastic.secret{abcd}", secretRefs) assert.Equal(t, "value1", val) + assert.True(t, replaced) } func TestReplaceStringRefPartial(t *testing.T) { secretRefs := map[string]string{ "abcd": "value1", } - val := replaceStringRef("partial $co.elastic.secret{abcd}", secretRefs) + val, replaced := replaceStringRef("partial $co.elastic.secret{abcd}", secretRefs) assert.Equal(t, "partial value1", val) + assert.True(t, replaced) } func TestReplaceStringRefPartial2(t *testing.T) { secretRefs := map[string]string{ "abcd": "http://localhost", } - val := replaceStringRef("$co.elastic.secret{abcd}/services", secretRefs) + val, replaced := replaceStringRef("$co.elastic.secret{abcd}/services", secretRefs) assert.Equal(t, "http://localhost/services", val) + assert.True(t, replaced) } func TestReplaceStringRefMultiple(t *testing.T) { @@ -47,32 +50,36 @@ func TestReplaceStringRefMultiple(t *testing.T) { "secret1": "value1", "secret2": "value2", } - val := replaceStringRef("partial \"$co.elastic.secret{secret1}\" \"$co.elastic.secret{secret2}\"", secretRefs) + val, replaced := replaceStringRef("partial \"$co.elastic.secret{secret1}\" \"$co.elastic.secret{secret2}\"", secretRefs) assert.Equal(t, "partial \"value1\" \"value2\"", val) + assert.True(t, replaced) } func TestReplaceStringRefMultipleOneNotFound(t *testing.T) { secretRefs := map[string]string{ "secret2": "value2", } - val := replaceStringRef("partial \"$co.elastic.secret{secret1}\" \"$co.elastic.secret{secret2}\"", secretRefs) + val, replaced := replaceStringRef("partial \"$co.elastic.secret{secret1}\" \"$co.elastic.secret{secret2}\"", secretRefs) assert.Equal(t, "partial \"$co.elastic.secret{secret1}\" \"$co.elastic.secret{secret2}\"", val) + assert.False(t, replaced) } func TestReplaceStringRefNotASecret(t *testing.T) { secretRefs := map[string]string{ "abcd": "value1", } - val := replaceStringRef("abcd", secretRefs) + val, replaced := replaceStringRef("abcd", secretRefs) assert.Equal(t, "abcd", val) + assert.False(t, replaced) } func TestReplaceStringRefNotFound(t *testing.T) { secretRefs := map[string]string{ "abcd": "value1", } - val := replaceStringRef("$co.elastic.secret{other}", secretRefs) + val, replaced := replaceStringRef("$co.elastic.secret{other}", secretRefs) assert.Equal(t, "$co.elastic.secret{other}", val) + assert.False(t, replaced) } func TestGetSecretValues(t *testing.T) { @@ -119,9 +126,10 @@ func TestGetPolicyInputsWithSecretsAndStreams(t *testing.T) { {"id": "input2", "streams": []interface{}{expectedStream}}, } - result, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) + result, keys, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) assert.Equal(t, expectedResult, result) + assert.Equal(t, []string{"inputs[0].package_var_secret", "inputs[0].input_var_secret", "inputs[1].streams[0].package_var_secret", "inputs[1].streams[0].input_var_secret", "inputs[1].streams[0].stream_var_secret"}, keys) assert.Nil(t, pData.SecretReferences) } @@ -165,11 +173,11 @@ func TestPolicyInputSteamsEmbedded(t *testing.T) { }}, } - result, err := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) + result, keys, err := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) require.NoError(t, err) assert.Equal(t, expected, result) - + assert.Equal(t, []string{"inputs[0].streams[0].embedded.embedded-arr[0].embedded-secret"}, keys) } func TestGetPolicyInputsNoopWhenNoSecrets(t *testing.T) { @@ -193,9 +201,10 @@ func TestGetPolicyInputsNoopWhenNoSecrets(t *testing.T) { {"id": "input2", "streams": []interface{}{expectedStream}}, } - result, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) + result, keys, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) assert.Equal(t, expectedResult, result) + assert.Empty(t, keys) } func TestProcessOutputSecret(t *testing.T) { diff --git a/model/openapi.yml b/model/openapi.yml index cadd0f6b5..317863191 100644 --- a/model/openapi.yml +++ b/model/openapi.yml @@ -522,6 +522,11 @@ components: id: description: The policy's ID. type: string + secret_paths: + description: A list of keys that reference secret values that have been injected into the policy. + type: array + items: + type: string outputs: description: A map of all outputs that the agent running the policy can use to send data to. type: object diff --git a/pkg/api/types.gen.go b/pkg/api/types.gen.go index 8060d2128..40fc1a92e 100644 --- a/pkg/api/types.gen.go +++ b/pkg/api/types.gen.go @@ -627,6 +627,9 @@ type PolicyData struct { // Revision The revision number of the policy. Should match revision_idx. Revision *int `json:"revision,omitempty"` + // SecretPaths A list of keys that reference secret values that have been injected into the policy. + SecretPaths *[]string `json:"secret_paths,omitempty"` + // Signed Optional action signing data. Signed *ActionSignature `json:"signed,omitempty" yaml:"signed"` } From 47899c3c4d271494841eac68ccba6f5c165367f9 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Mon, 16 Sep 2024 11:29:54 -0700 Subject: [PATCH 02/11] verify secret paths in integration test --- ...909-Add-secret-paths-list-to-policies-sent-to-agents.yaml | 2 +- internal/pkg/server/fleet_secrets_integration_test.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml b/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml index 9d9181b9c..15e884d2e 100644 --- a/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.yaml +++ b/changelog/fragments/1726509909-Add-secret-paths-list-to-policies-sent-to-agents.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/fleet-server/pull/3908 # 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/server/fleet_secrets_integration_test.go b/internal/pkg/server/fleet_secrets_integration_test.go index e3daddc1f..b7b926dfe 100644 --- a/internal/pkg/server/fleet_secrets_integration_test.go +++ b/internal/pkg/server/fleet_secrets_integration_test.go @@ -17,12 +17,13 @@ import ( "testing" - "github.com/elastic/go-elasticsearch/v8" "github.com/gofrs/uuid" "github.com/hashicorp/go-cleanhttp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/elastic/go-elasticsearch/v8" + "github.com/elastic/fleet-server/v7/internal/pkg/api" "github.com/elastic/fleet-server/v7/internal/pkg/apikey" "github.com/elastic/fleet-server/v7/internal/pkg/bulk" @@ -230,4 +231,6 @@ func Test_Agent_Policy_Secrets(t *testing.T) { "package_var_secret": "secret_value", "type": "fleet-server", }, input) + // expect that secret_paths lists the key + assert.Equal(t, []string{"inputs[0].package_secret_value"}, *actionData.Policy.SecretPaths) } From ebcd1d3a8fdfafe6b4d076c19f7e15ae10bb30e3 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 17 Sep 2024 10:25:05 -0700 Subject: [PATCH 03/11] Add output secrets to secret_paths --- internal/pkg/api/handleCheckin.go | 2 +- internal/pkg/policy/parsed_policy.go | 7 +++++-- internal/pkg/policy/secret.go | 22 ++++++++++++++++------ internal/pkg/policy/secret_test.go | 9 +++++++-- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index 674935ae4..a780d47d7 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -819,7 +819,7 @@ func processPolicy(ctx context.Context, zlog zerolog.Logger, bulker bulk.Bulk, a data := model.ClonePolicyData(pp.Policy.Data) for policyName, policyOutput := range data.Outputs { - err := policy.ProcessOutputSecret(ctx, policyOutput, bulker) + _, err := policy.ProcessOutputSecret(ctx, policyOutput, bulker) // FIXME: Do we need to handle output keys here? if err != nil { return nil, fmt.Errorf("failed to process output secrets %q: %w", policyName, err) diff --git a/internal/pkg/policy/parsed_policy.go b/internal/pkg/policy/parsed_policy.go index 2f79357e0..8a9abd584 100644 --- a/internal/pkg/policy/parsed_policy.go +++ b/internal/pkg/policy/parsed_policy.go @@ -67,11 +67,14 @@ func NewParsedPolicy(ctx context.Context, bulker bulk.Bulk, p model.Policy) (*Pa if err != nil { return nil, err } - for _, policyOutput := range p.Data.Outputs { - err := ProcessOutputSecret(ctx, policyOutput, bulker) // TODO: Do we want to add output secret paths to SecretKeys? + for name, policyOutput := range p.Data.Outputs { + ks, err := ProcessOutputSecret(ctx, policyOutput, bulker) if err != nil { return nil, err } + for _, key := range ks { + secretKeys = append(secretKeys, "outputs."+name+"."+key) + } } defaultName, err := findDefaultOutputName(p.Data.Outputs) if err != nil { diff --git a/internal/pkg/policy/secret.go b/internal/pkg/policy/secret.go index c3fc3c79d..b9b773572 100644 --- a/internal/pkg/policy/secret.go +++ b/internal/pkg/policy/secret.go @@ -188,14 +188,15 @@ func setSecretPath(output smap.Map, secretValue string, secretPaths []string) er } // Read secret from output and mutate output with secret value -func ProcessOutputSecret(ctx context.Context, output smap.Map, bulker bulk.Bulk) error { +func ProcessOutputSecret(ctx context.Context, output smap.Map, bulker bulk.Bulk) ([]string, error) { secrets := output.GetMap(FieldOutputSecrets) delete(output, FieldOutputSecrets) secretReferences := make([]model.SecretReferencesItems, 0) outputSecrets, err := getSecretIDAndPath(secrets) + keys := make([]string, 0, len(outputSecrets)) if err != nil { - return err + return nil, err } for _, secret := range outputSecrets { @@ -204,19 +205,28 @@ func ProcessOutputSecret(ctx context.Context, output smap.Map, bulker bulk.Bulk) }) } if len(secretReferences) == 0 { - return nil + return nil, nil } secretValues, err := getSecretValues(ctx, secretReferences, bulker) if err != nil { - return err + return nil, err } for _, secret := range outputSecrets { + var key string + for _, p := range secret.Path { + if key == "" { + key = p + continue + } + key = key + "." + p + } + keys = append(keys, key) err = setSecretPath(output, secretValues[secret.ID], secret.Path) if err != nil { - return err + return nil, err } } - return nil + return keys, nil } // replaceStringRef replaces values matching a secret ref regex, e.g. $co.elastic.secret{} -> diff --git a/internal/pkg/policy/secret_test.go b/internal/pkg/policy/secret_test.go index 08de63a3c..b66385b11 100644 --- a/internal/pkg/policy/secret_test.go +++ b/internal/pkg/policy/secret_test.go @@ -212,11 +212,13 @@ func TestProcessOutputSecret(t *testing.T) { name string outputJSON string expectOutputJSON string + expectKeys []string }{ { name: "Output without secrets", outputJSON: `{"password": "test"}`, expectOutputJSON: `{"password": "test"}`, + expectKeys: nil, }, { name: "Output with secrets", @@ -228,6 +230,7 @@ func TestProcessOutputSecret(t *testing.T) { expectOutputJSON: `{ "password": "passwordid_value" }`, + expectKeys: []string{"password"}, }, { name: "Output with nested secrets", @@ -239,6 +242,7 @@ func TestProcessOutputSecret(t *testing.T) { expectOutputJSON: `{ "ssl": {"key": "sslkey_value"} }`, + expectKeys: []string{"ssl.key"}, }, { name: "Output with multiple secrets", @@ -250,6 +254,7 @@ func TestProcessOutputSecret(t *testing.T) { expectOutputJSON: `{ "ssl": {"key": "sslkey_value", "other": "sslother_value"} }`, + expectKeys: []string{"ssl.key", "ssl.other"}, }, } @@ -262,11 +267,11 @@ func TestProcessOutputSecret(t *testing.T) { expectOutput, err := smap.Parse([]byte(tc.expectOutputJSON)) assert.NoError(t, err) - err = ProcessOutputSecret(context.Background(), output, bulker) + keys, err := ProcessOutputSecret(context.Background(), output, bulker) assert.NoError(t, err) assert.Equal(t, expectOutput, output) - + assert.Equal(t, tc.expectKeys, keys) }) } } From ba13964397126effeeaea4f7ce2117b53d6ca821 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 19 Sep 2024 15:33:24 -0700 Subject: [PATCH 04/11] Change unit test assertion --- internal/pkg/api/handleCheckin.go | 4 ++-- internal/pkg/policy/secret_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index a780d47d7..45413b820 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -35,8 +35,8 @@ import ( "github.com/miolini/datacounter" "github.com/rs/zerolog" - "go.elastic.co/apm/module/apmhttp/v2" - "go.elastic.co/apm/v2" + apmhttp "go.elastic.co/apm/module/apmhttp/v2" + apm "go.elastic.co/apm/v2" ) var ( diff --git a/internal/pkg/policy/secret_test.go b/internal/pkg/policy/secret_test.go index b66385b11..8017bad4d 100644 --- a/internal/pkg/policy/secret_test.go +++ b/internal/pkg/policy/secret_test.go @@ -129,7 +129,7 @@ func TestGetPolicyInputsWithSecretsAndStreams(t *testing.T) { result, keys, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) assert.Equal(t, expectedResult, result) - assert.Equal(t, []string{"inputs[0].package_var_secret", "inputs[0].input_var_secret", "inputs[1].streams[0].package_var_secret", "inputs[1].streams[0].input_var_secret", "inputs[1].streams[0].stream_var_secret"}, keys) + assert.ElementsMatch(t, []string{"inputs[0].package_var_secret", "inputs[0].input_var_secret", "inputs[1].streams[0].package_var_secret", "inputs[1].streams[0].input_var_secret", "inputs[1].streams[0].stream_var_secret"}, keys) assert.Nil(t, pData.SecretReferences) } @@ -177,7 +177,7 @@ func TestPolicyInputSteamsEmbedded(t *testing.T) { require.NoError(t, err) assert.Equal(t, expected, result) - assert.Equal(t, []string{"inputs[0].streams[0].embedded.embedded-arr[0].embedded-secret"}, keys) + assert.ElementsMatch(t, []string{"inputs[0].streams[0].embedded.embedded-arr[0].embedded-secret"}, keys) } func TestGetPolicyInputsNoopWhenNoSecrets(t *testing.T) { @@ -271,7 +271,7 @@ func TestProcessOutputSecret(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expectOutput, output) - assert.Equal(t, tc.expectKeys, keys) + assert.ElementsMatch(t, tc.expectKeys, keys) }) } } From 492753fa7c4faa4879672af6d2b88e195f212571 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 19 Sep 2024 16:21:37 -0700 Subject: [PATCH 05/11] revert package import change, fix linter in other pr --- internal/pkg/api/handleCheckin.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index 45413b820..a780d47d7 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -35,8 +35,8 @@ import ( "github.com/miolini/datacounter" "github.com/rs/zerolog" - apmhttp "go.elastic.co/apm/module/apmhttp/v2" - apm "go.elastic.co/apm/v2" + "go.elastic.co/apm/module/apmhttp/v2" + "go.elastic.co/apm/v2" ) var ( From 39fff74d668ab83186d1fb1b5d98b1411082e7ff Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 20 Sep 2024 11:23:23 -0700 Subject: [PATCH 06/11] Fix integration inputs secret test --- internal/pkg/server/fleet_secrets_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/server/fleet_secrets_integration_test.go b/internal/pkg/server/fleet_secrets_integration_test.go index b7b926dfe..108d2388e 100644 --- a/internal/pkg/server/fleet_secrets_integration_test.go +++ b/internal/pkg/server/fleet_secrets_integration_test.go @@ -232,5 +232,5 @@ func Test_Agent_Policy_Secrets(t *testing.T) { "type": "fleet-server", }, input) // expect that secret_paths lists the key - assert.Equal(t, []string{"inputs[0].package_secret_value"}, *actionData.Policy.SecretPaths) + assert.Equal(t, []string{"inputs[0].package_var_secret"}, *actionData.Policy.SecretPaths) } From ff8b0c72574788b1dc42612090916fe12365855e Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 20 Sep 2024 13:04:32 -0700 Subject: [PATCH 07/11] Add output secret_paths to integration test --- .../server/fleet_secrets_integration_test.go | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/internal/pkg/server/fleet_secrets_integration_test.go b/internal/pkg/server/fleet_secrets_integration_test.go index 108d2388e..c0f7dddeb 100644 --- a/internal/pkg/server/fleet_secrets_integration_test.go +++ b/internal/pkg/server/fleet_secrets_integration_test.go @@ -36,7 +36,7 @@ type SecretResponse struct { ID string } -func createSecret(t *testing.T, ctx context.Context, bulker bulk.Bulk) string { +func createSecret(t *testing.T, ctx context.Context, bulker bulk.Bulk, value string) string { t.Log("Setup secret for fleet integration test") esClient := bulker.Client() @@ -56,7 +56,7 @@ func createSecret(t *testing.T, ctx context.Context, bulker bulk.Bulk) string { defer res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) - req, err = http.NewRequestWithContext(ctx, "POST", "/_fleet/secret/", bytes.NewBuffer([]byte("{\"value\":\"secret_value\"}"))) + req, err = http.NewRequestWithContext(ctx, "POST", "/_fleet/secret/", strings.NewReader(fmt.Sprintf(`{"value":%q}`, value))) req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") // kibana_test:changeme base64 encoded @@ -82,12 +82,15 @@ func createSecret(t *testing.T, ctx context.Context, bulker bulk.Bulk) string { return resp.ID } -func createAgentPolicyWithSecrets(t *testing.T, ctx context.Context, bulker bulk.Bulk, secretID string, secretRef string) string { +func createAgentPolicyWithSecrets(t *testing.T, ctx context.Context, bulker bulk.Bulk, secretID, secretRef, outputID string) string { policyID := uuid.Must(uuid.NewV4()).String() var policyData = model.PolicyData{ Outputs: map[string]map[string]interface{}{ "default": { "type": "elasticsearch", + "secrets": map[string]interface{}{ + "secret-key": map[string]interface{}{"id": outputID}, + }, }, }, OutputPermissions: json.RawMessage(`{"default":{}}`), @@ -162,12 +165,14 @@ func Test_Agent_Policy_Secrets(t *testing.T) { require.NoError(t, err) ctx = testlog.SetLogger(t).WithContext(ctx) + // create secret for use with output + outputSecretID := createSecret(t, ctx, srv.bulker, "output_secret_value") // create secret with kibana_system user - secretID := createSecret(t, ctx, srv.bulker) + secretID := createSecret(t, ctx, srv.bulker, "secret_value") secretRef := fmt.Sprintf("$co.elastic.secret{%s}", secretID) // create agent policy with secret reference - enrollKey := createAgentPolicyWithSecrets(t, ctx, srv.bulker, secretID, secretRef) + enrollKey := createAgentPolicyWithSecrets(t, ctx, srv.bulker, secretID, secretRef, outputSecretID) cli := cleanhttp.DefaultClient() // enroll an agent t.Log("Enroll an agent") @@ -231,6 +236,25 @@ func Test_Agent_Policy_Secrets(t *testing.T) { "package_var_secret": "secret_value", "type": "fleet-server", }, input) + + // expect output secret to be replaced + output := (*actionData.Policy.Outputs)["default"] + assert.Conditionf(t, func() bool { + mp, ok := output.(map[string]interface{}) + if !ok { + return false + } + v, ok := mp["secret-key"] + if !ok { + return false + } + s, ok := v.(string) + if !ok { + return false + } + return "output_secret_value" == s + }, "expected output to contain secret-key: output_secret_value, got %v", output) + assert.NotContains(t, output, "secrets") // expect that secret_paths lists the key - assert.Equal(t, []string{"inputs[0].package_var_secret"}, *actionData.Policy.SecretPaths) + assert.ElementsMatch(t, []string{"inputs[0].package_var_secret", "outputs.default.secret-key"}, *actionData.Policy.SecretPaths) } From a87d4ca10d1b0c5f78e879deec72e9af404c1b96 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 20 Sep 2024 13:34:53 -0700 Subject: [PATCH 08/11] collect and dedup output_paths when sending policy change actions --- internal/pkg/api/handleCheckin.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index 341d61ecc..3e59a041d 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -16,6 +16,7 @@ import ( "math/rand" "net/http" "reflect" + "slices" "sync" "time" @@ -818,11 +819,13 @@ func processPolicy(ctx context.Context, zlog zerolog.Logger, bulker bulk.Bulk, a data := model.ClonePolicyData(pp.Policy.Data) for policyName, policyOutput := range data.Outputs { - _, err := policy.ProcessOutputSecret(ctx, policyOutput, bulker) // FIXME: Do we need to handle output keys here? + // NOTE: Not sure if output secret keys collected here include new entries, but they are collected for completeness + ks, err := policy.ProcessOutputSecret(ctx, policyOutput, bulker) if err != nil { return nil, fmt.Errorf("failed to process output secrets %q: %w", policyName, err) } + pp.SecretKeys = append(pp.SecretKeys, ks...) } // Iterate through the policy outputs and prepare them for _, policyOutput := range pp.Outputs { @@ -845,7 +848,10 @@ func processPolicy(ctx context.Context, zlog zerolog.Logger, bulker bulk.Bulk, a if err != nil { return nil, err } - d.SecretPaths = &pp.SecretKeys + // remove duplicates from secretkeys + slices.Sort(pp.SecretKeys) + keys := slices.Compact(pp.SecretKeys) + d.SecretPaths = &keys ad := Action_Data{} err = ad.FromActionPolicyChange(ActionPolicyChange{d}) if err != nil { From 5cb53f74e5d7389a6e5516f9a572b85768be8dc9 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 20 Sep 2024 13:44:15 -0700 Subject: [PATCH 09/11] fix linter --- internal/pkg/server/fleet_secrets_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/server/fleet_secrets_integration_test.go b/internal/pkg/server/fleet_secrets_integration_test.go index c0f7dddeb..f6e037380 100644 --- a/internal/pkg/server/fleet_secrets_integration_test.go +++ b/internal/pkg/server/fleet_secrets_integration_test.go @@ -252,7 +252,7 @@ func Test_Agent_Policy_Secrets(t *testing.T) { if !ok { return false } - return "output_secret_value" == s + return s == "output_secret_value" }, "expected output to contain secret-key: output_secret_value, got %v", output) assert.NotContains(t, output, "secrets") // expect that secret_paths lists the key From 1a1fd0570d94019b8e7163e34d204dc6ee7d619b Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 27 Sep 2024 11:49:07 -0700 Subject: [PATCH 10/11] Change array index key format --- internal/pkg/policy/secret.go | 11 ++++++----- internal/pkg/policy/secret_test.go | 4 ++-- internal/pkg/server/fleet_secrets_integration_test.go | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/pkg/policy/secret.go b/internal/pkg/policy/secret.go index b9b773572..f0b87029c 100644 --- a/internal/pkg/policy/secret.go +++ b/internal/pkg/policy/secret.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "regexp" + "strconv" "strings" "github.com/elastic/fleet-server/v7/internal/pkg/bulk" @@ -59,7 +60,7 @@ func getPolicyInputsWithSecrets(ctx context.Context, data *model.PolicyData, bul for i, input := range data.Inputs { newInput, ks := replaceMapRef(input, secretValues) for _, key := range ks { - keys = append(keys, fmt.Sprintf("inputs[%d].%s", i, key)) + keys = append(keys, fmt.Sprintf("inputs."+strconv.Itoa(i)+"."+key)) } result = append(result, newInput) } @@ -90,7 +91,7 @@ func replaceMapRef(input map[string]any, secrets map[string]string) (map[string] case []any: ref, ks := replaceSliceRef(value, secrets) for _, key := range ks { - keys = append(keys, k+key) + keys = append(keys, k+"."+key) } r = ref default: @@ -112,19 +113,19 @@ func replaceSliceRef(arr []any, secrets map[string]string) ([]any, []string) { case string: ref, replaced := replaceStringRef(value, secrets) if replaced { - keys = append(keys, fmt.Sprintf("[%d]", i)) + keys = append(keys, strconv.Itoa(i)) } r = ref case map[string]any: ref, ks := replaceMapRef(value, secrets) for _, key := range ks { - keys = append(keys, fmt.Sprintf("[%d].%s", i, key)) + keys = append(keys, strconv.Itoa(i)+"."+key) } r = ref case []any: ref, ks := replaceSliceRef(value, secrets) for _, key := range ks { - keys = append(keys, fmt.Sprintf("[%d]%s", i, key)) + keys = append(keys, strconv.Itoa(i)+"."+key) } r = ref default: diff --git a/internal/pkg/policy/secret_test.go b/internal/pkg/policy/secret_test.go index 8017bad4d..aaab567f6 100644 --- a/internal/pkg/policy/secret_test.go +++ b/internal/pkg/policy/secret_test.go @@ -129,7 +129,7 @@ func TestGetPolicyInputsWithSecretsAndStreams(t *testing.T) { result, keys, _ := getPolicyInputsWithSecrets(context.TODO(), &pData, bulker) assert.Equal(t, expectedResult, result) - assert.ElementsMatch(t, []string{"inputs[0].package_var_secret", "inputs[0].input_var_secret", "inputs[1].streams[0].package_var_secret", "inputs[1].streams[0].input_var_secret", "inputs[1].streams[0].stream_var_secret"}, keys) + assert.ElementsMatch(t, []string{"inputs.0.package_var_secret", "inputs.0.input_var_secret", "inputs.1.streams.0.package_var_secret", "inputs.1.streams.0.input_var_secret", "inputs.1.streams.0.stream_var_secret"}, keys) assert.Nil(t, pData.SecretReferences) } @@ -177,7 +177,7 @@ func TestPolicyInputSteamsEmbedded(t *testing.T) { require.NoError(t, err) assert.Equal(t, expected, result) - assert.ElementsMatch(t, []string{"inputs[0].streams[0].embedded.embedded-arr[0].embedded-secret"}, keys) + assert.ElementsMatch(t, []string{"inputs.0.streams.0.embedded.embedded-arr.0.embedded-secret"}, keys) } func TestGetPolicyInputsNoopWhenNoSecrets(t *testing.T) { diff --git a/internal/pkg/server/fleet_secrets_integration_test.go b/internal/pkg/server/fleet_secrets_integration_test.go index f6e037380..a156f0549 100644 --- a/internal/pkg/server/fleet_secrets_integration_test.go +++ b/internal/pkg/server/fleet_secrets_integration_test.go @@ -256,5 +256,5 @@ func Test_Agent_Policy_Secrets(t *testing.T) { }, "expected output to contain secret-key: output_secret_value, got %v", output) assert.NotContains(t, output, "secrets") // expect that secret_paths lists the key - assert.ElementsMatch(t, []string{"inputs[0].package_var_secret", "outputs.default.secret-key"}, *actionData.Policy.SecretPaths) + assert.ElementsMatch(t, []string{"inputs.0.package_var_secret", "outputs.default.secret-key"}, *actionData.Policy.SecretPaths) } From 72394e6c4e74862738afa62c65da96b574d68a04 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 27 Sep 2024 14:47:04 -0700 Subject: [PATCH 11/11] fix linter --- internal/pkg/policy/secret.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/pkg/policy/secret.go b/internal/pkg/policy/secret.go index f0b87029c..7d96cc070 100644 --- a/internal/pkg/policy/secret.go +++ b/internal/pkg/policy/secret.go @@ -6,7 +6,6 @@ package policy import ( "context" - "fmt" "regexp" "strconv" "strings" @@ -60,7 +59,7 @@ func getPolicyInputsWithSecrets(ctx context.Context, data *model.PolicyData, bul for i, input := range data.Inputs { newInput, ks := replaceMapRef(input, secretValues) for _, key := range ks { - keys = append(keys, fmt.Sprintf("inputs."+strconv.Itoa(i)+"."+key)) + keys = append(keys, "inputs."+strconv.Itoa(i)+"."+key) } result = append(result, newInput) }