Stormshield/create first integration #9337
Conversation
- Created the new integration with elastic-package
…ld/create_first_integration
…ld/create_first_integration
|
@cpascale43 this could be a good one for you to review and provide a seal of approval. Main areas to look at are the config options, dashboards and documentation. ECS mappings in the ingest pipeline are also an option, but no need to worry about those for now. |
@jamiehynds, do we want to come back for a second round after this PR and handle ECS mappings? I'm seeing plenty of opportunities in there right now for mappings to ECS. It's pretty easy to add those mappings later, only complication is if we have to adjust dashboard visualizations, but even that's not too big of a deal. |
|
Hey @taylor-swanson, Carrie may not be familiar with ingest pipelines yet so recommended avoiding them for the time being. But when it comes to the mappings, the preference is to ensure we have proper ECS mappings in place from the get go, so if possible can we include the mappings as part of the review and make adjustments before we merge? |
There was a problem hiding this comment.
Overall, this is looking good. There are a few things that need to be addressed right now, but other things (not listed in comments) can be handled in a follow up PR. Mostly things around ECS mappings and nit-picky items for the dashboards.
We are also missing a filestream input. I'm okay with that slipping to the next PR, but something we want to add as soon as possible.
packages/stormshield/data_stream/log/_dev/test/pipeline/test-common-config.yml
Outdated
Show resolved
Hide resolved
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
| target_field: source.ip | ||
| type: ip | ||
| if: ctx.stormshield?.origsrc != null | ||
| - convert: |
There was a problem hiding this comment.
What's orgsrc vs src supposed to mean in this context?
There was a problem hiding this comment.
origsrc means some NAT was done.
There was a problem hiding this comment.
Gotcha. Do we have any tests that verify this? Quick glance at the pipeline tests suggests no, but perhaps we could pull in some of the system test cases, since there are more over there?
There was a problem hiding this comment.
So it looks like origsrc is showing up as the same as src in places. That is weird to me.
There was a problem hiding this comment.
In non-NAT cases, they may still list both fields (odd choice, if that's actually the case). Are the ports the same? There should be a NAT port as well.
| if: ctx.stormshield?.origsrc != null && ctx.stormshield?.src != null | ||
| - remove: | ||
| field: stormshield.src | ||
| if: ctx.stormshield?.src != null |
There was a problem hiding this comment.
Use ignore_missing: true instead. Applies to other remove processors.
There was a problem hiding this comment.
I'm changing most of these. What about the compound if blocks? change it to a simple if: target_field == null and ignore_missing: true? or just leave as-is?
There was a problem hiding this comment.
For compound statements, I think we can leave it as-is.
There was a problem hiding this comment.
Only thing I'd add is making sure we have some tests to verify functionality. Things can get complicated when we start interacting with and checking against multiple fields.
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
| field: destination.ip | ||
| target_field: destination.geo | ||
| ignore_missing: true | ||
| if: ctx.destination?.geo == null |
There was a problem hiding this comment.
Redundant check if nothing has written to the geo fields yet.
There was a problem hiding this comment.
removing the above removal. also, this shouldn't be set, either, anyway
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/stormshield/manifest.yml
Outdated
| description: "Stormshield SNS integration." | ||
| type: integration | ||
| categories: | ||
| - custom |
There was a problem hiding this comment.
I think this should be changed, but I'm not too sure what it should be changed to
Looking at some other firewall integrations, they use network, security, proxy_security firewall_security, but not always the same set of items.
@taylor-swanson is there a standard list of categories to use?
There was a problem hiding this comment.
Stormshield Network Security is what SNS stands for, so I think either network and security or firewall_security or all three, or maybe all four, but I don't think it really does proxy work.
There was a problem hiding this comment.
elastic-package verifies this list, so it has to be a "valid" one. I won't bother listing them here, but you can misspell one of the entries and elastic-package list out the valid options.
The ones listed above seem fine to me (I'm good with leaving out proxy if it doesn't fit here).
packages/stormshield/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
| # | ||
| # The syslog header should have been stripped off, so we need to process the key=value data | ||
| # We will be using painless here, because it is the best option we have. | ||
| - script: |
There was a problem hiding this comment.
nit: I wonder if we could parse the same with an alternative kv processor like that:
- kv:
field: message
target_field: _tmp.fields
field_split: '(?:((\s+)?$|\s+(?=\S+=)))'
value_split: '='
ignore_failure: true
ignore_missing: true
There was a problem hiding this comment.
The problem with kv is it tends to perform worse than the script below. I don't have the numbers to prove it, but I'm willing to blame this on the regex used in field split. We had an issue filed for fortinet_fortimanager for the kv processor being too slow due to the regex in that case.
There was a problem hiding this comment.
I couldn't get any kv processor to work, because some of the fields have quotes and some have spaces and some have both. I haven't seen any = inside quotes, but it might be possible to configure some things with arbitrary data.
| field: stormshield.modsrcport | ||
| target_field: source.nat.port | ||
| type: long | ||
| if: ctx.stormshield?.modsrcport != null && ctx.stormshield?.modsrcport != ctx.stormshield?.srcport |
There was a problem hiding this comment.
I would still set source.nat.port even if it's the same as source port. We can skip setting it if source.nat.ip was not set.
There was a problem hiding this comment.
I still have a comment about source nat ip/port pairs outstanding (resolved), but otherwise things LGTM here.
We'll need to follow up with additional work related to observer and related fields, among other things, but there's enough here to get started with an initial preview.
💚 Build Succeeded
History
|
|
|
Package stormshield - 0.1.0 containing this change is available at https://epr.elastic.co/search?package=stormshield |
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots