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

Adapt to Zeek "files" log losing conn_uids+tx_hosts+rx_hosts and gaining uid+id #2981

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Jan 20, 2024

Fixes #2980

When #2971 merged, Zui went from using a Zeek artifact based on Zeek v3.2.1 to an artifact based on Zeek v6.0.2. This exposed Zui to a "breaking change" as part of Zeek v5.1.0 that broke the Download Packets feature in some circumstances as shown in #2980. From the Zeek release notes:

By default, files.log does not have the fields tx_hosts, rx_hosts and conn_uids anymore. These have been replaced with the more commonly used uid and id fields.

Here's the affected code as it stands on the tip of main:

export function findConnLog(pool: string, uid: string) {
return (
zedScript`
from ${pool}
| (` +
uidFilter(uid) +
`)
| is(ts, <time>)
| is(duration, <duration>)
| is(uid, <string>)
| head 1
`
)
}

export function uidFilter(uid: string) {
return zedScript`uid==${uid} or ${uid} in conn_uids or ${uid} in uids or referenced_file.uid==${uid}`
}

If I manually construct that query after loading the ifconfig.pcapng test data from #2980 and drop the head 1, we can see why it started failing.

image

This shows that the "breaking change" to the files record means it has taken on so many characteristics of the conn log that it ended up slipping through all the filter logic in findConnLog() only to be found unusable elsewhere when the pcap extraction logic looked for the proto field that's normally found on a true conn record. In this PR I've tightened up the logic so findConnLog() only finds a true conn record.


Once I became hip to the scope of the change, I looked into other possible side effects of the add/remove of those fields. As I recall it having been explained to me, the complexity with conn_uids, tx_hosts, and rx_hosts all being set types was an attempt to capture situations where multiple hosts could simultaneously be involved in send/receive of a given file, such as may happen in peer-to-peer file sharing protocols. And I seem to recall that the "breaking change" to the files log was effectively an acceptance of how that never really worked properly and just created confusion. Therefore the single uid would now correlate a files event with its corresponding conn record, and likewise the id.orig_h and id.resp_h would show the hosts involved in the transfer of a given file.

This all means that reacting to the change actually simplifies and fixes some things. Most of the other Zeek log types are trivially joined by uid, and now files is much the same, which means conn_uids can be dropped from the add-ons in our uid filtering logic. Likewise, while Zed v1.5.0 didn't have the #2980 problem, with tx_hosts and rx_hosts as sets, the Detail pane showed the count summary like this:

image

Whereas the primitive values plus the fixes in this branch means we can actually show meaningful IP addresses again.

image


Other things to note:

  1. After recognizing the impact this Zeek change had on us, I browsed all their release notes in versions since the Zeek v3.2.1 we used in the past. Nothing stood out.

  2. I fixed the hover text descriptions in this PR, but then I realized they don't actually work anymore and broke when Zui Polish 2 #2895 merged, so they were presumably a casualty of the "New Detail Pane Design" cited there. I don't know if this is a surprise but since we've not done another GA release since that merged we could make a call on if we're ready to let that functionality go (in which case we should probably remove the dead code) or if it can be fixed (I don't know how many users rely on it, but it always seemed to me like a nice proof-of-concept of something we could find a way for users to do with their own custom data sources, perhaps using plugins?)

  3. The fact that the Download Packets button failed so quietly in the Can't open pcap slice if the conn contains "files" activity #2980 repro is somewhat concerning to me, but I doubt I have the skills to dive into that. I could perhaps open another issue to pursue that separately.

@philrz philrz requested a review from jameskerr January 20, 2024 03:11
@philrz philrz self-assigned this Jan 20, 2024
@@ -29,7 +29,7 @@ export function activateZeekCorrelations() {
return zedScript`
from ${session.poolName}
| md5==${getMd5()}
| count() by tx_hosts
| count() by tx_host:=id.resp_h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do the := assignment here because if I leave it as just id.resp_h then only id appears as the column header in the table and no values under it, I guess since id is a record type and I assume the table is only prepared to render primitive values, and primitive values are all we want to show here anyway. So I'm kinda conjuring up tx_host and rx_host as fake field names just for presentation purposes, but that feels defensible.

@@ -25,6 +25,7 @@ export function findConnLog(pool: string, uid: string) {
| (` +
uidFilter(uid) +
`)
| _path=="conn"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jameskerr: I'll definitely appreciate your close review here. The function name findConnLog() and how I see it being used makes it pretty clear that we only ever want it to find true conn records, so I think adding what I've done here is safe and desirable. But please double check me!

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me!

@philrz philrz merged commit eec89c3 into main Jan 23, 2024
3 checks passed
@philrz philrz deleted the packet-slice-debug branch January 23, 2024 19:28
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.

Can't open pcap slice if the conn contains "files" activity
2 participants