[Fleet] View & Edit Secret Variables #157176

hop-dev · 2023-05-09T14:49:43Z

Summary

Related to #154715

Adds a new component for viewing and editing secret values in the policy editor. Secrets can be written on policy create, when editing a policy, if a secret is filled then we give the user the option to replace the value only.

Also when viewing the agent policy yaml, hide secret references and replace them with incremental env var names, and add a callout which tells the user they need to replace the vars.

I have kept the UI un-opinionated about which type of field can be a secret.

Secret variable in package policy editor:

secret_input_demo.mp4

Replaced secrets and new callout in policy yaml viewer:

policy_yaml_demo.mp4

Testing

start a docker package registry

docker run -p 8080:8080 -v /Users/markhopkin/dev/kibana/x-pack/test/fleet_api_integration/apis/fixtures/test_packages:/packages/test-packages -v /Users/markhopkin/dev/kibana/x-pack/test/fleet_api_integration/apis/fixtures/package_registry_config.yml:/package-registry/config.yml docker.elastic.co/package-registry/package-registry:main

point your kibana to the registry and use the secrets-1.0.0 package to test
create a package policy with secrets (should be successful, should show 'normal' input components)
edit a package policy with secrets (should be successful, should show "replace value" panel for filled values)
view agent policy yaml with secrets (should not show secret references in yaml, should show ${SECRET_0} etc)

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

apmmachine · 2023-05-09T14:49:59Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-05-16T15:01:44Z

Pinging @elastic/fleet (Team:Fleet)

juliaElastic · 2023-05-17T11:17:07Z

x-pack/plugins/fleet/common/services/full_agent_policy_to_yaml.ts

