While testing the new Broker code in master I came across this a bit unexpectedly when trying to run our full production policy stack:
2.5-544 | 2018-05-01 17:57:15 -0500
Rewrite the DHCP analyzer and accompanying script-layer API.
I’m all for analyzer updates and improvements, but what I’m honestly not sure about is this:
Reduced all DHCP events into a single dhcp_message event.
(removed legacy events since they weren’t widely used anyway)
How was the determination made that it’s not widely used? I don’t recall a survey on the bro/bro-dev lists and there’s clearly instances of it’s use when searching github.
I didn't dig around much which I made that change. Generally the DHCP analyzer hasn't been something that I've heard of much use (although Vlad did point out that I broke one of my own scripts!). How many scripts do you think it impacted? I suspect it's fairly few and we probably just need to put a change note in the release to say that developers will need to handle a new event to get the data. On the upside, you can handle both the old events and the new and they shouldn't impact each other (if you want to make a script work on multiple releases).
(There were a few others, like IVRE, but they’ve already updated).
Even if it’s not widely used, I think it’d be a nicer user experience if we were to ship a script that handled dhcp_message, and raised the old events. We could mark the old events as deprecated, and remove them in the next version. That way, people have at least one cycle to upgrade.
Hopefully, as we see more published Bro packages, we have a better idea of which events are/aren’t being used.
Ugh, I didn't consider that. Bro has re-reached the point where touching any number of things can set off an avalanche of problems like this.
Anyone on this thread up for submitting a patch which makes Bro cope with the changes automatically? You can then even mark the old events as deprecated.
The other aspect of Bro changes is in the logging format. This comment isn’t specifically tied the DHCP change, but since we’re talking about breaking things…
With the default TSV, any change can break export into the various back-end log stores and SIEMs. When adding new fields, it would be nice to see them added to the end of the Info structure.
This was a complete rework on the logs and scripts so the structure is completely different. Unfortunately it wasn't just one of the cases where a field or two was added.
I don't think that assuming the order of fields is ever a safe assumption. It's why we shipped a version of bro-cut with Bro 2.0. We wanted to encourage people to refer to fields by the field name rather than the ordinal position of the field.