[Alerting] Updating Webhook with removed headers doesn't work as expected

[YulNaumenko]

Currently, deleting existing header in the Webhook connector doesn't work. At the same time you can update Webhook headers with the new header.

Steps to reproduce:

1. Create new Webhook connector in the Alerting and Actions page
2. Add a few headers and Save
3. Open created Webhook connector for Edit
4. Open Browser dev tool -> Network
5. Remove one of the existing headers and click Save
6. Find the PUT request which was sent for the current connector.
7. Check the response - it contains correct data for updated Webhook connector - header was removed.
8. Refresh the page and open the same Webhook connector.
9. Observe that removed header appears again.

Expected behavior:
Deleting Webhook headers should works.

Seems like some there is some caching issue on the server side for savedObjectClient.find

[elasticmachine]

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

[gmmorris]

I've investigated this issue and it seems fixing this is trickier than you'd expect.

The reason this happens is that Saved Objects are updated using partial updates and so, removing a field requires explicitly setting the field with a null.

This is straight forward enough with a field you want to remove, but in this case - it's a sub field. Specifically, we have a dictionary on the config to hold the various headers and setting a specific header to null would result in a dictionary with null values but real keys - which would get sent with the webhook.
We could address this by filtering them out in the webhook - but this seems like a broken implementation and is seriously unmaintainable. We're also very likely to encounter this problem with future Connectors and Actions.... in fact, we're probably going to have the exact same problem with Alerts in the future.

Looking into how we're supposed to do a full update, @joshdover suggested doing the following:

client.create(type, newAttributes, { id, overwrite: true })

This would essentially create a new object, but the overwrite:true will tell it to replace the existing document.
We could replace our updates with creates in both Actions and Alerts, to address this problem... except this leads us to a new problem.
The EncryptedSavedObjects have a built in limitation: if you try to create a document with an id (which we need in order to overwrite the existing document) then you get this error: Error: Predefined IDs are not allowed for saved objects with encrypted attributes.

It turns out that for security reason we don't allow this:

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

Lines 60 to 67 in 719c292

	// Saved objects with encrypted attributes should have IDs that are hard to guess especially
	// since IDs are part of the AAD used during encryption, that's why we control them within this
	// wrapper and don't allow consumers to specify their own IDs directly.
	if (options.id) {
	throw new Error(
	'Predefined IDs are not allowed for saved objects with encrypted attributes.'
	);
	}

---

My suggestion would be to do the following:

1. Add a allowExplicitIDs field to the EncryptedSavedObjectTypeRegistration which will allow us to override this limitation specifically for alert and action Saved Object types.

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

Lines 26 to 37 in 719c292

	/**
	* Describes the registration entry for the saved object type that contain attributes that need to
	* be encrypted.
	*/
	export interface EncryptedSavedObjectTypeRegistration {
	readonly type: string;
	readonly attributesToEncrypt: ReadonlySet<string | AttributeToEncrypt>;
	readonly attributesToExcludeFromAAD?: ReadonlySet<string>;
	}
	/**
	* Uniquely identifies saved object.

2. Use client.create(type, newAttributes, { id, overwrite: true }) in place of update for our updates.

But obviously we'd need @elastic/kibana-security and @elastic/kibana-platform to weigh in before we make such a change. :)

[legrego]

I wonder if we could instead address this by solving #50256. I need to give this some more thought (so I'm sorry for the half-baked idea), but could we instead update the ESO client's update function to perform the client.create(type, newAttributes, { id, overwrite: true }) on behalf of consumers? This would only impact saved objects that register themselves as encrypted, so all other types will still get the current partial-update behavior.

Since partial-updates are problematic for encrypted saved objects, I think it makes sense to prevent that from happening in the first place. We could try to be smart about this and only use client.create when fields that are part of the AAD are modified, but it might not be worth the effort and added complexity.

[gmmorris]

Yeah sure, I can take that approach, I assumed you wouldn't want that, but from our perspective that's even better (I think 🤔 )

If that approach works for you then I'll revisit my PR

[legrego]

I assumed you wouldn't want that, but from our perspective that's even better

Ah that makes me think I'm overlooking something nebulous 😄. Were you thinking I wouldn't want this approach because we'd be forcing full updates for encrypted types even if it wasn't strictly necessary?

(also cc @azasypkin, I'd love your input here as well)

[gmmorris]

Were you thinking I wouldn't want this approach because we'd be forcing full updates for encrypted types even if it wasn't strictly necessary?

Exactly 👍
That and the fact that it would mean that developers who don't know the internals of ESO might be surprised by ESO update differing from SO client's update.

[azasypkin]

Specifically, we have a dictionary on the config to hold the various headers and setting a specific header to null would result in a dictionary with null values but real keys - which would get sent with the webhook.

Sorry, I'm not sure I completely understand what is going here. Would you mind giving a bit more details, maybe with real example? What exactly we pass to update, what we get back, what gets stored/encrypted and where things break etc.?

I'm not opposed to the idea of using create, but I'm concerned that create and update operations may diverge more over time (new operation specific parameters, behaviors etc.) that may result into other subtle bugs in the future as SO APIs evolve.

[gmmorris]

@azasypkin basically, this is the case:

Say you have a SO with these attributes:

{
    config: {
        headers: {
            asd: 123
        }
    }
}

If you use update with these attributes with the intention of deleting the “asd” header and adding the "zxc" header:

{
    config: {
        headers: {
            zxc: 456
        }
    }
}

Your SO ends up as:

{
    config: {
        headers: {
            asd: 13,
            zxc: 456
        }
    }
}

If you want to unset asd then you need to update the SO with:

{
    config: {
        headers: {
            asd: null,
            zxc: 456
        }
    }
}

But that doesn't remove the asd field, it just sets it to null.
Also, we can't control the objects in Solutions that add Alert and Action types of their own, so this isn't a maintainable solution long term.

Platform team express that this is by design for the SavedObjects and that if we want to wipe the inner fields then we need to overwrite the whole doc.

[gmmorris]

I'm not opposed to the idea of using create, but I'm concerned that create and update operations may diverge more over time (new operation specific parameters, behaviours etc.) that may result into other subtle bugs in the future as SO APIs evolve.

I agree completely, but it doesn't sound like we have another option here, unless @elastic/kibana-platform have another suggestion.