security: OpenVPN sanitizer redaction, pfSense validator atomicity, plugin loader preflight

.mergify.yml

@@ -44,7 +44,6 @@ queue_rules:
       - "check-success = Test (windows-latest)"
       - check-success = Integration Tests
       - check-success = Coverage
-      - check-success = codecov/project
       - check-success = codecov/patch
 
   # ─────────────────────────────────────────────────────────────────────────
@@ -61,7 +60,6 @@ queue_rules:
       - "check-success = Test (windows-latest)"
       - check-success = Integration Tests
       - check-success = Coverage
-      - check-success = codecov/project
       - check-success = codecov/patch
 
 pull_request_rules:
@@ -136,7 +134,6 @@ merge_protections:
       - "check-success = Test (windows-latest)"
       - check-success = Integration Tests
       - check-success = Coverage
-      - check-success = codecov/project
       - check-success = codecov/patch
 
   # ─────────────────────────────────────────────────────────────────────────

.tours/new-joiner-parser-public-api.tour

@@ -53,9 +53,9 @@
     },
     {
       "file": "pkg/parser/pfsense/parser.go",
-      "line": 62,
-      "title": "ValidateFunc — The Safe Default Injection Point",
-      "description": "Situation: ValidateFunc is a package-level variable holding the semantic validator. Mechanism: cmd/ (or an equivalent composition root) sets this once at startup to plug internal/validator into the public package without breaking the pkg/ import boundary. When nil, ParseAndValidate falls back to structural parsing only. Implication: Library consumers get a safe no-validator default; the CLI gets full validation. Gotcha: The nil fallback is a feature, not a bug — it's what lets external modules consume the parser without pulling in opnDossier's validator."
+      "line": 104,
+      "title": "SetValidator — The One-Shot Injection Point",
+      "description": "Situation: SetValidator is the public entry point for installing the pfSense semantic validator, guarded by a sync.Once over an atomic.Pointer. Mechanism: cmd/ calls SetValidator during its own init() to plug internal/validator in before any dynamic plugin is loaded; any subsequent caller (including a malicious plugin's init() fired by plugin.Open) hits the Once's already-ran branch and its fn is silently dropped. When no validator has been installed, ParseAndValidate falls back to structural parsing only. Implication: Library consumers get a safe no-validator default; the CLI gets full validation; plugin stomp attacks are closed off deterministically. Gotcha: The one-shot lock is global and process-wide — tests that need to swap validators use the ResetValidatorForTesting helper defined in export_test.go (see GOTCHAS.md §20)."
     },
     {
       "file": "pkg/parser/xmlutil.go",
@@ -71,7 +71,7 @@
     },
     {
       "title": "Next Moves",
-      "description": "You can now: (1) if you already have a parsed DTO, call opnsense.ConvertDocument(*schema.OpnSenseDocument) or pfsense.ConvertDocument(*pfschema.Document) directly — this is the idiomatic consumer entry point and bypasses the registry entirely; (2) if you only have raw XML on a reader, import pkg/parser + blank-import the per-device packages you need, inject an OPNsenseXMLDecoder, and call Factory.CreateDevice (auto-detect path); (3) add a new device type by creating a package whose init() calls parser.Register('vendorname', ConstructorFunc); (4) wire semantic validation by setting pfsense.ValidateFunc (and its future OPNsense equivalent, if added) from your composition root. See GOTCHAS.md §7.1 for the blank-import pitfall and the NATS-144 ticket for the audit that produced this public surface."
+      "description": "You can now: (1) if you already have a parsed DTO, call opnsense.ConvertDocument(*schema.OpnSenseDocument) or pfsense.ConvertDocument(*pfschema.Document) directly — this is the idiomatic consumer entry point and bypasses the registry entirely; (2) if you only have raw XML on a reader, import pkg/parser + blank-import the per-device packages you need, inject an OPNsenseXMLDecoder, and call Factory.CreateDevice (auto-detect path); (3) add a new device type by creating a package whose init() calls parser.Register('vendorname', ConstructorFunc); (4) wire semantic validation by calling pfsense.SetValidator (and its future OPNsense equivalent, if added) from your composition root. See GOTCHAS.md §7.1 for the blank-import pitfall, §20 for the SetValidator stomp-protection contract, and the NATS-144 ticket for the audit that produced this public surface."
     }
   ]
 }

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **sanitizer**: `Sanitizer.SetLogger` plus a reflection-path warning when `SanitizeStruct` encounters a `map[K]struct{...}` or `map[K]*struct{...}` value. Go does not allow in-place mutation of such map values, so the walker has always skipped them silently — the warning surfaces the gap so a future schema that routes secrets through a struct-valued map is detected at runtime rather than shipped as cleartext. The raw-XML `SanitizeXML` path is unaffected. Full redaction of struct-valued maps is scheduled under tag-based redaction.
 - **schema**: NATS-3 audit and harden public API surface for cross-repo consumption ([#569](https://github.com/EvilBit-Labs/opnDossier/pull/569))
 
 - **schema**: Parse OPNsense Unbound MVC and flip FIREWALL-007 polarity - NATS-77 ([#571](https://github.com/EvilBit-Labs/opnDossier/pull/571))
@@ -18,6 +19,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **parser**: Document the cancellation contract on `peekRootElementBounded`
+  in `pkg/parser/factory.go`. The function returns `ctx.Err()` immediately on
+  cancellation, but its internal goroutine only exits when the next read
+  unblocks. Library consumers supplying readers that can block indefinitely
+  (sockets, fifos, long-polling HTTP bodies) must cancel the context to
+  release the goroutine — CLI consumers using `*os.File` are unaffected.
+  No safety-net watchdog was added because `peekRootElementBounded` does not
+  own the reader it receives; closing a caller-owned reader from inside the
+  helper would corrupt caller state.
+- **[breaking]** `pkg/parser/pfsense.ValidateFunc` (public var) removed; use
+  `pkg/parser/pfsense.SetValidator` instead. Free pre-v1.5 per Current Regime.
 - **mergify**: Upgrade configuration to current format ([#543](https://github.com/EvilBit-Labs/opnDossier/pull/543))
 
 - Update labeling instructions and configuration settings in .coderabbit.yaml
@@ -125,6 +137,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **parser**: Liberal boolean parsing for OPNsense config.xml ([#558](https://github.com/EvilBit-Labs/opnDossier/pull/558)) ([#577](https://github.com/EvilBit-Labs/opnDossier/pull/577))
 
+### Security
+
+- Sanitizer now redacts OpenVPN `<tls>` and `<StaticKeys>` XML elements
+  and detects the OpenVPN static-key PEM envelope. Previously, operators
+  running `opnDossier sanitize` on configs containing OpenVPN TLS-auth
+  material leaked the raw HMAC keys (SEC-H1 from comprehensive review).
+- pfSense validator protected against stomp by malicious dynamic plugin
+  `init()` code. The public `pfsense.ValidateFunc` var was removed and
+  replaced by `pfsense.SetValidator`, which installs the validator once
+  via `sync.Once` over an internal `atomic.Pointer` holder. Second and
+  subsequent calls are silently dropped. Matches OPNsense's
+  `XMLDecoder.ParseAndValidate` DI pattern. Fixes SEC-H2 from
+  comprehensive review.
+- Plugin loader preflight now rejects symlinks, group/world-writable
+  plugin files, group/world-writable container directories, and
+  relative plugin paths. Emits structured audit log per load attempt
+  (path, SHA-256, mode, owner UID, verdict). Addresses SEC-M1 / CWE-732
+  footguns from the comprehensive review. Phase B hardening (owner
+  verification, size cap, path denylist, filename allowlist, optional
+  SHA-256 manifest) is tracked for v1.6.
+- `debug.Stack()` dumps at `internal/audit/plugin.go:275-287`,
+  `cmd/audit.go:302-313`, and `internal/processor/processor.go:91` are
+  now gated behind `logger.IsVerbose()`. Function names in stack dumps
+  can leak internal plugin paths (e.g. `acmecorp-pci-plugin.RunChecks`)
+  into centralized logs, revealing customer compliance posture. Fixes
+  SEC-M5 / QUAL-M8.
 
 ## [1.4.0] - 2026-04-03
 

GOTCHAS.md

@@ -55,6 +55,21 @@ Reclassified info-severity controls (e.g., FIREWALL-003 "Message of the Day") pa
 - **Mitigation:** Loading is opt-in: it requires an explicit `--plugin-dir` flag (or the equivalent config key). There is no `./plugins` auto-discovery fallback — `PluginManager.InitializePlugins` only calls `LoadDynamicPlugins` when `pluginDir != ""`. Plugins are never fetched remotely.
 - **Prevention:** Restrict filesystem permissions on the plugin directory. Only load plugins built from reviewed source code. In shared or CI environments, avoid pointing `--plugin-dir` at world-writable directories.
 
+**Phase A hardening (v1.5).** Before `plugin.Open` is invoked, `runPluginPreflight` in `internal/audit/plugin_preflight.go` rejects the following footguns and emits a structured audit log per attempt:
+
+- **Symlinks rejected** via `os.Lstat` + `os.ModeSymlink` check. `plugin.Open` follows links, so an attacker with write access to the plugin directory could otherwise point a `.so` at anything on the filesystem.
+- **Non-regular files rejected** via `info.Mode().IsRegular()` (cross-platform). A FIFO, socket, or device node named `*.so` would otherwise block `hashFileSizeCapped` indefinitely on `os.Open` or `io.CopyN` before `plugin.Open` is ever reached — a DoS primitive trivially reachable on POSIX.
+- **Group/world-writable plugin files rejected** when `info.Mode().Perm()&0o022 != 0` (POSIX only). Closes CWE-732 where another local account could swap the file between audits.
+- **Group/world-writable container directory rejected** via a second `os.Stat` on `filepath.Dir(path)` (POSIX only). The file bits alone are not enough — a writable parent lets an attacker unlink and replace the plugin.
+- **Absolute plugin file paths required at preflight** via `filepath.IsAbs` (cross-platform, defense-in-depth). Relative `--plugin-dir` inputs from the operator are accepted and normalized via `filepath.Abs` before the preflight runs, so `--plugin-dir ./plugins` is a supported invocation; the absolute-path check then fires if any caller ever bypasses `LoadDynamicPlugins` and hands a relative path directly to `runPluginPreflight`.
+- **Structured audit log per load attempt**: INFO for accepted loads, WARN for rejections, with fields `plugin`, `path`, `sha256`, `mode`, `owner_uid`, `mtime`, `size_bytes`, `verdict`, `reason`. The logged `path` is the normalized absolute plugin artifact path, and the SHA-256 is computed with a 64 MiB read cap so a pathological `.so` bounds preflight I/O and CPU time (the hasher streams rather than buffers, so this is a time/throughput cap, not a memory-allocation cap).
+
+Rejections are reported as `PluginLoadError` entries in the returned `LoadResult`, so callers see identical wiring for preflight and `plugin.Open` failures.
+
+**Phase B follow-ups (post-v1.5, tracked in todo #146):** owner-UID check (refuse `.so` whose UID does not match the process UID or a configured allowlist), hard configurable size cap (`--plugin-max-size-mb`), path denylist (`/tmp`, `/var/tmp`, `/dev/shm`, `$HOME` when EUID==0), filename allowlist (no NUL / shell metachars / path separators), optional `plugins.sha256` manifest enforcement, documented seccomp/landlock sandboxing recipe, and — aspirationally — out-of-process plugin isolation à la HashiCorp go-plugin.
+
+**Windows behaviour.** POSIX permission bits are meaningless on NTFS, so the writable-mode and writable-dir rejections are skipped at runtime when `runtime.GOOS == "windows"`. The symlink and absolute-path checks still run. The `owner_uid` audit field is emitted as `unavailable` on Windows; Phase B will introduce a SID-aware ownership check if needed.
+
 **See also:** [docs/solutions/runtime-errors/plugin-panic-recovery-audit-runchecks.md](docs/solutions/runtime-errors/plugin-panic-recovery-audit-runchecks.md) — fault-isolation pattern that contains panics from the untrusted plugins described here.
 
 ## 3. Data Processing
@@ -223,6 +238,23 @@ When adding a new device type (e.g., pfSense), audit the XML element names for c
 - **Detection:** `sanitize <config.xml> | grep -i 'hash\|secret\|key\|pass'` — check for unredacted sensitive values.
 - **Prevention:** When adding a new device schema, grep for credential-like fields and verify each is matched by a sanitizer rule.
 
+### 11.3 OpenVPN TLS and StaticKeys Are Credentials, Not Labels
+
+OpenVPN's `<tls>` element (under `<openvpn-server>` / `<openvpn-client>`) holds the `--tls-auth` / `--tls-crypt` HMAC key, and the MVC `<OpenVPN><StaticKeys>` element holds static-key material. Both arrive in a PEM-shaped envelope with the literal label `-----BEGIN OpenVPN Static key V1-----` — which the stock `IsPrivateKey` detector misses because the label is not `PRIVATE KEY`. The sanitizer's `private_key` rule uses path-anchored `FieldPatterns` (`openvpn.tls`, `openvpn-server.tls`, `openvpn-client.tls`, `openvpn.statickeys`, `statickeys`, `tls_crypt`, `tls_auth`) plus the `IsOpenVPNStaticKey` envelope detector to catch both the field-name and value-based paths.
+
+- **Gotcha:** The substring `tls` ALONE is not safe as a pattern. It would false-positive on:
+
+  - The Suricata IDS wrapper `opnsense.OPNsense.IDS.general.eveLog.tls.*` (a struct of enable/extended/sessionResumption/custom booleans — `pkg/schema/opnsense/security.go` L383).
+  - The IPsec strongSwan daemon log-level enum `opnsense.OPNsense.IPsec.charon.syslog.daemon.tls` (an integer 0–5 — `pkg/schema/opnsense/security.go` L443).
+
+  Always anchor OpenVPN TLS patterns to their parent element path (e.g., `openvpn-server.tls`) and verify new unambiguous OpenVPN field names (like `tls_crypt` / `tls_auth`) never collide with other device schemas before adding them bare.
+
+- **Detection:** `TestSanitizeXML_OpenVPNStaticKey` + `TestSanitizeXML_OpenVPN_TLS_NoFalsePositives` in `internal/sanitizer/sanitizer_test.go`. `TestIsOpenVPNStaticKey` in `patterns_test.go` covers the envelope detector.
+
+- **Rule-ordering impact:** None. The `private_key` rule is distinct from `authserver_config` / `password` / `email` / `hostname` and does not participate in the §19.1 ordering invariants.
+
+- **History:** SEC-H1 from the 2026-04-19 comprehensive review. Prior to the fix, `opnDossier sanitize` silently leaked raw HMAC keys sufficient to forge OpenVPN handshakes — the headline promise of the subcommand.
+
 ## 12. Git Tagging
 
 ### 12.1 Tag the Squash-Merge Commit on Main
@@ -260,6 +292,15 @@ When tagging a release after a squash-merge PR, always tag the resulting commit
 
 A fresh `NewRuleEngine` creates a fresh `NewMapper()` — mappings are deterministic (e.g., first private IP maps to `10.0.0.1`, first hostname to `host-001.example.com`). Always assert exact expected values, not just inequality.
 
+### 14.4 `SanitizeStruct` Skips Struct/Pointer-Valued Maps
+
+`sanitizeReflect` in `internal/sanitizer/sanitizer.go` cannot recurse into `map[K]struct{...}` or `map[K]*struct{...}` values. Map values are not addressable in Go, so `reflect.Value.SetMapIndex` is the only way to write back — and that requires a fully reconstructed element, which the current walker does not perform. The guard at the top of the `reflect.Map` case detects this and logs a warning via the optional logger injected through `Sanitizer.SetLogger`. When no logger is set, the gap is silent.
+
+- **Current scope:** This gap is reachable ONLY through `SanitizeStruct`, which is an opt-in consumer flow. The default `opnDossier sanitize` CLI uses `SanitizeXML` (raw element walk) and is not affected — element names like `<password>` and `<bcrypt-hash>` are still redacted regardless of the Go model shape.
+- **Known current paths:** OPNsense `KeaDhcp4` already uses map-style subnet containers, but those maps hold config metadata, not credentials. No currently-shipped schema path puts a secret behind a struct-valued map.
+- **Why warn instead of fix:** Supporting struct-valued maps via reflection requires reconstructing each element in place (read → recurse into a copy → `SetMapIndex` with the mutated copy). That work is scheduled under todo #151 (tag-based redaction) which will subsume this gap by annotating sensitive fields directly and driving redaction from tags instead of field-name heuristics. The warning is the bridge until #151 lands.
+- **Regression tests:** `TestSanitizeStruct_MapStructValues_WarnsAndSkips` and `TestSanitizeStruct_MapStructValues_NilLoggerNoPanic` in `internal/sanitizer/sanitizer_reflect_test.go` pin both the warning path and the nil-logger nil-safety invariant. If a future enhancement starts handling struct-valued maps, those tests must be updated (or replaced) to reflect the new behavior — do not delete them blind.
+
 ## 15. Liberal Boolean and Integer Parsing
 
 ### 15.0 `BoolFlag` vs `FlexBool` vs `FlexInt` vs strict `int`/`bool`
@@ -352,3 +393,16 @@ Kea's `<pools>` element on each `<subnet4>` stores newline-separated (`\n`) IP r
 - **Problem:** If `password` is moved above `authserver_config` in the `builtinRules()` slice, `ldap_bindpw` values silently switch from pseudonymized to flat-redacted. No error or warning is emitted.
 - **Symptom:** Sanitized output shows `[REDACTED-PASSWORD]` for LDAP bind passwords instead of a pseudonymized value like `ldap-bindpw-001`.
 - **Fix:** Ensure `authserver_config` remains the first rule in `builtinRules()`. The same first-match precedence applies to `email` vs `hostname` (email must precede hostname).
+
+## 20. pfSense Validator Injection
+
+### 20.1 `SetValidator` Is Guarded by `sync.Once`
+
+`pkg/parser/pfsense.SetValidator` installs the semantic validator used by `Parser.ParseAndValidate`. The slot is unexported (`validateFuncHolder atomic.Pointer[...]`) and is written exactly once per process — the first `SetValidator` call wins, every subsequent call is silently dropped. The `atomic.Pointer` pairs with `sync.Once` so the writer side synchronizes cleanly with the concurrent reader side in `ParseAndValidate`.
+
+The one-shot lock is the enforcement point against a dynamically loaded compliance plugin's `init()` reassigning the validator after CLI setup. Because `plugin.Open` fires the loaded `.so`'s `init()` at load time — later than Go's own `init()` ordering for in-tree packages — `cmd/root.go:init` calls `SetValidator` first, commits the sync.Once, and any subsequent plugin `init()` is effectively locked out. See the comprehensive-review ticket SEC-H2 / todos #105 and #128 for the original finding.
+
+- **Gotcha:** Do NOT reintroduce a public mutable `ValidateFunc` variable. Any value that is directly assignable from plugin code re-opens the stomp hazard. The injection point MUST go through `SetValidator`.
+- **Gotcha:** Test code that needs to swap validators across subtests uses the `ResetValidatorForTesting` / `ValidatorForTesting` helpers defined in `pkg/parser/pfsense/export_test.go`. These live in `_test.go` and therefore are NOT part of the public API — never promote them to a plain `.go` file.
+- **Gotcha:** The exported `SetValidator` is safe to call from any goroutine; concurrent writers race to win the `sync.Once`, but only one does. Readers never see a torn value because the holder is `atomic.Pointer[...]` — verified by `TestPfSense_SetValidator_Race` under `-race`.
+- **Regression tests:** `TestPfSense_SetValidator_CannotBeOverwritten` pins the stomp-protection invariant; `TestPfSense_SetValidator_Race` pins the concurrent-writer safety. Both live in `pkg/parser/pfsense/parser_test.go`. If either fails, the §20 defense has regressed.