diff --git a/internal/handler/conan.go b/internal/handler/conan.go index dddd79f4..90edcdb2 100644 --- a/internal/handler/conan.go +++ b/internal/handler/conan.go @@ -2,6 +2,7 @@ package handler import ( "net/http" + "slices" "strings" "github.com/git-pkgs/proxy/internal/retention" @@ -79,6 +80,11 @@ func (h *ConanHandler) handleRecipeFile(w http.ResponseWriter, r *http.Request) revision := r.PathValue("revision") filename := r.PathValue("filename") + if conanPartHasSlash(user, channel, revision, filename) { + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + // Only the archives are artifacts; the rest goes through the metadata cache. if !h.shouldCacheFile(filename) { h.proxyCached(w, r) @@ -89,7 +95,13 @@ func (h *ConanHandler) handleRecipeFile(w http.ResponseWriter, r *http.Request) "name", name, "version", version, "user", user, "channel", channel, "filename", filename) upstreamURL := h.upstreamURL + r.URL.Path - storageFilename := conanStorageFilename(user, channel, revision+"_"+filename) + storageFilename, err := h.proxy.cacheFilenameKeepingLegacy("conan", name, version, + conanStorageFilename(user, channel, revision, filename), + conanLegacyStorageFilename(user, channel, revision+"_"+filename), upstreamURL) + if err != nil { + h.proxy.serveArtifactError(w, err, "failed to fetch file") + return + } result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "conan", name, version, storageFilename, upstreamURL) if err != nil { @@ -111,6 +123,11 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request) pkgrev := r.PathValue("pkgrev") filename := r.PathValue("filename") + if conanPartHasSlash(user, channel, revision, pkgref, pkgrev, filename) { + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + // Only the archives are artifacts; the rest goes through the metadata cache. if !h.shouldCacheFile(filename) { h.proxyCached(w, r) @@ -121,7 +138,13 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request) "name", name, "version", version, "pkgref", pkgref, "filename", filename) upstreamURL := h.upstreamURL + r.URL.Path - storageFilename := conanStorageFilename(user, channel, revision+"_"+pkgref+"_"+pkgrev+"_"+filename) + storageFilename, err := h.proxy.cacheFilenameKeepingLegacy("conan", name, version, + conanStorageFilename(user, channel, revision, "packages", pkgref, pkgrev, filename), + conanLegacyStorageFilename(user, channel, revision+"_"+pkgref+"_"+pkgrev+"_"+filename), upstreamURL) + if err != nil { + h.proxy.serveArtifactError(w, err, "failed to fetch file") + return + } result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "conan", name, version, storageFilename, upstreamURL) if err != nil { @@ -133,9 +156,25 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request) } // conanStorageFilename names a cached archive. Archives are stored under -// pkg:conan/{name}@{version}, so the file name carries the revisions and, when -// set, user and channel, to keep every recipe and binary apart. -func conanStorageFilename(user, channel, file string) string { +// pkg:conan/{name}@{version}, so the file name carries user, channel and the +// revisions, to keep every recipe and binary apart. The handlers refuse a +// part that contains a slash, so joining them with one keeps different +// references apart. +func conanStorageFilename(user, channel string, parts ...string) string { + return strings.Join(append([]string{user, channel}, parts...), "/") +} + +// conanPartHasSlash reports whether a path value contains a slash, which it +// can only get from an encoded one (%2F) in the request. +func conanPartHasSlash(parts ...string) bool { + return slices.ContainsFunc(parts, func(part string) bool { return strings.Contains(part, "/") }) +} + +// conanLegacyStorageFilename is the name archives were cached under before +// their parts were joined with slashes. Underscores, which revisions, user +// and channel can contain, joined them, so it is only read for the URL an +// entry was fetched from. +func conanLegacyStorageFilename(user, channel, file string) string { if user == "_" && channel == "_" { return file } diff --git a/internal/handler/conan_test.go b/internal/handler/conan_test.go index cf8038e6..200f8671 100644 --- a/internal/handler/conan_test.go +++ b/internal/handler/conan_test.go @@ -1,6 +1,7 @@ package handler import ( + "context" "io" "log/slog" "net/http" @@ -9,6 +10,8 @@ import ( "sync/atomic" "testing" "time" + + "github.com/git-pkgs/registries/fetch" ) const testProxyURL = "http://localhost:8080" @@ -302,13 +305,13 @@ func TestConanV2FileRoutesCacheArchives(t *testing.T) { filename string }{ {"recipe", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_sources.tgz", - "rrev1_conan_sources.tgz"}, + "_/_/rrev1/conan_sources.tgz"}, {"package", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/packages/pkgid1/revisions/prev1/files/conan_package.tgz", - "rrev1_pkgid1_prev1_conan_package.tgz"}, + "_/_/rrev1/packages/pkgid1/prev1/conan_package.tgz"}, {"recipe xz", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_export.txz", - "rrev1_conan_export.txz"}, + "_/_/rrev1/conan_export.txz"}, {"package zstd", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/packages/pkgid1/revisions/prev1/files/conan_package.tzst", - "rrev1_pkgid1_prev1_conan_package.tzst"}, + "_/_/rrev1/packages/pkgid1/prev1/conan_package.tzst"}, } for _, tt := range tests { @@ -362,9 +365,9 @@ func TestConanUserChannelKeptApart(t *testing.T) { for _, ref := range []struct{ path, body, filename string }{ {"/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_sources.tgz", - "center sources", "rrev1_conan_sources.tgz"}, + "center sources", "_/_/rrev1/conan_sources.tgz"}, {"/v2/conans/zlib/1.3.1/acme/stable/revisions/rrev1/files/conan_sources.tgz", - "acme sources", "acme@stable_rrev1_conan_sources.tgz"}, + "acme sources", "acme/stable/rrev1/conan_sources.tgz"}, } { fetcher.artifact = artifactBody(ref.body) fetcher.fetchCalled = false @@ -651,3 +654,102 @@ func TestConanMetadataPreservesStatusCodes(t *testing.T) { upstream.Close() } } + +const ( + conanRecipeLookalike = "/v2/conans/zlib/1.3/_/_/revisions/R_P_Q/files/conan_package.tgz" + conanPackageFile = "/v2/conans/zlib/1.3/_/_/revisions/R/packages/P/revisions/Q/files/conan_package.tgz" +) + +// conanPathEchoServer answers every file with its own path, so each URL has +// different bytes. +func conanPathEchoServer(t *testing.T, available *atomic.Bool) *httptest.Server { + t.Helper() + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !available.Load() { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte(r.URL.Path)) + })) + t.Cleanup(upstream.Close) + return upstream +} + +func serveConanRequest(h *ConanHandler, target string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + return w +} + +// A recipe revision with underscores must not share a cache entry with a +// package whose revision, reference and package revision spell it. +func TestConanFileKeysKeepPathFieldsApart(t *testing.T) { + var available atomic.Bool + available.Store(true) + upstream := conanPathEchoServer(t, &available) + proxy, _, _, _ := setupTestProxy(t) + proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL) + + for _, target := range []string{conanRecipeLookalike, conanPackageFile} { + if w := serveConanRequest(h, target); w.Code != http.StatusOK || w.Body.String() != target { + t.Errorf("GET %s = %d %q, want its own file", target, w.Code, w.Body.String()) + } + } +} + +// A file cached under the name used before path fields were kept apart is +// still served for the URL it was fetched from, and only for that URL. +func TestConanLegacyFileEntryServedForItsURL(t *testing.T) { + var available atomic.Bool + available.Store(true) + upstream := conanPathEchoServer(t, &available) + proxy, _, _, _ := setupTestProxy(t) + proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL) + + legacy, err := proxy.GetOrFetchArtifactFromURL(context.Background(), "conan", "zlib", "1.3", + "R_P_Q_conan_package.tgz", upstream.URL+conanRecipeLookalike) + if err != nil { + t.Fatalf("seed legacy entry: %v", err) + } + _ = legacy.Reader.Close() + + if w := serveConanRequest(h, conanPackageFile); w.Body.String() != conanPackageFile { + t.Errorf("package file = %d %q, want its own file, not the legacy entry", w.Code, w.Body.String()) + } + available.Store(false) + if w := serveConanRequest(h, conanRecipeLookalike); w.Code != http.StatusOK || w.Body.String() != conanRecipeLookalike { + t.Errorf("recipe file with the upstream down = %d %q, want the legacy entry", w.Code, w.Body.String()) + } +} + +// A path value is decoded, so one holding an encoded slash could spell the +// cache key of another reference. Those requests are refused. +func TestConanFileRoutesRefuseEncodedSlash(t *testing.T) { + var available atomic.Bool + available.Store(true) + upstream := conanPathEchoServer(t, &available) + proxy, _, _, _ := setupTestProxy(t) + proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL) + + if w := serveConanRequest(h, conanPackageFile); w.Code != http.StatusOK || w.Body.String() != conanPackageFile { + t.Fatalf("GET %s = %d %q, want its own file", conanPackageFile, w.Code, w.Body.String()) + } + + const recipe, pkg = "/v2/conans/zlib/1.3/_/_/revisions/", "/v2/conans/zlib/1.3/_/_/revisions/R/packages/" + for field, target := range map[string]string{ + "revision spelling a package file": recipe + "R%2Fpackages%2FP%2FQ/files/conan_package.tgz", + "user": "/v2/conans/zlib/1.3/a%2Fb/_/revisions/R/files/conan_package.tgz", + "channel": "/v2/conans/zlib/1.3/_/a%2Fb/revisions/R/files/conan_package.tgz", + "recipe filename": recipe + "R/files/a%2Fconan_package.tgz", + "package reference": pkg + "P%2FX/revisions/Q/files/conan_package.tgz", + "package revision": pkg + "P/revisions/Q%2FX/files/conan_package.tgz", + "package filename": pkg + "P/revisions/Q/files/a%2Fconan_package.tgz", + } { + if w := serveConanRequest(h, target); w.Code != http.StatusBadRequest { + t.Errorf("%s: GET %s = %d %q, want 400", field, target, w.Code, w.Body.String()) + } + } +} diff --git a/internal/handler/debian.go b/internal/handler/debian.go index 565f181a..dc6d6dcc 100644 --- a/internal/handler/debian.go +++ b/internal/handler/debian.go @@ -168,7 +168,12 @@ func (h *DebianHandler) handlePackageDownload( filename := path[strings.LastIndex(path, "/")+1:] downloadURL := fmt.Sprintf("%s/%s", upstreamURL, path) - cacheFilename := h.artifactCacheFilename(repository, filename) + cacheFilename, err := h.proxy.cacheFilenameKeepingLegacy("deb", name, version, + h.artifactCacheFilename(repository, path), h.legacyArtifactCacheFilename(repository, filename), downloadURL) + if err != nil { + h.proxy.serveArtifactError(w, err, "failed to fetch package") + return + } h.proxy.Logger.Info("debian package download", "repository", repository, @@ -196,10 +201,21 @@ func (h *DebianHandler) handleMetadata( h.proxy.ProxyCached(w, r, fmt.Sprintf("%s/%s", upstreamURL, path), "debian", cacheKey, "*/*") } -// artifactCacheFilename scopes a package by repository, since the same -// filename can hold different bytes in different archives. The main archive -// keeps the unscoped filename its existing cache entries were stored under. -func (h *DebianHandler) artifactCacheFilename(repository, filename string) string { +// artifactCacheFilename names a package in the cache by its repository and +// its path in it, since the same file name can hold different bytes in +// different archives, and in different directories of one archive: Docker's +// repository has a pool under each suite. +func (h *DebianHandler) artifactCacheFilename(repository, path string) string { + if repository == "" { + return path + } + return repository + "/" + path +} + +// legacyArtifactCacheFilename is the name packages were cached under before +// they were keyed by path: the file name, scoped by repository for a named +// one. It is still read for the URL an entry was fetched from. +func (h *DebianHandler) legacyArtifactCacheFilename(repository, filename string) string { if repository == "" { return filename } diff --git a/internal/handler/debian_test.go b/internal/handler/debian_test.go index 54a72b61..f9c62847 100644 --- a/internal/handler/debian_test.go +++ b/internal/handler/debian_test.go @@ -1,10 +1,12 @@ package handler import ( + "context" "fmt" "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -37,10 +39,11 @@ func TestDebianHandler_Routes(t *testing.T) { } // TestDebianHandler_LegacyCacheKeysUnchanged pins the artifact cache identity -// the main archive used before named repositories existed. Deployments carry -// warm caches across upgrades, so a changed key here silently discards them. -// Metadata is cheap to refetch; its keys are covered by -// TestMetadataCacheKeysUnambiguous. +// the main archive used before named repositories existed. Packages are now +// keyed by path, but deployments carry warm caches across upgrades, so +// entries under this name are still read for the URL they were fetched from +// (TestDebianHandler_LegacyPackageEntryServedForItsPath). Metadata is cheap +// to refetch; its keys are covered by TestMetadataCacheKeysUnambiguous. func TestDebianHandler_LegacyCacheKeysUnchanged(t *testing.T) { const ( poolPath = "pool/main/h/hello/hello_2.10-3_amd64.deb" @@ -53,12 +56,12 @@ func TestDebianHandler_LegacyCacheKeysUnchanged(t *testing.T) { // Artifact cache: the bare filename, with no repository prefix. filename := poolPath[strings.LastIndex(poolPath, "/")+1:] - if got := h.artifactCacheFilename("", filename); got != "hello_2.10-3_amd64.deb" { + if got := h.legacyArtifactCacheFilename("", filename); got != "hello_2.10-3_amd64.deb" { t.Errorf("legacy artifact cache filename = %q, want %q", got, "hello_2.10-3_amd64.deb") } // A named repository must not reuse either identity. - if got := h.artifactCacheFilename("security", filename); got == "hello_2.10-3_amd64.deb" { + if got := h.legacyArtifactCacheFilename("security", filename); got == "hello_2.10-3_amd64.deb" { t.Error("named repository reused the legacy artifact cache filename") } if h.metadataCacheKeyFor("security", "https://security.test", distPath) == h.metadataCacheKeyFor("", h.upstreamURL, distPath) { @@ -324,3 +327,72 @@ func TestDebRetentionKey(t *testing.T) { t.Error("debRetentionKey(nil) accepted") } } + +// debianSuiteArchive serves a different build of one package file name per +// suite, as Docker's repository nests a pool under each suite. +func debianSuiteArchive(t *testing.T, available *atomic.Bool) *httptest.Server { + t.Helper() + archive := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !available.Load() { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + return + } + suite := strings.Split(strings.TrimPrefix(r.URL.Path, "/dists/"), "/")[0] + _, _ = fmt.Fprint(w, suite+" build") + })) + t.Cleanup(archive.Close) + return archive +} + +const ( + debianBullseyePackage = "/docker/dists/bullseye/pool/stable/amd64/containerd.io_1.7.25-1_amd64.deb" + debianBookwormPackage = "/docker/dists/bookworm/pool/stable/amd64/containerd.io_1.7.25-1_amd64.deb" +) + +// TestDebianHandler_PackageCacheScopedPerPath checks that one file name in +// two directories of a repository does not resolve to one cached artifact. +func TestDebianHandler_PackageCacheScopedPerPath(t *testing.T) { + var available atomic.Bool + available.Store(true) + archive := debianSuiteArchive(t, &available) + proxy, _, _, _ := setupTestProxy(t) + proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(archive.Client()), fetch.WithMaxRetries(0)) + h := NewDebianHandler(proxy, "http://proxy.example", "https://archive.test", map[string]string{"docker": archive.URL}) + + for _, tc := range []struct{ path, want string }{ + {debianBullseyePackage, "bullseye build"}, + {debianBookwormPackage, "bookworm build"}, + } { + if w := serveDebianRequest(h, tc.path); w.Code != http.StatusOK || w.Body.String() != tc.want { + t.Errorf("GET %s = %d %q, want %q", tc.path, w.Code, w.Body.String(), tc.want) + } + } +} + +// TestDebianHandler_LegacyPackageEntryServedForItsPath checks that a package +// cached under the file name alone, as before packages were keyed by path, is +// still served for the path it was fetched from, and only for that path. +func TestDebianHandler_LegacyPackageEntryServedForItsPath(t *testing.T) { + var available atomic.Bool + available.Store(true) + archive := debianSuiteArchive(t, &available) + proxy, _, _, _ := setupTestProxy(t) + proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(archive.Client()), fetch.WithMaxRetries(0)) + h := NewDebianHandler(proxy, "http://proxy.example", "https://archive.test", map[string]string{"docker": archive.URL}) + + // The entry an earlier version cached for the bullseye path. + legacy, err := proxy.GetOrFetchArtifactFromURL(context.Background(), "deb", "containerd.io", "1.7.25-1", + "docker/containerd.io_1.7.25-1_amd64.deb", archive.URL+strings.TrimPrefix(debianBullseyePackage, "/docker")) + if err != nil { + t.Fatalf("seed legacy entry: %v", err) + } + _ = legacy.Reader.Close() + + if w := serveDebianRequest(h, debianBookwormPackage); w.Body.String() != "bookworm build" { + t.Errorf("bookworm = %d %q, want its own build, not the legacy bullseye entry", w.Code, w.Body.String()) + } + available.Store(false) + if w := serveDebianRequest(h, debianBullseyePackage); w.Code != http.StatusOK || w.Body.String() != "bullseye build" { + t.Errorf("bullseye with the archive down = %d %q, want the legacy entry", w.Code, w.Body.String()) + } +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 9612e510..9e8b5657 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -2396,6 +2396,23 @@ func (p *Proxy) GetOrFetchArtifactFromURL(ctx context.Context, ecosystem, name, return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", "") } +// cacheFilenameKeepingLegacy returns legacy when an artifact recorded under +// it was fetched from downloadURL, and filename otherwise. +func (p *Proxy) cacheFilenameKeepingLegacy(ecosystem, name, version, filename, legacy, downloadURL string) (string, error) { + _, versionPURL, err := packagePURLStrings(ecosystem, name, version) + if err != nil { + return "", err + } + record, err := p.DB.GetArtifact(versionPURL, legacy) + if err != nil { + return "", fmt.Errorf("checking artifact cache: %w", err) + } + if record != nil && record.UpstreamURL == downloadURL { + return legacy, nil + } + return filename, nil +} + // GetOrFetchArtifactFromURLWithHeaders retrieves an artifact from cache or fetches from a URL // with additional request-specific HTTP headers. func (p *Proxy) GetOrFetchArtifactFromURLWithHeaders(ctx context.Context, ecosystem, name, version, filename, downloadURL string, headers http.Header) (*CacheResult, error) { diff --git a/internal/server/retention_conan_test.go b/internal/server/retention_conan_test.go index 17607bc7..e5023be0 100644 --- a/internal/server/retention_conan_test.go +++ b/internal/server/retention_conan_test.go @@ -29,15 +29,15 @@ type conanFile struct { var ( conanZlibSources = conanFile{ "/v2/conans/zlib/1.3.1/_/_/revisions/" + conanRecipeRevision + "/files/conan_sources.tgz", - conanRecipeRevision + "_conan_sources.tgz", + "_/_/" + conanRecipeRevision + "/conan_sources.tgz", } conanZlibPackage = conanFile{ "/v2/conans/zlib/1.3.1/_/_/revisions/" + conanRecipeRevision + "/packages/0123abc/revisions/4567def/files/conan_package.tgz", - conanRecipeRevision + "_0123abc_4567def_conan_package.tgz", + "_/_/" + conanRecipeRevision + "/packages/0123abc/4567def/conan_package.tgz", } conanZlibAcme = conanFile{ "/v2/conans/zlib/1.2.13/acme/stable/revisions/" + conanRecipeRevision + "/files/conan_export.tgz", - "acme@stable_" + conanRecipeRevision + "_conan_export.tgz", + "acme/stable/" + conanRecipeRevision + "/conan_export.tgz", } conanOpenSSLSources = conanFile{ "/v2/conans/openssl/3.3.2/_/_/revisions/" + conanRecipeRevision + "/files/conan_sources.tgz", @@ -209,7 +209,7 @@ func TestConanRetentionPackageRule(t *testing.T) { t.Errorf("%s evicted despite the never-evict rule for zlib", f.filename) } } - if conanArtifact(t, db, "pkg:conan/openssl", conanRecipeRevision+"_conan_sources.tgz").StoragePath.Valid { + if conanArtifact(t, db, "pkg:conan/openssl", "_/_/"+conanRecipeRevision+"/conan_sources.tgz").StoragePath.Valid { t.Error("openssl kept past the conan rule") } } diff --git a/internal/server/retention_deb_test.go b/internal/server/retention_deb_test.go index 8081224f..3f6f61d2 100644 --- a/internal/server/retention_deb_test.go +++ b/internal/server/retention_deb_test.go @@ -20,8 +20,8 @@ import ( const ( debCurlPath = "/pool/main/c/curl/curl_8.14.1-2_amd64.deb" debBusyboxPath = "/pool/main/b/busybox/busybox_1.37.0-6_amd64.deb" - debCurlFile = "curl_8.14.1-2_amd64.deb" - debBusyboxFile = "busybox_1.37.0-6_amd64.deb" + debCurlFile = "pool/main/c/curl/curl_8.14.1-2_amd64.deb" + debBusyboxFile = "pool/main/b/busybox/busybox_1.37.0-6_amd64.deb" ) // debRetentionUpstream serves package files the way a Debian archive does,