+
+  secretIds.forEach((secretId, idx) => {
+    const regex = new RegExp(`\\$co\\.elastic\\.secret\\{${secretId}\\}`, 'g');
+    formattedText = formattedText.replace(regex, `\${SECRET_${idx}}`);


It is not possible to use the same secretId multiple times in one policy, right? Asking because replace only replaces the first match.

This looks fine from what I can see: the ids are auto generated so they should at least be unique for a given package policy. This is stated in the proposal doc but can also be seen in the secrets service here.

juliaElastic · 2023-05-17T11:19:02Z

x-pack/plugins/fleet/common/services/full_agent_policy_to_yaml.test.ts

+    const yaml = fullAgentPolicyToYaml(agentPolicyWithSecrets, (policy) => JSON.stringify(policy));
+
+    expect(yaml).toMatchInlineSnapshot(
+      `"{\\"id\\":\\"1234\\",\\"outputs\\":{\\"default\\":{\\"type\\":\\"elasticsearch\\",\\"hosts\\":[\\"http://localhost:9200\\"]}},\\"inputs\\":[{\\"id\\":\\"test_input-secrets-abcd1234\\",\\"revision\\":1,\\"name\\":\\"secrets-1\\",\\"type\\":\\"test_input\\",\\"data_stream\\":{\\"namespace\\":\\"default\\"},\\"use_output\\":\\"default\\",\\"package_policy_id\\":\\"abcd1234\\",\\"package_var_secret\\":\\"\${SECRET_0}\\",\\"input_var_secret\\":\\"\${SECRET_1}\\",\\"streams\\":[{\\"id\\":\\"test_input-secrets.log-abcd1234\\",\\"data_stream\\":{\\"type\\":\\"logs\\",\\"dataset\\":\\"secrets.log\\"},\\"package_var_secret\\":\\"\${SECRET_0}\\",\\"input_var_secret\\":\\"\${SECRET_1}\\",\\"stream_var_secret\\":\\"\${SECRET_2}\\"}],\\"meta\\":{\\"package\\":{\\"name\\":\\"secrets\\",\\"version\\":\\"1.0.0\\"}}}],\\"secret_references\\":[{\\"id\\":\\"secret-id-1\\"},{\\"id\\":\\"secret-id-2\\"},{\\"id\\":\\"secret-id-3\\"}],\\"revision\\":2,\\"agent\\":{},\\"signed\\":{},\\"output_permissions\\":{},\\"fleet\\":{}}"`


If this policy was used with envvars to provide SECRET_x values, it shouldn't cause any problems that the policy has secret_references. We should test to make sure.

juliaElastic · 2023-05-17T11:25:53Z

...leet/public/applications/fleet/sections/agent_policy/components/agent_policy_yaml_flyout.tsx

+              >
+                <FormattedMessage
+                  id="xpack.fleet.policyDetails.secretsDescription"
+                  defaultMessage="Kibana does not have access to secret values. You will need to set these values manually after deploying the agent policy. Look out for environment variables in the format {envVarPrefix} in the agent configuration."


@nimarezainia Could you confirm the copy of the callout?

cc @zombieFox as well for input. Could we provide a screenshot here with context as well to help us confirm the copy?

See the second recording in the description.

I think the goal here is to be able to easily use the policy in a standalone config by providing envvars for secrets e.g. SECRET_0.

I don't have a screenshot of the Callout. I believe Julia is correct, it can be seen in the video demo.

i think the copy is very descriptive.

juliaElastic · 2023-05-17T11:28:11Z

Reviewed the code and looks good, added a few comments.

juliaElastic

LGTM

juliaElastic · 2023-06-21T13:22:45Z

Testing locally and noticed that the Edit integration policy page shows the secrets as hidden even when the feature flag is turned off (so secrets are still part of the policy doc).
I am wondering if the UI should go back to displaying the secrets if the feature flag is off to avoid confusion (e.g. a package containing secrets is released earlier than the secrets feature).

juliaElastic · 2023-06-21T13:26:13Z

@elasticmachine merge upstream

juliaElastic · 2023-06-21T13:29:17Z

Also noticed a bug on enabling the feature flag:

Add a "secrets" integration with feature flag turned off, fill out secret var values
Enable secrets feature flag xpack.fleet.enableExperimental: ['secretsStorage']
Edit "secrets" integration policy and change the values of secret vars
Save the integration
Getting an error, I think the code doesn't expect secret references to be empty on update

[2023-06-21T15:23:50.849+02:00][ERROR][plugins.fleet] KQLSyntaxError: Expected "(", NOT, value but ")" found.
ingest-package-policies.attributes.secret_references.id: ( or  or )
------------------------------------------------------------------^: Bad Request

juliaElastic · 2023-06-21T13:43:21Z

Creating a new integration policy after the feature flag is enabled looks good, I can see the replaced values in the Agent policy:

I could update the values as well in Edit integration policy without seeing the original values.

juliaElastic

Added a few comments, which should be addressed, I think.

jillguyonnet · 2023-06-21T15:44:54Z

Good point about the desired behaviour when the feature flag is turned off.

Also, I'm confused by something: when I edit the package policy and click Replace package var secret, I can still view the old value (as opposed to Mark's video above that shows the field is empty). I'd have to dig into it to see why, but doesn't seem right.

juliaElastic · 2023-06-21T15:53:17Z

Was the package policy created with feature flag off? If so, it is expected to see the previous value as the secrets were not stored separately.
I tested with the feature flag on to create and update, and I didn't see the secret when updating.

jillguyonnet · 2023-06-22T14:00:01Z

@nimarezainia Could we get your input on the expected behaviour when the feature flag is disabled per Julia's comment above? In the current implementation, if a package provides a secret value and the flag is disabled, when the user edits the package policy the UI will show a secret field type like in the screenshot below. Furthermore, editing the variable will result in the old value being visible (second screenshot).
cc @jlind23

jillguyonnet · 2023-06-22T14:44:49Z

@elasticmachine merge upstream

jlind23 · 2023-06-22T15:41:07Z

@jillguyonnet I would say:

If the secret was created when the policy had the flag enabled, the secret should remain secret.
If the secret was created before the policy had the flag enabled, the secret will be shown.

Which means that all secrets created before this option will never be completely hidden.

nimarezainia · 2023-06-23T06:14:13Z

@nimarezainia Could we get your input on the expected behaviour when the feature flag is disabled per Julia's comment above? In the current implementation, if a package provides a secret value and the flag is disabled, when the user edits the package policy the UI will show a secret field type like in the screenshot below. Furthermore, editing the variable will result in the old value being visible (second screenshot). cc @jlind23

@jillguyonnet if I understand this correctly, I would say that when the feature flag is disabled, the edit policy page should behave as it does today without these changes. Feature flag is off be default. So a user who doesn't want this feature (or is unaware of it) shouldn't see a change in behavior.

jillguyonnet · 2023-06-26T09:43:04Z

@elasticmachine merge upstream

kibana-ci · 2023-06-26T10:46:50Z

💚 Build Succeeded

Buildkite Build
Commit: 5b0ce62

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`fleet`	974.1KB	977.4KB	+3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	130.9KB	131.2KB	+301.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	416	420	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	15	17	+2
`securitySolution`	497	501	+4
total			+6

History

💔 Build #137350 failed 943bbba
💔 Build #136935 failed ad1f514
💛 Build #136145 was flaky 62d9ee9
💛 Build #128255 was flaky 140a86f
💚 Build #128206 succeeded bc7d2a2fc8f96ddddd586ace5968a51d5b95fcd7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jillguyonnet

jillguyonnet · 2023-06-26T13:57:55Z

@juliaElastic Here's a summary of the remaining questions:

👍 UI when with secretsStorage feature flag disabled: I pushed a commit so that the input field only renders with the Replace package var secret change when the flag is enabled. So the user will get the same behaviour as today, per @nimarezainia 's comment above.

👍 When a secret value was created with the flag disabled and the flag was then enabled at a later stage, in the current implementation the first time the user edits this input the old value will be shown, and in subsequent changes it will be hidden (@jlind23 's comment above).

⚠️ On the bug you reported: I can reproduce it, note that it consistently happens the first time we try to save the policy but on the second try (just click the button again) it is saved succesfully. The error originates here so something is wrong with the initial filter query param. Error stack trace and message:

KQLSyntaxError: Expected "(", NOT, value, whitespace but ")" found.
ingest-package-policies.attributes.secret_references.id: ()
----------------------------------------------------------^
    at Object.fromKueryExpression (ast.ts:52:13)
    at validateConvertFilterToKueryNode (filter_utils.ts:26:44)
    at performFind (find.ts:124:51)
    at SavedObjectsRepository.find (repository.ts:321:29)
    at SavedObjectsClient.find (saved_objects_client.ts:102:35)
    at PackagePolicyClientImpl.list (package_policy.ts:619:44)
    at findPackagePoliciesUsingSecrets (secrets.ts:152:54)
    at deleteSecretsIfNotReferenced (secrets.ts:115:45)
    at PackagePolicyClientImpl.update (package_policy.ts:841:22)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at updatePackagePolicyHandler (handlers.ts:366:34)
    at Router.handle (router.ts:206:30)
    at handler (router.ts:161:13)
    at exports.Manager.execute (/Users/jillguyonnet/Code/jillguyonnet/kibana/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
    at Object.internals.handler (/Users/jillguyonnet/Code/jillguyonnet/kibana/node_modules/@hapi/hapi/lib/handler.js:46:20)
    at exports.execute (/Users/jillguyonnet/Code/jillguyonnet/kibana/node_modules/@hapi/hapi/lib/handler.js:31:20)
    at Request._lifecycle (/Users/jillguyonnet/Code/jillguyonnet/kibana/node_modules/@hapi/hapi/lib/request.js:371:32)
    at Request._execute (/Users/jillguyonnet/Code/jillguyonnet/kibana/node_modules/@hapi/hapi/lib/request.js:281:9)

[2023-06-26T15:38:20.299+02:00][ERROR][plugins.fleet] KQLSyntaxError: Expected "(", NOT, value, whitespace but ")" found.
ingest-package-policies.attributes.secret_references.id: ()
----------------------------------------------------------^: Bad Request

I'm not sure yet how this could be addressed, but we could leave that as a followup. WDYT?

juliaElastic · 2023-06-26T14:00:35Z

Thanks for the changes! I'm ok to merge and leave the bugfix to a follow up.

jillguyonnet · 2023-06-26T14:12:05Z

Sounds good, I also looked up your question about the uniqueness of secret id per package policy and I think we're good. I'll merge and open a followup issue for the bugfix.

jillguyonnet · 2023-06-26T15:01:06Z

Followup bugfix issue: #160537

…ets index (#163075) Closes #162915 ## Summary Replace direct calls to Fleet Secrets index with new API calls introduced with elastic/elasticsearch#97728 ### New ES secrets APIs: ``` POST /_fleet/secret/ { "value": "<secret value>" } // Returns the id of the created secret { "id": "<secret_id>" } DELETE /_fleet/secret/<secret_id> // returns { "deleted": true } ``` NOTE: I tried running the secrets integration tests in #162732 but there is some ES error that I'm not sure how to address. I think that the test can be worked on separately ### Testing Testing steps are the exact same as #157176: - Start EPR locally loading the `Secrets` test package from Kibana: ``` docker run -p 8080:8080 -v /Users/<YOUR_PATH>/kibana/x-pack/test/fleet_api_integration/apis/fixtures/test_packages:/packages/test-packages -v /Users/<YOUR_PATH>/kibana/x-pack/test/fleet_api_integration/apis/fixtures/package_registry_config.yml:/package-registry/config.yml docker.elastic.co/package-registry/package-registry:main ``` - Point `kibana.dev.yml` to local EPR: ``` xpack.fleet.registryUrl: http://localhost:8080 ``` - Enable the secrets feature flag `secretsStorage` - Start kibana and navigate to `integrations`, install `Secrets` package. - It should create and edit the package policy successfully <img width="1800" alt="Screenshot 2023-08-08 at 16 26 52" src="https://github.com/elastic/kibana/assets/16084106/5e2b77d9-71a9-4c5f-8b3b-5fc6546d562f"> - The yml policy should have the redacted secrets and secrets ids: <img width="771" alt="Screenshot 2023-08-08 at 15 43 22" src="https://github.com/elastic/kibana/assets/16084106/7db22c6b-b0db-4eb6-bc68-7174374c9c74"> --------- Co-authored-by: Kibana Machine <[email protected]>

…ets index (elastic#163075) Closes elastic#162915 ## Summary Replace direct calls to Fleet Secrets index with new API calls introduced with elastic/elasticsearch#97728 ### New ES secrets APIs: ``` POST /_fleet/secret/ { "value": "<secret value>" } // Returns the id of the created secret { "id": "<secret_id>" } DELETE /_fleet/secret/<secret_id> // returns { "deleted": true } ``` NOTE: I tried running the secrets integration tests in elastic#162732 but there is some ES error that I'm not sure how to address. I think that the test can be worked on separately ### Testing Testing steps are the exact same as elastic#157176: - Start EPR locally loading the `Secrets` test package from Kibana: ``` docker run -p 8080:8080 -v /Users/<YOUR_PATH>/kibana/x-pack/test/fleet_api_integration/apis/fixtures/test_packages:/packages/test-packages -v /Users/<YOUR_PATH>/kibana/x-pack/test/fleet_api_integration/apis/fixtures/package_registry_config.yml:/package-registry/config.yml docker.elastic.co/package-registry/package-registry:main ``` - Point `kibana.dev.yml` to local EPR: ``` xpack.fleet.registryUrl: http://localhost:8080 ``` - Enable the secrets feature flag `secretsStorage` - Start kibana and navigate to `integrations`, install `Secrets` package. - It should create and edit the package policy successfully <img width="1800" alt="Screenshot 2023-08-08 at 16 26 52" src="https://github.com/elastic/kibana/assets/16084106/5e2b77d9-71a9-4c5f-8b3b-5fc6546d562f"> - The yml policy should have the redacted secrets and secrets ids: <img width="771" alt="Screenshot 2023-08-08 at 15 43 22" src="https://github.com/elastic/kibana/assets/16084106/7db22c6b-b0db-4eb6-bc68-7174374c9c74"> --------- Co-authored-by: Kibana Machine <[email protected]>

hop-dev mentioned this pull request May 9, 2023

[Fleet] Secrets storage phase 1 - Remove Kibana's ability to read secrets #154715

Closed

hop-dev changed the title ~~[Fleet] Pretty print secrets in policy yaml~~ [Fleet] View & Edit Secret Variables May 10, 2023

hop-dev force-pushed the secrets-ui branch 2 times, most recently from 6846323 to bc7d2a2 Compare May 16, 2023 13:09

hop-dev added 6 commits May 16, 2023 15:43

format secrets in policy yml

f06936f

secret input working

148a194

format code

9e92143

add info that policy contains secrets in yaml viewer

4475344

fix types

a4ab8ad

move callout to within header

140a86f

hop-dev force-pushed the secrets-ui branch from bc7d2a2 to 140a86f Compare May 16, 2023 14:44

hop-dev self-assigned this May 16, 2023

hop-dev added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.9.0 labels May 16, 2023

hop-dev marked this pull request as ready for review May 16, 2023 15:01

hop-dev requested a review from a team as a code owner May 16, 2023 15:01

juliaElastic assigned jillguyonnet May 17, 2023

juliaElastic self-requested a review May 17, 2023 10:41

juliaElastic reviewed May 17, 2023

View reviewed changes

jlind23 unassigned hop-dev May 23, 2023

jlind23 requested a review from juliaElastic June 19, 2023 09:10

Merge branch 'main' into secrets-ui

62d9ee9

juliaElastic approved these changes Jun 20, 2023

View reviewed changes

juliaElastic added the v8.10.0 label Jun 20, 2023

juliaElastic removed the v8.9.0 label Jun 20, 2023

Merge branch 'main' into secrets-ui

ad1f514

juliaElastic reviewed Jun 21, 2023

View reviewed changes

juliaElastic self-requested a review June 21, 2023 13:44

Merge branch 'main' into secrets-ui

943bbba

Put secret input field behing feature flag

e30e21e

Merge branch 'main' into secrets-ui

5b0ce62

jillguyonnet merged commit e72e055 into elastic:main Jun 26, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 26, 2023

jillguyonnet mentioned this pull request Jun 26, 2023

[Fleet] Package policy saving failure when secrets enabled for the first time #160537

Closed

juliaElastic mentioned this pull request Aug 7, 2023

replace secret refs elastic/fleet-server#2863

Merged

8 tasks

criamico mentioned this pull request Aug 8, 2023

[Fleet] Use new Fleet Secrets ES APIs instead of reading/writing secrets index #163075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] View & Edit Secret Variables #157176

[Fleet] View & Edit Secret Variables #157176

hop-dev commented May 9, 2023 •

edited by juliaElastic

Loading

apmmachine commented May 9, 2023

elasticmachine commented May 16, 2023

juliaElastic May 17, 2023

jillguyonnet Jun 26, 2023

juliaElastic May 17, 2023

juliaElastic May 17, 2023

kpollich May 17, 2023

juliaElastic May 17, 2023

zombieFox May 17, 2023

nimarezainia Jun 20, 2023

juliaElastic commented May 17, 2023

juliaElastic left a comment

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic left a comment

jillguyonnet commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

jillguyonnet commented Jun 22, 2023

jillguyonnet commented Jun 22, 2023

jlind23 commented Jun 22, 2023

nimarezainia commented Jun 23, 2023

jillguyonnet commented Jun 26, 2023

kibana-ci commented Jun 26, 2023

ESLint disabled line counts

Total ESLint disabled count

jillguyonnet commented Jun 26, 2023

juliaElastic commented Jun 26, 2023

jillguyonnet commented Jun 26, 2023

jillguyonnet commented Jun 26, 2023

[Fleet] View & Edit Secret Variables #157176

[Fleet] View & Edit Secret Variables #157176

Conversation

hop-dev commented May 9, 2023 • edited by juliaElastic Loading

Summary

Checklist

Risk Matrix

For maintainers

apmmachine commented May 9, 2023

🤖 GitHub comments

elasticmachine commented May 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaElastic commented May 17, 2023

juliaElastic left a comment

Choose a reason for hiding this comment

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

juliaElastic left a comment

Choose a reason for hiding this comment

jillguyonnet commented Jun 21, 2023

juliaElastic commented Jun 21, 2023

jillguyonnet commented Jun 22, 2023

jillguyonnet commented Jun 22, 2023

jlind23 commented Jun 22, 2023

nimarezainia commented Jun 23, 2023

jillguyonnet commented Jun 26, 2023

kibana-ci commented Jun 26, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

jillguyonnet commented Jun 26, 2023

juliaElastic commented Jun 26, 2023

jillguyonnet commented Jun 26, 2023

jillguyonnet commented Jun 26, 2023

hop-dev commented May 9, 2023 •

edited by juliaElastic

Loading