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

[RFC] Continue target process RFC to stage 1 #1297

Merged
merged 8 commits into from
Jun 9, 2021

Conversation

rw-access
Copy link
Contributor

@rw-access rw-access commented Mar 9, 2021

Continuing #1286 to stage 1

Markdown preview of the proposal

Stage 1 Criteria

  • Opened pull request for this proposal revising the existing strawperson
  • Identified "sponsor" at Elastic who will participate in RFC process and take ownership of the change after the process completes
  • Outlined initial field definitions
  • High-level description of examples of usage
  • High-level description of example sources of data
  • Identified potential concerns and implementation challenges/complexity
  • Subject matter experts identified and weighed in on the high level utility of these changes in the pull request
  • ECS team weighed in on appropriateness of these changes in the pull request

@andrewstucki
Copy link
Contributor

I'll see if I can add some auditbeat examples that could potentially capture this. Pretty sure if we capture ptrace syscalls with PTRACE_POKE* we could model that with these fields today.

<!--
Stage 1: Identify potential concerns, implementation challenges, or complexity. Spend some time on this. Play devil's advocate. Try to identify the sort of non-obvious challenges that tend to surface later. The goal here is to surface risks early, allow everyone the time to work through them, and ultimately document resolution for posterity's sake.
-->

The biggest concern is the duplication of fields and the double-nested `process` group at `process.target.parent`. This could require some updates to our reuse mechanism, but that's an issue internal to this repository. We should make sure that we don't accidentally populate `process.parent.target`, which would have different meaning. Because of this, we will need to make sure that we articulate what each reuse means, similar to https://www.elastic.co/guide/en/ecs/current/ecs-user.html#ecs-user-nestings.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for calling this out.

Changes will need to be made to support reusing process.* fields as parent.* underneath process.target since self-nestings (fields reused within themselves) aren't carried around to other places (avoids nestings like user.target.* from getting carried over to source.user.target.*)

rfcs/text/0016-target-process.md Outdated Show resolved Hide resolved
rfcs/text/0016-target-process.md Outdated Show resolved Hide resolved
rw-access and others added 2 commits March 25, 2021 15:51
Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
@rw-access
Copy link
Contributor Author

@andrewstucki any updates on your end?

@andrewstucki
Copy link
Contributor

@rw-access sorry, getting the auditbeat examples fell off my radar, I'll try and generate them in the next few days.

@epixa epixa changed the title Continue target process RFC to stage 1 [RFC] Continue target process RFC to stage 1 Apr 5, 2021
@rw-access
Copy link
Contributor Author

Hey @andrewstucki just another nudge.
@ebeahan outside those examples, anything else needed for this PR to reach the next phase?

@ebeahan
Copy link
Member

ebeahan commented May 13, 2021

@rw-access I added some more details for the stage 1 criteria into the PR description.

In addition to the mapping examples, any thoughts about who could act as a sponsor?

@rw-access
Copy link
Contributor Author

rw-access commented May 13, 2021

For sponsor, @magermark is probably the best fit, then @devonakerr. I'm also fine being the sponsor for this one.

@rw-access
Copy link
Contributor Author

Hey @ebeahan, just talked to @andrewstucki. Sounds like he's busy and won't be able to get the examples. Do you see those as a blocker for the RFC? Should we move forward without additional examples for what this would look like on linux, or ping another person who can get them?

@ebeahan
Copy link
Member

ebeahan commented Jun 2, 2021

Thanks for the update, @rw-access! Let's move forward with the current sysmon example. I think it does a good job of capturing the high-level intent with these proposed fields.

If you've finalized who will sponsor, can you add them as the sponsor under the People section in the document and also add them as a reviewer on this PR?

I'll do a final review from the ECS team as well.

@rw-access rw-access requested a review from ebeahan June 8, 2021 22:16
@ebeahan ebeahan requested a review from devonakerr June 9, 2021 15:05
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

When @devonakerr reviews and approves, I'll set the date and merge the PR.

Copy link

@devonakerr devonakerr left a comment

Choose a reason for hiding this comment

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

I approve, thanks for getting those adjustments in, Ross.

@ebeahan ebeahan merged commit 0416af5 into master Jun 9, 2021
@djptek
Copy link
Contributor

djptek commented Jun 16, 2021

Sorry to come in at this late stage with negative feedback - I hadn't been aware of the nesting aspects that may be implied by the current approach, specifically after after having seen: #1459

I'm concerned by the precedent of replicating a field structure nested within a field structure for multiple reasons:

proliferation of fields in Elasticsearch

The following JSON generates 8 fields which in reality might be more consistently expressed as four, not an issue at this scale, but if we are talking e.g. 1000 fields per document that may require 2000 fields which in turn could become a problem for scaling Elasticsearch downstream

{
  "pid" : 123,
  "morefields": "abc"
  "child" : {
      "pid" : 456,
      "morefields": "def"
      "child" : {
           "pid": 789,
            "morefields": "ghi"
       }
    }
}

this might be better expressed as e.g. 3 docs

{
  "pid": 123
  "morefields": "abc"
}

{
  "pid": 456,
  "morefields": "def"
  "parent": {
       pid: 123
  }
}

{
  "pid": 789,
  "morefields": "ghi"
  "parent": {
       pid: 456
   }
}

I appreciate this means that a search for the full history of the parentage of pid: 789 will now take multiple queries, however, handled that through business logic keeps the mapping consistent and allows flexibility, which may be preferable to coercing the data format to extend the mapping, potentially infinitely

Proliferation of fields at multiple levels

Aside from the impact on Elasticsearch, having multiple fields with the same name at multiple levels may create issues for e.g. anyone wishing to (or already) unpack(-ing) the structure using e.g. X-Path expressions

Impact of nesting on downstream apps

Requiring downstream apps to processes nested fields raises multiple issues:

-Obligation to handle nested structures

-Risk of Stack Overflow and System Outage

-Risk of Stack Overflow and Data Compromise

Alternatives

O11y currently handles the linkage of a transaction history by adding a unique ID + 1 parent ID to transactions derived from a common parent, which allows them to grouped by that ID & or unpacked to a tree structure based on the parent IDs

@rw-access rw-access deleted the rfc-target-process-stage1 branch June 16, 2021 14:30
@rw-access
Copy link
Contributor Author

Hey @djptek, thanks for the feedback.

I don't totally understand what you mean. A lot of these problems aren't new; process.parent is already a re-nesting of process within itself. Now we're also nesting it at process.target. If you're worried about process.target.parent being too much information, we can absolutely evaluate that and pull that out if it adds too much.

but if we are talking e.g. 1000 fields per document that may require 2000 fields which in turn could become a problem for scaling Elasticsearch downstream

Is that still a problem if it's 10 fields? 20 fields? 50 fields? Just trying to understand when it's a problem and when it's not.

I appreciate this means that a search for the full history of the parentage of pid: 789 will now take multiple queries, however, handled that through business logic keeps the mapping consistent and allows flexibility, which may be preferable to coercing the data format to extend the mapping, potentially infinitely

I'm unsure what role introducing process.target has on the lineage of pid: 789. Either way, it still requires multiple queries to Elasticsearch

which may be preferable to coercing the data format to extend the mapping, potentially infinitely

We're not extending the mapping infinitely though. We're repeating the same thing we did for process.parent with process.target, and then we nest again at process.target.parent. But it's not an infinitely recursive data structure. Every re-nesting is fixed.

And just in case it needs saying, we're not using any nested fields in the Elasticsearch mapping sense of the word nested. It's a little unfortunate that there's a collision of words, but I figured extra clarity doesn't hurt.


