From 589421f9952f6e74b35c63b6ae012b2e24ae49d7 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 9 Oct 2026 19:32:33 +1100 Subject: [PATCH] harden rpm virtual redirects, fail closed, keep data files resolvable - reject absolute and dot-segment member paths, escape the target, keep the query - return 502 when any member's repodata can't be fetched - serve recently merged data files by hash for 5m after repomd changes - fold xml:base under a member upstream into the virtual href --- internal/api/v1/proxy.go | 7 +- internal/virtual/engine.go | 126 ++++++++++++++++++++++------ internal/virtual/engine_test.go | 89 ++++++++++++++++++++ internal/virtual/rpm_merger.go | 23 ++++- internal/virtual/rpm_merger_test.go | 16 ++-- 5 files changed, 228 insertions(+), 33 deletions(-) create mode 100644 internal/virtual/engine_test.go diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index a056057..e77f43e 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -162,7 +162,12 @@ func (h *ProxyHandler) handleVirtual(w http.ResponseWriter, r *http.Request) { proxyBaseURL := fmt.Sprintf("%s://%s", scheme(r), r.Host) - if loc, ok := h.virtualEngine.MemberRedirect(r.Context(), *virt, path, proxyBaseURL); ok { + loc, ok, err := h.virtualEngine.MemberRedirect(r.Context(), *virt, path, r.URL.RawQuery, proxyBaseURL) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + if ok { http.Redirect(w, r, loc, http.StatusFound) return } diff --git a/internal/virtual/engine.go b/internal/virtual/engine.go index 0539493..0c680f2 100644 --- a/internal/virtual/engine.go +++ b/internal/virtual/engine.go @@ -8,9 +8,11 @@ import ( "log/slog" "net/http" "net/http/httptest" + "net/url" "slices" "strings" "sync" + "time" "git.unkin.net/unkin/artifactapi/internal/database" "git.unkin.net/unkin/artifactapi/internal/provider" @@ -21,10 +23,26 @@ import ( type Engine struct { db *database.DB proxyEngine *proxy.Engine + getRemote func(context.Context, string) (*models.Remote, error) + rpmMember func(context.Context, string) (*RPMMember, error) + + mu sync.Mutex + rpmFiles map[string]rpmFile } +type rpmFile struct { + body []byte + expires time.Time +} + +// rpmFileTTL keeps content-hashed data files resolvable after repomd moves on, +// so a client that fetched the previous repomd can still fetch its data. +const rpmFileTTL = 5 * time.Minute + func NewEngine(db *database.DB, proxyEngine *proxy.Engine) *Engine { - return &Engine{db: db, proxyEngine: proxyEngine} + e := &Engine{db: db, proxyEngine: proxyEngine, getRemote: db.GetRemote} + e.rpmMember = e.fetchRPMMember + return e } func (e *Engine) Fetch(ctx context.Context, virt models.Virtual, path string, proxyBaseURL string) ([]byte, string, error) { @@ -143,61 +161,89 @@ func (e *Engine) fetchLocalIndex(ctx context.Context, remote models.Remote, path return indexer.GenerateLocalIndex(ctx, e.db, remote.Name, path) } -var ErrNotFound = errors.New("not found") +var ( + ErrNotFound = errors.New("not found") + ErrBadPath = errors.New("invalid path") +) // MemberRedirect maps an rpm virtual package path (/, as written -// by MergeRPM) to the owning member's route. ok is false for any other path. -func (e *Engine) MemberRedirect(ctx context.Context, virt models.Virtual, path, proxyBaseURL string) (string, bool) { +// by MergeRPM) to the owning member's route. ok is false for any other path; +// ErrBadPath means the path is absolute or escapes the member. +func (e *Engine) MemberRedirect(ctx context.Context, virt models.Virtual, path, rawQuery, proxyBaseURL string) (string, bool, error) { if virt.PackageType != models.PackageRPM || strings.HasPrefix(path, "repodata/") { - return "", false + return "", false, nil } name, rest, found := strings.Cut(path, "/") if !found || !slices.Contains(virt.Members, name) { - return "", false + return "", false, nil } - remote, err := e.db.GetRemote(ctx, name) + segs, err := memberSegments(rest) if err != nil { - return "", false + return "", false, err } - return fmt.Sprintf("%s/api/v1/%s/%s/%s", strings.TrimRight(proxyBaseURL, "/"), remote.RepoType, name, rest), true + remote, err := e.getRemote(ctx, name) + if err != nil { + return "", false, nil + } + loc := fmt.Sprintf("%s/api/v1/%s/%s/%s", strings.TrimRight(proxyBaseURL, "/"), remote.RepoType, url.PathEscape(name), strings.Join(segs, "/")) + if rawQuery != "" { + loc += "?" + rawQuery + } + return loc, true, nil +} + +// memberSegments decodes rest and returns its path-escaped segments, rejecting +// absolute paths and any "." or ".." segment. +func memberSegments(rest string) ([]string, error) { + dec, err := url.PathUnescape(rest) + if err != nil || dec == "" || strings.HasPrefix(dec, "/") { + return nil, ErrBadPath + } + segs := strings.Split(dec, "/") + for i, s := range segs { + if s == "." || s == ".." || strings.Contains(s, "\\") { + return nil, ErrBadPath + } + segs[i] = url.PathEscape(s) + } + return segs, nil } func (e *Engine) fetchRPM(ctx context.Context, virt models.Virtual, path string) ([]byte, string, error) { if !strings.HasPrefix(path, "repodata/") { return nil, "", ErrNotFound } + if path != "repodata/repomd.xml" { + if body, ok := e.cachedRPMFile(path); ok { + return body, "application/gzip", nil + } + } - results := make([]*RPMMember, len(virt.Members)) + members := make([]RPMMember, len(virt.Members)) + errs := make([]error, len(virt.Members)) var wg sync.WaitGroup for i, name := range virt.Members { wg.Add(1) go func() { defer wg.Done() - m, err := e.fetchRPMMember(ctx, name) + m, err := e.rpmMember(ctx, name) if err != nil { - slog.Warn("virtual member fetch failed", "virtual", virt.Name, "member", name, "error", err) + errs[i] = fmt.Errorf("member %q: %w", name, err) return } - results[i] = m + members[i] = *m }() } wg.Wait() - - var members []RPMMember - for _, m := range results { - if m != nil { - members = append(members, *m) - } - } - if len(members) == 0 { - return nil, "", fmt.Errorf("no members reachable for virtual %q", virt.Name) + if err := errors.Join(errs...); err != nil { + return nil, "", fmt.Errorf("virtual %q: %w", virt.Name, err) } - // ponytail: merges on every repodata request; cache keyed on member repomd checksums if member count/size makes this slow. repo, err := MergeRPM(members) if err != nil { return nil, "", fmt.Errorf("merge rpm repodata: %w", err) } + e.storeRPMFiles(repo.Files) if path == "repodata/repomd.xml" { return repo.Repomd, "application/xml", nil } @@ -207,8 +253,38 @@ func (e *Engine) fetchRPM(ctx context.Context, virt models.Virtual, path string) return nil, "", ErrNotFound } +// ponytail: per-replica cache; behind a non-sticky LB a data request landing on +// another replica re-merges and 404s if a member changed in between. Move to the +// shared redis cache if that shows up. +func (e *Engine) storeRPMFiles(files map[string][]byte) { + now := time.Now() + e.mu.Lock() + defer e.mu.Unlock() + if e.rpmFiles == nil { + e.rpmFiles = map[string]rpmFile{} + } + for k, f := range e.rpmFiles { + if now.After(f.expires) { + delete(e.rpmFiles, k) + } + } + for k, body := range files { + e.rpmFiles[k] = rpmFile{body: body, expires: now.Add(rpmFileTTL)} + } +} + +func (e *Engine) cachedRPMFile(path string) ([]byte, bool) { + e.mu.Lock() + defer e.mu.Unlock() + f, ok := e.rpmFiles[path] + if !ok || time.Now().After(f.expires) { + return nil, false + } + return f.body, true +} + func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, error) { - remote, err := e.db.GetRemote(ctx, name) + remote, err := e.getRemote(ctx, name) if err != nil { return nil, fmt.Errorf("remote %q: %w", name, err) } @@ -220,7 +296,7 @@ func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, e if err != nil { return nil, err } - m := &RPMMember{RemoteName: name, Timestamp: ts, Data: map[string][]byte{}} + m := &RPMMember{RemoteName: name, Bases: remote.UpstreamPool(), Timestamp: ts, Data: map[string][]byte{}} for t, href := range locs { raw, err := e.fetchMemberPath(ctx, *remote, href) if err != nil { diff --git a/internal/virtual/engine_test.go b/internal/virtual/engine_test.go new file mode 100644 index 0000000..0571a56 --- /dev/null +++ b/internal/virtual/engine_test.go @@ -0,0 +1,89 @@ +package virtual + +import ( + "context" + "errors" + "regexp" + "testing" + + "git.unkin.net/unkin/artifactapi/pkg/models" +) + +func fakeEngine(members map[string]*RPMMember) *Engine { + return &Engine{ + getRemote: func(_ context.Context, name string) (*models.Remote, error) { + return &models.Remote{Name: name, RepoType: models.RepoTypeRemote}, nil + }, + rpmMember: func(_ context.Context, name string) (*RPMMember, error) { + if m := members[name]; m != nil { + return m, nil + } + return nil, errors.New("upstream down") + }, + } +} + +func rpmVirt(members ...string) models.Virtual { + return models.Virtual{Name: "v", PackageType: models.PackageRPM, Members: members} +} + +func TestFetchRPMFailsClosed(t *testing.T) { + e := fakeEngine(map[string]*RPMMember{ + "a": {RemoteName: "a", Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}}, + }) + _, _, err := e.Fetch(context.Background(), rpmVirt("a", "b"), "repodata/repomd.xml", "") + if err == nil || errors.Is(err, ErrNotFound) { + t.Fatalf("want upstream error with a member down, got %v", err) + } +} + +func TestFetchRPMDataSurvivesMemberChange(t *testing.T) { + m := &RPMMember{RemoteName: "a", Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}} + e := fakeEngine(map[string]*RPMMember{"a": m}) + virt := rpmVirt("a") + + repomd, _, err := e.Fetch(context.Background(), virt, "repodata/repomd.xml", "") + if err != nil { + t.Fatal(err) + } + m.Data = map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "2", "bbb", "foo2.rpm"))} + + href := regexp.MustCompile(`repodata/[0-9a-f]+-primary\.xml\.gz`).Find(repomd) + body, _, err := e.Fetch(context.Background(), virt, string(href), "") + if err != nil { + t.Fatalf("data from previous repomd must stay resolvable: %v", err) + } + if got := string(gunzip(t, body)); !regexp.MustCompile(`ver="1"`).MatchString(got) { + t.Fatalf("served wrong primary:\n%s", got) + } + + newMD, _, err := e.Fetch(context.Background(), virt, "repodata/repomd.xml", "") + if err != nil || string(newMD) == string(repomd) { + t.Fatalf("repomd should reflect the member change (err %v)", err) + } +} + +func TestMemberRedirect(t *testing.T) { + e := fakeEngine(nil) + virt := rpmVirt("gh", "other") + for _, tc := range []struct { + path, query, want string + ok bool + err error + }{ + {path: "gh/o/r/releases/download/v1/c++-1.rpm", query: "x=1", ok: true, + want: "http://h/api/v1/remote/gh/o/r/releases/download/v1/c++-1.rpm?x=1"}, + {path: "gh/a%20b/c%3Fd.rpm", ok: true, want: "http://h/api/v1/remote/gh/a%20b/c%3Fd.rpm"}, + {path: "gh/../../remote/other/x", err: ErrBadPath}, + {path: "gh/%2e%2e/%2e%2e/remote/other/x", err: ErrBadPath}, + {path: "gh/a/./b", err: ErrBadPath}, + {path: "gh//etc/passwd", err: ErrBadPath}, + {path: "repodata/repomd.xml"}, + {path: "nope/x.rpm"}, + } { + got, ok, err := e.MemberRedirect(context.Background(), virt, tc.path, tc.query, "http://h/") + if got != tc.want || ok != tc.ok || !errors.Is(err, tc.err) { + t.Errorf("%s: got (%q, %v, %v), want (%q, %v, %v)", tc.path, got, ok, err, tc.want, tc.ok, tc.err) + } + } +} diff --git a/internal/virtual/rpm_merger.go b/internal/virtual/rpm_merger.go index 2fda0b4..53f5c50 100644 --- a/internal/virtual/rpm_merger.go +++ b/internal/virtual/rpm_merger.go @@ -33,6 +33,7 @@ var locationRe = regexp.MustCompile(`]*/>`) // keyed by repomd data type. Missing types contribute no packages. type RPMMember struct { RemoteName string + Bases []string Timestamp int64 Data map[string][]byte } @@ -161,6 +162,24 @@ func splitPackages(doc []byte) ([][]byte, error) { } } +// memberHref returns href relative to the member root. An xml:base under one of +// the member's upstream bases is folded into the href; any other xml:base is a +// host the member can't serve, so ok is false and the location is left as is. +func memberHref(m RPMMember, base, href string) (string, bool) { + href = strings.TrimLeft(href, "/") + if base == "" { + return href, true + } + base = strings.TrimRight(base, "/") + "/" + for _, b := range m.Bases { + b = strings.TrimRight(b, "/") + "/" + if strings.HasPrefix(base, b) { + return strings.TrimPrefix(base, b) + href, true + } + } + return "", false +} + func xmlEscape(s string) string { var b bytes.Buffer _ = xml.EscapeText(&b, []byte(s)) @@ -196,8 +215,8 @@ func MergeRPM(members []RPMMember) (*RPMRepo, error) { seen[nevra] = true kept[fmt.Sprintf("%d/%s", i, p.Checksum)] = true - if p.Location.Base == "" { - loc := `` + if href, ok := memberHref(m, p.Location.Base, p.Location.Href); ok { + loc := `` raw = locationRe.ReplaceAll(raw, []byte(loc)) } out["primary"] = append(out["primary"], raw) diff --git a/internal/virtual/rpm_merger_test.go b/internal/virtual/rpm_merger_test.go index 144cfeb..cda6d50 100644 --- a/internal/virtual/rpm_merger_test.go +++ b/internal/virtual/rpm_merger_test.go @@ -145,13 +145,17 @@ func TestMergeRPMOutputIsWellFormed(t *testing.T) { } func TestMergeRPMHrefRewriting(t *testing.T) { - based := `extnoarch` + - `eee` - repo, err := MergeRPM([]RPMMember{{RemoteName: "gh", Data: map[string][]byte{ + based := func(name, base string) string { + return `` + name + `noarch` + + `` + name + `` + } + repo, err := MergeRPM([]RPMMember{{RemoteName: "gh", Bases: []string{"https://up.example/el9", "https://mirror.example/el9/"}, Data: map[string][]byte{ "primary": primaryXML( primaryPkgXML("foo", "1.0", "aaa", "storytold/photocraft/releases/download/v1.0/foo&bar.rpm"), primaryPkgXML("lead", "1.0", "bbb", "/Packages/lead.rpm"), - based, + based("root", "https://up.example/el9/"), + based("sub", "https://mirror.example/el9/extra"), + based("ext", "https://other.example/"), ), }}}) if err != nil { @@ -161,7 +165,9 @@ func TestMergeRPMHrefRewriting(t *testing.T) { for _, want := range []string{ ``, ``, - ``, + ``, + ``, + ``, } { if !strings.Contains(primary, want) { t.Errorf("missing %s in:\n%s", want, primary)