fix(snapshot azure): only send Azure credentials to Azure Container Registry

[AlexKantor87]

The problem

kosli snapshot azure reads each Web App's own linuxFxVersion and built an authenticated azcontainerregistry client for whatever registry it named, with no check that it was an Azure Container Registry. On a 401 Bearer challenge the SDK fetches an AAD token for the containerregistry.azure.net audience and POSTs it as access_token to <host>/oauth2/exchange. So any host named in an app's container configuration could collect that token, and the default --digests-source acr was enough.

Anyone with Microsoft.Web/sites/config/write on a scanned resource group can set that host. The built-in Website Contributor role grants it and carries no Microsoft.ContainerRegistry actions, so this turned a role with no registry access into one that could read every registry the snapshot service principal could reach. --dry-run did not prevent it, because the token is sent during discovery.

Reported privately; tracked internally. Detail here is limited to what is needed to review the fix.

The fix

Only an Azure Container Registry login server gets the Azure credential:

Registry	Resolver	Credential sent
an ACR login server (.azurecr.io / .azurecr.cn / .azurecr.us, matched on a whole label)	azcontainerregistry	Azure credential, as before
anything else	digest.OciSha256Anonymous	none

planImageFingerprint identifies the registry with reference.ParseNormalizedNamed rather than splitting the string by hand, and the reference handed to a resolver is the canonical form that was classified, so the two cannot disagree about which host is contacted. github.com/distribution/reference and github.com/opencontainers/go-digest move from indirect to direct; go.sum is unchanged from master.

The credential policy is a typed choice, not a caller-supplied SystemContext. credentialSource has noCredentials as its zero value, so a lookup that fails to state what it wants presents nothing rather than the host's credentials. This matters because containers/image falls back to credential discovery (~/.docker/config.json, ~/.config/containers/auth.json, credential helpers) whenever DockerAuthConfig is nil, and that fallback must not reach a registry named by an untrusted source. OciSha256's own behaviour is unchanged, so the ECR/Podman discovery path that kosli fingerprint -t oci relies on still works.

What five review rounds changed, because it is most of the value here

Each of these was a real defect in an earlier version of this branch:

• A bypass that reopened the vulnerability. Hand-rolled host classification disagreed with url.Parse: net.SplitHostPort does not validate the port, so myregistry.azurecr.io:443@attacker.example classified as ACR while resolving to attacker.example. Fixed by parsing properly. Regression test: TestGetImageFingerprintRejectsSmuggledACRHost.
• "Anonymous" was not anonymous. Empty credentials left DockerAuthConfig nil. Proven with a planted DOCKER_CONFIG: nil sent Basic bGVha2VkdXNlcjpsZWFrZWRwYXNz, a non-nil empty config sent Basic Og==.
• An unhandled panic. strings.Split(d.String(), "sha256:")[1] panics when a registry answers with a sha384/sha512 digest, which go-digest accepts. One hostile registry and one response header killed the whole process. This also affected 17 other commands that reach OciSha256; for them the behaviour changes from panic to a clean error.
• A digest-pinned ACR regression. Slicing the canonical reference stripped sha256:, so ACR read a bare hex string as a tag and 404'd, and one app's error cancels the whole run. main handled that case.
• The same digest rule written three times. Now one rule, in internal/digest, used from three call sites.
• The security claim was not tested. A mutation audit killed 52 of 69 mutants; the survivors included both ends of the credential boundary, the ACR arm never being driven past credential construction, and a pinned cross-check satisfied by comparing one character. All addressed below.

It also fixes an existing bug

References with no registry host (DOCKER|nginx:latest, DOCKER|myuser/myimage:tag) previously errored: GetManifest(ctx, "", "") returns parameter name cannot be empty. Because one app's error calls cancel(), a single such app broke the entire environment report. Normalisation makes them ordinary anonymous-arm cases.

Private third-party registries still cannot be resolved and produce an error naming the app and host, pointing at --digests-source logs. Deliberately an error rather than a silent fallback: changing a customer's fingerprint source without being asked is wrong for a compliance tool.

