Skip to content
This repository has been archived by the owner on Apr 1, 2024. It is now read-only.

Add corelight/zeek-community-id package #43

Merged
merged 2 commits into from
Oct 7, 2020
Merged

Add corelight/zeek-community-id package #43

merged 2 commits into from
Oct 7, 2020

Conversation

nwt
Copy link
Member

@nwt nwt commented Oct 6, 2020

The corelight/zeek-community-id package includes a Zeek plugin.
Building and loading plugins doesn't work on Windows, so this change
consists almost entirely of jiggery-pokery to address that, including a
modified cmake submodule at https://github.com/brimsec/cmake.

Should close #36 and #37.

The corelight/zeek-community-id package includes a Zeek plugin.
Building and loading plugins doesn't work on Windows, so this change
consists almost entirely of jiggery-pokery to address that, including a
modified cmake submodule at https://github.com/brimsec/cmake.
@nwt nwt requested a review from a team October 6, 2020 16:14
@@ -78,20 +78,35 @@ get_filename_component(ZEEK_SCRIPT_INSTALL_PATH ${ZEEK_SCRIPT_INSTALL_PATH}

set(BRO_PLUGIN_INSTALL_PATH ${ZEEK_ROOT_DIR}/lib/zeek/plugins CACHE STRING "Installation path for plugins" FORCE)

set(bro_plugin_install_path "${BRO_PLUGIN_INSTALL_PATH}")

Choose a reason for hiding this comment

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

I just noticed this after making my comment on zeek-config.in. So these are paths that have been cleared of any windows volume identifiers?

Copy link
Member Author

@nwt nwt Oct 6, 2020

Choose a reason for hiding this comment

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

Yes, though the volume identifiers have been MSYS-ified (by converting c:/ to /c/) rather than replaced cleared.

@philrz
Copy link

philrz commented Oct 7, 2020

I think we're in good shape here from a functional perspective. I tested things out by manually downloading the macOS Zeek artifact from the most recent Actions run on this branch (https://github.com/brimsec/zeek/suites/1299072896/artifacts/20371620). In my local dev Brim environment, I removed the regular zdeps/zeek/ directory replaced it with the one from this artifact. Manually importing pcaps into Brim, I can see the Community ID field is populated as we'd expect:

image

I also made sure the other Zeek packages are still working as expected.

JA3:

image

HASSH:

image

geoip-conn:

image

I also did an npm run itest to make sure that none of the Brim CI that deals with Zeek logs generated from pcaps were disturbed by the addition of the new Community ID field, and as I'd hoped, everything passed without complaint.

Test Suites: 9 passed, 9 total
Tests:       104 passed, 104 total
Snapshots:   54 passed, 54 total
Time:        222.266s, estimated 223s
Ran all test suites.

In conclusion, if you guys are ok with this at a code level, I'm game to see it merged so we can start delivering Community ID as a new field even in advance of the rest of Suricata support being ready. We've already had validation from our user community that it's helpful just to have the value from it even if they're manually cut & pasting the value out of Brim and into some other pane-of-glass where data is stored that has Community ID as a field.

@nwt nwt merged commit 0ef99ec into master Oct 7, 2020
@nwt nwt deleted the community-id branch October 7, 2020 22:23
brim-bot pushed a commit to brimdata/zui that referenced this pull request Oct 7, 2020
… by philrz

This is an auto-generated commit with a zq dependency update. The zq PR
brimdata/super#1435, authored by @philrz,
has been merged.

Update Zeek pointer to v3.2.1-brim1 and fix tests

While reviewing brimdata/zeek#43, I realized that I wasn't fully aware of how the zq repo actually has its own smoke tests that generate Zeek logs using our artifacts and do sanity check on the outputs. This has been pointing at v3.2.0-dev-brim3 for some time. However, we've since made changes that cause a few tests to fail if we advance the pointer to the current artifact:

1. The geolocation support added starting with our Zeek `v3.2.0-dev-brim4` adds additional `geo.*` fields that cause the `conn` records in `ztests/suite/zqd/pcappost.yaml` to widen.
2. The major update to catch up with GA Zeek v3.2.1 starting with our Zeek `v3.2.1-brim1` causes an additional `stats` record that made the event counts in `zqd/pcappost.yaml ` and `zqd/pcappost-suricata.yaml ` to be off-by-one.
3. The additional `stats` record also made the Space size checked in `ztests/suite/zqd/pcappost.yaml` increase.

Regarding the extra `stats` event, I confirmed this separately using "stock" Zeek builds (not our artifacts) for v3.0.2 and v3.2.1, generating logs from the `valid.pcap`. Compare:

```
$ zq -t 'cut _path,ts' zeek-3.0.2/*
brimsec/zq#0:record[_path:string,ts:time]
0:[conn;1501770877.471635;]
0:[stats;1501770877.471635;]
0:[http;1501770877.501001;]
0:[capture_loss;1501770880.988247;]

$ zq -t 'cut _path,ts' zeek-3.2.1/*
brimsec/zq#0:record[_path:string,ts:time]
0:[conn;1501770877.471635;]
0:[stats;1501770877.471635;]
0:[http;1501770877.501001;]
0:[capture_loss;1501770880.988247;]
0:[stats;1501770880.988247;]
```

In addition to updating the Zeek pointer in the Makefile, here I adjust the tests to recognize the new outputs.

I've also pre-tested what we can expect when brimdata/zeek#43 merges and we update the Zeek pointer there yet again. The addition of the Community ID field causes a similar effect because the additional field makes the `conn` records a little bigger and hence also increases the Space size yet again. I've got a branch up at https://github.com/brimsec/zq/tree/update-zeek-community-id that's ready to be turned into a PR once brimdata/zeek#43 merges and the new Zeek artifact has a name.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Community ID support
3 participants