Skip to content

Commit cbf457b

Browse files
authored
Merge pull request #479 from simonchrz/feat/registry-checksums
Verify npm and Composer archives against their SHA-1 shasum
2 parents e0fb8be + ba5e6e8 commit cbf457b

9 files changed

Lines changed: 257 additions & 22 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ require (
1313
github.com/git-pkgs/cooldown v0.2.0
1414
github.com/git-pkgs/enrichment v0.7.2
1515
github.com/git-pkgs/gcs v0.1.0
16-
github.com/git-pkgs/integrity v0.1.1
16+
github.com/git-pkgs/integrity v0.2.0
1717
github.com/git-pkgs/magic v0.5.0
1818
github.com/git-pkgs/purl v0.1.21
1919
github.com/git-pkgs/registries v0.9.3

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,8 @@ github.com/git-pkgs/enrichment v0.7.2 h1:flD/M89HxkOegUi6tC8EOspFiIMsl7m0o576IQK
135135
github.com/git-pkgs/enrichment v0.7.2/go.mod h1:xGyc/iz7u1UnbrOHk8lIX5ZaNF3mKFDEZc3VqBJ/LOk=
136136
github.com/git-pkgs/gcs v0.1.0 h1:E3awGtsO0xZyHT9FUfEwMHjMkRxw41Bh+c7lgTmPvBo=
137137
github.com/git-pkgs/gcs v0.1.0/go.mod h1:bdkCFD66ryaWnU8MBhokVA3WkJfyAEchm9qec5woBpE=
138-
github.com/git-pkgs/integrity v0.1.1 h1:nHQ7SktOiGM1dOb5BFnkdtttG/6FCgE6r5ru6QnsGts=
139-
github.com/git-pkgs/integrity v0.1.1/go.mod h1:hxu24lcd230377hCF28JQW7sGcCbuNLqo/0ULeb+F1Q=
138+
github.com/git-pkgs/integrity v0.2.0 h1:sxpAk7EUllZnTYttOoPTW/Yfe5J2imglpuJiuBUZOxM=
139+
github.com/git-pkgs/integrity v0.2.0/go.mod h1:vPLb2RB+hR/iCMIgqph0PZbQ7iktXDTK+uupoHQikKk=
140140
github.com/git-pkgs/magic v0.5.0 h1:Pm+4fkHR+K6kg9pwJM4hnwWUuBlkYdLmF1yIAI8lU9Q=
141141
github.com/git-pkgs/magic v0.5.0/go.mod h1:SXOqcsNmbmpZjJZHEEWnwxprbsFvpwWgTAZrgXp4Jm4=
142142
github.com/git-pkgs/pom v0.1.8 h1:mXCuWw6XkrKKZb6IurRZyIznYLPhV5AYg5ZO38Uj/l8=

‎internal/handler/composer.go‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,8 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request)
711711
if commit := strings.TrimSuffix(filename, path.Ext(filename)); isDevVersion(version) && composerCommitPattern.MatchString(commit) {
712712
h.discardOtherCommit(packageName, version, cacheFilename, commit)
713713
}
714-
result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "composer", packageName, version, cacheFilename, downloadURL)
714+
result, err := h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "composer", packageName, version, cacheFilename, downloadURL,
715+
composerIntegrity(dist.shasum, version, downloadURL == listedURL))
715716
if err != nil {
716717
h.proxy.serveArtifactError(w, err, "failed to fetch package")
717718
return
@@ -776,10 +777,11 @@ func (h *ComposerHandler) metadataURLsForVersion(vendor, pkg, version string) []
776777
}
777778

778779
// composerDist is what a version's metadata says about its archive: the dist
779-
// URL and type, and the raw publish time the cooldown is checked against.
780+
// URL, type and shasum, and the raw publish time the cooldown is checked
781+
// against.
780782
type composerDist struct {
781-
url, distType string
782-
published []byte
783+
url, distType, shasum string
784+
published []byte
783785
}
784786

785787
// findDownloadURLFromMetadata fetches a metadata document and returns the dist
@@ -862,6 +864,7 @@ func composerDistURL(body []byte, packageName, version string) (composerDist, er
862864
dist.url, _, _ = lookupJSONString(distRaw, "url")
863865
if dist.url != "" {
864866
dist.distType, _, _ = lookupJSONString(distRaw, "type")
867+
dist.shasum, _, _ = lookupJSONString(distRaw, "shasum")
865868
dist.published = fields.get("time")
866869
return errStopScan
867870
}
@@ -873,6 +876,22 @@ func composerDistURL(body []byte, packageName, version string) (composerDist, er
873876
return dist, err
874877
}
875878

879+
// composerIntegrity returns the integrity to verify an archive of version
880+
// against: the SHA-1 its dist lists as shasum. A dev branch is left
881+
// unverified: its listed shasum describes only the current head, while every
882+
// commit of the branch is cached under the same version, which holds a single
883+
// recorded integrity, so a cache hit for an older pinned commit would be
884+
// checked against the head's shasum. Composer checks dist.shasum itself
885+
// anyway. A shasum also only describes the listed archive, and Packagist
886+
// lists none for GitHub archives; an unusable one is ignored. Those archives
887+
// are served unverified as before.
888+
func composerIntegrity(shasum, version string, listed bool) string {
889+
if !listed || isDevVersion(version) {
890+
return ""
891+
}
892+
return sha1Checksum(shasum)
893+
}
894+
876895
// proxyUpstream forwards a request to packagist.org without caching.
877896
func (h *ComposerHandler) proxyUpstream(w http.ResponseWriter, r *http.Request) {
878897
upstreamURL := h.upstreamURL + r.URL.Path
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package handler
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"io"
7+
"net/http"
8+
"net/http/httptest"
9+
"sync"
10+
"testing"
11+
12+
"github.com/git-pkgs/registries/fetch"
13+
)
14+
15+
func TestComposerDownloadVerifiesDeclaredShasum(t *testing.T) {
16+
const archive = "archive bytes"
17+
18+
tests := []struct {
19+
name string
20+
shasum string
21+
wantStatus int
22+
}{
23+
{"matching shasum", sha1Hex(archive), http.StatusOK},
24+
{"mismatched shasum", sha1Hex("other bytes"), http.StatusBadGateway},
25+
{"empty shasum", "", http.StatusOK},
26+
{"unparseable shasum", "not-a-sha1", http.StatusOK},
27+
}
28+
29+
for _, tt := range tests {
30+
t.Run(tt.name, func(t *testing.T) {
31+
h, proxy := setupComposerShasum(t, "1.0.0", "/zip/p.zip", tt.shasum, map[string]string{"/zip/p.zip": archive})
32+
33+
w := httptest.NewRecorder()
34+
h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/p.zip", nil))
35+
if w.Code != tt.wantStatus {
36+
t.Fatalf("status = %d, want %d; body: %s", w.Code, tt.wantStatus, w.Body.String())
37+
}
38+
if wantCached := tt.wantStatus == http.StatusOK; isComposerArchiveCached(t, proxy, "1.0.0", "p.zip") != wantCached {
39+
t.Errorf("cached = %v, want %v", !wantCached, wantCached)
40+
}
41+
})
42+
}
43+
}
44+
45+
// TestComposerDownloadPinnedCommitIgnoresListedShasum checks that the shasum
46+
// listed for a branch's head is not held against an older commit a lock file
47+
// pins: it describes another archive.
48+
func TestComposerDownloadPinnedCommitIgnoresListedShasum(t *testing.T) {
49+
h, _ := setupComposerShasum(t, "dev-master", "/zip/"+composerCommitB, sha1Hex("commit B"), map[string]string{
50+
"/zip/" + composerCommitA: "commit A",
51+
"/zip/" + composerCommitB: "commit B",
52+
})
53+
54+
w := httptest.NewRecorder()
55+
h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+composerCommitA+".zip", nil))
56+
if w.Code != http.StatusOK || w.Body.String() != "commit A" {
57+
t.Errorf("status %d, body %q, want 200 %q", w.Code, w.Body.String(), "commit A")
58+
}
59+
}
60+
61+
func setupComposerShasum(t *testing.T, version, distPath, shasum string, archives map[string]string) (http.Handler, *Proxy) {
62+
t.Helper()
63+
h, proxy, _ := setupComposerShasumCounting(t, version, distPath, shasum, archives)
64+
return h, proxy
65+
}
66+
67+
// setupComposerShasumCounting is setupComposerShasum that also reports how
68+
// many times each archive path was fetched from upstream.
69+
func setupComposerShasumCounting(t *testing.T, version, distPath, shasum string, archives map[string]string) (http.Handler, *Proxy, func(string) int) {
70+
t.Helper()
71+
var mu sync.Mutex
72+
fetched := map[string]int{}
73+
var srv *httptest.Server
74+
srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
75+
if r.URL.Path == "/p2/v/p.json" || r.URL.Path == "/p2/v/p~dev.json" {
76+
_, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":%q,"dist":{"url":%q,"type":"zip","shasum":%q}}]}}`,
77+
version, srv.URL+distPath, shasum)
78+
return
79+
}
80+
body, ok := archives[r.URL.Path]
81+
if !ok {
82+
http.NotFound(w, r)
83+
return
84+
}
85+
mu.Lock()
86+
fetched[r.URL.Path]++
87+
mu.Unlock()
88+
_, _ = io.WriteString(w, body)
89+
}))
90+
t.Cleanup(srv.Close)
91+
92+
proxy, _, _, _ := setupTestProxy(t)
93+
fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0))
94+
proxy.Fetcher = fetcher
95+
t.Cleanup(func() { _ = fetcher.Close() })
96+
return NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes(), proxy, func(path string) int {
97+
mu.Lock()
98+
defer mu.Unlock()
99+
return fetched[path]
100+
}
101+
}
102+
103+
func isComposerArchiveCached(t *testing.T, proxy *Proxy, version, filename string) bool {
104+
t.Helper()
105+
cached, err := proxy.GetCachedArtifact(context.Background(), "composer", "v/p", version, filename)
106+
if err != nil {
107+
t.Fatalf("reading cache: %v", err)
108+
}
109+
if cached == nil {
110+
return false
111+
}
112+
_ = cached.Reader.Close()
113+
return true
114+
}
115+
116+
// TestComposerDevBranchArchivesStayServableFromCache is a regression test: a
117+
// version's integrity is one value shared by all its cached archives, but a
118+
// dev branch caches one archive per commit. Recording the head's shasum made
119+
// a pinned older commit fail its check when served from the cache, so its
120+
// archive was discarded and fetched again, which in turn cleared the head's
121+
// recorded shasum.
122+
func TestComposerDevBranchArchivesStayServableFromCache(t *testing.T) {
123+
h, proxy, fetched := setupComposerShasumCounting(t, "dev-master", "/zip/"+composerCommitB, sha1Hex("commit B"), map[string]string{
124+
"/zip/" + composerCommitA: "commit A",
125+
"/zip/" + composerCommitB: "commit B",
126+
})
127+
128+
for i, commit := range []string{composerCommitA, composerCommitB, composerCommitA, composerCommitB} {
129+
want := map[string]string{composerCommitA: "commit A", composerCommitB: "commit B"}[commit]
130+
w := httptest.NewRecorder()
131+
h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+commit+".zip", nil))
132+
if w.Code != http.StatusOK || w.Body.String() != want {
133+
t.Errorf("request %d for %s: status %d, body %q; want 200 %q", i+1, commit[:4], w.Code, w.Body.String(), want)
134+
}
135+
}
136+
for _, commit := range []string{composerCommitA, composerCommitB} {
137+
if n := fetched("/zip/" + commit); n != 1 {
138+
t.Errorf("commit %s fetched from upstream %d times, want once and then served from the cache", commit[:4], n)
139+
}
140+
if !isComposerArchiveCached(t, proxy, "dev-master", commit+".zip") {
141+
t.Errorf("commit %s is no longer cached", commit[:4])
142+
}
143+
}
144+
}

‎internal/handler/handler.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1808,10 +1808,11 @@ func (p *Proxy) GetOrFetchArtifactFromURLWithDigest(ctx context.Context, ecosyst
18081808
}
18091809

18101810
// GetOrFetchArtifactFromURLWithIntegrity retrieves an artifact and verifies a
1811-
// newly fetched response against declared, the Subresource Integrity string
1812-
// the registry's metadata declares, before serving or caching it. declared is
1813-
// recorded with the cached version, so cache hits are verified against it as
1814-
// well. An empty declared fetches without verification.
1811+
// newly fetched response against declared, the integrity the registry's
1812+
// metadata declares, before serving or caching it: a Subresource Integrity
1813+
// string, or a single algorithm-prefixed hex checksum such as "sha1-<hex>".
1814+
// declared is recorded with the cached version, so cache hits are verified
1815+
// against it as well. An empty declared fetches without verification.
18151816
func (p *Proxy) GetOrFetchArtifactFromURLWithIntegrity(ctx context.Context, ecosystem, name, version, filename, downloadURL, declared string) (*CacheResult, error) {
18161817
return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", declared)
18171818
}

‎internal/handler/integrity.go‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ import (
1010
type integrityChecks struct {
1111
contentHash integrity.SRI
1212
native integrity.SRI
13+
// checksum is native metadata given as a single algorithm-prefixed hex
14+
// checksum, such as a registry's SHA-1 shasum, which is not SRI.
15+
checksum integrity.Digest
16+
hasChecksum bool
1317
algorithms []integrity.Algorithm
1418
}
1519

@@ -28,7 +32,13 @@ func newIntegrityChecks(contentHash, native string) (integrityChecks, error) {
2832
if native != "" {
2933
digests, err := integrity.ParseSRI(native)
3034
if err != nil {
31-
return integrityChecks{}, fmt.Errorf("parse integrity: %w", err)
35+
checksum, checksumErr := integrity.ParseChecksum(native)
36+
if checksumErr != nil {
37+
return integrityChecks{}, fmt.Errorf("parse integrity: %w", err)
38+
}
39+
checks.checksum, checks.hasChecksum = checksum, true
40+
checks.algorithms = append(checks.algorithms, checksum.Algorithm())
41+
return checks, nil
3242
}
3343
checks.native = digests
3444
for _, digest := range digests {
@@ -189,4 +199,24 @@ func (r *verifyingReader) verify() {
189199
r.onMismatch("integrity: " + err.Error())
190200
}
191201
}
202+
if r.checks.hasChecksum {
203+
if err := result.VerifyDigest(r.checks.checksum); err != nil {
204+
r.mismatched = true
205+
r.onMismatch("integrity: " + err.Error())
206+
}
207+
}
208+
}
209+
210+
// sha1Checksum returns a registry's hexadecimal SHA-1 shasum as the
211+
// "sha1-<hex>" checksum newIntegrityChecks accepts, or "" when it is empty or
212+
// not a SHA-1 digest.
213+
func sha1Checksum(shasum string) string {
214+
if shasum == "" {
215+
return ""
216+
}
217+
digest, err := integrity.ParseHex(integrity.SHA1, shasum)
218+
if err != nil {
219+
return ""
220+
}
221+
return digest.Checksum()
192222
}

‎internal/handler/integrity_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"encoding/hex"
99
"errors"
1010
"io"
11+
"slices"
1112
"strings"
1213
"testing"
1314
)
@@ -84,6 +85,38 @@ func TestNewIntegrityChecksRejectsMalformedMetadata(t *testing.T) {
8485
}
8586
}
8687

88+
// TestIntegrityChecksVerifyRegistryChecksum checks native metadata given as
89+
// an algorithm-prefixed hex checksum, the form recorded for a registry's SHA-1
90+
// shasum, which is not SRI.
91+
func TestIntegrityChecksVerifyRegistryChecksum(t *testing.T) {
92+
const data = "hello world"
93+
checksum := "sha1-" + sha1Hex(data)
94+
checks, err := newIntegrityChecks(sha256Hex(data), checksum)
95+
if err != nil {
96+
t.Fatalf("newIntegrityChecks(%q): %v", checksum, err)
97+
}
98+
for _, tt := range []struct {
99+
name, body string
100+
wantMismatch bool
101+
}{
102+
{"matching", data, false},
103+
{"mismatched", "other bytes", true},
104+
} {
105+
t.Run(tt.name, func(t *testing.T) {
106+
var reasons []string
107+
reader, err := checks.wrapFailOnMismatch(io.NopCloser(strings.NewReader(tt.body)), func(reason string) { reasons = append(reasons, reason) })
108+
if err != nil {
109+
t.Fatal(err)
110+
}
111+
_, _ = io.ReadAll(reader)
112+
gotNative := slices.ContainsFunc(reasons, func(r string) bool { return strings.HasPrefix(r, "integrity: ") })
113+
if gotNative != tt.wantMismatch {
114+
t.Errorf("checksum mismatch reported = %v, want %v (reasons %q)", gotNative, tt.wantMismatch, reasons)
115+
}
116+
})
117+
}
118+
}
119+
87120
func TestVerifyingReader(t *testing.T) {
88121
const data = "hello world"
89122
goodSHA := sha256Hex(data)

‎internal/handler/npm.go‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"sync"
1717
"time"
1818

19+
"github.com/git-pkgs/integrity"
1920
"github.com/git-pkgs/proxy/internal/retention"
2021
)
2122

@@ -878,18 +879,21 @@ func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, versi
878879
return h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "npm", packageName, version, cacheFilename, downloadURL, sri)
879880
}
880881

881-
// npmVersionIntegrity returns versions[version].dist.integrity from a
882-
// packument when it is a Subresource Integrity string the proxy can verify,
883-
// or "" otherwise, in which case the tarball is fetched unverified as before.
882+
// npmVersionIntegrity returns the integrity a packument declares for a
883+
// version's tarball: dist.integrity when it is a Subresource Integrity string
884+
// the proxy can verify, or else the SHA-1 in dist.shasum, which is all
885+
// versions published before integrity existed carry, as "sha1-<hex>". It
886+
// returns "" when neither is usable, and the tarball is fetched unverified as
887+
// before.
884888
func npmVersionIntegrity(body []byte, version string) string {
885889
sri, _, _ := lookupJSONString(body, "versions", version, "dist", "integrity")
886-
if sri == "" {
887-
return ""
888-
}
889-
if _, err := newIntegrityChecks("", sri); err != nil {
890-
return ""
890+
if sri != "" {
891+
if _, err := integrity.ParseSRI(sri); err == nil {
892+
return sri
893+
}
891894
}
892-
return sri
895+
shasum, _, _ := lookupJSONString(body, "versions", version, "dist", "shasum")
896+
return sha1Checksum(shasum)
893897
}
894898

895899
// npmVersionTarball returns versions[version].dist.tarball from a packument,

‎internal/handler/npm_integrity_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ var npmIntegrityCases = []struct {
2727
{"strongest algorithm decides", map[string]string{"integrity": sha256SRI("other bytes") + " " + sha512SRI(npmIntegrityTarball)}, true},
2828
{"any digest of the strongest algorithm", map[string]string{"integrity": sha512SRI("other bytes") + " " + sha512SRI(npmIntegrityTarball)}, true},
2929
{"unparseable integrity", map[string]string{"integrity": "sha512-nope"}, true},
30-
{"shasum alone is not checked yet", map[string]string{"shasum": sha1Hex("other bytes")}, true},
30+
{"matching shasum", map[string]string{"shasum": sha1Hex(npmIntegrityTarball)}, true},
31+
{"mismatched shasum", map[string]string{"shasum": sha1Hex("other bytes")}, false},
32+
{"integrity wins over shasum", map[string]string{"integrity": sha512SRI("other bytes"), "shasum": sha1Hex(npmIntegrityTarball)}, false},
33+
{"unparseable integrity falls back to shasum", map[string]string{"integrity": "sha512-nope", "shasum": sha1Hex("other bytes")}, false},
34+
{"unparseable shasum", map[string]string{"shasum": "not-a-sha1"}, true},
3135
{"no checksum", map[string]string{}, true},
3236
}
3337

0 commit comments

Comments
 (0)