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

[Saved Objects] Import hook/interceptor for saved object management #109189

Closed
FrankHassanabad opened this issue Aug 19, 2021 · 14 comments
Closed
Labels
dependencies Pull requests that update a dependency file Feature:Detection Alerts Security Solution Detection Alerts Feature Feature:Detection Rules Anything related to Security Solution's Detection Rules Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Detections and Resp Security Detection Response Team Team:Security Solution Platform Security Solution Platform Team

Comments

@FrankHassanabad
Copy link
Contributor

FrankHassanabad commented Aug 19, 2021

Parent meta:
#109169

Describe the feature:
Right now within the Kibana core saved object management's onImport, we cannot subtract invalid data objects, we cannot do data validations, and we cannot update fields such as updated_by, updated_at. This allows end users to enter invalid data after they export data and then do an import of that invalid data which can cause unexpected UI breakages or other problems we want to avoid.

Also, we have an original import format for security_solution which does not contain the same data shape and data types as the saved object format. This format is based on a compact JSON format and we would like to retain backwards compatibility for a while if at all possible.

Describe a specific use case for the feature:

  1. As an end user (Security Platform Engineer), when I import rules I need the fields of updated_by, updated_at to be capable of being updated to the name of the engineer doing the import and the time of day.

  2. As an end user (Security Platform Engineer), I need data validation to ensure that invalid combinations of rule parameters are not entered.

    • This includes if as an end user I do an export and then manually update the file on disk or through scripting and then attempt a re-import I would like to know that I did not invalidate or mess up the data set.
    • This include immutable rules to not be allowed to be re-imported with changes. Immutable rules are a business rule within security_solution where we have pre-packaged rules which are updated through cloud services and are not directly and easily allowed to be mutated.
  3. As an end user (Security Platform Engineer), I need UI feedback of any invalid rules, skipped rules, skipped exceptions, skipped exception list items, or unavoidable dangling issues due to data problems during the import. Caveat is that we do not want and do not need all things in memory loaded. If we cannot validate without looking at other objects we can skip those data type validations and rely on the UI/UX to do validations later during runtime.

  4. As an end user (Security Platform Engineer), I would like to import the older export format from security_solution directly within this UI/UX screen and not have errors and it will be backwards compatible. It would be nice if I got a deprecated message that my older format will no longer work eventually and I need to re-do an export after an import to upgrade the format. ⚠️ Stretch goal (not a blocker)

Similar ticket but this one is just for data type and shape validation:
#104088

** Ticket about how we should move to the SOM for the rules import/export**:
#82537

@FrankHassanabad FrankHassanabad added the Team:Detections and Resp Security Detection Response Team label Aug 19, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@FrankHassanabad FrankHassanabad added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Aug 19, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@FrankHassanabad FrankHassanabad added Feature:Saved Objects Feature:Detection Alerts Security Solution Detection Alerts Feature labels Aug 19, 2021
@FrankHassanabad FrankHassanabad changed the title Import hook/interceptor for saved object management [Saved Objects] Import hook/interceptor for saved object management Aug 19, 2021
@pgayvallet
Copy link
Contributor

Also, we have an original import format for security_solution which does not contain the same data shape and data types as the saved object format. This format is based on a compact JSON format and we would like to retain backwards compatibility for a while if at all possible.

I'm not sure to see the implication of this statement regarding the feature you're requesting, could you elaborate? Also, if the format works for you, what's the global purpose of trying to have your objects listed in SOM and import/exportable via the global export API?

Regarding the onImport hook, when we designed the import/export hooks, we came to the conclusion, given the identified use cases, that allowing object mutation during export only was sufficient.

The thing is, technically, the import objects (from the import file) can be of any version. The migration is performed during the calls to SOR.create, and the import hooks are invoked after, with the definitive, migrated, objects.

Allowing to mutate the objects during the import would be very tedious. AFAIK there isn't really a correct solution here. We would need to migrate the documents explicitly instead of delegating that to the create call. The implication with the conflict checking algorithm may be tricky too.

Before we did further in this direction, I would really like to be sure there is no alternative to what you're proposing

cc @rudolf @kobelb would like your thoughts too

@kobelb
Copy link
Contributor

kobelb commented Aug 19, 2021

I agree with your thoughts @pgayvallet. During previous discussions, we decided that we should minimize the logic that plugins could perform because there's an inherent risk in performing a lot of logic at this step because it could cause imports to fail partway through, which makes the import logic a lot more complicated, and we'd have to work through all of those situations. As such, I think it'd be helpful to explore alternatives to adding all of this functionality to saved-object import.

@rudolf
Copy link
Contributor

rudolf commented Aug 20, 2021

I agree with the idea that imports need to be validated and that this includes business rule validation, not just simple schema validation. We need more teams in Kibana to think defensively about all user input including the HTTP APIs (not so much for security, but for supportability and helping users avoid difficult to recover from situations and not least preventing migrations from failing due to corrupted documents).

