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

[ESO] Add flag to allow ESO consumers to opt-out of highly random UIDs #198287

Merged
merged 15 commits into from
Nov 5, 2024

Conversation

SiddharthMantri
Copy link
Contributor

@SiddharthMantri SiddharthMantri commented Oct 30, 2024

Closes #194692

Summary

Allow consumers of ESOs to explicitly opt out of the strict highly random UID requirements while registering the ESO type

Description

The getValidId method was updated to allow consumers of Encrypted Saved Objects to explicitly opt-out of the enforced random ID requirement.

This change is added during ESO registration - consumers can now pass a new field to opt-out of random UIDs.

Additional changes

  • Updated canSpecifyID logic:
    • The canSpecifyID condition now also checks if enforceRandomId is explicitly set to false.
      This opt-out approach allows specific ESOs to bypass the random ID enforcement without affecting the default behavior, keeping it secure by default.

During the registration phase of the saved object, consumers can now specify if they'd like to opt-out of the random ID

savedObjects.registerType({
  name: TYPE_WITH_PREDICTABLE_ID,
 //...
});

encryptedSavedObjects.registerType({
  type: TYPE_WITH_PREDICTABLE_ID,
  //...
  enforceRandomId: false,
});

Release notes

Improves Encrypted Saved Objects (ESO) ID validation by adding an enforceRandomId parameter, allowing consumers to opt out of the default random ID requirement for specific use cases.

Checklist

For maintainers

@SiddharthMantri SiddharthMantri changed the title [ESO] Allow flag for creating predictable IDs on ESOs [ESO] Enforce random ID flag for consumers to use predictable IDs on ESOs Oct 30, 2024
@SiddharthMantri SiddharthMantri added Feature:Security/Encrypted Saved Objects release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Oct 30, 2024
@SiddharthMantri SiddharthMantri marked this pull request as ready for review October 30, 2024 11:02
@SiddharthMantri SiddharthMantri requested review from a team as code owners October 30, 2024 11:02
@elasticmachine
Copy link
Contributor

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

@SiddharthMantri
Copy link
Contributor Author

@elasticmachine merge upstream

@SiddharthMantri SiddharthMantri added the backport:skip This commit does not require backporting label Oct 30, 2024
@SiddharthMantri SiddharthMantri added backport v8.17.0 and removed backport:skip This commit does not require backporting labels Oct 30, 2024
@SiddharthMantri SiddharthMantri changed the title [ESO] Enforce random ID flag for consumers to use predictable IDs on ESOs [ESO] Add flag to allow ESO consumers to opt-out of highly random UIDs Oct 30, 2024
Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

Look great, Sid! I left some comments on test coverage, and one typo. Also some nits, which of course, feel free to ignore.

@@ -40,6 +40,10 @@ export class SavedObjectsEncryptionExtension implements ISavedObjectsEncryptionE
return this._service.isRegistered(type);
}

