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

Fix parent_process recursion and add grandparent_process #1158

Closed

Conversation

jonrau-at-queryai
Copy link
Contributor

Related Issue

#1108

Description of changes:

In an effort to limit the potentially limitless recursion within parent_process being typed as a process and to get a more straightforward approach to modeling both it and grandparent process I made the following changes.

  1. parent_process is now its own distinct Object, changes are mostly flavor but most notably it is no longer self-referential due to the current version being typed as a process object.
  2. Added a new grandparent_process but instead of starting it from parent_process, it also lives within the process object, this prevents an unnecessary (IMHO) extra layer of depth in the schema, which is already very deep depending if process is referred to from actor or evidences.

These changes do not break the current Observable for process nor does it break the Process Name or Process ID observables.

However, these can negatively impact mappers who are using the extra layer of depth to model grandparent processes under the current mechanism of process.parent_process.parent_process. This data is available from several EDR and APM tools.

@mlmitch
Copy link
Contributor

mlmitch commented Aug 15, 2024

After reading the issue and this pull request, I don't understand why this is something that needs fixing.

The recursive structure of the process object feels natural to me.
It matches the recursive nature of the "process tree" which is normally used to describe process genealogy.
Also, it is easy to stop the recursion by omitting the parent_process field at the depth that the implementer chooses.

Digging into your example from the issue on representing a grandparent inside of evidences from the linked issue:
It seems that if you have a process that is involved in a detection and you have some info on its parent and grandparent, then process.parent_process.parent_process is exactly the structure to use.
What is a grandparent if not the "parent of the parent"?

Furthermore, adding these objects and fields creates more ways to express process genealogy, which will reduce clarity in how to express this concept in OCSF.

@davemcatcisco
Copy link
Contributor

I don't really understand what real problem this PR is seeking to address. An OCSF implementation which features "potentially limitless recursion" when walking up the process hierarchy is to my mind a defective OCSF implementation. The documentation for the parent_process attribute specifically cautions against excessive recursion. Implementations must heed this guidance.

And even though this PR wouldn't solve any real problem, it would introduce (at least) three problems:

  1. This is a breaking change, and a significant one at that. The data type of the parent_process attribute in the dictionary is changing from a process object to the new parent_process object. This will break any existing implementation. My view is that this issue alone should render this PR a complete non-starter.

  2. At least some OCSF implementations - possibly most - will have data types (classes, structs, etc.) defined in their programming language of choice to represent OCSF entities like events and objects. This PR would mean going from one data type to represent a process to having three, depending on where the process sits in the three-level hierarchy. Think of all the places where this would introduce complexity in the code.

  3. The appropriate level of recursion to implement should be for individual vendors to decide. Some may favour a denormalised approach, attaching greater value to having immediate access right up to to grandparent level or even beyond. Others may prefer to minimise the size of individual events. This PR enforces three as the appropriate size of a process hierarchy, and effectively restricts all vendors to that.

@floydtree
Copy link
Contributor

@jonrau-at-queryai I agree with the general sentiments laid out above. These were pretty much the reasons we opted for a recursive definition of the parent_process object. But I am sure, all of us here would be happy to chat about it on the Tuesday call if you would like to.

@jonrau-at-queryai
Copy link
Contributor Author

Thanks for the feedback gents! This is exactly what I was looking for, the recursion is (self-inflicted) issue of sorts at Query for auto-generating schema for our mapping and nested search engines, where we have to set upper limits on depth.

Wasn't sure if it annoyed other folks beyond that "us" problem. I'll go ahead and close this up, since it's ultimately a waste of time.

@jonrau-at-queryai jonrau-at-queryai deleted the parent_process_mayhem branch August 15, 2024 20:28
@davemcatcisco
Copy link
Contributor

davemcatcisco commented Aug 16, 2024

Thanks, Jonathan.

FWIW, unless something is a simple "non-structural" change that's unlikely to be controversial, I tend to float any proposals in the schema channel on Slack before doing any work. Feedback comes relatively quickly there, and this can avoid the hassle of issue+PR if the community takes a different view. To be clear, it's not necessary to take this step but I like getting the provisional thumbs-up there before proceeding with the more formal submission.

@mikeradka
Copy link
Contributor

Thanks, Jonathan.

FWIW, unless something is a simple "non-structural" change that's unlikely to be controversial, I tend to float any proposals in the schema channel on Slack before doing any work. Feedback comes relatively quickly there, and this can avoid the hassle of issue+PR if the community takes a different view. To be clear, it's not necessary to take this step but I like getting the provisional thumbs-up there before proceeding with the more formal submission.

Agreed. Perhaps a topic of discussion in the upcoming Tuesday meeting or one of our workstream meetings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants