Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fleet] Encrypt ssl fields in logstash output #129131

Merged
merged 20 commits into from
Apr 5, 2022

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Mar 31, 2022

Summary

Encrypt the ssl secrets in the logstash output using encrypted saved object
This require the user to set the encryption key in kibana.yml

UI Changes

Screen Shot 2022-04-05 at 11 41 53 AM

@joshdover
Copy link
Contributor

Yeah I was afraid of this possibility. The right way to solve this would be to add a parameter to the output service to accept a regular SO client, with the encrypted SO client wrapper. This would requiring adding a new parameter to several code paths for this client, including any path that modifies an agent policy or integration policy since those result in needing to generating the full agent policy in the .fleet-policies index.

As discussed on Slack, I think we'll need to do something similar to this fake request strategy used here: https://github.com/elastic/kibana/blob/8bad83e3eb5d03c5dfbaf18069c707f8a87c0fa3/x-pack/plugins/fleet/server/services/agent_policy_update.ts/#L39

@nchaulet
Copy link
Member Author

nchaulet commented Apr 1, 2022

@joshdover I was able to get it working with the fake request hack, I had to wait on the licence to be available otherwise I get an error with unauthorized call to .kibana.
If it's the fix we want to use I can come with a fully working PR today.

@nchaulet
Copy link
Member Author

nchaulet commented Apr 1, 2022

@elasticmachine merge upstream

@nchaulet nchaulet self-assigned this Apr 1, 2022
@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 v8.3.0 labels Apr 1, 2022
@nchaulet nchaulet requested a review from joshdover April 1, 2022 17:38
@nchaulet nchaulet changed the title Test encrypt ssl fleet server [Fleet] Encrypt ssl fields in logstash output Apr 1, 2022
@nchaulet nchaulet marked this pull request as ready for review April 1, 2022 17:42
@nchaulet nchaulet requested a review from a team as a code owner April 1, 2022 17:42
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 1, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet requested a review from a team as a code owner April 1, 2022 20:08
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approach LGTM

>
<FormattedMessage
id="xpack.fleet.encryptionKeyRequired.calloutDescription"
defaultMessage="To configure logstash output, set a value of {key} in your {file} file. {link}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I wonder if we should use different wording here since not all environments use kibana.yml (eg. docker users use env vars often).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we probably can update this, (I think it can come as a follow up PR as merging this one will unlock testing the logstash output) the doc do not specify the docker env var variables either maybe it could be fixed here.

@nchaulet
Copy link
Member Author

nchaulet commented Apr 4, 2022

@elasticmachine merge upstream

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've offered suggestions for text string changes, but would like @gchaps to confirm.

id: options?.id ?? newSo.id,
...newSo.attributes,
};
return outputSavedObjectToOutput(newSo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is newSo.ssl unencrypted here? (but has been encrypted when written to ES?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct, when you use dangerouslyExposeValue in the saved object declaration the wrapped saved object client return the decrypted values

@nchaulet
Copy link
Member Author

nchaulet commented Apr 5, 2022

@elasticmachine merge upstream

@nchaulet nchaulet requested review from gchaps and dedemorton April 5, 2022 15:43
Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI text LGTM!

Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI copy and docslink LGTM.

@nchaulet
Copy link
Member Author

nchaulet commented Apr 5, 2022

@elasticmachine merge upstream

@nchaulet nchaulet enabled auto-merge (squash) April 5, 2022 18:53
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #12 / alerting api integration spaces only Alerting builtin alertTypes es_query alert runs correctly: use epoch millis - threshold on hit count < > for searchSource search type

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 578 579 +1

Async chunks

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

id before after diff
fleet 683.2KB 684.2KB +992.0B

Page load bundle

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

id before after diff
core 318.7KB 318.7KB +65.0B

History

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

cc @nchaulet

@nchaulet nchaulet merged commit 420359b into elastic:main Apr 5, 2022
kibanamachine pushed a commit that referenced this pull request Apr 5, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.2

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

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team ui-copy Review of UI copy with docs team is recommended v8.2.0 v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants