Repository navigation
Report the store host's cloud instance type/VM size (#4214) - #4330
Conversation
Part 2b: the store host profile now reports the cloud instance type (EC2) or VM size (Azure) alongside the existing platform/RAM/settings facts, via a dedicated link-local metadata probe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
The one-argument ProbeAsync(CancellationToken) built a new SocketsHttpHandler on every call and handed it to the two-argument overload, which constructs its HttpClient with disposeHandler: false. Nothing disposed that handler, so every probe (each --check-settings run and each get_store_host cache fill) leaked one handler and its connection pool. The production path now owns and disposes the handler it creates. Pinned with a source-text assertion that the one-argument overload's body contains "using var handler = CreateHandler()"; proven to fail against the pre-fix expression-bodied overload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
Round-1 security review at eaa64eeResult: no High and no Medium findings. There are eight Lows. Three of them (Lows 2-4) come from Threat model. The probe runs inside the Darling service, from the service account's place on the network. I considered three attackers:
What I read.
I also ran a scratch console app against the .NET 10 Findings, all Low, most consequential firstLow 1: The tests do not pin seven of the named safeguards. Each one below can be removed with every test still green. (
The pins that exist do fail on the exact reverts they target. I checked this by reading the code, and it agrees with the by-hand proof in the PR body. Two request-count checks also do real work. Low 2: The connection attempt outlives the 200 ms budget by about 5 seconds. ( The failure: take a host outside any cloud, on a network that silently drops connection attempts to 169.254.169.254. The budget cancels the PUT at 200 ms, and the probe returns In my test run, the request returned at 214 ms, and the connection attempt was cancelled at 5,235 ms, after the handler had already been disposed. With The cost is one socket left trying to connect for about 5 s per probe. The single-flight cache limits this to one probe per 5-minute cache fill, plus one per Low 3: The 256-byte cap limits what the probe keeps, not what the socket reads. ( The failure: the responder answers 200 with a body of up to 1 MiB, either chunked or with a Content-Length header. The probe reads 257 bytes and returns null. But when the response is disposed, In my test run, with a 500,000-byte body, the handler read all 500,000 body bytes, although the probe kept only 257. With The extra bytes are thrown away, never stored, so the reading is bounded and memory is not at risk. It can still be up to 1 MiB for each of the probe's three responses. Response headers have their own limit, the 64 KiB default. Fix: Low 4: Which headers go out depends on tracing settings elsewhere in the process, not on this file. (
The failure comes if tracing is turned on later: a logging provider on either host, OpenTelemetry, or a diagnostic listener. A caller then sends In my test run, with an Activity current, the Azure GET carried Even then, nothing secret would leak, only values the caller chose and random IDs. Still, the rule of Low 5: The catch-all also swallows the caller's cancellation, and the wrong result is then cached. ( The failure: the store reads use up almost all of the This is a wrong fact, not a leak. Fix: add Low 6: Today the In my test run, the current pattern matched Low 7: No one can check the reported value. Whatever answers at 169.254.169.254 chooses it, as long as it fits the pattern. The failure: on a network outside a cloud, a device on the link or the gateway answers the token PUT and the instance-type GET. The pattern stops injection, and no setting is computed from this field. So the harm is only a wrong fact on display, and plain-HTTP metadata always carries that risk. Fix (documentation only): say in the tool description, and next to the Low 8: The comment on the token TTL is wrong. ( The comment calls 60 "the shortest TTL IMDSv2 accepts", but IMDSv2 accepts any TTL from 1 to 21,600 seconds. The PUT, the Suggested handler, client and test changes (Lows 1-4): internal static HttpMessageHandler CreateHandler() => new SocketsHttpHandler
{
UseProxy = false,
AllowAutoRedirect = false,
ConnectTimeout = ProbeBudget, // Low 2
MaxResponseDrainSize = 0, // Low 3
ActivityHeadersPropagator = null, // Low 4
};
// in ProbeAsync(handler, ...)
using var client = new HttpClient(handler, disposeHandler: false)
{
Timeout = Timeout.InfiniteTimeSpan,
MaxResponseContentBufferSize = MaxResponseBytes + 1, // Low 1, streaming item
};
// test: replaces the two whole-file text pins
using var created = DarlingCloudIdentityProbe.CreateHandler();
var h = Assert.IsType<SocketsHttpHandler>(created);
Assert.False(h.UseProxy);
Assert.False(h.AllowAutoRedirect);
Assert.Equal(0, h.MaxResponseDrainSize);
Assert.Null(h.ActivityHeadersPropagator);
Assert.True(h.ConnectTimeout <= TimeSpan.FromMilliseconds(200));The six questions1. Request handling. No request can leave the address. Two Lows: 4 and 8.
2. Response handling. Nothing from a response can be injected into any output. Two Lows (3 and 6) and one integrity finding (Low 7).
3. Time and startup. No startup path reaches the probe. One Low: 2.
4. Failures. No exception text or response body reaches any output. One Low: 5.
5. Resources. Nothing leaks. There are two tails, both limited in time: Lows 2 and 3.
6. Tests. See Low 1. The source pins fail on the exact reverts described in the PR body. The fake-handler tests do not protect five safeguards: the cap, rejecting non-2xx answers, the pattern on the Azure branch, the single budget, and the streamed read. |
…4214 round 1, Lows 2/3/4/5/6/8) CreateHandler now sets ConnectTimeout = ProbeBudget (a cancelled connect no longer keeps retrying for its own default timeout after disposal), MaxResponseDrainSize = 0 (disposal never reads/discards up to 1 MiB) and ActivityHeadersPropagator = null (tracing can never add traceparent/baggage to a metadata request). The HttpClient caps MaxResponseContentBufferSize at MaxResponseBytes + 1 as defense in depth alongside the existing manual streamed cap. A new catch clause lets the caller's own cancellation propagate as OperationCanceledException instead of collapsing into CloudIdentity.None, so StoreHostProfileCache's single-flight gather (which only caches a gather that returns normally) never caches a wrong 5-minute "not found" for a caller that gave up mid-probe. ValuePattern is anchored with \z instead of $, since $ also matches just before a trailing newline. The TTL comment no longer claims 60 seconds is the IMDSv2 minimum (it accepts 1 to 21,600). Tests: extended CreateHandler_ReturnsASocketsHttpHandler to assert the five handler properties directly, in place of the two whole-file text pins for UseProxy/AllowAutoRedirect. Added a test proving an already-cancelled caller token throws through both ProbeAsync and StoreHostProfileCache.GetOrGatherAsync without caching anything, and a test proving ValuePattern rejects "t3.large\n". Reverted the catch filter and the pattern anchor in turn and confirmed each new test fails on the old shape, then restored both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
…round 1, Low 1) Adds tests that isolate each safeguard from the value that would otherwise let it hide: a 60-char valid value padded to 300 bytes with trailing spaces (passes ValuePattern once Trim()'d, so only the 256-byte cap can be rejecting it), a 404 and a 302 that each carry the body "t3.large" (passes the pattern, so only IsSuccessStatusCode can be rejecting it), and an Azure value that fails ValuePattern (the existing pattern test only exercises the EC2 side's own check). The existing oversized/404/redirect tests all used bodies that would already fail on other grounds even with the safeguard under test removed, so none of them can catch its removal. The three "never called from startup" name pins (checking one named method body, or one named file, for the ABSENCE of a string) are replaced with a call-site census: every .cs file under Darling/PerformanceMonitor.Darling.Service/ is scanned for each of three markers (DarlingCloudIdentityProbe.ProbeAsync(, DarlingStoreHostProfile.GatherAsync(, GetStoreHost() and the set of files that contain it must equal exactly the set ruling 3 names. A name pin only rules out the one spot someone thought to check; a new call site anywhere else would pass it with everything green. A Stopwatch-based timing test for "one shared budget, not a fresh timeout per request" was tried and dropped: measured directly, it is racy. The shared outer budget still bounds the EC2 leg's own delay regardless of what Azure gets, so under load EC2's delay can lose the race to that shared budget before completing "naturally" and the assertion passes for a reason unrelated to what it meant to check (measured 2 of 3 runs still passing against a deliberately broken, per-request-timeout build). Replaced with a deterministic source pin instead, exactly as the brief's fallback names: the file must contain exactly one CancelAfter( call, since a per-request-timeout implementation needs a second one. Reverted each new safeguard (the cap check, the IsSuccessStatusCode check, Azure's ValuePattern.IsMatch, the single CancelAfter() and, for the call-site census, a second ProbeAsync( call site added to DarlingWorker.cs) one at a time and confirmed each corresponding new test fails, then restored all of them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
get_store_host's tool description and DescribeCloud's doc comment both now say the instance_type/VM size is whatever the link-local metadata service reported, not verified against anything this host actually runs on. McpToolsListBudgetTests (5 tests) still passes without changing TotalCeilingBytes, so no constant or change-log line update was needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
|
Round-1 fixes done. Three commits on 1. Harden the probe (Lows 2, 3, 4, 5, 6, 8) — commit
|
Closes #4214.
Why
Part 2a (#4282) shipped
get_store_host: the store host's platform, RAM, data volume, PostgreSQL/TimescaleDBfacts, and a verdict per sizing-relevant setting. This part (2b) adds the one fact that needed leaving the host
to answer: which cloud instance type or VM size the store is actually running on, so a sizing question never
needs a shell on the box just to run
curlagainst the metadata endpoint by hand.What changes
A new
DarlingCloudIdentityProbe(Darling/PerformanceMonitor.Darling.Service/DarlingCloudIdentityProbe.cs)probes the link-local instance metadata address (
169.254.169.254) for an EC2 instance type or an Azure VMsize, and a
CloudIdentityrecord struct (Provider,InstanceType) carries the result — both null whennothing was found. It is wired into
HostProfileas a new requiredCloudfield.This is outbound HTTP from the service process, so the safeguards the ruling named are all in one file, load-bearing:
PUT /latest/api/token(withX-aws-ec2-metadata-token-ttl-seconds), then aGET /latest/meta-data/instance-typecarrying that token. No IMDSv1 fallback — that would be a second,unauthenticated way to reach the same data.
GET /metadata/instance/compute/vmSize?api-version=2021-02-01&format=textwithMetadata: true.DarlingCloudIdentityProbe.CreateHandler), never shared with any other outbound callin the process:
UseProxy = false(a configured proxy never carries these requests) andAllowAutoRedirect = false(a redirect off the link-local address is never legitimate metadata, and is neverfollowed).
169.254.169.254) — no DNS name is ever resolved for this probe.CancellationTokenSource, not a per-request timeout — theEC2 token PUT, the EC2 instance-type GET, and (only when EC2 found nothing) the Azure GET all share the one
budget, so a slow first hop cannot spend it twice.
^[A-Za-z0-9._-]{1,64}\z;\z, not$, so a trailing newline cannot pass) — deliberately NOTapplied to the intermediate EC2 token, which legitimately carries
+,/,=that pattern excludes.an oversized body, a pattern mismatch, a thrown exception —
ProbeAsynccatches all of it and returnsCloudIdentity.None. The one exception is the caller's own cancellation, which propagates, so the host-profilecache never stores a "not found" for a caller that gave up.
Per the ruling, the probe reports only the provider (
aws/azure) and the raw instance type/VM size string— no disk-class mapping or other derived fact rides along; defaults over speculative config.
Called only from
GatherAsync(--check-settingsandget_store_host), never fromGatherStartupProfileAsyncor anythingDarlingWorkerruns at service start — a host with no metadata endpointnever pays this probe's cost on every collection cycle.
get_store_host's existing 5-minute cache means oneprobe per cache fill from that path. A source-text pin test proves both never-called claims against the actual
source (see Test plan), not just against today's behavior.
Surfaces (all three the ruling named, nothing more):
get_store_host's JSON payload: a newcloud: { provider, instance_type }object, and a short clause added tothe tool's description tail (after
<<GUIDE>>, so it does not touch the servedtools/listhead budget).--check-settings' text output: a newCloud: aws (t3.large)/Cloud: not detectedline (never printed bythe once-per-start log, which never probes —
FormatStartupProfileTextdoesn't read the field at all). Alsoadded
cloudProvider/cloudInstanceTypeto--check-settings --jsonfor parity with the text line (a smallin-lane fix: the two output modes of the same command should carry the same facts).
renderStoreHost/renderStoreHostPayloadinsweeps.js): a new "Cloud" row between CPUs andRAM.
McpPayloadContractCensusTestsandMcpToolsListBudgetTestsboth passed unchanged — the new payload keys anddescription text stayed under budget, so no pin file needed updating.
Four existing
HostProfileobject-initializer fixtures (DarlingFileLoggerProviderTests,StoreHostProfileCacheTests, andStartupHostProfileLogTests, which now carries the same"SHOULD-NEVER-PRINT" sentinel trick it already uses for
Store) needed aCloudvalue added onceCloudbecamea required member — mechanical, no behavior change.
The production probe disposes the handler it creates (lane H2c).
Test plan
New
Darling/Darling.Tests/DarlingCloudIdentityProbeTests.cs:DarlingCloudIdentityProbeTests— driven entirely through a fakeHttpMessageHandler, no real networkcall: EC2 token+instance-type success (and proves Azure is never reached once EC2 succeeds); EC2 401 falling
back to Azure success; neither cloud responding (a handler that hangs until the probe's own 200 ms budget
cancels it — asserted to return
CloudIdentity.Nonewell under a generous 2 s ceiling, not just eventually);EC2 404 with Azure also not found; an oversized (300-byte) EC2 body rejected; an EC2 redirect never followed
(asserts exactly 3 requests total, proving the redirect target is never fetched); a value that fails the
accept pattern.
DarlingCloudIdentityProbeSourcePinTests— source-text pins:GatherStartupProfileAsyncandDarlingWorker.csnever reachDarlingCloudIdentityProbe;CreateHandlersetsUseProxy = falseandAllowAutoRedirect = false. Proved by hand that each of the four assertions fails when the source it pinsis broken (temporarily flipped
AllowAutoRedirecttotrue, and temporarily madeGatherStartupProfileAsynccall the probe; both broke their respective pin test; both reverted beforecommitting, confirmed by a clean
git statusand a clean rebuild).Ran (all green,
Total: 240, Errors: 0, Failed: 0, Skipped: 4— the 4 skips are the pre-existing[Collection("live-postgres")]live tests this brief said not to stand up a rig for):*StoreHost*(everyDarlingStoreHostProfile*/DarlingMcpStoreHost*/StoreHostProfileCache*class)DarlingCloudIdentityProbeTests,DarlingCloudIdentityProbeSourcePinTestsMcpPayloadContractCensusTests,McpToolsListBudgetTests,McpToolTypeRegistrationTests,ReadmeDerivedCountPinTests(the MCP inventory/description-budget pin classes)*DocCommentHygiene*StartupHostProfileLogTests,DarlingFileLoggerProviderTests(the two otherHostProfilefixture sitesthe new required member touched)
git merge origin/dev(clean merge, no conflicts) — still green.dotnet build Darling/Darling.Tests/Darling.Tests.csproj— 0 Warning(s), 0 Error(s).live-postgrestests this touches indirectly (DarlingStoreHostProfileLiveTests,DarlingMcpStoreHostToolsLiveTests,DarlingMcpStoreHostBudgetLiveTests) — brief said start no rig; theseconstruct a real
HostProfileend-to-end throughGatherAsync, so they'll now also exercise the probeagainst whatever
169.254.169.254resolves to on the CI runner (typically nothing reachable, so a fast"not found" within budget) — CI's rig will confirm.
Round-1 review fixes
Round 1 is the security review in comment 5837132200 (no High, no Medium, eight Lows). All eight landed here (fix report: PR comment 5837689251):
99404367): the handler bounds its connect attempt by the probe's 200 ms budget (ConnectTimeout), never drains an in-flight body when it is disposed (MaxResponseDrainSize = 0), and adds no trace headers (ActivityHeadersPropagator = null). The client caps a buffered body at 257 bytes, one past the 256-byte limit. A cancellation by the caller now propagates instead of reading as "not detected", so the host-profile cache never stores a wrong "not found". The value pattern anchors on\z, not$. The token TTL comment no longer calls 60 seconds the shortest TTL IMDSv2 accepts.98e3342e): tests pin each safeguard on its own (the 256-byte cap, the status check, Azure's pattern check), and a call-site census over every.csfile in the service project replaces two name pins.bf9e518b): theget_store_hostdescription andDescribeCloud's doc comment say the value is what the metadata service reported, not verified.CHANGELOG entry
SECTION: Added
ENTRY:
get_store_host,--check-settingsand the store host web panel now show the EC2 instance type or Azure VM size of the machine the Darling service runs on. For a managed store, or a store on the same machine, that is the store's own host. A short, bounded probe of the cloud metadata endpoint reads it, as the metadata service reports it, and the panel says "not detected" off the cloud or when the probe finds nothing.REF:
[Report the store host's cloud instance type/VM size (#4214) #4330]: Report the store host's cloud instance type/VM size (#4214) #4330
🤖 Generated with Claude Code
https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