Fix inflated RADIUS Acct-Session-Time across reconnects

A session that reconnects under the same cookie (roaming, DPD, a new-tunnel
rekey) reported an Acct-Session-Time far larger than the real duration. Each
connection segment reported uptime measured from the original session start
(now - e->created, as session_start_time is not reset across reconnects), and
sec-mod summed these per-segment cumulative values, so the reported time grew
~= D*(N+1)/2 with the number of reconnects.

Acct-Session-Time is the wall-clock lifetime of the logical session and is a
pure function of the session creation time, so it does not need to be reported
by the worker, carried through cli_stats_msg / secm_session_close_msg, or
accumulated alongside the byte counters. Drop stats_st.uptime and the uptime
fields of both IPC messages, and compute it in sec-mod as now - e->created:
live for each interim update, and snapshotted at disconnect for the Stop so the
cookie-timeout lingering period is not counted. The byte counters keep their
per-segment report-and-sum handling.

Signed-off-by: Alex Protsko <fidget2015@yahoo.com>

doc/README-radius.md

@@ -66,6 +66,16 @@ possible when the user disconnects explicitly. When the disconnection
 is due to timeout or other network reasons, the users have their connection
 remain valid until the `cookie-timeout` value expires.
 
+The `Acct-Session-Time` reported is the wall-clock lifetime of the logical
+session: the time from the initial authentication to the last activity of
+the session. A single logical session spans all reconnections performed
+under the same cookie (e.g. roaming, DTLS rekey, or a brief link loss), so
+an idle gap between such reconnections is included in `Acct-Session-Time`.
+The value is bounded by `session-timeout` (the session is torn down once it
+is reached), and any idle gap that can be folded in is bounded by
+`cookie-timeout`, after which a fresh authentication starts a new accounting
+session.
+
 
 Dictionary
 ==========

doc/sample.config

@@ -271,6 +271,11 @@ rate-limit-ms = 100
 # worker process will report its usage statistics (number of
 # bytes transferred etc). This is useful when accounting like
 # radius is in use.
+#
+# Note: the reported Acct-Session-Time is the wall-clock lifetime of the
+# logical session (initial authentication to last activity); it spans
+# reconnections under the same cookie and is bounded by session-timeout.
+# See doc/README-radius.md for details.
 # [scope: vhost user]
 #stats-report-time = 360
 

src/acct/radius.c

@@ -88,12 +88,13 @@ static void acct_radius_vhost_deinit(void *_vctx)
 		rc_destroy(vctx->rh);
 }
 
