Re: [PATCH 0/5] Add Zabbix functionality to suricata-reporter

[Robin Roevens <robin.roevens@disroot.org>]

Hi Michael

Michael Tremer schreef op vr 28-08-2026 om 21:21 [+0200]:
> Hello Robin,
> 
> > On 28 Aug 2026, at 00:51, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > Hi Michael
> > 
> > Vacation period here is officially over.. So I have no more excuses
> > and
> > I'm ready to dive into this again :-)
> 
> Haha, I hope you had a relaxing time and didn’t think too much about
> this.
> 
> > Michael Tremer schreef op vr 31-07-2026 om 11:24 [+0100]:
> > > Hello Robin,
> > > 
> > > Thank you very much for sending these patches.
> > > 
> > > Before we dig into the code, I have a couple of questions about
> > > the
> > > design...
> > Ok, I will try to answer them first, as discussing this may result
> > in
> > significant design changes :-)
> 
> Probably only less code because we can let SQLite do all the work :)
> 
> > > 
> > > > On 30 Jul 2026, at 20:15, Robin Roevens
> > > > <robin.roevens@disroot.org>
> > > > wrote:
> > > > 
> > > > Hi all,
> > > > 
> > > > As discussed here earlier, I've worked on implementing sending
> > > > Suricata alerts straight to Zabbix from within suricata-
> > > > reporter
> > > > instead
> > > > of trying to parse the suricata logging separately using the
> > > > Zabbix
> > > > agent.
> > > > 
> > > > For this I use the zabbix-utils python library, which I
> > > > submited
> > > > here
> > > > also as a separate pak (but meanwhile already requires an
> > > > update,
> > > > which
> > > > I will post soon). This set of patches makes suricata-reporter
> > > > able
> > > > to
> > > > directly communicate to a Zabbix server without having the
> > > > zabbix_agentd
> > > > pak installed, sending suricata alerts in real-time.
> > > 
> > > Yes, this is a good choice and I like that suricate-reporter will
> > > try
> > > to load support for Zabbix and if the module is not available, it
> > > simply disables support for Zabbix. That allows us to have a
> > > smaller
> > > configuration file if things like this are auto-detected.
> > > 
> > > > As Zabbix supports sending items in bulk, I have opted to
> > > > create an
> > > > async background task that will send all events from last 1
> > > > second
> > > > in
> > > > bulk so that even in the case that there are hundreds of
> > > > incoming
> > > > alerts, Zabbix server is only contacted once per second.
> > > 
> > > Okay, this makes sense. But I believe that there is already a
> > > small
> > > race in the implementation:
> > > 
> > > If the client side (in this case suricata-reporter) does not
> > > finish
> > > the call of flush_pending_to_zabbix() within that second, it will
> > > be
> > > called again which will result in the same rows being selected
> > > again,
> > > transmitted again, and assuming that there are just thousands of
> > > alarms it will take over a second again, the function will be
> > > called
> > > again, and so on. So the application will stall very quickly.
> > > 
> > > Although we should not see thousands of alerts per second under
> > > normal conditions, there could be other reasons why this is
> > > taking
> > > some time. For example, the Zabbix host could be in a different
> > > location and round-trips around half the planet are taking some
> > > time;
> > > it could be busy writing other things to its database or the
> > > database
> > > has just decided to do a little cleanup job. One second isn’t a
> > > lot
> > > of time then and we will have to make the system a little bit
> > > more
> > > resilient against this.
> > 
> > Is this really the case? For what I understood of the Python async
> > methods is that by using await in:
> > 
> > async def _periodic_zabbix_sender_flush(self):
> >   ...
> >   if await self.flush_pending_to_zabbix():
> >    ...
> >   await asyncio.sleep(1)
> > 
> > The task will wait for the flush/sending to complete before waiting
> > 1
> > sec so the next itteration should not be able to begin until
> > flush_pending_to_zabbix effectively returns.
> > Hence if the sending would take 20s the flow would be:
> > flush starts 
> >  -> send pending events, taking 20s
> >  -> wait 20s until flush_pending_to_zabbix finishes
> >  -> wait an additional 1s
> >  -> select pending rows again  
> > So there should be no overlaps with previous calls and the same
> > rows
> > are not concurrently selected and transmitted by this task.
> 
> Yes, you are right.
> 
> But don’t we need some changes so that this cannot completely block
> the IO loop?
> 
> Right now, the entire process would pause at the "await
> self.flush_pending_to_zabbix()” stage which means that we are not
> able to collect any events from the Suricata socket.

