RFC: encryption key rotation support for the `encryptedSavedObjects` plugin by azasypkin · Pull Request #72828 · elastic/kibana

azasypkin · 2020-07-22T11:52:02Z

Summary

RFC for encryption key rotation support for the encryptedSavedObjects plugin.

Rendered

Changelog

See the commit messages for a detailed list of changes.

Related to: #56889

…plugin.

elasticmachine · 2020-07-22T12:09:43Z

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

mikecote

RFC LGTM from alerting perspective 👍

mikecote · 2020-07-23T14:08:24Z

rfcs/text/0012_encryption_key_rotation.md

+* Any possible failures that may happen during this operation shouldn't make Kibana nonfunctional.
+* Ordinary users should not be able to trigger this migration since it may consume a considerable amount of computing resources.
+
+We think that the best option we have right now is a dedicated API endpoint that would trigger this migration:


There may also be the option of using a soon-to-be-developed CLI tool: #65788.

Good point, wondering if it still can use the APIs we may expose.

Good question! My guess would be that the CLI tool can't use the Kibana APIs as it would probably not be running. The tool could generate a new encryption key and move the old one to the keyRotation yml setting for the admin if ever that makes things easier.

The original requirement for the tool is to set an initial xpack.encryptedSavedObjects.encryptionKey value to facilitate setting up Kibana.

Got it, thanks for the context! I'll try to sync with @tylersmalley next week then to make sure we're aligned.

kobelb · 2020-07-23T16:25:10Z

rfcs/text/0012_encryption_key_rotation.md

+
+As the name implies, the key from the `decryptionOnlyKeys` is only used to decrypt content that we cannot decrypt with the primary encryption key. It's allowed to have multiple decryption-only keys at the same time. When user creates a new Saved Object or updates the existing one then its content is always encrypted with the primary encryption key. Config schema won't allow having the same key in `encryptionKey` and `decryptionOnlyKeys`.
+
+Having multiple decryption keys at the same time brings one problem though: we need to figure out which key to use to decrypt specific Saved Object. If our encryption keys could have a unique ID that we would store together with the encrypted data (we cannot use encryption key hash for that for obvious reasons) we could know for sure which key to use, but we don't have such functionality right now and it may not be the easiest one to manage through `yml` configuration anyway. 


we cannot use encryption key hash for that for obvious reasons

Apologies for being dense. If it's a one-way hash, why couldn't we store it along with the encrypted data?

Apologies for being dense. If it's a one-way hash, why couldn't we store it along with the encrypted data?

It could be problematic. Interesting thread here: https://crypto.stackexchange.com/questions/61915/can-i-hash-a-secret-key-and-used-the-hash-as-key-id

Since these encryption keys are not necessarily securely/randomly generated, we probably shouldn't use hashes of the keys (even though we do enforce a 32-character minimum). With my cursory understanding of the subject, I tend to agree with @azasypkin that the potential performance benefit does not outweigh the risk in this situation.

If we decided the performance hit is too onerous, perhaps we should look at implementing randomly-generated key IDs.

That's a good question @kobelb and thanks for the answer @jportner! Yeah, my main concern was that we don't control the entropy of the encryption keys that would potentially simplify brute-forcing (no need to make a decryption attempt and you know which objects use which key). Also I guess when admin adds encryption key to the key store they may not assume that we expose hash of it in the Elasticsearch.

Less of a concern but in some setups the same encryption key may be used for the session, reporting and ESO (:wave: to the SaaS setup we all know) that would significantly increase damage if the key is cracked.

To summarize my idea was to use the most flexible and the least controversial way for now since performance impact can be eliminated with the single API call if it ever becomes a problem (currently nobody is using dangerouslyExposeValue: true marker, so it may have just one-time cost if encrypted objects are migrated). And in the future I can see key management UI/tool that would be used for rotation, generation etc. that would internally generate opaque IDs for the keys.

Drone comment: We were confronted with the problem of key id for encrypted snapshots (WIP) as well. We were also reluctant to expose the hash of the secret. Our approach for generating the id is to use the encrypt operation on a hard coded value. Exposing the ciphertext of a known plaintext is guaranteed to not expose any information on the secret key.

Thanks for sharing @albertzaharovits. Can you point me to the relevant code/issue/PR? I'm curious how that is implemented.

Search for AESKeyUtils.computeId in https://github.com/elastic/elasticsearch/pull/53352/files. The code is in a bit of neglected state, but it is bound to get into master. You can also follow the discussion elastic/elasticsearch#53352 (comment) .

legrego · 2020-07-22T14:03:17Z

rfcs/text/0012_encryption_key_rotation.md

+
+# Unresolved questions
+
+* Is it reasonable to have this feature in Basic?


Making this available in Basic makes sense to me, since that's where we license encrypted saved objects as a whole. Moving this feature into a different license level would hurt our basic users, since they wouldn't be able to recover their encrypted objects in the event that a rotation was required. To me, this is more of a reliability/stability feature than a "sellable" feature.

++, I definitely agree.

legrego · 2020-07-22T14:10:46Z