Why not read the digest from Azure instead

That would remove the registry call altogether. No such field exists. digest and sha256 appear zero times in the Microsoft.Web/sites OpenAPI specs from 2023-01-01 through 2026-07-15, and zero times in armappservice v2.3.0 or v5.1.0. siteContainers exists from 2023-12-01 but is configuration only. The reason is structural: App Service re-pulls on every restart and scale-out, so instances of one app can legitimately run different digests.

Making logs the default was also considered. It removes the leak but is not a sound control: it fails open silently, returning an empty fingerprint with a nil error when no Digest: line is found, and the API returns only the last lines of the log, so the pull line ages out on long-running apps.

Testing

golangci-lint run ./...                                   0 issues
go vet ./... / gofmt -l .                                 clean
go mod tidy                                               no-op; go.sum identical to master
go test ./internal/azure/ ./internal/digest/ -count=1      ok (see note)
go test ./internal/azure/ -race -count=2                   ok

Full suite, baselined. go test ./... -short fails in four packages (cmd/kosli, internal/digest, internal/gitview, internal/requests) which need the local Kosli server on :8001 and registry network access. The identical command on unmodified main produces an identical failure set. In internal/digest the only failure is TestRemoteDockerImageSha256, which needs a registry on :5001 and fails the same way on main; every test added here passes, including under -race -count=2. make test_integration was not run locally because it starts or resets a server stack; CI covers it.

Every guard is mutation-verified. Mutations that now fail a test include: reverting the parser to a hand-rolled split; isACRLoginServer returning true; swapping the dispatch arms; handing the resolver repoPath instead of the reference; stripping sha256: from the digest; keeping a tag alongside a digest; accepting a non-sha256 pin; a nil DockerAuthConfig; restoring the Split(...)[1] panic; swapping anonymousFingerprint for the credential-discovering resolver; pointing the ACR client at a hardcoded host; swapping repoPath and tagOrDigest; comparing one character of the pinned digest; skipping the cross-check on the ACR arm; dropping the GetManifest error; inverting the digests-source condition; and replacing the resolver with a constant.

Known residuals, stated rather than implied

• The two-line delegation in OciSha256Anonymous is not pinned. Making it ask for callerOrHostCredentials still passes. Pinning it needs a global seam or a trusted-cert registry; the typed source makes the fail-open-by-omission direction impossible, which is the part that matters.
• isACRLoginServer accepts any ACR, not only one the customer owns. ACR names are globally registrable, so an attacker can name their own *.azurecr.io. They do not receive the token, because Microsoft operates that endpoint, but they can make the victim's snapshot authenticate to their registry and serve arbitrary manifests. The stronger fix is cross-checking against the subscription's registries via ARM, which needs a new permission; raised separately.
• Unpinned tags have no cross-check. The registry named in app config is authoritative for what a tag resolves to. Standard behaviour, and the cross-check never claimed to cover it.
• The two arms send different Accept headers, so for a multi-arch image the ACR arm may report a platform manifest where the anonymous arm reports an index. Pre-existing asymmetry, now visible because there are two arms.
• AppData.DigestsSource still reports acr for anonymously-resolved digests. The server validates the enum (models/environments.py:370, schemas/azure_web_app_snapshot.json:35), so a new value needs a server change deployed first.

Pre-existing issues found while testing, not fixed here

Raising separately rather than widening a security fix: DockerImageSha256 has the same Split(..., "@sha256:")[1] panic shape; RemoteDockerImageSha256 fails open on a missing digest header; extractImageFingerprintAndStartedTimestampFromLogs panics on a short Digest: sha256: line; NewAppData dereferences SiteConfig.LinuxFxVersion without a nil check; and one app's error cancelling the whole snapshot deserves an abort-vs-partial decision of its own.

Checklist

