sec-mod: prevent malicious clients from piling up client_entry_st per pid

Malicious clients cannot send CMD_SEC_AUTH_INIT arbitrarily: a worker's
pid has one client_entry_st attached to it. A repeated SEC_AUTH_INIT
is accepted (replacing the previous entry) only when the prior
attempt already ended in PS_AUTH_FAILED with no session attached
(in_use == 0), matching the legitimate case for it (GSSAPI/certificate
falling back to the next auth method). PS_AUTH_FAILED alone does not
imply this: handle_sec_auth_ban_ip_reply() can mark an entry with an
open session PS_AUTH_FAILED on a non-OK ban reply, and deleting that
entry out from under an open session would desync it from the state
main still holds.

Any other repeat - while a previous attempt on that pid is still mid-flight,
already completed, or failed but still attached to an open session - is
refused.

Resolves: #249

Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>

doc/requirements/internal/sec-mod.md

@@ -186,8 +186,12 @@ the human-facing `occtl terminate` command only).
 **Acceptance:** negative, local — send `SEC_AUTH_CONT` with a `sid` that is
 a correct prefix of a valid SID but padded/truncated to `SID_SIZE`;
 confirm `find_client_entry` returns NULL (no match) — i.e., this is a
-full-value comparison, covered structurally by REQ-IPC-015.
-**Links:** REQ-IPC-015, REQ-SECMOD-SESSION-005
+full-value comparison, covered structurally by REQ-IPC-015. This requirement
+governs `find_client_entry()` specifically; it does not preclude the
+pid-scoped pre-creation lookup (`find_client_entry_by_pid()`) added by
+REQ-SECMOD-SESSION-007, which is a distinct, additional lookup used only to
+decide whether `handle_sec_auth_init` may create a new entry.
+**Links:** REQ-IPC-015, REQ-SECMOD-SESSION-005, REQ-SECMOD-SESSION-007
 
 ### REQ-SECMOD-SESSION-002 — Expiry requires in_use == 0
 
@@ -281,6 +285,137 @@ confirm the expired entry is absent, and the `in_use>0` entry has
 `expires=0` in the reply (`occtl show sessions valid` reflects this).
 **Links:** REQ-SECMOD-SESSION-002, REQ-IPC-070
 
+### REQ-SECMOD-SESSION-007 — At most one pid-stamped client_entry_st per worker pid; only a prior PS_AUTH_FAILED, unattached entry may be replaced, anything else is scored and rejected
+
+**Requirement:** `handle_sec_auth_init()` MUST look up an existing
+`client_entry_st` for the requesting worker's `pid` (`find_client_entry_by_pid()`)
+before calling `new_client_entry()`, and MUST NOT allow more than one
+`client_entry_st` to exist for the same pid at a time. This invariant covers
+only entries currently carrying a non-zero `acct_info.id` (see the third
+bullet below for when that is and is not the case):
+  - If the existing entry's `status == PS_AUTH_FAILED` AND `in_use == 0`,
+    sec-mod MUST delete it (`del_client_entry`) before creating the new one.
+    `in_use == 0` MUST be checked because `PS_AUTH_FAILED` alone does not
+    imply no session is open: `handle_sec_auth_ban_ip_reply()` sets
+    `status = PS_AUTH_FAILED` on any SID-matched entry on a non-OK ban
+    reply (`sec-mod-auth.c:709-712`) without touching `in_use`, so an
+    open session (`in_use > 0`, reachable only via the replay described in
+    the second bullet, since a conforming worker never re-sends
+    `SEC_AUTH_INIT` once a session is open) could otherwise be deleted out
+    from under main, which still holds its SID and TUN/IP state — the next
+    `SECM_SESSION_CLOSE` for that SID would then hit the "non-existing SID"
+    path and lose the final accounting Stop. If `in_use > 0`, this case
+    MUST instead fall through to the reject-and-score handling below, the
+    same as any other status.
+
+    **This `PS_AUTH_FAILED` replace branch exists for exactly one reason:
+    GSSAPI ticket-verification failure falling back to the next configured
+    auth method.** `ws_switch_auth_to_next()` (`worker-auth.c:1930-1938`)
+    is the only place a worker legitimately resets `ws->auth_state =
+    S_AUTH_INACTIVE` — and therefore re-sends `SEC_AUTH_INIT` on the same
+    pid — *after* a real sec-mod round trip has already created an entry;
+    the worker-side contract for this reset (why it is bounded to these two
+    call sites and cannot loop) is REQ-WORKER-AUTH-007. It is reached only
+    when a GSSAPI attempt has
+    already gone through `handle_sec_auth_res()`'s failure path
+    (`sec-mod-auth.c:435-441`, setting `PS_AUTH_FAILED` without deleting the
+    entry) and the worker falls back to the next method. This is *not* an
+    illustrative example among several — no other configured auth type
+    (certificate, plain, RADIUS, PAM, OIDC) produces this pattern: a missing
+    client certificate is rejected before `SEC_AUTH_INIT` is ever sent
+    (`worker-auth.c:1741-1751`, pre-secmod), and a failed plain/RADIUS/PAM
+    password attempt terminates the connection outright
+    (`goto auth_fail`) rather than resetting to `S_AUTH_INACTIVE`. If the
+    GSSAPI fallback mechanism is ever removed or replaced with something
+    that does not need to resurrect a failed pid's entry, this branch (and
+    the `PS_AUTH_FAILED` special-casing throughout this requirement) has no
+    remaining justification and MUST be reconsidered for removal — at that
+    point every `SEC_AUTH_INIT` for a pid that already has an entry could
+    revert to the simpler always-reject-and-score rule below, and
+    `find_client_entry_by_pid()`'s only remaining caller would need
+    re-evaluating too.
+  - For any other status (`PS_AUTH_INIT`, `PS_AUTH_CONT`, `PS_AUTH_COMPLETED`),
+    or `PS_AUTH_FAILED` with `in_use > 0`, sec-mod MUST NOT create a new
+    entry, MUST NOT modify or delete the existing one, MUST NOT send any
+    reply, and MUST call `sec_mod_add_score_to_ip()` with
+    `score = ban_points_wrong_password` against the existing entry's
+    vhost/IP (same mechanism as REQ-SECMOD-SEC-003) — a conforming worker
+    never re-sends `SEC_AUTH_INIT` while a previous attempt on the same pid
+    is still mid-flight (`PS_AUTH_INIT`/`PS_AUTH_CONT`), already completed
+    (`PS_AUTH_COMPLETED`), or failed but still attached to an open session
+    (`PS_AUTH_FAILED` with `in_use > 0`), so this is treated as a protocol
+    violation (e.g. a compromised worker replaying `SEC_AUTH_INIT` with its
+    still-valid original HMAC/`session_start_time`) and scored like a
+    qualifying auth failure rather than silently retried for free.
+  - `acct_info.id` is the pid correlator `find_client_entry_by_pid()`
+    matches against. It is stamped in three places: `new_client_entry()` at
+    entry creation, `handle_sec_auth_stats_cmd()` on every `CMD_SEC_CLI_STATS`
+    from a worker presenting a valid SID (`sec-mod-auth.c:754`), and cleared
+    (`= 0`) by `expire_client_entry()` whenever an entry is kept (not
+    immediately deleted) after `e->in_use` reaches 0. A zero `acct_info.id`
+    therefore means "no worker has reported against this entry since it was
+    last detached" — it prevents a later, unrelated worker that inherits
+    the same OS pid from spuriously matching a lingering/disconnected
+    entry. It is NOT a guarantee that a non-zero-id match is the entry's
+    originally-authenticating worker: any worker later presenting that
+    entry's valid SID over `CMD_SEC_CLI_STATS` re-stamps the id to its own
+    pid. One consequence: a `client_entry_st` resumed via
+    `SECM_SESSION_OPEN` (persistent-cookie reconnect, a different pid) keeps
+    `acct_info.id == 0` — `handle_secm_session_open()` bumps `in_use` but
+    does not stamp the pid — until that worker's first `CMD_SEC_CLI_STATS`;
+    during that window `find_client_entry_by_pid()` does not see it, so the
+    new worker's pid is not yet protected by this requirement's one-entry
+    rule (bounded impact: at most one extra `client_entry_st` for that pid
+    until the first stats report).
+**Strength:** MUST
+**Status:** DERIVED
+**Source:** src/sec-mod-auth.c:874-926 (`handle_sec_auth_init`);
+src/sec-mod-auth.c:366-451 (`handle_sec_auth_res`, `PS_AUTH_FAILED`
+transition); src/sec-mod-auth.c:694-713 (`handle_sec_auth_ban_ip_reply`,
+the other `PS_AUTH_FAILED` transition); src/sec-mod-auth.c:715-778
+(`handle_sec_auth_stats_cmd`, the `acct_info.id` re-stamp at line 754);
+src/sec-mod-db.c:178-247 (`find_client_entry_by_pid`, `expire_client_entry`)
+**Acceptance:** positive, `tests/test-gssapi-opt-pass` — a GSSAPI
+ticket-verification failure (an NTLMSSP Type1 token replayed as its own
+continuation, which `gss_accept_sec_context()` rejects) followed by a
+password fallback on the same worker connection (driven over one persistent
+`http.client.HTTPSConnection`, not curl - curl's connection reuse across
+separate request legs is not portable across the curl versions in this
+project's CI matrix) obtains a cookie
+(`<auth id="success">`), and sec-mod's debug log shows exactly one
+mid-test `sec_auth_user_deinit()` "permanently closing session" line (the
+`PS_AUTH_FAILED` entry being replaced) — not zero (which would mean the
+entry was never reclaimed until final shutdown, i.e. leaked). occtl's
+"Sec-mod client entries" counter would be the more direct way to assert
+this but requires a real uid 0 peer (`check_upeer_id()`), which the
+`NO_NEED_ROOT`/`uid_wrapper` emulation this test relies on for the server
+does not extend to occtl as a separate client process. Negative — (a) send a second
+`CMD_SEC_AUTH_INIT` for a pid whose existing entry is still `PS_AUTH_INIT`
+(don't complete the `SEC_AUTH_CONT` round); confirm no new entry is created,
+no reply is sent, and `CMD_SECM_BAN_IP` is sent with
+`score = ban_points_wrong_password`; confirm enough repeats
+(`max_ban_score / ban_points_wrong_password`) get the source IP refused on
+its next connection. (b) unit, local, `tests/sec-mod-db` — set `e->in_use = 1`
+on a kept (non-deleted, `discon_reason` unset) entry, call
+`expire_client_entry` (decrementing `in_use` to 0), confirm `acct_info.id
+== 0` afterward and that `find_client_entry_by_pid()` no longer matches it
+for that numeric pid. (c) unit, local, `tests/sec-mod-db` — populate a
+`client_entry_st` with `status = PS_AUTH_FAILED` and `in_use = 1`
+(simulating `handle_sec_auth_ban_ip_reply()` marking a live session
+`PS_AUTH_FAILED`); confirm `find_client_entry_by_pid()` still matches it —
+`find_client_entry_by_pid()` does not itself consult `status` or `in_use`,
+which is precisely why `handle_sec_auth_init()` (the only caller that acts
+on the match) MUST check `in_use == 0` itself rather than relying on the
+lookup to exclude attached sessions. `[GAP: the `in_use == 0` condition in
+handle_sec_auth_init() itself — i.e. that a PS_AUTH_FAILED entry with
+in_use > 0 is scored-and-rejected rather than deleted — has no dedicated
+test yet; exercising it requires either a full-stack CI test driving a
+real SECM_BAN_IP round-trip against an open session, or a unit test
+providing HMAC/vhost/auth-module fixtures for handle_sec_auth_init()
+directly, neither of which exists today.]`
+**Links:** REQ-SECMOD-SESSION-001, REQ-SECMOD-SESSION-002,
+REQ-SECMOD-SEC-003, REQ-WORKER-AUTH-007
+
 ## TEARDOWN
 
 ### REQ-SECMOD-TEARDOWN-001 — db deinit calls auth_deinit for every remaining entry

doc/requirements/internal/worker.md

@@ -241,6 +241,65 @@ with no LeakSanitizer/ASAN report against `recv_cookie_auth_reply` or
 `ws->user_config` is sufficient acceptance; no new test required.
 **Links:** —
 
+### REQ-WORKER-AUTH-007 — `ws_switch_auth_to_next()` gates every worker-initiated reset to `S_AUTH_INACTIVE`; each fallback strictly advances or fails closed
+
+**Requirement:** `post_auth_handler()` MUST NOT reset `ws->auth_state` to
+`S_AUTH_INACTIVE` (and thereby re-send `CMD_SEC_AUTH_INIT` on the current
+connection) except immediately after a successful call to
+`ws_switch_auth_to_next(ws)` (return value non-zero). There are exactly two
+such call sites, and no other code path may perform this reset:
+  - **Missing certificate** (`src/worker-auth.c:1744-1755`): while still in
+    the `ws->auth_state == S_AUTH_INACTIVE` branch, if `AUTH_TYPE_CERTIFICATE`
+    is selected and `ws->cert_auth_ok == 0`, the fallback fires *before* this
+    attempt's `CMD_SEC_AUTH_INIT` is ever built or sent
+    (`send_msg_to_secmod(..., CMD_SEC_AUTH_INIT, ...)` is at
+    `src/worker-auth.c:1823-1834`, after this check) — sec-mod has no
+    `client_entry_st` for this attempt at all.
+  - **GSSAPI ticket verification failure** (`src/worker-auth.c:1932-1941`):
+    after a `CMD_SEC_AUTH_INIT`/`CMD_SEC_AUTH_CONT` round-trip to sec-mod for
+    `AUTH_TYPE_GSSAPI` returns `ret < 0`, the fallback fires *after* sec-mod
+    has already created a `client_entry_st` for this pid and set its status
+    to `PS_AUTH_FAILED` (`handle_sec_auth_res()`,
+    `src/sec-mod-auth.c:435-441`). The immediately following
+    `CMD_SEC_AUTH_INIT` for the next method is therefore a second
+    `SEC_AUTH_INIT` on the same worker pid, which is the specific case
+    REQ-SECMOD-SESSION-007 requires sec-mod to special-case (replace the
+    `PS_AUTH_FAILED` entry rather than reject the request).
+
+  `ws_switch_auth_to_next()` (`src/worker-auth.c:140-158`) sets
+  `ws->selected_auth->enabled = 0` on the failed method before searching for
+  the next enabled one — `ws->selected_auth` points into
+  `WSSCONFIG(ws)` (`&ws->vhost->static_config`), the worker's own
+  process-private copy of the vhost's auth-method list (received at worker
+  startup; not shared with other workers or with main/sec-mod), so a method
+  once disabled by a fallback is never reselected for the remainder of this
+  connection, and disabling it has no effect on any other client's
+  connection. If no other enabled method remains, `ws_switch_auth_to_next()`
+  returns 0 and both call sites `goto auth_fail` instead of resetting to
+  `S_AUTH_INACTIVE` — a connection can therefore never loop indefinitely
+  through fallbacks, and (as a consequence relevant to
+  REQ-SECMOD-SESSION-007) a single worker pid can cause sec-mod to replace a
+  `PS_AUTH_FAILED` entry at most once per distinct GSSAPI-then-something-else
+  fallback on that connection, not repeatedly.
+**Strength:** MUST
+**Status:** DERIVED
+**Source:** src/worker-auth.c:140-158 (`ws_switch_auth_to_next`);
+src/worker-auth.c:1744-1755, 1932-1941 (the two call sites);
+src/sec-mod-auth.c:435-441 (`handle_sec_auth_res`, `PS_AUTH_FAILED`
+transition); src/worker.h:223-224 (`WSRCONFIG`/`WSSCONFIG`)
+**Acceptance:** positive, `tests/test-cert-opt-pass` — connecting without a
+client certificate to a vhost configured with `auth = "certificate"` falling
+back to a password method completes authentication via the pre-secmod
+fallback branch. positive, `tests/test-gssapi-opt-pass` — a failed GSSAPI
+attempt followed by a password fallback on the same connection completes
+authentication via the post-secmod fallback branch (this is also
+REQ-SECMOD-SESSION-007's acceptance test, from the sec-mod side of the same
+exchange). [SEC] negative — configure a vhost with only `auth = "gssapi"`
+(no fallback method enabled); confirm a failed GSSAPI attempt reaches
+`auth_fail` (connection terminated) rather than resetting to
+`S_AUTH_INACTIVE` and re-prompting.
+**Links:** REQ-SECMOD-SESSION-007, REQ-AUTH-AUTH-031, REQ-AUTH-AUTH-032
+
 ---
 
 ## SEC

src/sec-mod-auth.c

@@ -876,6 +876,7 @@ int handle_sec_auth_init(int cfd, sec_mod_st *sec, const SecAuthInitMsg *req,
 {
 	int ret = -1;
 	client_entry_st *e;
+	client_entry_st *old;
 	unsigned int i;
 	unsigned int need_continue = 0;
 	vhost_cfg_st *vhost;
@@ -922,6 +923,37 @@ int handle_sec_auth_init(int cfd, sec_mod_st *sec, const SecAuthInitMsg *req,
 		return -1;
 	}
 
+	/* A conforming worker only re-sends SEC_AUTH_INIT on the same pid
+	 * after a terminal failure of a previous attempt (PS_AUTH_FAILED,
+	 * e.g. GSSAPI/certificate falling back to the next auth method). Any
+	 * other match means a client_entry_st is already attached to this
+	 * pid and mid-flight or already authenticated - not something a
+	 * conforming worker does, so treat it as a protocol violation rather
+	 * than silently allowing a second entry to accumulate.
+	 *
+	 * in_use == 0 is required in addition to PS_AUTH_FAILED because
+	 * status alone does not imply no session is open:
+	 * handle_sec_auth_ban_ip_reply() can set PS_AUTH_FAILED on an entry
+	 * with an open session (in_use > 0) on a non-OK ban reply, and
+	 * deleting that entry would desync it from the session main still
+	 * holds.
+	 */
+	old = find_client_entry_by_pid(sec, pid);
+	if (old != NULL) {
+		if (old->status == PS_AUTH_FAILED && old->in_use == 0) {
+			del_client_entry(sec, old);
+		} else {
+			seclog(sec, LOG_NOTICE,
+			       "worker pid %u sent a new auth init while a previous one (status %s) is still attached to it - dropping connection",
+			       (unsigned int)pid,
+			       ps_status_to_str(old->status, 0));
+			sec_mod_add_score_to_ip(
+				sec, old, old->acct_info.remote_ip,
+				old->vhost->config->ban_points_wrong_password);
+			return -1;
+		}
+	}
+
 	e = new_client_entry(sec, vhost, req->remote_ip, pid);
 	if (e == NULL) {
 		seclog(sec, LOG_ERR, "cannot initialize memory");

src/sec-mod-db.c

@@ -175,6 +175,34 @@ client_entry_st *find_client_entry(sec_mod_st *sec, uint8_t sid[SID_SIZE])
 	return htable_get(db, rehash(&t, NULL), client_entry_cmp, &t);
 }
 
+/* Finds a client_entry_st whose acct_info.id (pid correlator) matches
+ * this pid, if any. acct_info.id is 0 unless a worker has stamped it
+ * (at creation, or on a later CMD_SEC_CLI_STATS - see
+ * handle_sec_auth_stats_cmd()) and is cleared by expire_client_entry()
+ * once no worker is attached, so a match here rules out a stale/reused
+ * pid, but does not guarantee the match is the entry's originally
+ * authenticating worker: any worker later presenting the entry's SID
+ * over CMD_SEC_CLI_STATS re-stamps this id to its own pid.
+ */
+client_entry_st *find_client_entry_by_pid(sec_mod_st *sec, unsigned int pid)
+{
+	struct htable *db = sec->client_db;
+	client_entry_st *t;
+	struct htable_iter iter;
+
+	if (pid == 0)
+		return NULL;
+
+	t = htable_first(db, &iter);
+	while (t != NULL) {
+		if (t->acct_info.id == pid)
+			return t;
+		t = htable_next(db, &iter);
+	}
+
+	return NULL;
+}
+
 static void clean_entry(sec_mod_st *sec, client_entry_st *e)
 {
 	sec_auth_user_deinit(sec, e);
@@ -242,6 +270,11 @@ void expire_client_entry(sec_mod_st *sec, client_entry_st *e)
 			seclog(sec, LOG_INFO,
 			       "temporarily closing session for %s " SESSION_STR,
 			       e->acct_info.username, e->acct_info.safe_id);
+
+			/* No worker is attached to this entry anymore; invalidate
+			 * the pid so a later, unrelated worker that inherits the
+			 * same OS pid never matches this lingering entry. */
+			e->acct_info.id = 0;
 		}
 	}
 }

src/sec-mod.h

@@ -147,6 +147,7 @@ unsigned int sec_mod_client_db_elems(sec_mod_st *sec);
 client_entry_st *new_client_entry(sec_mod_st *sec, struct vhost_cfg_st *,
 				  const char *ip, unsigned int pid);
 client_entry_st *find_client_entry(sec_mod_st *sec, uint8_t sid[SID_SIZE]);
+client_entry_st *find_client_entry_by_pid(sec_mod_st *sec, unsigned int pid);
 void del_client_entry(sec_mod_st *sec, client_entry_st *e);
 void expire_client_entry(sec_mod_st *sec, client_entry_st *e);
 void cleanup_client_entries(sec_mod_st *sec);

tests/meson.build

@@ -39,6 +39,7 @@ unit_tests = {
   'kkdcp-parsing':  {'src': ['kkdcp-parsing.c'], 'args': [], 'timeout': 30},
   'json-escape':    {'src': ['json-escape.c'],   'args': [], 'timeout': 30},
   'ban-ips':        {'src': ['ban-ips.c'],        'args': ['-DUNDER_TEST'], 'timeout': 120},
+  'sec-mod-db':     {'src': ['sec-mod-db.c'],     'args': ['-DUNDER_TEST'], 'timeout': 30},
   'port-parsing':   {'src': ['port-parsing.c'],  'args': ['-DUNDER_TEST'], 'timeout': 30},
   'human_addr':     {'src': ['human_addr.c'],    'args': ['-DUNDER_TEST'], 'timeout': 30},
   'valid-hostname': {'src': ['valid-hostname.c'], 'args': [], 'timeout': 30},

tests/sec-mod-db.c

@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2026 Nikos Mavrogiannopoulos
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Unit test for sec-mod's client_entry_st <-> worker-pid correlator
+ * (client_entry_st.acct_info.id, matched by find_client_entry_by_pid()).
+ *
+ * handle_sec_auth_init() uses this correlator to stop a single worker pid
+ * from accumulating more than one client_entry_st via repeated
+ * CMD_SEC_AUTH_INIT messages (REQ-SECMOD-SESSION-007, resolving
+ * https://gitlab.com/openconnect/ocserv/-/issues/249). This test does not
+ * exercise handle_sec_auth_init() itself (that requires HMAC/vhost/
+ * auth-module fixtures this test does not build - see the sec-mod.md
+ * acceptance criteria for what still needs a full-stack test); it covers
+ * only the two sec-mod-db.c primitives that decision is built on:
+ *
+ *  - expire_client_entry() MUST invalidate the correlator (set
+ *    acct_info.id = 0) once a worker detaches, otherwise a later,
+ *    unrelated worker that the OS happens to reuse the same pid for
+ *    would spuriously match a stale entry.
+ *  - find_client_entry_by_pid() itself does NOT gate on the entry's
+ *    status or in_use - it is a pure pid lookup. Callers that delete on
+ *    a match (like handle_sec_auth_init()'s PS_AUTH_FAILED-replace
+ *    branch) are therefore responsible for their own in_use == 0 check
+ *    before deleting; the lookup will not do it for them.
+ */
+
+#include <config.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <talloc.h>
+
+#include "../src/sec-mod.h"
+#include "../src/vhost.h"
+#include "../src/sec-mod-db.c"
+
+/* sec_auth_user_deinit() is defined in sec-mod-auth.c, which this test
+ * does not link (it pulls in the full auth-module/IPC machinery); provide
+ * a no-op stand-in so clean_entry()/del_client_entry() remain callable.
+ */
+void sec_auth_user_deinit(sec_mod_st *sec, client_entry_st *e)
+{
+	(void)sec;
+	(void)e;
+}
+
+static sec_mod_st *make_sec(void *pool)
+{
+	sec_mod_st *sec = talloc_zero(pool, sec_mod_st);
+
+	if (sec == NULL)
+		exit(1);
+
+	if (sec_mod_client_db_init(sec) == NULL)
+		exit(1);
+
+	return sec;
+}
+
+static vhost_cfg_st *make_vhost(void *pool)
+{
+	vhost_cfg_st *vhost = talloc_zero(pool, vhost_cfg_st);
+
+	if (vhost == NULL)
+		exit(1);
+
+	vhost->config = talloc_zero(vhost, ReloadableConfig);
+	if (vhost->config == NULL)
+		exit(1);
+
+	vhost->config->cookie_timeout = 300;
+
+	return vhost;
+}
+
+/* Detach case: acct_info.id is cleared, so the detached pid no longer
+ * matches (REQ-SECMOD-SESSION-007, acceptance (b)).
+ */
+static void test_expire_clears_pid_correlator(void *pool)
+{
+	sec_mod_st *sec = make_sec(pool);
+	vhost_cfg_st *vhost = make_vhost(sec);
+	client_entry_st *e;
+	const unsigned int pid = 4242;
+
+	e = new_client_entry(sec, vhost, "192.0.2.1", pid);
+	if (e == NULL) {
+		fprintf(stderr, "new_client_entry failed\n");
+		exit(1);
+	}
+
+	if (find_client_entry_by_pid(sec, pid) != e) {
+		fprintf(stderr,
+			"find_client_entry_by_pid did not find the freshly "
+			"created entry for pid %u\n",
+			pid);
+		exit(1);
+	}
+
+	/* Simulate a worker detaching: in_use goes from 1 to 0 via
+	 * expire_client_entry(). discon_reason is left at its zero default
+	 * (not REASON_SERVER_DISCONNECT/REASON_SESSION_TIMEOUT), so the
+	 * entry is kept rather than deleted - the case acct_info.id
+	 * clearing exists to protect. */
+	e->in_use = 1;
+	expire_client_entry(sec, e);
+
+	if (e->acct_info.id != 0) {
+		fprintf(stderr,
+			"expected acct_info.id == 0 after expire_client_entry "
+			"on a kept entry, got %u\n",
+			e->acct_info.id);
+		exit(1);
+	}
+
+	if (find_client_entry_by_pid(sec, pid) != NULL) {
+		fprintf(stderr,
+			"find_client_entry_by_pid still matches pid %u after "
+			"its worker detached\n",
+			pid);
+		exit(1);
+	}
+
+	sec_mod_client_db_deinit(sec);
+}
+
+/* Still-attached case: a PS_AUTH_FAILED entry with in_use > 0 (e.g.
+ * handle_sec_auth_ban_ip_reply() marking a live session PS_AUTH_FAILED on
+ * a non-OK ban reply) is still returned by the lookup - it is not
+ * filtered out (REQ-SECMOD-SESSION-007, acceptance (c)).
+ */
+static void test_lookup_does_not_gate_on_status_or_in_use(void *pool)
+{
+	sec_mod_st *sec = make_sec(pool);
+	vhost_cfg_st *vhost = make_vhost(sec);
+	client_entry_st *e;
+	const unsigned int pid = 4343;
+
+	e = new_client_entry(sec, vhost, "192.0.2.2", pid);
+	if (e == NULL) {
+		fprintf(stderr, "new_client_entry failed\n");
+		exit(1);
+	}
+
+	e->status = PS_AUTH_FAILED;
+	e->in_use = 1;
+
+	if (find_client_entry_by_pid(sec, pid) != e) {
+		fprintf(stderr,
+			"find_client_entry_by_pid did not match a "
+			"PS_AUTH_FAILED, in_use>0 entry for pid %u\n",
+			pid);
+		exit(1);
+	}
+
+	sec_mod_client_db_deinit(sec);
+}
+
+int main(void)
+{
+	void *pool = talloc_new(NULL);
+
+	if (pool == NULL)
+		exit(1);
+
+	test_expire_clears_pid_correlator(pool);
+	test_lookup_does_not_gate_on_status_or_in_use(pool);
+
+	talloc_free(pool);
+
+	return 0;
+}

tests/test-gssapi-opt-pass

@@ -22,6 +22,30 @@ srcdir=${srcdir:-.}
 builddir=${builddir:-.}
 NO_NEED_ROOT=1
 OUTFILE=test-gssapi-opt-pass.$$.tmp
+SERVER_LOG=ocserv-gssapi-opt-pass.$$.log
+
+# Regression test for https://gitlab.com/openconnect/ocserv/-/work_items/249:
+# a worker (pid) that falls back from a failed GSSAPI attempt to a
+# successful password attempt on the same connection must leave sec-mod
+# with exactly one client_entry_st for that attempt, not two (one orphaned
+# PS_AUTH_FAILED entry plus one PS_AUTH_COMPLETED entry).
+#
+# occtl's control socket would be the natural way to check this, but it
+# authorizes the peer via SO_PEERCRED (check_upeer_id() in
+# src/common/system.c), which requires a real uid 0 - the uid_wrapper root
+# emulation this NO_NEED_ROOT test relies on for the server does not fake
+# that for occtl as a separate client process, and no other NO_NEED_ROOT
+# test in this suite uses occtl for that reason. Instead this checks
+# sec-mod's own debug log (already captured via VERBOSE=1) for
+# sec_auth_user_deinit()'s "permanently closing session" line, which fires
+# exactly once - mid-test, when the second (password) auth init replaces
+# the failed GSSAPI entry - if the fix is working. Without it, the failed
+# GSSAPI entry is only ever cleaned up together with everything else at
+# final server shutdown, never mid-test.
+mid_test_session_closes()
+{
+	grep -c "permanently closing session" ${SERVER_LOG}
+}
 
 connect()
 {
@@ -74,7 +98,13 @@ echo "Testing local backend with gssapi and password fallback... "
 VERBOSE=1
 
 update_config test-gssapi-opt-pass.config
-launch_sr_server -d 1 -f -c ${CONFIG} & PID=$!
+# Not using launch_sr_server() here (it discards output unless VERBOSE is
+# read by *it* before backgrounding) so the server's debug log can be
+# captured to a file this script can grep mid-test - see
+# mid_test_session_closes() above.
+LD_PRELOAD=libsocket_wrapper.so:libuid_wrapper.so UID_WRAPPER=1 UID_WRAPPER_ROOT=1 \
+	$SERV -d 1 -f -c ${CONFIG} -d 3 >${SERVER_LOG} 2>&1 &
+PID=$!
 wait_server $PID
 
 echo -n "Connecting to obtain cookie (user with non-gssapi password)... "
@@ -103,9 +133,116 @@ echo "Connecting with curl/negotiate... "
 LD_PRELOAD=libsocket_wrapper.so curl https://testinvalid:testpass@$ADDRESS:$PORT ${CURLOPTS} --negotiate -f -v ||
 	fail $PID "Could not connect to server"
 
+echo -n "Checking that a failed GSSAPI attempt followed by a successful "
+echo "password fallback on the same connection leaves a single sec-mod entry... "
+
+BEFORE_CLOSES=$(mid_test_session_closes)
+
+if ! command -v python3 >/dev/null 2>&1;then
+	echo "python3 not found"
+	exit 77
+fi
+
+# curl's --next does reliably reuse a connection for a follow-up request
+# *within* one 401-triggered auth retry (that's how the "curl/negotiate"
+# check above and the GSSAPI leg below work at all), but reusing that same
+# connection for a *separate* subsequent leg is a much newer, inconsistently
+# supported curl optimization - it silently opens a fresh connection (and
+# thus a fresh worker pid) on older curl (e.g. curl 7.61 on CentOS 8),
+# which defeats the entire point of this test (same pid) without any
+# visible error. http.client.HTTPSConnection keeps one connection under
+# our own control across multiple requests, deterministically, regardless
+# of curl version.
+#
+# Three requests over one connection (hence one worker pid):
+#  1. GET / with a GSSAPI/NTLM Type1 (negotiate-init) token and
+#     X-Support-HTTP-Auth set (so the server doesn't immediately give up
+#     on GSSAPI the way the plain "curl/negotiate" check above does - see
+#     http_header_complete_cb()). This drives a real CMD_SEC_AUTH_INIT for
+#     GSSAPI; a Type1 token alone is never sufficient to complete NTLM, so
+#     the module replies ERR_AUTH_CONTINUE (401 + WWW-Authenticate:
+#     Negotiate <challenge>).
+#  2. GET / again, replaying the same Type1 blob as the "continuation" -
+#     gss_accept_sec_context() rejects it (it's not a valid response to
+#     its own challenge), which is a real, guaranteed authentication
+#     failure without needing genuine NTLM credential material. This
+#     drives CMD_SEC_AUTH_CONT, which fails and falls back to the next
+#     configured method (plain), leaving a PS_AUTH_FAILED entry for this
+#     pid.
+#  3. POST /auth with a valid username/password -> a second
+#     CMD_SEC_AUTH_INIT for the same pid, which must replace the
+#     PS_AUTH_FAILED entry rather than add a second one.
+LD_PRELOAD=libsocket_wrapper.so python3 - "$ADDRESS" "$PORT" >${OUTFILE} <<'PYEOF'
+import http.client
+import ssl
+import sys
+
+address, port = sys.argv[1], int(sys.argv[2])
+
+# A captured, generic GSSAPI/SPNEGO NTLMSSP Type1 (negotiate) token. Type1
+# carries only capability flags, no credentials, so it is not tied to any
+# particular username/password and is safe to reuse as a static fixture.
+TYPE1 = (
+	"YEgGBisGAQUFAqA+MDygDjAMBgorBgEEAYI3AgIKoioEKE5UTE1TU1AAAQAAABeCCOIA"
+	"AAAAAAAAAAAAAAAAAAAABgIAAAAAAA8="
+)
+
+ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
+ctx.check_hostname = False
+ctx.verify_mode = ssl.CERT_NONE
+
+conn = http.client.HTTPSConnection(address, port, context=ctx, timeout=15)
+
+conn.request("GET", "/", headers={
+	"Authorization": "Negotiate " + TYPE1,
+	"X-Support-HTTP-Auth": "1",
+})
+conn.getresponse().read()
+
+conn.request("GET", "/", headers={
+	"Authorization": "Negotiate " + TYPE1,
+	"X-Support-HTTP-Auth": "1",
+})
+conn.getresponse().read()
+
+conn.request("POST", "/auth", body="username=test&password=test", headers={
+	"Content-Type": "application/x-www-form-urlencoded",
+})
+sys.stdout.buffer.write(conn.getresponse().read())
+PYEOF
+if test $? != 0;then
+	cat ${OUTFILE}
+	cat ${SERVER_LOG}
+	fail $PID "Could not complete GSSAPI-then-password fallback on one connection"
+fi
+
+# give sec-mod's debug log time to flush the "permanently closing session"
+# line before we grep for it.
+sleep 1
+
+grep '<auth id="success">' ${OUTFILE} >/dev/null
+if test $? != 0;then
+	cat ${OUTFILE}
+	cat ${SERVER_LOG}
+	fail $PID "GSSAPI-then-password fallback on one connection did not obtain a cookie"
+fi
+
+AFTER_CLOSES=$(mid_test_session_closes)
+DELTA=$((AFTER_CLOSES - BEFORE_CLOSES))
+
+cat ${SERVER_LOG}
+
+if test "${DELTA}" != 1;then
+	echo "FAIL: expected exactly one mid-test session close (replacing the failed GSSAPI attempt), got ${DELTA} (before: ${BEFORE_CLOSES}, after: ${AFTER_CLOSES})"
+	fail $PID "GSSAPI-fail-then-password-fallback leaked a client_entry_st (work_item #249)"
+fi
+echo ok
+
 kill $PID
 wait
 
+rm -f ${SERVER_LOG}
+
 rm -f ${builddir}/ntlm.$$.pass.tmp
 
 exit 0