At the moment our architecture depends on doing mutation and potential validation on a document-per-document way. Reading between the lines, it sounds like some of your business rules might require you to have all the objects being imported at once so that you can e.g. apply business rules that span across more than one object? (Even if this is not an immediate ask, it feels like it's just a matter of time before we encounter a business domain or document model that requires this).

This document-for-document design comes from trying to future-proof imports so that we could eventually stream them to support large files. Loading it into memory for validation would block us from achieving this if we ever had to import gigabytes of data. We're not aware of a need to support such large files yet, so this might not even be necessary in the next 12 months, but being forced to load an entire import into memory feels like a likely future risk.

Given this streaming design and @kobelb's point about reducing the risk of import failures. I wonder if the best strategy might be do something similar to what we're doing with alerting where a simple onImport hook sets a disabled: true property on each document. There would then be an async process to actually validate these disabled objects before enabling them. If, after the import, we prompt the user to click a button to start the process, the user could be taken to the plugin's application which could provide a much more tailored experience (plugins might just show a list of failures or provide an interactive wizard that's very domain specific to fix the problem).

@pgayvallet
Copy link
Contributor

I agree with the idea that imports need to be validated and that this includes business rule validation

OTOH this specific need seems to be more about reconfiguring/mutating objects at import time rather than really validating them.

This document-for-document design comes from trying to future-proof imports so that we could eventually stream them to support large files

It's also because import tends to reflect the migration approach, and doc migration during the SO migration is document-per-document too.

I wonder if the best strategy might be do exactly what we're doing with alerting where a simple onImport hook sets a disabled: true property on each document

If we want to do that, we can already do it by adding this disabled property during export rather than import, and then have the import hook returns a warning directing to an application's page to finish the import. This is actually just how the import/export hooks were designed to work.

@lukeelmers
Copy link
Member

we can already do it by adding this disabled property during export rather than import, and then have the import hook returns a warning directing to an application's page to finish the import.

@FrankHassanabad @spong Any thoughts on the approach described above? Basically:

  • Add an onExport transform to add some sort of disabled property to your objects
  • Add an onImport hook looking for this property, and if it exists add a warning that directs to the security solution app
  • Then in the security solution app, handle whatever business logic you need to address before removing the disabled flag (adding/removing/validating fields for these disabled objects, etc)

So the user experience for end users would be:

  • Export my objects
  • Import my objects, get a warning which redirects me to security solution app
  • UI in security solution app walks me through any invalid/skipped rules/exceptions, and updates the objects for me

If I understand correctly, this would come close to addressing most of the user stories here, except this one:

I would like to import the older exports security_solution provided me directly within this screen and not have errors and it will be backwards compatible.

But in that case, perhaps there could be a special place in the security solution app to handle importing from legacy format, and it could convert to ndjson behind the scenes?

@FrankHassanabad
Copy link
Contributor Author

FrankHassanabad commented Aug 31, 2021

Appreciate all the context and information everyone has provided here. Thank you. I read through the comments twice to make sure I understood the history and decisions behind why the choices were made.

I'm not sure to see the implication of this statement regarding the feature you're requesting, could you elaborate? Also, if the format works for you, what's the global purpose of trying to have your objects listed in SOM and import/exportable via the global export API?

Today we have an existing format for exports and imports that we use directly on the security_solution detections page as well as a REST API specific for it. We created the security_solution alerting import/export feature before kibana-alerting had integrated within the SOM system. Now that kibana-alerting is integrated within the SOM system, we would like to deprecate and remove our import and export REST API's and move towards the same REST API's and towards similar if not exactly the same UI/UX as kibana-alerting.

This work includes the SOM since kibana-alerting has moved this direction. The other reasons towards moving to the SOM is that it saves us from writing more and more code and more and more maintenance if we can rely on utilizing the SOM and its features.

But in that case, perhaps there could be a special place in the security solution app to handle importing from legacy format, and it could convert to ndjson behind the scenes?

Yes, @lukeelmers, I think we can handle that part and we can live without the older legacy format branching logic hook within SOM. I labeled this as use case 4 above. Branching off the older format is a stretch goal and a nice to have to be able to import today's existing legacy format. I think we can work around this when users are on our page and when we do an import on security_solution existing UI/UX pages/REST routes we could read a few lines of ndjson to determine the data type of the objects to determine which service to call and put deprecation warnings within our UI/UX pages. We should be mostly fine if use case 4 cannot be done.

it sounds like some of your business rules might require you to have all the objects being imported at once so that you can e.g. apply business rules that span across more than one object?

No, that should not be the case. Having them come in batches or 1 at a time into a function should be fine for us. I added some of the business rules to the description. One of them is that currently we have immutable rules which is a boolean flag that indicates the rule should never be updated by a user. A user should clone and then update it. We exclude exporting immutable at the moment but we want to start exporting it since that rule type can technically have actions from alerting on it. It just cannot have other fields outside of actions changed since it is rules provided by our content authors here at Elastic. The immutable is a stop gap measure and eventually we want to remove the flag altogether.

