Encrypted Saved Objects can be partially updated and cause AAD issues

[mikecote]

The alerting team has been caught a few times updating encrypted attributes while forgetting to provide all the values for AAD. This issue would be to enhance this developer experience when such scenario is encountered.

The alerting team is still planning to leverage partial updates to update attributes excluded from AAD (see: #76830).

Original description

Fix ESO to not do partial update. See: https://github.com//pull/40694#pullrequestreview-261268183

[elasticmachine]

Pinging @elastic/kibana-stack-services (Team:Stack Services)

[mikecote]

@pmuellr @gmmorris this issue is pretty old, we can probably close this and mark as similar to #71995?

[gmmorris]

hmm not sure... tbh this feels like a Security team issue, not an Alerting one 🤔
@legrego ?

[legrego]

Yeah let's leave open for now. I'll re-label as a security team issue

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[mikecote]

Thanks @legrego, I've updated the title and description to better reflect the problem.

[gmmorris]

I just want to note a use case we have in Alerting:

There are times when we need to partially update fields - such as situation where we don't want to have to decrypt the encrypted fields (if, for example, we know that encryption has broken and want to mark this SO somehow).
This is limited to fields that are not in AAD though.

Ideally for us the type system could somehow allow us to either update non AAD fields or the whole object, but never partially update a field that's part of AAD. We're happy for this to be enforced through generics or something like that (so it wouldn't necessarily throw, but it would mean that if anyone wanted to break it they would have to go out of their way to cheat the compiler - in which case, they are taking responsibility for breaking things.

[pgayvallet]

Situation might have changed since this issue was open:

Because of BWC reasons, we are now performing a "client-side" update for the SOR update and bulkUpdate operations, meaning that we are resolving the current document, updating the attributes, then re-indexing the whole result.

Which means, we have the whole set of attributes during an update, so we could use the current attributes for any encryption/AAD recomputation we may need, even in case of partial updates.

Not sure if this could be achieved in the current state of the code, or if it would required ESO to be integrated directly into the SOR (as we've been discussing for some time now).

[mikecote]

For the alerting team, this is no longer an issue as we moved to using index operations instead of update a while ago. As long as the new way the SOR does this uses index, I think the issue goes away.

[alexfrancoeur]

I don't know, I really think we need to check in with @gmmorris first.

[gmmorris]

I don't know, I really think we need to check in with @gmmorris first.

Bin it.
If its a real requirement it'll come up again.

[alexfrancoeur]

Agreed, I think we're all done here ✅

[legrego]

💚 I miss you folks

[gmmorris]

Good thing we were here @legrego, who know what you would have got to up to otherwise 😉

(miss you too 💚)

[alexfrancoeur]

Miss y'all too! Hope everyone is well 😎 :elastic-heart: :alex-approved:

[mikecote]

Well well well, here we are again with @gmmorris and @alexfrancoeur debating priorities 😆

[gmmorris]

I can't believe you kept that @mikecote 💚

[alexfrancoeur]

Bahahha that's amazing.

I'll defer to @pgayvallet here, but according to @mikecote the alerting team is all set. Feels like we could close this out for now.

I also intend to continue to provide extremely valuable input like this on other random Kibana issues when it makes sense 😊

We should hop on a zoom soon and catch up!

[gmmorris]

We should hop on a zoom soon and catch up!

Yes!

[pgayvallet]

The spam is strong with those ones

[jeramysoucy]

@mikecote Looks like it was recently discussed this summer, but can this be closed now that the SOR update function includes a get prior to updating?

[mikecote]

@jeramysoucy yup, since the SOR performs an index operation for updates, we are no longer exposed to the merging performed on Elasticsearch's end. 👍 for this to be closed now.

[jeramysoucy]

Closing - resolved by changes to SOR update, which now gets the raw SO doc before applying incoming attribute changes.