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