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

Do not generate an ephemeral encryption key in production. #81511

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Oct 22, 2020

Currently we generate a random encryption key if it's not specified by default when Kibana is run in production setup (dist === true). After this PR we won't be automatically generating a random key in production in case it's not explicitly specified. We're also exposing canEncrypt property in the setup contract to help consumers figure out if ESO is able to perform encryption\decryption or not (instead of usingEphemeralEncryptionKey).

Note to reviewers: we initially planned to disable ESO plugin if encryption key isn't specified, but we later figured out that it can introduce non-trivial issues with Saved Objects migrations, so we decided not to do that for now.

Fixes: #79943

Release note

Previously Kibana was mistakenly allowed to perform a number of operations (e.g. migrations) with Saved Objects (alerts, actions etc.) using the ephemeral encryption key that's automatically generated when xpack.encryptedSavedObjects.encryptionKey isn't specified. This could potentially lead to a data-loss since ephemeral key is lost after restart. It's no longer the case with this fix.

@azasypkin azasypkin force-pushed the issue-xxx-optional-eso branch from 6fb6495 to 28a6846 Compare February 9, 2021 11:42
@azasypkin azasypkin removed the blocked label Feb 9, 2021
@@ -4,14 +4,13 @@
"server": true,
"ui": true,
"configPath": ["xpack", "fleet"],
"requiredPlugins": ["licensing", "data"],
"requiredPlugins": ["licensing", "data", "encryptedSavedObjects"],
Copy link
Member Author

Choose a reason for hiding this comment

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

note: for 7.12 we decided to not disable plugin automatically and since you still depend SO migrations it makes sense to keep this as a required dependency for now. Let me know if you have any concerns.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense 👍 and we will fix our SO migration

@azasypkin azasypkin marked this pull request as ready for review February 9, 2021 13:48
@azasypkin azasypkin requested review from a team as code owners February 9, 2021 13:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Fleet changes 👍

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM for Stack Monitoring!

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Tested locally, looks great!

@@ -26,8 +25,8 @@ export interface PluginsSetup {
}

export interface EncryptedSavedObjectsPluginSetup {
canEncrypt: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

optional It's probably a good idea to start documenting our public interface

Suggested change
canEncrypt: boolean;
/** Indicates if Saved Object encryption is possible. Requires an encryption key to be explicitly set via `xpack.encryptedSavedObjects.encryptionKey`. */
canEncrypt: boolean;

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely agree, thanks!

@@ -32,6 +29,17 @@ describe('config schema', () => {
}
`);

expect(ConfigSchema.validate({ encryptionKey: 'z'.repeat(32) }, { dist: true }))
Copy link
Member

Choose a reason for hiding this comment

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

I know that this is probably "testing the library", but would you mind adding a test to ensure that encryptionKey cannot be null when dist: true? Plugin code checks that the key isn't undefined, but if schema.maybe() ever allowed null, then we would incorrectly assume that an encryption key was set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will add, thanks

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Looked it over for security_solutions, this looks 👍 , lgtm

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@azasypkin azasypkin merged commit 03a53b9 into elastic:master Feb 10, 2021
@azasypkin azasypkin deleted the issue-xxx-optional-eso branch February 10, 2021 10:27
@azasypkin
Copy link
Member Author

7.x/7.12.0: fc4beaf

gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 10, 2021
* master: (99 commits)
  [Fleet] Use Fleet Server indices in the search bar (elastic#90835)
  [Search Sessions] added an info flyout to session management (elastic#90559)
  [ILM] Revisit searchable snapshot field after new redesign (elastic#90793)
  [Alerting] License Errors on Alert List View (elastic#89920)
  RFC Improve saved object migrations algorithm (elastic#84333)
  [Lens] (Accessibility) Fix focus on drag and drop actions (elastic#90561)
  Use new shortcut links to Fleet discuss forums. (elastic#90786)
  Do not generate an ephemeral encryption key in production. (elastic#81511)
  [Fleet] Use staging registry for snapshot builds (elastic#90327)
  Actually deleting x-pack/tsconfig.refs.json (elastic#90898)
  Add deprecation warning to all Beats CM pages. (elastic#90741)
  skip flaky suite (elastic#90136)
  Revert "Revert "[Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)"" (elastic#90889)
  remove ref to removed tsconfig file
  [core.logging] Uses host timezone as default (elastic#90368)
  [Maps] remove maps_file_upload plugin and fold public folder into file_upload plugin (elastic#90292)
  Revert "[Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)"
  [dev-utils/ci-stats] support disabling ship errors (elastic#90851)
  Prefix with / (elastic#90836)
  [Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't generate xpack.encryptedSavedObjects.encryptionKey
8 participants