replace secret refs #2863

juliaElastic · 2023-08-07T11:36:25Z

What is the problem this PR solves?

Agent policy secrets are sent to agents in agent policy.

How does this PR solve the problem?

On checkin, read secret values of secret references in the agent policy, and replace occurrences in input and stream variables.

How to test this PR locally

Start local es, kibana, fleet server
Enable secrets feature flag in kibana.dev.yml: xpack.fleet.enableExperimental: ['secretsStorage']
Start local registry with secret package here: [Fleet] View & Edit Secret Variables kibana#157176
Open kibana, add integration: secret package, add values for package, input and stream secret vars
When the agent policy with the secret integration is created, the agent policy should have 3 secret references
Enroll an agent locally
Download diagnostics and check in computed-config.yml that the inputs contain the secret values, not references
Note: the agent is unhealthy with the error message input not supported is not related to secrets, looks like a validation changed on agent side, so that the test package doesn't have a valid input type. Can be ignored for now.

Example:

inputs:
- data_stream:
    namespace: default
  id: test_input-secrets-7c6b08d8-dbe5-4278-8929-dfb26ea8e579
  input_var_secret: input
  meta:
    package:
      name: secrets
      version: 1.0.0
  name: secrets-1
  package_policy_id: 7c6b08d8-dbe5-4278-8929-dfb26ea8e579
  package_var_secret: pkg
  policy:
    revision: 5
  revision: 1
  streams:
  - config.version: "2"
    data_stream:
      dataset: secrets.log
      type: logs
    id: test_input-secrets.log-7c6b08d8-dbe5-4278-8929-dfb26ea8e579
    input_var_secret: input
    package_var_secret: pkg
    stream_var_secret: stream
  type: test_input

Design Checklist

I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer.
I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected.
I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.

Checklist

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

Related issues

Closes [Agent policy secrets] Retrieve agent policy secrets from .fleet-secrets index and replace secret references with secret values #2485

juliaElastic · 2023-08-07T11:37:14Z

internal/pkg/dl/secret.go

Following the example here: https://github.com/elastic/go-elasticsearch/blob/main/_examples/extension/main.go

internal/pkg/dl/secret.go

internal/pkg/policy/parsed_policy.go

juliaElastic · 2023-08-07T11:41:06Z

internal/pkg/policy/parsed_policy.go

