diff --git a/go.mod b/go.mod index 052604e5..b76c338e 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/git-pkgs/cooldown v0.2.0 github.com/git-pkgs/enrichment v0.7.2 github.com/git-pkgs/gcs v0.1.0 - github.com/git-pkgs/integrity v0.1.1 + github.com/git-pkgs/integrity v0.2.0 github.com/git-pkgs/magic v0.5.0 github.com/git-pkgs/purl v0.1.21 github.com/git-pkgs/registries v0.9.3 diff --git a/go.sum b/go.sum index c6bed9e1..3533e642 100644 --- a/go.sum +++ b/go.sum @@ -135,8 +135,8 @@ github.com/git-pkgs/enrichment v0.7.2 h1:flD/M89HxkOegUi6tC8EOspFiIMsl7m0o576IQK github.com/git-pkgs/enrichment v0.7.2/go.mod h1:xGyc/iz7u1UnbrOHk8lIX5ZaNF3mKFDEZc3VqBJ/LOk= github.com/git-pkgs/gcs v0.1.0 h1:E3awGtsO0xZyHT9FUfEwMHjMkRxw41Bh+c7lgTmPvBo= github.com/git-pkgs/gcs v0.1.0/go.mod h1:bdkCFD66ryaWnU8MBhokVA3WkJfyAEchm9qec5woBpE= -github.com/git-pkgs/integrity v0.1.1 h1:nHQ7SktOiGM1dOb5BFnkdtttG/6FCgE6r5ru6QnsGts= -github.com/git-pkgs/integrity v0.1.1/go.mod h1:hxu24lcd230377hCF28JQW7sGcCbuNLqo/0ULeb+F1Q= +github.com/git-pkgs/integrity v0.2.0 h1:sxpAk7EUllZnTYttOoPTW/Yfe5J2imglpuJiuBUZOxM= +github.com/git-pkgs/integrity v0.2.0/go.mod h1:vPLb2RB+hR/iCMIgqph0PZbQ7iktXDTK+uupoHQikKk= github.com/git-pkgs/magic v0.5.0 h1:Pm+4fkHR+K6kg9pwJM4hnwWUuBlkYdLmF1yIAI8lU9Q= github.com/git-pkgs/magic v0.5.0/go.mod h1:SXOqcsNmbmpZjJZHEEWnwxprbsFvpwWgTAZrgXp4Jm4= github.com/git-pkgs/pom v0.1.8 h1:mXCuWw6XkrKKZb6IurRZyIznYLPhV5AYg5ZO38Uj/l8= diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 9dac530c..c89a18ba 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -711,7 +711,8 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) if commit := strings.TrimSuffix(filename, path.Ext(filename)); isDevVersion(version) && composerCommitPattern.MatchString(commit) { h.discardOtherCommit(packageName, version, cacheFilename, commit) } - result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "composer", packageName, version, cacheFilename, downloadURL) + result, err := h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "composer", packageName, version, cacheFilename, downloadURL, + composerIntegrity(dist.shasum, version, downloadURL == listedURL)) if err != nil { h.proxy.serveArtifactError(w, err, "failed to fetch package") return @@ -776,10 +777,11 @@ func (h *ComposerHandler) metadataURLsForVersion(vendor, pkg, version string) [] } // composerDist is what a version's metadata says about its archive: the dist -// URL and type, and the raw publish time the cooldown is checked against. +// URL, type and shasum, and the raw publish time the cooldown is checked +// against. type composerDist struct { - url, distType string - published []byte + url, distType, shasum string + published []byte } // findDownloadURLFromMetadata fetches a metadata document and returns the dist @@ -862,6 +864,7 @@ func composerDistURL(body []byte, packageName, version string) (composerDist, er dist.url, _, _ = lookupJSONString(distRaw, "url") if dist.url != "" { dist.distType, _, _ = lookupJSONString(distRaw, "type") + dist.shasum, _, _ = lookupJSONString(distRaw, "shasum") dist.published = fields.get("time") return errStopScan } @@ -873,6 +876,22 @@ func composerDistURL(body []byte, packageName, version string) (composerDist, er return dist, err } +// composerIntegrity returns the integrity to verify an archive of version +// against: the SHA-1 its dist lists as shasum. A dev branch is left +// unverified: its listed shasum describes only the current head, while every +// commit of the branch is cached under the same version, which holds a single +// recorded integrity, so a cache hit for an older pinned commit would be +// checked against the head's shasum. Composer checks dist.shasum itself +// anyway. A shasum also only describes the listed archive, and Packagist +// lists none for GitHub archives; an unusable one is ignored. Those archives +// are served unverified as before. +func composerIntegrity(shasum, version string, listed bool) string { + if !listed || isDevVersion(version) { + return "" + } + return sha1Checksum(shasum) +} + // proxyUpstream forwards a request to packagist.org without caching. func (h *ComposerHandler) proxyUpstream(w http.ResponseWriter, r *http.Request) { upstreamURL := h.upstreamURL + r.URL.Path diff --git a/internal/handler/composer_shasum_test.go b/internal/handler/composer_shasum_test.go new file mode 100644 index 00000000..b4847951 --- /dev/null +++ b/internal/handler/composer_shasum_test.go @@ -0,0 +1,144 @@ +package handler + +import ( + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/git-pkgs/registries/fetch" +) + +func TestComposerDownloadVerifiesDeclaredShasum(t *testing.T) { + const archive = "archive bytes" + + tests := []struct { + name string + shasum string + wantStatus int + }{ + {"matching shasum", sha1Hex(archive), http.StatusOK}, + {"mismatched shasum", sha1Hex("other bytes"), http.StatusBadGateway}, + {"empty shasum", "", http.StatusOK}, + {"unparseable shasum", "not-a-sha1", http.StatusOK}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h, proxy := setupComposerShasum(t, "1.0.0", "/zip/p.zip", tt.shasum, map[string]string{"/zip/p.zip": archive}) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/p.zip", nil)) + if w.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d; body: %s", w.Code, tt.wantStatus, w.Body.String()) + } + if wantCached := tt.wantStatus == http.StatusOK; isComposerArchiveCached(t, proxy, "1.0.0", "p.zip") != wantCached { + t.Errorf("cached = %v, want %v", !wantCached, wantCached) + } + }) + } +} + +// TestComposerDownloadPinnedCommitIgnoresListedShasum checks that the shasum +// listed for a branch's head is not held against an older commit a lock file +// pins: it describes another archive. +func TestComposerDownloadPinnedCommitIgnoresListedShasum(t *testing.T) { + h, _ := setupComposerShasum(t, "dev-master", "/zip/"+composerCommitB, sha1Hex("commit B"), map[string]string{ + "/zip/" + composerCommitA: "commit A", + "/zip/" + composerCommitB: "commit B", + }) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+composerCommitA+".zip", nil)) + if w.Code != http.StatusOK || w.Body.String() != "commit A" { + t.Errorf("status %d, body %q, want 200 %q", w.Code, w.Body.String(), "commit A") + } +} + +func setupComposerShasum(t *testing.T, version, distPath, shasum string, archives map[string]string) (http.Handler, *Proxy) { + t.Helper() + h, proxy, _ := setupComposerShasumCounting(t, version, distPath, shasum, archives) + return h, proxy +} + +// setupComposerShasumCounting is setupComposerShasum that also reports how +// many times each archive path was fetched from upstream. +func setupComposerShasumCounting(t *testing.T, version, distPath, shasum string, archives map[string]string) (http.Handler, *Proxy, func(string) int) { + t.Helper() + var mu sync.Mutex + fetched := map[string]int{} + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/p2/v/p.json" || r.URL.Path == "/p2/v/p~dev.json" { + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":%q,"dist":{"url":%q,"type":"zip","shasum":%q}}]}}`, + version, srv.URL+distPath, shasum) + return + } + body, ok := archives[r.URL.Path] + if !ok { + http.NotFound(w, r) + return + } + mu.Lock() + fetched[r.URL.Path]++ + mu.Unlock() + _, _ = io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + + proxy, _, _, _ := setupTestProxy(t) + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + return NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes(), proxy, func(path string) int { + mu.Lock() + defer mu.Unlock() + return fetched[path] + } +} + +func isComposerArchiveCached(t *testing.T, proxy *Proxy, version, filename string) bool { + t.Helper() + cached, err := proxy.GetCachedArtifact(context.Background(), "composer", "v/p", version, filename) + if err != nil { + t.Fatalf("reading cache: %v", err) + } + if cached == nil { + return false + } + _ = cached.Reader.Close() + return true +} + +// TestComposerDevBranchArchivesStayServableFromCache is a regression test: a +// version's integrity is one value shared by all its cached archives, but a +// dev branch caches one archive per commit. Recording the head's shasum made +// a pinned older commit fail its check when served from the cache, so its +// archive was discarded and fetched again, which in turn cleared the head's +// recorded shasum. +func TestComposerDevBranchArchivesStayServableFromCache(t *testing.T) { + h, proxy, fetched := setupComposerShasumCounting(t, "dev-master", "/zip/"+composerCommitB, sha1Hex("commit B"), map[string]string{ + "/zip/" + composerCommitA: "commit A", + "/zip/" + composerCommitB: "commit B", + }) + + for i, commit := range []string{composerCommitA, composerCommitB, composerCommitA, composerCommitB} { + want := map[string]string{composerCommitA: "commit A", composerCommitB: "commit B"}[commit] + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+commit+".zip", nil)) + if w.Code != http.StatusOK || w.Body.String() != want { + t.Errorf("request %d for %s: status %d, body %q; want 200 %q", i+1, commit[:4], w.Code, w.Body.String(), want) + } + } + for _, commit := range []string{composerCommitA, composerCommitB} { + if n := fetched("/zip/" + commit); n != 1 { + t.Errorf("commit %s fetched from upstream %d times, want once and then served from the cache", commit[:4], n) + } + if !isComposerArchiveCached(t, proxy, "dev-master", commit+".zip") { + t.Errorf("commit %s is no longer cached", commit[:4]) + } + } +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 5e0688d8..c3a9ea0f 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1808,10 +1808,11 @@ func (p *Proxy) GetOrFetchArtifactFromURLWithDigest(ctx context.Context, ecosyst } // GetOrFetchArtifactFromURLWithIntegrity retrieves an artifact and verifies a -// newly fetched response against declared, the Subresource Integrity string -// the registry's metadata declares, before serving or caching it. declared is -// recorded with the cached version, so cache hits are verified against it as -// well. An empty declared fetches without verification. +// newly fetched response against declared, the integrity the registry's +// metadata declares, before serving or caching it: a Subresource Integrity +// string, or a single algorithm-prefixed hex checksum such as "sha1-". +// declared is recorded with the cached version, so cache hits are verified +// against it as well. An empty declared fetches without verification. func (p *Proxy) GetOrFetchArtifactFromURLWithIntegrity(ctx context.Context, ecosystem, name, version, filename, downloadURL, declared string) (*CacheResult, error) { return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", declared) } diff --git a/internal/handler/integrity.go b/internal/handler/integrity.go index f78d231d..7e800b83 100644 --- a/internal/handler/integrity.go +++ b/internal/handler/integrity.go @@ -10,6 +10,10 @@ import ( type integrityChecks struct { contentHash integrity.SRI native integrity.SRI + // checksum is native metadata given as a single algorithm-prefixed hex + // checksum, such as a registry's SHA-1 shasum, which is not SRI. + checksum integrity.Digest + hasChecksum bool algorithms []integrity.Algorithm } @@ -28,7 +32,13 @@ func newIntegrityChecks(contentHash, native string) (integrityChecks, error) { if native != "" { digests, err := integrity.ParseSRI(native) if err != nil { - return integrityChecks{}, fmt.Errorf("parse integrity: %w", err) + checksum, checksumErr := integrity.ParseChecksum(native) + if checksumErr != nil { + return integrityChecks{}, fmt.Errorf("parse integrity: %w", err) + } + checks.checksum, checks.hasChecksum = checksum, true + checks.algorithms = append(checks.algorithms, checksum.Algorithm()) + return checks, nil } checks.native = digests for _, digest := range digests { @@ -189,4 +199,24 @@ func (r *verifyingReader) verify() { r.onMismatch("integrity: " + err.Error()) } } + if r.checks.hasChecksum { + if err := result.VerifyDigest(r.checks.checksum); err != nil { + r.mismatched = true + r.onMismatch("integrity: " + err.Error()) + } + } +} + +// sha1Checksum returns a registry's hexadecimal SHA-1 shasum as the +// "sha1-" checksum newIntegrityChecks accepts, or "" when it is empty or +// not a SHA-1 digest. +func sha1Checksum(shasum string) string { + if shasum == "" { + return "" + } + digest, err := integrity.ParseHex(integrity.SHA1, shasum) + if err != nil { + return "" + } + return digest.Checksum() } diff --git a/internal/handler/integrity_test.go b/internal/handler/integrity_test.go index 9aa99589..16f47b19 100644 --- a/internal/handler/integrity_test.go +++ b/internal/handler/integrity_test.go @@ -8,6 +8,7 @@ import ( "encoding/hex" "errors" "io" + "slices" "strings" "testing" ) @@ -84,6 +85,38 @@ func TestNewIntegrityChecksRejectsMalformedMetadata(t *testing.T) { } } +// TestIntegrityChecksVerifyRegistryChecksum checks native metadata given as +// an algorithm-prefixed hex checksum, the form recorded for a registry's SHA-1 +// shasum, which is not SRI. +func TestIntegrityChecksVerifyRegistryChecksum(t *testing.T) { + const data = "hello world" + checksum := "sha1-" + sha1Hex(data) + checks, err := newIntegrityChecks(sha256Hex(data), checksum) + if err != nil { + t.Fatalf("newIntegrityChecks(%q): %v", checksum, err) + } + for _, tt := range []struct { + name, body string + wantMismatch bool + }{ + {"matching", data, false}, + {"mismatched", "other bytes", true}, + } { + t.Run(tt.name, func(t *testing.T) { + var reasons []string + reader, err := checks.wrapFailOnMismatch(io.NopCloser(strings.NewReader(tt.body)), func(reason string) { reasons = append(reasons, reason) }) + if err != nil { + t.Fatal(err) + } + _, _ = io.ReadAll(reader) + gotNative := slices.ContainsFunc(reasons, func(r string) bool { return strings.HasPrefix(r, "integrity: ") }) + if gotNative != tt.wantMismatch { + t.Errorf("checksum mismatch reported = %v, want %v (reasons %q)", gotNative, tt.wantMismatch, reasons) + } + }) + } +} + func TestVerifyingReader(t *testing.T) { const data = "hello world" goodSHA := sha256Hex(data) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 0e0d2b00..be42d4f7 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -16,6 +16,7 @@ import ( "sync" "time" + "github.com/git-pkgs/integrity" "github.com/git-pkgs/proxy/internal/retention" ) @@ -878,18 +879,21 @@ func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, versi return h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "npm", packageName, version, cacheFilename, downloadURL, sri) } -// npmVersionIntegrity returns versions[version].dist.integrity from a -// packument when it is a Subresource Integrity string the proxy can verify, -// or "" otherwise, in which case the tarball is fetched unverified as before. +// npmVersionIntegrity returns the integrity a packument declares for a +// version's tarball: dist.integrity when it is a Subresource Integrity string +// the proxy can verify, or else the SHA-1 in dist.shasum, which is all +// versions published before integrity existed carry, as "sha1-". It +// returns "" when neither is usable, and the tarball is fetched unverified as +// before. func npmVersionIntegrity(body []byte, version string) string { sri, _, _ := lookupJSONString(body, "versions", version, "dist", "integrity") - if sri == "" { - return "" - } - if _, err := newIntegrityChecks("", sri); err != nil { - return "" + if sri != "" { + if _, err := integrity.ParseSRI(sri); err == nil { + return sri + } } - return sri + shasum, _, _ := lookupJSONString(body, "versions", version, "dist", "shasum") + return sha1Checksum(shasum) } // npmVersionTarball returns versions[version].dist.tarball from a packument, diff --git a/internal/handler/npm_integrity_test.go b/internal/handler/npm_integrity_test.go index 202a7412..2c14614a 100644 --- a/internal/handler/npm_integrity_test.go +++ b/internal/handler/npm_integrity_test.go @@ -27,7 +27,11 @@ var npmIntegrityCases = []struct { {"strongest algorithm decides", map[string]string{"integrity": sha256SRI("other bytes") + " " + sha512SRI(npmIntegrityTarball)}, true}, {"any digest of the strongest algorithm", map[string]string{"integrity": sha512SRI("other bytes") + " " + sha512SRI(npmIntegrityTarball)}, true}, {"unparseable integrity", map[string]string{"integrity": "sha512-nope"}, true}, - {"shasum alone is not checked yet", map[string]string{"shasum": sha1Hex("other bytes")}, true}, + {"matching shasum", map[string]string{"shasum": sha1Hex(npmIntegrityTarball)}, true}, + {"mismatched shasum", map[string]string{"shasum": sha1Hex("other bytes")}, false}, + {"integrity wins over shasum", map[string]string{"integrity": sha512SRI("other bytes"), "shasum": sha1Hex(npmIntegrityTarball)}, false}, + {"unparseable integrity falls back to shasum", map[string]string{"integrity": "sha512-nope", "shasum": sha1Hex("other bytes")}, false}, + {"unparseable shasum", map[string]string{"shasum": "not-a-sha1"}, true}, {"no checksum", map[string]string{}, true}, }