Not necessarily, I think.. For as far as I have understood it, await
itself is "cooperative" and should allow the loop to continue doing
it's work as long as the awaited function doesn't do long-running
synchronous calls.. In current implementation, I think only the DB
query to select the queries is synchronous as we use the async zabbix
sender. So as long as the DB query itself is fast enough, it should not
block the main loop, even if the zabbix sending takes too long as that
should happen asynchronous.

To mitigate possible slow DB queries, I'm thinking about limiting them
to a max of zabbix_utils chunk size (currently 250), resulting in
possibly faster queries (in case there are tons of events to return)
and a better control over the zabbix sender functionality and faster
recording of the succesfully sent events in a separate table.
This should keep the synchronous work inside the function to a minimum.

Alternatively we could write a _trigger_zabbix_flush that would launch
a separate task for the work to be done in the background.. But then we
will need to mitigate possible race conditions as you described earlier
as that would then effectively be a possibility

> 
> I suppose we can leave this code as is for now and address the other
> things first as it does the job. But I think we might be able to come
> up with a solution here that gives us some stronger guarantees.

For as far as I understand all the async python, I think the risks
should already be quite small currently as long as the DB is
responsive. If the DB is not responsive, the main loop would also have
troubles inserting new events into the DB, so as long as I try to keep
the DB queries in check...?

