Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypted Saved Objects can be partially updated and cause AAD issues #50256

Closed
mikecote opened this issue Nov 12, 2019 · 23 comments
Closed

Encrypted Saved Objects can be partially updated and cause AAD issues #50256

mikecote opened this issue Nov 12, 2019 · 23 comments
Labels
Feature:Saved Objects Feature:Security/Encrypted Saved Objects papercut Small "burr" in the product that we should fix. Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@mikecote
Copy link
Contributor

mikecote commented Nov 12, 2019

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
Copy link
Contributor

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

@mikecote
Copy link
Contributor Author

mikecote commented Sep 9, 2020

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

@gmmorris
Copy link
Contributor

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

@legrego
Copy link
Member

legrego commented Sep 14, 2020

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

@legrego legrego added Feature:Saved Objects Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! and removed Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Sep 14, 2020
@elasticmachine
Copy link
Contributor

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

@mikecote mikecote changed the title Encrypted Saved Objects can be partially updated, but shouldn't be, because of AAD issues Encrypted Saved Objects can be partially updated and cause AAD issues Sep 16, 2020
@mikecote
Copy link
Contributor Author

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

@gmmorris
Copy link
Contributor

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.

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 5, 2021
@azasypkin azasypkin added papercut Small "burr" in the product that we should fix. Feature:Security/Encrypted Saved Objects labels Aug 18, 2022
@legrego legrego removed EnableJiraSync loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 18, 2022
@pgayvallet
Copy link
Contributor

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
Copy link
Contributor Author

mikecote commented Jun 7, 2024

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
Copy link

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

@gmmorris
Copy link
Contributor

gmmorris commented Jun 7, 2024

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
Copy link

Agreed, I think we're all done here ✅

@legrego
Copy link
Member

legrego commented Jun 7, 2024

💚 I miss you folks

@gmmorris
Copy link
Contributor

gmmorris commented Jun 7, 2024

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

(miss you too 💚)

@alexfrancoeur
Copy link

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

@mikecote
Copy link
Contributor Author

mikecote commented Jun 7, 2024

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

Screenshot 2024-06-07 at 1 50 50 PM

@gmmorris
Copy link
Contributor

gmmorris commented Jun 7, 2024

I can't believe you kept that @mikecote 💚

@alexfrancoeur
Copy link

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
Copy link
Contributor

gmmorris commented Jun 7, 2024

We should hop on a zoom soon and catch up!

Yes!

@pgayvallet
Copy link
Contributor

The spam is strong with those ones

@jeramysoucy
Copy link
Contributor

jeramysoucy commented Oct 11, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects Feature:Security/Encrypted Saved Objects papercut Small "burr" in the product that we should fix. Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

9 participants