Event/init-bare: Add add_missing_remote_network_timestamp logic

Make defaulting to the local network timestamp for remote events opt-in.

NEWS

@@ -30,6 +30,13 @@ Breaking Changes
   dispatched. Previously this would've likely been 0.0, or the previously
   dispatched event.
 
+- Missing network timestamp metadata on remote events is not set to the local
+  network time anymore by default. This potentially hid useful debugging information
+  about another node not sending timestamp metadata. The old behavior can be
+  re-enabled as follows:
+
+	redef EventMetadata::add_missing_remote_network_timestamp = T;
+
 New Functionality
 -----------------
 

scripts/base/init-bare.zeek

@@ -601,6 +601,16 @@ export {
 	## might be a value before the network_time() when the event
 	## was actually dispatched.
 	const add_network_timestamp: bool = F &redef;
+
+	## By default, remote events without network timestamp metadata
+	## will yield a negative zeek:see:`current_event_time` during
+	## processing. To have the receiving Zeek node set the event's
+	## network timestamp metadata with its current local network time,
+	## set this option to true.
+	##
+	## This setting is only in effect if :zeek:see:`EventMetadata::add_network_timestamp`
+	## is also set to true.
+	const add_missing_remote_network_timestamp: bool = F &redef;
 }
 
 module FTP;

src/Event.cc

@@ -11,6 +11,7 @@
 #include "zeek/Val.h"
 #include "zeek/iosource/Manager.h"
 #include "zeek/plugin/Manager.h"
+#include "zeek/util.h"
 
 #include "const.bif.netvar_h"
 #include "event.bif.netvar_h"
@@ -165,12 +166,19 @@ void EventMgr::Enqueue(const EventHandlerPtr& h, Args vl, util::detail::SourceID
     detail::EventMetadataVectorPtr meta;
 
     double ts = double(deprecated_ts);
-    if ( src == util::detail::SOURCE_LOCAL && BifConst::EventMetadata::add_network_timestamp ) {
+
     // If this is a local event and EventMetadata::add_network_timestamp is
-        // enabled automatically set the network timestamp for this event to the
+    // enabled, automatically set the network timestamp for this event to the
-        // current network time when it is < 0 (default is -1.0).
+    // current network time when it is < 0 (default of deprecated_ts is -1.0).
     //
-        // See the other Enqueue() implementation for the local motivation.
+    // See the other Enqueue() implementation for the local vs broker/remote
+    // motivation of want_network_timestamp.
+    bool want_network_timestamp =
+        BifConst::EventMetadata::add_network_timestamp &&
+        ((src == util::detail::SOURCE_LOCAL) ||
+         (src == util::detail::SOURCE_BROKER && BifConst::EventMetadata::add_missing_remote_network_timestamp));
+
+    if ( want_network_timestamp ) {
         if ( ts < 0.0 )
             ts = run_state::network_time;
 
@@ -189,15 +197,23 @@ void EventMgr::Enqueue(const EventHandlerPtr& h, Args vl, util::detail::SourceID
 
 void EventMgr::Enqueue(detail::EventMetadataVectorPtr meta, const EventHandlerPtr& h, Args vl,
                        util::detail::SourceID src, analyzer::ID aid, Obj* obj) {
-    if ( src == util::detail::SOURCE_LOCAL && BifConst::EventMetadata::add_network_timestamp ) {
+    // Attach network timestamps to all events if EventMetadata::add_network_timestamp is T and
-        // If all events are supposed to have a network time attached, ensure
-        // that the meta vector was passed *and* contains a network timestamp.
     //
-        // This is only done for local events, however. For remote events (src == BROKER)
+    //   1) this event is locally generated
-        // that do not hold network timestamp metadata, it seems less surprising to keep
+    //   or
-        // it unset. If it is required that a remote node sends *their* network timestamp,
+    //   2) this is a remote event and EventMetadata::add_missing_remote_network_timestamp is T
-        // defaulting to this node's network time seems more confusing and error prone
+    //
-        // than just leaving it unset and having the consumer deal with the situation.
+    // Why so complicated? It seems less surprising behavior to keep network timestamp metadata unset
+    // if a remote event didn't have any attached. It should help to more easily figure out what's
+    // actually going on compared to setting it to the local network time. If all nodes are required to
+    // send *their* network timestamp, filling it with this node's network time seems more confusing
+    // and error prone compared to just leaving it unset and having the consumer deal with the situation.
+    bool want_network_timestamp =
+        BifConst::EventMetadata::add_network_timestamp &&
+        ((src == util::detail::SOURCE_LOCAL) ||
+         (src == util::detail::SOURCE_BROKER && BifConst::EventMetadata::add_missing_remote_network_timestamp));
+
+    if ( want_network_timestamp ) {
         bool has_time = false;
 
         if ( ! meta ) {

src/const.bif

@@ -36,3 +36,4 @@ const Log::write_buffer_size: count;
 const Storage::expire_interval: interval;
 
 const EventMetadata::add_network_timestamp: bool;
+const EventMetadata::add_missing_remote_network_timestamp: bool;

testing/btest/broker/remote_event_ts_compat.zeek

@@ -19,6 +19,11 @@ redef exit_only_after_terminate = T;
 redef allow_network_time_forward = F;
 redef EventMetadata::add_network_timestamp = T;
 
+# This is needed so that the receiving node sets its
+# own local network timestamp on remote events that do
+# not have network timestamp metadata.
+redef EventMetadata::add_missing_remote_network_timestamp = T;
+
 event zeek_init()
 	{
 	Broker::subscribe(getenv("TOPIC"));