• Docs are autogenerated from CLI help. Help text updated; the tracked cmd/kosli/testdata/output/docs/mintlify/ goldens do not cover snapshot azure.
• Helm chart not affected.
• Terraform provider not affected.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 4m 59s —— View job

---

Review — round 4

• Gather context (diff, prior review threads)
• Static analysis (go build, go vet, go mod tidy)
• Review internal/azure/azure_apps.go
• Review internal/digest/digest.go
• Review tests
• Post review

No blocking findings. Everything from round 3 is addressed, and the three things I raised then were each handled in a way that made the code smaller rather than larger. Three minor/follow-up notes posted inline.

---

Static analysis

Check	Result
go build ./...	pass
go vet ./...	pass
go mod tidy + git diff go.mod go.sum	clean — distribution/reference and opencontainers/go-digest correctly promoted to direct

I could not run go test in this environment (the command needed approval that wasn't granted), so my verification is build/vet/tidy plus reading. Everything below is from reading the code and the upstream libraries, not from a green suite.

Round-3 items, verified

• Zero value fails closed. Better than what I suggested: imageFingerprintSource and plan.source are gone entirely, and GetImageFingerprint calls isACRLoginServer(plan.domain) at the point of use (azure_apps.go:510). Deleting derived state beats picking a safe zero value for it. The require.Equal(t, fingerprintPlan{}, got) assertion on error cases is now meaningful too.
• App name in the error. Wrapped once at azure_apps.go:110, in the goroutine that calls cancel(), so every failing path is covered exactly once rather than each return site. Correct place. One leftover duplicate — see inline.
• Sha256Fingerprint validates what it's handed. parsed.Validate() added at digest.go:185, with TestSha256FingerprintValidatesWhatItIsHanded covering sha256:hello and friends.

Things I checked that hold up

• The pinned-digest cross-check is real, not a no-op. I read containers/image@v5.36.2/docker/docker_image.go:141-186: GetDigest issues a HEAD and parses Docker-Content-Digest even when the reference is already digested — it does not short-circuit. So the existence check restored by removing the short-circuit genuinely runs, and the cross-check at azure_apps.go:522 compares a header against the pin rather than a value against itself.
• OciSha256 semantics are unchanged for its existing callers. callerOrHostCredentials with both fields empty leaves DockerAuthConfig nil, preserving discovery for attest artifact / ECR-via-Podman. Pinned by the no caller credentials leaves discovery enabled case in TestCredentialContext.
• credentialSource as a typed choice is a better seam than the SystemContext argument it replaced — "hand it the wrong policy" stops being expressible, and noCredentials being the zero value means the failure mode of forgetting is safe, not host credentials.
• reference.Domain can carry uppercase (ParseNormalizedNamed only lowercase-checks the path, not the domain), and isACRLoginServer lowercases before matching — MyRegistry.AzureCR.IO is covered in the table.
• IPv6 domains ([::1]:5000, [2001:db8::1]) are in the classifier table, and keeping net.SplitHostPort over strings.Cut was the right call for exactly the reason given in the round-2 reply.

Notes posted inline

1. azure_apps.go:277 — cosmetic: downloadAppPackage (line 225) still names the app, so with the new wrap the zip path reads app [x]: failed to download package for app [x]: 404.
2. azure_apps.go:540 — pre-existing, now cheap to fix: a ClientSecretCredential is constructed per image, so N ACR-backed apps do N independent AAD token acquisitions. NewAzureClient already builds one and throws it away.
3. digest.go:171 — the doc comment names RemoteDockerImageSha256 as uncovered; that path returns the literal "sha512:ccc…" as a fingerprint on a non-sha256 header, and Cloud Run's hex == "" guard doesn't catch it. Worth an issue, not a change here.

Also, not worth an inline: internal/cloudrun/registry.go:78-116 is the same hand-split-plus-suffix-check shape this PR just replaced with a parser. I worked the smuggling variants against it — gcr.io:443@attacker.example fails HasSuffix(host, ".gcr.io"), so it fails closed and there is no live bypass there. Still the same pattern, and the same reference.ParseNormalizedNamed treatment would remove the class.

Tiny nit: the mismatch error at azure_apps.go:523 prints the pinned digest twice, since plan.reference already ends in @sha256:<pinned>.

What's good here

The test work is the strongest part, and it's noticeably stronger than round 3.

• hostRewritingTransport recording req.URL.Host before redirecting is a neat solution to "drive the ACR arm against a fake registry and still assert which host it was pointed at" — it catches a hardcoded domain, which a plain fake-server test would not.
• Asserting EscapedPath rather than URL.Path is why /v2/team%2Fapp/manifests/sha256:… is a real regression test for the prefix-stripping bug from round 3.
• The near-miss fixture ("a"*63 + "b") closing a cross-check that a single-character comparison would have satisfied, and TestAnonymousFingerprintIsTheCredentialFreeResolver pinning the production identity of a var every other test replaces, are both the kind of thing that only shows up if you actually mutate.
• Reporting the mutation audit honestly — 52/69, with the surviving mutants named and the two-line delegations called out as still unpinned — is more useful than a green tick.

The write-ups on each thread continue to correct your own claims where they didn't hold (the main-parity point on tag+digest, the pinned-digest removal not being a security win). That's what made the sha512 panic findable.
· branch fix/azure-registry-credential-scope

[claude]

Good change — but one sibling on the zip path still names the app, so it now reads twice.

downloadAppPackage (azure_apps.go:225) returns failed to download package for app [%s]: %s, and the goroutine at azure_apps.go:110 wraps every NewAppData error with app [%s]: . A non-200 from Kudu therefore surfaces as:

app [payments-api]: failed to download package for app [payments-api]: 404 Not Found

Same one-line fix as here — drop the app name from the inner message, since the wrap covers every path:

return fmt.Errorf("failed to download the package: %s", resp.Status)

Cosmetic only; the wrap itself is the right call and puts the name on the ACR/anonymous errors that previously had none.

[claude]

Not introduced here, but this arm is now isolated enough that it's cheap to fix: a fresh ClientSecretCredential is built per image.

NewAzureClient (azure_apps.go:593) already constructs one from the same three fields and then discards it after building the app-service factory. azidentity caches the AAD token on the credential instance, so N ACR-backed apps in a resource group — resolved concurrently in the goroutine loop at azure_apps.go:90 — means N credentials and N independent containerregistry.azure.net token acquisitions, none of which can reuse each other's.

Holding it on the struct alongside AppServiceFactory collapses that to one, and also moves the "bad service principal" failure to client construction rather than to whichever app happens to be resolved first:

type AzureClient struct {
	Credentials       AzureStaticCredentials
	AppServiceFactory *armappservice.ClientFactory
	credential        azcore.TokenCredential
	acrClientOptions  *azcontainerregistry.ClientOptions
}

Worth checking against TestGetImageFingerprintUsesACRForACRHost, which relies on the empty tenant id failing inside the arm — that assertion would need to move to client construction.

[claude]

Naming the uncovered paths in the doc comment is the right call. One of the two is closer to a live bug than the comment suggests, so it's worth an issue rather than just a note.

RemoteDockerImageSha256 ends with strings.TrimPrefix(digestHeader, "sha256:") (digest.go:420). For a Docker-Content-Digest of sha512:ccc…, TrimPrefix is a no-op and the function returns the literal string "sha512:ccc…" as the fingerprint. Its only production caller — internal/cloudrun/registry.go:62 — guards on hex == "", which a non-empty non-hex string passes, so the value goes to the Kosli API as a fingerprint. Not a panic like the ociSha256 case you fixed, and Artifact Registry won't do it in practice, but it's silently wrong rather than an error.

Sha256FingerprintFromDigest closes it in one line:

return Sha256FingerprintFromDigest(res.Resp.Header.Get("docker-content-digest"))

That also picks up the empty-header case, which currently returns ("", nil). Fair to leave for a follow-up given the scope discipline on this branch — just worth filing rather than only commenting.