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

azasypkin · 2020-07-20T12:29:59Z

Summary

In this PR we'll introduce rotation support for the encryption keys used by Encrypted Saved Objects plugin (ESO).

Today if we cannot decrypt attributes (e.g. when encryption key changes) we either fail operation (when getDecryptedAsInternalUser() is used) or strip encrypted attributes from the response and record decryption error (when standard Saved Objects (SO) APIs are used for attributes that should be automatically decrypted).

Having a dedicated error type allows consumers to gracefully handle such errors (e.g. prompt user to re-enter data that needs to be encrypted). This approach works reasonably well in some scenarios, but it may be a very troublesome if we have to deal with lots of SOs. Moreover we'd like to recommend to periodically rotate encryption keys even if they aren't leaked, but before we do so we need to provide a way of seamless migration of the existing encrypted SOs to a new encryption key.

There are two possible scenarios here:

Encryption key is lost

In this scenario encrypted portion of the existing SOs will be lost completely and the only way to recover from this state is manual intervention described above and hence ESO consumers should continue supporting this scenario.

Note: in some cases leaked, but known encryption key can be treated as the lost key, for example when there is a risk that encrypted data was tampered with and it's risky to continue relying on it.

Encryption key is known, but needs to be rotated

In this scenario a new encryption key (primary encryption key) will be generated and Kibana will use it to encrypt new or updated SOs. Kibana will still need to know the old encryption key to be able to decrypt existing data, but this key will no longer be used to encrypt any of the new or existing SOs. It's also should be possible to have multiple old decryption-only keys.

Having multiple decryption keys at the same time brings one problem though: we need to figure out which key to use to decrypt specific SO. 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.

The other solution is to try available existing decryption keys one by one to decrypt data (always starting from the primary encryption key). That's the easiest solution, but as you'll see in the Explorations section below it comes with the cost.

So when the key rotation is required configuration could look like this:

xpack.encryptedSavedObjects:
  encryptionKey: new-and-shiny-encryption-key
  keyRotation:
    decryptionOnlyKeys: [old-and-boring-encryption-key, old-and-leaked-encryption-key]

Depending on the organization policies decryptionOnlyKeys array should be eventually cleaned up and removed from the kibana.yml.

One important thing to note here is that being able to decrypt with the old encryption key at run time can also be helpful in scenarios when SOs were created by two different Kibana instances that use different encryption keys. That's likely to happen only during initial Elastic Stack setup stage though:

# Kibana instance #1
xpack.encryptedSavedObjects:
  encryptionKey: kibana-instance-1-encryption-key
  keyRotation:
    decryptionOnlyKeys: [kibana-instance-2-encryption-key]

# Kibana instance #2
xpack.encryptedSavedObjects:
  encryptionKey: kibana-instance-2-encryption-key
  keyRotation:
    decryptionOnlyKeys: [kibana-instance-1-encryption-key]

But since it's expensive to try multiple decryption keys to decrypt SO encrypted with the old key whenever it's accessed we should also provide a way to migrate all SOs to a new encryption key at once.

For example we could introduce a new kibana.yml option that would tell ESO to re-encrypt everything it knows about with the new key on start and don't use decryptionOnlyKeys after that (or keep using if decryption fails, not a big deal). But I don't like that it's not really convenient and would require restart. Assuming we generally want to have a more UI-driven user friendly configuration we may want to introduce UI for something like this in the future and hence we'll need an API endpoint that would support such UI.

That's why I'm leaning towards introducing a protected API endpoint that would perform bulk re-encryption on-demand/by:

POST https://localhost:5601/api/encrypted_saved_objects/_rotate_key?batchSize=1000

This way admins would have more control on when and how to perform re-encryption and we can respond with more useful stat as well.

Explorations

With the current WIP I tried to gather some data regarding the performance impact of multiple decryption attempts. The difference isn't definitely not catastrophic when you request SOs separately, but it may add up in bulk operations and search/find.

