DPD: change policy script for service violation logging; add NEWS

This commit renames the `service_violation` column that can be added via
a policy script to `failed_service`. This expresses the intent of it
better - the column contains services that failed and were removed after
confirmation.

Furthermore, the script is fixed so it actually does this - before it
would sometimes add services to the list that were not actually removed.
In the course of this, the type of the column was changed from a vector
to an ordered set.

Due to the column rename, the policy script itself is also renamed.

Also adds a NEWS entry for the DPD changes.

NEWS

@@ -22,6 +22,46 @@ New Functionality
 Changed Functionality
 ---------------------
 
+- The ``service`` field in the connection log is now sorted in the order that
+  protocol analyzers raise their confirmation events.
+  Since the time at which the protocol confirmation is raised depends on the
+  indivivual implementation of each analyzer, there is no specific meaning
+  to the order that the services appear. However, the order should be
+  deterministic between runs. It also will in many cases represent
+  the order in which layered protocols are parsed (e.g. "quic,ssl").
+
+- The way that protocol violations are handled by the dynamic protocol
+  detection (DPD) changed. Now, a violation that is raised by an analyzer
+  before it is confirmed will immediately disable the analyzer. This adjusts
+  the behavior back to the historically desired state, and aligns it with
+  the treatment of confirmed analyzers.
+
+  As a consequence of this, the option ``DPD::max_violations`` is no longer used.
+  It will be retained till Zeek 8.1 to prevent script errors, and raises a
+  deprecation warning.
+
+  The way failed services interact with the ``service`` field in the connection
+  log also changed. In the past, protocol analyzers that were confirmed and
+  later failed were removed from the ``service`` field in some cases. This
+  commonly lead to the case that a protocol log exists, while the service is
+  not listed in the connection.log - so, e.g., an etry in ``http.log`` existing
+  without ``http`` showing up in the connection log.
+
+  Now, protocol analyzers that raised a confirmation event will always be added to
+  the ``service`` field in the connection log, and the entry will be retained
+  even if the analyzer raises a violation later.
+
+  To extend the visibility of protocol violations, a new option
+  ``DPD::track_removed_services_in_connection`` was added. Enabling it causes
+  failed analyzers to be logged to the ``service`` field of the connection log,
+  with a prepended "-". So a connection that attached the ``ssl`` analyzer
+  which later failed due to a protocol error will be logged as ``ssl,-ssl``.
+
+  This change also adds a new policy script,
+  ``protocols/conn/failed-service-logging.zeek``. Loading this script adds the
+  column ``failed_service`` to the connection.log. This column contains the
+  list of protocol analyzers that failed due to a protocol error.
+
 - Command line options processing will no longer print usage whenever there
   is an error. Instead, issues in command line processing will print an error,
   then prompt to use --help. The --help usage will now print to standard output

scripts/policy/protocols/conn/failed-service-logging.zeek

@@ -0,0 +1,38 @@
+##! This script adds the new column ``failed_service`` to the connection log.
+##! The column contains the list of protocols in a connection that raised protocol
+##! violations causing the analyzer to be removed. Protocols are listed in order
+##! that they were removed.
+
+@load base/protocols/conn
+
+module Conn;
+
+redef record Conn::Info += {
+	## List of analyzers in a connection that raised violations
+	## causing their removal.
+	## Analyzers are listed in order that they were removed.
+	failed_service: set[string] &log &optional &ordered;
+};
+
+hook Analyzer::disabling_analyzer(c: connection, atype: AllAnalyzers::Tag, aid: count) &priority=-1000
+	{
+	if ( ! is_protocol_analyzer(atype) && ! is_packet_analyzer(atype) )
+		return;
+
+
+	# Only add if previously confirmed
+	if ( Analyzer::name(atype) !in c$service )
+		return;
+
+	set_conn(c, F);
+
+	local aname = to_lower(Analyzer::name(atype));
+	# No duplicate logging
+	if ( c$conn?$failed_service && aname in c$conn$failed_service )
+		return;
+
+	if ( ! c$conn?$failed_service )
+		c$conn$failed_service = set();
+
+	add c$conn$failed_service[aname];
+	}

scripts/policy/protocols/conn/service-violation-logging.zeek

@@ -1,27 +0,0 @@
-##! This script adds the new column ``service_violation`` to the connection log.
-##! The column contains the list of protocols in a connection that raised protocol
-##! violations causing the analyzer to be removed. Protocols are listed in order
-##! that they were removed.
-
-@load base/protocols/conn
-
-module Conn;
-
-redef record Conn::Info += {
-	## List of protocols in a connection that raised protocol violations
-	## causing the analyzer to be removed.
-	## Protocols are listed in order that they were removed.
-	service_violation: vector of string  &log &optional;
-};
-
-# Not using connection removal hook, as this has to run for every connection.
-event connection_state_remove(c: connection) &priority=4
-	{
-	if ( c?$conn && |c$service_violation| > 0 )
-		{
-		c$conn$service_violation = {};
-		local sv: string;
-		for ( sv in c$service_violation)
-			c$conn$service_violation += to_lower(sv);
-		}
-	}

scripts/test-all-policy.zeek

@@ -98,7 +98,7 @@
 @load misc/unknown-protocols.zeek
 @load protocols/conn/community-id-logging.zeek
 @load protocols/conn/disable-unknown-ip-proto-support.zeek
-@load protocols/conn/service-violation-logging.zeek
+@load protocols/conn/failed-service-logging.zeek
 @load protocols/conn/ip-proto-name-logging.zeek
 @load protocols/conn/known-hosts.zeek
 @load protocols/conn/known-services.zeek

testing/btest/Baseline/scripts.base.frameworks.analyzer.dpd-logging-configuration/conn.log

@@ -5,7 +5,7 @@
 #unset_field	-
 #path	conn
 #open XXXX-XX-XX-XX-XX-XX
-#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents	ip_proto	service_violation
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents	ip_proto	failed_service
-#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count	vector[string]
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count	set[string]
 XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	tcp	ftp,-ftp	9.891089	34	71	SF	T	T	0	ShAdDaFf	13	718	10	599	-	6	ftp
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/scripts/base/frameworks/analyzer/dpd-logging-configuration.zeek

@@ -2,6 +2,6 @@
 # @TEST-EXEC: zeek -r $TRACES/ftp/ftp-invalid-reply-code.pcap %INPUT
 # @TEST-EXEC: btest-diff conn.log
 
-@load policy/protocols/conn/service-violation-logging
+@load policy/protocols/conn/failed-service-logging
 
 redef DPD::track_removed_services_in_connection = T;

testing/external/scripts/testing-setup.zeek

@@ -1,7 +1,8 @@
 # Sets some testing specific options.
 
 @load external-ca-list
-@load protocols/conn/service-violation-logging
+
+@load protocols/conn/failed-service-logging
 
 @ifdef ( SMTP::never_calc_md5 )
 	# MDD5s can depend on libmagic output.