> 
> > > 
> > > > When for some reason sending to Zabbix server fails, it will be
> > > > retried
> > > > 3 times and then the background task will be suspended until a
> > > > new
> > > > suricata event comes in. That will wake the task again and
> > > > retry to
> > > > send all
> > > > pending events. In environments with many events, that may
> > > > actually
> > > > not
> > > > have that much of an effect. But in the average environment,
> > > > this
> > > > will
> > > > give the Zabbix Server some breathing space as it failing to
> > > > receive our
> > > > events, may indicate a Zabbix server overload.
> > > 
> > > Good thinking here.
> > > 
> > > > For this I have to keep track which events are sent and which
> > > > are
> > > > pending. So I added a column in the database that keeps track
> > > > of
> > > > that.
> > > 
> > > So, this is a very crucial thing we probably need to discuss :)
> > > 
> > > What is the rationale behind this? Obviously there are some easy
> > > answers:
> > > 
> > > 1) We don’t want to loose any history if the network or Zabbix is
> > > down
> > > 
> > > 2) We can even restart the reporter without losing any alerts
> > 
> > I would add that it can even be killed, crash, powerfail,
> > kernelfail or
> > any other disaster may happen. When it restarts (and still has its
> > database) the events won't be lost :-)
> 
> Well, if there is no power, there is no code that we can run. So no
> matter how smart we are, it won’t work.
> 
> > > 
> > > But then I am already running out of ideas why this could be a
> > > good
> > > idea. The cons that I can see are:
> > > 
> > > * A lot of additional I/O on the database. Although we would be
> > > updating rows very briefly after they have been written to the
> > > database, it will create a copy of the row and change the append-
> > > only
> > > architecture of the database. It will have a lot more cleaning up
> > > to
> > > do to evict all updated rows.
> > 
> > True
> > 
> > > 
> > > * You will only ever go back by about 1h by default. Could we
> > > just
> > > not keep things in RAM for that long?
> > 
> > I would rather not only keep it in memory. On systems with only one
> > alert every x time, that won't be a problem, but on systems with
> > many
> > alerts per second, we risk losing many alerts by any failure. 
> > Maybe postponing DB writes a few alert-batch sends is possible, but
> > with memory-only the risk of lost alerts is too high for me.
> > I considered only updating the DB on shutdown, but an unexpected
> > shutdown/crash would then potentially cause large replay bursts
> > depending on how long reporter has been running, which could be
> > days,
> > months, years (hopefully not, as they should upgrade their IPFire
> > regularly ;-))
> 
> I agree. Memory can be helpful and be used for caching, but we want
> to have guarantees that we have done our best to submit any alerts to
> Zabbix and anything else.
> 
> I don’t think that we should try too hard to save any IO operations,
> because with everyone on SSD storage, these are becoming all
> extremely cheap.
> 
> > > I am not saying that I hate the idea, but I am not sure whether
> > > it is
> > > worth paying the price. The good side is that if people are not
> > > using
> > > Zabbix, there is no overhead except the space for the extra
> > > column.
> > > But if we would add another monitoring solution, we would
> > > potentially
> > > have to add another field, and another, and another?
> > 
> > That is indeed one of the goals: no extra overhead when Zabbix is
> > not
> > used. But I do think if people bother to set up a proper monitoring
> > and/or logging system, they generally would like the data flow as
> > robust as possible. Such systems can also be configured to react on
> > incoming data, possible starting whole workflows, making it even
> > more
> > important that there is no data missing.
> > 
> > I may have a solution for adding more alert consumers a bit further
> > in
> > this mail. 
> > 
> > > 
> > > So a possible other solution that I can come up with would be:
> > > Creating a separate table with all pending events that have to be
> > > transmitted. And every once in a while we truncate it should it
> > > become too long. We could even keep a list of IDs in memory only
> > > if
> > > we want to go down that route.
> > > 
> > 
> > Considering your valid remarks, I have been rethinking possible
> > other
> > methods, trying to keep the alerts-table append-only and SQLite
> > work
> > minimal and came up with 3 alternatives to the current method:
> > 
> > * A separate table for keeping the queue of event id's to be sent
> > -> Pending queries should be cheaper and the table would remain
> > bounded
> > if delivery keeps up.
> > However, it would require deletes for every event that is sent,
> > still
> > causing extra SQLite writes and cleanup work, especially with high
> > event counts. I don't see how to implement once-in-a-while
> > truncating
> > on this kind of table if we are not updating every event once it is
> > sent to actually mark it as sent. So I don't think this would be
> > less
> > work for SQLite?
> > Also a crash between inserting an alert in the alert table and
> > adding
> > the alert to the queue table would cause missing deliveries or it
> > should be done in one transaction, possible causing longer lock
> > times.
> > So I don't think that is a good solution.
> > 
> > * A separate table for keeping successfully sent event id's
> > -> This makes the separate table also append-only and would allow
> > for
> > once-in-a-while truncating the table as you suggest.
> > It would require a query on the alerts table using NOT EXISTS which
> > will probably be a little more costly for SQLite than current
> > method or
> > the pending table method, although an indexed primary key should
> > make
> > this inexpensive enough?
> > Worst case after a crash between sending a batch of events
> > succesfully
> > and recording it in this table, would be that those events would be
> > sent again, causing duplicate entries in Zabbix. But in my opinion
> > it
> > is better to have an alert reported twice than missing it.
> 
> I like this option, because it gives us a lot of advantages.
> 
> We could basically decide what the window is we are interested in and
> truncate the table from that point. If we submit any new events to
> Zabbix, we create a row and mark it as successful. That way, we can
> easily check what alerts inside our window have been successfully
> transmitted.
> 
> If something does not transmit, we can still add the row and keep
> some state here. We could add a “try again after” timestamp and a
> counter of how many attempts we have done. That way, this will be
> persistent and we won’t just fire and forget too much.
> 
> The table itself would be really small and with a regular truncate,
> SQLite will be able to re-use the same pages over and over again.
> 
> The solution above is very similar but would basically create rows
> even though Zabbix is not in use. Or we add some complex logic, but
> we basically make the INSERT of a new event more complicated because
> multiple tables are being touched. This solution keeps the second
> table independent.

Ok, I've done some more thinking/analysing about this solution then,
combining it with the consumer idea below:

We can create an alerts_delivery table with colums for alert_id,
consumer, sent_at-timestamp, failed-flag, retry_after-timestamp and
retry_attempts-count.