+	return result, nil
+}
+
+func getPolicyInputsWithSecrets(inputsRaw json.RawMessage, secretReferences map[string]string) ([]map[string]interface{}, error) {


Is it possible to define a type in Go where some fields are known (streams) and some are dynamic (e.g. package_var_secret)?
Example inputs and secret refs:

"inputs": [ { "id": "test_input-secrets-7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "revision": 1, "name": "secrets-1", "type": "test_input", "data_stream": { "namespace": "default" }, "use_output": "default", "package_policy_id": "7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "package_var_secret": "$co.elastic.secret{2LnzzokBsuDBJ5hx_Zo_}", "input_var_secret": "$co.elastic.secret{2bnzzokBsuDBJ5hx_Zo_}", "streams": [ { "id": "test_input-secrets.log-7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "data_stream": { "type": "logs", "dataset": "secrets.log" }, "config.version": "2", "package_var_secret": "$co.elastic.secret{2LnzzokBsuDBJ5hx_Zo_}", "input_var_secret": "$co.elastic.secret{2bnzzokBsuDBJ5hx_Zo_}", "stream_var_secret": "$co.elastic.secret{2rnzzokBsuDBJ5hx_Zo_}" } ], "meta": { "package": { "name": "secrets", "version": "1.0.0" } } } ], "secret_references": [ { "id": "2LnzzokBsuDBJ5hx_Zo_" }, { "id": "2bnzzokBsuDBJ5hx_Zo_" }, { "id": "2rnzzokBsuDBJ5hx_Zo_" } ],

Example of replaced values:

"inputs": [ { "data_stream": { "namespace": "default" }, "id": "test_input-secrets-7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "input_var_secret": "input", "meta": { "package": { "name": "secrets", "version": "1.0.0" } }, "name": "secrets-1", "package_policy_id": "7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "package_var_secret": "pkg", "revision": 1, "streams": [ { "config.version": "2", "data_stream": { "dataset": "secrets.log", "type": "logs" }, "id": "test_input-secrets.log-7c6b08d8-dbe5-4278-8929-dfb26ea8e579", "input_var_secret": "input", "package_var_secret": "pkg", "stream_var_secret": "stream" } ], "type": "test_input", "use_output": "default" } ]

internal/pkg/policy/parsed_policy.go

elasticmachine · 2023-08-07T11:50:13Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-08-10T11:48:29.527+0000
Duration: 37 min 5 sec

Test stats 🧪

Test	Results
Failed	0
Passed	751
Skipped	1
Total	752

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

juliaElastic · 2023-08-08T15:42:45Z

I'm planning to add an integration or e2e test for this, though not sure how to create a secret as it requires privileges that only kibana_system user has. I would try to create a kibana user, unless someone has a better idea.

internal/pkg/policy/secret.go

joshdover · 2023-08-09T12:20:23Z

internal/pkg/policy/monitor.go

 	if len(policies) == 0 {
 		return nil
 	}

 	latest := m.groupByLatest(policies)
 	for _, policy := range latest {
-		pp, err := NewParsedPolicy(policy)
+		pp, err := NewParsedPolicy(ctx, m.bulker, policy)


Ah we get policies through the monitor - so I think this will be shared with all agents current subscribed to checkin. That should minimize the amount of re-fetching we have to do here.

internal/pkg/policy/secret.go

internal/pkg/server/fleet_secrets_integration_test.go

Co-authored-by: Josh Dover <[email protected]>

joshdover

LGTM thanks for the e2e test!

juliaElastic · 2023-08-10T06:53:33Z

I'm seeing some flakyness with integration tests using fake checkin request:

FAIL: TestServerInstrumentation (6.16s) | 2s
-- | --
  | === RUN   Test_SmokeTest_Agent_Calls
  | fleet_integration_test.go:598: Enroll an agent
  | fleet_integration_test.go:608: Agent enrollment successful, verify body
  | fleet_integration_test.go:633: Fake a checkin for agent d56ff08b-bad3-452d-8bb5-f012c5e90127
  | fleet_integration_test.go:640:
  | Error Trace:	/opt/buildkite-agent/builds/bk-agent-prod-gcp-1691594516224871631/elastic/fleet-server/internal/pkg/server/fleet_integration_test.go:640
  | Error:      	Received unexpected error:
  | Post "http://localhost:46833/api/fleet/agents/d56ff08b-bad3-452d-8bb5-f012c5e90127/checkin": EOF

juliaElastic · 2023-08-10T11:04:14Z

The integration tests all pass now, but there is still an issue that I don't know how to fix:

--- PASS: TestCheckCompatibilityInternal/supported_elasticsearch_715-800a1 (0.00s)
--
  | PASS
  | ok  	github.com/elastic/fleet-server/v7/internal/pkg/ver	0.062s
  | ?   	github.com/elastic/fleet-server/v7/pkg/api	[no test files]
  | ?   	github.com/elastic/fleet-server/v7/pkg/api/versions/2022_06_01/api	[no test files]
  | ?   	github.com/elastic/fleet-server/v7/version	[no test files]
  | FAIL
  | make[1]: *** [Makefile:328: test-int-set] Error 1
  | make[1]: Leaving directory '/opt/buildkite-agent/builds/bk-agent-prod-gcp-1691664493893470492/elastic/fleet-server'
  | make: *** [Makefile:317: test-int] Error 2
  | 🚨 Error: The command exited with status 2
  | user command error: exit status 2

Line 328 is this:

fleet-server/Makefile

Line 328 in 4834215

    
           ELASTICSEARCH_SERVICE_TOKEN=$(shell ./dev-tools/integration/get-elasticsearch-servicetoken.sh ${ELASTICSEARCH_USERNAME}:${ELASTICSEARCH_PASSWORD}@${TEST_ELASTICSEARCH_HOSTS}) \

Trying to revert the last 3 commits as I saw a passing built at that point.

joshdover · 2023-08-10T11:33:29Z

Did the last commit fix? Seems like it should. Otherwise I can dig in to help

juliaElastic · 2023-08-10T11:38:24Z

Did the last commit fix? Seems like it should. Otherwise I can dig in to help

I had a passing build before this commit, which excludes some unit tests from the integration tests run, not sure if this is flakyness, or something breaks when I exclude those.

I think I got it, it seems that an integration test referenced a variable from a unit test and it was missing when I excluded the unit test.

# github.com/elastic/fleet-server/v7/internal/pkg/policy [github.com/elastic/fleet-server/v7/internal/pkg/policy.test]
--
  | internal/pkg/policy/policy_output_integration_test.go:146:10: undefined: TestPayload
  | FAIL	github.com/elastic/fleet-server/v7/internal/pkg/policy [build failed]

juliaElastic · 2023-08-10T12:00:57Z

internal/pkg/policy/secret_test.go

+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build !integration


I don't understand though we we have to exclude unit tests like this explicitly, as integration tests are tagged also.
Are we missing something?
the Makefile has this command to tigger integration tests:
go test -v -tags=integration -count=1 -race -p 1 ./...

I guess that the idea is to don't run unit tests when running integration tests, so the same tests are not executed at two different stages of testing.

Yes, I mean shouldn't it be enough to mark integration tests with //go:build integration? I think it's easy to forget when adding new unit tests to add the negated condition, and I'm trying to understand why it is needed.

Files without any build tag are always included, so -tags=integration wouldn't discard unit test files if they don't have the negated tag. So we need !integration for test files that we don't want to execute during integration tests.

I agree that this is easy to forget. Tough I wouldn't see as a great problem to execute some fast unit tests during integration test execution.

If we strongly want to have their execution separated, a possible strategy to don't forget it is to have only unit tests in packages, and move the integration tests to their own package/s, in a similar fashion to what we do for e2e tests. This also help to validate interfaces.
Or we could have some check in CI to ensure that all test files have tags.

Thanks for the explanation!

replace secret refs

1d2a1d5

juliaElastic added the enhancement New feature or request label Aug 7, 2023

juliaElastic self-assigned this Aug 7, 2023

juliaElastic commented Aug 7, 2023

View reviewed changes

internal/pkg/dl/secret.go Outdated Show resolved Hide resolved

juliaElastic commented Aug 7, 2023

View reviewed changes

internal/pkg/policy/parsed_policy.go Outdated Show resolved Hide resolved

juliaElastic commented Aug 7, 2023

View reviewed changes

internal/pkg/policy/parsed_policy.go Outdated Show resolved Hide resolved

juliaElastic requested a review from michel-laterman August 7, 2023 11:42

juliaElastic requested review from hop-dev and jillguyonnet August 7, 2023 12:37

juliaElastic and others added 6 commits August 7, 2023 15:06

fix check, remove TODO

a58d86b

moved ReadSecrets to bulker, tests

3e988f3

fix lint, more tests

cd41303

Merge branch 'main' into secret-refs

50c2359

added nil check

be2c079

added changelog

af0523c

juliaElastic marked this pull request as ready for review August 8, 2023 11:03

juliaElastic requested a review from a team as a code owner August 8, 2023 11:03

juliaElastic added 2 commits August 8, 2023 13:45

fix nil check

2f1c3d3

added comments

75abca5

joshdover reviewed Aug 9, 2023

View reviewed changes

added integration test on secrets

90c6814

juliaElastic commented Aug 9, 2023

View reviewed changes

internal/pkg/server/fleet_secrets_integration_test.go Outdated Show resolved Hide resolved

juliaElastic and others added 4 commits August 9, 2023 15:22

Update internal/pkg/policy/secret.go

077cd6e

Co-authored-by: Josh Dover <[email protected]>

Update internal/pkg/policy/secret.go

4c7d246

Co-authored-by: Josh Dover <[email protected]>

rename to getSecretValues, moved out regex

1c699ec

refactored getPolicyInputsWithSecrets

af4346e

juliaElastic added 2 commits August 9, 2023 15:54

added kibana_system to kibana.yml

5105216

create kibana user

918592a

juliaElastic requested a review from joshdover August 9, 2023 15:23

joshdover approved these changes Aug 9, 2023

View reviewed changes

juliaElastic force-pushed the secret-refs branch from 49595c5 to 918592a Compare August 10, 2023 11:06

fixed integration test

ca9ec09

juliaElastic force-pushed the secret-refs branch from fa41fef to ca9ec09 Compare August 10, 2023 11:48

juliaElastic commented Aug 10, 2023

View reviewed changes

juliaElastic merged commit 32b7910 into elastic:main Aug 10, 2023

juliaElastic mentioned this pull request Nov 7, 2023

Replace secret vars for arrays #3083

Closed

romulets mentioned this pull request Nov 30, 2023

Adopt Secrets in Cloud Posture Management elastic/integrations#8604

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace secret refs #2863

replace secret refs #2863

juliaElastic commented Aug 7, 2023 •

edited

Loading

juliaElastic Aug 7, 2023

juliaElastic Aug 7, 2023

elasticmachine commented Aug 7, 2023 •

edited

Loading

Build stats

Test stats 🧪

juliaElastic commented Aug 8, 2023 •

edited

Loading

joshdover Aug 9, 2023

joshdover left a comment

juliaElastic commented Aug 10, 2023

juliaElastic commented Aug 10, 2023 •

edited

Loading

joshdover commented Aug 10, 2023

juliaElastic commented Aug 10, 2023 •

edited

Loading

juliaElastic Aug 10, 2023

jsoriano Aug 10, 2023

juliaElastic Aug 10, 2023

jsoriano Aug 10, 2023

juliaElastic Aug 10, 2023

replace secret refs #2863

replace secret refs #2863

Conversation

juliaElastic commented Aug 7, 2023 • edited Loading

What is the problem this PR solves?

How does this PR solve the problem?

How to test this PR locally

Design Checklist

Checklist

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 7, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

juliaElastic commented Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

juliaElastic commented Aug 10, 2023

juliaElastic commented Aug 10, 2023 • edited Loading

joshdover commented Aug 10, 2023

juliaElastic commented Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaElastic commented Aug 7, 2023 •

edited

Loading

elasticmachine commented Aug 7, 2023 •

edited

Loading

juliaElastic commented Aug 8, 2023 •

edited

Loading

juliaElastic commented Aug 10, 2023 •

edited

Loading

juliaElastic commented Aug 10, 2023 •

edited

Loading