Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135602

ymao1 · 2022-06-30T21:45:00Z

Summary

This PR fixes a bug introduced in 8.3.0 which causes any rule created or updated by an 8.2.x cluster to fail on migration on upgrade to 8.3.0 or 8.3.1.

The root cause of the bug is that the snoozeEndTime field which was added in 8.2.0 and subsequently removed in 8.3.0 was omitted from the AlertAttributesExcludedFromAAD list, which meant they could no longer be decrypted.
As a result, any rule Saved Object which was created or updated in 8.2.x would try to use the snoozeEndTime field to decrypt the rule as part of the 8.3.0 migration - which consistently fails as the SO uses the field (even if empty) as part of AAD.

When this migration fails for such a rule the following error message is visible in the server log:

Failed to decrypt "apiKey" attribute: Unsupported state or unable to authenticate data

Once Kibana runs it might still try to run these failed rules, which should result in an error such as this:

<rule-type>:<UUID>: execution failed - security_exception: [security_exception] Reason: missing authentication credentials for REST request [/_security/user/_has_privileges], caused by: ""

Remediation for customers who have already upgraded to 8.3.0 or 8.3.1

If a customer upgrades to 8.3.0 or 8.3.1 and their alerting rules have stopped running with an error similar to the example above, they will need to use the Update API key operation in Stack Management > Rules and Connectors to restore the alerting rule to a functional state. It should be noted that the Update API Key operation causes the alerting rule to run as the user that performed the operation.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

ymao1 · 2022-06-30T21:48:12Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/migrations.ts

+      expect(response.body._source?.alert?.snoozeSchedule).not.to.be(undefined);
+
+      const snoozeSchedule = response.body._source?.alert.snoozeSchedule!;
+      expect(snoozeSchedule.length).to.eql(1);


Ideally, we should add expect(response.body._source?.alert?.apiKey).not.to.be(undefined);

ymao1 · 2022-06-30T21:49:44Z

x-pack/test/functional/es_archives/alerts/data.json

+           "throttle":null,
+           "notifyWhen" : "onActiveAlert",
+           "apiKeyOwner" : "elastic",
+           "apiKey" : "y86eFOuG06GNd4p/90Zh4V4C+4wF8rFHiLcOeNqHQFFAbagYCXIuggq/N/AB0HBncCsWrh5PsCs4RiX53+LP2o+oLtoQkdTK9m+iv0BnzP6aavoypIudKtnElN3SotI3XpIRm7c3MEdM/C/zZXHgCmn4UMExhkulOO1S/Q//FwvIr2/SKw/mp8Vx5qP41hqDX4YFfSMoFkPSLw==",


I think there is a correct-er way to generate this data so that the api key can be decrypted by the functional test server. This is just one I've copied from local data and it gives a decryption error during the test :(

x-pack/plugins/alerting/server/saved_objects/index.ts

gmmorris · 2022-07-02T15:44:56Z

x-pack/test/functional/es_archives/alerts/mappings.json

+            "notifyWhen": {
+              "type": "keyword"
+            },


Why is this necessary here? Am I missing something?

Any field that's inside a document in data.json needs to have a mapping defined. It just so happened that none of the other rule SO examples in data.json had notifyWhen but the SO I just added does, so I added the mapping. It's not strictly needed for this PR, I could remove it from the new doc I added to data.json

ahhh gotcha.
Thanks Ying.... enjoy the weekend! ;)

Co-authored-by: Gidi Meir Morris <github@gidi.io>

kibana-ci · 2022-07-03T19:41:46Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💛 Build #54791 was flaky dc06a8b

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

gmmorris · 2022-07-04T08:34:28Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/migrations.ts

@@ -443,5 +443,22 @@ export default function createGetTests({ getService }: FtrProviderContext) {
      expect(response.statusCode).to.equal(200);
      expect(response.body._source?.alert?.tags).to.eql(['test-tag-1', 'foo-tag']);
    });
+
+    it('8.3.0 removes snoozeEndTime in favor of snoozeSchedule', async () => {


This test doesn't seem to be working.
I commented out the return of the snoozeEndTime field in the AlertAttributesExcludedFromAAD variable and this test is still passing - it should be failing.

I know there's more work to be done by @mikecote today, so this might be a known issue, but I wanted to flag just in case.

Yes, this is because the migration will succeed even if decryption fails. I left a comment that we should add an expect here to ensure the api key still exists after migration but that can't be done until this (loading the test data in the correct manner)

Brilliant, thanks, I'll work with Mike to get that sorted asap.

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD

dc06a8b

ymao1 commented Jun 30, 2022

View reviewed changes

gmmorris reviewed Jul 2, 2022

View reviewed changes

x-pack/plugins/alerting/server/saved_objects/index.ts Outdated Show resolved Hide resolved

gmmorris reviewed Jul 2, 2022

View reviewed changes

x-pack/plugins/alerting/server/saved_objects/index.ts Outdated Show resolved Hide resolved

gmmorris reviewed Jul 2, 2022

View reviewed changes

Apply suggestions from code review

4348163

Co-authored-by: Gidi Meir Morris <github@gidi.io>

gmmorris reviewed Jul 4, 2022

View reviewed changes

gmmorris added v8.4.0 v8.3.2 labels Jul 4, 2022

mikecote mentioned this pull request Jul 4, 2022

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135663

Merged

9 tasks

ymao1 closed this Sep 21, 2022

ymao1 deleted the alerting/restore-snoozeEndTime branch September 21, 2022 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135602

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135602

ymao1 commented Jun 30, 2022 •

edited by gmmorris

Loading

ymao1 Jun 30, 2022 •

edited

Loading

ymao1 Jun 30, 2022

gmmorris Jul 2, 2022

ymao1 Jul 3, 2022

gmmorris Jul 3, 2022

kibana-ci commented Jul 3, 2022

gmmorris Jul 4, 2022

ymao1 Jul 4, 2022

gmmorris Jul 4, 2022

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135602

Restoring snoozeEndTime to AlertAttributesExcludedFromAAD #135602

Conversation

ymao1 commented Jun 30, 2022 • edited by gmmorris Loading

Summary

Remediation for customers who have already upgraded to 8.3.0 or 8.3.1

Checklist

Risk Matrix

For maintainers

ymao1 Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

ymao1 Jun 30, 2022

Choose a reason for hiding this comment

gmmorris Jul 2, 2022

Choose a reason for hiding this comment

ymao1 Jul 3, 2022

Choose a reason for hiding this comment

gmmorris Jul 3, 2022

Choose a reason for hiding this comment

kibana-ci commented Jul 3, 2022

💚 Build Succeeded

Metrics [docs]

History

gmmorris Jul 4, 2022

Choose a reason for hiding this comment

ymao1 Jul 4, 2022

Choose a reason for hiding this comment

gmmorris Jul 4, 2022

Choose a reason for hiding this comment

ymao1 commented Jun 30, 2022 •

edited by gmmorris

Loading

ymao1 Jun 30, 2022 •

edited

Loading