It's worth mentioning that we decrypt in bulk operations only when SO type registered attributes with dangerouslyExposeValue and to best of my knowledge nobody is using this API yet. Here are my explorations nevertheless:

Input

I ingested 1000 of simple SOs with a single attribute that needs to be decrypted during bulk operations (publicPropertyStoredEncrypted):

core.savedObjects.registerType({
  name: 'saved-object-with-secret',
  ...,
  mappings: deepFreeze({
    properties: {
      publicProperty: { type: 'keyword' },
      publicPropertyExcludedFromAAD: { type: 'keyword' },
      publicPropertyStoredEncrypted: { type: 'binary' },
      privateProperty: { type: 'binary' },
    },
  }),
});

deps.encryptedSavedObjects.registerType({
  type: 'saved-object-with-secret',
  attributesToEncrypt: new Set([
    'privateProperty',
    { key: 'publicPropertyStoredEncrypted', dangerouslyExposeValue: true },
  ]),
  attributesToExcludeFromAAD: new Set(['publicPropertyExcludedFromAAD']),
});

I'm using standard _find operation to fetch all of these 1000 SOs at once and measure the time it takes Kibana to process (don't pay too much attention to the absolute values since it's a Kibana in a dev mode with a verbose logging enabled, the difference is what matters here). So the basic autocannon snippet I used looks like this (100 requests in total with 10 simulatenous connections):

#!/usr/bin/env bash

CONNECTIONS=10
REQUESTS=100
MAX_DOCUMENTS=1000

npx autocannon \
  --connections "$CONNECTIONS" \
  --amount "$REQUESTS" \
  --timeout 120 \
  --headers Authorization="Basic ZWxhc3RpYzpjaGFuZ2VtZQ==" \
  --headers Accept="application/json" \
  --headers Content-Type="application/json" \
  "https://localhost:5601/api/saved_objects/_find?type=saved-object-with-secret&per_page=$MAX_DOCUMENTS"

Results

Successful decryption with the primary key

┌─────────┬──────────┬──────────┬──────────┬──────────┬─────────────┬───────────┬─────────────┐
│ Stat    │ 2.5%     │ 50%      │ 97.5%    │ 99%      │ Avg         │ Stdev     │ Max         │
├─────────┼──────────┼──────────┼──────────┼──────────┼─────────────┼───────────┼─────────────┤
│ Latency │ 26522 ms │ 28864 ms │ 29693 ms │ 29712 ms │ 28566.16 ms │ 934.45 ms │ 29721.03 ms │
└─────────┴──────────┴──────────┴──────────┴──────────┴─────────────┴───────────┴─────────────┘

100 requests in 286.33s, 40.5 MB read

Failed decryption with the primary key following successful with the old key

┌─────────┬──────────┬──────────┬──────────┬──────────┬────────────┬────────────┬─────────────┐
│ Stat    │ 2.5%     │ 50%      │ 97.5%    │ 99%      │ Avg        │ Stdev      │ Max         │
├─────────┼──────────┼──────────┼──────────┼──────────┼────────────┼────────────┼─────────────┤
│ Latency │ 54644 ms │ 58862 ms │ 60795 ms │ 60917 ms │ 58858.9 ms │ 1680.31 ms │ 60939.69 ms │
└─────────┴──────────┴──────────┴──────────┴──────────┴────────────┴────────────┴─────────────┘

100 requests in 589.63s, 40.5 MB read

So with two decryption attempts ESO is twice as slow as with a single attempt. I didn't dig into @elastic/node-crypto to understand why the difference is that large yet, but that's what we have right now.

RFC: #72828
Fixes: #56889
Affected by: #77961

Release note: xpack.encryptedSavedObjects.encryptionKey can now be rotated without losing access to existing encrypted Saved Objects (alerts, actions etc.). Old key(s) can be moved to xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys to be used only to decrypt existing objects while new or updated objects will be encrypted using new primary encryption key. Administrators can also use dedicated API endpoint /api/encrypted_saved_objects/_rotate_key to trigger re-encryption of all existing objects with a new primary key so that old keys can be safely disposed.