- When an alerts batch is sent successfully to Zabbix, the individual
alerts are recorded in the table as successful.
- When an alerts batch to Zabbix has failed as a whole (with an
exception in python indicating network or communication error) they
won't be recorded, so that they are retried as if they where never
tried before on the next second. Pausing the process after 3 retries
until a new event comes in (exactly like the current logic).
- When an alerts batch gets sent to Zabbix, but Zabbix replies about
failed alerts, the alerts will be recorded in the table as failed and
retried individually at retry_after-timestamp until retry_attempts-
count reached a configurable max_retry_count. Retry_after-timestamp can
be increased with an exponential backoff after each retry (maybe up to
a specific max backoff-time to prevent )

Periodically the table gets cleaned up removing all successful and
permanently failed entries older than a preset max_alert_age, based on
the sent_at-timestamp, but still keeping retryable entries, for in case
the user has set a very high max_retries setting ?

To determine which events to send to Zabbix, currently they are added
with a pending = 1 in the alerts table if zabbix = enabled, which made
it very easy, but we will no longer have with this method..
I was thinking, during zabbix_sender_init, I can fetch the currently
latest alert-id and keep that in memory as start_alert_id, so I can
query alerts with alert_id > start_alert_id and not exists in
alters_delivery table with consumer=zabbix. I considered timestamps,
but that can start behaving strange and unexpected if time is for some
reason changed during runtime. 


> 
> > * Using a high-watermark in a separate "state" table
> > -> This would record only the last successfully sent alert ID and
> > update it when a new alert or batch of alerts is successfully sent.
> > So even with a high alert rate, only a single update is required
> > after
> > send a batch of alerts succesfully. So in worst case there is still
> > only a single update once per second.
> > The query on the alerts table would also be cheap only using a ">"
> > equation on the primary key.
> 
> In theory this is an option, but in reality things might become
> complicated. I never consider an ID strictly incrementing. Integers
> could wrap around and we could have different transactions committed
> at different times which results in rows with lower IDs becoming
> visible to other processes later.
> 
> A solution could be a timestamp because that would at least solve the
> problem with the counter not wrapping around.

Except for when the system time was wrong during start of reporter and
it gets adjusted during runtime.. then time could be running backwards
or even jump if it is set manually.

The chances of the id wrapping around are not very high I think. The
integer primary key that alert.id currently is should be a signed 64bit
integer with the largest value being 2^63 - 1 =~ 9.22 * 10^18. So even
when 1 million rows are inserted every second, it would still take
about 9.22 * 10^12 seconds or roughly 292.000.000 years. By the time
that wraps around, I assume me and you won't be around anymore, and the
system that may be running suricata-reporter that long will probably
also not be very relevant anymore by that time..
So I don't think we have to be afraid of the ID wrapping around.
Arbitrary time adjustments are a far greater risk.

I concur about the risk of of getting lower IDs becoming visible to
other processes later, when reporter becomes much more complex than it
currently is. So the high watermark idea then indeed will need some
inventive hacking to work around that. Keeping the modified successful
alerts table is then probably the best out of my 3 proposals..
There I will need to use the last ID in DB + 1 as the first ID to
include in next zabbix-send-task during startup/zabbix init and I
assume there won't be much risk of lower ID's coming in at that point
in the code ever. Depending on a timestamp there would be much more
risky, in my opinion.


