Introduce generic analyzer_confirmation_info and analyzer_violation_info

[awelzel]

Introduce two new events for analyzer confirmation and analyzer violation
reporting. The current analyzer_confirmation and analyzer_violation
events assume connection objects and analyzer ids are available which
is not always the case. We're already passing aid=0 for packet analyzers
and there's not currently a way to report violations from file analyzers
using analyzer_violation, for example.

These new events use an extensible Info record approach so that additional
(optional) information can be added later without changing the signature.
It would allow for per analyzer extensions to the info records to pass
analyzer specific info to script land. It's not clear that this would be
a good idea, however.

The previous analyzer_confirmation and analyzer_violation events
continue to exist, they are invoked from script-land using the new
analyzer_confirmation_info and analyzer_violation_info events.

The last point yields to a subtle change of behavior visible in some of
the disable-analyzer tests: As the old events are now scheduled from
script-land now, they may be raised after protocol specific events have
been processed already. I presume if that matters to someone we can ask
them to use the new event.

[awelzel]

Related to #2031

[awelzel]

Adding this here for thoughts: @ckreibich mentioned that an analyzer_not_implemented or analyzer_todo event came up in discussions with @vpax (?) to raise information from an analyzers to script land for parts of a protocol that just hasn't been done (rather than using weird or debug logging).

I'm wondering a bit if that would be worth considering a very generic analyzer_event_info and funneling confirm/violation/todo through that. But I don't think it is - having discrete events seems more sensible and common.

[vpax]

Adding this here for thoughts: @ckreibich mentioned that an analyzer_not_implemented or analyzer_todo event came up in discussions with @vpax (?) to raise information from an analyzers to script land for parts of a protocol that just hasn't been done (rather than using weird or debug logging).

I don't recall that. That said, it makes sense to me, and as you then comment, I think it also makes sense to keep the events discrete, rather than bundled.

[rsmmr]

I looked over this high-level: I like the approach with the two new events and passing them records with fields set as a available. I'll go over in more detail but one question: What's the plan for the existing events? This code keeps triggering them, but doesn't deprecate. I think I'd do deprecate and then phase them out over time.

Also wondering if we should keep triggering the old ones from C++ so that we don't change timing for them. While for most events this wouldn't be an issue, I can see it for analyzer_violation that the order may impact something.

[awelzel]

I looked over this high-level: I like the approach with the two new events and passing them records with fields set as a available. I'll go over in more detail but one question: What's the plan for the existing events? This code keeps triggering them, but doesn't deprecate. I think I'd do deprecate and then phase them out over time.

Okay. I had intended to keep them and even thought of introducing a file_analyzer_violation(...) based off the generic events, but seems you're leaning in the other direction. One other motivating factor for not deprecating them was that we just deprecated protocol_violation and introduced analyzer_violation as replacement, and now we're deprecating them again. But, oh well I guess ;-)

Also wondering if we should keep triggering the old ones from C++ so that we don't change timing for them. While for most events this wouldn't be an issue, I can see it for analyzer_violation that the order may impact something.

Yep, okay. I actually ran into the timing issue for the "dpd segement logging" script which gets the current packet, but that already had a comment that it might not be doing the right thing. If we go for deprecation of those event, I wouldn't mind continuing to have the C++ layer raise the events. If we weren't deprecating, that would feel a bit duplicated..

I'll address both as follow-up commits, but will probably rebase once #2379 is in. Maybe better to wait before you look closer?

[rsmmr]

Okay. I had intended to keep them and even thought of introducing a file_analyzer_violation(...) based off the generic events, but seems you're leaning in the other direction.

Yeah, that would get confusing I think.

One other motivating factor for not deprecating them was that we just deprecated protocol_violation and introduced >analyzer_violation as replacement, and now we're deprecating them again.

Yeah, I know, but I'd rather work towards where we want to be eventually, even if that means changing again.

I'd actually prefer to call the new events analyzer_confirmation/violation as well, just with different arguments. But I don't see how we'd get there without breaking existing scripts.

I'll address both as follow-up commits, but will probably rebase once #2379 is in. Maybe better to wait before you look closer?

Ack.

[awelzel]

@rsmmr - I've pushed a few more fixups. I'd rebase on top of master and squash it down and we could possibly chat with this is Zeek 5.1 or 5.2 material. It might be nice to have it in 5.1 as a stepping-stone.

[rsmmr]

Sorry, too late now for 5.1, but yeah, go ahead and rebase on master and then I'll take another look and we can wrap it up.

[awelzel]

Sorry, too late now for 5.1, but yeah, go ahead and rebase on master and then I'll take another look and we can wrap it up.

@rsmmr - I have rebased, squashed and added the "violation only once for packet analyzers" change we talked about.

Let me know if something looks off to you.

[awelzel]

I had looked at analyzer_confirmation/violation a bit more.

• Found packet_analysis: Do not raise analyzer_confirmation per-packet for tunnels #2437 while there. This will conflict with this branch so can also fold it in here if you'd prefer, but could also see it back-port worthy.

• The analyzer/dpd.zeek script within master can currently be tricked into calling disable_analyzer() with a packet analyzers tag and tunnel connections that will have no effect.

• In the same area one can also trigger a dpd.log for every other vxlan packet in a connection by interleaving good and corrupted packets. It's mostly unexpected.

The latter two should be fixed with this PR by raising confirmation/violations only once and also ignoring packet analyzers when attempting to disable analyzers.

[rsmmr]

Looks good, just one question.