diff --git a/README.md b/README.md index 13bcc1e..f0af600 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A self-hosted, full-stack network management platform for MikroTik devices. Monitor, configure, and manage your entire MikroTik infrastructure — routers, switches, and wireless access points — from a single web interface. -![Version](https://img.shields.io/badge/version-0.19.2_Beta-blue) +![Version](https://img.shields.io/badge/version-0.20.0_Beta-blue) ![License](https://img.shields.io/badge/license-AGPLv3-blue) ![Docker](https://img.shields.io/badge/docker-compose-2496ED?logo=docker&logoColor=white) ![TypeScript](https://img.shields.io/badge/TypeScript-5.3-3178C6?logo=typescript&logoColor=white) @@ -138,6 +138,81 @@ A Meraki/UniFi-grade firewall experience built on the full RouterOS feature set: - **Bandwidth control** — simple-queue management with up/down caps per IP, subnet, or interface, and a one-click "Limit this client" action on the client detail page - **Active connections viewer** — live connection-tracking table (source/destination, protocol, state, rate, bytes) with search +### Lockout Protection & Config Health +RouterOS applies every command immediately and independently. There is no transaction and no +cross-object validation, so it will accept a change that severs its own management path — MikroTik's +documented advice for enabling VLAN filtering is literally to have a serial console ready. Three +layers close that gap: + +- **Change Guard (device-side auto-revert)** — before a risky change, the device saves a restore + point and arms a scheduler to reapply it. The change is applied, then reachability is proven on a + brand-new connection. Confirmed → the scheduler is disarmed; unreachable → the device restores + itself and comes back on its own. All over the RouterOS API, so it works without SSH credentials. + Covers 12 change types: bridge VLAN filtering, port PVID/tagging, bridge VLAN add/update/delete, + IP address add/remove, route add/remove, bond create/delete, and management service toggles +- **Lockout prediction** — rather than blocklisting "dangerous" operations (which only catches what + someone thought of), the platform reads live device state, resolves the actual management path, + simulates the proposed change against it, and reports any invariant that flips from satisfied to + violated. The ingress port is taken from the bridge forwarding table and the manager's own address + from the device's connection tracking, so the warning names the real mechanism: *"management + arrives untagged on sfp28-1 (PVID 1) — the gateway's MAC is learned there — but VLAN 1 has no + bridge VLAN entry listing bridge1 as an untagged member."* A predicted lockout is refused with a + 409 and an explanation of the hop chain; overriding a critical verdict requires typing the device + name, and the change then runs under Change Guard anyway +- **Config Health (standing audit)** — a scheduled, read-only audit for configurations RouterOS + accepted and will never complain about, encoding MikroTik's documented Layer2 misconfigurations: + an IP address or VLAN interface on a bridge slave port, a VLAN interface added as a bridge port, + a bond slave that is also a bridge port, MTU above L2MTU, a multi-VLAN bridge entry with untagged + ports, several bridges competing for hardware offload, a port belonging to no VLAN on a filtering + bridge, a PVID that `frame-type=admit-only-vlan-tagged` makes inert, and management that survives + only on a dynamic VLAN entry. Each finding explains what it actually does to the network, how to + fix it, how long it has been present, and links the MikroTik documentation. Findings appear on the + device's Security tab and in the dashboard's "Things to handle" + +- **Capability probe** — `POST /api/devices/:id/change-guard/probe` reports what safety mechanisms a + device actually supports, tested on the device rather than assumed. Non-destructive: it creates and + removes a harmless scheduler and leaves nothing behind. It also establishes whether `/system history` + can revert a change without a reboot — on RouterOS 7.23 over the binary API, `/system/history/print` + works and a bare `/undo` reverts the newest action, while `/system/history/undo` and `/undo numbers=` + do not exist. Undo is therefore untargeted, and it cannot replace auto-revert (a dead-man switch has + to fire once the manager has already lost contact), so binary restore remains the default + +#### What you see when auto-revert fires + +Verified end to end on a CRS running RouterOS 7.23.3, by deliberately deleting a switch's own +management address and letting the safety net recover it unattended: + +``` +T+0 restore point saved, revert scheduler armed, change applied +T+0s device drops off the network +T+40s manager gives up after 4 fresh connection attempts and reports back +T+120s device restores its own backup and reboots +T+3m device is back, change undone, scheduler and restore point gone +``` + +The request returns **HTTP 200** with `guard.auto_reverting: true` and *"Contact with the device was +lost while applying this change. It is restoring itself and should come back shortly."* — not an +error. This matters: the most dangerous changes sever the very connection carrying them, so the API +call times out even though the change succeeded on the device. Reachability, not the exception, +decides the outcome; a change that throws while the device is still reachable is a genuine failure +and is disarmed immediately so nothing reboots for nothing. + +Two consequences worth knowing before you rely on it: + +- **Binary mode reboots the device** to restore. That is the cost of a restore that recovers deleted + interfaces exactly. Script mode (`/export` + `/import`) avoids the reboot but is additive, so it + cannot recover a deletion. +- **The restore point is saved before the scheduler is armed**, so a rollback returns a configuration + containing neither the scheduler nor the change. It is self-cleaning, and a revert loop is + impossible. + +Guard history is recorded per device in `device_change_guards`: `committed` (change kept), +`reverted` (device left to rescue itself), `failed` (change rejected, nothing applied). + +Configure under **Settings → Change Guard** (`change_guard_enabled`, `change_guard_mode`, +`change_guard_timeout_sec`) and **Config Health** (`config_health_enabled`, +`config_health_interval_min`, default 60 minutes). + ### Switches - VLAN management (create, edit, delete VLANs) - Per-port configuration and VLAN membership @@ -460,6 +535,22 @@ All configuration is done via environment variables in `.env`: | `HTTP_PORT` | `80` | Host port for HTTP (redirects to HTTPS) | | `HTTPS_PORT` | `443` | Host port for HTTPS | +### Change Guard & Config Health settings + +These live in `app_settings` (Settings UI), not `.env`, so they can be changed without a restart: + +| Setting | Default | Description | +|---|---|---| +| `change_guard_enabled` | `true` | Master switch. When off, guarded changes still apply but with no safety net and no lockout refusal. | +| `change_guard_mode` | `binary` | `binary` = `/system backup` restore; recovers deletions exactly, **reboots the device**. `script` = `/export` + `/import`; no reboot, but additive, so it cannot undo a deletion. | +| `change_guard_timeout_sec` | `120` | How long the device waits before rescuing itself. Must exceed the time the manager needs to verify reachability (~40s), with margin for a slow device. | +| `config_health_enabled` | `true` | Runs the standing configuration audit. | +| `config_health_interval_min` | `60` | Audit cadence per device. Each pass is a read-only snapshot over the API. | + +Overriding a predicted lockout is a deliberate act: the API requires `confirm_lockout: true` in the +request body, and the UI requires typing the device name. The change then runs under Change Guard +regardless, so an override is a decision to rely on auto-revert rather than to bypass protection. + ### Secret management (self-healing) `JWT_SECRET` (session signing) and `ENCRYPTION_KEY` (AES-256-GCM credential encryption, `backend/src/utils/crypto.ts`) are managed automatically: diff --git a/backend/package.json b/backend/package.json index ca67b94..af64a86 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-backend", - "version": "0.19.2-beta", + "version": "0.20.0-beta", "description": "Mikrotik Network Management Platform - Backend API", "main": "dist/index.js", "scripts": { diff --git a/backend/src/db/migrate.ts b/backend/src/db/migrate.ts index faf112c..0d93c91 100644 --- a/backend/src/db/migrate.ts +++ b/backend/src/db/migrate.ts @@ -498,6 +498,64 @@ CREATE TABLE IF NOT EXISTS device_change_guards ( CREATE INDEX IF NOT EXISTS idx_change_guards_device ON device_change_guards(device_id, created_at DESC); CREATE INDEX IF NOT EXISTS idx_change_guards_pending ON device_change_guards(status, expires_at); +-- Change Guard ledger: committed_at used to be stamped for every terminal status, +-- so an abandoned or failed guard read as though the change had been kept. It now +-- means what it says, and resolved_at records when the guard finished either way. +ALTER TABLE device_change_guards ADD COLUMN IF NOT EXISTS resolved_at TIMESTAMPTZ; +UPDATE device_change_guards + SET resolved_at = COALESCE(resolved_at, committed_at) + WHERE status <> 'pending' AND resolved_at IS NULL; +UPDATE device_change_guards SET committed_at = NULL WHERE status <> 'committed'; + +-- VLAN cache correctness. +-- +-- The original key, UNIQUE(device_id, vlan_id), assumed a VID is unique per device. +-- It is not: RouterOS keys the bridge VLAN table on (bridge, vlan-ids), so the same +-- VID on two bridges collapsed onto one row and each poll overwrote the other. The +-- collector also stored only the first VID of a spec like "10-20"; it now expands +-- ranges into one row per VID and records the spec it came from in vlan_ids. +ALTER TABLE vlans ADD COLUMN IF NOT EXISTS vlan_ids TEXT; +ALTER TABLE vlans ADD COLUMN IF NOT EXISTS updated_at TIMESTAMPTZ DEFAULT NOW(); +UPDATE vlans SET bridge = '' WHERE bridge IS NULL; +ALTER TABLE vlans ALTER COLUMN bridge SET DEFAULT ''; +ALTER TABLE vlans ALTER COLUMN bridge SET NOT NULL; +ALTER TABLE vlans DROP CONSTRAINT IF EXISTS vlans_device_id_vlan_id_key; +-- Collapse rows the old key could not distinguish, keeping the most recent. +DELETE FROM vlans a USING vlans b + WHERE a.id < b.id AND a.device_id = b.device_id AND a.bridge = b.bridge AND a.vlan_id = b.vlan_id; +CREATE UNIQUE INDEX IF NOT EXISTS idx_vlans_device_bridge_vid ON vlans (device_id, bridge, vlan_id); + +-- Per-port VLAN membership. vlan_ids was declared but never written and tagged +-- was hardcoded false, which left the port editor unable to tell a trunk from an +-- access port. Both are now derived from the bridge VLAN table. +ALTER TABLE bridge_vlan_entries ADD COLUMN IF NOT EXISTS tagged_vlan_ids TEXT[]; +ALTER TABLE bridge_vlan_entries ADD COLUMN IF NOT EXISTS untagged_vlan_ids TEXT[]; + +-- Bridge VLAN filtering lived only inside interfaces.config_json, so nothing could +-- query for it. Null for non-bridge interfaces. +ALTER TABLE interfaces ADD COLUMN IF NOT EXISTS vlan_filtering BOOLEAN; + +-- Config Health: standing audit findings, one row per (device, distinct problem). +-- Rows are refreshed in place so first_seen shows how long a problem has existed, +-- and findings absent from the latest scan are pruned. +CREATE TABLE IF NOT EXISTS device_config_findings ( + id SERIAL PRIMARY KEY, + device_id INTEGER NOT NULL REFERENCES devices(id) ON DELETE CASCADE, + rule VARCHAR(64) NOT NULL, + fingerprint VARCHAR(160) NOT NULL, + severity VARCHAR(16) NOT NULL, + title TEXT NOT NULL, + detail TEXT NOT NULL, + remediation TEXT, + doc_url TEXT, + objects TEXT[] NOT NULL DEFAULT '{}', + first_seen TIMESTAMPTZ DEFAULT NOW(), + last_seen TIMESTAMPTZ DEFAULT NOW(), + UNIQUE(device_id, fingerprint) +); +CREATE INDEX IF NOT EXISTS idx_config_findings_device ON device_config_findings(device_id, severity); +ALTER TABLE devices ADD COLUMN IF NOT EXISTS config_health_checked_at TIMESTAMPTZ; + -- Wireless security profiles (WPA/WPA2/WPA3 config) CREATE TABLE IF NOT EXISTS wireless_security_profiles ( id SERIAL PRIMARY KEY, @@ -547,6 +605,8 @@ const DEFAULT_SETTINGS = [ { key: 'change_guard_enabled', value: true }, { key: 'change_guard_mode', value: 'binary' }, { key: 'change_guard_timeout_sec', value: 120 }, + { key: 'config_health_enabled', value: true }, + { key: 'config_health_interval_min', value: 60 }, ]; export async function runMigrations(): Promise { diff --git a/backend/src/routes/devices.ts b/backend/src/routes/devices.ts index dae9032..9dbb777 100644 --- a/backend/src/routes/devices.ts +++ b/backend/src/routes/devices.ts @@ -12,9 +12,11 @@ import { createDeviceFromBody, type CreateDeviceInput, type CreateDeviceContext import { parsePort } from '../utils/parsePort'; import { safeConnectionError } from '../utils/safeClientError'; import { detectLockoutRisk } from '../utils/firewallSafety'; -import { withSafeApply, type GuardDevice } from '../services/changeGuard/ChangeGuard'; +import { withSafeApply, probeCapability, probeUndoCapability, type GuardDevice } from '../services/changeGuard/ChangeGuard'; import { captureSnapshot, resolveManagementPath } from '../services/changeGuard/pathModel'; import { analyzeChange, type PlannedChange } from '../services/changeGuard/analyzeChange'; +import { runConfigHealth } from '../services/changeGuard/configHealth'; +import { isMultiVlanSpec } from '../utils/vlan'; import { redis } from '../config/redis'; import { enqueueBulkAddJob, getBulkAddJobState } from '../services/DeviceBulkAddWorker'; @@ -995,6 +997,11 @@ async function withGuardedChange( ...(typeof outcome.result === 'object' && outcome.result !== null ? outcome.result as Record : { result: outcome.result }), + // A change that severed the connection produces no result — the request never + // got a reply. Say what is happening rather than returning a bare guard block. + ...(outcome.result === undefined && outcome.autoReverting + ? { message: 'Contact with the device was lost while applying this change. It is restoring itself and should come back shortly.' } + : {}), guard: { protected: !outcome.unprotectedReason, confirmed: outcome.confirmed, @@ -1060,6 +1067,64 @@ router.post('/:id/preflight', requireWrite, async (req: Request, res: Response) } }); +// POST /api/devices/:id/change-guard/probe — what safety mechanisms this device +// actually supports. Non-destructive: both probes create and remove a harmless +// scheduler, and neither leaves anything behind. +router.post('/:id/change-guard/probe', requireWrite, async (req: Request, res: Response) => { + const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); + if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); + const device = deviceRow as unknown as GuardDevice; + try { + // Sequential, not parallel: both add a scheduler, and overlapping them would + // make each one's history diff ambiguous. + const scheduler = await probeCapability(device); + const undo = await probeUndoCapability(device); + return res.json({ scheduler, undo }); + } catch (err) { + return res.status(502).json({ error: `Could not probe device: ${(err as Error).message}` }); + } +}); + +// GET /api/devices/:id/config-health — the standing audit's latest findings, read +// from cache so the page loads without touching the device. +router.get('/:id/config-health', async (req: Request, res: Response) => { + const device = await queryOne<{ config_health_checked_at: string | null }>( + `SELECT config_health_checked_at FROM devices WHERE id = $1`, + [req.params.id] + ); + if (!device) return res.status(404).json({ error: 'Device not found' }); + + const findings = await query( + `SELECT rule, severity, title, detail, remediation, doc_url, objects, first_seen, last_seen + FROM device_config_findings + WHERE device_id = $1 + ORDER BY CASE severity WHEN 'critical' THEN 0 WHEN 'warning' THEN 1 ELSE 2 END, rule`, + [req.params.id] + ); + return res.json({ findings, checked_at: device.config_health_checked_at }); +}); + +// POST /api/devices/:id/config-health/scan — audit now instead of waiting for the +// scheduled pass. Read-only on the device, but it opens a connection. +router.post('/:id/config-health/scan', requireWrite, async (req: Request, res: Response) => { + const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); + if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); + try { + const { findings, checkedAt } = await runConfigHealth(deviceRow as unknown as GuardDevice); + // Same shape as the cached GET, so the UI has one contract for both. + return res.json({ + findings: findings.map((f) => ({ + rule: f.rule, severity: f.severity, title: f.title, detail: f.detail, + remediation: f.remediation, doc_url: f.docUrl, objects: f.objects, + first_seen: null, last_seen: checkedAt.toISOString(), + })), + checked_at: checkedAt.toISOString(), + }); + } catch (err) { + return res.status(502).json({ error: `Could not read device state: ${(err as Error).message}` }); + } +}); + router.get('/:id/address-lists', async (req, res) => { await withCollector(req.params.id, res, (c) => c.getAddressLists()); }); @@ -1797,6 +1862,18 @@ router.get('/:id/routing/router-id', async (req: Request, res: Response) => { }); // POST /api/devices/:id/vlans (add bridge VLAN) +/** + * A single RouterOS bridge VLAN row can cover a range or list ("10-20"), which the + * cache expands into one row per VLAN. Editing or deleting one of those rows would + * rewrite the whole entry, silently taking the other VLANs with it — so refuse, and + * refuse *before* arming the guard, since a mid-apply failure would cost a reboot. + */ +function multiVlanRefusal(vlan: { vlan_id: number; bridge: string; vlan_ids: string | null }): string | null { + if (!isMultiVlanSpec(vlan.vlan_ids ?? undefined)) return null; + return `VLAN ${vlan.vlan_id} on ${vlan.bridge} is part of the multi-VLAN entry "${vlan.vlan_ids}". ` + + `Changing it here would affect every VLAN in that entry, so split the entry on the device first.`; +} + // Guarded: bridge VLAN table edits decide which ports (and the bridge/CPU port // itself) may carry the management VLAN. router.post('/:id/vlans', requireWrite, async (req: Request, res: Response) => { @@ -1824,11 +1901,13 @@ router.post('/:id/vlans', requireWrite, async (req: Request, res: Response) => { // PUT /api/devices/:id/vlans/:vlanDbId (update tagged/untagged ports) router.put('/:id/vlans/:vlanDbId', requireWrite, async (req: Request, res: Response) => { - const vlan = await queryOne<{ id: number; vlan_id: number; bridge: string }>( - `SELECT id, vlan_id, bridge FROM vlans WHERE id = $1 AND device_id = $2`, + const vlan = await queryOne<{ id: number; vlan_id: number; bridge: string; vlan_ids: string | null }>( + `SELECT id, vlan_id, bridge, vlan_ids FROM vlans WHERE id = $1 AND device_id = $2`, [req.params.vlanDbId, req.params.id] ); if (!vlan) return res.status(404).json({ error: 'VLAN not found' }); + const multi = multiVlanRefusal(vlan); + if (multi) return res.status(409).json({ error: multi, reason: multi }); const { tagged_ports = [], untagged_ports = [] } = req.body; @@ -1854,11 +1933,13 @@ router.put('/:id/vlans/:vlanDbId', requireWrite, async (req: Request, res: Respo // DELETE /api/devices/:id/vlans/:vlanDbId router.delete('/:id/vlans/:vlanDbId', requireWrite, async (req: Request, res: Response) => { - const vlan = await queryOne<{ vlan_id: number; bridge: string }>( - `SELECT vlan_id, bridge FROM vlans WHERE id = $1 AND device_id = $2`, + const vlan = await queryOne<{ vlan_id: number; bridge: string; vlan_ids: string | null }>( + `SELECT vlan_id, bridge, vlan_ids FROM vlans WHERE id = $1 AND device_id = $2`, [req.params.vlanDbId, req.params.id] ); if (!vlan) return res.status(404).json({ error: 'VLAN not found' }); + const multi = multiVlanRefusal(vlan); + if (multi) return res.status(409).json({ error: multi, reason: multi }); await withGuardedChange( req.params.id, diff --git a/backend/src/routes/operations.ts b/backend/src/routes/operations.ts index 94543d1..23968d7 100644 --- a/backend/src/routes/operations.ts +++ b/backend/src/routes/operations.ts @@ -278,6 +278,38 @@ router.get('/insights', async (_req: Request, res: Response) => { } } catch { /* scan data unavailable */ } + // Config Health — configurations RouterOS accepted but that do not work. These + // are latent: nothing alerts on them because the device reports no error, so the + // dashboard is where they surface. Grouped per device to avoid flooding the feed. + try { + const findings = await query<{ + device_id: number; name: string; severity: string; title: string; rule: string; count: string; + }>(` + SELECT f.device_id, d.name, f.severity, f.title, f.rule, + COUNT(*) OVER (PARTITION BY f.device_id, f.severity) AS count + FROM device_config_findings f + JOIN devices d ON d.id = f.device_id + WHERE f.severity IN ('critical', 'warning') + ORDER BY f.device_id, CASE f.severity WHEN 'critical' THEN 0 ELSE 1 END, f.rule`); + + const seen = new Set(); + for (const f of findings) { + const key = `${f.device_id}:${f.severity}`; + if (seen.has(key)) continue; // one item per device+severity, naming the first + seen.add(key); + const extra = parseInt(f.count, 10) - 1; + attention.push({ + sev: f.severity === 'critical' ? 'error' : 'warn', + category: 'config', + title: `${f.name}: ${f.title}`, + body: extra > 0 + ? `RouterOS accepted this configuration without complaint. ${extra} more ${f.severity} finding${extra !== 1 ? 's' : ''} on this device.` + : 'RouterOS accepted this configuration without complaint.', + action: 'Open Config Health', path: `/devices/${f.device_id}?tab=security`, + }); + } + } catch { /* findings table unavailable */ } + // Baseline anomalies (client counts, CPU, error bursts) const anomalies = await detectAnomalies(); for (const a of anomalies) attention.push(a); diff --git a/backend/src/services/PollerService.ts b/backend/src/services/PollerService.ts index be4c87c..7b95f52 100644 --- a/backend/src/services/PollerService.ts +++ b/backend/src/services/PollerService.ts @@ -6,10 +6,12 @@ import { Server as SocketServer } from 'socket.io'; import { getWriteApi } from '../config/influxdb'; import { Point } from '@influxdata/influxdb-client'; import { alertService } from './AlertService'; +import { runConfigHealth } from './changeGuard/configHealth'; +import type { GuardDevice } from './changeGuard/ChangeGuard'; interface PollJob { deviceId: number; - type: 'fast' | 'slow' | 'logs' | 'full' | 'macscan' | 'spectral' | 'apscan' | 'configsnap'; + type: 'fast' | 'slow' | 'logs' | 'full' | 'macscan' | 'spectral' | 'apscan' | 'configsnap' | 'confighealth'; } export class PollerService { @@ -75,6 +77,8 @@ export class PollerService { await this.slowQueue.add('device-apscan', jobData, { attempts: 1 }); } else if (type === 'configsnap') { await this.slowQueue.add('device-configsnap', jobData, { attempts: 1 }); + } else if (type === 'confighealth') { + await this.slowQueue.add('device-confighealth', jobData, { attempts: 1 }); } } @@ -102,6 +106,8 @@ export class PollerService { const backupScheduleCron = (appSettings['backup_schedule_cron'] as string) || '0 2 * * *'; const configSnapEnabled = appSettings['config_snapshot_enabled'] !== false; const configSnapIntervalMin = (appSettings['config_snapshot_interval_min'] as number) || 60; + const configHealthEnabled = appSettings['config_health_enabled'] !== false; + const configHealthIntervalMin = (appSettings['config_health_interval_min'] as number) || 60; const devices = await query( `SELECT * FROM devices WHERE status != 'disabled'` @@ -171,6 +177,17 @@ export class PollerService { await this.setTimestamp(configKey, now, configSnapIntervalMin * 60 + 120); } } + + // Config Health — audit for configurations RouterOS accepted but that do + // not work. Own cadence (default 60min) because it costs a full state read. + if (configHealthEnabled) { + const healthKey = `poll:confighealth:${device.id}`; + const lastHealth = await this.getTimestamp(healthKey); + if (now - lastHealth > configHealthIntervalMin * 60_000) { + await this.scheduleDeviceSync(device.id, 'confighealth'); + await this.setTimestamp(healthKey, now, configHealthIntervalMin * 60 + 120); + } + } } // Reverse DNS enrichment — global, runs every 5 minutes when enabled @@ -709,6 +726,25 @@ export class PollerService { } } + /** + * Audit the device for configurations RouterOS accepted but that do not work. + * Read-only over the API, and failures are non-fatal: a device we cannot read is + * left with its previous findings rather than being wrongly reported as clean. + */ + private async processConfigHealthJob(data: PollJob): Promise { + const device = await this.getDevice(data.deviceId); + if (!device) return; + try { + const { findings } = await runConfigHealth(device as unknown as GuardDevice); + const critical = findings.filter((f) => f.severity === 'critical').length; + if (critical > 0) { + this.io?.emit('device:updated', { deviceId: device.id }); + } + } catch (err) { + console.error(`[Poller] Config health audit failed for ${device.name}:`, (err as Error).message); + } + } + private async processSlowJob(data: PollJob): Promise { if (data.type === 'spectral') { return this.processSpectralJob(data); @@ -719,6 +755,9 @@ export class PollerService { if (data.type === 'configsnap') { return this.processConfigSnapJob(data); } + if (data.type === 'confighealth') { + return this.processConfigHealthJob(data); + } const device = await this.getDevice(data.deviceId); if (!device) return; diff --git a/backend/src/services/changeGuard/ChangeGuard.ts b/backend/src/services/changeGuard/ChangeGuard.ts index bcb4dfd..a998f4c 100644 --- a/backend/src/services/changeGuard/ChangeGuard.ts +++ b/backend/src/services/changeGuard/ChangeGuard.ts @@ -118,6 +118,165 @@ export async function probeCapability(device: GuardDevice): Promise<{ ok: boolea } } +export interface UndoProbeResult { + /** True only when an undo was performed and verified to have taken effect. */ + ok: boolean; + /** `/system/history/print` is reachable over the binary API. */ + historyReadable: boolean; + /** The device marked the probe action as undoable. */ + actionUndoable: boolean; + /** Entries currently in the history buffer (RouterOS caps this at 100). */ + historyDepth: number; + /** RouterOS version, since history/undo support varies across releases. */ + rosVersion: string | null; + /** What each candidate undo command did, so a failure says which forms were tried. */ + attempts: { command: string; error: string }[]; + reason?: string; +} + +/** + * In the CLI, `undo` is a global command used after `/system history print` rather + * than a child of that path, and releases differ on whether it can be pointed at a + * specific entry or only steps back from the most recent one. Rather than betting on + * one spelling, the probe tries each form and reports what every one of them did. + * + * `targeted: false` means the command takes no entry id and simply undoes the newest + * undoable action on the device — which is only safe when we know the newest action + * is ours. + */ +const UNDO_COMMANDS: { command: string; targeted: boolean }[] = [ + { command: '/system/history/undo', targeted: true }, + { command: '/undo', targeted: true }, + { command: '/undo', targeted: false }, + { command: '/system/history/unset', targeted: true }, +]; + +/** + * Can this device revert a change through `/system history` instead of a binary + * restore? + * + * A binary restore reboots. `/system history` records every action with its exact + * inverse, so undoing would be surgical and take no outage at all — but MikroTik + * documents the history/undo pair for the CLI, not the binary API, and the buffer + * is capped at 100 actions. Whether it works at all is an empirical question, so + * this answers it on the real device rather than assuming either way. + * + * The probe is non-destructive: it adds the same harmless scheduler `probeCapability` + * uses, undoes that one action, and confirms the scheduler is gone. If the undo does + * not work the scheduler is removed directly, so the device is unchanged either way. + * + * Measured on RouterOS 7.23.1 and 7.23.3 (CRS switches, binary API): + * `/system/history/print` works + * `/system/history/undo` "no such command" + * `/undo numbers=` "unknown parameter numbers" + * `/undo` works — reverts the newest undoable action, no reboot + * + * So undo is real over the API but **untargeted**, and it cannot replace the + * scheduler-based auto-revert: the dead-man switch has to fire when the manager has + * *lost* contact, and issuing an undo requires the contact we just lost. Its value is + * as an operator-driven rollback while the device is still reachable, and only when + * our action is verifiably the newest entry — which is why the untargeted form is + * guarded by that check below. + */ +export async function probeUndoCapability(device: GuardDevice): Promise { + const client = newClient(device); + const probeName = `mtm-undo-probe-${token()}`; + const attempts: { command: string; error: string }[] = []; + const fail = (over: Partial): UndoProbeResult => ({ + ok: false, historyReadable: false, actionUndoable: false, historyDepth: 0, + rosVersion: null, attempts, ...over, + }); + + let rosVersion: string | null = null; + let created = false; + + try { + await client.connect(); + + rosVersion = (await client.execute('/system/resource/print').catch(() => []))[0]?.['version'] ?? null; + + let before: Record[]; + try { + before = await client.execute('/system/history/print'); + } catch (err) { + return fail({ rosVersion, reason: `/system/history/print is not available: ${(err as Error).message}` }); + } + const beforeIds = new Set(before.map((h) => h['.id']).filter(Boolean)); + + await client.execute('/system/scheduler/add', { + name: probeName, + interval: '1d', + 'on-event': ':log info "mtm-undo-probe"', + }); + created = true; + + const after = await client.execute('/system/history/print'); + const entry = after.find((h) => h['.id'] && !beforeIds.has(h['.id'])); + const base = { + historyReadable: true, + historyDepth: after.length, + rosVersion, + }; + + if (!entry) { + return fail({ ...base, reason: 'the scheduler was created but no matching history entry appeared' }); + } + const actionUndoable = entry['undoable'] !== 'false'; + if (!actionUndoable) { + return fail({ ...base, reason: 'the device recorded the action as not undoable' }); + } + + for (const { command, targeted } of UNDO_COMMANDS) { + const label = targeted ? `${command} numbers=` : command; + // An untargeted undo steps back from whatever is newest on the device, so only + // try it while our own action is still the most recent entry. + if (!targeted) { + const newest = (await client.execute('/system/history/print').catch(() => []))[0]; + if (newest?.['.id'] !== entry['.id']) { + attempts.push({ command: label, error: 'skipped — our action is no longer the newest history entry' }); + continue; + } + } + try { + await client.execute(command, targeted ? { numbers: entry['.id'] } : {}); + } catch (err) { + attempts.push({ command: label, error: (err as Error).message }); + continue; + } + + // Only the observable result counts: a command that returns without error but + // leaves the scheduler in place is not an undo we can build a revert mode on. + const stillThere = (await client.execute('/system/scheduler/print')) + .some((s) => s['name'] === probeName); + if (stillThere) { + attempts.push({ command: label, error: 'accepted, but the change was still applied' }); + continue; + } + + created = false; + return { ok: true, ...base, actionUndoable, attempts }; + } + + return fail({ + ...base, + actionUndoable, + reason: `no undo command worked — ${attempts.map((a) => `${a.command}: ${a.error}`).join('; ')}`, + }); + } catch (err) { + return fail({ rosVersion, reason: (err as Error).message }); + } finally { + // Never leave the probe behind, whatever went wrong above. + if (created) { + const leftover = (await client.execute('/system/scheduler/print').catch(() => [])) + .find((s) => s['name'] === probeName); + if (leftover?.['.id']) { + await client.execute('/system/scheduler/remove', { '.id': leftover['.id'] }).catch(() => {}); + } + } + client.disconnect(); + } +} + /** Prove reachability on a brand-new connection, retried across the window. */ async function verifyReachable(device: GuardDevice, attempts = VERIFY_ATTEMPTS): Promise { for (let i = 0; i < attempts; i++) { @@ -174,7 +333,10 @@ async function setGuardStatus(id: number | undefined, status: string, note?: str `UPDATE device_change_guards SET status = $2::varchar, note = COALESCE($3::text, note), - committed_at = CASE WHEN $2::varchar <> 'pending' THEN NOW() ELSE committed_at END + -- Only a genuine commit is a commit. Stamping this on 'failed' or + -- 'reverted' made the ledger read as though the change had been kept. + committed_at = CASE WHEN $2::varchar = 'committed' THEN NOW() ELSE committed_at END, + resolved_at = CASE WHEN $2::varchar <> 'pending' THEN NOW() ELSE resolved_at END WHERE id = $1`, [id, status, note ?? null] ).catch((e) => { @@ -247,31 +409,56 @@ export async function withSafeApply( } // ── Apply ────────────────────────────────────────────────────────────── - let result: T; + // + // A thrown error here does NOT mean the change failed. The most dangerous + // changes sever the very connection carrying them: deleting the management + // address makes `/ip/address/remove` succeed on the device and then time out + // waiting for a reply we can never receive. Proven on hardware — the switch + // locked itself out, self-restored and came back, while the API reported only + // "Read timeout waiting for API response". + // + // So reachability, not the exception, decides what happened. Disarming here on + // the assumption of failure would be the worst possible move: it would strip the + // safety net off a device that has just gone silent. + let result: T | undefined; + let applyError: unknown; try { result = await applyFn(); } catch (err) { - // The change itself failed. Disarm so the device isn't rebooted for nothing. - await cleanup(client, restorePoint, schedulerName, mode).catch(() => {}); - await setGuardStatus(guardId, 'failed', (err as Error).message); - armed = false; - throw err; + applyError = err; } // ── Verify ───────────────────────────────────────────────────────────── const reachable = await verifyReachable(device); if (!reachable) { // Leave the scheduler armed: the device will restore itself and come back. - await setGuardStatus(guardId, 'reverted', 'Device unreachable after change; left to self-restore'); + const note = applyError + ? `Contact lost while applying (${(applyError as Error).message}); left to self-restore` + : 'Device unreachable after change; left to self-restore'; + await setGuardStatus(guardId, 'reverted', note); armed = false; // deliberately not cleaned up - return { result, confirmed: false, autoReverting: true, guardId }; + // `result` is undefined when the change took the connection with it. The + // caller only reports the outcome in that case, so the guard fields carry the + // meaning rather than the result. + return { result: result as T, confirmed: false, autoReverting: true, guardId }; + } + + if (applyError) { + // Still reachable, so the change genuinely failed and nothing needs undoing. + // Disarm, or the device would reboot for no reason. + await cleanup(client, restorePoint, schedulerName, mode).catch(() => {}); + await setGuardStatus(guardId, 'failed', (applyError as Error).message); + armed = false; + throw applyError; } // ── Commit ───────────────────────────────────────────────────────────── await cleanup(client, restorePoint, schedulerName, mode); armed = false; await setGuardStatus(guardId, 'committed'); - return { result, confirmed: true, autoReverting: false, guardId }; + // Reaching here means applyFn returned normally, so `result` is assigned; the + // optional type only exists for the severed-connection path above. + return { result: result as T, confirmed: true, autoReverting: false, guardId }; } finally { if (armed) await setGuardStatus(guardId, 'pending'); client.disconnect(); diff --git a/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts b/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts index c5f0aa0..484a275 100644 --- a/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts +++ b/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts @@ -134,6 +134,44 @@ describe('withSafeApply', () => { expect(deviceSchedulers).toHaveLength(0); }); + /** + * Observed on real hardware: deleting the management address applies on the device + * and then times out, because the reply has to travel over the address that just + * disappeared. Treating that throw as "the change failed" would disarm the guard on + * a device that has already gone silent — stripping the safety net at the exact + * moment it is needed — and would report a raw timeout instead of telling the user + * the device is restoring itself. + */ + it('treats a change that severs the connection as auto-reverting, not as a failure', async () => { + unreachableAfterApply = true; + + const out = await withSafeApply(device, { kind: 'ip.remove', summary: 's' }, async () => { + applyDone = true; + throw new Error('Read timeout waiting for API response'); + }); + + expect(out.autoReverting).toBe(true); + expect(out.confirmed).toBe(false); + // The safety net must survive: the device restores itself from these. + expect(deviceSchedulers).toHaveLength(1); + expect(deviceFiles).toHaveLength(1); + }, 30_000); + + it('records a severed-connection guard as reverted, never as committed', async () => { + unreachableAfterApply = true; + await withSafeApply(device, { kind: 'ip.remove', summary: 's' }, async () => { + applyDone = true; + throw new Error('Read timeout waiting for API response'); + }); + + const statuses = mockedQuery.mock.calls + .filter(([sql]) => String(sql).includes('UPDATE device_change_guards')) + .map(([, params]) => (params as unknown[])[1]); + expect(statuses).toContain('reverted'); + expect(statuses).not.toContain('committed'); + expect(statuses).not.toContain('failed'); + }, 30_000); + it('still applies the change when protection cannot be armed, and says so', async () => { MockedClient.mockImplementation(() => ({ connect: jest.fn(async () => {}), diff --git a/backend/src/services/changeGuard/__tests__/analyzeChange.test.ts b/backend/src/services/changeGuard/__tests__/analyzeChange.test.ts index 7712663..ad0b737 100644 --- a/backend/src/services/changeGuard/__tests__/analyzeChange.test.ts +++ b/backend/src/services/changeGuard/__tests__/analyzeChange.test.ts @@ -36,6 +36,7 @@ function baseSnapshot(over: Partial = {}): DeviceSnapshot { { bridge: 'bridge', 'vlan-ids': '10', tagged: 'sfp28-1', untagged: '', 'current-tagged': 'sfp28-1', 'current-untagged': '' }, ], + bonds: [], routes: [ { '.id': '*A', 'dst-address': '0.0.0.0/0', gateway: '192.168.0.1', active: 'true' }, { '.id': '*B', 'dst-address': '192.168.0.0/24', gateway: 'bridge', active: 'true' }, diff --git a/backend/src/services/changeGuard/__tests__/configHealth.test.ts b/backend/src/services/changeGuard/__tests__/configHealth.test.ts new file mode 100644 index 0000000..ca43909 --- /dev/null +++ b/backend/src/services/changeGuard/__tests__/configHealth.test.ts @@ -0,0 +1,359 @@ +import { auditConfig } from '../configHealth'; +import type { DeviceSnapshot } from '../pathModel'; +import type { GuardDevice } from '../ChangeGuard'; + +const device: GuardDevice = { + id: 1, + name: 'test-switch', + ip_address: '192.168.0.40', + api_port: 8728, + api_username: 'admin', + api_password_encrypted: 'x', +}; + +/** + * A healthy VLAN-filtering switch: management untagged on VLAN 1 with the bridge + * explicitly a member, one tagged trunk, no anti-patterns. Every test starts here + * and breaks exactly one thing, so a rule that fires on the base snapshot is a + * false positive by construction. + */ +function healthy(over: Partial = {}): DeviceSnapshot { + return { + addresses: [{ '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }], + interfaces: [ + { name: 'bridge', type: 'bridge', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + { name: 'sfp28-1', type: 'ether', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + { name: 'ether1', type: 'ether', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + ], + vlanInterfaces: [], + bridges: [{ name: 'bridge', 'vlan-filtering': 'true', pvid: '1' }], + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + ], + bridgeVlans: [ + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'bridge,sfp28-1,ether1', + 'current-tagged': '', 'current-untagged': 'bridge,sfp28-1,ether1', dynamic: 'false' }, + { bridge: 'bridge', 'vlan-ids': '10', tagged: 'sfp28-1,bridge', untagged: '', + 'current-tagged': 'sfp28-1,bridge', 'current-untagged': '', dynamic: 'false' }, + ], + bonds: [], + routes: [{ '.id': '*A', 'dst-address': '0.0.0.0/0', gateway: '192.168.0.1', active: 'true' }], + arp: [{ address: '192.168.0.1', 'mac-address': 'AA:BB:CC:00:00:01', interface: 'bridge' }], + bridgeHosts: [{ 'mac-address': 'AA:BB:CC:00:00:01', 'on-interface': 'sfp28-1', local: 'false' }], + services: [{ '.id': '*3', name: 'api', port: '8728', disabled: 'false' }], + firewallFilter: [], + mgmtConnections: [], + ...over, + }; +} + +const rules = (snap: DeviceSnapshot) => auditConfig(snap, device).map((f) => f.rule); + +describe('auditConfig', () => { + it('reports nothing on a correctly configured switch', () => { + expect(auditConfig(healthy(), device)).toEqual([]); + }); + + describe('ip-on-bridge-port', () => { + it('flags an address on a bridge slave port', () => { + const snap = healthy({ + addresses: [ + { '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }, + { '.id': '*2', address: '10.0.0.1/24', interface: 'ether1', disabled: 'false' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'ip-on-bridge-port'); + expect(found?.severity).toBe('critical'); + expect(found?.objects).toContain('ether1'); + }); + + /** + * Taken verbatim from a CRS running 7.23.3: the stock `defconf` address is + * configured on a slave port and RouterOS relocates it to the bridge. Calling + * this critical would fire on a default configuration that works fine. + */ + it('reports a RouterOS-relocated address as informational, not critical', () => { + const snap = healthy({ + addresses: [{ + '.id': '*1', address: '192.168.0.51/24', interface: 'ether1', + 'actual-interface': 'bridge', slave: 'true', disabled: 'false', invalid: 'false', + comment: 'defconf', + }], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'ip-on-bridge-port'); + expect(found?.severity).toBe('info'); + expect(found?.title).toContain('served by bridge'); + }); + + it('does not flag an address configured directly on the bridge', () => { + expect(rules(healthy())).not.toContain('ip-on-bridge-port'); + }); + + it('does not flag an address on an inactive bridge port', () => { + const snap = healthy({ + addresses: [ + { '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }, + { '.id': '*2', address: '10.0.0.1/24', interface: 'ether1', disabled: 'false' }, + ], + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '1', disabled: 'false', inactive: 'true' }, + ], + }); + expect(rules(snap)).not.toContain('ip-on-bridge-port'); + }); + + it('does not flag a disabled address', () => { + const snap = healthy({ + addresses: [ + { '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }, + { '.id': '*2', address: '10.0.0.1/24', interface: 'ether1', disabled: 'true' }, + ], + }); + expect(rules(snap)).not.toContain('ip-on-bridge-port'); + }); + }); + + describe('vlan-iface-on-bridge-port', () => { + it('flags a VLAN interface whose parent is a bridge port', () => { + const snap = healthy({ + vlanInterfaces: [{ name: 'mgmt-vlan', interface: 'ether1', 'vlan-id': '99', disabled: 'false' }], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'vlan-iface-on-bridge-port'); + expect(found?.severity).toBe('critical'); + expect(found?.objects).toEqual(expect.arrayContaining(['mgmt-vlan', 'ether1'])); + }); + + it('does not flag a VLAN interface built on the bridge itself', () => { + const snap = healthy({ + vlanInterfaces: [{ name: 'vlan10', interface: 'bridge', 'vlan-id': '10', disabled: 'false' }], + }); + expect(rules(snap)).not.toContain('vlan-iface-on-bridge-port'); + }); + }); + + describe('bridged-vlan-interface', () => { + it('flags a VLAN interface added as a bridge port', () => { + const snap = healthy({ + vlanInterfaces: [{ name: 'vlan10', interface: 'bridge', 'vlan-id': '10', disabled: 'false' }], + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'vlan10', bridge: 'bridge', pvid: '1', disabled: 'false' }, + ], + }); + expect(rules(snap)).toContain('bridged-vlan-interface'); + }); + }); + + describe('bond-slave-is-bridge-port', () => { + it('flags a bond slave that is also a bridge port', () => { + const snap = healthy({ + bonds: [{ name: 'bond1', slaves: 'ether1,ether2', disabled: 'false' }], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'bond-slave-is-bridge-port'); + expect(found?.severity).toBe('critical'); + expect(found?.objects).toEqual(expect.arrayContaining(['ether1', 'bond1'])); + }); + + it('does not flag a bond whose slaves are outside the bridge', () => { + const snap = healthy({ + bonds: [{ name: 'bond1', slaves: 'ether7,ether8', disabled: 'false' }], + }); + expect(rules(snap)).not.toContain('bond-slave-is-bridge-port'); + }); + }); + + describe('mtu-exceeds-l2mtu', () => { + it('flags an MTU above the L2MTU', () => { + const snap = healthy({ + interfaces: [ + { name: 'bridge', type: 'bridge', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + { name: 'sfp28-1', type: 'ether', disabled: 'false', mtu: '9000', l2mtu: '1596' }, + { name: 'ether1', type: 'ether', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'mtu-exceeds-l2mtu'); + expect(found?.severity).toBe('warning'); + expect(found?.objects).toContain('sfp28-1'); + }); + + it('does not flag when L2MTU is absent', () => { + const snap = healthy({ + interfaces: [{ name: 'bridge', type: 'bridge', disabled: 'false', mtu: '9000' }], + }); + expect(rules(snap)).not.toContain('mtu-exceeds-l2mtu'); + }); + }); + + describe('multi-vid-untagged', () => { + it('flags a multi-VLAN entry that lists untagged ports', () => { + const snap = healthy({ + bridgeVlans: [ + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'bridge,sfp28-1,ether1', + 'current-tagged': '', 'current-untagged': 'bridge,sfp28-1,ether1', dynamic: 'false' }, + { bridge: 'bridge', 'vlan-ids': '20-25', tagged: 'sfp28-1', untagged: 'ether1', + 'current-tagged': 'sfp28-1', 'current-untagged': 'ether1', dynamic: 'false' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'multi-vid-untagged'); + expect(found?.severity).toBe('warning'); + expect(found?.detail).toContain('6 VLANs'); + }); + + it('does not flag a multi-VLAN entry that is tagged-only', () => { + const snap = healthy({ + bridgeVlans: [ + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'bridge,sfp28-1,ether1', + 'current-tagged': '', 'current-untagged': 'bridge,sfp28-1,ether1', dynamic: 'false' }, + { bridge: 'bridge', 'vlan-ids': '20-25', tagged: 'sfp28-1', untagged: '', + 'current-tagged': 'sfp28-1', 'current-untagged': '', dynamic: 'false' }, + ], + }); + expect(rules(snap)).not.toContain('multi-vid-untagged'); + }); + }); + + describe('pvid-with-tagged-only-frame-type', () => { + it('flags a PVID that the frame-type makes inert', () => { + const snap = healthy({ + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '20', disabled: 'false', + 'frame-types': 'admit-only-vlan-tagged' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'pvid-with-tagged-only-frame-type'); + expect(found?.severity).toBe('warning'); + expect(found?.objects).toContain('ether1'); + }); + + it('does not flag a tagged-only port left at the default PVID', () => { + const snap = healthy({ + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '1', disabled: 'false', + 'frame-types': 'admit-only-vlan-tagged' }, + ], + }); + expect(rules(snap)).not.toContain('pvid-with-tagged-only-frame-type'); + }); + }); + + describe('multiple-hw-bridges', () => { + it('flags two bridges both requesting hardware offload', () => { + const snap = healthy({ + bridges: [ + { name: 'bridge', 'vlan-filtering': 'true', pvid: '1' }, + { name: 'bridge2', 'vlan-filtering': 'false', pvid: '1' }, + ], + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether5', bridge: 'bridge2', pvid: '1', disabled: 'false', hw: 'true' }, + ], + }); + expect(rules(snap)).toContain('multiple-hw-bridges'); + }); + }); + + describe('vlan-iface-not-tagged-on-bridge', () => { + it('flags a VLAN interface whose VLAN the bridge is not tagged in', () => { + const snap = healthy({ + vlanInterfaces: [{ name: 'vlan30', interface: 'bridge', 'vlan-id': '30', disabled: 'false' }], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'vlan-iface-not-tagged-on-bridge'); + expect(found?.severity).toBe('critical'); + expect(found?.detail).toContain('no bridge VLAN entry for VLAN 30'); + }); + + it('does not flag when the bridge is a tagged member', () => { + const snap = healthy({ + vlanInterfaces: [{ name: 'vlan10', interface: 'bridge', 'vlan-id': '10', disabled: 'false' }], + }); + expect(rules(snap)).not.toContain('vlan-iface-not-tagged-on-bridge'); + }); + + it('does not flag when the bridge has VLAN filtering off', () => { + const snap = healthy({ + bridges: [{ name: 'bridge', 'vlan-filtering': 'false', pvid: '1' }], + vlanInterfaces: [{ name: 'vlan30', interface: 'bridge', 'vlan-id': '30', disabled: 'false' }], + }); + expect(rules(snap)).not.toContain('vlan-iface-not-tagged-on-bridge'); + }); + }); + + describe('mgmt-vlan-dynamic-only', () => { + it('flags management that survives only on a dynamic entry', () => { + const snap = healthy({ + bridgeVlans: [ + // The PVID-derived row: RouterOS marks it dynamic, and it is the only + // thing putting the bridge in VLAN 1. + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: '', + 'current-tagged': '', 'current-untagged': 'bridge,sfp28-1,ether1', dynamic: 'true' }, + { bridge: 'bridge', 'vlan-ids': '10', tagged: 'sfp28-1,bridge', untagged: '', + 'current-tagged': 'sfp28-1,bridge', 'current-untagged': '', dynamic: 'false' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'mgmt-vlan-dynamic-only'); + expect(found?.severity).toBe('warning'); + expect(found?.objects).toEqual(expect.arrayContaining(['bridge', '1'])); + }); + + it('does not flag management backed by a static entry', () => { + expect(rules(healthy())).not.toContain('mgmt-vlan-dynamic-only'); + }); + }); + + describe('port-in-no-vlan', () => { + it('flags an enabled port that belongs to no VLAN', () => { + const snap = healthy({ + bridgePorts: [ + { interface: 'sfp28-1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether1', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + { interface: 'ether9', bridge: 'bridge', pvid: '1', disabled: 'false', hw: 'true' }, + ], + }); + const found = auditConfig(snap, device).find((f) => f.rule === 'port-in-no-vlan'); + expect(found?.objects).toContain('ether9'); + }); + + it('does not flag ports on a bridge without VLAN filtering', () => { + const snap = healthy({ + bridges: [{ name: 'bridge', 'vlan-filtering': 'false', pvid: '1' }], + bridgeVlans: [], + }); + expect(rules(snap)).not.toContain('port-in-no-vlan'); + }); + }); + + describe('duplicate-address', () => { + it('flags the same address on two interfaces', () => { + // On a routed interface outside the bridge, so only this rule can fire. + const snap = healthy({ + addresses: [ + { '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }, + { '.id': '*2', address: '192.168.0.40/24', interface: 'ether9', disabled: 'false' }, + ], + }); + expect(rules(snap)).toEqual(['duplicate-address']); + }); + }); + + it('orders critical findings before warnings', () => { + const snap = healthy({ + addresses: [ + { '.id': '*1', address: '192.168.0.40/24', interface: 'bridge', disabled: 'false' }, + { '.id': '*2', address: '10.0.0.1/24', interface: 'ether1', disabled: 'false' }, + ], + interfaces: [ + { name: 'bridge', type: 'bridge', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + { name: 'sfp28-1', type: 'ether', disabled: 'false', mtu: '9000', l2mtu: '1596' }, + { name: 'ether1', type: 'ether', disabled: 'false', mtu: '1500', l2mtu: '1596' }, + ], + }); + const found = auditConfig(snap, device); + expect(found[0].severity).toBe('critical'); + expect(found[found.length - 1].severity).toBe('warning'); + }); +}); diff --git a/backend/src/services/changeGuard/configHealth.ts b/backend/src/services/changeGuard/configHealth.ts new file mode 100644 index 0000000..e449df8 --- /dev/null +++ b/backend/src/services/changeGuard/configHealth.ts @@ -0,0 +1,571 @@ +/** + * Config Health — a standing audit for configurations RouterOS accepts but that + * do not work. + * + * RouterOS applies each command immediately and independently, with no transaction + * and no cross-object validation. Contradictory state is accepted and simply + * misbehaves: an address on a bridge slave port is silently unreachable, a VLAN + * interface on a slave port takes the device off the network, a PVID next to + * `frame-type=admit-only-vlan-tagged` does nothing at all. MikroTik documents these + * on its Layer2 misconfiguration page — as prose. This module encodes them. + * + * Change Guard answers "will this change break reachability?". This answers the + * other half: "what is already wrong, and has been for a while?" — including + * configurations that work today only because a dynamic entry happens to exist. + * + * `auditConfig` is pure over a snapshot so every rule is testable against fixtures; + * persistence lives in `runConfigHealth`. + */ +import { query } from '../../config/database'; +import { expandVlanIds, rosList } from '../../utils/vlan'; +import { + captureSnapshot, + resolveManagementPath, + vlanMembership, + type DeviceSnapshot, + type RosRow, +} from './pathModel'; +import type { GuardDevice } from './ChangeGuard'; + +export type FindingSeverity = 'critical' | 'warning' | 'info'; + +export interface ConfigFinding { + /** Stable rule identifier, e.g. 'ip-on-bridge-port'. */ + rule: string; + /** Identifies this specific problem instance, so it can be tracked over time. */ + fingerprint: string; + severity: FindingSeverity; + title: string; + /** What is wrong and what it actually does to the network. */ + detail: string; + /** The concrete fix. */ + remediation: string; + docUrl: string; + /** Device objects involved — interface names, VLAN ids, addresses. */ + objects: string[]; +} + +const L2_DOC = 'https://help.mikrotik.com/docs/spaces/ROS/pages/19136718/Layer2+misconfiguration'; +const VLAN_TABLE_DOC = 'https://help.mikrotik.com/docs/spaces/ROS/pages/28606465/Bridge+VLAN+Table'; +const BRIDGING_DOC = 'https://help.mikrotik.com/docs/spaces/ROS/pages/328068/Bridging+and+Switching'; + +const isTrue = (v: string | undefined): boolean => v === 'true' || v === 'yes'; +const stripCidr = (addr: string): string => (addr || '').split('/')[0].trim(); + +/** + * Bridge ports that are actually carrying traffic. A disabled or inactive port is + * not slaved in practice, so configurations that would conflict with it do work — + * flagging them would be a false positive. + */ +function activeBridgePorts(snap: DeviceSnapshot): Map { + const map = new Map(); + for (const p of snap.bridgePorts) { + const name = p['interface']; + if (!name) continue; + if (isTrue(p['disabled']) || isTrue(p['inactive'])) continue; + map.set(name, p); + } + return map; +} + +// ─── rules ──────────────────────────────────────────────────────────────────── + +/** + * An IP address on a bridge slave port. + * + * The naive form of this check is wrong on RouterOS 7, and real hardware proved it: + * a `defconf` address configured on a slave port is reported with `slave=true` and + * `actual-interface=`, because RouterOS quietly relocates it to the bridge + * and it works perfectly. Calling that "unreachable" would be a false positive on a + * very common default configuration. + * + * So the severity follows what RouterOS actually did: + * - relocated to the bridge → informational; the address works, but the + * configuration names an interface that is not the one serving it + * - not relocated → critical; the port hands frames to the bridge and + * nothing terminates the address + */ +function ruleIpOnBridgePort(snap: DeviceSnapshot): ConfigFinding[] { + const ports = activeBridgePorts(snap); + const bridgeNames = new Set(snap.bridges.map((b) => b['name']).filter(Boolean)); + const out: ConfigFinding[] = []; + + for (const a of snap.addresses) { + if (isTrue(a['disabled']) || isTrue(a['invalid'])) continue; + const configured = a['interface'] || ''; + const actual = a['actual-interface'] || configured; + + // Only interesting when the address was configured on a port that is slaved to + // a bridge; an address configured directly on the bridge is entirely normal. + const port = ports.get(configured); + if (!port || bridgeNames.has(configured)) continue; + + const relocated = actual !== configured && bridgeNames.has(actual); + if (relocated) { + out.push({ + rule: 'ip-on-bridge-port', + fingerprint: `ip-on-bridge-port:${configured}:${a['address']}`, + severity: 'info', + title: `${a['address']} is configured on ${configured} but served by ${actual}`, + detail: + `${configured} is a member port of bridge ${actual}, so RouterOS moved the address to ` + + `the bridge — it works, but the configuration names a port that is not the one ` + + `answering. Anything reasoning about this address from the config alone, including ` + + `firewall rules scoped to ${configured}, will not match where the traffic really is.`, + remediation: + `Move ${a['address']} onto ${actual} directly so the configuration says what is ` + + `actually happening.`, + docUrl: L2_DOC, + objects: [configured, actual, a['address']], + }); + continue; + } + + out.push({ + rule: 'ip-on-bridge-port', + fingerprint: `ip-on-bridge-port:${configured}:${a['address']}`, + severity: 'critical', + title: `${a['address']} is unreachable — it sits on bridge port ${configured}`, + detail: + `${configured} is a member port of bridge ${port['bridge']}, so it forwards every frame ` + + `into the bridge instead of terminating it locally, and RouterOS has not relocated the ` + + `address to the bridge. ${a['address']} is configured but nothing can reach it.`, + remediation: + `Move the address onto ${port['bridge']} itself (or onto a VLAN interface on that bridge), ` + + `or remove ${configured} from the bridge if it is meant to be routed.`, + docUrl: L2_DOC, + objects: [configured, a['address']], + }); + } + return out; +} + +/** + * A VLAN interface whose parent is a bridge slave port. MikroTik's own wording for + * this case is that the device becomes unreachable: the parent port passes frames to + * the bridge, so the VLAN interface never sees them. + */ +function ruleVlanIfaceOnBridgePort(snap: DeviceSnapshot): ConfigFinding[] { + const ports = activeBridgePorts(snap); + const out: ConfigFinding[] = []; + for (const v of snap.vlanInterfaces) { + if (isTrue(v['disabled'])) continue; + const parent = v['interface'] || ''; + const port = ports.get(parent); + if (!port) continue; + out.push({ + rule: 'vlan-iface-on-bridge-port', + fingerprint: `vlan-iface-on-bridge-port:${v['name']}`, + severity: 'critical', + title: `VLAN interface ${v['name']} is built on bridge port ${parent}`, + detail: + `${parent} is a member of bridge ${port['bridge']}, so tagged frames are handed to the ` + + `bridge rather than to ${v['name']}. The VLAN interface receives nothing, and anything ` + + `depending on it — including management — is unreachable.`, + remediation: + `Create the VLAN interface on bridge ${port['bridge']} instead of on ${parent}, and let the ` + + `bridge VLAN table decide which ports carry VLAN ${v['vlan-id']}.`, + docUrl: L2_DOC, + objects: [v['name'], parent], + }); + } + return out; +} + +/** + * A VLAN interface added as a bridge port. Documented as causing loops and RSTP + * flapping, because the tagged traffic re-enters the same broadcast domain. + */ +function ruleBridgedVlanInterface(snap: DeviceSnapshot): ConfigFinding[] { + const vlanNames = new Set(snap.vlanInterfaces.map((v) => v['name']).filter(Boolean)); + const out: ConfigFinding[] = []; + for (const p of snap.bridgePorts) { + const name = p['interface']; + if (!name || !vlanNames.has(name)) continue; + if (isTrue(p['disabled'])) continue; + out.push({ + rule: 'bridged-vlan-interface', + fingerprint: `bridged-vlan-interface:${p['bridge']}:${name}`, + severity: 'critical', + title: `VLAN interface ${name} is a port of bridge ${p['bridge']}`, + detail: + `Adding a VLAN interface as a bridge port feeds tagged traffic back into the same ` + + `broadcast domain it came from. This creates a forwarding loop, and where STP is ` + + `running it shows up as ports flapping between forwarding and blocking.`, + remediation: + `Remove ${name} from bridge ${p['bridge']} and use the bridge VLAN table to carry the ` + + `VLAN on the physical ports instead.`, + docUrl: L2_DOC, + objects: [name, p['bridge']], + }); + } + return out; +} + +/** + * A bonding slave that is also directly a bridge port. The bond and the bridge both + * claim the interface; traffic takes an undefined path and the bond loses members + * without saying so. + */ +function ruleBondSlaveIsBridgePort(snap: DeviceSnapshot): ConfigFinding[] { + const ports = activeBridgePorts(snap); + const out: ConfigFinding[] = []; + for (const bond of snap.bonds) { + if (isTrue(bond['disabled'])) continue; + for (const slave of rosList(bond['slaves'])) { + const port = ports.get(slave); + if (!port) continue; + out.push({ + rule: 'bond-slave-is-bridge-port', + fingerprint: `bond-slave-is-bridge-port:${bond['name']}:${slave}`, + severity: 'critical', + title: `${slave} is both a slave of bond ${bond['name']} and a port of bridge ${port['bridge']}`, + detail: + `An interface can belong to a bond or to a bridge, not both. With both configured the ` + + `forwarding path is undefined: frames may bypass the bond, and the bond silently runs ` + + `with fewer working members than it reports.`, + remediation: + `Remove ${slave} from bridge ${port['bridge']} and add the bond ${bond['name']} as the ` + + `bridge port instead.`, + docUrl: BRIDGING_DOC, + objects: [slave, bond['name'], port['bridge']], + }); + } + } + return out; +} + +/** + * MTU above L2MTU. The interface accepts the larger MTU and then cannot carry it; + * small packets pass and large ones vanish, which is why this usually surfaces as + * "the network works but file transfers hang". + */ +function ruleMtuExceedsL2Mtu(snap: DeviceSnapshot): ConfigFinding[] { + const out: ConfigFinding[] = []; + for (const i of snap.interfaces) { + if (isTrue(i['disabled'])) continue; + const mtu = parseInt(i['mtu'] || '', 10); + const l2mtu = parseInt(i['l2mtu'] || '', 10); + if (isNaN(mtu) || isNaN(l2mtu) || l2mtu <= 0) continue; + if (mtu <= l2mtu) continue; + out.push({ + rule: 'mtu-exceeds-l2mtu', + fingerprint: `mtu-exceeds-l2mtu:${i['name']}`, + severity: 'warning', + title: `${i['name']} has MTU ${mtu} above its L2MTU of ${l2mtu}`, + detail: + `The interface is configured to carry ${mtu}-byte payloads but the underlying layer-2 ` + + `path only supports ${l2mtu}. Frames above the L2MTU are dropped without an error, so ` + + `small traffic works while large transfers stall.`, + remediation: + `Lower the MTU on ${i['name']} to ${l2mtu} or below, or raise the L2MTU on this interface ` + + `and every interface along the path.`, + docUrl: L2_DOC, + objects: [i['name']], + }); + } + return out; +} + +/** + * A bridge VLAN entry covering several VLANs while also listing untagged ports. + * RouterOS prints a warning for this and applies it anyway: every listed VLAN + * becomes untagged on those ports, which is almost never what was intended. + */ +function ruleMultiVidUntagged(snap: DeviceSnapshot): ConfigFinding[] { + const out: ConfigFinding[] = []; + for (const row of snap.bridgeVlans) { + if (isTrue(row['dynamic'])) continue; + const vids = expandVlanIds(row['vlan-ids']); + if (vids.length < 2) continue; + const untagged = rosList(row['untagged']); + if (untagged.length === 0) continue; + out.push({ + rule: 'multi-vid-untagged', + fingerprint: `multi-vid-untagged:${row['bridge']}:${row['vlan-ids']}`, + severity: 'warning', + title: `Bridge VLAN entry ${row['vlan-ids']} on ${row['bridge']} has untagged ports`, + detail: + `This one entry covers ${vids.length} VLANs and marks ${untagged.join(', ')} untagged in ` + + `all of them. A port can only meaningfully be untagged in one VLAN, so the result is ` + + `ambiguous. RouterOS warns about this and applies it regardless.`, + remediation: + `Split the entry: keep the range for tagged trunk ports, and give each untagged access ` + + `port its own single-VLAN entry with a matching PVID.`, + docUrl: VLAN_TABLE_DOC, + objects: [row['bridge'], row['vlan-ids'], ...untagged], + }); + } + return out; +} + +/** + * A PVID next to `frame-type=admit-only-vlan-tagged`. The docs are explicit that + * this frame-type disables the dynamic untagged entry that a PVID would create, so + * the PVID is inert — the classic "configuration that looks like it does something". + */ +function rulePvidWithTaggedOnlyFrameType(snap: DeviceSnapshot): ConfigFinding[] { + const out: ConfigFinding[] = []; + for (const p of snap.bridgePorts) { + if (isTrue(p['disabled'])) continue; + if (p['frame-types'] !== 'admit-only-vlan-tagged' && p['frame-type'] !== 'admit-only-vlan-tagged') continue; + const pvid = parseInt(p['pvid'] || '1', 10); + if (isNaN(pvid) || pvid === 1) continue; + out.push({ + rule: 'pvid-with-tagged-only-frame-type', + fingerprint: `pvid-with-tagged-only-frame-type:${p['bridge']}:${p['interface']}`, + severity: 'warning', + title: `PVID ${pvid} on ${p['interface']} has no effect`, + detail: + `${p['interface']} admits only VLAN-tagged frames, which disables the dynamic untagged ` + + `entry a PVID would otherwise create. The PVID of ${pvid} is stored but never applied, ` + + `so untagged devices on this port are dropped rather than placed in VLAN ${pvid}.`, + remediation: + `Either set frame-types back to admit-all (or admit-only-untagged-and-priority-tagged) if ` + + `untagged traffic belongs here, or clear the PVID so the configuration says what it does.`, + docUrl: VLAN_TABLE_DOC, + objects: [p['interface'], p['bridge'], String(pvid)], + }); + } + return out; +} + +/** + * Hardware offload can only be applied to one bridge per switch chip. When a second + * bridge also has hw ports, one of them silently falls back to CPU forwarding and + * throughput collapses under load without any error. + */ +function ruleMultipleHwBridges(snap: DeviceSnapshot): ConfigFinding[] { + const hwBridges = new Set(); + for (const p of snap.bridgePorts) { + if (isTrue(p['disabled'])) continue; + if (isTrue(p['hw'])) hwBridges.add(p['bridge']); + } + if (hwBridges.size < 2) return []; + const names = [...hwBridges].sort(); + return [{ + rule: 'multiple-hw-bridges', + fingerprint: `multiple-hw-bridges:${names.join(',')}`, + severity: 'warning', + title: `${names.length} bridges request hardware offload (${names.join(', ')})`, + detail: + `A switch chip can hardware-offload the ports of only one bridge. With several bridges ` + + `asking for it, the ones that lose out fall back to CPU forwarding — no error is raised, ` + + `but throughput on those ports drops to what the CPU can handle.`, + remediation: + `Consolidate the ports onto a single hardware-offloaded bridge and separate the networks ` + + `with the bridge VLAN table, or accept CPU forwarding and disable hw on the extra bridges.`, + docUrl: BRIDGING_DOC, + objects: names, + }]; +} + +/** + * A VLAN interface on a VLAN-filtering bridge whose VLAN the bridge is not tagged + * in. The bridge is the CPU port: without tagged membership the VLAN interface is + * up and carries nothing. + */ +function ruleVlanIfaceNotTaggedOnBridge(snap: DeviceSnapshot): ConfigFinding[] { + const out: ConfigFinding[] = []; + const filtering = new Set( + snap.bridges.filter((b) => isTrue(b['vlan-filtering'])).map((b) => b['name']) + ); + for (const v of snap.vlanInterfaces) { + if (isTrue(v['disabled'])) continue; + const parent = v['interface'] || ''; + if (!filtering.has(parent)) continue; + const vid = parseInt(v['vlan-id'] || '', 10); + if (isNaN(vid)) continue; + const membership = vlanMembership(snap, parent, vid); + if (membership.tagged.includes(parent)) continue; + out.push({ + rule: 'vlan-iface-not-tagged-on-bridge', + fingerprint: `vlan-iface-not-tagged-on-bridge:${v['name']}`, + severity: 'critical', + title: `${v['name']} carries no traffic — ${parent} is not tagged in VLAN ${vid}`, + detail: + `${parent} has VLAN filtering enabled, and the bridge itself is the CPU port. For ` + + `${v['name']} to receive VLAN ${vid}, ${parent} must be listed as a tagged member of ` + + `VLAN ${vid} in the bridge VLAN table. ` + + (membership.found + ? `The VLAN exists on the bridge but ${parent} is not tagged in it.` + : `There is no bridge VLAN entry for VLAN ${vid} at all.`), + remediation: + `Add ${parent} to the tagged list of VLAN ${vid} on bridge ${parent} in the bridge VLAN table.`, + docUrl: VLAN_TABLE_DOC, + objects: [v['name'], parent, String(vid)], + }); + } + return out; +} + +/** + * The management VLAN membership that keeps the manager connected exists only as a + * dynamic entry created by a port PVID. It works right now, but any change that + * stops PVIDs creating dynamic entries — setting frame-types to tagged-only being + * the documented one — removes it and takes the device off the network. + */ +function ruleMgmtVlanDynamicOnly(snap: DeviceSnapshot, device: GuardDevice): ConfigFinding[] { + const path = resolveManagementPath(snap, device); + if (!path.bridge || !path.bridgeVlanFiltering || path.mgmtVlanId === null) return []; + if (path.taggedManagement) return []; + + const vid = path.mgmtVlanId; + const membership = vlanMembership(snap, path.bridge, vid); + const effective = membership.untagged.includes(path.bridge) || membership.tagged.includes(path.bridge); + const isStatic = membership.untaggedStatic.includes(path.bridge) || membership.taggedStatic.includes(path.bridge); + if (!effective || isStatic) return []; + + return [{ + rule: 'mgmt-vlan-dynamic-only', + fingerprint: `mgmt-vlan-dynamic-only:${path.bridge}:${vid}`, + severity: 'warning', + title: `Management on VLAN ${vid} depends on a dynamic bridge VLAN entry`, + detail: + `Bridge ${path.bridge} carries management on VLAN ${vid}, but its membership in that VLAN ` + + `is not configured — it exists only as a dynamic entry derived from a port PVID. That ` + + `entry disappears the moment a port is set to admit only tagged frames, and management ` + + `goes with it. Nothing in the saved configuration records why this device is reachable.`, + remediation: + `Add ${path.bridge} explicitly to VLAN ${vid} in the bridge VLAN table (untagged, matching ` + + `how management arrives today) so the reachability is written down rather than incidental.`, + docUrl: VLAN_TABLE_DOC, + objects: [path.bridge, String(vid)], + }]; +} + +/** + * A port on a VLAN-filtering bridge that belongs to no VLAN. Every frame entering it + * is dropped by ingress filtering — the port is up, linked, and completely dead. + */ +function rulePortInNoVlan(snap: DeviceSnapshot): ConfigFinding[] { + const filtering = new Set( + snap.bridges.filter((b) => isTrue(b['vlan-filtering'])).map((b) => b['name']) + ); + if (filtering.size === 0) return []; + + const member = new Set(); + for (const row of snap.bridgeVlans) { + for (const p of rosList(row['current-tagged'] ?? row['tagged'])) member.add(`${row['bridge']}:${p}`); + for (const p of rosList(row['current-untagged'] ?? row['untagged'])) member.add(`${row['bridge']}:${p}`); + } + + const out: ConfigFinding[] = []; + for (const p of snap.bridgePorts) { + const name = p['interface']; + const bridge = p['bridge']; + if (!name || !bridge || !filtering.has(bridge)) continue; + if (isTrue(p['disabled']) || isTrue(p['inactive'])) continue; + if (member.has(`${bridge}:${name}`)) continue; + out.push({ + rule: 'port-in-no-vlan', + fingerprint: `port-in-no-vlan:${bridge}:${name}`, + severity: 'warning', + title: `${name} is a member of no VLAN on VLAN-filtering bridge ${bridge}`, + detail: + `${bridge} enforces the VLAN table on ingress, and ${name} appears in no entry — not ` + + `tagged, not untagged, and not via a PVID-derived dynamic entry. Anything plugged into ` + + `this port links up and is then dropped silently.`, + remediation: + `Either add ${name} to the VLANs it should carry, or remove it from ${bridge} so the ` + + `configuration reflects that it is unused.`, + docUrl: VLAN_TABLE_DOC, + objects: [name, bridge], + }); + } + return out; +} + +/** + * Two interfaces holding the same address. Only one can answer; which one wins is + * not something the configuration states. + */ +function ruleDuplicateAddress(snap: DeviceSnapshot): ConfigFinding[] { + const byAddr = new Map(); + for (const a of snap.addresses) { + if (isTrue(a['disabled']) || isTrue(a['invalid'])) continue; + const ip = stripCidr(a['address']); + if (!ip) continue; + const iface = a['actual-interface'] || a['interface'] || '?'; + byAddr.set(ip, [...(byAddr.get(ip) ?? []), iface]); + } + const out: ConfigFinding[] = []; + for (const [ip, ifaces] of byAddr) { + if (ifaces.length < 2) continue; + out.push({ + rule: 'duplicate-address', + fingerprint: `duplicate-address:${ip}`, + severity: 'warning', + title: `${ip} is configured on ${ifaces.length} interfaces`, + detail: + `${ip} appears on ${ifaces.join(', ')}. Only one of them can answer for it, and which one ` + + `depends on internal ordering rather than on anything in the configuration.`, + remediation: `Remove the address from every interface except the one that should own it.`, + docUrl: L2_DOC, + objects: [ip, ...ifaces], + }); + } + return out; +} + +// ─── audit ──────────────────────────────────────────────────────────────────── + +const SEVERITY_RANK: Record = { critical: 0, warning: 1, info: 2 }; + +/** Run every rule against a snapshot. Pure — no device or database access. */ +export function auditConfig(snap: DeviceSnapshot, device: GuardDevice): ConfigFinding[] { + const findings = [ + ...ruleIpOnBridgePort(snap), + ...ruleVlanIfaceOnBridgePort(snap), + ...ruleBridgedVlanInterface(snap), + ...ruleBondSlaveIsBridgePort(snap), + ...ruleVlanIfaceNotTaggedOnBridge(snap), + ...ruleMgmtVlanDynamicOnly(snap, device), + ...rulePortInNoVlan(snap), + ...ruleMultiVidUntagged(snap), + ...rulePvidWithTaggedOnlyFrameType(snap), + ...ruleMultipleHwBridges(snap), + ...ruleMtuExceedsL2Mtu(snap), + ...ruleDuplicateAddress(snap), + ]; + return findings.sort( + (a, b) => SEVERITY_RANK[a.severity] - SEVERITY_RANK[b.severity] || a.rule.localeCompare(b.rule) + ); +} + +export interface ConfigHealthResult { + findings: ConfigFinding[]; + checkedAt: Date; +} + +/** + * Audit a device and persist the result. Findings are upserted so `first_seen` + * records how long a problem has existed, and anything absent from this run is + * removed — the table always describes the device as it is now. + */ +export async function runConfigHealth(device: GuardDevice): Promise { + const snap = await captureSnapshot(device); + const findings = auditConfig(snap, device); + const checkedAt = new Date(); + + for (const f of findings) { + await query( + `INSERT INTO device_config_findings + (device_id, rule, fingerprint, severity, title, detail, remediation, doc_url, objects, last_seen) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,NOW()) + ON CONFLICT (device_id, fingerprint) DO UPDATE SET + rule=$2, severity=$4, title=$5, detail=$6, remediation=$7, doc_url=$8, objects=$9, last_seen=NOW()`, + [device.id, f.rule, f.fingerprint, f.severity, f.title, f.detail, f.remediation, f.docUrl, f.objects] + ); + } + + await query( + `DELETE FROM device_config_findings + WHERE device_id = $1 AND NOT (fingerprint = ANY($2::text[]))`, + [device.id, findings.map((f) => f.fingerprint)] + ); + + await query(`UPDATE devices SET config_health_checked_at = NOW() WHERE id = $1`, [device.id]); + + return { findings, checkedAt }; +} diff --git a/backend/src/services/changeGuard/pathModel.ts b/backend/src/services/changeGuard/pathModel.ts index 915712e..d0236ba 100644 --- a/backend/src/services/changeGuard/pathModel.ts +++ b/backend/src/services/changeGuard/pathModel.ts @@ -21,8 +21,13 @@ */ import { RouterOSClient } from '../mikrotik/RouterOSClient'; import { decrypt } from '../../utils/crypto'; +import { expandVlanIds } from '../../utils/vlan'; import type { GuardDevice } from './ChangeGuard'; +// Re-exported so the analysis modules keep importing VLAN handling from the path +// model they reason about, while the collector shares the same implementation. +export { expandVlanIds }; + export type RosRow = Record; export interface DeviceSnapshot { @@ -32,6 +37,8 @@ export interface DeviceSnapshot { bridges: RosRow[]; bridgePorts: RosRow[]; bridgeVlans: RosRow[]; + /** Bonding interfaces with their slave lists; used by the config audit. */ + bonds: RosRow[]; routes: RosRow[]; arp: RosRow[]; bridgeHosts: RosRow[]; @@ -100,7 +107,7 @@ export async function captureSnapshot(device: GuardDevice): Promise v === 'true' || v === 'yes'; const csv = (v: string | undefined): string[] => (v || '').split(',').map((s) => s.trim()).filter(Boolean); -/** RouterOS vlan-ids may be a list and/or ranges: "10", "10,20", "10-12". */ -export function expandVlanIds(spec: string | undefined): number[] { - const out: number[] = []; - for (const part of csv(spec)) { - const range = part.match(/^(\d+)\s*-\s*(\d+)$/); - if (range) { - const from = parseInt(range[1], 10); - const to = parseInt(range[2], 10); - if (!isNaN(from) && !isNaN(to) && to >= from) { - for (let v = from; v <= to; v++) out.push(v); - } - continue; - } - const n = parseInt(part, 10); - if (!isNaN(n)) out.push(n); - } - return out; -} - /** * Effective membership for a VLAN on a bridge. RouterOS exposes `current-tagged` * / `current-untagged`, which include dynamically-added ports (notably the PVID- diff --git a/backend/src/services/mikrotik/DeviceCollector.ts b/backend/src/services/mikrotik/DeviceCollector.ts index 8e5f9ee..74724cf 100644 --- a/backend/src/services/mikrotik/DeviceCollector.ts +++ b/backend/src/services/mikrotik/DeviceCollector.ts @@ -6,6 +6,7 @@ import { Point } from '@influxdata/influxdb-client'; import { decrypt } from '../../utils/crypto'; import { lookupVendor } from '../../utils/oui'; import { buildServerArpMap } from '../../utils/serverArp'; +import { aggregateBridgeVlans, portVlanMembership, expandVlanIds } from '../../utils/vlan'; import { BackupService } from '../BackupService'; import { alertService } from '../AlertService'; @@ -309,11 +310,18 @@ export class DeviceCollector { const mtu = !isNaN(rawMtu) && rawMtu > 0 ? rawMtu : null; try { + // Only bridges have vlan-filtering; null everywhere else keeps the column + // meaningful for queries rather than defaulting to a misleading false. + const vlanFiltering = resolvedType === 'bridge' + ? enrichedIface['vlan-filtering'] === 'true' + : null; + await query( - `INSERT INTO interfaces (device_id, name, type, mac_address, mtu, running, disabled, comment, speed, config_json, updated_at) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,NOW()) + `INSERT INTO interfaces (device_id, name, type, mac_address, mtu, running, disabled, comment, speed, vlan_filtering, config_json, updated_at) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,NOW()) ON CONFLICT (device_id, name) DO UPDATE SET - type=$3, mac_address=$4, mtu=$5, running=$6, disabled=$7, comment=$8, speed=$9, config_json=$10, updated_at=NOW()`, + type=$3, mac_address=$4, mtu=$5, running=$6, disabled=$7, comment=$8, speed=$9, + vlan_filtering=$10, config_json=$11, updated_at=NOW()`, [ this.device.id, name, @@ -324,6 +332,7 @@ export class DeviceCollector { enrichedIface['disabled'] === 'true', enrichedIface['comment'] || null, enrichedIface['speed'] || null, + vlanFiltering, JSON.stringify(enrichedIface), ] ); @@ -372,58 +381,106 @@ export class DeviceCollector { async collectVlans(): Promise { try { - // Bridge VLAN table (CRS switches / bridge-based switching) - const bridgeVlans = await this.client - .execute('/interface/bridge/vlan/print', { detail: '' }) - .catch(() => []); + // Bridge VLAN table (CRS switches / bridge-based switching). + // + // Both fetches are tracked for success separately: a stale cache beats a + // wrongly-empty one, so pruning only happens when we know the device + // actually reported an authoritative list. + let bridgeVlans: Record[] = []; + let bridgeVlansOk = true; + try { + bridgeVlans = await this.client.execute('/interface/bridge/vlan/print', { detail: '' }); + } catch { + bridgeVlansOk = false; + } - for (const vlan of bridgeVlans) { - const vlanId = parseInt(vlan['vlan-ids'] || '0', 10); - if (!vlanId) continue; + const agg = aggregateBridgeVlans(bridgeVlans); + for (const v of agg) { await query( - `INSERT INTO vlans (device_id, vlan_id, name, bridge, tagged_ports, untagged_ports, config_json) - VALUES ($1,$2,$3,$4,$5,$6,$7) - ON CONFLICT (device_id, vlan_id) DO UPDATE SET - name=$3, bridge=$4, tagged_ports=$5, untagged_ports=$6, config_json=$7`, + `INSERT INTO vlans (device_id, vlan_id, name, bridge, vlan_ids, tagged_ports, untagged_ports, config_json, updated_at) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,NOW()) + ON CONFLICT (device_id, bridge, vlan_id) DO UPDATE SET + name=$3, vlan_ids=$5, tagged_ports=$6, untagged_ports=$7, config_json=$8, updated_at=NOW()`, [ this.device.id, - vlanId, - vlan['comment'] || `VLAN ${vlanId}`, - vlan['bridge'] || null, - this.parseList(vlan['tagged']), - this.parseList(vlan['untagged']), - JSON.stringify(vlan), + v.vlanId, + v.name || `VLAN ${v.vlanId}`, + v.bridge, + v.spec, + v.tagged, + v.untagged, + JSON.stringify(v.rows.length === 1 ? v.rows[0] : v.rows), ] ); } + const seenVlans = agg.map((v) => ({ bridge: v.bridge, vlanId: v.vlanId })); - // Also collect bridge port PVIDs - const bridgePorts = await this.client - .execute('/interface/bridge/port/print', { detail: '' }) - .catch(() => []); + if (bridgeVlansOk) { + // Drop VLANs deleted on the device. Comparing composite keys needs two + // parallel arrays because Postgres has no tuple-array membership test. + await query( + `DELETE FROM vlans + WHERE device_id = $1 + AND NOT (bridge || ':' || vlan_id::text = ANY($2::text[]))`, + [this.device.id, seenVlans.map((v) => `${v.bridge}:${v.vlanId}`)] + ); + } + // Per-port membership, derived from the same VLAN table so the port editor + // can tell a trunk from an access port. + let bridgePorts: Record[] = []; + let bridgePortsOk = true; + try { + bridgePorts = await this.client.execute('/interface/bridge/port/print', { detail: '' }); + } catch { + bridgePortsOk = false; + } + + const membership = portVlanMembership(bridgeVlans); + + const seenPorts: string[] = []; for (const port of bridgePorts) { const pvid = parseInt(port['pvid'] || '1', 10); const bridge = port['bridge'] || ''; const portName = port['interface'] || ''; if (!portName || !bridge) continue; + seenPorts.push(`${bridge}:${portName}`); + + const m = membership.get(portName) ?? { tagged: [], untagged: [] }; + const taggedIds = m.tagged.map(String); + const untaggedIds = m.untagged.map(String); + const allIds = [...new Set([...m.tagged, ...m.untagged])].sort((a, b) => a - b).map(String); await query( - `INSERT INTO bridge_vlan_entries (device_id, bridge, port, pvid, tagged, config_json, updated_at) - VALUES ($1,$2,$3,$4,$5,$6,NOW()) + `INSERT INTO bridge_vlan_entries + (device_id, bridge, port, pvid, tagged, vlan_ids, tagged_vlan_ids, untagged_vlan_ids, config_json, updated_at) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,NOW()) ON CONFLICT (device_id, bridge, port) DO UPDATE SET - pvid=$4, tagged=$5, config_json=$6, updated_at=NOW()`, + pvid=$4, tagged=$5, vlan_ids=$6, tagged_vlan_ids=$7, untagged_vlan_ids=$8, + config_json=$9, updated_at=NOW()`, [ this.device.id, bridge, portName, pvid, - false, + taggedIds.length > 0, + allIds, + taggedIds, + untaggedIds, JSON.stringify(port), ] ); } + + if (bridgePortsOk) { + await query( + `DELETE FROM bridge_vlan_entries + WHERE device_id = $1 + AND NOT (bridge || ':' || port = ANY($2::text[]))`, + [this.device.id, seenPorts] + ); + } } catch (err) { console.error(`[${this.device.name}] Failed to collect VLANs:`, err); } @@ -2689,11 +2746,31 @@ export class DeviceCollector { } async removeBridgeVlan(bridge: string, vlanId: number): Promise { + // Matching is done here rather than with a `?vlan-ids=` server-side filter: + // that filter compares the raw string, so it misses a row written as a range + // or list ("10-20") that nonetheless contains this VLAN — and missing it would + // fail silently, leaving the VLAN in place while reporting success. const entries = await this.client - .execute('/interface/bridge/vlan/print', {}, [`?bridge=${bridge}`, `?vlan-ids=${vlanId}`]) + .execute('/interface/bridge/vlan/print', { detail: '' }, [`?bridge=${bridge}`]) .catch(() => []); - for (const entry of entries) { - const id = (entry as Record)['.id']; + + const matches = (entries as Record[]).filter((e) => + expandVlanIds(e['vlan-ids']).includes(vlanId) + ); + + const multi = matches.find((e) => expandVlanIds(e['vlan-ids']).length > 1); + if (multi) { + throw new Error( + `VLAN ${vlanId} on ${bridge} is part of the multi-VLAN entry "${multi['vlan-ids']}". ` + + `Removing it here would delete every VLAN in that entry, so it must be split on the device first.` + ); + } + + for (const entry of matches) { + // Dynamic rows cannot be removed; RouterOS rejects the attempt and the row + // reappears from the port PVID that created it. + if (entry['dynamic'] === 'true') continue; + const id = entry['.id']; if (id) await this.client.execute('/interface/bridge/vlan/remove', { numbers: id }).catch(() => {}); } } diff --git a/backend/src/utils/__tests__/vlan.test.ts b/backend/src/utils/__tests__/vlan.test.ts new file mode 100644 index 0000000..844d42d --- /dev/null +++ b/backend/src/utils/__tests__/vlan.test.ts @@ -0,0 +1,107 @@ +import { expandVlanIds, isMultiVlanSpec, aggregateBridgeVlans, portVlanMembership } from '../vlan'; + +describe('expandVlanIds', () => { + it('handles a single id, a list, and a range', () => { + expect(expandVlanIds('10')).toEqual([10]); + expect(expandVlanIds('10,20,30')).toEqual([10, 20, 30]); + expect(expandVlanIds('10-12')).toEqual([10, 11, 12]); + expect(expandVlanIds('1,10-12,20')).toEqual([1, 10, 11, 12, 20]); + }); + + it('ignores empty and malformed specs rather than inventing VLAN 0', () => { + expect(expandVlanIds(undefined)).toEqual([]); + expect(expandVlanIds('')).toEqual([]); + expect(expandVlanIds('abc')).toEqual([]); + expect(expandVlanIds('20-10')).toEqual([]); // inverted range + }); + + it('identifies multi-VLAN specs', () => { + expect(isMultiVlanSpec('10')).toBe(false); + expect(isMultiVlanSpec('')).toBe(false); + expect(isMultiVlanSpec('10-20')).toBe(true); + expect(isMultiVlanSpec('10,20')).toBe(true); + }); +}); + +describe('aggregateBridgeVlans', () => { + it('expands a range into one entry per VLAN, keeping the source spec', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge', 'vlan-ids': '10-12', tagged: 'sfp1', untagged: '', dynamic: 'false' }, + ]); + expect(out.map((v) => v.vlanId)).toEqual([10, 11, 12]); + expect(out.every((v) => v.spec === '10-12')).toBe(true); + expect(out.every((v) => v.tagged.length === 1 && v.tagged[0] === 'sfp1')).toBe(true); + }); + + it('keeps the same VLAN ID on two bridges apart', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge1', 'vlan-ids': '1', tagged: '', untagged: 'ether1', dynamic: 'false' }, + { bridge: 'bridge2', 'vlan-ids': '1', tagged: '', untagged: 'ether5', dynamic: 'false' }, + ]); + expect(out).toHaveLength(2); + expect(out.find((v) => v.bridge === 'bridge1')?.untagged).toEqual(['ether1']); + expect(out.find((v) => v.bridge === 'bridge2')?.untagged).toEqual(['ether5']); + }); + + /** + * The regression that broke VLAN editing: a dynamic row created by a port PVID + * sits alongside the operator's row for the same VLAN. Because updateBridgeVlan is + * remove-then-add, anything reported here is written back — so a dynamic port must + * never appear in the membership, or every save bakes it into the configuration. + */ + it('excludes dynamic rows from membership but still lists the VLAN', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'bridge', dynamic: 'true', + comment: 'added by pvid', 'current-untagged': 'bridge' }, + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'sfp28-2', dynamic: 'false' }, + ]); + expect(out).toHaveLength(1); + expect(out[0].untagged).toEqual(['sfp28-2']); // not ['bridge', 'sfp28-2'] + expect(out[0].rows).toHaveLength(2); // both kept for config_json + }); + + it('lists a VLAN that exists only dynamically, with no editable membership', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge', 'vlan-ids': '1', tagged: '', untagged: 'bridge', dynamic: 'true' }, + ]); + expect(out).toHaveLength(1); + expect(out[0].vlanId).toBe(1); + expect(out[0].untagged).toEqual([]); + expect(out[0].spec).toBe(''); + }); + + it('unions two static rows for one VLAN instead of letting the last one win', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge', 'vlan-ids': '20', tagged: 'sfp1', untagged: '', dynamic: 'false' }, + { bridge: 'bridge', 'vlan-ids': '20', tagged: 'sfp2', untagged: '', dynamic: 'false' }, + ]); + expect(out).toHaveLength(1); + expect(out[0].tagged.sort()).toEqual(['sfp1', 'sfp2']); + }); + + it('takes the name from a static row, never from a dynamic one', () => { + const out = aggregateBridgeVlans([ + { bridge: 'bridge', 'vlan-ids': '1', untagged: 'bridge', dynamic: 'true', comment: 'added by pvid' }, + { bridge: 'bridge', 'vlan-ids': '1', untagged: 'sfp28-2', dynamic: 'false', comment: 'Management' }, + ]); + expect(out[0].name).toBe('Management'); + }); +}); + +describe('portVlanMembership', () => { + it('maps each port to the VLANs it is statically tagged and untagged in', () => { + const m = portVlanMembership([ + { bridge: 'bridge', 'vlan-ids': '10,20', tagged: 'sfp1', untagged: 'ether1', dynamic: 'false' }, + { bridge: 'bridge', 'vlan-ids': '30', tagged: 'sfp1', untagged: '', dynamic: 'false' }, + ]); + expect(m.get('sfp1')).toEqual({ tagged: [10, 20, 30], untagged: [] }); + expect(m.get('ether1')).toEqual({ tagged: [], untagged: [10, 20] }); + }); + + it('ignores dynamic rows so the port editor does not write them back', () => { + const m = portVlanMembership([ + { bridge: 'bridge', 'vlan-ids': '1', untagged: 'bridge,ether1', dynamic: 'true' }, + ]); + expect(m.size).toBe(0); + }); +}); diff --git a/backend/src/utils/vlan.ts b/backend/src/utils/vlan.ts new file mode 100644 index 0000000..fb4d8e2 --- /dev/null +++ b/backend/src/utils/vlan.ts @@ -0,0 +1,132 @@ +/** + * RouterOS VLAN-ID parsing. + * + * A single `/interface/bridge/vlan` row can carry a list and/or ranges in its + * `vlan-ids` field — "10", "10,20", "10-12", "1,10-12,20". Anything that reduces + * that to one integer (a bare `parseInt`) silently loses VLANs, so both the safety + * analysis and the collector share this expansion. + */ + +/** Split a RouterOS comma-separated field into trimmed, non-empty parts. */ +export function rosList(val: string | undefined): string[] { + if (!val) return []; + return val.split(',').map((s) => s.trim()).filter(Boolean); +} + +/** Every VLAN ID a `vlan-ids` spec covers, in the order RouterOS lists them. */ +export function expandVlanIds(spec: string | undefined): number[] { + const out: number[] = []; + for (const part of rosList(spec)) { + const range = part.match(/^(\d+)\s*-\s*(\d+)$/); + if (range) { + const from = parseInt(range[1], 10); + const to = parseInt(range[2], 10); + if (!isNaN(from) && !isNaN(to) && to >= from) { + for (let v = from; v <= to; v++) out.push(v); + } + continue; + } + const n = parseInt(part, 10); + if (!isNaN(n)) out.push(n); + } + return out; +} + +/** True when the spec names more than one VLAN (a range or a list). */ +export function isMultiVlanSpec(spec: string | undefined): boolean { + return expandVlanIds(spec).length > 1; +} + +export interface AggregatedVlan { + bridge: string; + vlanId: number; + /** Raw RouterOS specs this VLAN was found in, e.g. "10" or "10-20". */ + spec: string; + tagged: string[]; + untagged: string[]; + /** Operator's comment, taken from a static row only. */ + name: string | null; + /** The source rows, kept for the cached config_json. */ + rows: Record[]; +} + +/** + * Collapse the bridge VLAN table into one entry per (bridge, VLAN ID). + * + * Two things make this non-obvious: + * + * - A row's `vlan-ids` may be a range or list, so one row expands into several. + * - RouterOS can hold several rows for the same (bridge, VID) — typically the + * operator's row plus a dynamic one that a port PVID created. + * + * Membership comes from the STATIC `tagged`/`untagged` fields and dynamic rows + * contribute none. This is deliberate and load-bearing: the result backs the VLAN + * editor, and `updateBridgeVlan` is remove-then-add, so whatever is shown is written + * back. Using the effective `current-*` view here would bake RouterOS's dynamic + * entries into the saved configuration on every save. A dynamic row still registers + * the VLAN's existence so it appears in the list. + */ +export function aggregateBridgeVlans(rows: Record[]): AggregatedVlan[] { + const out = new Map; t: Set; u: Set }>(); + + for (const row of rows) { + const bridge = row['bridge'] || ''; + const spec = row['vlan-ids'] || ''; + const dynamic = row['dynamic'] === 'true'; + + for (const vlanId of expandVlanIds(spec)) { + const key = `${bridge}:${vlanId}`; + const entry = out.get(key) ?? { + bridge, vlanId, spec: '', tagged: [], untagged: [], name: null, rows: [], + specs: new Set(), t: new Set(), u: new Set(), + }; + entry.rows.push(row); + if (!dynamic) { + if (spec) entry.specs.add(spec); + for (const p of rosList(row['tagged'])) entry.t.add(p); + for (const p of rosList(row['untagged'])) entry.u.add(p); + if (!entry.name && row['comment']) entry.name = row['comment']; + } + out.set(key, entry); + } + } + + return [...out.values()].map((e) => ({ + bridge: e.bridge, + vlanId: e.vlanId, + spec: [...e.specs].join(','), + tagged: [...e.t], + untagged: [...e.u], + name: e.name, + rows: e.rows, + })); +} + +/** + * Per-port VLAN membership derived from the same table. Static-only for the same + * round-trip reason as above: these lists prefill the port editor, and + * `setPortVlanConfig` rewrites whatever it is handed. + */ +export function portVlanMembership(rows: Record[]): Map { + const out = new Map; untagged: Set }>(); + const slot = (port: string) => { + const existing = out.get(port) ?? { tagged: new Set(), untagged: new Set() }; + out.set(port, existing); + return existing; + }; + + for (const row of rows) { + if (row['dynamic'] === 'true') continue; + const vids = expandVlanIds(row['vlan-ids']); + for (const p of rosList(row['tagged'])) for (const v of vids) slot(p).tagged.add(v); + for (const p of rosList(row['untagged'])) for (const v of vids) slot(p).untagged.add(v); + } + + const asc = (a: number, b: number) => a - b; + return new Map( + [...out.entries()].map(([port, m]) => [port, { + tagged: [...m.tagged].sort(asc), + untagged: [...m.untagged].sort(asc), + }]) + ); +} diff --git a/frontend/package.json b/frontend/package.json index df62089..aa15371 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-frontend", - "version": "0.19.2-beta", + "version": "0.20.0-beta", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/components/device-detail/ConfigHealthCard.tsx b/frontend/src/components/device-detail/ConfigHealthCard.tsx new file mode 100644 index 0000000..6f96551 --- /dev/null +++ b/frontend/src/components/device-detail/ConfigHealthCard.tsx @@ -0,0 +1,205 @@ +import { useState } from 'react'; +import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; +import { + Activity, AlertTriangle, ChevronDown, ChevronRight, CircleCheck, + ExternalLink, RefreshCw, ShieldAlert, Wrench, +} from 'lucide-react'; +import { devicesApi, type ConfigFinding } from '../../services/api'; +import { useCanWrite } from '../../hooks/useCanWrite'; +import clsx from 'clsx'; + +const SEV: Record = { + critical: { + chip: 'bg-red-100 dark:bg-red-900/30 text-red-700 dark:text-red-400', + card: 'border-red-300 dark:border-red-800', + label: 'Broken', + }, + warning: { + chip: 'bg-amber-100 dark:bg-amber-900/30 text-amber-700 dark:text-amber-400', + card: 'border-amber-300 dark:border-amber-800', + label: 'Fragile', + }, + info: { + chip: 'bg-blue-100 dark:bg-blue-900/30 text-blue-700 dark:text-blue-400', + card: 'border-blue-300 dark:border-blue-800', + label: 'Note', + }, +}; + +function since(iso: string | null): string { + if (!iso) return ''; + const days = Math.floor((Date.now() - new Date(iso).getTime()) / 86_400_000); + if (days < 1) return 'first seen today'; + if (days === 1) return 'present for 1 day'; + return `present for ${days} days`; +} + +function Finding({ f }: { f: ConfigFinding }) { + const [open, setOpen] = useState(f.severity === 'critical'); + const style = SEV[f.severity]; + + return ( +
+ + + {open && ( +
+

{f.detail}

+ + {f.remediation && ( +
+ +

{f.remediation}

+
+ )} + +
+ {f.objects.length > 0 && ( + {f.objects.join(' · ')} + )} + {f.first_seen && {since(f.first_seen)}} + {f.doc_url && ( + + MikroTik docs + + )} +
+
+ )} +
+ ); +} + +/** + * Standing audit for configurations RouterOS accepted but that do not work. + * + * Distinct from the hardening score above it: nothing here is a security setting. + * These are contradictions the device will never report — an address on a slave + * port, a PVID that a frame-type makes inert — so this panel is the only place + * they become visible. + */ +export default function ConfigHealthCard({ deviceId }: { deviceId: number }) { + const qc = useQueryClient(); + const canWrite = useCanWrite(); + const [scanError, setScanError] = useState(''); + + const { data, isLoading } = useQuery({ + queryKey: ['config-health', deviceId], + queryFn: () => devicesApi.getConfigHealth(deviceId).then((r) => r.data), + }); + + const scan = useMutation({ + mutationFn: () => devicesApi.scanConfigHealth(deviceId).then((r) => r.data), + onSuccess: (fresh) => { + qc.setQueryData(['config-health', deviceId], fresh); + setScanError(''); + }, + onError: (err: unknown) => { + const r = (err as { response?: { data?: { error?: string } } })?.response?.data; + setScanError(r?.error || 'Could not audit the device'); + }, + }); + + const findings = data?.findings ?? []; + const critical = findings.filter((f) => f.severity === 'critical').length; + const warnings = findings.filter((f) => f.severity === 'warning').length; + + return ( +
+
+
+
+ +
+

Config Health

+ {critical > 0 && ( + + {critical} broken + + )} + {warnings > 0 && ( + + {warnings} fragile + + )} +
+ {canWrite && ( + + )} +
+ +

+ RouterOS applies each command on its own, with no check that the result is coherent — so a + configuration can be accepted, saved, and still not work. This audit looks for those cases. +

+ + {scanError && ( +
+ +

{scanError}

+
+ )} + + {isLoading ? ( +
+ Loading findings… +
+ ) : findings.length === 0 ? ( +
+ +
+
+ No configuration conflicts found +
+
+ {data?.checked_at + ? `Last audited ${new Date(data.checked_at).toLocaleString()}.` + : 'This device has not been audited yet — run one with Re-audit.'} +
+
+
+ ) : ( + <> +
+ {findings.map((f) => )} +
+ {data?.checked_at && ( +
+ + Last audited {new Date(data.checked_at).toLocaleString()} +
+ )} + + )} +
+ ); +} diff --git a/frontend/src/components/device-detail/SecurityTab.tsx b/frontend/src/components/device-detail/SecurityTab.tsx index a763ae1..0a8564b 100644 --- a/frontend/src/components/device-detail/SecurityTab.tsx +++ b/frontend/src/components/device-detail/SecurityTab.tsx @@ -5,6 +5,7 @@ import { devicesApi } from '../../services/api'; import type { SecurityCheck } from '../../services/api'; import { useCanWrite } from '../../hooks/useCanWrite'; import { LockoutVerdictDialog, lockoutVerdictOf, type LockoutVerdict } from '../ChangeGuardDialog'; +import ConfigHealthCard from './ConfigHealthCard'; import clsx from 'clsx'; type Row = Record & { '.id': string }; @@ -190,6 +191,12 @@ export default function SecurityTab({ deviceId, deviceName }: { deviceId: number )} + {/* Correctness rather than hardening: configurations the device accepted and + will never complain about, but that do not do what they appear to. */} +
+ +
+ {lockout && ( 1 ? 'access' : 'none', pvid: bridgePvid, - tagged_vlans: '', + // Prefill from the port's actual tagged membership so opening the dialog and + // saving does not silently strip the trunk's VLANs. + tagged_vlans: (port.bridgeInfo?.tagged_vlan_ids ?? []).join(','), }); }; diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index a4cb1a9..48a6309 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -176,6 +176,28 @@ export interface SecurityCheck { serviceId?: string; } +/** + * A configuration RouterOS accepted but that does not work. Produced by the + * standing Config Health audit, which encodes MikroTik's documented Layer2 + * misconfigurations plus the latent lockout cases. + */ +export interface ConfigFinding { + rule: string; + severity: 'critical' | 'warning' | 'info'; + title: string; + detail: string; + remediation: string | null; + doc_url: string | null; + objects: string[]; + first_seen: string | null; + last_seen: string | null; +} + +export interface ConfigHealthResponse { + findings: ConfigFinding[]; + checked_at: string | null; +} + export const devicesApi = { list: () => api.get('/devices'), discovered: () => api.get('/devices/discovered'), @@ -320,6 +342,11 @@ export const devicesApi = { { disabled, ...(confirmLockout ? { confirm_lockout: true } : {}) }), getSecurityPosture: (id: number) => api.get<{ score: number; checks: SecurityCheck[] }>(`/devices/${id}/security-posture`), + // Config Health — cached findings load instantly; the scan re-reads the device. + getConfigHealth: (id: number) => + api.get(`/devices/${id}/config-health`), + scanConfigHealth: (id: number) => + api.post(`/devices/${id}/config-health/scan`), patchLocation: (id: number, data: { location_address?: string | null; location_lat?: number | null; diff --git a/frontend/src/types/index.ts b/frontend/src/types/index.ts index 97f0e30..67b9b52 100644 --- a/frontend/src/types/index.ts +++ b/frontend/src/types/index.ts @@ -103,6 +103,8 @@ export interface Vlan { vlan_id: number; name?: string; bridge?: string; + /** Raw RouterOS spec this row came from; a range/list means the entry covers several VLANs. */ + vlan_ids?: string | null; tagged_ports?: string[]; untagged_ports?: string[]; } @@ -112,7 +114,11 @@ export interface BridgeVlanEntry { bridge: string; port: string; pvid?: number; + /** Every VLAN this port belongs to, tagged or untagged. */ vlan_ids?: string[]; + tagged_vlan_ids?: string[]; + untagged_vlan_ids?: string[]; + /** True when the port is a tagged member of at least one VLAN (i.e. a trunk). */ tagged: boolean; } diff --git a/frontend/src/version.ts b/frontend/src/version.ts index a198f6d..c424488 100644 --- a/frontend/src/version.ts +++ b/frontend/src/version.ts @@ -1 +1 @@ -export const APP_VERSION = 'v0.19.2 Beta'; +export const APP_VERSION = 'v0.20.0 Beta';