security: OpenVPN sanitizer redaction, pfSense validator atomicity, plugin loader preflight - #587
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the mutable pfSense validator hook with a one-shot Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Operator
participant CLI as Audit CLI
participant Loader as Plugin Loader
participant Preflight as PluginPreflight
participant AuditLog as Audit Logger
participant Plugin as .so Plugin
participant Compliance as Compliance Runner
User->>CLI: opnDossier audit --plugin-dir=...
CLI->>CLI: warnPluginDirTrustModel(stderr)
CLI->>Loader: LoadDynamicPlugins(absDir)
loop per .so file
Loader->>Preflight: runPluginPreflight(path)
Preflight->>Preflight: abs/path check<br/>symlink & perms check<br/>hashFileSizeCapped(64MiB)
alt accepted
Preflight-->>Loader: verdict=accepted, sha256,...
Loader->>AuditLog: logPreflight(INFO, fields...)
Loader->>Plugin: plugin.Open(path)
Plugin-->>Loader: plugin loaded
else rejected
Preflight-->>Loader: verdict=rejected, err
Loader->>AuditLog: logPreflight(WARN, fields...)
Loader->>Loader: record failure, skip load
end
end
Loader-->>CLI: loaded plugins
CLI->>Compliance: RunComplianceChecks(...)
Compliance->>Plugin: execute
Plugin-->>Compliance: results
Compliance->>AuditLog: Log results (stack only if IsVerbose())
CLI-->>User: audit report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
dosubot
Bot
added
the
size:XL
dosubot
Bot
added
breaking_change
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 5 security hardening across sanitizer redaction, pfSense validator injection, dynamic plugin trust model disclosure, and plugin loader preflight checks to reduce secret leakage and plugin-based attack surface.
Changes:
- Extend sanitizer detection/redaction to cover OpenVPN
<tls>/<StaticKeys>static-key material and add reflect-path observability for struct/pointer-valued maps. - Replace pfSense
ValidateFuncinjection withSetValidator(sync.Once + atomic.Pointer) plus regression tests and API snapshot updates. - Add dynamic plugin trust-model warnings/docs and introduce plugin loader preflight (symlink/permissions/sha256/size cap) with structured audit logging and tests.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/testdata/api-snapshots/pkg-parser-pfsense.golden | Updates public API snapshot text for SetValidator semantics. |
| pkg/parser/pfsense/parser_test.go | Adds regression/race tests for SetValidator and validator fallback behavior. |
| pkg/parser/pfsense/parser.go | Implements SetValidator with sync.Once + atomic.Pointer and updates ParseAndValidate behavior/docs. |
| pkg/parser/pfsense/export_test.go | Adds test-only helpers to reset/introspect the validator slot. |
| pkg/parser/factory_test.go | Adds cancellation regression test for hung readers and CreateDevice behavior. |
| pkg/parser/factory.go | Documents peekRootElementBounded cancellation contract and goroutine lifetime implications. |
| internal/sanitizer/sanitizer_test.go | Adds OpenVPN static-key redaction + false-positive guard tests across sanitizer modes. |
| internal/sanitizer/sanitizer_reflect_test.go | New tests for struct/pointer-valued map warning behavior and nil-logger safety. |
| internal/sanitizer/sanitizer.go | Adds optional logger injection and logs when reflect-based sanitization hits non-addressable map values. |
| internal/sanitizer/rules.go | Adds OpenVPN path-anchored field patterns and aliases to private-key rule. |
| internal/sanitizer/patterns_test.go | Adds OpenVPN static-key envelope detector tests and ensures IsPrivateKey matches it. |
| internal/sanitizer/patterns.go | Enhances private-key detection to include OpenVPN static-key envelope. |
| internal/processor/validate_test.go | Expands panic-recovery tests to verify stack dumps are gated by verbosity. |
| internal/processor/processor.go | Gates panic stack dumps behind logger.IsVerbose(). |
| internal/logging/logger_test.go | Adds tests for Logger.IsVerbose() behavior and nil-safety. |
| internal/logging/logger.go | Introduces IsVerbose() helper for gating sensitive/expensive diagnostics. |
| internal/audit/plugin_preflight_windows.go | Windows build-tag implementation for owner UID extraction (placeholder). |
| internal/audit/plugin_preflight_posix.go | POSIX owner UID extraction for audit log metadata. |
| internal/audit/plugin_preflight.go | Implements plugin preflight checks (symlink/permissions/abs path/sha256 size-capped). |
| internal/audit/plugin_hardening_test.go | Adds comprehensive tests for preflight acceptance/rejection and audit-log emission. |
| internal/audit/plugin.go | Integrates preflight into dynamic plugin loading and gates plugin panic stack dumps behind verbosity. |
| docs/user-guide/configuration-reference.md | Updates --plugin-dir documentation to disclose trust model and link guidance. |
| docs/user-guide/commands/audit.md | Adds “Dynamic Plugin Security” section and expands dynamic plugin trust-model disclosure. |
| docs/development/standards.md | Updates developer standards to reference pfsense.SetValidator injection model. |
| docs/cli/opnDossier_audit.md | Expands CLI help output for --plugin-dir trust model. |
| cmd/shared_flags.go | Centralizes --plugin-dir help text and adds stderr trust-model warning helper. |
| cmd/root.go | Wires pfSense validator via SetValidator and documents plugin-stomp threat model. |
| cmd/audit_test.go | Adds test ensuring PreRunE emits --plugin-dir trust-model warning to stderr. |
| cmd/audit.go | Uses shared --plugin-dir help text and emits trust-model warning in PreRunE. |
| README.md | Documents dynamic plugin trust model at a high level and links detailed guidance. |
| GOTCHAS.md | Documents OpenVPN static-key redaction nuances, sanitize reflect map limitation, plugin preflight hardening, and pfSense SetValidator model. |
| CHANGELOG.md | Records sanitizer reflect warning, peekRootElementBounded cancellation contract, breaking validator API change, and security hardening items. |
| .tours/new-joiner-parser-public-api.tour | Updates onboarding tour to reflect SetValidator instead of ValidateFunc. |
Contributor
|
Related Documentation 2 document(s) may need updating based on files changed in this PR: opnDossier plugin-development
|
dosubot
Bot
added
size:XXL
9 tasks
unclesp1d3r
force-pushed
the
chore/todos-phase4-public-api
branch
from
April 19, 2026 20:43
6321c12 to
66d5e75
Compare
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 Enforce conventional commitWaiting for:
This rule is failing.Require conventional commit format per https://www.conventionalcommits.org/en/v1.0.0/. Skipped for dependabot and dosubot.
🔴 Full CI must passWaiting for:
This rule is failing.All CI checks must pass. Activates for non-bot authors, or dependabot when files exist outside .github/workflows/.
🟢 📃 Configuration Change RequirementsWonderful, this rule succeeded.Mergify configuration change
🟢 Do not merge outdated PRsWonderful, this rule succeeded.Make sure PRs are within 10 commits of the base branch before merging
|
…dator hardening, plugin-dir trust model, plugin loader Phase A
- Sanitizer now redacts OpenVPN <tls> and <StaticKeys> XML elements.
Adds IsOpenVPNStaticKey detector matching the -----BEGIN OpenVPN
Static key V1----- envelope. Field-patterns use path-anchored forms
(openvpn.tls, openvpn-server.tls, openvpn-client.tls,
openvpn.statickeys) to avoid substring collisions with the
non-OpenVPN <tls> tags in Suricata eveLog and IPsec charon syslog
(both found during schema audit). Regression tests cover both
positive redaction (4 fixtures x 3 modes) and false-positive
protection (Suricata + IPsec). Fixes SEC-H1.
- pfSense validator protected against stomp. Unexport ValidateFunc;
add SetValidator guarded by sync.Once over atomic.Pointer so
external readers race-free. Naive sync.Once + plain var had a
data race between writers and ParseAndValidate readers detected
under -race. Test-only ResetValidatorForTesting in export_test.go
lets tests swap validators without leaking into the public API.
TestPfSense_SetValidator_CannotBeOverwritten and
TestPfSense_SetValidator_Race added. cmd/root.go:wirePfSenseValidator
calls SetValidator. Fixes SEC-H2. BREAKING change to pkg/parser/
pfsense (free pre-v1.5 per Current Regime).
- --plugin-dir CLI flag: expand help text with trust-model warning
("full process privileges", "no signature verification",
"GOTCHAS §2.5"). Add stderr warning in audit PreRunE mirroring the
red-mode precedent. Shared helper warnPluginDirTrustModel lives in
cmd/shared_flags.go so future commands accepting --plugin-dir emit
byte-identical warnings. Unit test pins the warning substrings.
README § Security gains a Dynamic plugin trust model paragraph;
docs/user-guide/commands/audit.md gains a Dynamic Plugin Security
subsection. Fixes SEC-H3.
- Plugin loader Phase A hardening: os.Lstat preflight rejects
symlinks, group/world-writable files, group/world-writable
container directories, and relative paths. SHA-256 digest with
64 MiB size cap computed before plugin.Open. Structured audit log
(path, sha256, mode, owner_uid, mtime, size_bytes, verdict, reason)
on every load attempt — INFO accept / WARN reject. POSIX/Windows
split via build tags (owner_uid unavailable on Windows). 10 new
tests in plugin_hardening_test.go with documented cross-platform
skip rationale. Phase B (operator-configurable size cap, owner
UID check, path denylist, filename allowlist, optional SHA-256
manifest, seccomp/landlock recipe) tracked for post-v1.5.
Todos resolved: #104, #105, #106, #123, #127, #128, #146 (Phase A),
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…rbose gate, peek contract - sanitizer: new SanitizeStruct guard warns when a map's element kind is Struct or Ptr (previously silently skipped). Opt-in logger via Sanitizer.SetLogger keeps the 28+ existing callsites nil-safe. GOTCHAS §14.4 documents the limitation; tag-based redaction (#151) will subsume it structurally. Fixes SEC-M3/QUAL-M3. - logging: Logger.IsVerbose() added with nil-safety guards (returns false for nil receiver or zero-value). Used to gate three debug.Stack() call sites (internal/audit/plugin.go, cmd/audit.go panic recovery, internal/processor/processor.go). Function names in stack dumps can leak internal plugin paths into centralized logs revealing customer compliance posture. Fixes SEC-M5/QUAL-M8. - pkg/parser.peekRootElementBounded: expanded godoc documents the cancellation contract explicitly. Safety-net watchdog intentionally NOT added — ownership violation on caller-owned readers. Regression test asserts the outer select returns context.Canceled within 100ms of cancellation against a hung io.Pipe reader. Addresses SEC-M4/CWE-401 observability gap. Todos resolved: #134, #139, #147. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…tor and attack vector details Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
unclesp1d3r
force-pushed
the
chore/todos-phase5-security
branch
from
April 19, 2026 23:33
3096653 to
6697d89
Compare
dosubot
Bot
added
size:XL
…eanup
- internal/audit/plugin.go: LoadDynamicPlugins now resolves the plugin
directory to an absolute path via filepath.Abs before iterating entries.
Without this, a common `--plugin-dir ./plugins` invocation caused every
plugin to be rejected by preflight as a "relative path". The absolute-
path preflight check itself remains as defense-in-depth if a future
caller bypasses LoadDynamicPlugins.
- internal/audit/plugin_preflight.go:
- Constant-block comment said "exported" but the identifiers
(pluginWritableMask, pluginMaxSize, etc.) are unexported; reword to
"package-level constants" and keep the single-source-of-truth
rationale intact.
- Symlink preflight note said "POSIX only — skipped at runtime" for
Windows, but the code rejects symlinks on every platform via
os.Lstat. Align the comment with the actual behavior.
- internal/audit/plugin_hardening_test.go: rename
TestLoadDynamicPlugins_RejectsRelativePath →
TestLoadDynamicPlugins_NormalizesRelativeDir and assert the new
behavior — relative --plugin-dir values are normalized and pass
preflight; the loader-invocation failure (alwaysFailLoader) is the
one we expect. The direct-preflight unit test at line 436 still
covers relative-path rejection when someone bypasses LoadDynamicPlugins.
- pkg/parser/factory.go: cancellation-contract comment referenced
AGENTS.md §1.1 but the t.Parallel / global-state guidance lives in
GOTCHAS.md §1.1. Correct the cross-reference.
- docs/user-guide/commands/audit.md: remove stale claims about default
./plugins auto-load and about symlinks being followed; document the
Phase A preflight (symlink rejection, writable-bit checks, absolute-
path normalization, SHA-256 audit trail, platform caveats).
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
audit
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
internal/audit/plugin_hardening_test.go (4)
594-646:⚠️ Potential issue | 🟡 MinorTest doesn't verify verbose-only stack trace emission.
This test creates a verbose logger but doesn't capture its output. The assertions only confirm panic isolation (zero findings, "panicked" version marker), not that
debug.Stack()diagnostics are actually emitted at verbose level. UsingbufferLogger()and asserting on the captured output would lock in the verbose-branch behavior this test claims to cover.🛠️ Capture logger output for assertion
func TestRunComplianceChecks_PanicVerboseLogging(t *testing.T) { t.Parallel() - verboseLogger, err := logging.New(logging.Config{Level: "debug"}) - if err != nil { - t.Fatalf("failed to create verbose logger: %v", err) - } + verboseLogger, buf := bufferLogger(t) if !verboseLogger.IsVerbose() { t.Fatal("expected debug-level logger to report verbose") } // ... panicking plugin setup unchanged ... result, err := registry.RunComplianceChecks( device, []string{"panicking-plugin-verbose"}, verboseLogger, ) // ... existing assertions ... + output := buf.String() + if !strings.Contains(output, "goroutine") && !strings.Contains(output, "panic") { + t.Errorf("expected stack trace in verbose log output; got:\n%s", output) + } }As per coding guidelines: "
debug.Stack()dumps must be gated behindlogger.IsVerbose()to prevent leakage of internal plugin paths and customer compliance posture in centralized logs."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 594 - 646, The test TestRunComplianceChecks_PanicVerboseLogging currently doesn't assert that debug.Stack() output is emitted at verbose level; replace the plain verboseLogger with a captured logger (use bufferLogger() or similar test helper that returns a logger plus captured buffer), ensure the logger still reports IsVerbose(), call registry.RunComplianceChecks as before, then inspect the captured output buffer to assert it contains the stack-trace signature (e.g., "goroutine" or other debug.Stack() text) when verboseLogger.IsVerbose() is true; keep the existing isolation assertions (result.Findings and PluginInfo) as well.
108-110: 🧹 Nitpick | 🔵 TrivialMinor inconsistency: mixed
Error()and.Erraccessors.Line 108 uses
result.Failures[0].Error()for the assertion while line 109 logsresult.Failures[0].Err. Using the same accessor in both places improves readability.♻️ Suggested consistency fix
if !strings.Contains(result.Failures[0].Error(), "symlink") { - t.Errorf("expected 'symlink' in failure error, got: %v", result.Failures[0].Err) + t.Errorf("expected 'symlink' in failure error, got: %s", result.Failures[0].Error()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 108 - 110, The test mixes two different accessors for the first failure (using result.Failures[0].Error() in the assertion but result.Failures[0].Err in the error message); make them consistent by using the same accessor for both the contains check and the t.Errorf output (e.g., replace result.Failures[0].Err with result.Failures[0].Error() or vice versa) in the test that inspects result.Failures[0] so both the assertion and logged value refer to the same property.
425-477:⚠️ Potential issue | 🟡 MinorDon't skip platform-neutral preflight tests on Windows.
TestRunPluginPreflight_Unitskips entirely on Windows, but the "relative path is rejected" and "missing file is rejected" subtests exercise cross-platform code paths that don't depend on POSIX permission semantics. Only the POSIX-specific permission checks (not exercised in this test) should be gated.🛠️ Move Windows skip inside POSIX-only subtests
func TestRunPluginPreflight_Unit(t *testing.T) { t.Parallel() - if runtime.GOOS == goosWindows { - t.Skip("POSIX permission semantics required") - } - dir := t.TempDir() goodPath := writePluginFile(t, dir, "good.so", 0o600, []byte("payload")) t.Run("accepted plugin returns sha256", func(t *testing.T) { t.Parallel() + if runtime.GOOS == goosWindows { + t.Skip("POSIX permission semantics required for acceptance path") + } res := runPluginPreflight(goodPath) // ... rest unchanged }) t.Run("relative path is rejected", func(t *testing.T) { t.Parallel() + // This test is platform-neutral — runs on Windows too // ... unchanged }) t.Run("missing file is rejected", func(t *testing.T) { t.Parallel() + // This test is platform-neutral — runs on Windows too // ... unchanged }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 425 - 477, The test currently skips the entire TestRunPluginPreflight_Unit on Windows; instead, remove the top-level runtime.GOOS check and only skip the POSIX-permissions-dependent subtest(s). Concretely, keep the subtests for "relative path is rejected" and "missing file is rejected" running on all platforms, and move the runtime.GOOS == goosWindows t.Skip into the "accepted plugin returns sha256" subtest (the one that uses writePluginFile/goodPath and checks permission semantics) so that only that POSIX-specific case is skipped while runPluginPreflight is still exercised cross-platform.
201-214: 🧹 Nitpick | 🔵 TrivialNolint directive placement should be on separate line.
Per coding guidelines,
//nolint:directives should appear on a separate line above the call to avoid being stripped by gofumpt. Line 203 has the directive inline. Additionally, the//nolint:gosecon line 210 is unnecessary since0o700is a restrictive permission that doesn't trigger gosec G302.♻️ Fix nolint placement
// gosec G302: intentional world-writable chmod — the whole point of // this test is to exercise the preflight's rejection of that mode. - if err := os.Chmod(pluginDir, 0o777); err != nil { //nolint:gosec // intentional world-writable mode under test + //nolint:gosec // intentional world-writable mode under test + if err := os.Chmod(pluginDir, 0o777); err != nil { t.Fatalf("failed to chmod plugin dir to world-writable: %v", err) } t.Cleanup(func() { // Restore permissions so t.TempDir cleanup can remove the dir on // platforms where the test user cannot unlink files inside a // world-writable directory without write on the parent. - //nolint:gosec // 0o700 is the default t.TempDir mode; explicit restore. if err := os.Chmod(pluginDir, 0o700); err != nil { t.Logf("warning: failed to restore plugin dir permissions: %v", err) } })As per coding guidelines: "Place
//nolint:directives on SEPARATE LINE above call, not inline, to avoid stripping by gofumpt."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 201 - 214, Move the inline nolint for the world-writable chmod to its own line immediately above the os.Chmod(pluginDir, 0o777) call (e.g., add a line "// nolint:gosec // intentional world-writable mode under test") and remove the unnecessary "//nolint:gosec" that precedes the os.Chmod(pluginDir, 0o700) inside t.Cleanup (since 0o700 is restrictive and won't trigger G302); ensure both changes reference the os.Chmod calls around pluginDir and keep the t.Cleanup block unchanged otherwise.internal/audit/plugin_preflight.go (1)
113-117:⚠️ Potential issue | 🟠 MajorMissing rejection for non-regular files (FIFOs, sockets, device nodes).
After the symlink check passes, the preflight proceeds to permission checks and eventually
hashFileSizeCapped()without verifyinginfo.Mode().IsRegular(). Special files like FIFOs or device nodes pass the symlink check but can block indefinitely when opened for reading, causing the preflight to hang.🛡️ Add regular-file check after symlink rejection
if info.Mode()&os.ModeSymlink != 0 { res.reason = "plugin is a symlink" res.err = fmt.Errorf("refusing to load plugin symlink: %s", path) return res } + + if !info.Mode().IsRegular() { + res.reason = "plugin is not a regular file" + res.err = fmt.Errorf("refusing non-regular plugin file: %s", path) + return res + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_preflight.go` around lines 113 - 117, After rejecting symlinks, add a non-regular-file guard to avoid opening FIFOs/sockets/devices: check info.Mode().IsRegular() and if false set res.reason (e.g., "plugin is not a regular file") and res.err (e.g., fmt.Errorf("refusing to load non-regular file: %s", path)) and return res before calling hashFileSizeCapped() so only regular files proceed; update the same function in plugin_preflight.go where the symlink check currently lives and ensure the new branch mirrors the existing symlink-rejection pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/audit/plugin_hardening_test.go`:
- Around line 594-646: The test TestRunComplianceChecks_PanicVerboseLogging
currently doesn't assert that debug.Stack() output is emitted at verbose level;
replace the plain verboseLogger with a captured logger (use bufferLogger() or
similar test helper that returns a logger plus captured buffer), ensure the
logger still reports IsVerbose(), call registry.RunComplianceChecks as before,
then inspect the captured output buffer to assert it contains the stack-trace
signature (e.g., "goroutine" or other debug.Stack() text) when
verboseLogger.IsVerbose() is true; keep the existing isolation assertions
(result.Findings and PluginInfo) as well.
- Around line 108-110: The test mixes two different accessors for the first
failure (using result.Failures[0].Error() in the assertion but
result.Failures[0].Err in the error message); make them consistent by using the
same accessor for both the contains check and the t.Errorf output (e.g., replace
result.Failures[0].Err with result.Failures[0].Error() or vice versa) in the
test that inspects result.Failures[0] so both the assertion and logged value
refer to the same property.
- Around line 425-477: The test currently skips the entire
TestRunPluginPreflight_Unit on Windows; instead, remove the top-level
runtime.GOOS check and only skip the POSIX-permissions-dependent subtest(s).
Concretely, keep the subtests for "relative path is rejected" and "missing file
is rejected" running on all platforms, and move the runtime.GOOS == goosWindows
t.Skip into the "accepted plugin returns sha256" subtest (the one that uses
writePluginFile/goodPath and checks permission semantics) so that only that
POSIX-specific case is skipped while runPluginPreflight is still exercised
cross-platform.
- Around line 201-214: Move the inline nolint for the world-writable chmod to
its own line immediately above the os.Chmod(pluginDir, 0o777) call (e.g., add a
line "// nolint:gosec // intentional world-writable mode under test") and remove
the unnecessary "//nolint:gosec" that precedes the os.Chmod(pluginDir, 0o700)
inside t.Cleanup (since 0o700 is restrictive and won't trigger G302); ensure
both changes reference the os.Chmod calls around pluginDir and keep the
t.Cleanup block unchanged otherwise.
In `@internal/audit/plugin_preflight.go`:
- Around line 113-117: After rejecting symlinks, add a non-regular-file guard to
avoid opening FIFOs/sockets/devices: check info.Mode().IsRegular() and if false
set res.reason (e.g., "plugin is not a regular file") and res.err (e.g.,
fmt.Errorf("refusing to load non-regular file: %s", path)) and return res before
calling hashFileSizeCapped() so only regular files proceed; update the same
function in plugin_preflight.go where the symlink check currently lives and
ensure the new branch mirrors the existing symlink-rejection pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca5fea03-ed8e-472c-9d32-da3d2f5c2cfc
📒 Files selected for processing (4)
internal/audit/plugin_hardening_test.gointernal/audit/plugin_preflight.gointernal/audit/plugin_preflight_posix.gointernal/audit/plugin_preflight_windows.go
…tions
The previous Dynamic Plugin Security section in docs/user-guide/commands/
audit.md mixed developer commentary ("Phase A preflight", "Phase B
hardening") with operator guidance. Rewrite it as user-facing security
documentation organized around what the loader enforces, what it does
not enforce, and what operator responsibilities remain:
- Trust-model callout escalated from warning to danger, framed around the
"unsigned executable" analogy so risk is immediate.
- "What the loader does for you" table enumerates every automatic
restriction (symlink rejection, file/directory permission checks,
absolute-path normalization, 64 MiB size cap, SHA-256 audit trail)
with platform applicability.
- "What the loader does NOT do" section honestly discloses the gaps:
no signature verification, no sandboxing, no capability restriction,
no remote fetch.
- "Operator responsibilities" translates those gaps into actionable
guidance (own the directory, vet source, pin toolchain, review audit
log, keep CI out).
- "Threat scenarios" tables map concrete attacker moves to the specific
check that blocks them (or the mitigation they require).
docs/user-guide/configuration-reference.md tightened the one-line
--plugin-dir entry to summarize the automatic restrictions and link to
the full section rather than repeating "full process privileges" twice.
No behavioral change; documentation only.
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
type:feature
…tweak Platform support for Go plugins: - Go's plugin package is only implemented on Linux, macOS, and FreeBSD. On Windows/other platforms, plugin.Open returns "plugin: not implemented" at runtime, so previously a Windows user with --plugin-dir would see one load failure per .so file in the audit log. - internal/audit/plugin.go: add pluginLoadingSupported() guard. When the current GOOS is not linux/darwin/freebsd, LoadDynamicPlugins now short-circuits with a single stderr warning naming the platform and returns an empty LoadResult with nil error. --plugin-dir becomes a clean no-op instead of producing spurious per-file failures. - internal/audit/plugin_hardening_test.go: two new tests — TestPluginLoadingSupported_ReportsCurrentPlatform asserts the invariant against runtime.GOOS; TestLoadDynamicPlugins_UnsupportedPlatformShortCircuits covers the no-op path (skipped when running on a supported platform). Docs updated to reflect reality: - docs/user-guide/commands/audit.md: add platform-support info callout at the top of Dynamic Plugin Security; drop the per-row Platforms column from the preflight-restrictions table since every listed check applies uniformly on all supported platforms now; describe the Windows/unsupported behavior as an explicit no-op rather than a runtime failure. - docs/user-guide/configuration-reference.md: --plugin-dir one-liner now leads with "Linux/macOS/FreeBSD only". - docs/development/plugin-development.md: correct the troubleshooting entry that previously said "Linux and macOS, not Windows" — FreeBSD is also supported; WSL2/Linux containers are the Windows workaround. Mergify: - .mergify.yml: drop codecov/project check from all 3 queue success_conditions. codecov/patch remains as the enforced gate for per-PR diff coverage; the project-wide codecov/project check is redundant with the existing coverage-gate Coverage job and has been flagged as unreliable. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/user-guide/commands/audit.md (1)
103-103:⚠️ Potential issue | 🟡 MinorUse "absolute path" instead of "canonical path."
Line 103 states "so the audit log and preflight always work on a canonical path," but the implementation calls
filepath.Abs()(seeinternal/audit/plugin.golines 147-155), which normalizes to an absolute path without resolving symlinks. Calling it "canonical" overstates the guarantee and incorrectly implies symlink resolution (which would requirefilepath.EvalSymlinks). As per coding guidelines, documentation must be technically accurate.📝 Proposed fix
-| **Absolute path normalization** | A relative `--plugin-dir` (e.g. `./plugins`) is resolved to an absolute path before any check runs, so the audit log and preflight always work on a canonical path. | All platforms | +| **Absolute path normalization** | A relative `--plugin-dir` (e.g. `./plugins`) is resolved to an absolute path before any check runs, so the audit log and preflight always work on an absolute path. | All platforms |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide/commands/audit.md` at line 103, Replace the phrase "canonical path" with "absolute path" in the audit documentation where it describes --plugin-dir normalization; the implementation uses filepath.Abs() (not filepath.EvalSymlinks()), so update the wording to accurately reflect that the path is converted to an absolute path without claiming symlink resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/user-guide/commands/audit.md`:
- Line 103: Replace the phrase "canonical path" with "absolute path" in the
audit documentation where it describes --plugin-dir normalization; the
implementation uses filepath.Abs() (not filepath.EvalSymlinks()), so update the
wording to accurately reflect that the path is converted to an absolute path
without claiming symlink resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85017507-cdc5-47a6-bdc3-b0794becbe30
📒 Files selected for processing (2)
docs/user-guide/commands/audit.mddocs/user-guide/configuration-reference.md
End users could reasonably read the previous "Dynamic Plugins" language as implying --plugin-dir toggles the built-in stig/sans/firewall plugins. It does not. Built-ins are compiled into the opnDossier binary and are always available; --plugin-dir is strictly a third-party extension point. Make that distinction explicit everywhere the flag is mentioned. docs/user-guide/commands/audit.md: - Rename Dynamic Plugins section header framing to split "Built-in plugins" (the stig/sans/firewall table) from "Third-party dynamic plugins" (--plugin-dir) under a single Compliance Plugins heading. - Lead each subsection with a one-sentence reminder of what --plugin-dir does and does not affect. - Rename Dynamic Plugin Security → Third-Party Plugin Security and open with an explicit scoping note: the restrictions apply only to third-party .so plugins, not the built-ins. - Document the additive behavior: third-party plugins augment the built-in set, do not replace it, and same-name registration fails. - Flag table row for --plugin-dir now says "third-party" and parenthetically excludes the built-ins. docs/user-guide/configuration-reference.md: - Leading sentence of the --plugin-dir row now calls out "third-party" and explicitly mentions the built-ins are unaffected. README.md: - Trust-model subsection renamed and rewritten to lead with the built-in-vs-third-party distinction. Anchor link updated to #third-party-plugin-security. cmd/shared_flags.go: - pluginDirFlagUsage (the --help text surfaced on every command that accepts --plugin-dir) updated to say "third-party" and mention the built-in exclusion and the Windows no-op. Existing test assertions on the "full process privileges", "signatures are not verified", and "GOTCHAS §2.5" substrings continue to hold. - docs/cli/opnDossier_audit.md regenerated from the new flag text. docs/security/security-assurance.md: - Cross-reference to audit.md anchor updated to the new section name. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
go
Plugin preflight hardening: - Reject non-regular files (FIFOs, sockets, device nodes) in runPluginPreflight before hashFileSizeCapped would block indefinitely on os.Open/io.CopyN. New info.Mode().IsRegular() check fires after symlink rejection on every platform. - Correct pluginMaxSize comment: the cap bounds preflight I/O and CPU time, not memory — hashFileSizeCapped streams into the SHA-256 hasher rather than buffering. - Update runPluginPreflight doc-list to reflect the six-step sequence (was five; non-regular check inserted at step 3). Tests (internal/audit/plugin_hardening_test.go): - TestRunPluginPreflight_Unit no longer skips platform-neutral subtests on Windows. The POSIX permission-bit cases stay gated; the accepted/relative-path/missing-file/non-regular subtests now run cross-platform. - Add "non-regular file is rejected" subtest that uses a directory named *.so (portable, no syscall.Mkfifo) to exercise the new preflight check. - TestRunComplianceChecks_PanicVerboseLogging now uses bufferLogger and asserts the captured panic-recovery output contains the stack trace plus the "goroutine" marker, so the IsVerbose() gate cannot be silently removed by a future refactor. - Fix failure-accessor inconsistency in TestLoadDynamicPlugins_RejectsSymlink (`.Error()` for both the assertion and the log). - Move inline //nolint:gosec directives to separate lines above the chmod calls in TestLoadDynamicPlugins_RejectsWorldWritableContainerDir per gofumpt-survival guideline, and add a nolint on the 0o700 cleanup chmod (gosec G302 flags > 0o600 regardless of owner-only intent). Sanitizer reflection: - SanitizeStruct's map-element guard now also inspects interface-typed maps (map[string]any and friends). If the first entry's concrete dynamic kind is Struct or Ptr, emit the existing skip warning so the gap is surfaced at runtime instead of shipping as cleartext. - Test pins the exact redacted placeholder "[REDACTED-PASSWORD]" rather than an inequality-only check so partial/malformed redactions can no longer sneak past. Parser factory: - Tighten peekRootElementBounded cancellation-contract comment: the ctx-wrapped reader returns ctx.Err() on a *subsequent* Read call after the underlying blocked Read has returned; it does not interrupt an already-blocked Read. Previous wording implied the latter. - Drop the misleading "see GOTCHAS.md §1.1" cross-reference from the cancellation-contract comment. §1.1 is about cmd/ global-flag races, not about this parser factory. pfSense parser tests: - Remove stale "each writer just increments the shared counter" comment in TestParser_SetValidator_ConcurrentWritersDoNotRace — the test no longer uses a counter. - Remove t.Parallel() from TestParser_ParseAndValidate so it does not race against TestParser_ParseAndValidateWithValidator and TestParser_SetValidator_StompProtection, both of which mutate the process-wide validator holder. Add a doc comment explaining why. Documentation: - CHANGELOG: Phase 5 Security entry now says pfsense.ValidateFunc (public var) was removed and replaced by pfsense.SetValidator (sync.Once + atomic.Pointer), not "renamed to unexported validateFunc" (the actual implementation has a holder + load helpers, no single variable). - docs/security/security-assurance.md AV-9: validator-stomp occurs during plugin.Open() load (when init() fires), not "before preflight" — runPluginPreflight runs before plugin.Open in LoadDynamicPlugins. - GOTCHAS.md §2.5 Phase A bullets: absolute-path check is defense-in-depth at the preflight boundary; relative --plugin-dir inputs are accepted and normalized via filepath.Abs before preflight runs. Add the new non-regular-file bullet. Correct the SHA-256-cap rationale to time/throughput rather than memory. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
♻️ Duplicate comments (6)
docs/security/security-assurance.md (1)
32-32:⚠️ Potential issue | 🟡 MinorFix AV-9 ordering:
init()is not “before preflight.”Line 32 and Line 46 currently invert loader order. Preflight runs before
plugin.Open();init()risk is during plugin load/initialization after preflight acceptance.Suggested wording update
-| Malicious plugin author | Stomp validator, exfiltrate credentials, inject backdoor via plugin init() before preflight | Can supply .so plugin files when --plugin-dir is enabled | +| Malicious plugin author | Stomp validator, exfiltrate credentials, inject backdoor during plugin load/init() after preflight acceptance | Can supply .so plugin files when --plugin-dir is enabled | -| AV-9 | Malicious plugin init() stomps validator before preflight | SR-4 | +| AV-9 | Malicious plugin init() stomps validator during plugin load (post-preflight) | SR-4 |As per coding guidelines, "Documentation: Ensure clarity, accuracy, and completeness."
Also applies to: 46-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/security-assurance.md` at line 32, Update the wording that incorrectly states init() occurs “before preflight”: change the entry so it reflects that preflight runs before plugin.Open(), and the init() risk happens during plugin load/initialization (i.e., when plugin.Open() runs) after preflight acceptance; apply this correction to the two affected table rows so references to init(), plugin.Open(), and “preflight” show the correct ordering.internal/audit/plugin.go (1)
179-190:⚠️ Potential issue | 🟠 MajorPrevent implicit CWD plugin discovery when
diris empty.Line 183 normalizes before any empty-path guard.
filepath.Abs("")resolves to the current working directory, which can turn an empty plugin dir into unintended.sodiscovery.Suggested fix
func (pr *PluginRegistry) LoadDynamicPlugins( ctx context.Context, dir string, explicitDir bool, logger *logging.Logger, ) (LoadResult, error) { @@ ctxLogger := logger.WithContext(ctx) + + if strings.TrimSpace(dir) == "" { + if explicitDir { + return LoadResult{}, errors.New("empty plugin directory") + } + ctxLogger.Debug("Dynamic plugin directory is empty; skipping load") + return LoadResult{}, nil + } // Normalize dir to an absolute path up front. The preflight rejects // non-absolute paths, so a common invocation like `--plugin-dir ./plugins`Based on learnings:
PluginRegistry.LoadDynamicPluginsusesplugin.Open()with full process privileges, and loading is opt-in via explicit--plugin-dir(no auto-discovery fallback).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin.go` around lines 179 - 190, The code calls filepath.Abs(dir) before checking for an empty dir, which makes an empty string resolve to the current working directory and can cause unintended plugin discovery; add an early guard in LoadDynamicPlugins (or the function containing filepath.Abs) that checks if dir == "" and returns early (either a no-op LoadResult{} with nil error or a clear error/log entry via ctxLogger.Error/Info consistent with the project's opt-in policy) so filepath.Abs and subsequent plugin.Open logic never run for an empty path.docs/user-guide/commands/audit.md (1)
113-113:⚠️ Potential issue | 🟡 MinorUse “absolute path” here, not “canonical path.”
Line 113 overstates the guarantee. The loader normalizes with absolute-path resolution; that should not imply canonicalization.
Suggested fix
-| **Absolute path normalization** | A relative `--plugin-dir` (e.g. `./plugins`) is resolved to an absolute path before any check runs, so the audit log and preflight always work on a canonical path. | +| **Absolute path normalization** | A relative `--plugin-dir` (e.g. `./plugins`) is resolved to an absolute path before any check runs, so the audit log and preflight always work on an absolute path. |As per coding guidelines, "Documentation: Ensure clarity, accuracy, and completeness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide/commands/audit.md` at line 113, Update the wording in the table row titled "Absolute path normalization" so it uses "absolute path" instead of "canonical path" and does not imply filesystem canonicalization; specifically change the sentence that currently ends with "so the audit log and preflight always work on a canonical path" to mention "absolute path" (or similar phrasing) and remove any suggestion that the loader performs canonicalization beyond resolving a relative --plugin-dir to an absolute path.internal/audit/plugin_hardening_test.go (3)
203-211:⚠️ Potential issue | 🟡 MinorMove
//nolint:gosecabove theos.Chmodcall and drop it from cleanup.The inline directive can be stripped by
gofumpt, and the0o700restore path does not need suppression.As per coding guidelines: "Place
//nolint:directives on SEPARATE LINE above call, not inline, to avoid stripping by gofumpt."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 203 - 211, The //nolint:gosec directive should be on its own line above the intentional world-writable os.Chmod call and removed from the cleanup restore call; edit the code around the os.Chmod(pluginDir, 0o777) in internal/audit/plugin_hardening_test.go so the nolint comment sits on the line immediately above that os.Chmod invocation (not inline), and delete the redundant //nolint:gosec on the os.Chmod(pluginDir, 0o700) inside the t.Cleanup block; keep the rest of the chmod logic and error handling for pluginDir and t.Cleanup unchanged.
597-646:⚠️ Potential issue | 🟡 MinorAssert the verbose panic diagnostics, not just panic isolation.
This still passes if the
IsVerbose()branch stops emitting the stack/diagnostic entirely. UsebufferLogger()here and assert the captured output contains the verbose-only panic log.As per coding guidelines: "
debug.Stack()dumps must be gated behindlogger.IsVerbose()to prevent leakage of internal plugin paths and customer compliance posture in centralized logs (SEC-M5/QUAL-M8)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 597 - 646, Test currently verifies only panic isolation; change the logger to a captured buffer and assert verbose diagnostic output is emitted when verboseLogger.IsVerbose() is true. Replace logging.New(...) with bufferLogger() (or construct a logger that returns true for IsVerbose and writes to a buffer), pass that buffer-backed verboseLogger into registry.RunComplianceChecks(device, []string{"panicking-plugin-verbose"}, verboseLogger), then assert the buffer contains the verbose-only panic diagnostics/stack trace (e.g., "panic" or stack dump substring) in addition to the existing checks that findings are zero and PluginInfo for "panicking-plugin-verbose" contains "panicked".
425-430:⚠️ Potential issue | 🟡 MinorDon't skip the platform-neutral preflight cases on Windows.
Only the POSIX writable-bit branches are Windows-specific here. Skipping the whole test drops coverage for the relative-path and missing-file contract.
Based on learnings: "On Windows (POSIX permission check skipped), writable-mode and writable-dir rejections are skipped at runtime when
runtime.GOOS == "windows". The symlink and absolute-path checks still run."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/audit/plugin_hardening_test.go` around lines 425 - 430, The test TestRunPluginPreflight_Unit currently skips entirely on Windows via runtime.GOOS == goosWindows; instead, only skip or conditionally execute the POSIX-specific writable-bit branches inside that test. Modify TestRunPluginPreflight_Unit to run platform-neutral subtests (e.g., relative-path, missing-file, symlink, absolute-path checks) unconditionally and guard only the POSIX permission cases with runtime.GOOS == goosWindows or by using t.Run subtests that return early on Windows; update the branches that reference writable-mode/writable-dir so they are executed only when runtime.GOOS != goosWindows while leaving the other checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/audit/plugin_hardening_test.go`:
- Around line 247-285: The tests (e.g.,
TestLoadDynamicPlugins_NormalizesRelativeDir) assume dynamic plugin loading is
available but fail on unsupported platforms; update each affected test to
early-skip when pluginLoadingSupported() is false (call pluginLoadingSupported()
at the top of the test and t.Skipf("dynamic plugin loading not supported on this
platform") if it returns false) so the expectations about
preflight/os.ReadDir/injected loader are only executed on platforms that support
LoadDynamicPlugins; apply the same guard to the other tests referenced (lines
~295-323, ~332-383, ~566-584) that exercise LoadDynamicPlugins.
---
Duplicate comments:
In `@docs/security/security-assurance.md`:
- Line 32: Update the wording that incorrectly states init() occurs “before
preflight”: change the entry so it reflects that preflight runs before
plugin.Open(), and the init() risk happens during plugin load/initialization
(i.e., when plugin.Open() runs) after preflight acceptance; apply this
correction to the two affected table rows so references to init(),
plugin.Open(), and “preflight” show the correct ordering.
In `@docs/user-guide/commands/audit.md`:
- Line 113: Update the wording in the table row titled "Absolute path
normalization" so it uses "absolute path" instead of "canonical path" and does
not imply filesystem canonicalization; specifically change the sentence that
currently ends with "so the audit log and preflight always work on a canonical
path" to mention "absolute path" (or similar phrasing) and remove any suggestion
that the loader performs canonicalization beyond resolving a relative
--plugin-dir to an absolute path.
In `@internal/audit/plugin_hardening_test.go`:
- Around line 203-211: The //nolint:gosec directive should be on its own line
above the intentional world-writable os.Chmod call and removed from the cleanup
restore call; edit the code around the os.Chmod(pluginDir, 0o777) in
internal/audit/plugin_hardening_test.go so the nolint comment sits on the line
immediately above that os.Chmod invocation (not inline), and delete the
redundant //nolint:gosec on the os.Chmod(pluginDir, 0o700) inside the t.Cleanup
block; keep the rest of the chmod logic and error handling for pluginDir and
t.Cleanup unchanged.
- Around line 597-646: Test currently verifies only panic isolation; change the
logger to a captured buffer and assert verbose diagnostic output is emitted when
verboseLogger.IsVerbose() is true. Replace logging.New(...) with bufferLogger()
(or construct a logger that returns true for IsVerbose and writes to a buffer),
pass that buffer-backed verboseLogger into registry.RunComplianceChecks(device,
[]string{"panicking-plugin-verbose"}, verboseLogger), then assert the buffer
contains the verbose-only panic diagnostics/stack trace (e.g., "panic" or stack
dump substring) in addition to the existing checks that findings are zero and
PluginInfo for "panicking-plugin-verbose" contains "panicked".
- Around line 425-430: The test TestRunPluginPreflight_Unit currently skips
entirely on Windows via runtime.GOOS == goosWindows; instead, only skip or
conditionally execute the POSIX-specific writable-bit branches inside that test.
Modify TestRunPluginPreflight_Unit to run platform-neutral subtests (e.g.,
relative-path, missing-file, symlink, absolute-path checks) unconditionally and
guard only the POSIX permission cases with runtime.GOOS == goosWindows or by
using t.Run subtests that return early on Windows; update the branches that
reference writable-mode/writable-dir so they are executed only when runtime.GOOS
!= goosWindows while leaving the other checks intact.
In `@internal/audit/plugin.go`:
- Around line 179-190: The code calls filepath.Abs(dir) before checking for an
empty dir, which makes an empty string resolve to the current working directory
and can cause unintended plugin discovery; add an early guard in
LoadDynamicPlugins (or the function containing filepath.Abs) that checks if dir
== "" and returns early (either a no-op LoadResult{} with nil error or a clear
error/log entry via ctxLogger.Error/Info consistent with the project's opt-in
policy) so filepath.Abs and subsequent plugin.Open logic never run for an empty
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6ac7514-217f-4004-89a0-98485bf5ab08
📒 Files selected for processing (10)
.mergify.ymlREADME.mdcmd/shared_flags.godocs/cli/opnDossier_audit.mddocs/development/plugin-development.mddocs/security/security-assurance.mddocs/user-guide/commands/audit.mddocs/user-guide/configuration-reference.mdinternal/audit/plugin.gointernal/audit/plugin_hardening_test.go
💤 Files with no reviewable changes (1)
- .mergify.yml
unclesp1d3r
added a commit
that referenced
this pull request
Apr 20, 2026
…sed by squash The Phase 6-8 squash in the previous commit was a snapshot taken before the Phase 5 review-feedback improvements landed in main (via #587 squash). Several files regressed to pre-feedback wording and were missing the non-regular-file preflight check, the Windows no-op guard pointer (pluginOwnerUIDUnavailable const), and the third-party-plugin doc rewrite. Restore the affected files from origin/main so the Phase 6-8 branch carries Phase 5's current state plus Phase 6-8's code changes, not a mixture of pre-feedback docs and post-feedback code. Files restored wholesale from origin/main (doc-only regressions with no Phase 6-8 source changes to merge): - GOTCHAS.md — §2.5 Phase A bullets (non-regular file check, normalized absolute paths, SHA-256 I/O/CPU cap rationale) - docs/security/security-assurance.md — threat-actor and AV-9 updates (validator stomp during plugin.Open load, not before preflight) - docs/user-guide/configuration-reference.md — --plugin-dir Linux/macOS/FreeBSD + third-party framing - internal/audit/plugin_preflight.go — IsRegular() check, 6-step doc list, corrected pluginMaxSize rationale - internal/audit/plugin_preflight_posix.go — pluginOwnerUIDUnavailable constant use - internal/audit/plugin_preflight_windows.go — same constant reference Files hand-merged (Phase 6-8 changed source + Phase 5 changed wording): - cmd/shared_flags.go — kept Phase 7's audit-flag global removal and NewPluginManager(logger, nil) 2-arg call, plus the updated --include-tunables help; restored Phase 5's third-party wording, Linux/macOS/FreeBSD caveat, and Third-Party Plugin Security anchor - pkg/parser/factory.go — kept Phase 7's ctx propagation; restored Phase 5's tightened cancellation comment (ctx-wrapped reader returns ctx.Err() on a *subsequent* Read, not on a currently-blocked Read; dropped misleading AGENTS.md §1.1 cross-reference) No behavior change relative to Phase 5 + Phase 6-8 combined intent; this commit just unwinds a snapshot regression. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the sanitizer, plugin loader, and parser-injection surface before v1.5 tags. Closes a sanitizer gap that leaked OpenVPN TLS-auth keys, protects the pfSense validator injection point from malicious plugin
init()stomp, adds a preflight pass in front ofplugin.Open(), and disclosures the dynamic-plugin trust model honestly to operators.Impact: 35 files changed, +2,190 / -153. Most growth is in new test files (plugin hardening, race detection, preflight scenarios) and new implementation files for the preflight and POSIX/Windows splits.
Risk: 🟠 High-to-medium — one breaking public-API change (
pfsense.ValidateFunc→pfsense.SetValidator, pre-v1.5 free window) and several security-sensitive code paths. All changes are additive or behaviorally backward-compatible for non-plugin invocations.Review time: ~45 minutes. Recommended review order:
internal/sanitizer/— OpenVPN redaction gap (highest-severity fix)pkg/parser/pfsense/parser.go+export_test.go— validator atomicity (breaking API)internal/audit/plugin_preflight.go+ POSIX/Windows split files — loader preflightcmd/shared_flags.go+cmd/audit.go+docs/user-guide/commands/audit.md— trust-model disclosureinternal/sanitizer/sanitizer.go+pkg/parser/factory.go+ audit callsites — reflect-map warning, cancellation contract, log-redaction gatesWhat Changed
OpenVPN TLS-auth key redaction
opnDossier sanitizenow redacts OpenVPN<tls>and<StaticKeys>XML elements. Path-anchored patterns (openvpn.tls,openvpn.statickeys) identify the real OpenVPN paths without colliding with unrelated<tls>tags in Suricata IDS and IPsec charon syslog configurations. A newIsOpenVPNStaticKeyvalue detector matches the OpenVPN static-key PEM envelope (-----BEGIN OpenVPN Static key V1-----). Previously, operators running sanitize on configs containing OpenVPN TLS-auth material leaked the raw HMAC keys to stdout. Regression coverage: 4 fixtures × 3 sanitize modes plus 2 false-positive cases.pfSense validator injection, atomic one-shot
pkg/parser/pfsense.ValidateFunc(exportedvar) is removed and replaced bypkg/parser/pfsense.SetValidator, guarded bysync.Onceover anatomic.Pointer. The public API is now:Second and subsequent
SetValidatorcalls are silently dropped. This closes a data race between plugininit()functions andParseAndValidatereaders under-race, and — intentionally — prevents a malicious dynamic plugin from stomping the validator at package-init time.ResetValidatorForTestinglives inexport_test.goso it stays out of the public API; production builds cannot reach it. Matches the injection discipline the OPNsense parser already has viaXMLDecoder.ParseAndValidate.This is a breaking public-API change under the pre-v1.5 free window. Downstream consumers must migrate
ValidateFunc = fn→SetValidator(fn).Plugin loader preflight + structured audit log
Before
plugin.Open()runs on any candidate.so, a newrunPluginPreflightfunction enforces:LoadDynamicPluginsnormalizes--plugin-dirviafilepath.Absat function entry, so--plugin-dir ./pluginsworks correctly and the audit log records the canonical path.os.Lstat).plugin.Openfollows symlinks, which would bypass the remaining checks against the link target.FileMode.Perm() & 0o022).hashFileSizeCapped). Files exceeding the cap are rejected rather than silently truncated.Every load attempt emits a structured audit log entry — INFO for accepted, WARN for rejected — carrying
plugin,path,sha256,mode,owner_uid,mtime,size_bytes,verdict, andreason. Operators gain a forensic trail even whenplugin.Opensubsequently fails.POSIX and Windows implementations are split via build tags:
internal/audit/plugin_preflight_posix.go(//go:build !windows) — full permission-bit enforcement viasyscall.Stat_t, owner UID extraction viaStat_t.Uidinternal/audit/plugin_preflight_windows.go(//go:build windows) — permission bits are skipped (NTFS permissions do not map onto POSIXFileMode.Perm()bits); symlink and absolute-path checks still run;owner_uid=unavailablein the audit log--plugin-dirtrust-model disclosure--plugin-dirhelp text is expanded and now reads:At invocation time,
audit PreRunEemits a matching stderr warning whenever--plugin-diris non-empty, via a sharedwarnPluginDirTrustModel(io.Writer, string)helper so every command that accepts--plugin-dirstays aligned. README and the user-guide audit command doc now carry a "Dynamic Plugin Security" subsection that documents the threat model (full process privileges, no signature verification, opt-in only) alongside the preflight checks listed above.Reflect-map redaction warning
internal/sanitizer/sanitizer.goaddsSanitizer.SetLoggerplus a reflection-path warning emitted fromSanitizeStructwhenever the walker encounters amap[K]struct{...}ormap[K]*struct{...}value. Go disallows in-place mutation of map-struct values, so the walker has always skipped them silently. The warning surfaces the gap so a future schema routing secrets through a struct-valued map is detected at runtime rather than shipped as cleartext. The raw-XMLSanitizeXMLpath is unaffected. Full structural redaction of struct-valued maps will land separately under the tag-based-redaction refactor.Goroutine-leak contract on
peekRootElementBoundedpkg/parser/factory.go'speekRootElementBoundednow documents its cancellation contract in-file: the function returnsctx.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 wrapping*os.Filesee promptio.EOFand are unaffected. No safety-net watchdog was added —peekRootElementBoundeddoes not own the reader it receives, and closing a caller-owned reader would corrupt caller state. A 100 ms regression test against a hungio.Pipeexercises the contract.Log redaction gate
debug.Stack()dumps atinternal/audit/plugin.go,cmd/audit.go, andinternal/processor/processor.goare now gated behindlogger.IsVerbose(). Go stack traces include package paths, so unredacted stacks can leak customer-specific plugin identifiers (e.g.,acmecorp-pci-plugin.RunChecks) into centralized logs and expose compliance posture. Default log mode emits a structured error without the stack; verbose mode retains the full stack for debugging. Nil-safeIsVerbose()handles the panic-recovery path edge cases.Test Plan
just ci-checkpasses (lint + tests + race detector + integration)just test-race ./pkg/parser/pfsense/...— validator atomicity pinned viaatomic.Pointer; new concurrent-writer race testjust test-race ./internal/audit/...— plugin loader preflight exercises concurrent loadGOOS=linux go build ./internal/audit/...andGOOS=windows go build ./internal/audit/...both succeedpre-commit run --all-filespassespkg/parser/pfsense—SetValidatorpresent,ValidateFuncremovedIsOpenVPNStaticKeyPEM-envelope matcherOut of Scope