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

Add tunnel fields to Suricata shaper #2370

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Mar 18, 2021

#2369 describes a problem where missing NDJSON typing config for rarely-seen Suricata fields generated a user-facing warning. This PR adds the additional config to the Suricata shaper to cover these fields.

Our switchover from legacy types.json to the new shaping configs in the time since the user reported the issue actually changes the symptom, so before I show the fix having its impact, I'll take a step back and show how the symptom was looking as of Brim commit brimdata/zui@8569148 which pointed at zqd commit 8ab4607. In that case, because of the change in #2309, the user no longer was shown a warning, but the additional fields were silently imported with inferred types. So for instance, notice how the tunnel fields are shown in black font that's used for strings, rather than the blue that's used for the ip type fields.

More importantly, if the user doesn't notice this subtle difference and tries to treat them like the IP addresses they otherwise appear to be, they get unexpected behavior, such as this failing CIDR match.

Of course, if they catch the problem and do the cast at query time, they can fix it. But this is what we want the shaper to do, hence the changes in this PR.

If I start from the same Brim commit brimdata/zui@8569148 and advance my zq pointer to 6613aba to use the shaper in this branch, then re-import, now we get the correct typing out of the gate.

A couple additional details:

  1. The sample data I'm using from Missing type conversion config for Suricata "tunnel" fields #2369 doesn't happen to generate the tunnel.src_port and tunnel.dest_port fields in the Suricata EVE JSON. However, since the original user's issue showed these being generated, I've included them here in the shaping config.
  2. Since we've been through so many example pcaps before seeing Suricata generates this field, some may wonder if it makes sense to make the current single schema even wider to always make room for them as opposed to letting multiple shapes get created. However, we've had users recently be confused by the disappearing columns that result from unexpected multiple shapes (When not showing headers, use the header row for information zui#967). Therefore, until we have new UX in Brim that makes it easier for users to browse their data in such situations and/or also apply+modify their own shaper modifications, I'm keen to maintain the single schema.
  3. While this now covers us for all Suricata fields we're aware of for alert events, the fact the user was not being informed these tunnel fields were being imported with inferred types seems hazardous. I've opened separate issue brimsec/brim#1519 regarding this, as it seems like something we'll need to address in the wider context of shaper UX.

 Fixes #2369

@philrz philrz requested a review from a team March 18, 2021 01:08
@philrz philrz self-assigned this Mar 18, 2021
@philrz philrz merged commit 7ee2a8d into master Mar 18, 2021
@philrz philrz deleted the suricata-tunnel-field-types branch March 18, 2021 18:12
brim-bot pushed a commit to brimdata/zui that referenced this pull request Mar 18, 2021
This is an auto-generated commit with a zq dependency update. The zq PR
brimdata/zed#2370, authored by @philrz,
has been merged.

Add tunnel fields to Suricata shaper

brimdata/zed#2369 describes a problem where missing NDJSON typing config for rarely-seen Suricata fields generated a user-facing warning. This PR adds the additional config to the Suricata shaper to cover these fields.

Our switchover from legacy `types.json` to the new shaping configs in the time since the user reported the issue actually changes the symptom, so before I show the fix having its impact, I'll take a step back and show how the symptom was looking as of Brim commit 8569148 which pointed at `zqd` commit 8ab4607. In that case, because of the change in brimdata/zed#2309, the user no longer was shown a warning, but the additional fields were silently imported with inferred types. So for instance, notice how the `tunnel` fields are shown in black font that's used for strings, rather than the blue that's used for the `ip` type fields.

![](https://user-images.githubusercontent.com/5934157/111556887-d2dc9a80-8748-11eb-9a86-329b652f1c1a.png)

More importantly, if the user doesn't notice this subtle difference and tries to treat them like the IP addresses they otherwise _appear_ to be, they get unexpected behavior, such as this failing CIDR match.

![](https://user-images.githubusercontent.com/5934157/111557022-1afbbd00-8749-11eb-9e7e-8258739777fe.png)

Of course, if they catch the problem and do the cast at query time, they can fix it. But this is what we want the shaper to do, hence the changes in this PR.

![](https://user-images.githubusercontent.com/5934157/111557058-2b139c80-8749-11eb-9ca2-746635067188.png)

If I start from the same Brim commit 8569148 and advance my `zq` pointer to 6613aba to use the shaper in this branch, then re-import, now we get the correct typing out of the gate.

![](https://user-images.githubusercontent.com/5934157/111557351-bc830e80-8749-11eb-897c-b43862ed9d54.png)

![](https://user-images.githubusercontent.com/5934157/111557392-cf95de80-8749-11eb-9d99-144e8479d31a.png)

A couple additional details:

1.  The sample data I'm using from brimdata/zed#2369 doesn't happen to generate the `tunnel.src_port` and `tunnel.dest_port` fields in the Suricata EVE JSON. However, since the original user's issue showed these being generated, I've included them here in the shaping config.
2.  Since we've been through so many example pcaps before seeing Suricata generates this field, some may wonder if it makes sense to make the current single schema even wider to always make room for them as opposed to letting multiple shapes get created. However, we've had users recently be confused by the disappearing columns that result from unexpected multiple shapes (#967). Therefore, until we have new UX in Brim that makes it easier for users to browse their data in such situations and/or also apply+modify their own shaper modifications, I'm keen to maintain the single schema.
3.  While this now covers us for all Suricata fields we're aware of for `alert` events, the fact the user was not being informed these `tunnel` fields were being imported with inferred types seems hazardous. I've opened separate issue brimsec/brim#1519 regarding this, as it seems like something we'll need to address in the wider context of shaper UX.

 Fixes brimdata/zed#2369
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.

Missing type conversion config for Suricata "tunnel" fields
2 participants