[Defend Workflows][8.11] Unblock fleet setup when cannot decrypt uninstall tokens #171998

gergoabraham · 2023-11-27T16:27:08Z

Summary

This PR should help in case of an improper encryption key rotation. Prior to this PR, the user was blocked on Fleet setup:

This PR does the following:

don't decrypt tokens when trying to generate missing tokens on Fleet setup -> hence no error is thrown
but, checks if existing tokens can be decrypted, and returns with a non-fatal error during Fleet setup if not

prohibits user to enable tamper protection when the given policy's uninstall token cannot be decrypted
no change: still throws error when trying to decrypt

Reproducing the encryption key issue

setup a Kibana, add at least one policy (probably Fleet policy is enough)
modify (or add a new) encryption key in your kibana.dev.yml:

xpack.encryptedSavedObjects.encryptionKey: "some-random-encryption-key-min-32-bytes"

Checklist

Delete any items that are not applicable to this PR.

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

kpollich

We should still try to add logic here that checks for the .error property set on saved objects returned from createPointInTimeFinderDecryptedAsInternalUser.

The above method will "strip" encrypted attributes (which is how we get into a state with token = undefined in the first place, generally) here:

kibana/x-pack/plugins/encrypted_saved_objects/server/saved_objects/index.ts

Lines 163 to 171 in a9c41e5

    
           } catch (error) { 
        
             // catch error and enrich SO with it, return stripped attributes. Then consumer of API can decide either proceed 
        
             // with only unsecured properties or stop when error happens 
        
             const { attributes: strippedAttrs } = await service.stripOrDecryptAttributes( 
        
               descriptor, 
        
               savedObject.attributes as Record<string, unknown> 
        
             ); 
        
             return { ...savedObject, attributes: strippedAttrs as T, error }; 
        
           }

.

We need to surface this error property that's appended to the ESO object somewhere in Fleet setup.

kpollich · 2023-11-27T16:52:36Z

x-pack/plugins/fleet/server/services/security/uninstall_token_service/index.test.ts

