Merge branch 'maint'

testing/acl-allowlist/test.sh

@@ -116,27 +116,62 @@ wait_for_peers_exact() {
     exit 1
 }
 
-assert_log_contains() {
+# Assert that ONE log line in $container contains every one of the given
+# fixed strings.
+#
+# Single-line matching is the point. Independent whole-log greps for an
+# npub and for `decision=denylist match` are jointly satisfied by "this
+# npub appears somewhere" plus "somebody was rejected by denylist", which
+# is not the property this suite exists to prove. Node-a lists the denied
+# peers as auto_connect peers, so it logs their npubs on the outbound
+# connect path whether or not a rejection ever happened; the npub has to
+# be on the rejection line itself to mean anything.
+#
+# Strings match in any order, by chaining fixed-string greps over the
+# surviving lines, so the assertion does not depend on the order the
+# tracing formatter emits a message and its fields in. A grep over empty
+# input yields the empty string rather than a value that could satisfy
+# the caller, so a container that cannot be read times out and fails
+# rather than passing.
+#
+# Polls rather than reading once: under the XX handshake the
+# cross-connection tie-breaker decides which side reaches its ACL check
+# first, so an inbound-handshake rejection may not emit until a later
+# retry. Same wait-with-timeout shape as wait_for_peers_exact above.
+#
+# Deliberately NOT registered in check-log-strings.py's SHELL_HELPERS,
+# for two reasons, and note that registering it would in fact capture
+# nothing: that extractor reads only a helper's FIRST argument and only
+# when it is a quoted literal free of `$` (check-log-strings.py:116),
+# whereas the first argument here is the unquoted container name
+# carrying ${FIPS_CI_NAME_SUFFIX}. Verified by running the extractor
+# with this helper added: zero hits. The same is true of the
+# `assert_log_contains` this replaced, so nothing left the check's scope.
+#
+# It should stay out of scope regardless: that check exists for strings
+# whose disappearance from src/ would let an assertion silently pass,
+# and every string here is a positive requirement, so a missing one
+# exhausts the poll and exits 1, which is loud.
+assert_log_line_contains_all() {
     local container="$1"
-    local pattern="$2"
-    local timeout="${3:-15}"
-    local logs
+    local timeout="$2"
+    shift 2
 
-    # Poll docker logs instead of one-shot reading: under XX handshake,
-    # the cross-connection tie-breaker determines which side reaches
-    # its ACL-check point first, so the inbound-handshake-context
-    # rejection may not emit until a later retry. Same wait-with-timeout
-    # shape as wait_for_peers_exact above.
+    local logs surviving pattern
     for _ in $(seq 1 "$timeout"); do
         logs="$(docker logs "$container" 2>&1 | python3 -c 'import re,sys; print(re.sub(r"\x1b\[[0-9;]*m", "", sys.stdin.read()), end="")' || true)"
-        if printf '%s' "$logs" | grep -F "$pattern" >/dev/null; then
-            echo "PASS: $container logs contain expected ACL rejection"
+        surviving="$logs"
+        for pattern in "$@"; do
+            surviving="$(printf '%s' "$surviving" | grep -F -- "$pattern" || true)"
+        done
+        if [ -n "$surviving" ]; then
+            echo "PASS: $container has a log line matching all of: $*"
             return 0
         fi
         sleep 1
     done
 
-    echo "FAIL: missing log pattern in $container: $pattern (waited ${timeout}s)" >&2
+    echo "FAIL: no single log line in $container contains all of: $* (waited ${timeout}s)" >&2
     exit 1
 }
 
@@ -175,9 +210,39 @@ assert_acl_field fips-acl-container-c${FIPS_CI_NAME_SUFFIX:-} allow_file_entries
 assert_acl_field fips-acl-container-c${FIPS_CI_NAME_SUFFIX:-} allow_entries "npub1cld9yay0u24davpu6c35l4vldrhzvaq66pcqtg9a0j2cnjrn9rtsxx2pe6 npub1n9lpnv0592cc2ps6nm0ca3qls642vx7yjsv35rkxqzj2vgds52sqgpverl npub1sjlh2c3x9w7kjsqg2ay080n2lff2uvt325vpan33ke34rn8l5jcqawh57m npub1tdwa4vjrjl33pcjdpf2t4p027nl86xrx24g4d3avg4vwvayr3g8qhd84le npub1x5z9rwzzm26q9verutx4aajhf2zw2pyp34c6whhde2zduxqav40qgq36l6 npub1ytrut7gjncn2zfnhn56c0zgftf0w6p99gf6fu8j73hzw5603zglqc9av6c"
 
 log "Checking ACL rejection logs"
-assert_log_contains fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} "npub1cld9yay0u24davpu6c35l4vldrhzvaq66pcqtg9a0j2cnjrn9rtsxx2pe6"
-assert_log_contains fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} "npub1n9lpnv0592cc2ps6nm0ca3qls642vx7yjsv35rkxqzj2vgds52sqgpverl"
-assert_log_contains fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} "context=inbound_handshake"
-assert_log_contains fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} "decision=denylist match"
+# One assertion per denied peer, each requiring the real message, that
+# peer's npub and the denylist decision on the SAME line. The npub being
+# on the rejection line is what carries the weight here: node-a rejected
+# THIS peer. The `decision=` conjunct adds no discrimination, since
+# PeerAclDecision::allowed() returns early for AllowList and DefaultAllow
+# (src/node/acl.rs:44-46), so DenyList is the only value that can reach
+# that warn! and every rejection line carries it. It is kept because the
+# remediation prescribes it and it documents the expected decision.
+#
+# Residual, recorded rather than hidden: node-a has auto_connect stanzas
+# for both denied peers and authorizes before dialing, so it emits a
+# fully-formed rejection line for each on the outbound_connect path.
+# These two assertions are therefore satisfiable without the inbound ACL
+# check running at all. The suite still catches that, because the denied
+# peer would then connect and the peer-count assertions above would time
+# out; but these two lines alone do not prove the inbound path.
+assert_log_line_contains_all fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} 15 \
+    "Rejected peer by ACL" \
+    "npub1cld9yay0u24davpu6c35l4vldrhzvaq66pcqtg9a0j2cnjrn9rtsxx2pe6" \
+    "decision=denylist match"
+assert_log_line_contains_all fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} 15 \
+    "Rejected peer by ACL" \
+    "npub1n9lpnv0592cc2ps6nm0ca3qls642vx7yjsv35rkxqzj2vgds52sqgpverl" \
+    "decision=denylist match"
+# The outsider-initiated path specifically, asserted separately and not
+# per-npub. Node-a carries static stanzas for both denied peers, so each
+# can be rejected on the outbound_connect path as well and which context
+# a given npub lands in is not deterministic (see README). Requiring an
+# inbound rejection of a *named* peer would red on scheduling rather than
+# on a regression; requiring that one exists at all does not.
+assert_log_line_contains_all fips-acl-container-a${FIPS_CI_NAME_SUFFIX:-} 15 \
+    "Rejected peer by ACL" \
+    "context=inbound_handshake" \
+    "decision=denylist match"
 
 log "ACL allowlist integration test passed"

testing/firewall/README.md

@@ -84,7 +84,7 @@ PASS: fips-fw-container-b: fips.nft baseline + drop-in loaded
 === Case (c): ICMPv6 echo-request to firewalled node
 PASS: (c) ICMPv6 ping node-a → node-b accepted
 === Case (a): unallowed inbound TCP/8000 from node-a → node-b
-PASS: (a) inbound TCP/8000 blocked (curl rc=28)
+PASS: (a) inbound TCP/8000 dropped (curl rc=28, timed out as expected)
 === Case (b): node-b initiates outbound TCP, expects reply via conntrack
 PASS: (b) outbound from node-b got HTTP 200 via conntrack reply path
 === Case (d): drop-in allowlisted TCP/22 from node-a → node-b

testing/firewall/test.sh

@@ -124,8 +124,20 @@ assert_baseline_loaded() {
     listing="$(docker exec "$container" nft list table inet fips)"
     # Default-deny is achieved via the trailing `counter drop` (chain
     # policy is `accept` for return-on-non-fips0 to work safely).
-    if ! printf '%s' "$listing" | grep -q 'counter packets'; then
-        fail "$container: counter drop rule missing from inet fips"
+    # Require the verdict, not just the counter: `counter packets` alone
+    # matches a counter rule with any verdict, including one that accepts.
+    # nft renders the rule as `counter packets N bytes M drop`.
+    #
+    # This checks that a counted drop rule exists somewhere in the table,
+    # not that it is the baseline's trailing one. A drop-in under
+    # /etc/fips/fips.d/ contributing its own counted drop would satisfy
+    # this even with the baseline rule removed. Asserting the rule's
+    # position is a larger change than this finding calls for; the
+    # drop-counter check at the end of the run is the one that reads the
+    # trailing rule specifically.
+    if ! printf '%s' "$listing" \
+        | grep -qE 'counter packets [0-9]+ bytes [0-9]+ drop'; then
+        fail "$container: trailing 'counter drop' rule missing from inet fips"
     fi
     if ! printf '%s' "$listing" | grep -q 'iifname != "fips0" return'; then
         fail "$container: non-fips0 early return rule missing"
@@ -191,7 +203,15 @@ fi
 # ── (a) Unallowed inbound is dropped ───────────────────────────────────
 log "Case (a): unallowed inbound TCP/${UNALLOWED_PORT} from node-a → node-b"
 # python3 http.server is already listening on :: per entrypoint default mode.
-# Use curl --max-time 5 — must time out (exit 28) or otherwise fail.
+#
+# The rule is a DROP, so the SYN is discarded with no RST and curl must
+# hit --max-time and exit 28. Assert exactly that rather than "any
+# non-zero rc": a REJECT, a closed port, an unroutable address or a
+# missing listener all fail too, with rc 7 or similar, and accepting
+# those would let the suite report a blocked connection when nothing was
+# ever blocked. Only 28 distinguishes "silently dropped" from "failed for
+# some other reason".
+CURL_TIMEOUT_RC=28
 set +e
 docker exec "$CONTAINER_A" curl -6 --silent --output /dev/null \
     --max-time 5 "http://[${ADDR_B}]:${UNALLOWED_PORT}/"
@@ -200,7 +220,10 @@ set -e
 if [ "$RC" -eq 0 ]; then
     fail "(a) connection to ${UNALLOWED_PORT} succeeded but should have been DROP'd (rc=0)"
 fi
-pass "(a) inbound TCP/${UNALLOWED_PORT} blocked (curl rc=$RC)"
+if [ "$RC" -ne "$CURL_TIMEOUT_RC" ]; then
+    fail "(a) connection to ${UNALLOWED_PORT} failed with curl rc=$RC, expected $CURL_TIMEOUT_RC (timeout). A DROP produces no RST, so anything else means the connection failed for a reason other than the firewall dropping it"
+fi
+pass "(a) inbound TCP/${UNALLOWED_PORT} dropped (curl rc=$RC, timed out as expected)"
 
 # ── (b) Outbound-initiated flow + conntrack reply ──────────────────────
 log "Case (b): node-b initiates outbound TCP, expects reply via conntrack"
@@ -237,8 +260,28 @@ fi
 
 # ── Drop-counter sanity ────────────────────────────────────────────────
 log "Drop counter incremented (case a should have ticked it)"
+# Scope to a drop rule rather than any counter rule: `/counter packets/`
+# alone takes the first counter rule of whatever verdict.
+#
+# Extract the count by position within the matched text, not by field
+# number. `$3` assumes the line starts with `counter`, so on a rule like
+# `tcp dport 9 counter packets 42 bytes 3000 drop` it prints the port,
+# not the packet count -- a plausible shape, since fips.nft includes
+# /etc/fips/fips.d/*.nft ahead of the trailing drop and a drop-in may
+# legitimately add its own counted drop.
+#
+# Take the LAST match, not the first. Case (a) is an unallowed inbound
+# that matches no accept rule and falls through to the baseline
+# default-deny, which fips.nft emits as the final rule of the chain
+# (packaging/common/fips.nft, `counter drop` after the drop-in include).
+# A drop-in's own counted drop would otherwise be read instead, and it
+# has nothing to do with what case (a) exercised.
 DROP_PKTS="$(docker exec "$CONTAINER_B" nft list table inet fips \
-    | awk '/counter packets/ && !seen { print $3; seen=1 }')"
+    | awk 'match($0, /counter packets [0-9]+ bytes [0-9]+ drop/) {
+             line = substr($0, RSTART); sub(/^counter packets /, "", line);
+             split(line, f, " "); val = f[1]
+           }
+           END { if (val != "") print val }')"
 if [ -z "${DROP_PKTS:-}" ] || [ "$DROP_PKTS" -lt 1 ]; then
     fail "drop counter is $DROP_PKTS — case (a) should have produced drops"
 fi

testing/static/scripts/gateway-test.sh

@@ -410,10 +410,26 @@ docker exec "$GATEWAY" conntrack -F 2>/dev/null || true
 echo "  Waiting 25s for TTL + grace period to expire (two tick cycles)..."
 sleep 25
 
-# Query gateway control socket for mapping count
+# Query gateway control socket for mapping count.
+#
+# The expected value here is zero, so the reader must not be able to
+# produce a zero from a failed query: an error response carries no `data`
+# field, and `r.get('data',{}).get('mappings',[])` would report that as
+# zero mappings and pass this check without the gateway having answered.
+# The same hazard is documented at the show_mappings poll above, which is
+# safe only because it waits for a positive "2". Require the key to exist
+# and exit non-zero if it does not, so the `|| echo "error"` fallback
+# fires and the check reds.
 MAPPING_COUNT=$(docker exec "$GATEWAY" bash -c \
     'echo "{\"command\":\"show_mappings\"}" | nc -U -w1 /run/fips/gateway.sock 2>/dev/null' \
-    | python3 -c "import sys,json; r=json.load(sys.stdin); print(len(r.get('data',{}).get('mappings',[])))" 2>/dev/null || echo "error")
+    | python3 -c "
+import sys, json
+r = json.load(sys.stdin)
+data = r.get('data')
+if not isinstance(data, dict) or not isinstance(data.get('mappings'), list):
+    sys.exit(1)
+print(len(data['mappings']))
+" 2>/dev/null || echo "error")
 if [ "$MAPPING_COUNT" = "0" ]; then
     check "Mapping reclaimed after TTL+grace" 0
 else