rfcs/text/0012_encryption_key_rotation.md

+
+# Drawbacks
+
+* Multiple decryption attempts affect performance. See [the performance test results](https://github.com/elastic/kibana/pull/72420#issue-453400211) for more details, but making two decryption attempts is basically twice as slow as with a single attempt. Although it's only relevant for the batch operations that perform automatic decryption (only for the Saved Objects registered with `dangerouslyExposeValue: true` marker) and that nobody is using this functionality in Kibana right now, we may have this use case in the future.


If I'm understanding correctly, I believe this also has the potential to affect the performance of saved object migrations.

If an encrypted saved object requires a migration, then we need to decrypt the object (even if it's not registered with dangerouslyExposeValue: true) to allow the migration to complete.

Yep, that's a good point, I'll mention that.

UPDATE: updated via 160697f

legrego

LGTM
Thanks for this, Aleh!

azasypkin · 2020-08-05T06:43:33Z

Thanks everyone for the feedback! I'm moving RFC into final comment period and will merge it on Monday if no concerns are raised by that time.

azasypkin · 2020-08-10T07:18:46Z

7.x/7.10.0: fa2bf9b

azasypkin force-pushed the issue-xxx-rfc-rotation branch from 291c028 to 43f34b3 Compare July 22, 2020 11:52

RFC: encryption key rotation support for the encryptedSavedObjects …

1fc13b2

…plugin.

azasypkin force-pushed the issue-xxx-rfc-rotation branch from 43f34b3 to 1fc13b2 Compare July 22, 2020 12:08

azasypkin added feedback_needed release_note:skip Skip the PR/issue when compiling release notes RFC Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// labels Jul 22, 2020

azasypkin marked this pull request as ready for review July 22, 2020 12:09

azasypkin added the v7.10.0 label Jul 22, 2020

azasypkin requested review from kobelb, legrego and mikecote July 22, 2020 12:10

azasypkin mentioned this pull request Jul 22, 2020

Add support for the encryption key rotation to the encrypted saved objects. #72420

Merged

mikecote approved these changes Jul 23, 2020

View reviewed changes

kobelb reviewed Jul 23, 2020

View reviewed changes

legrego reviewed Jul 28, 2020

View reviewed changes

azasypkin added 2 commits July 29, 2020 10:16

Merge branch 'master' into issue-xxx-rfc-rotation

0048cb6

Change#1: mention impact for the encrypted Saved Objects migrations.

160697f

azasypkin added review and removed feedback_needed labels Jul 29, 2020

azasypkin requested review from kobelb and legrego July 30, 2020 06:17

legrego approved these changes Jul 30, 2020

View reviewed changes

kobelb approved these changes Aug 4, 2020

View reviewed changes

azasypkin added the RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted label Aug 5, 2020

tylersmalley mentioned this pull request Aug 5, 2020

Ephemeral encrypted saved-object encryption key #65788

Closed

mikecote mentioned this pull request Aug 5, 2020

[alerting] When you rotate/change/lose your encrypted saved object key, alerts should go from enabled to disabled #74393

Closed

Merge branch 'master' into issue-xxx-rfc-rotation

9665a47

azasypkin merged commit 37ce101 into elastic:master Aug 10, 2020

azasypkin deleted the issue-xxx-rfc-rotation branch August 10, 2020 07:08

azasypkin mentioned this pull request Aug 10, 2020

[7.x] RFC: encryption key rotation support for the encryptedSavedObjects plugin (#72828) #74628

Merged

kobelb mentioned this pull request Apr 27, 2021

RFC template #98501

Merged


		As the name implies, the key from the `decryptionOnlyKeys` is only used to decrypt content that we cannot decrypt with the primary encryption key. It's allowed to have multiple decryption-only keys at the same time. When user creates a new Saved Object or updates the existing one then its content is always encrypted with the primary encryption key. Config schema won't allow having the same key in `encryptionKey` and `decryptionOnlyKeys`.

		Having multiple decryption keys at the same time brings one problem though: we need to figure out which key to use to decrypt specific Saved Object. If our encryption keys could have a unique ID that we would store together with the encrypted data (we cannot use encryption key hash for that for obvious reasons) we could know for sure which key to use, but we don't have such functionality right now and it may not be the easiest one to manage through `yml` configuration anyway.


		# Unresolved questions

		* Is it reasonable to have this feature in Basic?


		# Drawbacks

		* Multiple decryption attempts affect performance. See [the performance test results](https://github.com/elastic/kibana/pull/72420#issue-453400211) for more details, but making two decryption attempts is basically twice as slow as with a single attempt. Although it's only relevant for the batch operations that perform automatic decryption (only for the Saved Objects registered with `dangerouslyExposeValue: true` marker) and that nobody is using this functionality in Kibana right now, we may have this use case in the future.

Conversation

azasypkin commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Uh oh!

elasticmachine commented Jul 22, 2020

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Aug 5, 2020

Uh oh!

azasypkin commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

azasypkin commented Jul 22, 2020 •

edited

Loading

azasypkin Jul 29, 2020 •

edited

Loading