Verify every OCI digest and Helm charts before caching

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{

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"))

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:<hex>" or "sha512:<hex>", 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

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

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)
 	}
 }
 

internal/handler/integrity_test.go

@@ -7,13 +7,16 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"slices"
 	"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)
+	}
+}