+        it('rejects if any of the tokens is missing', async () => {
+          await expect(
+            uninstallTokenService.checkTokenValidityForAllPolicies()
+          ).rejects.toThrowError('Uninstall Token is missing the token.');


Could we try to improve this error message? It's not particularly helpful to users in its current state. We could provide something like:

Invalid uninstall token for policy ID ${policyId}. Saved object is missing the "token" attribute.

elasticmachine · 2023-11-27T16:54:36Z

Pinging @elastic/fleet (Team:Fleet)

kpollich · 2023-11-27T16:55:41Z

We need to surface this error property that's appended to the ESO object somewhere in Fleet setup.

We should be able to get into this state by encrypting values once -> changing the encryption key in kibana.dev.yml -> observe errors in Fleet setup following. Errors would be expected, and we probably want to do the following in that case:

Disable agent tamper protection
Show the decryption errors to the user

gergoabraham · 2023-11-27T17:21:54Z

We need to surface this error property that's appended to the ESO object somewhere in Fleet setup.

We should be able to get into this state by encrypting values once -> changing the encryption key in kibana.dev.yml -> observe errors in Fleet setup following. Errors would be expected, and we probably want to do the following in that case:

Disable agent tamper protection

Show the decryption errors to the user

I'm not sure we can disable agent tamper protection at this point, if encryption key and therefore message signing key is changed, so maybe all we can do is inform the user about the issue, and unblock accessing Fleet pages. I think we should continue investigation, but I'd propose to add this quick fix to tomorrow's 8.11.2 FF. @kpollich, what do you think about this approach?

kpollich · 2023-11-27T17:24:10Z

what do you think about this approach?

SGTM in interest of getting this in for feature freeze 👍

joeypoon · 2023-11-27T17:38:40Z

We should still try to add logic here that checks for the .error property set on saved objects returned from createPointInTimeFinderDecryptedAsInternalUser.

The above method will "strip" encrypted attributes (which is how we get into a state with token = undefined in the first place, generally) here:

kibana/x-pack/plugins/encrypted_saved_objects/server/saved_objects/index.ts

Lines 163 to 171 in a9c41e5

} catch (error) {

// catch error and enrich SO with it, return stripped attributes. Then consumer of API can decide either proceed

// with only unsecured properties or stop when error happens

const { attributes: strippedAttrs } = await service.stripOrDecryptAttributes(

descriptor,

savedObject.attributes as Record<string, unknown>

);

return { ...savedObject, attributes: strippedAttrs as T, error };

}

.
We need to surface this error property that's appended to the ESO object somewhere in Fleet setup.

Agree with this one. Right now we're ignoring the fact that ESO is returning an error (in the body) so we'll need to add a check for this in getDecryptedTokens. In the event that ESO returns an error, I think the expected behavior from UninstallTokenService should be to throw but from fleet setup we catch and log the error without blocking the rest of fleet setup (at least for now, since defend is the only integration using the feature right now).

kpollich · 2023-11-27T17:42:23Z

Unblocking Fleet setup with explicit errors messages available in logs is the way to go here. Even if we don't have any real logic, if we can detect the error from the ESO plugin and surface it in Fleet's setup logs that will be good enough here.

gergoabraham · 2023-11-27T19:04:32Z

@joeypoon @kpollich
34500d7
added a modification to surface the error returning from the encrypted SO client, now we got this error message:

👉 please approve the PR if it looks good, approval is needed for the merge

👉 @joeypoon I'd appreciate if you could merge the PR if it looks good, so it can make it before FF - I'm signing off for today

kpollich

Latest changes LGTM - thanks for addressing the feedback! 🚀

joeypoon

Looks good. Thanks for handling this one. Left a comment but I think it can be done as a follow up. Will watch and merge.

joeypoon · 2023-11-27T19:51:00Z

x-pack/plugins/fleet/server/services/security/uninstall_token_service/index.ts

-      ({ id: _id, attributes, created_at: createdAt }) => {
+      ({ id: _id, attributes, created_at: createdAt, error }) => {
+        if (error) {
+          throw new UninstallTokenError(`Error when reading Uninstall Token: ${error.message}`);


I think it may be safer to not have error.message within the error here since we don't own what's in the error and it may potentially contain sensitive data. I don't think it's blocking since from what I can tell looking at ESO code, they never expose the sensitive data in the error messages. But given that this is something that can change at anytime and ESO already logs these errors on their end, I think we should update this in a follow up to not contain error.message.

kibana-ci · 2023-11-27T20:21:28Z

💔 Build Failed

Failed CI Steps

Defend Workflows Cypress Tests, burning changed specs #1

Metrics [docs]

✅ unchanged

History

💔 Build #179238 failed e9f6f0b

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

cc @gergoabraham

… uninstall tokens (elastic#172058) ## Summary This PR is the `8.12` port of: - elastic#171998 The original PR was opened to `8.11` to make it faster to include it in `8.12.2`. Now this PR is meant to port the changes to `main`, so: - we can build upon it, - and can easily backport any further changes to `8.11.x` > [!Important] > The changes cannot be tested on `main` because they are hidden by other behaviours (namely the retry logic for reading Message SIgning key) that weren't part of `8.11`. Those behaviours will be also adapted in follow up PRs.

…eet setup (#172072) ## Summary When encryption key is rotated improperly, Message Signing key retrieval is retried infinitely, and because this is done in Fleet setup, none of the Fleet pages are loaded when the user tries to visit any of them. This PR re-configures the retry logic so it now attempts significantly less times than infinite. Also, it changes the errors to non-fatal from the Fleet setup point of view, similarly to these PRs: - #171998 - #172058 ## Reproducing the encryption key issue - setup a Kibana, add at least one policy (probably Fleet policy is enough) - modify (or add a new) encryption key in your `kibana.dev.yml`: ```yml xpack.encryptedSavedObjects.encryptionKey: "some-random-encryption-key-min-32-bytes" ``` ## Screenshots After ~15 sec of loading spinner, this is what the user sees: ![image](https://github.com/elastic/kibana/assets/39014407/2a29d0d9-4975-46b5-b662-bfbb6e888b0f) ### Checklist Delete any items that are not applicable to this PR. - [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

gergoabraham added 4 commits November 27, 2023 17:21

simplify token generation: no decrypt to avoid decryption errors

747ee00

do not allow enabling tamper protection without valid uninstall token

03ff478

fix UninstallTokenService mock

827a86c

add uninstall token issue to Fleet non-fatal setup errors

58bd53b

gergoabraham added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.11.2 labels Nov 27, 2023

gergoabraham requested review from joeypoon and dasansol92 November 27, 2023 16:27

gergoabraham self-assigned this Nov 27, 2023

gergoabraham marked this pull request as ready for review November 27, 2023 16:35

gergoabraham requested a review from kpollich November 27, 2023 16:41

kpollich reviewed Nov 27, 2023

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 27, 2023

improve wording for missing token error

e9f6f0b

kpollich mentioned this pull request Nov 27, 2023

[Encrypted Saved Objects] Multiple plugins experiencing errors with encrypted saved objects in 8.11.1 #171630

Closed

gergoabraham added 2 commits November 27, 2023 19:40

remove unnecessary todo comment

06a1ff3

surface error coming from encrypted SO client

34500d7

kpollich approved these changes Nov 27, 2023

View reviewed changes

joeypoon approved these changes Nov 27, 2023

View reviewed changes

joeypoon enabled auto-merge (squash) November 27, 2023 19:59

joeypoon merged commit 75cdadf into elastic:8.11 Nov 27, 2023

gergoabraham deleted the unblock-fleet-setup-when-cannot-decrypt-uninstall-tokens branch November 28, 2023 12:21

gergoabraham restored the unblock-fleet-setup-when-cannot-decrypt-uninstall-tokens branch November 28, 2023 12:21

gergoabraham mentioned this pull request Nov 28, 2023

[Defend Workflows][8.12 port] Unblock fleet setup when cannot decrypt uninstall tokens #172058

Merged

gergoabraham mentioned this pull request Nov 28, 2023

[Defend Workflows] Handling message signing decryption fail during Fleet setup #172072

Merged

1 task

gergoabraham deleted the unblock-fleet-setup-when-cannot-decrypt-uninstall-tokens branch December 5, 2023 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Defend Workflows][8.11] Unblock fleet setup when cannot decrypt uninstall tokens #171998

[Defend Workflows][8.11] Unblock fleet setup when cannot decrypt uninstall tokens #171998

gergoabraham commented Nov 27, 2023 •

edited

Loading

kpollich left a comment

kpollich Nov 27, 2023

gergoabraham Nov 27, 2023

elasticmachine commented Nov 27, 2023

kpollich commented Nov 27, 2023

gergoabraham commented Nov 27, 2023

kpollich commented Nov 27, 2023

joeypoon commented Nov 27, 2023 •

edited

Loading

kpollich commented Nov 27, 2023

gergoabraham commented Nov 27, 2023

kpollich left a comment

joeypoon left a comment

joeypoon Nov 27, 2023

kibana-ci commented Nov 27, 2023 •

edited

Loading

	} catch (error) {
	// catch error and enrich SO with it, return stripped attributes. Then consumer of API can decide either proceed
	// with only unsecured properties or stop when error happens
	const { attributes: strippedAttrs } = await service.stripOrDecryptAttributes(
	descriptor,
	savedObject.attributes as Record<string, unknown>
	);
	return { ...savedObject, attributes: strippedAttrs as T, error };
	}

[Defend Workflows][8.11] Unblock fleet setup when cannot decrypt uninstall tokens #171998

[Defend Workflows][8.11] Unblock fleet setup when cannot decrypt uninstall tokens #171998

Conversation

gergoabraham commented Nov 27, 2023 • edited Loading

Summary

Reproducing the encryption key issue

Checklist

kpollich left a comment

Choose a reason for hiding this comment

kpollich Nov 27, 2023

Choose a reason for hiding this comment

gergoabraham Nov 27, 2023

Choose a reason for hiding this comment

elasticmachine commented Nov 27, 2023

kpollich commented Nov 27, 2023

gergoabraham commented Nov 27, 2023

kpollich commented Nov 27, 2023

joeypoon commented Nov 27, 2023 • edited Loading

kpollich commented Nov 27, 2023

gergoabraham commented Nov 27, 2023

kpollich left a comment

Choose a reason for hiding this comment

joeypoon left a comment

Choose a reason for hiding this comment

joeypoon Nov 27, 2023

Choose a reason for hiding this comment

kibana-ci commented Nov 27, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

gergoabraham commented Nov 27, 2023 •

edited

Loading

joeypoon commented Nov 27, 2023 •

edited

Loading

kibana-ci commented Nov 27, 2023 •

edited

Loading