[alerting] decrypt errors during migration yield unmigrated alert saved objects

[pmuellr]

This code in the alerting migration looks a bit suspect to me:

kibana/x-pack/plugins/alerting/server/saved_objects/migrations.ts

Lines 84 to 103 in 15ab9d7

	function executeMigrationWithErrorHandling(
	migrationFunc: SavedObjectMigrationFn<RawAlert, RawAlert>,
	version: string
	) {
	return (doc: SavedObjectUnsanitizedDoc<RawAlert>, context: SavedObjectMigrationContext) => {
	try {
	return migrationFunc(doc, context);
	} catch (ex) {
	context.log.error<AlertLogMeta>(
	`encryptedSavedObject ${version} migration failed for alert ${doc.id} with error: ${ex.message}`,
	{
	migrations: {
	alertDocument: doc,
	},
	}
	);
	}
	return doc;
	};
	}

It appears that if an ESO decrypt error occurs during the execution of the migration of rules, the code falls into the catch logic, and so the error is logged, but the document in returned unchanged - not actually migrated. We've seen that logged message now in a user's deployment.

I believe the proper course of action in this case is to "fix" the decryption error by removing the API key, essentially disabling the rule, which should allow it to be migrated. This has the unfortunate effect of disabling a rule that was by definition not disabled before - but then presumably it wasn't running either, since it has a decrypt error. The other option is to not migrate it during normal migration, but instead migrate it later when it's actually needed - but that seems a lot harder.

I guess the other case where this can happen is if the encryption key is changed at the same time the migration takes place, in which case every rule and connector would be broken because of decryption errors. That almost seems worthy of failing the migration completely, but it's also not clear how we recognize that state compared to just a handful of "broken" ESOs.

Note that we can't simply replace the API key in the rule, since that requires a user context to create the API key, and during migration, we won't have one. We'd have to remove it, by "disabling" the rule.

The actions migration is very similar:

kibana/x-pack/plugins/actions/server/saved_objects/migrations.ts

Lines 56 to 75 in b740640

	function executeMigrationWithErrorHandling(
	migrationFunc: SavedObjectMigrationFn<RawAction, RawAction>,
	version: string
	) {
	return (doc: SavedObjectUnsanitizedDoc<RawAction>, context: SavedObjectMigrationContext) => {
	try {
	return migrationFunc(doc, context);
	} catch (ex) {
	context.log.error<ActionsLogMeta>(
	`encryptedSavedObject ${version} migration failed for action ${doc.id} with error: ${ex.message}`,
	{
	migrations: {
	actionDocument: doc,
	},
	}
	);
	}
	return doc;
	};
	}

Remediation is worse though, since we'll lose the secrets. But the good news is we do have a story for "actions that are missing secrets via export/import" - which is presumably how we'd set these up after the migration completes.

One thing I'm not sure about, is how to get this to work with the current ESO migration. It's a framework call, we just pass in functions to morph the raw saved object shapes, we don't have any way of handling decrypt errors differently than anything else (and what do we do when "anything else" errors occur!?). Feels like we'll need to beef up the ESO createMigration() function with some kind of error handling function, that would let us do some shape changes on the raw objects (to disable the rule, or mark the connector as "needs secrets"), and continue with the migration instead of failing.

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[pmuellr]

During conversation on this issue yesterday, I happened to think that I remember seeing Kibana logs that seemed to indicate it was possible to have saved objects migrated "as needed" somehow. Not sure if this is still something we can do - I'm not seeing the message right now in my dev env, and can't remember exactly what it said - but something about "not doing migrations now, will migrate saved objects on read" or something. But if this capability does exist, perhaps we don't really have an issue?

I suspect even if it is true, there will be some gotcha - we still can't really update the encryption key during one of these "as needed" migrations (I'm guessing here). But if there is such a capability, perhaps we could make use of it in whatever the solution for this is - like the migration could leave it in a state where an "as needed" migration would be possible.

[mikecote]

Adding to To-Do so we can better understand the issue and what can be done to fix this, and if we should prioritize something further to fix the problem.

[pmuellr]

I just added a research tag - I think we need to understand if this is really a problem, how we plan on fixing it (it may require changes to the ESO migration code), and if there are work-arounds to prevent this from happening, or fix things after this breaks something.

[ymao1]

After some research, it seems that the original decision to add the try/catch block in the migrations code stemmed from the span of time when alerting functionality was in beta and made use of the ephemeral encryption key. The thought was that we did not want to penalize users who were trying out a beta functionality by stopping Kibana upgrades due to failure to migrate their beta rules (which they may or may not actually be using) that used an ephemeral encryption key.

Edit After a discussion with @mikecote, it looks like the fear of having users that created rules with an ephemeral encryption key was unfounded as the PR to block this ability was merged before alerting beta was released.

OTOH, the recommended behavior from @elastic/kibana-core, as stated in this comment on the Saved Objects Migration RFC and from offline discussions, is that a migration failures should cause a failed upgrade, allowing users to immediately roll back to the previous version. This is considered more desired behavior than having the upgrade succeed with unmigrated saved objects that may cause other issues at a future time that are more difficult to trace.

