Allow encrypted saved object types to support predefined ids #42762
Conversation
kobelb
changed the title
kobelb
added
the
Team:Security
|
Pinging @elastic/kibana-security |
kobelb
added
release_note:plugin_api_changes
|
/cc @mikecote |
💚 Build Succeeded |
|
ACK: will review today or the first thing tomorrow morning (likely). |
There was a problem hiding this comment.
LGTM, thanks! Would you mind also adding a quick note about this new option to the plugin's README.md?
| @@ -107,6 +108,15 @@ export class EncryptedSavedObjectsService { | |||
| return this.typeRegistrations.has(type); | |||
| } | |||
|
|
|||
| public allowPredefinedID(type: string) { | |||
There was a problem hiding this comment.
optional nit: would you mind adding a brief JSDoc to this method. The name is clear enough, but the fact that this method always returns true for non-registered types may not be obvious without reading the method body.
| ); | ||
| // since IDs are part of the AAD used during encryption. Types can opt-out of this restriction, | ||
| // when necessary, but it's much safer for this wrapper to generate them. | ||
| if (!this.options.service.allowPredefinedID(type) && options.id) { |
There was a problem hiding this comment.
nit: would you mind swapping options.id and this.options.service.allowPredefinedID(type) here and in bulkCreate so that there is a higher chance that JS engine can optimize code and doesn't call allowPredefinedID() when options.id isn't specified?
💚 Build Succeeded |
|
We don't need this at the moment, but I don't want to get rid of this PR quite yet. Feel free to just ignore it for the time being. |
|
@kobelb We are getting a similar issue in Fleet where we want to encrypt saved object with predefined ids, is there any plans to move forward with this PR? |
|
@nchaulet, if it's a requirement for Fleet we can resurrect this PR. Alerting ended up not needing it, so we abandoned it. Is there code or technical specs for what Fleet is doing here? Allowing predefined IDs could potentially be a security issue based on how they're consumed and used. |
|
@kobelb no there is no issue, (I should add doc about that) but trying to resume: |
|
Going to resurrect this PR as it's required for audit logging of alerts/actions |
Task Manager would like to use predefined IDs with encrypted saved objects. We were previously insisting on generating the ID within the
EncryptedSavedObjectsClientWrapperso that we could use a UUID v4. This restriction was put in place because generally some other saved object has a reference to an "encrypted saved object" and we wanted to reduce the likelihood of someone potentially being able to guess the reference ID and use it for a nefarious purpose. Instead of relaxing this constraint for all saved object types used with the encrypted saved objects plugin, this PR allows certain saved object types to opt-out of this protection.Resolves: #42688
"Release Note: Consumers of the Encrypted Saved Objects plugin can register types which allow predefined IDs to be specified"