From ff10ab3491322e3f2a6d1ce3ab7867b7a2dc2760 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 08:12:59 +0200 Subject: [PATCH 1/2] Verify sha512 OCI digests and refuse ones that can't be checked Manifests and digest-addressed artifacts were only checked against sha256 digests. A sha512 digest, which the OCI spec also allows, or a digest with any other algorithm was fetched and cached unchecked: a manifest requested as sha512:, or a tag whose Docker-Content-Digest header named one, was stored under that digest whatever the body was. Manifests are now checked against any digest in the reference or the Docker-Content-Digest header with the digest's own algorithm, and one the proxy can't compute is answered with DIGEST_INVALID and not cached. GetOrFetchArtifactFromURLWithDigest verifies sha512 (and sha384) digests through the declared-integrity check, so a mismatch is never committed, and refuses other algorithms with ErrUnsupportedDigest instead of fetching unverified. Blob routes still only accept sha256. --- internal/handler/container_manifest.go | 13 ++-- internal/handler/container_test.go | 85 ++++++++++++++++++++------ internal/handler/handler.go | 71 ++++++++++++++++++--- internal/handler/integrity_test.go | 63 +++++++++++++++++++ 4 files changed, 201 insertions(+), 31 deletions(-) diff --git a/internal/handler/container_manifest.go b/internal/handler/container_manifest.go index 82eed454..af69967a 100644 --- a/internal/handler/container_manifest.go +++ b/internal/handler/container_manifest.go @@ -114,18 +114,23 @@ func (h *ContainerHandler) serveManifest(w http.ResponseWriter, r *http.Request, h.serveStaleManifestOrError(w, r, cacheName, reference, cached, fmt.Errorf("reading manifest: %w", err)) return } - computedDigest := sha256Digest(body) contentDigest := resp.Header.Get("Docker-Content-Digest") + // A digest reference, and the digest the upstream says the manifest + // has, are both cache keys, so each must be the manifest's own digest. + // One the proxy can't compute is refused rather than cached unchecked. for _, expected := range []string{reference, contentDigest} { - if strings.HasPrefix(expected, "sha256:") && expected != computedDigest { + if !strings.Contains(expected, ":") { + continue + } + if err := verifyDigest(expected, body); err != nil { h.proxy.Logger.Error("upstream manifest failed digest verification", - "name", name, "reference", reference, "expected", expected, "actual", computedDigest) + "name", name, "reference", reference, "expected", expected, "error", err) h.containerError(w, http.StatusBadGateway, "DIGEST_INVALID", "manifest digest verification failed") return } } if contentDigest == "" { - contentDigest = computedDigest + contentDigest = sha256Digest(body) } manifest := &cachedContainerManifest{ diff --git a/internal/handler/container_test.go b/internal/handler/container_test.go index f7b7076c..d5467251 100644 --- a/internal/handler/container_test.go +++ b/internal/handler/container_test.go @@ -1,6 +1,8 @@ package handler import ( + "crypto/sha512" + "encoding/hex" "encoding/json" "io" "net/http" @@ -10,6 +12,7 @@ import ( "testing" "time" + "github.com/git-pkgs/proxy/internal/database" upstreamhttp "github.com/git-pkgs/proxy/internal/httpclient" "github.com/git-pkgs/purl" "github.com/git-pkgs/registries/fetch" @@ -900,38 +903,82 @@ func TestContainerHandler_ManifestDigestMismatchIsNotCached(t *testing.T) { } } -func TestContainerHandler_ManifestNonSHA256DigestReferenceIsProxied(t *testing.T) { +// TestContainerHandler_ManifestDigestsAreVerified checks that a sha512 +// digest, in the reference or in Docker-Content-Digest, is verified like a +// sha256 one, and that a digest the proxy can't compute is refused rather +// than cached unverified. +func TestContainerHandler_ManifestDigestsAreVerified(t *testing.T) { manifest := `{"schemaVersion":2}` - sha512Reference := "sha512:" + strings.Repeat("a", 128) - sha512Header := "sha512:" + strings.Repeat("b", 128) - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") - w.Header().Set("Docker-Content-Digest", sha512Header) - _, _ = io.WriteString(w, manifest) - })) - defer upstream.Close() - - proxy, _, _, _ := setupTestProxy(t) - proxy.HTTPClient = upstream.Client() - h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL} + sum := sha512.Sum512([]byte(manifest)) + sha512Digest := "sha512:" + hex.EncodeToString(sum[:]) + otherSHA512 := "sha512:" + strings.Repeat("b", 128) + unsupported := "sha1:" + strings.Repeat("c", 40) - for _, reference := range []string{sha512Reference, "latest"} { - t.Run(reference, func(t *testing.T) { + tests := []struct { + name, reference, header string + wantOK bool + }{ + {"sha512 reference", sha512Digest, sha512Digest, true}, + {"sha512 reference without header", sha512Digest, "", true}, + {"tag with sha512 header", "latest", sha512Digest, true}, + {"mismatched sha512 reference", otherSHA512, "", false}, + {"tag with mismatched sha512 header", "latest", otherSHA512, false}, + {"unsupported reference", unsupported, "", false}, + {"tag with unsupported header", "latest", unsupported, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + if tt.header != "" { + w.Header().Set("Docker-Content-Digest", tt.header) + } + _, _ = io.WriteString(w, manifest) + })) + defer upstream.Close() + + proxy, db, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL} + req := httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/"+tt.reference, nil) w := httptest.NewRecorder() - h.Routes().ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/"+reference, nil)) + h.Routes().ServeHTTP(w, req) + + if !tt.wantOK { + if w.Code != http.StatusBadGateway { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusBadGateway, w.Body.String()) + } + requireManifestNotCached(t, h, db, upstream.URL, req, tt.reference, tt.header) + return + } if w.Code != http.StatusOK { t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) } if got := w.Body.String(); got != manifest { t.Errorf("body = %q, want %q", got, manifest) } - if got := w.Header().Get("Docker-Content-Digest"); got != sha512Header { - t.Errorf("Docker-Content-Digest = %q, want %q", got, sha512Header) - } }) } } +func requireManifestNotCached(t *testing.T, h *ContainerHandler, db *database.DB, upstreamURL string, req *http.Request, keys ...string) { + t.Helper() + accept := normalizeContainerManifestAccept(containerManifestAccept(req)) + for _, key := range keys { + if key == "" { + continue + } + entry, err := db.GetMetadataCache(containerManifestCacheEcosystem, + h.containerManifestSharedCacheKey(upstreamURL, "library/nginx", key, accept)) + if err != nil { + t.Fatalf("checking manifest cache: %v", err) + } + if entry != nil { + t.Errorf("manifest was cached under unverified %s", key) + } + } +} + func TestContainerHandler_ManifestTagWithInvalidDigestIsNotAliased(t *testing.T) { manifest := `{"schemaVersion":2}` invalidDigest := sha256Digest([]byte("different manifest")) diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 8bcac24f..e5ab5650 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -18,6 +18,7 @@ import ( "github.com/git-pkgs/artifacts" "github.com/git-pkgs/cooldown" + "github.com/git-pkgs/integrity" "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/denylist" "github.com/git-pkgs/proxy/internal/metrics" @@ -1921,15 +1922,69 @@ func (p *Proxy) GetOrFetchArtifactFromURLWithHeaders(ctx context.Context, ecosys return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, headers, "", "") } -// GetOrFetchArtifactFromURLWithDigest retrieves an artifact and verifies its -// SHA-256 digest before adding a newly fetched response to the cache. -// Non-sha256 digests are proxied without verification. -func (p *Proxy) GetOrFetchArtifactFromURLWithDigest(ctx context.Context, ecosystem, name, version, filename, downloadURL, digest string) (*CacheResult, error) { - upstreamHash, _ := strings.CutPrefix(digest, "sha256:") - if upstreamHash == digest { - upstreamHash = "" +// GetOrFetchArtifactFromURLWithDigest retrieves an artifact and verifies a +// newly fetched response against expected, an OCI-style digest such as +// "sha256:" or "sha512:", before serving or caching it. A digest +// whose algorithm can't be verified is refused with ErrUnsupportedDigest +// rather than fetched unverified. An empty expected fetches without +// verification. +func (p *Proxy) GetOrFetchArtifactFromURLWithDigest(ctx context.Context, ecosystem, name, version, filename, downloadURL, expected string) (*CacheResult, error) { + if expected == "" { + return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", "") } - return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, upstreamHash, "") + upstreamHash, declared, err := digestChecks(expected) + if err != nil { + return nil, err + } + return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, upstreamHash, declared) +} + +// ErrUnsupportedDigest is returned for a digest whose algorithm or encoding +// the proxy can't verify. +var ErrUnsupportedDigest = errors.New("unsupported digest") + +// digestChecks turns an OCI-style digest into the checks the artifact fetch +// runs: the hex of a sha256 digest, which is also how cached artifacts are +// keyed, or for another algorithm the integrity string verifyIntegrity +// checks. +func digestChecks(expected string) (upstreamHash, declared string, _ error) { + // The sha256 hex is compared as given, ignoring case; a malformed one + // fails as a mismatch once the body has been hashed. + if hash, ok := strings.CutPrefix(expected, "sha256:"); ok { + return hash, "", nil + } + d, err := digest.Parse(expected) + if err != nil { + return "", "", fmt.Errorf("%w %q: %w", ErrUnsupportedDigest, expected, err) + } + var algorithm integrity.Algorithm + switch d.Algorithm() { + case digest.SHA384: + algorithm = integrity.SHA384 + case digest.SHA512: + algorithm = integrity.SHA512 + default: + return "", "", fmt.Errorf("%w %q", ErrUnsupportedDigest, expected) + } + sum, err := integrity.ParseHex(algorithm, d.Encoded()) + if err != nil { + return "", "", fmt.Errorf("%w %q: %w", ErrUnsupportedDigest, expected, err) + } + return "", sum.SRI(), nil +} + +// verifyDigest checks body against expected, an OCI-style digest. A digest +// whose algorithm can't be computed is an ErrUnsupportedDigest, and one that +// doesn't match is an ErrArtifactDigestMismatch. +func verifyDigest(expected string, body []byte) error { + d, err := digest.Parse(expected) + if err != nil { + return fmt.Errorf("%w %q: %w", ErrUnsupportedDigest, expected, err) + } + if actual := d.Algorithm().FromBytes(body); actual != d { + return fmt.Errorf("%w: expected %s, got %s", ErrArtifactDigestMismatch, d, actual) + } + return nil } // GetOrFetchArtifactFromURLWithIntegrity retrieves an artifact and verifies a diff --git a/internal/handler/integrity_test.go b/internal/handler/integrity_test.go index cb84b14a..f5dcae42 100644 --- a/internal/handler/integrity_test.go +++ b/internal/handler/integrity_test.go @@ -7,6 +7,7 @@ import ( "encoding/base64" "encoding/hex" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -14,6 +15,8 @@ import ( "strings" "testing" "testing/iotest" + + "github.com/git-pkgs/registries/fetch" ) func sha256Hex(data string) string { @@ -451,3 +454,63 @@ func TestFailOnMismatchHoldsBackFinalChunk(t *testing.T) { }) } } + +// TestFetchWithDigestVerifiesEveryAlgorithm checks that a sha512 digest is +// verified before the artifact is cached, like a sha256 one, and that a digest +// whose algorithm can't be verified is refused without fetching. +func TestFetchWithDigestVerifiesEveryAlgorithm(t *testing.T) { + const content = "blob content" + sum512 := sha512.Sum512([]byte(content)) + tests := []struct { + name, digest string + wantErr error + }{ + {"sha256", "sha256:" + sha256Hex(content), nil}, + {"sha512", "sha512:" + hex.EncodeToString(sum512[:]), nil}, + {"mismatched sha256", "sha256:" + sha256Hex("other"), ErrArtifactDigestMismatch}, + {"mismatched sha512", "sha512:" + strings.Repeat("a", 128), ErrArtifactDigestMismatch}, + {"unsupported algorithm", "sha1:" + strings.Repeat("a", 40), ErrUnsupportedDigest}, + {"malformed sha512", "sha512:abc", ErrUnsupportedDigest}, + } + for _, stream := range []bool{false, true} { + for _, tt := range tests { + t.Run(fmt.Sprintf("%s/stream=%t", tt.name, stream), func(t *testing.T) { + checkFetchWithDigest(t, stream, content, tt.digest, tt.wantErr) + }) + } + } +} + +func checkFetchWithDigest(t *testing.T, stream bool, content, digest string, wantErr error) { + t.Helper() + proxy, _, _, fetcher := setupTestProxy(t) + proxy.StreamArtifacts = stream + fetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader(content))} + + result, err := proxy.GetOrFetchArtifactFromURLWithDigest(t.Context(), "oci", "library/demo", digest, digest, + "https://registry.example/v2/library/demo/blobs/"+digest, digest) + if err == nil { + var body []byte + body, err = io.ReadAll(result.Reader) + _ = result.Reader.Close() + if err == nil && string(body) != content { + t.Errorf("body = %q, want %q", body, content) + } + } + if !errors.Is(err, wantErr) { + t.Fatalf("error = %v, want %v", err, wantErr) + } + if errors.Is(wantErr, ErrUnsupportedDigest) && fetcher.fetchCalled { + t.Error("fetched an artifact whose digest can't be verified") + } + cached, err := proxy.GetCachedArtifact(t.Context(), "oci", "library/demo", digest, digest) + if err != nil { + t.Fatal(err) + } + if cached != nil { + _ = cached.Reader.Close() + } + if wantCached := wantErr == nil && !stream; (cached != nil) != wantCached { + t.Errorf("cached = %v, want %v", cached != nil, wantCached) + } +} From ce3958378439236b89fb9ac41eaa1dccc457e249 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 08:12:59 +0200 Subject: [PATCH 2/2] Verify Helm charts before they are cached Without artifact streaming, a chart was fetched with no expected digest, committed to the cache, and only then compared with the index's digest by serveChart, which cleared it again. Until then the unverified chart was a cache entry like any other. The index digest is now always passed to the fetch, so a mismatching chart is discarded before it is committed, as it already was when streaming. serveChart's check stays for charts that were cached before. --- internal/handler/helm.go | 10 +++------- internal/handler/helm_test.go | 4 ++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/handler/helm.go b/internal/handler/helm.go index 6183d971..bb6b6230 100644 --- a/internal/handler/helm.go +++ b/internal/handler/helm.go @@ -126,14 +126,10 @@ func (h *HelmHandler) handleChart(w http.ResponseWriter, r *http.Request) { return } - // A streamed fetch is never stored, so serveChart's digest check has - // nothing to compare against: verify the stream itself instead. - expectedDigest := "" - if h.proxy.StreamArtifacts { - expectedDigest = "sha256:" + digest - } + // Verify the chart against the index digest while it is fetched, so a + // mismatch is never committed to the cache, nor streamed out complete. result, err := h.proxy.GetOrFetchArtifactFromURLWithDigest( - r.Context(), helmMetadataEcosystem, repository, digest, filename, downloadURL, expectedDigest) + r.Context(), helmMetadataEcosystem, repository, digest, filename, downloadURL, "sha256:"+digest) if err != nil { h.proxy.serveArtifactError(w, err, "failed to fetch chart") return diff --git a/internal/handler/helm_test.go b/internal/handler/helm_test.go index c50533d6..26c0877e 100644 --- a/internal/handler/helm_test.go +++ b/internal/handler/helm_test.go @@ -179,8 +179,8 @@ func TestHelmHandler_RejectsChartDigestMismatch(t *testing.T) { t.Errorf("rejected chart at %q is not queued for deletion", path) } } - if stored != requests { - t.Errorf("found %d stored charts, want one per request (%d)", stored, requests) + if stored != 0 { + t.Errorf("found %d stored charts, want none: a mismatch is discarded before it is committed", stored) } }