> 
> With SQLite, we are not very likely to have many concurrent
> transactions, but if this grows bigger, we might run a PostgreSQL
> database or something similar, or even make some other design
> changes, so I would rather be careful now and now make my own life
> harder in the future.
> 
> > It could look something like:
> > 
> > zabbix_state
> > ------------
> > last_sent_id INTEGER NOT NULL
> > 
> > it could even be used for possible additional future consumers or
> > maybe
> > even for tracking succesfully sent alerts by syslog or email?
> > 
> > consumer_state
> > ------------
> > consumer       TEXT PRIMARY KEY  -> in this case consumer =
> > "zabbix"
> > last_sent_id   INTEGER NOT NULL
> 
> I like the consumer idea, because the table with the successfully
> transmitted events could have this row and we already have a solution
> that allows us to extend this all to other monitoring solutions.
> 
> > Caveats are that I must make sure alerts are always sent in order
> > of
> > their ID and when an alert failed to be sent, it would block newer
> > alerts to be sent.
> 
> I know that some people add a lag or something with a timestamp, but
> I consider this way too hacky.
> 
> > And as I send alerts in bulk, which in turn is chopped into chunks
> > by
> > zabbix_utils itself, it is possible that out of 3 chunks the middle
> > chunk failed and in that case there are events with higher ID's
> > sent
> > and lower ID's that failed, rendering this method unusable. So I
> > will
> > then need to split the batches into chunks <= zabbix_utils chunk-
> > size
> > and send them separately myself. Possibly generating more DB writes
> > within a second. (However current default chunk-size is 250, so it
> > takes > 250 alerts within a second to cause an extra DB update)
> > But then I still risk that if, for some reason a single alert
> > consequently fails to be sent (I don't think this should ever
> > happen,
> > but you never know..Maybe a bug in Zabbix failing to parse the
> > event
> > due to some unexpected character or something like that?), would
> > still
> > block any subsequent alert to be sent. 
> 
> I have been working on similar software that sends data to AWS SQS
> and ElasticSearch and this has indeed been a problem. A bunch of
> messages that simply could not be parsed and the software was looping
> for forever.
> 
> So there should be some way to at least give up at some point.
> 
> > And by batch-sending, there is unfortunately no way of knowing
> > which
> > alert(s) in the batch failed, and which where successfully
> > accepted.
> > Zabbix server only returns how much have failed and how much have
> > succeeded. Currently I retry sending the whole chunk for an hour
> > (by
> > default), but I don't block newer events.
> 
> Hmm, this is slightly bad design of the API because we could simply
> drop that alerts and send the rest again. Other solutions could
> simply be to attempt submitting everything individually if the batch
> was not successful as a whole. But I am not sure whether we are able
> to get a clear exception raised to judge that.

When sending to Zabbix itself was successful, it always replies with
how many values are successfully processed and how many have failed.
There will be no python exceptions, but it can be read from the reply.
So as proposed above, I would then mark all alerts in the batch as
failed and start sending them individually. This will result in
duplicates in Zabbix, but in the end we will know which alerts failed
to process (if those then still fail).

> 
> > In this method a failed batch would be resent indefinitely so some
> > sane
> > threshold should also be implemented here. Possibly something like
> > retries=3.. however this would risk dropping alerts too soon when
> > Zabbix or the network is effectively having troubles itself.. Not
> > sure
> > yet how to handle this, maybe retries=3 if there are any
> > succesfully
> > sent alerts in the batch and up to 1 hour if all events in the
> > batch
> > fail. Or something like that..
> 
> That would be indeed bad. We cannot send indefinitely. But we could
> store a counter for each attempt and then set a limit of 5 or maybe
> even 10 times if we want to try very hard.
> 
> > Or I could try implementing to recursively split a failed batch
> > until
> > such a possible single failing alert is separated, and then drop
> > that
> > alert, but I think that feels quite overkill as this situation
> > should
> > actually never happen.
> 
> Hopefully not.
> 
> > This last method introduces some challenges, but I think it has a
> > very
> > high potential of being extremely cheap, both on database
> > operations
> > and size as only ever one single row is maintained, and makes it
> > also
> > cheap to add even more alert consumers, therefore may be worth
> > investigating deeper?
> 
> I think the extra table is cheap enough. Each row will hold the ID (8
> bytes), when we tried last or after when we want to try again (8
> bytes), as well as a counter (8 bytes). In total that would be 24
> bytes, so a megabyte of data on disk would hold around 45,000 entries
> - not considering any overhead. That would be quite a lot of records
> in the window of one hour.
> 
> Cheaper is possible, but then we will have to find solutions for
> other problems. This one is simple and extensible to other solutions,
> too.

Ok, then I will go for method 2 as explained above ? 

Regards
Robin