-static void append_stats(rc_handle *rh, VALUE_PAIR **send, stats_st *stats)
+static void append_stats(rc_handle *rh, VALUE_PAIR **send, stats_st *stats,
+			 time_t uptime)
 {
 	uint32_t uin, uout;
 
-	if (stats->uptime) {
-		uin = (uint32_t)MIN(stats->uptime, UINT32_MAX);
+	if (uptime) {
+		uin = (uint32_t)MIN(uptime, UINT32_MAX);
 		rc_avpair_add(rh, send, PW_ACCT_SESSION_TIME, &uin, -1, 0);
 	}
 
@@ -194,7 +195,7 @@ static void radius_acct_session_stats(void *_vctx, unsigned int auth_method,
 	}
 
 	append_acct_standard(vctx, vctx->rh, ai, &send);
-	append_stats(vctx->rh, &send, stats);
+	append_stats(vctx->rh, &send, stats, ai->uptime);
 
 	ret = rc_aaa(vctx->rh, 0, send, &recvd, NULL, 0, PW_ACCOUNTING_REQUEST);
 
@@ -294,7 +295,7 @@ static void radius_acct_close_session(void *_vctx, unsigned int auth_method,
 	rc_avpair_add(vctx->rh, &send, PW_ACCT_TERMINATE_CAUSE, &ret, -1, 0);
 
 	append_acct_standard(vctx, vctx->rh, ai, &send);
-	append_stats(vctx->rh, &send, stats);
+	append_stats(vctx->rh, &send, stats, ai->uptime);
 
 	ret = rc_aaa(vctx->rh, 0, send, &recvd, NULL, 0, PW_ACCOUNTING_REQUEST);
 	if (recvd != NULL)

src/ipc.proto

@@ -137,7 +137,9 @@ message cli_stats_msg
 	required uint64 bytes_in = 1;
 	required uint64 bytes_out = 2;
 	optional bytes sid = 3;
-	required int64 uptime = 4;
+	/* field 4 (uptime) removed: session uptime is derived from the
+	 * session creation time in sec-mod, not reported by the worker */
+	reserved 4;
 	optional string remote_ip = 5;
 	optional string ipv4 = 6;
 	optional string ipv6 = 7;
@@ -320,7 +322,9 @@ message secm_session_open_msg
 message secm_session_close_msg
 {
 	required bytes sid = 1; /* cookie */
-	optional int64 uptime = 3;
+	/* field 3 (uptime) removed: session uptime is derived from the
+	 * session creation time in sec-mod, not reported by main */
+	reserved 3;
 	optional uint64 bytes_in = 4;
 	optional uint64 bytes_out = 5;
 	optional string ipv4 = 6;

src/main-sec-mod-cmd.c

@@ -744,8 +744,6 @@ int session_close(sec_mod_instance_st *sec_mod_instance, struct proc_st *proc)
 
 	PROTOBUF_ALLOCATOR(pa, proc);
 
-	ireq.uptime = time(NULL) - proc->conn_time;
-	ireq.has_uptime = 1;
 	ireq.bytes_in = proc->bytes_in;
 	ireq.has_bytes_in = 1;
 	ireq.bytes_out = proc->bytes_out;

src/sec-mod-auth.c

@@ -465,7 +465,6 @@ static void stats_add_to(stats_st *dst, stats_st *src1, stats_st *src2)
 {
 	dst->bytes_out = src1->bytes_out + src2->bytes_out;
 	dst->bytes_in = src1->bytes_in + src2->bytes_in;
-	dst->uptime = src1->uptime + src2->uptime;
 }
 
 static int send_failed_session_open_reply(sec_mod_st *sec, int fd)
@@ -654,9 +653,11 @@ int handle_secm_session_close_cmd(sec_mod_st *sec, int fd,
 				(pack_func)cli_stats_msg__pack);
 	}
 
-	if (req->has_uptime && req->uptime > e->stats.uptime) {
-		e->stats.uptime = req->uptime;
-	}
+	/* Acct-Session-Time is the wall-clock lifetime of the logical session
+	 * (now - e->created). Snapshot it at disconnect so the Stop does not
+	 * also count the cookie-timeout period during which the session lingers
+	 * with no connected worker. */
+	e->acct_info.uptime = time(NULL) - e->created;
 	if (req->has_bytes_in && req->bytes_in > e->stats.bytes_in) {
 		e->stats.bytes_in = req->bytes_in;
 	}
@@ -744,8 +745,6 @@ int handle_sec_auth_stats_cmd(sec_mod_st *sec, const CliStatsMsg *req,
 		e->stats.bytes_in = req->bytes_in;
 	if (req->bytes_out > e->stats.bytes_out)
 		e->stats.bytes_out = req->bytes_out;
-	if (req->uptime > e->stats.uptime)
-		e->stats.uptime = req->uptime;
 
 	if (req->has_discon_reason && req->discon_reason != 0) {
 		e->discon_reason = req->discon_reason;
@@ -769,6 +768,9 @@ int handle_sec_auth_stats_cmd(sec_mod_st *sec, const CliStatsMsg *req,
 		strlcpy(e->acct_info.ipv6, req->ipv6,
 			sizeof(e->acct_info.ipv6));
 
+	/* live wall-clock session lifetime for this interim update */
+	e->acct_info.uptime = time(NULL) - e->created;
+
 	e->vhost->static_config.acct.amod->session_stats(
 		e->vhost_acct_ctx, e->auth_type, &e->acct_info, &totals);
 

src/sec-mod.h

@@ -56,7 +56,6 @@ typedef struct sec_mod_st {
 typedef struct stats_st {
 	uint64_t bytes_in;
 	uint64_t bytes_out;
-	time_t uptime;
 } stats_st;
 
 typedef struct common_auth_init_st {
@@ -79,6 +78,10 @@ typedef struct common_acct_info_st {
 	char ipv4[MAX_IP_STR];
 	char ipv6[MAX_IP_STR];
 	unsigned int id;
+	/* Wall-clock lifetime of the logical session in seconds (now -
+	 * client-entry creation time), computed by sec-mod when an accounting
+	 * record is emitted. For the Stop it is snapshotted at disconnect. */
+	time_t uptime;
 } common_acct_info_st;
 
 #define IS_CLIENT_ENTRY_EXPIRED_FULL(sec, e, now, clean) \

src/worker-vpn.c

@@ -548,7 +548,6 @@ static void send_stats_to_secmod(worker_st *ws, time_t now,
 
 		msg.bytes_in = ws->tun_bytes_in;
 		msg.bytes_out = ws->tun_bytes_out;
-		msg.uptime = now - ws->session_start_time;
 		msg.sid.len = sizeof(ws->sid);
 		msg.sid.data = ws->sid;
 		msg.has_sid = 1;