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

Implements 'Key concepts' developer documentation for Encrypted Saved Objects #184334

Merged
merged 13 commits into from
Jun 7, 2024

Conversation

jeramysoucy
Copy link
Contributor

Closes #176097

Summary

Adds 'Key concepts' developer documentation for Encrypted Saved Objects, covering the basic theory, usage, and maintenance for encrypted saved objects in Kibana.

@jeramysoucy jeramysoucy added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! DevDocs Feature:Security/Encrypted Saved Objects v8.15.0 labels May 28, 2024
@elasticmachine
Copy link
Contributor

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

@jeramysoucy jeramysoucy added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels May 28, 2024
@jeramysoucy jeramysoucy requested a review from a team May 28, 2024 09:50
@azasypkin azasypkin self-requested a review May 29, 2024 11:57
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I've left a few suggestions/questions.

dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
an Saved Object type, all related business logic for that type must be capable of gracefully and appropriately handling an object with or without the attribute in both
the current and previous version of Kibana. Some of the advice here is applicable to any Saved Object type migration.

| Change to ESO | Encrypted? | In AAD? | General Guidance |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is awesome ❤️ !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I hope it gets used! 🤞

dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
@jeramysoucy jeramysoucy requested review from azasypkin and a team June 4, 2024 09:30
@jeramysoucy
Copy link
Contributor Author

@elastic/kibana-core Is there anything we should add or change? We'd also like your general feedback on readability.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for adding real examples for encrypted and AAD attributes!

dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
dev_docs/key_concepts/encrypted_saved_objects.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Left a couple suggestions

Comment on lines +121 to +123
included values requires re-encryption, which means the entire object must be provided when updating to avoid corrupting the object. If an ESO is corrupted by
a partial update, it will be effectively undecryptable. Currently, there is nothing preventing or limiting partial updates of ESOs (see open GitHub issue
[50256](https://github.com/elastic/kibana/issues/50256)), so this requires consistent diligence from developers utilizing ESOs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated to the PR) TIL. I'll reply on the linked issue to reopen the discussion. Things have changed since 2019, we're now doing client-side updates, meaning that we do fetch the attributes from ES for partial updates, so this should be addressable (but maybe it requires encryption to be a first class citizen of the SOR first/.

Comment on lines +314 to +315
Saved Object Descriptor (see <DocLink id="kibDevDocsEncryptedSavedObjectsIntro" section="aad" text="AAD"/>). In theory, the space id will never change for a
`'multiple-isolated'`, which makes it a reasonable candidate for an AAD field.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also debated including some explanation regarding whether adding spaceId to AAD is offering any real protection in this case.

"...but it would also require some additional management to ensure it contributes to protecting the data. For example, if an object is moved to a different space maliciously (or accidentally) in order to try to gain access to its encrypted attributes, the spaceId attribute does not necessarily reflect the space the object resides in, and on its own provides no mechanism to guarantee the object has not moved. Some additional mechanism to override the attribute's value with the object's current namespace prior to attempting decryption would add protection."

But I don't know if this is just too much, or if what I am saying here is even feasible to implement.

cc @azasypkin @pgayvallet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to merge the doc as-is, and potentially address this in an update.

@jeramysoucy jeramysoucy merged commit 2c8201d into elastic:main Jun 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting DevDocs Feature:Security/Encrypted Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AAD inclusion guidance to encrypted saved object documentation
5 participants