…jects plugin.

azasypkin · 2020-09-23T08:05:43Z

x-pack/plugins/encrypted_saved_objects/server/config.ts

@@ -37,7 +53,8 @@ export function createConfig$(context: PluginInitializerContext) {
      }

      return {


note: the config object we return here is supposed to be a convenient wrapper around config related stuff used by plugin internally, so it's not required to reflect raw config shape exactly, so I reshaped it slightly.

azasypkin · 2020-09-23T08:09:38Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encryption_key_rotation_service.ts

+    // Keeps track of object IDs that have been processed already.
+    const processedObjectIDs = new Set<string>();
+
+    // Until we get scroll/search_after support in Saved Objects client we have to retrieve as much objects as allowed


note: in an absence of scroll/search_after support that's the only reasonable workaround I could come up with so far. Happy to hear other ideas, critique, objects, concerns! I tried to explain the approach in details in this comment, but let me know if something isn't clear.

azasypkin · 2020-09-23T08:10:46Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encryption_key_rotation_service.ts

+    });
+
+    // Keeps track of object IDs that have been processed already.
+    const processedObjectIDs = new Set<string>();


note: even though this set can get quite large, I believe it shouldn't become a problem memory wise since we store only IDs....

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

azasypkin · 2020-09-23T08:26:26Z

Hey @legrego!

I believe PR is ready for the preliminary feedback whenever you have time (no tests yet). I tried to outline all the details in the description and additional comments, but basically there are two parts that I'd like to get your opinion on:

Configuration and run-time decryption using all available keys
Bulk rotation. This is an optional operation that is supposed to be performed when users need to dispose old keys completely or when performance impact of using multiple decryption keys becomes perceivable. And because of a lack of scroll/search_after support in SO client we can either base it on a workaround (one of them is proposed in PR), or ship it without advertisement as an emergency escape hatch, or just don't ship it at all until Add support to SavedObjects.find for fetching more than 10k objects #77961 is resolved.

legrego · 2020-09-23T11:53:46Z

Thanks @azasypkin! I'll start taking a look tomorrow at the latest, so that you can have some preliminary feedback for next week

legrego

Better late than never (sorry!), but here's some preliminary feedback for you. I did not have a chance to run this yet, but will do so as part of the next review round. Looks great so far! I appreciate all of the thought & work that went into this. You made it look easy 😄

x-pack/plugins/encrypted_saved_objects/server/config.ts

legrego · 2020-09-27T17:43:10Z

x-pack/plugins/encrypted_saved_objects/server/routes/index.ts