shouldEnforceRandomId(type: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should augment the unit tests in saved_objects_encryption_extension.test.ts to cover this function as well.

Trivial nit:

Suggested change
shouldEnforceRandomId(type: string) {
typeEnforcesRandomId(type: string) {

@SiddharthMantri
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
encryptedSavedObjects 46 47 +1
Unknown metric groups

API count

id before after diff
@kbn/core-saved-objects-server 562 564 +2
encryptedSavedObjects 53 54 +1
total +3

History

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

LGTM, did not test

@SiddharthMantri SiddharthMantri merged commit 56c0806 into elastic:main Nov 5, 2024
23 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11686452591

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 5, 2024
elastic#198287)

Closes elastic#194692

## Summary
Allow consumers of ESOs to explicitly opt out of the strict highly
random UID requirements while registering the ESO type

### Description

The `getValidId` method was updated to allow consumers of Encrypted
Saved Objects to explicitly opt-out of the enforced random ID
requirement.

This change is added during ESO registration - consumers can now pass a
new field to opt-out of random UIDs.

Additional changes

- Updated canSpecifyID logic:
- The canSpecifyID condition now also checks if enforceRandomId is
explicitly set to false.
This opt-out approach allows specific ESOs to bypass the random ID
enforcement without affecting the default behavior, keeping it secure by
default.

During the registration phase of the saved object, consumers can now
specify if they'd like to opt-out of the random ID

```
savedObjects.registerType({
  name: TYPE_WITH_PREDICTABLE_ID,
 //...
});

encryptedSavedObjects.registerType({
  type: TYPE_WITH_PREDICTABLE_ID,
  //...
  enforceRandomId: false,
});

```

### Release notes

Improves Encrypted Saved Objects (ESO) ID validation by adding an
enforceRandomId parameter, allowing consumers to opt out of the default
random ID requirement for specific use cases.

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Jeramy Soucy <[email protected]>
(cherry picked from commit 56c0806)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Nov 5, 2024
…om UIDs (#198287) (#198956)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ESO] Add flag to allow ESO consumers to opt-out of highly random
UIDs (#198287)](#198287)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Sid","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-05T14:40:53Z","message":"[ESO]
Add flag to allow ESO consumers to opt-out of highly random UIDs
(#198287)\n\nCloses
https://github.com/elastic/kibana/issues/194692\r\n\r\n##
Summary\r\nAllow consumers of ESOs to explicitly opt out of the strict
highly\r\nrandom UID requirements while registering the ESO
type\r\n\r\n### Description\r\n\r\nThe `getValidId` method was updated
to allow consumers of Encrypted\r\nSaved Objects to explicitly opt-out
of the enforced random ID\r\nrequirement.\r\n\r\nThis change is added
during ESO registration - consumers can now pass a\r\nnew field to
opt-out of random UIDs.\r\n\r\nAdditional changes\r\n\r\n- Updated
canSpecifyID logic:\r\n- The canSpecifyID condition now also checks if
enforceRandomId is\r\nexplicitly set to false.\r\nThis opt-out approach
allows specific ESOs to bypass the random ID\r\nenforcement without
affecting the default behavior, keeping it secure
by\r\ndefault.\r\n\r\n\r\nDuring the registration phase of the saved
object, consumers can now\r\nspecify if they'd like to opt-out of the
random ID\r\n\r\n```\r\nsavedObjects.registerType({\r\n name:
TYPE_WITH_PREDICTABLE_ID,\r\n
//...\r\n});\r\n\r\nencryptedSavedObjects.registerType({\r\n type:
TYPE_WITH_PREDICTABLE_ID,\r\n //...\r\n enforceRandomId:
false,\r\n});\r\n\r\n```\r\n\r\n\r\n### Release notes\r\n\r\nImproves
Encrypted Saved Objects (ESO) ID validation by adding
an\r\nenforceRandomId parameter, allowing consumers to opt out of the
default\r\nrandom ID requirement for specific use cases.\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Jeramy
Soucy
<[email protected]>","sha":"56c0806af5a7f20903e92bfe88dc227e93ca2858","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","backport","Team:Security","v9.0.0","Feature:Security/Encrypted
Saved Objects","v8.17.0"],"title":"[ESO] Add flag to allow ESO consumers
to opt-out of highly random
UIDs","number":198287,"url":"https://github.com/elastic/kibana/pull/198287","mergeCommit":{"message":"[ESO]
Add flag to allow ESO consumers to opt-out of highly random UIDs
(#198287)\n\nCloses
https://github.com/elastic/kibana/issues/194692\r\n\r\n##
Summary\r\nAllow consumers of ESOs to explicitly opt out of the strict
highly\r\nrandom UID requirements while registering the ESO
type\r\n\r\n### Description\r\n\r\nThe `getValidId` method was updated
to allow consumers of Encrypted\r\nSaved Objects to explicitly opt-out
of the enforced random ID\r\nrequirement.\r\n\r\nThis change is added
during ESO registration - consumers can now pass a\r\nnew field to
opt-out of random UIDs.\r\n\r\nAdditional changes\r\n\r\n- Updated
canSpecifyID logic:\r\n- The canSpecifyID condition now also checks if
enforceRandomId is\r\nexplicitly set to false.\r\nThis opt-out approach
allows specific ESOs to bypass the random ID\r\nenforcement without
affecting the default behavior, keeping it secure
by\r\ndefault.\r\n\r\n\r\nDuring the registration phase of the saved
object, consumers can now\r\nspecify if they'd like to opt-out of the
random ID\r\n\r\n```\r\nsavedObjects.registerType({\r\n name:
TYPE_WITH_PREDICTABLE_ID,\r\n
//...\r\n});\r\n\r\nencryptedSavedObjects.registerType({\r\n type:
TYPE_WITH_PREDICTABLE_ID,\r\n //...\r\n enforceRandomId:
false,\r\n});\r\n\r\n```\r\n\r\n\r\n### Release notes\r\n\r\nImproves
Encrypted Saved Objects (ESO) ID validation by adding
an\r\nenforceRandomId parameter, allowing consumers to opt out of the
default\r\nrandom ID requirement for specific use cases.\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Jeramy
Soucy
<[email protected]>","sha":"56c0806af5a7f20903e92bfe88dc227e93ca2858"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198287","number":198287,"mergeCommit":{"message":"[ESO]
Add flag to allow ESO consumers to opt-out of highly random UIDs
(#198287)\n\nCloses
https://github.com/elastic/kibana/issues/194692\r\n\r\n##
Summary\r\nAllow consumers of ESOs to explicitly opt out of the strict
highly\r\nrandom UID requirements while registering the ESO
type\r\n\r\n### Description\r\n\r\nThe `getValidId` method was updated
to allow consumers of Encrypted\r\nSaved Objects to explicitly opt-out
of the enforced random ID\r\nrequirement.\r\n\r\nThis change is added
during ESO registration - consumers can now pass a\r\nnew field to
opt-out of random UIDs.\r\n\r\nAdditional changes\r\n\r\n- Updated
canSpecifyID logic:\r\n- The canSpecifyID condition now also checks if
enforceRandomId is\r\nexplicitly set to false.\r\nThis opt-out approach
allows specific ESOs to bypass the random ID\r\nenforcement without
affecting the default behavior, keeping it secure
by\r\ndefault.\r\n\r\n\r\nDuring the registration phase of the saved
object, consumers can now\r\nspecify if they'd like to opt-out of the
random ID\r\n\r\n```\r\nsavedObjects.registerType({\r\n name:
TYPE_WITH_PREDICTABLE_ID,\r\n
//...\r\n});\r\n\r\nencryptedSavedObjects.registerType({\r\n type:
TYPE_WITH_PREDICTABLE_ID,\r\n //...\r\n enforceRandomId:
false,\r\n});\r\n\r\n```\r\n\r\n\r\n### Release notes\r\n\r\nImproves
Encrypted Saved Objects (ESO) ID validation by adding
an\r\nenforceRandomId parameter, allowing consumers to opt out of the
default\r\nrandom ID requirement for specific use cases.\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Jeramy
Soucy
<[email protected]>","sha":"56c0806af5a7f20903e92bfe88dc227e93ca2858"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Sid <[email protected]>
*/
public shouldEnforceRandomId(type: string) {
const typeDefinition = this.typeDefinitions.get(type);
return typeDefinition?.enforceRandomId !== false;
Copy link
Member

Choose a reason for hiding this comment

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

Tip

Optional and not that important, but I’m wondering if we should throw an error here if type isn’t registered (isEncryptableType(type) !== true)? It’s unlikely to happen in practice since we expect consumers of this API to call isEncryptableType first, but it still feels a bit odd that this function would return true for non-ESOs if someone mistakenly uses it in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@azasypkin That's a good point! I think i'll address it in a follow-up PR.

I was also thinking about the scenario where a type explicitly opts out but no ID is provided during creation. In it's current implementation, a random ID will be created but I was wondering if it should behave differently?

Copy link
Member

Choose a reason for hiding this comment

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

@azasypkin That's a good point! I think i'll address it in a follow-up PR.

Thanks!

I was also thinking about the scenario where a type explicitly opts out but no ID is provided during creation. In it's current implementation, a random ID will be created but I was wondering if it should behave differently?

Hmm, I think the current behavior is correct and works the same for both ESO and non-ESO saved objects. I interpret enforceRandomId as "prevent consumers from explicitly specifying non-random IDs, but keep the default behavior if they don’t specify IDs".

Copy link
Contributor

Choose a reason for hiding this comment

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

++ Yeah, if an type has opted out of enforcing random IDs but an ID is not provided during creation, a random ID should be generated.

mgadewoll pushed a commit to mgadewoll/kibana that referenced this pull request Nov 7, 2024
elastic#198287)

Closes elastic#194692

## Summary
Allow consumers of ESOs to explicitly opt out of the strict highly
random UID requirements while registering the ESO type

### Description

The `getValidId` method was updated to allow consumers of Encrypted
Saved Objects to explicitly opt-out of the enforced random ID
requirement.

This change is added during ESO registration - consumers can now pass a
new field to opt-out of random UIDs.

Additional changes

- Updated canSpecifyID logic:
- The canSpecifyID condition now also checks if enforceRandomId is
explicitly set to false.
This opt-out approach allows specific ESOs to bypass the random ID
enforcement without affecting the default behavior, keeping it secure by
default.


During the registration phase of the saved object, consumers can now
specify if they'd like to opt-out of the random ID

```
savedObjects.registerType({
  name: TYPE_WITH_PREDICTABLE_ID,
 //...
});

encryptedSavedObjects.registerType({
  type: TYPE_WITH_PREDICTABLE_ID,
  //...
  enforceRandomId: false,
});

```


### Release notes

Improves Encrypted Saved Objects (ESO) ID validation by adding an
enforceRandomId parameter, allowing consumers to opt out of the default
random ID requirement for specific use cases.

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Jeramy Soucy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax the ESO UUID-ID requirement for the rules SO
6 participants