> 
> > 
> > > > I have also added an alert_max_age config parameter that allows
> > > > the
> > > > user
> > > > to set how long suricata-reporter should retry to send events
> > > > to
> > > > Zabbix.
> > > > Events older than that set age, will no longer be sent to
> > > > Zabbix.
> > > > This also give the user the implicit option to send older
> > > > events
> > > > when
> > > > only just enabling the zabbix sending functionality, since the
> > > > DB
> > > > column
> > > > exists and no event was ever sent to Zabbix, all events will be
> > > > 'pending". At first run with zabbix functionality enabled, all
> > > > events up
> > > > to alert_max_age that are in the database will be sent to
> > > > zabbix
> > > > immediatly.
> > > 
> > > I like the mechanism, but whenever I am building something like
> > > this,
> > > I am never sure what would be a reasonable window.
> > 
> > Me neither, I think this highly depends on the user's
> > infrastructure
> > and/or needs. If network outages or zabbix server outages are
> > expected
> > to be longer than 1 hour in some environments, then 1 hour is
> > probably
> > not a good threshold.. Therefore I would definitely make it user
> > configurable. But a default of one hour, feels sane to me.
> 
> Agreed.
> 
> > > 
> > > Locally, suricate-reporter is keeping the events for pretty much
> > > forever. So we could even go back three days or something. Or we
> > > could give up really quickly. After maybe a minute. I never know
> > > what
> > > is right, but for the implementation, the length of the window
> > > plays
> > > a role - see above.
> > > 
> > > With email and syslog we do more of a “fire and forget” approach.
> > > If
> > > we send the syslog message and syslog wasn’t ready to receive it,
> > > we
> > > wouldn’t know and we would not try again...
> > 
> > If you have no way of knowing, there are no other options, I think.
> > But
> > in the case of Zabbix, we do know. And knowing how much fuss the
> > SoC
> > team at my work makes when some security related logs are missing,
> > I
> > think many really do like it to be as reliable as possible when
> > exporting the alerts to a monitoring or other collecting system. 
> 
> They are right. We should try really hard so that Zabbix sees the
> full picture.
> 
> > > 
> > > > All events sent to Zabbix contain the timestamp of retrieval by
> > > > suricata-reporter, so Zabbix will register and order them as
> > > > received on that
> > > > timestamp independently of the actual time Zabbix itself
> > > > received
> > > > the
> > > > event.
> > > > 
> > > > This is my first adventure in Python async programming, so I
> > > > hope I
> > > > did
> > > > not make any flagrant mistakes. But the code has been running
> > > > here
> > > > for
> > > > weeks now without any problem. I have not actually tested large
> > > > bursts
> > > > of events, as I could not simulate that.. But I did make Zabbix
> > > > server
> > > > slow, unavailable and finally replaced it with netcat (to
> > > > accept
> > > > the connection, but
> > > > not react on it) and I had the connection with the server off
> > > > for a
> > > > few
> > > > hours to then re-establish the connection to see hundereds of
> > > > pending events 
> > > > being registered in only a few milliseconds.
> > > > I did not notice any problems with suricata-reporter in any of
> > > > these
> > > > cases.
> > > 
> > > This is good testing. Usually, if I need to create a lot of
> > > events, I
> > > enable the “PING” rule in “icmp_info” and just send a lot of ping
> > > packets to the firewall. You could try a flood ping with “ping -
> > > f”.
> > > 
> > > I will send some more comments about the code in the other
> > > emails.
> > 
> > For now I will await your (or maybe other list members'?) reaction
> > to
> > above db approach proposals before diving into your code comments.
> > But
> > I will make sure to review and properly implement/consider or
> > answer
> > them when I start changing the code based on the outcome of current
> > discussion.
> 
> Cool. Feel free to ask questions on the way and we will get this all
> done in no time!
> 
> All the best,
> -Michael
> 
> > Regards
> > Robin
> > > 
> > > Best,
> > > -Michael
> > > 
> > > > 
> > > > Regards
> > > > 
> > > > Robin
> > > > 
> > > > -- 
> > > > Dit bericht is gescanned op virussen en andere gevaarlijke
> > > > inhoud door MailScanner en lijkt schoon te zijn.
> > > > 
> > > > 
> > > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> 
> 
> 

-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.