I see three options here, although I don't think (1) is the right approach, so for the purposes of this discussion, I will only be focusing on (2) and (3).

(1) Disable saved object when decryption errors occur

Instead of returning the un-migrated document on decryption error, we can disable the rule/connector and apply the migration function to the disabled saved object. I think this is the least desirable option because for rules, if we want to fully and correctly disable a rule on failed migration without leaving any dangling references, we should also be removing the scheduled task associated with the rule and invalidating the API key. It feels kind of weird and wrong to be doing these things during a migration.

(2) Fail the upgrade when decryption errors occur

Upgrade fails with properly logged decryption errors. Log messages can be informative and tell users to check their encryption keys Users are able to roll back to previous Kibana version and fix their encryption key mismatch.

(3) Apply the migration on the unencrypted document

Even if decryption fails, apply the migration function. This would result in a correctly transformed document that will error when it runs post-migration due to broken AAD.

---

I see the following possible scenarios that might lead to decryption failures during migrations:

User unintentionally connects a Kibana with a different encryption key to a cluster AND upgrades this spurious Kibana, triggering a migration.

With option (2), the spurious Kibana would attempt to migrate rules and connectors and fail. Normal cluster would be unaffected.

With option (3), the spurious Kibana would attempt to migrate rules and connectors and succeed. Rules and connectors on the normal cluster would start failing due to broken AAD. Errors would show up in rule management UI but action execution errors would only show up in the logs. It would likely be unintuitive for user to track down why their rules and connectors were erroring. User would either need to roll back their accidental upgrade. However, there's the question of whether the cluster's Kibana would work at all since all saved objects, not just rules & connectors would have been migrated by the spurious Kibana.

Encryption key changed during upgrade (accidentally or intentionally).

With option (2), the upgrade would fail with log messages indicating to the admin/user to verify the correct encryption key was used and/or add old encryption key to decryption only config. If the configs are fixed, upgrade can be tried again.

With option (3), the upgrade would succeed but all rules and connectors on the normal cluster would start failing due to broken AAD. Errors would show up in rule management UI but action execution errors would only show up in the logs. It would likely be unintuitive for user to track down why their rules and connectors were erroring. User would either need to roll back their upgrade with a fixed encryption key or disable/reenable all their rules and re-enter secrets for all their connectors.

User changed encryption key during upgrade and did not back up the key

With option (2), the upgrade would fail with log messages indicating to the admin/user to verify the correct encryption key was used and/or add old encryption key to decryption only config. If the user did not back up the key, they cannot complete the upgrade unless they delete all the rules and connectors

With option (3), the upgrade would succeed but all rules and connectors would start failing due to broken AAD. Errors would show up in rule management UI but action execution errors would only show up in the logs. It would likely be unintuitive for user to track down why their rules and connectors were erroring. User would need to disable/reenable all their rules and re-enter secrets for all their connectors.

User has old rules and connectors using outdated encryption keys

This could happen if user tried out alerting but isn't actively using any rules or connectors. There could be rules running and continuously erroring, but they don't care and haven't cleaned them up.

With option (2), the upgrade would fail and the user might be confused as to why rule and connector migrations are blocking the upgrade when they don't use rules or connectors. Hopefully the log messages should provide enough context for them to delete these rules and connectors that were erroring anyway. However, since we've been catching decryption errors since 7.9 and returning unmigrated documents, it is possible that this will cause many many upgrade failures the first version after we change this behavior. All of the user's old rules/connectors could come back to haunt them.

With option (3), the migration would succeed. Rules and connectors that were erroring before would continue to error. User would never know and never need to know.

User has active working rules AND old rules and connectors using outdated encryption keys

User only uses security rules in the security UI but also has some older rules and connectors using outdated encryption keys that they never see errors for because they don't use the stack rules management page.

With option (2), the upgrade would fail and the user might be confused as to why. Hopefully the log messages should provide enough context for them to delete these rules and connectors that were erroring anyway. In the worst case, they might think that the migration failures are on their active rules/connectors and delete all rules and connectors.

With option (3), the migration would succeed. Active rules and connectors would continue to work and rules that were erroring before would continue to error.

---

[ymao1]

It seems to me that the worst case scenario for option (2) if a user actively using rules tries to upgrade and rotate encryption keys without backing up their old key. In this scenario, they would have to delete their rules and connectors in order to finish the upgrade.

The worst case scenario for option (3) seems to be if a user changes their encryption key during the upgrade (accidentally or forgets to set the decrypt only config), all their rules and connectors would be migrated but broken due to AAD. It may take some time for these broken rules/connectors to get noticed, take some more time to figure out why they are broken, and then they either need to revert the upgrade and redo it with the correct encryption key or spend time disabling and reenabling and reentering secrets. This could be very tedious if they have a lot of rules and connectors.

I think at this time I'm leaning toward option (3) as it is the option that would not lead to potential data loss, but I'm interested to know what others on @elastic/kibana-alerting-services think