Audit fixes: 15 MEDIUM findings (vuln engine, parsers, discovery, auth, analysis)

backend/netsecops/core/config.py

@@ -400,6 +400,14 @@ def _guard_oidc(self) -> Self:
     def _guard_production(self) -> Self:
         """Fail closed on unsafe production configuration."""
         if self.env is Environment.PROD:
+            if "secret_key" not in self.model_fields_set:
+                # secret_key has a default_factory that mints a random key per process.
+                # Convenient in dev; in production it fails open — with more than one
+                # worker a token signed by one worker fails to verify on another, and
+                # every restart silently invalidates every session. An unset key is a
+                # misconfiguration, and must fail loudly rather than fall back to an
+                # ephemeral one that only looks like it works from a single worker.
+                raise ValueError("SECRET_KEY must be set explicitly in production")
             if self.debug:
                 raise ValueError("debug must be False in production")
             if not self.cookie_secure:

backend/netsecops/core/security.py

@@ -309,6 +309,34 @@ def verify_totp(secret: str, code: str, settings: Settings | None = None) -> boo
     return build_totp(secret, settings).verify(code, valid_window=settings.mfa_totp_valid_window)
 
 
+def matched_totp_step(secret: str, code: str, settings: Settings | None = None) -> int | None:
+    """The absolute time-step a valid code belongs to, or ``None`` if it is not valid.
+
+    ``verify_totp`` answers only yes/no, which is not enough to reject a replay. A code is
+    accepted anywhere in ``[S - window, S + window]``, so it stays valid across several
+    steps; a replay guard that records the *current* step rather than the step the code
+    belongs to leaves the code replayable for the rest of its own window. This returns the
+    step the code actually matched — the value the guard must store and compare against —
+    so a used code cannot be presented again while it is still inside its window.
+    """
+    settings = settings or get_settings()
+    code = code.strip().replace(" ", "")
+    if not code.isdigit() or len(code) != settings.mfa_totp_digits:
+        return None
+
+    totp = build_totp(secret, settings)
+    period = settings.mfa_totp_period_seconds
+    current_step = int(datetime.now(UTC).timestamp()) // period
+    window = settings.mfa_totp_valid_window
+
+    matched: int | None = None
+    for offset in range(-window, window + 1):
+        step = current_step + offset
+        if totp.verify(code, for_time=step * period, valid_window=0):
+            matched = step  # ascending, so the highest matching step wins
+    return matched
+
+
 def generate_recovery_codes(count: int = 10) -> list[str]:
     """Single-use recovery codes, issued alongside TOTP enrolment."""
     return [f"{secrets.token_hex(4)}-{secrets.token_hex(4)}" for _ in range(count)]

backend/netsecops/db/migrations/versions/0017_oidc_login_states.py

@@ -24,6 +24,12 @@
 The index on `expires_at` is for the sweep that removes abandoned sign-ins: a browser
 that never comes back leaves a row, and without the index that cleanup is a sequential
 scan on a table every login writes to.
+
+`org_id` carries `index=True` through `OrgMixin` (DATA-04), like every other tenant-scoped
+table, so the model declares `ix_oidc_login_states_org_id`. It is created here rather than
+in a later corrective migration (as 0011 had to do for `reports`): this is the head, so the
+index can go in where it belongs instead of the model and the database disagreeing and
+`alembic check` failing the build (C-5).
 """
 
 from __future__ import annotations
@@ -70,9 +76,13 @@ def upgrade() -> None:
     )
     op.create_index("ix_oidc_login_states_state", "oidc_login_states", ["state"], unique=True)
     op.create_index("ix_oidc_login_states_expires_at", "oidc_login_states", ["expires_at"])
+    op.create_index(
+        op.f("ix_oidc_login_states_org_id"), "oidc_login_states", ["org_id"], unique=False
+    )
 
 
 def downgrade() -> None:
+    op.drop_index(op.f("ix_oidc_login_states_org_id"), table_name="oidc_login_states")
     op.drop_index("ix_oidc_login_states_expires_at", table_name="oidc_login_states")
     op.drop_index("ix_oidc_login_states_state", table_name="oidc_login_states")
     op.drop_table("oidc_login_states")

backend/netsecops/discovery/executor.py

@@ -360,6 +360,15 @@ async def _record_batch(
             summary.addresses_probed += 1
             summary.probes_sent += result.probes_sent
 
+            # Run-level caveats (ICMP going away mid-run) ride on whichever host was being
+            # probed when they occurred, and that host is frequently one that does not
+            # respond. Collected before the liveness short-circuit below, or the very
+            # degradation that makes a run partial is the note most likely to be dropped —
+            # and the run would then report "found N hosts" with no sign echo had stopped.
+            for note in result.notes:
+                if note not in summary.notes:
+                    summary.notes.append(note)
+
             if not result.responded:
                 # Silence is the normal answer and is not recorded. A row per dead
                 # address would bury the queue under the network's empty space.
@@ -377,10 +386,6 @@ async def _record_batch(
                 hostname=result.hostname,
             )
 
-            for note in result.notes:
-                if note not in summary.notes:
-                    summary.notes.append(note)
-
             if scope_row.auto_onboard:
                 device = await self.reviews.auto_onboard(host, actor=actor)
                 if device is not None:

backend/netsecops/discovery/transport.py

@@ -110,6 +110,12 @@ class ProbeOutcome:
 
     probe: Probe
     responded: bool
+    #: The host proved it is on the network even if this probe did not succeed. A TCP RST
+    #: (connection refused) is the case: the port is closed, so `responded` is False and
+    #: the port is not opened or followed up, but the host is provably up. Distinct from
+    #: `responded` precisely so a refused port does not read as an open one. A successful
+    #: probe implies liveness and sets both.
+    host_alive: bool = False
     #: Whatever the host volunteered: a banner line, a certificate subject, a header
     #: block. Kept verbatim — the review queue shows a person the raw string, because
     #: "Cisco, 70%" is not something anybody can check.
@@ -276,9 +282,21 @@ async def send_tcp_connect(probe: Probe, *, timeout: float = DEFAULT_PROBE_TIMEO
             reader, writer = await asyncio.open_connection(probe.host, probe.port)
         await _close(writer)
         del reader
-        return ProbeOutcome(probe=probe, responded=True)
+        return ProbeOutcome(probe=probe, responded=True, host_alive=True)
     except TimeoutError:
         return ProbeOutcome(probe=probe, responded=False, detail="Connect timed out.")
+    except ConnectionRefusedError:
+        # A refused connection is an RST from the host itself: the port is closed, but
+        # the host is provably on the network. Timeouts and unreachable errors are
+        # indistinguishable from dead space; a refusal is not. Recorded as not-open yet
+        # alive, so a hardened device that drops ICMP and closes every scanned port is
+        # still found rather than filed as absent.
+        return ProbeOutcome(
+            probe=probe,
+            responded=False,
+            host_alive=True,
+            detail="Connection refused (host up, port closed).",
+        )
     except OSError as exc:
         return ProbeOutcome(probe=probe, responded=False, detail=str(exc))
 
@@ -516,6 +534,9 @@ async def probe(self, address: str) -> HostResult:
                 timeout=self.timeout,
             )
             result.probes_sent += 1
+            # A refusal proves the host is up without opening the port: it counts towards
+            # liveness but is not an open port and gets no follow-up read.
+            result.responded |= connect.host_alive
             if not connect.responded:
                 continue
 

backend/netsecops/parsers/arista/eos.py

@@ -53,9 +53,14 @@
 #: `ip address 10.10.10.2/30` — CIDR, unlike IOS.
 _CIDR_ADDRESS = re.compile(r"^\s*ip address (\d{1,3}(?:\.\d{1,3}){3}/\d{1,2})")
 
-#: `ip route 0.0.0.0/0 10.10.10.1 [name X] [tag N]`
+#: `ip route 0.0.0.0/0 10.10.10.1 [name X] [tag N]`, and the interface+gateway form
+#: `ip route 0.0.0.0/0 Ethernet1 10.1.1.1 [tag N]` where the egress interface and the
+#: next-hop gateway are both stated. The second group only matches a trailing address,
+#: so an administrative distance (a bare number) or `name`/`tag` keywords do not capture.
 _ROUTE = re.compile(
-    r"^ip route (?:vrf (?P<vrf>\S+) )?(?P<destination>\d{1,3}(?:\.\d{1,3}){3}/\d{1,2})\s+(?P<next_hop>\S+)"
+    r"^ip route (?:vrf (?P<vrf>\S+) )?(?P<destination>\d{1,3}(?:\.\d{1,3}){3}/\d{1,2})"
+    r"\s+(?P<first>\S+)"
+    r"(?:\s+(?P<gateway>\d{1,3}(?:\.\d{1,3}){3}))?"
 )
 
 #: `username admin privilege 15 role network-admin secret sha512 $6$…`
@@ -321,15 +326,20 @@ def _routing(self, parse: CiscoConfParse, result: ParseResult) -> None:
             found = _ROUTE.match(obj.text.strip())
             if not found:
                 continue
-            next_hop = found.group("next_hop")
-            # EOS accepts an interface where IOS would want an address. Stored as a next
-            # hop it becomes an edge to a device that does not exist.
-            is_address = re.match(r"^\d{1,3}(?:\.\d{1,3}){3}$", next_hop) is not None
+            first = found.group("first")
+            gateway = found.group("gateway")
+            # EOS accepts an interface where IOS would want an address, and it accepts
+            # both together (`ip route <prefix> <intf> <gw>`). Read the first token as an
+            # interface unless it is itself an address; the real next hop is then the
+            # trailing gateway. Storing an interface name as a next hop would make an edge
+            # to a device that does not exist; dropping the trailing gateway would make
+            # the route read as directly-attached and lose the forwarding hop entirely.
+            first_is_address = re.match(r"^\d{1,3}(?:\.\d{1,3}){3}$", first) is not None
             ncm.routing.routes.append(
                 Route(
                     destination=found.group("destination"),
-                    next_hop=next_hop if is_address else None,
-                    interface=None if is_address else next_hop,
+                    next_hop=first if first_is_address else gateway,
+                    interface=None if first_is_address else first,
                     protocol="static",
                     vrf=found.group("vrf"),
                 )

backend/netsecops/parsers/cloud/aws.py

@@ -217,14 +217,28 @@ def _groups(self, groups: list[dict[str, Any]], result: ParseResult) -> None:
         # Prefix lists are AWS-managed address sets — `pl-...` for S3, DynamoDB and the
         # rest — and their contents are not in this export either. Same treatment as a
         # security group, for the same reason.
+        #
+        # A rule may also reference a group by id (`sg-...`) that is not in this export at
+        # all: one in a peered VPC or another account. Its membership is externally
+        # resolved, exactly like a group whose instances live in describe-instances — not
+        # a collection gap the operator can close by re-collecting this account. Left
+        # untyped it would fall to the MISSING bucket and advise a re-collection that can
+        # never produce it.
         known = {o.name for o in ncm.firewall.address_groups}
         for rule in ncm.firewall.security_rules:
             for member in (*rule.src, *rule.dst):
-                if member.startswith("pl-") and member not in known:
+                if member in known:
+                    continue
+                if member.startswith("pl-"):
                     known.add(member)
                     ncm.firewall.address_groups.append(
                         NetworkObject(name=member, type="prefix-list", members=[])
                     )
+                elif member.startswith("sg-"):
+                    known.add(member)
+                    ncm.firewall.address_groups.append(
+                        NetworkObject(name=member, type="security-group", members=[])
+                    )
 
         ncm.firewall.zones = sorted(vpcs)
         # There is no single hostname for a VPC. The account's groups are the unit, and

backend/netsecops/parsers/cloud/azure.py

@@ -36,6 +36,7 @@
 
 from __future__ import annotations
 
+import ipaddress
 import json
 from typing import Any
 
@@ -51,6 +52,19 @@
 _ANY = frozenset({"*"})
 
 
+def _is_address_literal(member: str) -> bool:
+    """Whether a rule member is a CIDR or bare address rather than a service tag.
+
+    A dot is not proof: regional service tags (Storage.EastUS, Sql.WestEurope) all carry
+    one. Only a value that actually parses as an IP address or network is a literal.
+    """
+    try:
+        ipaddress.ip_network(member, strict=False)
+    except ValueError:
+        return False
+    return True
+
+
 def _payload(bundle: Any) -> list[dict[str, Any]]:
     """The NSGs in an export, whatever it was wrapped in.
 
@@ -226,9 +240,13 @@ def _service_tags(result: ParseResult) -> None:
             for member in (*rule.src, *rule.dst):
                 if member == "any" or member in known:
                     continue
-                # Anything that is not an address literal is a tag. CIDRs and bare
-                # addresses contain a dot or a colon; tags never do.
-                if any(ch in member for ch in ".:/"):
+                # Anything that is not an address literal is a tag. A dot or colon is not
+                # proof of an address: regional service tags always carry a dot
+                # (Storage.EastUS, Sql.WestEurope, AzureCloud.westus2). Only something
+                # that actually parses as an address or network is a literal; everything
+                # else stands for a set Microsoft defines elsewhere and is typed a tag,
+                # rather than falling to the unresolved/MISSING bucket as a fake gap.
+                if _is_address_literal(member):
                     continue
                 known.add(member)
                 ncm.firewall.address_objects.append(

backend/netsecops/parsers/vmware/nsx.py

@@ -155,7 +155,13 @@ def _groups(self, bundle: dict[str, Any], result: ParseResult) -> None:
             if "/groups" not in endpoint:
                 continue
             for item in _results(payload):
-                name = item.get("display_name") or item.get("id")
+                # Keyed on `id`, because that is the last path segment a rule's
+                # source_groups/destination_groups reduce to via _leaf. Preferring
+                # display_name (which differs from id for any API/Terraform-created group,
+                # e.g. id='grp-4821', display_name='Web Servers') catalogues the group
+                # under a name no rule references, so every rule using it loses its
+                # members and drops out of the overlap and shadowing analysis.
+                name = item.get("id") or item.get("display_name")
                 if not name:
                     continue
 
@@ -186,7 +192,10 @@ def _services(self, bundle: dict[str, Any], result: ParseResult) -> None:
             if "/services" not in endpoint:
                 continue
             for item in _results(payload):
-                name = item.get("display_name") or item.get("id")
+                # Keyed on `id` for the same reason as _groups: a rule's `services`
+                # reduce to the path's last segment (the id) via _leaf, so a service
+                # catalogued under a differing display_name would never resolve.
+                name = item.get("id") or item.get("display_name")
                 if not name:
                     continue
                 ports: list[str] = []