Repository navigation
Conversation
Add report-only deployment security reviews with owner-approved collection. Application authorization remains unknown without historical audit evidence. New management configurations require authentication. Public listeners require explicit configuration. Existing listeners remain unchanged during upgrades.
Exercise proxy capture with authenticated exports, including successful suspicious requests. Unauthenticated exports remain rejected.
Code Review SummaryThe PR adds an opt-in, report-only deployment security review subsystem: profile drafts and owner approvals bound to exact revisions, sandboxed collectors, signed evidence, findings, holds and delivery. Management configuration now defaults to a loopback listener with validation, and ingestion, IP mutation and global security endpoints require internal token, admin scope, verified attribution and parsed addresses. Firewall default-deny no longer implicitly permits SSH. 🚀 Key Improvements
📉 Regressions
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| manifestProcessing := "unknown" | ||
| var pendingManifest *Manifest | ||
| if inputs.Snapshot != "" { | ||
| manifest, manifestErr := e.Store.ReconcileSnapshot(d.Name, id+"."+collector.ID, snapshot) |
There was a problem hiding this comment.
The temporary snapshot and metadata copies are only deleted after Runner.Run; these early returns skip that cleanup and leave them in the scratch root until the next process start, since only Prepare sweeps it. Remove the temporaries before returning.
| manifest, manifestErr := e.Store.ReconcileSnapshot(d.Name, id+"."+collector.ID, snapshot) | |
| manifest, manifestErr := e.Store.ReconcileSnapshot(d.Name, id+"."+collector.ID, snapshot) | |
| if manifestErr != nil { | |
| _ = os.RemoveAll(inputs.Snapshot) | |
| if inputs.Metadata != "" { | |
| _ = os.RemoveAll(inputs.Metadata) | |
| } | |
| return manifestErr | |
| } | |
| manifestProcessing = manifest.ManifestProcessing | |
| pendingManifest = &manifest | |
| inputs.Manifest, err = WriteMetadata(e.Runner.ScratchRoot, manifest) | |
| if err != nil { | |
| _ = os.RemoveAll(inputs.Snapshot) | |
| if inputs.Metadata != "" { | |
| _ = os.RemoveAll(inputs.Metadata) | |
| } | |
| return err | |
| } |
| "process_status": { | ||
| "type": "string" | ||
| }, | ||
| "items": { |
There was a problem hiding this comment.
CollectorResult.Items is []json.RawMessage, so the generator emitted array-of-byte-array; real collector items (objects, strings) are rejected by this published contract. Override the generated items subschema with an unconstrained schema and regenerate.
| "items": { | |
| "items": { | |
| "type": [ | |
| "null", | |
| "array" | |
| ], | |
| "items": true | |
| }, |
| // GetDockerHostIP returns the IP address that containers can use to reach the host. | ||
| // It tries multiple methods and falls back to the default Docker bridge gateway. | ||
| func (m *Manager) GetDockerHostIP() string { | ||
| if m.config.API.PrivateHost!=""{return m.config.API.PrivateHost} |
There was a problem hiding this comment.
GetDockerHostIP must return an address containers can reach, but this returns api.private_host verbatim. A loopback, wildcard (0.0.0.0) or hostname value is unusable from a container and silently breaks the generated nginx/Lua agent IP. Prefer it only when it parses to a private, non-loopback address, otherwise fall through to bridge-gateway detection.
| "window_complete": { | ||
| "type": "boolean" | ||
| }, | ||
| "items": { |
There was a problem hiding this comment.
Envelope.Items is []json.RawMessage, but the generated schema declares items as byte arrays (0-255 integers). Evidence pages hold JSON objects, so schema validation rejects genuine envelopes; constrain the items to arbitrary JSON and regenerate from an overridden schema.
| "items": { | |
| "items": { | |
| "type": [ | |
| "null", | |
| "array" | |
| ] | |
| }, |
| result.Boundaries = parsed.Boundaries | ||
| result.Limitations = append(result.Limitations, parsed.Limitations...) | ||
| result.Coverage = parsed.Coverage | ||
| if result.ProcessStatus != "completed" || runErr != nil || exit.ExitCode != 0 || out.truncated || stderr.truncated || len(result.Omissions) > 0 { |
There was a problem hiding this comment.
The collector's self-declared process_status is parsed but never read; only the container exit code decides result.ProcessStatus. A collector that exits 0 while reporting process_status: "failed" is recorded as completed and its declared coverage is kept, so the report understates collection gaps.
| if result.ProcessStatus != "completed" || runErr != nil || exit.ExitCode != 0 || out.truncated || stderr.truncated || len(result.Omissions) > 0 { | |
| if parsed.ProcessStatus != "completed" { | |
| result.ProcessStatus = "failed" | |
| } | |
| if result.ProcessStatus != "completed" || runErr != nil || exit.ExitCode != 0 || out.truncated || stderr.truncated || len(result.Omissions) > 0 { | |
| result.Coverage.DeclaredObservation = "partial" | |
| } |
| if !cfg.Enabled { | ||
| return "disabled" | ||
| } | ||
| p.mu.RLock() |
There was a problem hiding this comment.
Plugin.runner is nil in production (New sets only store), so as soon as apply sets enforced true, p.runner.ListTable() calls a method on a nil interface and panics the request. Guard the runner and release the read lock before exec'ing nft.
| p.mu.RLock() | |
| p.mu.RLock() | |
| enforced := p.enforced | |
| p.mu.RUnlock() | |
| if enforced && p.runner != nil { | |
| table, err := p.runner.ListTable() | |
| if err == nil && strings.TrimSpace(table) != "" { | |
| return "applied" | |
| } | |
| } | |
| return "unhealthy" |
| replacement = module.get("Replace", module) | ||
| if replacement.get("Path") and replacement.get("Version"): | ||
| items.append({"ecosystem": "go", "name": replacement["Path"], "version": replacement["Version"], "identity": "binary_metadata"}) | ||
| except (OSError, ValueError, KeyError, TypeError, MemoryError, RecursionError, zipfile.BadZipFile, subprocess.SubprocessError): |
There was a problem hiding this comment.
A parseable file of the wrong JSON shape (package.json or package-lock.json holding a list or null, installed.json holding a string) makes calls such as data.get(...) raise AttributeError, which this tuple does not catch, so one bad file aborts the whole inventory instead of recording a single omission. Catch AttributeError here as well.
| except (OSError, ValueError, KeyError, TypeError, MemoryError, RecursionError, zipfile.BadZipFile, subprocess.SubprocessError): | |
| except (OSError, ValueError, AttributeError, KeyError, TypeError, MemoryError, RecursionError, zipfile.BadZipFile, subprocess.SubprocessError): |
Proposal schemas reject unsupported profile protocol and baseline versions.
Add report-only deployment security reviews with owner-approved collection. Application authorization remains unknown without historical audit evidence.
New management configurations require authentication. Public listeners require explicit configuration. Existing listeners remain unchanged during upgrades.