Repository navigation
chore(secrets): extract IMDS-blocking transport into shared pkg/httpclient #25
Description
Severity / Confidence
P3, high confidence. Chore. Duplicated SSRF-defense helper across two modules; no behavior change pending refactor.
Affected files
internal/secrets/azure_resolver.go:17-60(duplicateimdsAddresses,blockIMDSDialer,imdsBlockingTransport)providers/azure/internal/httpclient/httpclient.go:14-53(originalimdsAddresses,blockIMDSDialer,New)
Evidence
PR LeanerCloud/cloud-commitments-cli#1224 added an IMDS-blocking transport to NewAzureResolver (imdsBlockingTransport() at internal/secrets/azure_resolver.go:46). The implementation is bit-for-bit equivalent to httpclient.New() at providers/azure/internal/httpclient/httpclient.go:38: same imdsAddresses map (169.254.169.254, fd00:ec2::254), same blockIMDSDialer struct + DialContext, same 10s/30s/30s timeouts, same http.Transport fields.
The duplication is currently forced by Go's internal/ import rule plus the module boundary (providers/azure/go.mod is a separate module from the root). The root-module internal/secrets package cannot import github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient.
Memory feedback_azure_use_httpclient_new.md explicitly says: "Every Azure service client must use httpclient.New(), not &http.Client{Timeout: ...}. Any new Azure service client that omits this step loses the SSRF defense-in-depth without any compile-time signal." Two parallel implementations defeat the grep-based regression check the memory recommends (grep '&http.Client{' in providers/azure/).
Recommendation
Extract imdsAddresses, blockIMDSDialer, and the New() constructor into a shared, non-internal package importable by both modules:
pkg/httpclient/httpclient.go(thepkg/module already has its owngo.mod, so it can be required by both root andproviders/azure/).- Replace the contents of
providers/azure/internal/httpclient/httpclient.goandinternal/secrets/azure_resolver.goIMDS helpers with a re-export / direct call into the newpkg/httpclient.New(). - Keep the regression test pattern: each consuming module retains a small test that calls
pkg/httpclient.New()and asserts a request tohttp://169.254.169.254/...errors with "blocked".
This collapses the duplication and gives the grep guard a single canonical answer.
Triage labels
type/chore, priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, triaged.
Source: PR LeanerCloud/cloud-commitments-cli#1224 adversarial review.
Activity
cristim commentedon Jul 28, 2026
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.The 2026-07-28 full-repo review re-derived this duplication and found the situation has changed since this issue was written:
pkg/httpclientnow exists and claims to be the single implementation, butinternal/secretsnever migrated onto it. That makes this issue'stype/choreframing understate it, because the drift now silently strands a security fix.Where
internal/secrets/azure_resolver.go:17-60versuspkg/httpclient/httpclient.go:20-68What the review adds
imdsAddresses,blockIMDSDialer,DialContextand the client constructor are duplicated almost verbatim. The copy ininternal/secretsinlines its timeouts as bare literals (10 * time.Second,30 * time.Secondat lines 49-57) where the shared package names them as constants (dialTimeout,keepAliveInterval,tlsHandshakeTimeout,requestTimeout).pkg/httpclient's own package header now asserts the migration is complete:That is true of
providers/azure/internal/httpclient/httpclient.go:18and false ofinternal/secrets/azure_resolver.go. A reader auditing the SSRF posture from that comment concludes there is one implementation to fix.Failure scenario
The IMDS block-list needs several fixes (see #20 and LeanerCloud/cloud-commitments-cli#1324; the guard matches the hostname rather than the resolved IP, so
metadata.google.internalpasses). Those fixes will land inpkg/httpclientand will silently not apply to Azure Key Vault secret retrieval, which is the path that fetches the credential encryption key itself (internal/credentials/cipher.go:106). The two copies are currently textually near-identical, so the drift will not be visible in review of the fix PR; the patch simply leaves the highest-value path unprotected while the header comment says otherwise.Fix direction
Delete the copy and call
httpclient.New(). Add a guard test assertinginternal/secretsdeclares no dialer of its own, matching the existingazure_sdk_module_guard_test.gopattern already in that package.Given the above, suggest sequencing this before #20 / LeanerCloud/cloud-commitments-cli#1324 so the hostname-resolution fix is written once, and re-checking whether
type/chore/severity/lowstill fits now that the header comment actively misreports the state.cristim commentedon Oct 4, 2026
Platform prerequisite published as LeanerCloud/cloud-commitments-platform#482 at b97a8d0068878c4ab581d6125e1326588a1c095e. It replaces the Key Vault literal-host guard with the existing shared resolved-address guard. Independent Astra review and confined native macOS baseline/mutation/treatment, real SDK local-TLS, secrets race suite, build, lint and normal hooks passed. Positive evidence is synthetic, not live Azure/Private Link acceptance; the PR remains open pending acceptance and CI. This does not resolve broader public-only policy requirements in #20/#152 or close the remaining scope of this issue.