In addition, certain other fields we do not want the user to be able to update after an export. If they try to update those fields it just will ignore them. Other use cases similar to this is disallowing other fields such as created_by to be changed and some fields we do want to mutate such as updated_by to the current user doing the import (if possible) we are ok to negotiate some here ;-)

@lukeelmers, I think the main issue with the approached outlined above is the user editing of the exported objects after the import and us needing a way to validate them on re-import to ensure the user does not break them and we get bad UI/UX bugs and lots of escalations around why our UI's are throwing errors.

We already know of several users who put together fast and loose python scripts and use the existing import feature directly as a REST API as well as users who export and then do global "search and replace" before doing imports.

@rudolf
Copy link
Contributor

rudolf commented Sep 1, 2021

This include immutable rules to not be allowed to be re-imported with changes. Immutable rules are a business rule within security_solution where we have pre-packaged rules which are updated through cloud services and are not directly and easily allowed to be mutated.

Could this validation be performed by a synchronous function or would you need e.g. the saved objects client to run a query to enforce this?

@FrankHassanabad
Copy link
Contributor Author

FrankHassanabad commented Sep 1, 2021

Could this validation be performed by a synchronous function or would you need e.g. the saved objects client to run a query to enforce this?

For immutable rules it can be synchronous, @rudolf

@pgayvallet
Copy link
Contributor

pgayvallet commented Sep 2, 2021

We already know of several users who put together fast and loose python scripts and use the existing import feature directly as a REST API as well as users who export and then do global "search and replace" before doing imports

AFAIK, this is the case with most (all) of the exportable SO types. It's trivial to temper an exported file before re-importing it. We could eventually create a distinct issue for it. OTOH, it's functionally so close to the other part of the request (being able to mutate the object during import) that it's probably not worth it

A user should clone and then update it. We exclude exporting immutable at the moment but we want to start exporting it since that rule type can technically have actions from alerting on it.

I'm wondering: even if we do introduce validation during the import stage, how could you properly validate that immutable wasn't added or removed?

E.g

  • I export an object with immutable: true
  • I edit the file to remove this property
  • I import the file back

How the validation would be able to check if that property was removed (or added)?

@FrankHassanabad
Copy link
Contributor Author

I'm wondering: even if we do introduce validation during the import stage, how could you properly validate that immutable wasn't added or removed?

Our intent is for us to safe guard the best we can. In some cases it won't be "fool proof" but in some cases like the updated_by where we can completely ignore user supplied data to ensure invalid values aren't added is 👍 .

If we can only check this flag and they can flip it to false because we only have a sync call without a readonly SO client that's a good improvement to prevent a casual user from hurting themselves. A better way, yet, would be to have a readonly SO client to where we can do queries of existing rules and check if they are trying to overwrite something accidentally that is already marked as immutable. That would be better, yeah. And one step further is for us to read the existing immutable elastic rule SID (Signature ID's) that we know of to-date to prevent them from first deleting an immutable and then overriding it.

But I'm good with first steps here and what I can get with validation and then improve upon that. However, I would take a read only SO client along with an async function for sending down the import rules to hit more validation (mis)use cases above ;-)

We do check existing rules first and update them unless they're new right now and we ignore user supplied immutable for creating either new ones or being able to tamper with the existing ones.

Eventually the immutable should probably go away. No one really likes it and there have been a few proposals to make it go away.

@FrankHassanabad
Copy link
Contributor Author

Eventually the immutable should probably go away. No one really likes it and there have been a few proposals to make it go away.

Let me edit this statement a bit after I was pinged about it. There are no immediate plans for it to go away and we will be supporting it past 8.0.0 at the very least. It is going to be around for a bit and we will have to maintain and support it for the foreseeable future and depending on changing priorities it might be around for some time.

@pgayvallet
Copy link
Contributor

However, I would take a read only SO client along with an async function for sending down the import rules to hit more validation (mis)use cases above ;-)

onExport is async and get passed the request object for consumer to instantiate scoped clients if required

export type SavedObjectsExportTransform<T = unknown> = (
context: SavedObjectsExportTransformContext,
objects: Array<SavedObject<T>>
) => SavedObject[] | Promise<SavedObject[]>;

/**
* Context passed down to a {@link SavedObjectsExportTransform | export transform function}
*
* @public
*/
export interface SavedObjectsExportTransformContext {
/**
* The request that initiated the export request. Can be used to create scoped
* services or client inside the {@link SavedObjectsExportTransform | transformation}
*/
request: KibanaRequest;
}

To be honest, onImport is already async anyhow, and exposing the initiating request in a context would be far from the most complex thing in this feature request, so if there is an identified need to do so, I wouldn't mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Feature:Detection Alerts Security Solution Detection Alerts Feature Feature:Detection Rules Anything related to Security Solution's Detection Rules Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Detections and Resp Security Detection Response Team Team:Security Solution Platform Security Solution Platform Team
Projects
None yet
Development

No branches or pull requests

7 participants