I think there might be some mutual confusion over this RFC. Do you think it would make sense for us to communicate synchronously, so we don't accidentally talk past each other? Some of the things you mention don't seem totally relevant from my understanding, but I don't want to disregard your concerns just because I don't understand them.

@djptek
Copy link
Contributor

djptek commented Jun 16, 2021

Hi @rw-access

process.parent is already a re-nesting of process

if this is simply a pointer from one _doc to a parent doc, I wouldn't consider that nesting it's more of a linked-list & that's OK because ought not to blow any stacks

Is that still a problem if it's 10 fields? 20 fields? 50 fields?

It Depends... would be the official answer see also index.mapping.total_fields.limit
though that is totally configurable
- my real concern isn't around one RFC adding more fields to deal with an exception circumstance, I'm worried about setting a precedent to say it's OK to nest documents inside documents inside documents ... which may be elegant and optimal when encapsulated in business logic but could constitute an anti-pattern in a format.

We're not extending the mapping infinitely though

So the goal isn't to nest each child, child-of-child, child-of-child-of-child, etc... within the parent, but that the child has two pointers, one each to predecessor and follower? That would put us back in linked list territory, which feels a lot more OK

And just in case it needs saying, we're not using any nested fields in the Elasticsearch mapping

Thanks for clarifying - don't worry, I was clear on that one :-)

@rw-access
Copy link
Contributor Author

rw-access commented Jun 16, 2021

process.parent is already a re-nesting of process

if this is simply a pointer from one _doc to a parent doc, I wouldn't consider that nesting it's more of a linked-list & that's OK because ought not to blow any stacks

I'm still not sure if we're talking about the same thing.
To be clear, here's what events already look like today:

{
  "process": {
    "name": "cmd.exe",
    "pid": 1848,
    "command_line": "cmd /c whoami",
    "parent": {
      "name": "explorer.exe",
      "pid": 1240
    }
  }
}

here's the proposal:

{
  "process": {
    "name": "cmd.exe",
    "pid": 1848,
    "command_line": "cmd /c whoami",
    "parent": {
      "name": "explorer.exe",
      "pid": 1240
    },
    "target": {
      "name": "lsass.exe",
      "command_line": "C:\\Windows\\System32\\lsass.exe",
      "pid": 604
    }
  }
}

and if process.target.parent is okay:

{
  "process": {
    "name": "cmd.exe",
    "pid": 1848,
    "command_line": "cmd /c whoami",
    "parent": {
      "name": "explorer.exe",
      "pid": 1240
    },
    "target": {
      "name": "lsass.exe",
      "command_line": "C:\\Windows\\System32\\lsass.exe",
      "pid": 604,
      "parent": {
        "name": "wininit.exe",
        "pid": 132
      }
    }
  }
}

@djptek
Copy link
Contributor

djptek commented Jun 17, 2021

@rw-access Thanks for the clarification, I'm nearly there, though 'fraid I still have a couple more questions:

  1. If the process has multiple targets, will those be represented as an array target: [] or in separate documents?
  2. If the target parent e.g. "parent": { "name": "wininit.exe","pid": 132} itself has a target parent, will that require an addition level of nesting, or will it go in a separate document?

Thanks for doing the detail with me on this

@rw-access
Copy link
Contributor Author

rw-access commented Jun 17, 2021

The RFC only proposes an additional nesting of the process.* fields at process.target and process.target.parent, assuming we don't run into problems with that (i.e. too many new fields added)

There would only be one target process in a cross-process event. Please take another look at the RFC for the examples

@djptek
Copy link
Contributor

djptek commented Jun 17, 2021

There would only be one target process in a cross-process event

LGTM - thanks for covering these extra questions at this late stage @rw-access

@nicpenning
Copy link
Contributor

This is great. A much needed capability for further standardization of process fields. I look forward to the SysMon use cases as well as EDR solutions when it comes to cross process activity. Thank you!

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

Successfully merging this pull request may close these issues.

6 participants