[Fleet] Only enable output secrets if all Fleet servers are compatible #173398

jillguyonnet · 2023-12-14T15:35:26Z

Summary

Output secret storage requires that all Fleet servers are on version 8.12.0 or above.

The implementation is similar to package policy secrets: #163627: this PR adds the new output_secret_storage_requirements_met flag on the ingest_manager_settings saved object.

Testing

Define a preconfigured output wit a secret value in your kibana.dev.yml file, e.g.:

xpack.fleet.outputs:
  - id: my-logstash-output-with-a-secret
    name: preconfigured logstash output with a secret
    type: logstash
    hosts: ['localhost:9999']
    ssl:
      certificate: xxxxxxxxxx
    secrets:
      ssl:
        key: secretLogstashKey

Start ES and Kibana. Do not start a Fleet server.
Go to Fleet settings and inspect the preconfigured output: it should have been created and the secret value should not have been set (optionally, you can check in the Console with GET .fleet-secrets/_search that the secret was not created). However, a plain text equivalent should have been created (in the example above, ssl.key should be set to secretLogstashKey).
Start a Fleet server on version less than 8.12.0. Kibana should update the output. Again, check that the secret value was not set and that the plain text equivalent is set.
Stop the Fleet server and start another one on version 8.12.0 or higher. Kibana should update the output. This time, the secret value should have been set.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

apmmachine · 2023-12-14T15:35:42Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

jillguyonnet · 2023-12-18T08:58:01Z

/ci

jillguyonnet · 2023-12-18T11:25:26Z

/ci

jillguyonnet · 2023-12-19T09:24:12Z

/ci

… src/core/server/integration_tests/ci_checks'

jillguyonnet · 2023-12-19T10:22:24Z

/ci

jillguyonnet · 2023-12-19T14:42:09Z

@elastic/fleet @juliaElastic Can I get some input about the expectation here?

As is, a secret will not be set if the Fleet servers version check fails. This could have the effect that the output is not fully set. Is this sufficient, or should there be added logic to set a plain text value instead?

The Cypress test failures are a reflection of this: for instance, this expectation fails as the secret is not set (this is because there is no Fleet server).

juliaElastic · 2023-12-19T15:13:42Z

As is, a secret will not be set if the Fleet servers version check fails. This could have the effect that the output is not fully set. Is this sufficient, or should there be added logic to set a plain text value instead?

If the Fleet server version check fails, the logic should fallback to storing plain text secrets (existing behaviour), this is also how it works for policy secrets.

juliaElastic · 2024-01-02T11:20:13Z

@jillguyonnet Do you think we can get this pr in before the last BC is built on Jan 4? Let me know if you need help.

jillguyonnet · 2024-01-02T13:30:04Z

Hey @juliaElastic - yes tackling this now.

jillguyonnet · 2024-01-02T13:30:28Z

@elasticmachine merge upstream

jillguyonnet · 2024-01-03T10:59:10Z

/ci

jillguyonnet · 2024-01-03T14:46:31Z

/ci

jillguyonnet · 2024-01-03T16:22:55Z

/ci

juliaElastic · 2024-01-04T09:34:52Z

x-pack/plugins/fleet/server/services/output.ts

+        updateData.type === outputType.RemoteElasticsearch
+      ) {
+        if (!data.service_token && data.secrets?.service_token) {
+          updateData.service_token = data.secrets?.service_token as string;


I wonder if this logic can be extracted, seems very similar to the create method. It could be done in a follow up pr.
Do we have test coverage over this logic?

Also, is it necessary for originalOutput to have the same type as the updated output? It can happen that the type changes, isn't it?

I wonder if this logic can be extracted, seems very similar to the create method. It could be done in a follow up pr.

Agreed. These methods are very long and could use some extracting/refactoring. It's always a little dangerous to leave refactoring for followup, but it might be the right call in this case.

Do we have test coverage over this logic?

We do in the integration tests, I suppose, but absolutely happy to add some unit tests as well.

Also, is it necessary for originalOutput to have the same type as the updated output? It can happen that the type changes, isn't it?

Good point! I guess that is not required (and perhaps even a liability). Perhaps it would be good to add a test with an output type change.

I think we have some tests on output type change in cypress tests.

juliaElastic · 2024-01-04T09:41:25Z

buildkite, test this

juliaElastic

Tested, and looks good.

There is one scenario which looks expected, though maybe something we should clarify in the docs. Namely how existing outputs can be recreated with secret storage.
Steps to reproduce:

Create an output with a secret without fleet server, the output secret is stored as plain text (e.g. remote ES with secret token)
Enroll a fleet server 8.12
Go back to the existing output to edit
Outcome: the service token shows up as a plain text, there is no way to update it to be saved as a secret from the UI, unless creating a new output.

jillguyonnet · 2024-01-04T10:20:14Z

Thanks for your review @juliaElastic 🙏

I will address your point regarding originalOutput above, let me know if you would like to see added tests and/or refactor in the PR (otherwise happy to open a followup issue).

Regarding the UI, there is this followup issue that was considered a nice-to-have, although I definitely agree that the docs should reflect the current situation: #171418. WDYT?

I'm also waiting for approval from @elastic/kibana-core.

juliaElastic · 2024-01-04T10:36:35Z

I will address your point regarding originalOutput above, let me know if you would like to see added tests and/or refactor in the PR (otherwise happy to open a followup issue).

Thanks, I'm fine to leave the refactor as a follow up, to get this in before the next BC build.

Regarding the UI, there is this followup issue that was considered a nice-to-have, although I definitely agree that the docs should reflect the current situation: #171418. WDYT?

Good to know that there is already a follow up for this. We could check if there is already any docs being added on output secrets.

jillguyonnet · 2024-01-04T13:36:52Z

Pushed the fix with a couple of associated unit tests.

I opened #174258 as a followup refactor issue.

Also pinged the core team on Slack, hopefully will get this out now 🤞

jillguyonnet · 2024-01-04T14:12:34Z

@elasticmachine merge upstream

jloleysens

Additional mapping looks good to me! Approving to unblock progress -- although I can't see the new field currently being used for search?

jillguyonnet · 2024-01-08T08:41:39Z

Thanks for approving @jloleysens - it's a good point, I thought the implementation should mirror what was done for policy secrets in #163627.

kibana-ci · 2024-01-08T09:17:22Z

💚 Build Succeeded

Buildkite Build
Commit: 692740a

Metrics [docs]

✅ unchanged

History

💚 Build #185873 succeeded eb8063c
💔 Build #185856 failed 02504e2
💚 Build #185795 succeeded b5f864a
💔 Build #185680 failed fa6700a
💔 Build #185646 failed ae33645
💔 Build #185590 failed 0ff0c5b

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

cc @jillguyonnet

elastic#173398) ## Summary Closes elastic#173041 Output secret storage requires that all Fleet servers are on version 8.12.0 or above. The implementation is similar to package policy secrets: elastic#163627: this PR adds the new `output_secret_storage_requirements_met` flag on the `ingest_manager_settings` saved object. ### Testing 1. Define a preconfigured output wit a secret value in your `kibana.dev.yml` file, e.g.: ```yml xpack.fleet.outputs: - id: my-logstash-output-with-a-secret name: preconfigured logstash output with a secret type: logstash hosts: ['localhost:9999'] ssl: certificate: xxxxxxxxxx secrets: ssl: key: secretLogstashKey ``` 3. Start ES and Kibana. Do not start a Fleet server. 4. Go to Fleet settings and inspect the preconfigured output: it should have been created and the secret value should not have been set (optionally, you can check in the Console with `GET .fleet-secrets/_search` that the secret was not created). However, a plain text equivalent should have been created (in the example above, `ssl.key` should be set to `secretLogstashKey`). 5. Start a Fleet server on version less than 8.12.0. Kibana should update the output. Again, check that the secret value was not set and that the plain text equivalent is set. 6. Stop the Fleet server and start another one on version 8.12.0 or higher. Kibana should update the output. This time, the secret value should have been set. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Julia Bardi <[email protected]> (cherry picked from commit 70508b9)

kibanamachine · 2024-01-08T09:26:06Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…mpatible (#173398) (#174433) # Backport This will backport the following commits from `main` to `8.12`: - [[Fleet] Only enable output secrets if all Fleet servers are compatible (#173398)](#173398)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Jill Guyonnet <[email protected]>

elastic#173398) ## Summary Closes elastic#173041 Output secret storage requires that all Fleet servers are on version 8.12.0 or above. The implementation is similar to package policy secrets: elastic#163627: this PR adds the new `output_secret_storage_requirements_met` flag on the `ingest_manager_settings` saved object. ### Testing 1. Define a preconfigured output wit a secret value in your `kibana.dev.yml` file, e.g.: ```yml xpack.fleet.outputs: - id: my-logstash-output-with-a-secret name: preconfigured logstash output with a secret type: logstash hosts: ['localhost:9999'] ssl: certificate: xxxxxxxxxx secrets: ssl: key: secretLogstashKey ``` 3. Start ES and Kibana. Do not start a Fleet server. 4. Go to Fleet settings and inspect the preconfigured output: it should have been created and the secret value should not have been set (optionally, you can check in the Console with `GET .fleet-secrets/_search` that the secret was not created). However, a plain text equivalent should have been created (in the example above, `ssl.key` should be set to `secretLogstashKey`). 5. Start a Fleet server on version less than 8.12.0. Kibana should update the output. Again, check that the secret value was not set and that the plain text equivalent is set. 6. Stop the Fleet server and start another one on version 8.12.0 or higher. Kibana should update the output. This time, the secret value should have been set. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Julia Bardi <[email protected]>

jillguyonnet self-assigned this Dec 18, 2023

jillguyonnet added 2 commits December 18, 2023 12:00

[Fleet] Only enable output secrets if all Fleet servers are compatible

d9d3582

Fix mappings

2ff0875

jillguyonnet force-pushed the fleet/173041-output-secrets-fleet-server-check branch from ca07d48 to 2ff0875 Compare December 18, 2023 11:18

jillguyonnet added Team:Fleet Team label for Observability Data Collection Fleet team release_note:skip Skip the PR/issue when compiling release notes v8.12.0 labels Dec 18, 2023

Add SO model version

3361e7c

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

30c859d

… src/core/server/integration_tests/ci_checks'

kibanamachine and others added 2 commits January 2, 2024 08:30

Merge branch 'main' into fleet/173041-output-secrets-fleet-server-check

0215067

Fix WIP

0ff0c5b

Fix tests

ae33645

jillguyonnet added 2 commits January 3, 2024 16:24

Fix type check

437413b

Additional tests

fa6700a

Final fixes and additions to integration tests

b5f864a

jillguyonnet marked this pull request as ready for review January 4, 2024 09:10

jillguyonnet requested a review from a team as a code owner January 4, 2024 09:10

jillguyonnet requested a review from juliaElastic January 4, 2024 09:10

juliaElastic reviewed Jan 4, 2024

View reviewed changes

juliaElastic approved these changes Jan 4, 2024

View reviewed changes

Feedback

eed6548

jillguyonnet mentioned this pull request Jan 4, 2024

[Fleet] Refactor output CRUD logic #174258

Open

kibanamachine and others added 2 commits January 4, 2024 09:12

Merge branch 'main' into fleet/173041-output-secrets-fleet-server-check

02504e2

Remove faulty unit test

eb8063c

juliaElastic requested review from rudolf and pgayvallet January 5, 2024 08:28

jloleysens approved these changes Jan 5, 2024

View reviewed changes

Merge branch 'main' into fleet/173041-output-secrets-fleet-server-check

692740a

jillguyonnet merged commit 70508b9 into elastic:main Jan 8, 2024
20 checks passed

jillguyonnet deleted the fleet/173041-output-secrets-fleet-server-check branch January 8, 2024 09:20

kibanamachine added the v8.13.0 label Jan 8, 2024

kibanamachine mentioned this pull request Jan 8, 2024

[8.12] [Fleet] Only enable output secrets if all Fleet servers are compatible (#173398) #174433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Only enable output secrets if all Fleet servers are compatible #173398

[Fleet] Only enable output secrets if all Fleet servers are compatible #173398

jillguyonnet commented Dec 14, 2023 •

edited by kibanamachine

Loading

apmmachine commented Dec 14, 2023

jillguyonnet commented Dec 18, 2023

jillguyonnet commented Dec 18, 2023

jillguyonnet commented Dec 19, 2023

jillguyonnet commented Dec 19, 2023

jillguyonnet commented Dec 19, 2023

juliaElastic commented Dec 19, 2023 •

edited

Loading

juliaElastic commented Jan 2, 2024 •

edited

Loading

jillguyonnet commented Jan 2, 2024

jillguyonnet commented Jan 2, 2024

jillguyonnet commented Jan 3, 2024

jillguyonnet commented Jan 3, 2024

jillguyonnet commented Jan 3, 2024

juliaElastic Jan 4, 2024

jillguyonnet Jan 4, 2024

juliaElastic Jan 5, 2024

juliaElastic commented Jan 4, 2024

juliaElastic left a comment

jillguyonnet commented Jan 4, 2024

juliaElastic commented Jan 4, 2024

jillguyonnet commented Jan 4, 2024

jillguyonnet commented Jan 4, 2024

jloleysens left a comment

jillguyonnet commented Jan 8, 2024

kibana-ci commented Jan 8, 2024

kibanamachine commented Jan 8, 2024

[Fleet] Only enable output secrets if all Fleet servers are compatible #173398

[Fleet] Only enable output secrets if all Fleet servers are compatible #173398

Conversation

jillguyonnet commented Dec 14, 2023 • edited by kibanamachine Loading

Summary

Testing

Checklist

apmmachine commented Dec 14, 2023

🤖 GitHub comments

jillguyonnet commented Dec 18, 2023

jillguyonnet commented Dec 18, 2023

jillguyonnet commented Dec 19, 2023

jillguyonnet commented Dec 19, 2023

jillguyonnet commented Dec 19, 2023

juliaElastic commented Dec 19, 2023 • edited Loading

juliaElastic commented Jan 2, 2024 • edited Loading

jillguyonnet commented Jan 2, 2024

jillguyonnet commented Jan 2, 2024

jillguyonnet commented Jan 3, 2024

jillguyonnet commented Jan 3, 2024

jillguyonnet commented Jan 3, 2024

juliaElastic Jan 4, 2024

Choose a reason for hiding this comment

jillguyonnet Jan 4, 2024

Choose a reason for hiding this comment

juliaElastic Jan 5, 2024

Choose a reason for hiding this comment

juliaElastic commented Jan 4, 2024

juliaElastic left a comment

Choose a reason for hiding this comment

jillguyonnet commented Jan 4, 2024

juliaElastic commented Jan 4, 2024

jillguyonnet commented Jan 4, 2024

jillguyonnet commented Jan 4, 2024

jloleysens left a comment

Choose a reason for hiding this comment

jillguyonnet commented Jan 8, 2024

kibana-ci commented Jan 8, 2024

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Jan 8, 2024

💚 All backports created successfully

Questions ?

jillguyonnet commented Dec 14, 2023 •

edited by kibanamachine

Loading

juliaElastic commented Dec 19, 2023 •

edited

Loading

juliaElastic commented Jan 2, 2024 •

edited

Loading