+}
+
+export function defineRoutes(params: RouteDefinitionParams) {
+  if (params.config.keyRotation.decryptionOnlyKeys.length > 0) {


Rather than conditionally defining this route, what do you think about returning a 400 or similar, with an error message that describes why we can't fulfill the request?

Perhaps something like:

Kibana is not configured to support encryption key rotation. Update kibana.yml to include xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys to rotate your encryption keys.

I could see the conditional definition being hard to diagnose if there are multiple instances behind a load-balancer, and only one of them is misconfigured. That would result in intermittent 404 responses from this endpoint

I could see the conditional definition being hard to diagnose if there are multiple instances behind a load-balancer, and only one of them is misconfigured. That would result in intermittent 404 responses from this endpoint

++, didn't like that as well, but didn't have a solution yet. Sounds like a good proposal, let me try to do that.

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

legrego · 2020-09-27T22:31:08Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

+        } catch (err) {
+          // Remember the error thrown when we tried to decrypt with the primary key.
+          if (!decryptionError) {
+            decryptionError = err;


Same comment as above: Should we check to make sure that this is an EncryptionError, and not some other unexpected failure condition?

legrego · 2020-09-27T22:34:47Z

x-pack/plugins/encrypted_saved_objects/server/plugin.ts

+
+    // This list of cryptos consists of the primary crypto that is used for both encryption and
+    // decryption, and the optional secondary cryptos that are used for decryption only.
+    const cryptos = [


I don't love this, but given its usage on the encryption service, it feels tolerable. I worry about consumers having to know that the first entry can be used for encryption, and all other keys can only be used for decryption.

What do you think about creating a "patched" version of the "decryption only" crypto instances to throw an error if we ever attempt to call one of the encryption functions?

I don't love this, but given its usage on the encryption service, it feels tolerable. I worry about consumers having to know that the first entry can be used for encryption, and all other keys can only be used for decryption.

I see, we can try to split this constructor argument into two separate ones and join inside constructor so that for consumers it will be something like primaryCrypto and optional decryptionOnlyCryptos?

What do you think about creating a "patched" version of the "decryption only" crypto instances to throw an error if we ever attempt to call one of the encryption functions?

You mean wrap/patch them inside of the constructor? If so, yeah I can try to do that and we'll see how that looks like.

You mean wrap/patch them inside of the constructor? If so, yeah I can try to do that and we'll see how that looks like.

Yeah something like that is what I had in mind.

I see, we can try to split this constructor argument into two separate ones and join inside constructor so that for consumers it will be something like primaryCrypto and optional decryptionOnlyCryptos?

I'd be happy with either of these approaches -- don't feel like you have to implement both. Whichever you feel is better is good with me 👍

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

legrego · 2020-09-28T10:11:09Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

+        });
+      } catch (err) {
+        logger.error(err);
+        return response.internalError();


Given this is an administrative endpoint, it might make sense to return some information about the failure. Are you aware of failure scenarios that would expose information we don't want leaked?

Are you aware of failure scenarios that would expose information we don't want leaked?

Can't think of any. Agree, we can return custom error here instead that would wrap the original error. Will do.

legrego · 2020-09-28T10:12:48Z

x-pack/plugins/encrypted_saved_objects/server/config.ts

+      schema.string({ minLength: 32, defaultValue: 'a'.repeat(32) })
+    ),
+    keyRotation: schema.object({
+      decryptionOnlyKeys: schema.arrayOf(schema.string({ minLength: 32 }), { defaultValue: [] }),


Don't forget to add this to kibana_docker, and allow on ESS once this merges.

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

legrego · 2020-09-28T10:21:50Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

+  /**
+   * Indicates whether decryption should only be performed using secondary decryption-only keys.
+   */
+  useDecryptionOnlyKeys?: boolean;


nit: I think reversing this would make it easier to understand:

Suggested change

useDecryptionOnlyKeys?: boolean;

includePrimaryEncryptionKey?: boolean;

or if you didn't want to reverse it, something like:

Suggested change

useDecryptionOnlyKeys?: boolean;

omitPrimaryEncryptionKey?: boolean;

or (my least favorite suggestion)

Suggested change

useDecryptionOnlyKeys?: boolean;

onlyUseDecryptionOnlyKeys?: boolean;

omitPrimaryEncryptionKey?: boolean;

Sounds good 👍

…n route, prevent multiple concurrent rotation operation, limit max number of batches during rotation and more, add tests.

azasypkin · 2020-10-01T09:11:41Z

Hey @legrego,

I believe I've handled all the comments you left and PR is ready for review whenever you have time.

Thanks!

azasypkin · 2020-10-01T10:09:24Z

x-pack/test/encrypted_saved_objects_api_integration/fixtures/es_archiver/key_rotation/data.json

@@ -0,0 +1,186 @@
+{


note: Essentially I have 6 Saved Objects here: 2 x "single-namespace", 2 x "multiple-namespaces", 2 x "hidden". Three of them are encrypted with 'a'.repeat(32) encryption key and three with 'b'.repeat(32).

azasypkin · 2020-10-01T10:10:12Z

x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts

@@ -437,15 +437,15 @@ export default function ({ getService }: FtrProviderContext) {
      }:${id}`;
    }

-    afterEach(async () => {


note: we need this hook just for basic tests, not for migration and key rotation tests.

spalger

Docker config change LGTM

legrego

This looks and works great! Just a couple of nits/questions for you, but otherwise I think we're good to go!

legrego · 2020-10-02T13:01:48Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encryption_key_rotation_service.ts

+      }
+    }
+
+    this.options.logger.debug(


Given the infrequency of this operation, we can likely make this informational

Suggested change

this.options.logger.debug(

this.options.logger.info(

legrego · 2020-10-02T13:02:45Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encryption_key_rotation_service.ts

+
+    const result = { total: 0, successful: 0, failed: 0 };
+    if (registeredSavedObjectTypes.length === 0) {
+      this.options.logger.debug(


Given the infrequency of this operation, we can likely make this informational

Suggested change

this.options.logger.debug(

this.options.logger.info(

legrego · 2020-10-02T13:03:00Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encryption_key_rotation_service.ts

+      return result;
+    }
+
+    this.options.logger.debug(


Given the infrequency of this operation, we can likely make this informational

Suggested change

this.options.logger.debug(

this.options.logger.info(

legrego · 2020-10-02T13:10:01Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

+  let rotationInProgress = false;
+  router.post(
+    {
+      path: '/api/encrypted_saved_objects/rotate_key',


nit: We've tried to prefix our non-REST "RPC-style" calls with an underscore:

Suggested change

path: '/api/encrypted_saved_objects/rotate_key',

path: '/api/encrypted_saved_objects/_rotate_key',

Ah, yeah, good point, thanks!

legrego · 2020-10-02T13:12:11Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts

+      },
+    },
+    async (context, request, response) => {
+      if (config.keyRotation.decryptionOnlyKeys.length === 0) {


legrego · 2020-10-02T13:13:55Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.test.ts

+    let routeHandler: RequestHandler<any, any, any>;
+    let routeConfig: RouteConfig<any, any, any, any>;
+    beforeEach(() => {
+      const [rotateRouteConfig, rotateRouteHandler] = router.post.mock.calls.find(


Ohh I like this approach to locating routes a lot better than the positional lookups we've done in the past

legrego · 2020-10-02T13:14:13Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.test.ts

+    let routeConfig: RouteConfig<any, any, any, any>;
+    beforeEach(() => {
+      const [rotateRouteConfig, rotateRouteHandler] = router.post.mock.calls.find(
+        ([{ path }]) => path === '/api/encrypted_saved_objects/rotate_key'


Suggested change

([{ path }]) => path === '/api/encrypted_saved_objects/rotate_key'

([{ path }]) => path === '/api/encrypted_saved_objects/_rotate_key'

legrego · 2020-10-02T13:15:58Z

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.test.ts

+        options: { body: { total: 3, successful: 6, failed: 0 } },
+      });
+
+      // And consequent requests resolve properly too.


super-nit

Suggested change

// And consequent requests resolve properly too.

// And subsequent requests resolve properly too.

🤦 thanks!

legrego · 2020-10-02T13:31:14Z

x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts

+          ([, , , hiddenSavedObject]) => !hiddenSavedObject
+        )) {
+          const url = hidden
+            ? `/api/saved_objects/${type}/${id}`


Are you intentionally filtering out hidden saved objects in this test? url is always resolving to the same value here.

Ugh, just oversight, was initially thinking that there is some trick to force this endpoint to return hidden types as well and wanted to preserve this logic. Will remove, thanks!

legrego · 2020-10-02T13:35:33Z

x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts

@@ -529,5 +537,104 @@ export default function ({ getService }: FtrProviderContext) {
        });
      });
    });
+
+    describe('key rotation', () => {


Can we add a test that verifies that a user with just the kibana_admin role can't invoke the endpoint?
The api access check we have now is relying in a subtle implementation detail, so it'd be good to verify this continues to work going forward.

That's a great idea, let me try to figure out how to do that.

…e proper naming convention for the rotate route URL and more.

azasypkin · 2020-10-02T17:09:54Z

ERROR x-pack/test failed
x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts:31:9 - error TS2339: Property '_source' does not exist on type 'void'.

  31         _source: { [type]: savedObject },
             ~~~~~~~

  x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts:32:24 - error TS2345: Argument of type '{ id: string; index: string; }' is not assignable to parameter of type 'GetParams'.
    Property 'type' is missing in type '{ id: string; index: string; }' but required in type 'GetParams'.

  32       } = await es.get({
                            ~
  33         id: generateRawID(id, type),
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  34         index: '.kibana',
     ~~~~~~~~~~~~~~~~~~~~~~~~~
  35       });
     ~~~~~~~

    ../../../kibana/node_modules/@types/elasticsearch/index.d.ts:392:5
      392     type: string;
              ~~~~
      'type' is declared here.

Hmm, have no idea why type check out of blue started to complain like that .... Looking.

legrego

LGTM on green CI. Nice job!

kibanamachine · 2020-10-02T21:01:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0fb4615

Metrics [docs]

distributable file count

id	before	after	diff
`default`	45824	45827	+3

History

💚 Build #79253 succeeded 0fb4615
💔 Build #79232 failed 726f6ba
💚 Build #78664 succeeded 1808db3
💚 Build #78594 succeeded 1e53e61

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

azasypkin · 2020-10-02T21:08:06Z

7.x/7.10.0: 07e2720

azasypkin · 2020-10-05T07:57:12Z

Hey @elastic/kibana-qa,

Just wanted to ping you to check if you can include few test cases that are related to the new functionality introduced in this PR. We can probably join it with the test scenarios you may have for Alerting already (cc @elastic/kibana-alerting-services for better and real examples). We basically want to test that:

When you create an alert with some encrypted attributes using xpack.encryptedSavedObjects.encryptionKey: X and then restart Kibana changing encryption key to Y, alert doesn't work, but not lost (whatever that means, basically this should not lead to unusable Kibana or Alerting app).
Then when you start Kibana with xpack.encryptedSavedObjects.encryptionKey: Y and xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys: [X] that alert works fine again.
In this configuration you'd need to rotate encryption key with the following request (should be performed on behalf of the user with a superuser role):

POST http://localhost:5601/api/encrypted_saved_objects/_rotate_key
Authorization: XXXX
Accept: application/json
Content-Type: application/json
kbn-xsrf: true

Once this is done (result should include the number of successfully updated objects) you remove xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys: [X], restart Kibana and alert should still work fine.

Thanks!

liza-mae · 2020-10-05T22:14:02Z

@azasypkin I looked into this from an automation side, I let Lee know there were start/stop functions for the ES and KBN servers in our Kibana test package. Then, I spoke to Spencer to get more details to see if this can be used to do this type of testing and he referred me that this test coverage can be added as part of our jest integration tests, since it supports starting and stopping Elasticsearch and Kibana servers, it needs a few additions to support reusing the configuration, but looks possible. However, the jest integration tests are currently broken, but @spalger and @tylersmalley are working to get this fixed soon, we will know more once that is done. Thanks.

LeeDr · 2020-10-05T22:27:28Z

@azasypkin are there user-facing docs for this functionality? If so could you pls post a link here.

azasypkin · 2020-10-06T06:44:45Z

@azasypkin I looked into this from an automation side, I let Lee know there were start/stop functions for the ES and KBN servers in our Kibana test package. Then, I spoke to Spencer to get more details to see if this can be used to do this type of testing and he referred me that this test coverage can be added as part of our jest integration tests, since it supports starting and stopping Elasticsearch and Kibana servers, it needs a few additions to support reusing the configuration, but looks possible. However, the jest integration tests are currently broken, but @spalger and @tylersmalley are working to get this fixed soon, we will know more once that is done. Thanks.

Thanks for looking into this! Currently we cover major parts of this functionality with unit and API integration tests, but having ability to cover entire flow would be great.

At this point we want to raise awareness for this new flow even if we cannot automate its testing entirely.

@azasypkin are there user-facing docs for this functionality? If so could you pls post a link here.

Alerting Team mentions encryption functionality in the docs and planning to document key rotation in the scope of this issue. We also have a public RFC that gives a more general overview of this functionality.

LeeDr · 2020-10-06T14:25:36Z

I have a couple of concerns here;

It doesn't seem like we're giving users very clear guidance on how they should manage rotating keys. If they move their existing encryptionKey into decryptionOnlyKeys and create a new encryptionKey (and restart Kibana) I don't think the users have any idea when the existing saved objects have been re-encrypted with the new key. So they don't know when they can get rid of the old key.
We haven't documented the rotate_key API. Can it (or must it) be run while Kibana is running? Or does Kibana have to be stopped. If I store encryptionKey in the keystore, could Kibana re-read it without restarting?

I'm thinking that if Kibana has to always be restarted to use a new key, we should just document to follow one consistent process like;

stop Kibana
change the encryptionKey (but save the old one)
call the _rotate_key API passing in the old key (use the new key from kibana.yml or keystore)
start Kibana

peterschretlen · 2020-10-06T15:37:54Z

@azasypkin am I correct that this would be the process we'd recommend?

Update xpack.encryptedSavedObjects.encryptionKey to a new value. Put the old key in the xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys list.
Restart Kibana. At this point, only new or updated saved objects will be encrypted using the new key - this does not update all saved objects. You'll have to keep the old key for decryption purposes until all objects have been converted.
To force a bulk rotation of all saved objects, call _rotate_key as a superuser. Once this is completed, the old key is no longer in use and can be removed from kibana.yml

the PR and RFC cover all the details, but I think @LeeDr is correct it's not clear what our recommended steps are to users.

On cloud what is the expectation? I don't believe we expose encryptionKey and it is automatically set, then we should not need to expose xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys either?

cc @bmcconaghy who is updating the alerting docs for 7.10

liza-mae · 2020-10-06T16:12:19Z

Referenced cloud issue

legrego · 2020-10-06T16:20:07Z

@peterschretlen your understanding is correct. We have a number of documentation changes to make which we were planning on addressing once we got through feature freeze. This was going to be my primary focus over the next couple of weeks, as we have a number of exciting(?) changes that we need to educate our users about.

On cloud what is the expectation? I don't believe we expose encryptionKey and it is automatically set, then we should not need to expose xpack.encryptedSavedObjects.keyRotation.decryptionOnlyKeys either?

That's correct - as of now, we do not allow the encryption key to be set by deployment administrators. If they wish to use a custom encryption key, then they can contact support to have that done. @azasypkin was discussing this with Cloud earlier today, and he is drafting instructions for support/cloud to follow whenever the encryption key needs to be rotated.

azasypkin added release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.10.0 discuss feedback_needed blocked and removed feedback_needed labels Jul 20, 2020

azasypkin force-pushed the issue-56889-key-rotation branch from cb5a066 to a0a1344 Compare August 5, 2020 16:06

azasypkin force-pushed the issue-56889-key-rotation branch from a0a1344 to b9d07de Compare August 19, 2020 15:34

azasypkin mentioned this pull request Sep 21, 2020

Add support to SavedObjects.find for fetching more than 10k objects #77961

Closed

Add support for the encryption key rotation to the encrypted saved ob…

79c45cb

…jects plugin.

azasypkin force-pushed the issue-56889-key-rotation branch from b9d07de to 79c45cb Compare September 22, 2020 12:11

azasypkin added feedback_needed and removed blocked discuss labels Sep 23, 2020

azasypkin commented Sep 23, 2020

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts Show resolved Hide resolved

azasypkin commented Sep 23, 2020

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts Show resolved Hide resolved

azasypkin commented Sep 23, 2020

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/routes/key_rotation.ts Show resolved Hide resolved

legrego self-requested a review September 27, 2020 17:29

legrego reviewed Sep 28, 2020

View reviewed changes

azasypkin added 4 commits September 29, 2020 09:55

Merge branch 'master' into issue-56889-key-rotation

5fa2d13

Merge branch 'master' into issue-56889-key-rotation

ad7aa6d

Merge branch 'master' into issue-56889-key-rotation

e417209

Review#1: allow new setting in docker config, always register rotatio…

1e53e61

…n route, prevent multiple concurrent rotation operation, limit max number of batches during rotation and more, add tests.

azasypkin removed the feedback_needed label Oct 1, 2020

azasypkin requested a review from legrego October 1, 2020 09:10

Add integration tests.

1808db3

azasypkin commented Oct 1, 2020

View reviewed changes

spalger approved these changes Oct 1, 2020

View reviewed changes

legrego reviewed Oct 2, 2020

View reviewed changes

azasypkin added 2 commits October 2, 2020 16:50

Merge branch 'master' into issue-56889-key-rotation

fecc14a

Review#2: add test to verify that non-superuser cannot rotate key, us…

726f6ba

…e proper naming convention for the rotate route URL and more.

Workaround weird type issue.

0fb4615

legrego approved these changes Oct 2, 2020

View reviewed changes

azasypkin merged commit 388a905 into elastic:master Oct 2, 2020

azasypkin deleted the issue-56889-key-rotation branch October 2, 2020 19:09

azasypkin mentioned this pull request Oct 2, 2020

[7.x] Add support for the encryption key rotation to the encrypted saved objects. (#72420) #79349

Merged

mikecote mentioned this pull request Oct 2, 2020

Alerting docs to have some mention about key rotation #74781

Closed

azasypkin added backported v8.0.0 labels Oct 2, 2020

jportner mentioned this pull request Feb 23, 2022

[Spike] Investigate Encrypted Saved Objects for Synthetic monitors elastic/uptime#406

Closed

azasypkin mentioned this pull request Jun 6, 2024

Consider including the Saved Object's top-level created_by and updated_by properties in the AAD automatically. #184800

Open

		@@ -37,7 +53,8 @@ export function createConfig$(context: PluginInitializerContext) {
		}

		return {

	useDecryptionOnlyKeys?: boolean;
	includePrimaryEncryptionKey?: boolean;

	useDecryptionOnlyKeys?: boolean;
	omitPrimaryEncryptionKey?: boolean;

	useDecryptionOnlyKeys?: boolean;
	onlyUseDecryptionOnlyKeys?: boolean;

	path: '/api/encrypted_saved_objects/rotate_key',
	path: '/api/encrypted_saved_objects/_rotate_key',

	([{ path }]) => path === '/api/encrypted_saved_objects/rotate_key'
	([{ path }]) => path === '/api/encrypted_saved_objects/_rotate_key'

	// And consequent requests resolve properly too.
	// And subsequent requests resolve properly too.

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

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

Conversation

azasypkin commented Jul 20, 2020 • edited Loading

Summary

Encryption key is lost

Encryption key is known, but needs to be rotated

Explorations

Input

Results

Successful decryption with the primary key

Failed decryption with the primary key following successful with the old key

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Sep 23, 2020

legrego commented Sep 23, 2020

legrego left a 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

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

azasypkin commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

legrego left a 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

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

azasypkin commented Oct 2, 2020

legrego left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 2, 2020

💚 Build Succeeded

Metrics [docs]

distributable file count

History

azasypkin commented Oct 2, 2020

azasypkin commented Oct 5, 2020 • edited Loading

liza-mae commented Oct 5, 2020

LeeDr commented Oct 5, 2020

azasypkin commented Oct 6, 2020

LeeDr commented Oct 6, 2020

peterschretlen commented Oct 6, 2020 • edited Loading

liza-mae commented Oct 6, 2020

legrego commented Oct 6, 2020

azasypkin commented Jul 20, 2020 •

edited

Loading

azasypkin commented Oct 5, 2020 •

edited

Loading

peterschretlen commented Oct 6, 2020 •

edited

Loading