Finding Severity Contract#

Note on Finding Type Location (as of PR #391): The canonical Finding struct is defined in internal/analysis/finding.go. The compliance.Finding type is now a type alias to analysis.Finding, unifying finding representations across audit, compliance, and processor packages. While the Finding type has been moved, the contract and field structure documented below remain consistent for backward compatibility.

The Finding Severity Contract is a breaking change in the opnDossier compliance plugin system that resolved a critical architectural bug where severity information could not be propagated from controls to findings. Implemented in PR #373, the contract introduced a new Severity field to the Finding struct and established standardized severity derivation patterns for both built-in and external plugins in the opnDossier audit framework.

The underlying issue manifested when audit reports displayed only total finding counts without severity breakdowns, despite severity information being properly defined on Control structs. Investigation revealed that all severity counters (CriticalFindings, HighFindings, MediumFindings, LowFindings) consistently returned zero due to the calculateSummary() function incorrectly switching on finding.Type instead of a non-existent finding.Severity field.

This contract defined the requirements for plugins to populate the new Severity field, established automatic normalization rules for backward compatibility with existing plugins, and specified migration requirements for external plugin developers. The change was intentionally breaking to ensure all findings contain valid severity information, preventing the silent failures that characterized the original bug.

Background#

The Severity Counting Bug#

The bug originated in the calculateSummary() function in internal/audit/plugin.go, which attempted to categorize findings by severity level but switched on finding.Type instead of a severity-specific field:

// Count findings by severity
for _, finding := range result.Findings {
    switch finding.Type { // BUG: Type is always "compliance", not severity
    case "critical":
        summary.CriticalFindings++
    case "high":
        summary.HighFindings++
    case "medium":
        summary.MediumFindings++
    case "low":
        summary.LowFindings++
    }
}

This logic failed because all production plugins set Type: "compliance" for every finding, never using severity values like "critical" or "high". Furthermore, the Finding struct (originally defined in internal/compliance/interfaces.go, moved to internal/analysis/finding.go in PR #391) lacked a Severity field entirely — severity information existed only on the Control struct and was never propagated to individual findings.

Semantic Distinction Between Type and Severity#

The confusion arose from overloading the Type field for multiple purposes. The two fields serve distinct semantic roles:

Type: Categorizes the finding type (e.g., "compliance", "inventory", "security", "configuration")
Severity: Indicates remediation priority (critical, high, medium, low, info)

By design, severity is defined at the Control level to ensure consistency across all findings associated with that control. However, the absence of a mechanism to propagate this information to findings created the counting bug.

Impact#

The severity counting bug had several practical consequences:

Audit reports displayed only total finding counts without severity breakdown
Security teams could not prioritize remediation by severity level
ComplianceSummary severity counters remained perpetually at zero
Compliance reports provided reduced value for risk assessment and prioritization

Contract Specification#

Canonical Finding Type (PR #391)#

As of PR #391, the Finding struct was moved to a new internal/analysis package to serve as the canonical type across audit, compliance, and processor modules:

// internal/analysis/finding.go
package analysis

// Severity represents the severity levels for findings.
type Severity string

const (
    SeverityCritical Severity = "critical"
    SeverityHigh Severity = "high"
    SeverityMedium Severity = "medium"
    SeverityLow Severity = "low"
    SeverityInfo Severity = "info"
)

type Finding struct {
    Type string `json:"type"`
    Severity string `json:"severity,omitempty"`
    Title string `json:"title"`
    Description string `json:"description"`
    Recommendation string `json:"recommendation"` // No omitempty
    Component string `json:"component"` // No omitempty
    Reference string `json:"reference"` // No omitempty
    References []string `json:"references,omitempty"`
    Tags []string `json:"tags,omitempty"`
    Metadata map[string]string `json:"metadata,omitempty"`
}

The compliance.Finding type is now a type alias:

// internal/compliance/interfaces.go
import "github.com/EvilBit-Labs/opnDossier/internal/analysis"

// Finding is a type alias for the canonical analysis.Finding type.
type Finding = analysis.Finding

This unification eliminates duplicate Finding definitions that previously existed in the compliance, processor, and audit packages. All modules now reference the same underlying struct, ensuring consistency.

Key Changes from Original PR #373 Definition:

Recommendation, Component, and Reference fields no longer use omitempty in the canonical struct
Severity is defined as a typed constant (analysis.Severity) rather than a plain string, with exported constants like analysis.SeverityCritical, analysis.SeverityHigh, etc.
The struct is documented to be shared across audit, compliance, and processor packages

New Finding.Severity Field#

The core of the contract is the addition of a Severity field to the Finding struct. As documented above, the canonical definition is in internal/analysis/finding.go (as of PR #391), but the field structure remains consistent with the original PR #373 implementation:

type Finding struct {
    Type string `json:"type"`
    Severity string `json:"severity,omitempty"` // Introduced in PR #373
    Title string `json:"title"`
    Description string `json:"description"`
    Recommendation string `json:"recommendation"`
    Component string `json:"component"`
    Reference string `json:"reference"`
    References []string `json:"references,omitempty"`
    Tags []string `json:"tags,omitempty"`
    Metadata map[string]string `json:"metadata,omitempty"`
}

The omitempty JSON tag on the Severity field maintains backward compatibility with existing JSON consumers. However, the contract mandates that all findings must have the Severity field populated with a valid value at runtime. In the canonical analysis.Finding, the Recommendation, Component, and Reference fields no longer use omitempty, which should be verified by downstream JSON consumers during migration.

Updated calculateSummary Logic#

The calculateSummary() function was modified in PR #373 to switch on strings.ToLower(finding.Severity) instead of finding.Type, correctly aggregating findings by severity level:

func (pr *PluginRegistry) calculateSummary(result *ComplianceResult) *ComplianceSummary {
    counts := countSeverities(result.Findings)

    summary := &ComplianceSummary{
        TotalFindings: len(result.Findings),
        CriticalFindings: counts.critical,
        HighFindings: counts.high,
        MediumFindings: counts.medium,
        LowFindings: counts.low,
        PluginCount: len(result.PluginInfo),
        Compliance: make(map[string]PluginCompliance),
    }

    return summary
}

The implementation uses a shared countSeverities() helper function that provides DRY severity counting across both aggregate summary and per-plugin summary paths (via computePerPluginSummary()). The use of strings.ToLower() provides case-insensitive matching, allowing for minor variations in severity string formatting. As of PR #510, countSeverities() includes support for the "info" severity level and tracks unrecognized severity values in an unknown counter.

Implementation Patterns#

Plugins can implement severity propagation using one of several established patterns. The choice depends on whether the plugin maintains a control registry and the desired balance between simplicity and maintainability.

Pattern 1: Direct Control Lookup#

The recommended pattern derives severity by looking up the control definition at the time of finding creation:

func (p *MyPlugin) RunChecks(device *common.CommonDevice) ([]compliance.Finding, []string, error) {
    var findings []compliance.Finding
    var evaluated []string

    if !isCompliant(device) {
        controlID := "MY-CONTROL-001"
        severity := "medium" // default fallback

        // Derive severity from control definition
        if ctrl, err := p.GetControlByID(controlID); err == nil {
            severity = ctrl.Severity
        }

        findings = append(findings, compliance.Finding{
            Type: "compliance",
            Severity: severity, // compliance.Finding uses analysis.Finding via type alias
            Title: "Non-compliant configuration",
            References: []string{controlID},
            // ... other fields
        })

        evaluated = append(evaluated, controlID)
    }
    return findings, evaluated, nil
}

This pattern ensures severity always matches the control definition, eliminating the risk of mismatches between control metadata and finding severity.

Pattern 2: Helper Function#

To reduce code duplication in plugins with multiple checks, a dedicated helper function can centralize severity lookup logic:

// Helper function to retrieve severity for a control ID
func (p *MyPlugin) controlSeverity(id string) string {
    if ctrl, err := p.GetControlByID(id); err == nil {
        return ctrl.Severity
    }
    return "medium" // fallback if control not found
}

func (p *MyPlugin) RunChecks(device *common.CommonDevice) ([]compliance.Finding, []string, error) {
    var findings []compliance.Finding
    var evaluated []string

    if !isCompliant(device) {
        controlID := "MY-CONTROL-001"
        findings = append(findings, compliance.Finding{
            Type: "compliance",
            Severity: p.controlSeverity(controlID),
            Title: "Non-compliant configuration",
            References: []string{controlID},
            // ... other fields
        })
        evaluated = append(evaluated, controlID)
    }
    return findings, evaluated, nil
}

This helper pattern is used by all three built-in plugins (Firewall, SANS, STIG) as of PR #373, ensuring consistent severity derivation from control metadata.

Pattern 3: Direct Assignment#

External plugins without a control registry may directly assign severity values:

findings = append(findings, compliance.Finding{
    Type: "compliance",
    Severity: "high", // Hard-coded severity
    // ... other fields
})

Advantages: Simple implementation, no control lookup infrastructure needed

Disadvantages:

Creates maintenance burden when control definitions change
Risk of severity inconsistency across plugin versions
Difficult to audit compliance posture across the codebase

This approach is not recommended for built-in plugins but remains valid for simple external plugins that don't maintain formal control registries.

Automatic Severity Derivation#

The contract includes automatic severity derivation logic implemented in PR #373 to maintain backward compatibility with plugins that don't explicitly set the Severity field.

Derivation Algorithm#

When RunComplianceChecks encounters a finding with an empty Severity field, it invokes deriveSeverityFromControl() which:

Checks whether finding.References or finding.Reference contains control IDs
Calls plugin.GetControlByID(controlID) to retrieve the control definition
Extracts control.Severity and assigns it to finding.Severity
Returns an error if no matching control is found

This normalization is intentionally strict. If severity cannot be derived, the audit fails with an error rather than silently defaulting to an empty severity value, which was the root cause of the original bug.

Implementation#

The deriveSeverityFromControl() function normalizes findings by deriving severity from control references:

Panic Recovery and Severity Handling#

As of PR #442, RunComplianceChecks wraps each plugin's RunChecks() call in a panic recovery boundary to prevent misbehaving plugins from crashing the entire audit process. This panic recovery mechanism interacts with severity validation in the following ways:

Panic Recovery Behavior:

Each plugin's RunChecks() is executed inside a defer recover() block
If a plugin panics, the panic is caught and logged via *slog.Logger
The panicking plugin is retained in the result with zero findings (findings remains nil)
The plugin appears in PluginFindings, PluginInfo, Compliance, and summary maps with zero findings

Severity Validation Exception:

Severity derivation and validation logic is never invoked for panicked plugins
Since panicked plugins return nil findings, there are no findings to normalize or validate
This is a special case distinct from plugins that return empty finding arrays ([]compliance.Finding{})

Key Invariant: Every selected plugin must appear in the audit result, even if it panicked during execution. Panicked plugins are not skipped or dropped from results; they are logged and represented with zero findings, allowing downstream consumers to see that the plugin was requested and evaluated but failed.

Contract Requirements for Dynamic Plugins#

This strict error handling enforces that dynamic plugins must either:

Explicitly set Severity on all findings (using string values like "high", "medium", or the typed constants from analysis.Severity), OR
Provide resolvable control references via References or Reference fields that map to controls with valid Severity values

Plugins that fail to meet either requirement will cause audits to fail with descriptive error messages, preventing the silent data loss that characterized the original bug. However, plugins that panic during RunChecks() do not trigger severity validation failures, as they produce no findings to validate.

Built-in Plugin Updates#

All three built-in plugins were updated in PR #373 to comply with the new contract using the controlSeverity() helper pattern:

Firewall Plugin (internal/plugins/firewall/firewall.go) - 8 controls (FIREWALL-001 through FIREWALL-008)
SANS Plugin (internal/plugins/sans/sans.go) - 4 controls (SANS-FW-001 through SANS-FW-004)
STIG Plugin (internal/plugins/stig/stig.go) - 4 controls (V-206xxx series)

Severity Distribution#

Each plugin implements controls with varying severity levels reflecting the criticality of different security requirements.

Firewall Plugin (severity distribution):

High (1): FIREWALL-008 (HTTPS Web Management)
Medium (5): FIREWALL-001 (SSH Banner), FIREWALL-002 (Auto Backup), FIREWALL-004 (Hostname), FIREWALL-005 (DNS), FIREWALL-006 (IPv6)
Low (2): FIREWALL-007 (DNS Rebind)
Info (5): FIREWALL-003 (MOTD), FIREWALL-026 (Disabled Rule Cleanup), FIREWALL-042 (Log Retention), FIREWALL-044 (Timezone), FIREWALL-060 (Configuration Revision Tracking) — reclassified from "low" to "info" in PR #510 as these are informational observations, not security failures
Inventory (2): FIREWALL-062 (DHCP Scope Inventory), FIREWALL-063 (Active Interface Summary) — introduced in PR #510 as Type: "inventory" findings excluded from compliance evaluation

SANS Plugin (severity distribution):

High (2): SANS-FW-001 (Default Deny), SANS-FW-003 (Network Segmentation)
Medium (2): SANS-FW-002 (Explicit Rules), SANS-FW-004 (Logging)

STIG Plugin (severity distribution):

High (2): V-206694 (Default Deny), V-206674 (Packet Filtering)
Medium (2): V-206690 (Services), V-206682 (Logging)

Migration Example#

The following example demonstrates the changes made to the STIG plugin in PR #373:

Before:

findings = append(findings, compliance.Finding{
    Type: "compliance", // No Severity field
    Title: "Missing Default Deny Policy",
    Description: "Firewall does not implement a default deny policy",
    References: []string{"V-206694"},
    // ...
})

After (PR #373):

findings = append(findings, compliance.Finding{
    Type: "compliance",
    Severity: p.controlSeverity("V-206694"), // NEW: Derive from control
    Title: "Missing Default Deny Policy",
    Description: "Firewall does not implement a default deny policy",
    References: []string{"V-206694"},
    // ...
})

After (PR #588 — single-pass signature):

func (sp *Plugin) RunChecks(device *common.CommonDevice) ([]compliance.Finding, []string, error) {
    findings := make([]compliance.Finding, 0, len(sp.controls))
    evaluated := make([]string, 0, len(sp.controls))

    // Pre-build evaluated with every control ID — STIG evaluates them all.
    for _, c := range sp.controls {
        evaluated = append(evaluated, c.ID)
    }

    if !sp.hasDefaultDenyPolicy(device) {
        findings = append(findings, compliance.Finding{
            Type: "compliance",
            Severity: sp.controlSeverity("V-206694"),
            Title: "Missing Default Deny Policy",
            Description: "Firewall does not implement a default deny policy",
            References: []string{"V-206694"},
            // ...
        })
    }

    return findings, evaluated, nil
}

The controlSeverity() helper method retrieves the severity from the control definition, ensuring consistency between control metadata and finding severity. PR #588 refactored the signature to return (findings, evaluated, error) in a single pass, eliminating the need for the separate EvaluatedControlIDs method.

External Plugin Migration#

Binary Compatibility#

Dynamic plugins loaded via Go's plugin system (.so files) face hard binary compatibility breaks when the Finding struct changes:

The Finding struct is part of the compliance.Plugin interface contract
Go's plugin system does not maintain binary compatibility across struct changes
External plugin developers must recompile against the updated Finding definition

PR #391 Note: The migration from compliance.Finding to analysis.Finding (with compliance.Finding as a type alias) represents another binary compatibility break. Plugins compiled against pre-PR #391 versions must be recompiled to work with the unified finding type.

Migration Options#

External plugin developers have three migration strategies, each with distinct tradeoffs:

Option 1: Direct Severity Assignment#

Simple plugins without formal control registries can hard-code severity values:

findings = append(findings, compliance.Finding{
    Type: "compliance",
    Severity: "high",
    Title: "Security violation detected",
    // ...
})

Advantages: Minimal implementation complexity, no control lookup infrastructure
Disadvantages: Manual maintenance when control severities change, potential inconsistencies

Option 2: Control Registry Implementation#

Sophisticated plugins should maintain control registries and derive severity systematically:

func (p *ExternalPlugin) GetControls() []compliance.Control {
    return []compliance.Control{
        {
            ID: "EXT-001",
            Title: "External Control",
            Severity: "high",
            Description: "...",
            // ... other fields
        },
    }
}

func (p *ExternalPlugin) RunChecks(device *common.CommonDevice) ([]compliance.Finding, []string, error) {
    var findings []compliance.Finding
    var evaluated []string

    if !isCompliant(device) {
        severity := "medium" // default fallback
        if ctrl, err := p.GetControlByID("EXT-001"); err == nil {
            severity = ctrl.Severity
        }

        findings = append(findings, compliance.Finding{
            Type: "compliance",
            Severity: severity,
            References: []string{"EXT-001"},
            // ...
        })

        evaluated = append(evaluated, "EXT-001")
    }
    return findings, evaluated, nil
}

Advantages: Consistent with built-in plugin architecture, easier compliance auditing
Disadvantages: Increased code complexity, requires control management infrastructure

Option 3: Automatic Derivation#

Plugins can omit Severity and rely on the audit engine's automatic derivation:

findings = append(findings, compliance.Finding{
    Type: "compliance",
    // Severity intentionally omitted
    References: []string{"EXT-001"},
    Title: "Configuration issue",
    // ...
})

evaluated = append(evaluated, "EXT-001")

Requirements:

Plugin must implement GetControlByID(id string) (*compliance.Control, error)
Control definitions must include valid Severity field
References or Reference field must contain resolvable control IDs

Failure Modes:

Severity contract violation: Audits fail with descriptive errors if severity cannot be derived, preventing silent data loss
Plugin panic: Plugins that panic during RunChecks() are caught by the panic recovery mechanism (PR #442), logged, and retained with zero findings — severity validation is not invoked for panicked plugins since they produce no findings

Validation and Testing#

Contract Validation Rules#

The Finding Severity Contract enforces the following validation rules at audit runtime:

Non-empty severity: All findings must have non-empty Severity after normalization
Standard severity values: Severity must be one of: "critical", "high", "medium", "low", "info" (case-insensitive comparison). As of PR #510, findings with unrecognized severity values are tracked in an unknown counter and logged as warnings via mode_controller.go. The system uses analysis.IsValidSeverity() for validation.
Derivation requirement: Findings with no Severity must have resolvable control references. The deriveSeverityFromControl() function validates severity values using IsValidSeverity and returns errors for unrecognized values rather than silently accepting them.
No hard-coding in built-in plugins: Built-in plugins must never hard-code severity literals in RunChecks() methods

Test Migration#

Test fixtures require updates to reflect the proper distinction between Type and Severity:

Incorrect Test Fixture (pre-contract):

finding := compliance.Finding{
    Type: "high", // WRONG: Type should be "compliance", not severity
    Title: "Test Finding",
}

Correct Test Fixture (post-contract):

finding := compliance.Finding{
    Type: "compliance", // Finding category
    Severity: "high", // Remediation priority
    Title: "Test Finding",
}

Tests must verify that calculateSummary() correctly counts findings by severity and that automatic derivation functions properly for plugins that don't explicitly set severity values.

Backward Compatibility#

JSON Serialization#

The omitempty tag on the Severity field enables gradual migration of JSON consumers:

{
  "type": "compliance",
  "severity": "high",
  "title": "Missing Default Deny Policy",
  "description": "...",
  "recommendation": "..."
}

Clients that don't recognize the severity field will ignore it without errors, while updated clients can immediately leverage severity information for prioritization and filtering.

PR #391 Note: The canonical analysis.Finding struct does not use omitempty for Recommendation, Component, and Reference fields. JSON consumers should be prepared to handle empty-string values for these fields, rather than the absence of keys.

Type Field Semantics#

The Type field retains its original semantic meaning as a finding category indicator:

Type = "compliance": Identifies this as a compliance-related finding (participates in compliance evaluation)
Type = "inventory": Identifies this as an informational observation (excluded from compliance evaluation entirely)
Severity = "high": Indicates this finding requires urgent remediation

This separation preserves semantic clarity and enables future expansion of finding types (e.g., "security", "performance", "configuration") without conflicting with severity levels. The unified analysis.Finding type (PR #391) supports this flexibility across all opnDossier modules.

Inventory Findings vs. Info Severity#

As documented in GOTCHAS.md §2.4, Type: "inventory" findings and Severity: "info" findings serve distinct purposes:

Info-severity compliance findings (e.g., Type: "compliance", Severity: "info") still participate in the compliance map — they can PASS or FAIL and affect compliance status. Examples include FIREWALL-003 (Message of the Day), FIREWALL-026 (Disabled Rule Cleanup), and FIREWALL-042 (Log Retention Configuration).
Inventory findings (e.g., Type: "inventory", Severity: "info") are excluded from the evaluated control ID list returned by RunChecks and never appear in the compliance map. They are rendered separately in a "Configuration Notes" section of audit reports. Examples include FIREWALL-062 (DHCP Scope Inventory) and FIREWALL-063 (Active Interface Summary).

The compliance flip in RunComplianceChecks skips inventory-type findings explicitly to prevent map pollution — their referenced controls are not in the evaluated slice, so the flip would be a no-op, but the skip is explicit for clarity. Severity never bypasses the compliance flip for non-inventory findings.

Phased Implementation#

The contract implementation was completed in PR #373 with the following phases:

✅ Complete: Add Severity field to Finding struct with omitempty JSON tag
✅ Complete: Modify calculateSummary() to switch on finding.Severity with shared countSeverities() helper
✅ Complete: Update all three built-in plugins to populate Severity via controlSeverity() helpers
✅ Complete: Implement automatic derivation logic in RunComplianceChecks via deriveSeverityFromControl()
✅ Complete: Add per-plugin severity breakdown via PluginFindings map and computePerPluginSummary()
✅ Complete: Update test fixtures and validation tests
Pending: Publish release notes with breaking change documentation for external plugin developers

The Finding Severity Contract addresses one component of broader compliance plugin system improvements:

Issue #309: No panic recovery around plugin RunChecks() calls — Resolved by PR #442, which adds panic recovery boundaries around each plugin's RunChecks() invocation
Issue #310: Severity breakdown missing in audit reports — Resolved by PR #373
Issue #311: Dynamic plugin load failures are silently swallowed — No visibility into plugin loading errors

These issues collectively represent an effort to harden the plugin system against failure modes that compromise audit reliability and observability.

Relevant Code Files#

File	Purpose	Key Changes
internal/analysis/finding.go	Canonical Finding and Severity types (PR #391)	Define unified `Finding` struct and `Severity` type with constants
internal/compliance/interfaces.go	Finding and Control struct definitions (pre-PR #391)	Add `Severity string` field to Finding (PR #373); migrate to type alias (PR #391)
internal/audit/plugin.go	Audit engine with calculateSummary	Switch on `finding.Severity` instead of `finding.Type`
internal/plugins/firewall/firewall.go	Firewall compliance plugin	Populate `Severity` in all findings
internal/plugins/sans/sans.go	SANS compliance plugin	Populate `Severity` in all findings
internal/plugins/stig/stig.go	DISA STIG compliance plugin	Populate `Severity` in all findings
docs/development/plugin-development.md	Plugin development guide	Update with severity population guidance

Audit Plugin Architecture: The plugin-based architecture for compliance checks
Compliance Standards: Severity definitions and compliance frameworks supported by opnDossier
Plugin Interface Contract: The compliance.Plugin interface that all plugins must implement
Control Definitions: How security controls are defined with severity, rationale, and remediation guidance