feat(security): informational Pass-1 scan on every admission + one-shot baseline sweep - #1031
Merged
Merged
Conversation
…ot baseline sweep The free in-process TPA Pass-1 scan effectively never ran: the only automatic trigger required trust_mode="scan" (never the default) plus quarantined plus never-scanned plus no approval baseline, and nothing ever scanned existing servers. Telemetry showed 12 of 157 capable installs had ever scanned. Two informational paths now populate the verdict without touching gating: - Admission: a newly added, enabled server gets one Pass-1 scan in any trust mode. Servers the scan-mode admission gate owns are skipped so nothing is scanned twice, and the known-server set is seeded from the startup config so a restart is not mistaken for a wave of new admissions. - Baseline sweep: on startup, behind a persisted BBolt marker, enabled servers with no scan summary are swept through the same path. Backgrounded, delayed so upstreams can connect, serialized, cancellable, and it declines to burn the marker when every candidate failed. Results are stored through the normal scan-summary path, so badges light up; nothing quarantines, approves, or unquarantines. Kill switch: security.auto_baseline_scan (default on, env MCPPROXY_AUTO_BASELINE_SCAN).
Deploying mcpproxy-docs with
|
| Latest commit: |
e12fcd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5df8ff7e.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://feat-tpa-scan-on-admission.mcpproxy-docs.pages.dev |
…l-closed
Cross-model review (opencode, gpt-5.6-sol) round 1 findings on the
informational Pass-1 scan paths. Four genuine defects, four rejected.
1. Informational scan could cause a gating state change (HIGH).
scanModeAdmissionOwns keyed on sc.Quarantined, but quarantine is MUTABLE
while a scan is in flight and maybeAutoApproveScanSettled re-reads it at
settle time. A trust_mode:"scan" server that was unquarantined when the
informational path claimed it, and that the operator quarantined before
the clean verdict landed, would be silently unquarantined by the settle
handler — exactly the "informational scans never gate" invariant this
feature promises. The predicate now keys only on (trust mode, prior
approval baseline): a scan-mode server without an approval baseline is
precisely the set the settle handler can act on, so the informational path
leaves it alone in every quarantine state. Scan-mode servers that already
have a baseline stay eligible (the settle handler bails on them).
2. A failed admission scan was never retryable.
maybeStartInformationalScans records a server in infoScanKnown before the
scan is attempted; releaseInformationalScan dropped only the infoScanQueued
claim, so the server stayed permanently "not new" and no later
servers.changed could pick it up — and once the one-shot sweep marker is
burned, nothing would ever scan it. Release now forgets the name in both
maps.
3. A storage read failure burned the one-shot sweep marker.
The sweep read its inventory through listStoredServers, which collapses a
read error into an empty slice — the sweep's "nothing to do" signal, which
persists the marker. A transient BBolt error would therefore permanently
mark a sweep that never examined a single server. The sweep now calls
ListUpstreamServers directly and fails closed, leaving the marker unset.
4. An invalid MCPPROXY_AUTO_BASELINE_SCAN value disabled the feature.
The loader's env pass used a bare non-empty check, so a typo ("yes") wrote
AutoBaselineScan=false over a config that had explicitly enabled it; the
accessor then ignores the unrecognized env value and reads that overwritten
false. The loader now uses the same true/1/false/0 vocabulary as
IsAutoBaselineScanEnabled and ignores anything else.
Rejected as not defects: the settle-timeout serialization release and the
partial-sweep marker rule are documented deliberate tradeoffs (an unbounded
wait would let one hung scan block the queue forever; requiring zero failures
would re-sweep on every start for a permanently broken server); the
"unbounded goroutines" claim is bounded by the number of distinct newly
admitted servers, which drain; and the engine's detachment onto
context.Background in executeScan is pre-existing behaviour shared by the
spec-086 admission scan and every manual scan, not introduced here.
Tests: scanModeAdmissionOwns table gains both unquarantined-scan-mode cases;
new TestInformationalScan_FailedStartRetriesOnNextServersChanged,
TestBaselineSweep_UnreadableStoreNeverBurnsMarker, and loader-vocabulary
coverage in internal/config.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 32688939658 --repo smart-mcp-proxy/mcpproxy-go
|
Follow-up to the previous commit's retry fix. Making
releaseInformationalScan clear infoScanKnown restored the intended retry,
but left it unbounded: StartScan returns an error for a server it cannot
connect to ("no source files available and server is disconnected"), and
that failure path spends up to ~60s inside StartScan (EnsureConnected plus
a 30s connection wait, twice) while holding infoScanRunMu. For a
permanently broken upstream every servers.changed would re-enter it,
stalling the informational queue and respawning the process endlessly.
Retries now stop after maxInformationalScanAttempts (3) per server per
process; past the cap the server keeps its "known" mark and is retired to
manual scanning. A server that was merely still connecting — the case the
retry exists for — is unaffected.
… time Cross-model review round 2 (opencode) confirmed the round-1 fixes and found one remaining real gap: a queued informational scan never re-read the kill switch. docs/configuration.md documents security.auto_baseline_scan as hot-reloadable and "read live at each decision point", but the flag was only consulted where a scan was QUEUED. A queued scan can sit behind infoScanRunMu for minutes while earlier scans settle, and the one-shot sweep waits out a 45s startup delay before doing anything — so an operator who disabled automatic scanning in either window still watched the queue drain into their upstreams. The flag is now re-read at the last decision point before StartScan, and again after the sweep's startup delay. Three consequences handled explicitly: - A kill-switch skip is not a failed attempt: it releases the claim WITHOUT consuming one of the server's bounded retries, so toggling the flag off and on cannot retire servers that never failed a scan. - The sweep abandons rather than counting the remaining servers as failures. Counting them would still burn the one-shot marker whenever an earlier server had already scanned, permanently retiring servers the sweep never examined. - The marker is left unset in both abandon paths, so a re-enabled sweep resumes on the next start. Rejected from this round: the claim that a mutable trust_mode lets an informational scan become an auto-approving admission scan. The scenario (operator PATCHes a quarantined server from manual to scan mid-scan, clean settle unquarantines it) reaches no state that is not already reachable on main: maybeStartAdmissionScans runs on every servers.changed — which connection-state changes emit continuously — and would scan that same never-scanned, scan-mode, quarantined server and auto-approve it on the same clean verdict. The informational path adds no new capability there, and the transition is what flipping a quarantined server to trust_mode:"scan" means.
Cross-model review round 3 (opencode) found the remaining hole in the
"informational scans never gate" invariant, and it is a TOCTOU the previous
predicate could not close.
scanModeAdmissionOwns still allowed one class through: a trust_mode:"scan"
server that HAS an approval baseline, on the reasoning that
maybeAutoApproveScanSettled bails unconditionally on such servers. But that
gate is re-read at SETTLE time and the baseline is deletable in between —
POST /api/v1/security/servers/{name}/reject calls Service.RejectServer, which
deletes the integrity baseline. So: a re-quarantined scan-mode server is
claimed for an informational scan, the operator rejects it mid-scan, the
baseline disappears, and the clean settle then auto-approves and unquarantines
the server that was just rejected.
Every input the settle handler gates on (quarantine, approval baseline) is
mutable while a scan is in flight, so no static snapshot of them is safe to
scan on. The rule is now the blunt, provable one: the informational path never
touches a trust_mode:"scan" server, in any quarantine state, with or without a
baseline. Those servers still get a verdict from the gating path's own
admission scan or from a manual scan. This also subsumes the round-2 mutable-
trust_mode concern.
Also from round 3: context cancellation no longer consumes one of a server's
bounded retries. A shutdown draining the queue would otherwise count an
attempt per parked scan even though none ran, retiring servers on a Server
object that is later restarted in-process. Cancellation now takes the same
uncounted unclaim path as a kill-switch skip.
Docs updated to state the scan-mode exclusion and why.
Cross-model review round 4 (opencode). The previous commit stopped context cancellation from consuming a bounded retry in the admission goroutine but left the sweep loop on the old path, so a shutdown draining the sweep still charged an attempt per server for scans that never ran. Both call sites now route cancellation to the uncounted unclaim. Rejected from this round: - "Trust mode is checked only at claim time, so a manual server flipped to scan+quarantined mid-scan can still reach maybeAutoApproveScanSettled." This was raised and rejected in round 2 and re-raised here. It reaches no state that is not already reachable on main: maybeStartAdmissionScans runs on every servers.changed — which connection-state changes emit continuously — and independently scans and auto-approves exactly that server (scan mode, quarantined, never scanned, no baseline) on the same clean verdict. The informational path adds no capability, and unquarantine-on-clean is the documented meaning of putting a quarantined server in trust_mode:"scan". - "A sweep that skips servers already claimed by the admission path burns the marker with scanned==0 && failed==0." Changing this would be worse than the problem: the admission path queues its scans before the sweep's 45s delay elapses, so counting in-flight claims as unfinished work would leave the marker unset on every start and the one-shot sweep would run forever — precisely what the marker exists to prevent. Servers in that state are covered by the admission path's own bounded retries.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Fixes the E2E Tests (ubuntu-latest) job, which was red on this branch from
the first push. All nine failures were one data race, reported identically
every time:
Write test goroutine mutating a config.ServerConfig in place
Read (*Server).findServerConfig
<- (*Server).maybeAutoApproveScanSettled
<- (*Server).listenForRoutingModeRefresh
findServerConfig ranged runtime.Config().Servers, whose comment claimed the
snapshot was "immutable — safe to call from event-loop goroutines". It is
not: Config() hands back a shared, lock-free snapshot whose ServerConfig
structs other goroutines mutate in place. maybeStartAdmissionScans, twenty
lines above, already documents exactly this hazard and reads from storage to
avoid it; findServerConfig did not.
The race is pre-existing code, but this branch is what makes it fire: before
these informational scan paths, a stock configuration never started a scan,
so nothing ever emitted the settle event that reaches findServerConfig. Once
scans actually run, the settle handler runs with them.
findServerConfig now reads through StorageManager.ListUpstreamServers, which
the storage manager mutex serializes against SaveUpstreamServer and which
returns fresh copies. It has exactly one caller, maybeAutoApproveScanSettled,
so nothing else is affected — this is the one place this branch touches the
spec-086 gating path, and it changes the read seam, not the gating logic.
The failure is not reproducible locally (it needs the ubuntu runner's
timing); CI is the verification.
A disabled server was skipped for scanning (correct: the scan would have to start it to export tool definitions) but still recorded in infoScanKnown by seedKnownServers and by the servers.changed handler. That mark is permanent, so enabling the server minutes later looked to the admission path like a server it had already seen, and the one-shot, marker-gated sweep never came back for it. The server's security badge read "not scanned" for the life of the installation — the exact hole this feature exists to close. Skip disabled servers in BOTH recording sites instead: they are not the sweep's job either (the sweep skips them too), so leaving them unrecorded costs one map lookup per servers.changed and makes the enable act as the admission. Regression test covers seed + admission + enable, and asserts the enable still yields exactly one scan and no approval.
… temp-dir leak Three confirmed defects from the cross-model review of the informational baseline scan paths. 1. securityScanner was published unsynchronized. NewServerWithConfigPath starts the event-listener goroutine, but the scanner service is only assigned much later in startCustomHTTPServer. That goroutine reads the field on every servers.changed / scan-settled event, so publish and read are concurrent — a genuine data race on an interface value, which can observe a torn or nil service and silently drop an admission scan. Route every access through securityScannerSvc()/setSecurityScanner() behind a dedicated RWMutex (not s.mu, so it cannot join a lock cycle with the lifecycle lock), and capture the service once per call site. 2. A partially failing baseline sweep burned its one-shot marker. The rule "scanned > 0 || failed == 0" marked the sweep done as soon as one server succeeded, so a server that was merely still connecting was stranded: the marker outlives the process, and the in-process retry route dies with it. Require failed == 0. Retrying is self-limiting — the next sweep's only candidates are servers that still have no scan summary. 3. Service.StartScan leaked its prepared temp source directory whenever engine.StartScan rejected the scan. The engine only invokes the callback (which owns the cleanup) for a scan it accepted, so all three rejection paths — concurrent scan in progress, scanner resolution failure, no scanners installed — orphaned the directory. The automatic paths retry and collide with manual scans, which turns a rare leak into a recurring one. Release the cleanup on the error return. Also documents two findings that are NOT fixed here, in the code at the places a future reader will look: the settle handler can still be reached if a server's trust_mode is flipped to "scan" mid-scan (closing it needs scan provenance threaded through the runtime event payload and into spec-086's gating path, whose semantics this change leaves untouched), and findServerConfig's storage read can be stale inside ApplyConfig's emit-before-sync window.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
2 participants
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.
What
Make the free in-process TPA Pass-1 scan actually run for real users.
Today the only automatic trigger is
maybeStartAdmissionScan, which needstrust_mode="scan"(never the default) and quarantined and never-scanned and no approval baseline — and nothing ever rescans existing servers. Telemetry: 12 / 157 capable installs had ever scanned.Two new informational paths (
internal/server/scan_informational.go):NewServerWithConfigPath, so "new" means appeared since process start (and a fresh install's very first server still counts).meta/baseline_sweep_done) is absent, enabled servers with no scan summary are swept through the same path, then the marker is persisted. Backgrounded (never delays startup), delayed 45s so upstreams can connect, serialized, cancellable via the server context, and it logs one summary line.Key decisions
trust_mode:"scan"admission flow is untouched. The informational path explicitly skips the servers that gate owns (scan+ quarantined + no approval baseline), so nothing is scanned twice and the settle-driven auto-approval cannot be reached from it.StartScanreturns as soon as the job is created, so without the wait a sweep would launch every scan at once. The user-facing batchScanQueueis deliberately not hijacked (it allows only one batch and drives the UI progress panel).security.auto_baseline_scan(default on), envMCPPROXY_AUTO_BASELINE_SCANresolved in the accessor so/api/v1/config/applycannot defeat it. Hot-reloadable (read live;DetectConfigChangesalready deep-compares the wholesecurityblock). Swagger + docs regenerated/updated.