From cfb1f93d489e0208831b4e77da025de038af7ff4 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 15:13:57 +1100 Subject: [PATCH 1/3] Add parallel_fetch toggle for virtual member index files (#165) Virtual members already merge concurrently, but each rpm member fetches primary/filelists/other one after another, and each deb member its Packages files the same way. On large virtuals the slowest member's serial chain pushes the merge past dnf's 30s timeout. - add `parallel_fetch` (default false) to virtuals: model, v2 API, migration 0007 - fetch a member's index files concurrently (up to 4) when set; first error still fails the member - thread the flag through rpm and deb member fetches Reviewed-on: https://git.unkin.net/unkin/artifactapi/pulls/165 Co-authored-by: unkin-agent Co-committed-by: unkin-agent --- internal/api/v2/virtuals_test.go | 56 ++++++ internal/database/database_test.go | 34 ++++ internal/database/virtuals.go | 18 +- internal/virtual/engine.go | 104 +++++++--- internal/virtual/engine_test.go | 218 ++++++++++++++++++++- migrations/0007_virtual_parallel_fetch.sql | 1 + pkg/models/virtual.go | 15 +- 7 files changed, 395 insertions(+), 51 deletions(-) create mode 100644 internal/api/v2/virtuals_test.go create mode 100644 migrations/0007_virtual_parallel_fetch.sql diff --git a/internal/api/v2/virtuals_test.go b/internal/api/v2/virtuals_test.go new file mode 100644 index 0000000..2513556 --- /dev/null +++ b/internal/api/v2/virtuals_test.go @@ -0,0 +1,56 @@ +package v2 + +import ( + "encoding/json" + "net/http/httptest" + "testing" + + "git.unkin.net/unkin/artifactapi/internal/database" +) + +func TestVirtualParallelFetchRoundTrip(t *testing.T) { + if testDSN == "" { + t.Skip("Docker unavailable") + } + db, err := database.New(testDSN) + if err != nil { + t.Fatal(err) + } + defer db.Close() + h := NewVirtualsHandler(db).Routes() + parallel := func() bool { + t.Helper() + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("GET", "/vpf", nil)) + var got map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil || w.Code != 200 { + t.Fatalf("get = %d %s", w.Code, w.Body) + } + v, ok := got["parallel_fetch"].(bool) + if !ok { + t.Fatalf("parallel_fetch missing from %s", w.Body) + } + return v + } + const body = `"package_type":"rpm","members":["a"]` + + steps := []struct { + method, path, json string + want bool + }{ + {"POST", "/", `{"name":"vpf",` + body + `}`, false}, + {"PUT", "/vpf", `{` + body + `,"parallel_fetch":true}`, true}, + {"PUT", "/vpf", `{` + body + `,"parallel_fetch":false}`, false}, + } + for i, s := range steps { + if c := do(t, h, s.method, s.path, s.json); c/100 != 2 { + t.Fatalf("step %d %s %s = %d", i, s.method, s.path, c) + } + if got := parallel(); got != s.want { + t.Errorf("step %d: parallel_fetch = %v, want %v", i, got, s.want) + } + } + if c := do(t, h, "DELETE", "/vpf", ""); c != 204 { + t.Fatalf("delete = %d", c) + } +} diff --git a/internal/database/database_test.go b/internal/database/database_test.go index ef0e8a9..c97f314 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -335,6 +335,40 @@ func TestDeleteRemoteDropsLocalFiles(t *testing.T) { } } +func TestVirtualParallelFetchRoundTrip(t *testing.T) { + requireDB(t) + if err := testDB.CreateVirtual(ctx(), &models.Virtual{ + Name: "v-parallel", PackageType: models.PackageRPM, Members: []string{"a"}, ParallelFetch: true, + }); err != nil { + t.Fatal(err) + } + defer func() { _ = testDB.DeleteVirtual(ctx(), "v-parallel") }() + v, err := testDB.GetVirtual(ctx(), "v-parallel") + if err != nil || !v.ParallelFetch { + t.Fatalf("created parallel_fetch = %v, %v; want true", v, err) + } + v.ParallelFetch = false + if err := testDB.UpdateVirtual(ctx(), v); err != nil { + t.Fatal(err) + } + vs, err := testDB.ListVirtuals(ctx()) + if err != nil { + t.Fatal(err) + } + for _, l := range vs { + if l.Name == "v-parallel" && l.ParallelFetch { + t.Fatal("updated parallel_fetch not persisted") + } + } + if err := testDB.CreateVirtual(ctx(), &models.Virtual{Name: "v-serial", PackageType: models.PackageRPM, Members: []string{"a"}}); err != nil { + t.Fatal(err) + } + defer func() { _ = testDB.DeleteVirtual(ctx(), "v-serial") }() + if v, err := testDB.GetVirtual(ctx(), "v-serial"); err != nil || v.ParallelFetch { + t.Fatalf("default parallel_fetch = %v, %v; want false", v, err) + } +} + func TestVirtualsCRUD(t *testing.T) { requireDB(t) if err := testDB.CreateVirtual(ctx(), &models.Virtual{ diff --git a/internal/database/virtuals.go b/internal/database/virtuals.go index ffe815c..11d6a06 100644 --- a/internal/database/virtuals.go +++ b/internal/database/virtuals.go @@ -8,12 +8,12 @@ import ( func (db *DB) GetVirtual(ctx context.Context, name string) (*models.Virtual, error) { row := db.Pool.QueryRow(ctx, ` - SELECT name, package_type, description, members, managed_by, created_at, updated_at + SELECT name, package_type, description, members, managed_by, parallel_fetch, created_at, updated_at FROM virtuals WHERE name = $1 `, name) var v models.Virtual - err := row.Scan(&v.Name, &v.PackageType, &v.Description, &v.Members, &v.ManagedBy, &v.CreatedAt, &v.UpdatedAt) + err := row.Scan(&v.Name, &v.PackageType, &v.Description, &v.Members, &v.ManagedBy, &v.ParallelFetch, &v.CreatedAt, &v.UpdatedAt) if err != nil { return nil, err } @@ -22,7 +22,7 @@ func (db *DB) GetVirtual(ctx context.Context, name string) (*models.Virtual, err func (db *DB) ListVirtuals(ctx context.Context) ([]models.Virtual, error) { rows, err := db.Pool.Query(ctx, ` - SELECT name, package_type, description, members, managed_by, created_at, updated_at + SELECT name, package_type, description, members, managed_by, parallel_fetch, created_at, updated_at FROM virtuals ORDER BY name `) if err != nil { @@ -33,7 +33,7 @@ func (db *DB) ListVirtuals(ctx context.Context) ([]models.Virtual, error) { var virtuals []models.Virtual for rows.Next() { var v models.Virtual - if err := rows.Scan(&v.Name, &v.PackageType, &v.Description, &v.Members, &v.ManagedBy, &v.CreatedAt, &v.UpdatedAt); err != nil { + if err := rows.Scan(&v.Name, &v.PackageType, &v.Description, &v.Members, &v.ManagedBy, &v.ParallelFetch, &v.CreatedAt, &v.UpdatedAt); err != nil { return nil, err } virtuals = append(virtuals, v) @@ -43,18 +43,18 @@ func (db *DB) ListVirtuals(ctx context.Context) ([]models.Virtual, error) { func (db *DB) CreateVirtual(ctx context.Context, v *models.Virtual) error { _, err := db.Pool.Exec(ctx, ` - INSERT INTO virtuals (name, package_type, description, members, managed_by) - VALUES ($1, $2, $3, $4, $5) - `, v.Name, v.PackageType, v.Description, v.Members, v.ManagedBy) + INSERT INTO virtuals (name, package_type, description, members, managed_by, parallel_fetch) + VALUES ($1, $2, $3, $4, $5, $6) + `, v.Name, v.PackageType, v.Description, v.Members, v.ManagedBy, v.ParallelFetch) return err } func (db *DB) UpdateVirtual(ctx context.Context, v *models.Virtual) error { _, err := db.Pool.Exec(ctx, ` UPDATE virtuals SET - package_type=$2, description=$3, members=$4, managed_by=$5, updated_at=NOW() + package_type=$2, description=$3, members=$4, managed_by=$5, parallel_fetch=$6, updated_at=NOW() WHERE name=$1 - `, v.Name, v.PackageType, v.Description, v.Members, v.ManagedBy) + `, v.Name, v.PackageType, v.Description, v.Members, v.ManagedBy, v.ParallelFetch) return err } diff --git a/internal/virtual/engine.go b/internal/virtual/engine.go index 9ab1905..a47b1dc 100644 --- a/internal/virtual/engine.go +++ b/internal/virtual/engine.go @@ -19,6 +19,7 @@ import ( "git.unkin.net/unkin/artifactapi/internal/provider" "git.unkin.net/unkin/artifactapi/internal/proxy" "git.unkin.net/unkin/artifactapi/pkg/models" + "golang.org/x/sync/errgroup" "golang.org/x/sync/singleflight" ) @@ -26,10 +27,11 @@ type Engine struct { db *database.DB proxyEngine *proxy.Engine getRemote func(context.Context, string) (*models.Remote, error) - rpmMember func(context.Context, string) (*RPMMember, error) - debMember func(ctx context.Context, name, base string) (*DebMember, error) + rpmMember func(ctx context.Context, name string, parallel bool) (*RPMMember, error) + debMember func(ctx context.Context, name, base string, parallel bool) (*DebMember, error) apkMember func(ctx context.Context, name, dir string) (*AlpineMember, error) npmMember func(ctx context.Context, name, pkg string) (*NPMMember, error) + memberPath func(ctx context.Context, remote models.Remote, path string) ([]byte, error) // NPMLocal returns a local npm repo's packument with tarball paths relative // to the repo, or ErrNotFound. @@ -91,6 +93,7 @@ func NewEngine(db *database.DB, proxyEngine *proxy.Engine) *Engine { e.ansibleDoc = e.ansibleMemberDoc e.goMember = e.fetchGoMember e.puppetDoc = e.fetchMemberPath + e.memberPath = e.fetchMemberPath return e } @@ -353,7 +356,7 @@ func (e *Engine) merged(ctx context.Context, key string, merge func(context.Cont } func (e *Engine) mergeRPM(ctx context.Context, virt models.Virtual) (*RPMRepo, error) { - members, err := fetchMembers(virt, func(name string) (*RPMMember, error) { return e.rpmMember(ctx, name) }) + members, err := fetchMembers(virt, func(name string) (*RPMMember, error) { return e.rpmMember(ctx, name, virt.ParallelFetch) }) if err != nil { return nil, err } @@ -402,7 +405,7 @@ func (e *Engine) fetchDeb(ctx context.Context, virt models.Virtual, reqPath stri } repo, err := e.merged(ctx, key, func(ctx context.Context) (mergedRepo, error) { - members, err := fetchMembers(virt, func(name string) (*DebMember, error) { return e.debMember(ctx, name, base) }) + members, err := fetchMembers(virt, func(name string) (*DebMember, error) { return e.debMember(ctx, name, base, virt.ParallelFetch) }) if err != nil { return nil, err } @@ -430,26 +433,22 @@ func debContentType(name string) string { // fetchDebMember reads a member's Release under base and each binary Packages // index it lists. -func (e *Engine) fetchDebMember(ctx context.Context, name, base string) (*DebMember, error) { +func (e *Engine) fetchDebMember(ctx context.Context, name, base string, parallel bool) (*DebMember, error) { remote, err := e.getRemote(ctx, name) if err != nil { return nil, fmt.Errorf("remote %q: %w", name, err) } - rel, err := e.fetchMemberPath(ctx, *remote, base+"Release") + rel, err := e.memberPath(ctx, *remote, base+"Release") if err != nil { return nil, err } - m := &DebMember{RemoteName: name, Release: rel, Packages: map[string][]byte{}} - for dir, file := range debIndexFiles(parseDebRelease(rel)) { - raw, err := e.fetchMemberPath(ctx, *remote, base+file) - if err != nil { - return nil, err - } - if m.Packages[dir], err = decompress(file, raw); err != nil { - return nil, fmt.Errorf("decompress %s: %w", file, err) - } + pkgs, err := fetchFiles(ctx, parallel, debIndexFiles(parseDebRelease(rel)), func(ctx context.Context, file string) ([]byte, error) { + return e.fetchDecompressed(ctx, *remote, base, file) + }) + if err != nil { + return nil, err } - return m, nil + return &DebMember{RemoteName: name, Release: rel, Packages: pkgs}, nil } func (e *Engine) mergedAlpine(ctx context.Context, virt models.Virtual, dir string) (mergedRepo, error) { @@ -559,12 +558,12 @@ func (e *Engine) cachedFile(key, path string) ([]byte, bool) { return nil, false } -func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, error) { +func (e *Engine) fetchRPMMember(ctx context.Context, name string, parallel bool) (*RPMMember, error) { remote, err := e.getRemote(ctx, name) if err != nil { return nil, fmt.Errorf("remote %q: %w", name, err) } - repomd, err := e.fetchMemberPath(ctx, *remote, "repodata/repomd.xml") + repomd, err := e.memberPath(ctx, *remote, "repodata/repomd.xml") if err != nil { return nil, err } @@ -572,17 +571,66 @@ func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, e if err != nil { return nil, err } - 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 { - return nil, err - } - if m.Data[t], err = decompress(href, raw); err != nil { - return nil, fmt.Errorf("decompress %s: %w", href, err) - } + data, err := fetchFiles(ctx, parallel, locs, func(ctx context.Context, href string) ([]byte, error) { + return e.fetchDecompressed(ctx, *remote, "", href) + }) + if err != nil { + return nil, err } - return m, nil + return &RPMMember{RemoteName: name, Bases: remote.UpstreamPool(), Timestamp: ts, Data: data}, nil +} + +func (e *Engine) fetchDecompressed(ctx context.Context, remote models.Remote, base, file string) ([]byte, error) { + raw, err := e.memberPath(ctx, remote, base+file) + if err != nil { + return nil, err + } + body, err := decompress(file, raw) + if err != nil { + return nil, fmt.Errorf("decompress %s: %w", file, err) + } + return body, nil +} + +// maxFileFetches bounds the concurrent index file fetches per member. +const maxFileFetches = 4 + +// fetchFiles fetches each key's file, one at a time or up to maxFileFetches at +// once, failing on the first error and cancelling the rest. +func fetchFiles(ctx context.Context, parallel bool, files map[string]string, fetch func(ctx context.Context, file string) ([]byte, error)) (map[string][]byte, error) { + out := make(map[string][]byte, len(files)) + if !parallel { + for k, f := range files { + body, err := fetch(ctx, f) + if err != nil { + return nil, err + } + out[k] = body + } + return out, nil + } + var mu sync.Mutex + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(maxFileFetches) + for k, f := range files { + g.Go(func() error { + if err := gctx.Err(); err != nil { + return err + } + body, err := fetch(gctx, f) + if err != nil { + return err + } + mu.Lock() + out[k] = body + mu.Unlock() + return nil + }) + } + if err := g.Wait(); err != nil { + return nil, err + } + return out, nil } // fetchMemberPath reads one path from a member the same way its own route diff --git a/internal/virtual/engine_test.go b/internal/virtual/engine_test.go index 04a6a82..abd22ff 100644 --- a/internal/virtual/engine_test.go +++ b/internal/virtual/engine_test.go @@ -19,7 +19,7 @@ func fakeEngine(members map[string]*RPMMember) *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) { + rpmMember: func(_ context.Context, name string, _ bool) (*RPMMember, error) { if m := members[name]; m != nil { return m, nil } @@ -75,7 +75,7 @@ func TestFetchRPMLocalMemberWithoutRepodataIsUpstreamError(t *testing.T) { e := fakeEngine(map[string]*RPMMember{ "a": {RemoteName: "a", Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}}, }) - e.rpmMember = func(_ context.Context, name string) (*RPMMember, error) { + e.rpmMember = func(_ context.Context, name string, _ bool) (*RPMMember, error) { if name == "local" { return nil, fmt.Errorf("local/repodata/repomd.xml: %w", ErrNotFound) } @@ -91,7 +91,7 @@ func TestFetchRPMMergeIsShared(t *testing.T) { var calls atomic.Int32 e := fakeEngine(nil) e.mergeTTL = time.Minute - e.rpmMember = func(_ context.Context, name string) (*RPMMember, error) { + e.rpmMember = func(_ context.Context, name string, _ bool) (*RPMMember, error) { calls.Add(1) time.Sleep(20 * time.Millisecond) return &RPMMember{RemoteName: name, Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}}, nil @@ -160,7 +160,7 @@ func TestMemberRedirect(t *testing.T) { func debEngine(members map[string]*DebMember) *Engine { e := fakeEngine(nil) - e.debMember = func(_ context.Context, name, base string) (*DebMember, error) { + e.debMember = func(_ context.Context, name, base string, _ bool) (*DebMember, error) { if m := members[name]; m != nil && base == "" { return m, nil } @@ -213,7 +213,7 @@ func TestFetchDebMergeIsSharedPerSuite(t *testing.T) { var calls atomic.Int32 e := fakeEngine(nil) e.mergeTTL = time.Minute - e.debMember = func(_ context.Context, name, base string) (*DebMember, error) { + e.debMember = func(_ context.Context, name, base string, _ bool) (*DebMember, error) { calls.Add(1) m := flatDebMember(name, "", debStanza("foo", "1", "amd64", "pool/foo.deb")) return &m, nil @@ -307,7 +307,9 @@ func suiteDebMember(name, version string) *DebMember { func TestFetchDebByHashSurvivesRemerge(t *testing.T) { version := "1" e := fakeEngine(nil) - e.debMember = func(_ context.Context, name, _ string) (*DebMember, error) { return suiteDebMember(name, version), nil } + e.debMember = func(_ context.Context, name, _ string, _ bool) (*DebMember, error) { + return suiteDebMember(name, version), nil + } virt := debVirt("a") fetch := func(p string) []byte { body, _, err := e.Fetch(context.Background(), virt, p, "") @@ -340,7 +342,9 @@ func TestFetchDebByHashSurvivesRemerge(t *testing.T) { func TestFetchDebBoundsSuitesPerVirtual(t *testing.T) { e := fakeEngine(nil) e.mergeTTL = time.Minute - e.debMember = func(_ context.Context, name, _ string) (*DebMember, error) { return suiteDebMember(name, "1"), nil } + e.debMember = func(_ context.Context, name, _ string, _ bool) (*DebMember, error) { + return suiteDebMember(name, "1"), nil + } other := models.Virtual{Name: "other", PackageType: models.PackageDeb, Members: []string{"a"}} if _, _, err := e.Fetch(context.Background(), other, "dists/keep/Release", ""); err != nil { t.Fatal(err) @@ -367,3 +371,203 @@ func TestFetchDebBoundsSuitesPerVirtual(t *testing.T) { t.Fatal("another virtual's suites must not be evicted") } } + +func fetchFilesInput(n int) map[string]string { + files := map[string]string{} + for i := range n { + files[fmt.Sprintf("k%d", i)] = fmt.Sprintf("f%d", i) + } + return files +} + +func TestFetchFilesParallelRunsConcurrently(t *testing.T) { + var inFlight, peak atomic.Int32 + release := make(chan struct{}) + go func() { + for peak.Load() < maxFileFetches { + time.Sleep(time.Millisecond) + } + close(release) + }() + got, err := fetchFiles(context.Background(), true, fetchFilesInput(6), func(_ context.Context, f string) ([]byte, error) { + n := inFlight.Add(1) + defer inFlight.Add(-1) + for p := peak.Load(); n > p && !peak.CompareAndSwap(p, n); p = peak.Load() { + } + select { + case <-release: + case <-time.After(5 * time.Second): + return nil, errors.New("fetches never overlapped") + } + return []byte(f), nil + }) + if err != nil { + t.Fatal(err) + } + if peak.Load() != maxFileFetches { + t.Fatalf("peak concurrency %d, want %d", peak.Load(), maxFileFetches) + } + for k, f := range fetchFilesInput(6) { + if string(got[k]) != f { + t.Fatalf("%s = %q, want %q", k, got[k], f) + } + } +} + +func TestFetchFilesSerialByDefault(t *testing.T) { + var inFlight atomic.Int32 + _, err := fetchFiles(context.Background(), false, fetchFilesInput(4), func(_ context.Context, f string) ([]byte, error) { + defer inFlight.Add(-1) + if inFlight.Add(1) > 1 { + return nil, errors.New("concurrent fetch") + } + time.Sleep(time.Millisecond) + return []byte(f), nil + }) + if err != nil { + t.Fatal(err) + } +} + +func TestFetchFilesErrorFailsMember(t *testing.T) { + boom := errors.New("boom") + for _, parallel := range []bool{false, true} { + got, err := fetchFiles(context.Background(), parallel, fetchFilesInput(5), func(_ context.Context, f string) ([]byte, error) { + if f == "f2" { + return nil, boom + } + return []byte(f), nil + }) + if !errors.Is(err, boom) || got != nil { + t.Fatalf("parallel=%v: got %v, %v; want boom and no files", parallel, got, err) + } + } +} + +func TestMergeRPMPassesParallelFetchAndKeepsMemberOrder(t *testing.T) { + e := fakeEngine(nil) + var sawParallel atomic.Bool + e.rpmMember = func(_ context.Context, name string, parallel bool) (*RPMMember, error) { + sawParallel.Store(parallel) + if name == "a" { + time.Sleep(20 * time.Millisecond) + } + return &RPMMember{RemoteName: name, Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", name+"sum", name+".rpm"))}}, nil + } + virt := rpmVirt("a", "b") + virt.ParallelFetch = true + repo, err := e.mergeRPM(context.Background(), virt) + if err != nil { + t.Fatal(err) + } + if !sawParallel.Load() { + t.Fatal("parallel_fetch not passed to member fetch") + } + primary := regexp.MustCompile(`repodata/[0-9a-f]+-primary\.xml\.gz`).FindString(string(repo.Repomd)) + body := string(gunzip(t, repo.Files[primary])) + if a, b := strings.Index(body, "a.rpm"), strings.Index(body, "b.rpm"); a < 0 || b >= 0 && b < a { + t.Fatalf("member priority order lost:\n%s", body) + } +} + +func TestFetchFilesErrorCancelsRest(t *testing.T) { + boom := errors.New("boom") + var calls atomic.Int32 + _, err := fetchFiles(context.Background(), true, fetchFilesInput(8), func(ctx context.Context, _ string) ([]byte, error) { + if calls.Add(1) == 1 { + return nil, boom + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(5 * time.Second): + return nil, errors.New("in-flight fetch not cancelled") + } + }) + if !errors.Is(err, boom) { + t.Fatalf("got %v, want boom", err) + } + if n := calls.Load(); n > maxFileFetches { + t.Fatalf("%d fetches started after the first error, want at most %d", n, maxFileFetches) + } +} + +// overlapEngine serves fixed member index paths and records the peak number of +// index file fetches in flight. Each fetch blocks until want are in flight (or +// fails after 5s); with want 1 it also holds briefly so a stray overlap shows. +func overlapEngine(index map[string][]byte, want int32, peak *atomic.Int32) *Engine { + e := fakeEngine(nil) + var inFlight atomic.Int32 + e.memberPath = func(_ context.Context, _ models.Remote, path string) ([]byte, error) { + body, ok := index[path] + if !ok { + return nil, fmt.Errorf("%s: %w", path, ErrNotFound) + } + if strings.HasSuffix(path, "Release") || strings.HasSuffix(path, "repomd.xml") { + return body, nil + } + n := inFlight.Add(1) + defer inFlight.Add(-1) + for p := peak.Load(); n > p && !peak.CompareAndSwap(p, n); p = peak.Load() { + } + for deadline := time.Now().Add(5 * time.Second); peak.Load() < want; { + if time.Now().After(deadline) { + return nil, fmt.Errorf("%s: only %d of %d fetches in flight", path, peak.Load(), want) + } + time.Sleep(time.Millisecond) + } + if want == 1 { + time.Sleep(10 * time.Millisecond) + } + return body, nil + } + return e +} + +func TestFetchRPMMemberHonoursParallelFetch(t *testing.T) { + index := map[string][]byte{ + "repodata/repomd.xml": []byte(`` + + `` + + `` + + ``), + "repodata/p.xml": []byte("P"), "repodata/f.xml": []byte("F"), "repodata/o.xml": []byte("O"), + } + for parallel, want := range map[bool]int32{false: 1, true: 3} { + var peak atomic.Int32 + m, err := overlapEngine(index, want, &peak).fetchRPMMember(context.Background(), "a", parallel) + if err != nil { + t.Fatal(err) + } + if peak.Load() != want { + t.Errorf("parallel=%v: peak concurrency %d, want %d", parallel, peak.Load(), want) + } + if string(m.Data["primary"]) != "P" || string(m.Data["filelists"]) != "F" || string(m.Data["other"]) != "O" { + t.Errorf("parallel=%v: data %q", parallel, m.Data) + } + } +} + +func TestFetchDebMemberHonoursParallelFetch(t *testing.T) { + index := map[string][]byte{ + "dists/s/Release": []byte("Suite: s\nSHA256:\n" + + " aa 1 main/binary-amd64/Packages\n" + + " bb 1 contrib/binary-amd64/Packages\n" + + " cc 1 non-free/binary-amd64/Packages\n"), + "dists/s/main/binary-amd64/Packages": []byte("M"), + "dists/s/contrib/binary-amd64/Packages": []byte("C"), + "dists/s/non-free/binary-amd64/Packages": []byte("N"), + } + for parallel, want := range map[bool]int32{false: 1, true: 3} { + var peak atomic.Int32 + m, err := overlapEngine(index, want, &peak).fetchDebMember(context.Background(), "a", "dists/s/", parallel) + if err != nil { + t.Fatal(err) + } + if peak.Load() != want { + t.Errorf("parallel=%v: peak concurrency %d, want %d", parallel, peak.Load(), want) + } + if string(m.Packages["main/binary-amd64/"]) != "M" || string(m.Packages["non-free/binary-amd64/"]) != "N" { + t.Errorf("parallel=%v: packages %q", parallel, m.Packages) + } + } +} diff --git a/migrations/0007_virtual_parallel_fetch.sql b/migrations/0007_virtual_parallel_fetch.sql new file mode 100644 index 0000000..0178cb1 --- /dev/null +++ b/migrations/0007_virtual_parallel_fetch.sql @@ -0,0 +1 @@ +ALTER TABLE virtuals ADD COLUMN IF NOT EXISTS parallel_fetch BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/pkg/models/virtual.go b/pkg/models/virtual.go index c7be7b2..83a39fa 100644 --- a/pkg/models/virtual.go +++ b/pkg/models/virtual.go @@ -3,11 +3,12 @@ package models import "time" type Virtual struct { - Name string `json:"name"` - PackageType PackageType `json:"package_type"` - Description string `json:"description,omitempty"` - Members []string `json:"members"` - ManagedBy string `json:"managed_by,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + Name string `json:"name"` + PackageType PackageType `json:"package_type"` + Description string `json:"description,omitempty"` + Members []string `json:"members"` + ManagedBy string `json:"managed_by,omitempty"` + ParallelFetch bool `json:"parallel_fetch"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } From 6cf84c4f6c4159bb78f1400e76325476c1584874 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 15:16:23 +1100 Subject: [PATCH 2/3] Enforce remote quarantine for fresh immutable artifacts (#170) Remotes store `quarantine_enabled`/`quarantine_days` but the proxy never enforces them, so freshly published (possibly malicious) artifacts are cached immediately. - refuse uncached immutable paths published under `quarantine_days` ago with a JSON 403 - take publish time from `Last-Modified` capped at now, else a Redis first-seen key; 503 + `Retry-After` when that lookup fails - reject enabled quarantine with `quarantine_days<1`; backfill stored `<=0` to 3 - default `quarantine_days` to 3 on create and keep it on update - let goproxy virtual probes skip a quarantining member Reviewed-on: https://git.unkin.net/unkin/artifactapi/pulls/170 Co-authored-by: unkin-agent Co-committed-by: unkin-agent --- docs/common/caching.md | 20 ++- internal/api/v1/proxy.go | 24 ++- internal/api/v1/proxyerror_test.go | 16 ++ internal/api/v2/remotes.go | 11 +- internal/api/v2/remotes_ttl_test.go | 64 ++++++++ internal/cache/redis.go | 14 ++ internal/proxy/engine.go | 66 ++++++++- internal/proxy/engine_test.go | 2 +- internal/proxy/quarantine_test.go | 186 ++++++++++++++++++++++++ internal/virtual/goproxy_merger.go | 13 +- internal/virtual/goproxy_merger_test.go | 33 +++++ migrations/0006_quarantine_days.sql | 1 + pkg/models/remote.go | 11 ++ 13 files changed, 451 insertions(+), 10 deletions(-) create mode 100644 internal/api/v1/proxyerror_test.go create mode 100644 internal/proxy/quarantine_test.go create mode 100644 migrations/0006_quarantine_days.sql diff --git a/docs/common/caching.md b/docs/common/caching.md index 3b32604..5a88f3d 100644 --- a/docs/common/caching.md +++ b/docs/common/caching.md @@ -15,7 +15,7 @@ Every proxied request on a remote is classified as **denied**, **immutable** or | `mutable_ttl` | Seconds a mutable path stays fresh. `0` also means never expires. Omitted on create, it defaults to `3600`; omitted on update, the stored value is kept. | | `check_mutable` | When a mutable path's `mutable_ttl` expires, sends a conditional `HEAD` (`If-None-Match`) with the stored upstream ETag. A `304` re-arms the TTL and serves the stored copy; anything else refetches in full. The ETag is kept for `max(10 × mutable_ttl, 7 days)`, re-armed on each `304`, so idle paths age out. It is only stored when `check_mutable` is set and `mutable_ttl` is non-zero, and never for immutable paths. | | `stale_on_error` | On an upstream network error, serve the stored copy instead of failing. | -| `quarantine_enabled` / `quarantine_days` | Stored on the remote but not enforced by the proxy yet. | +| `quarantine_enabled` / `quarantine_days` | Refuses immutable artifacts published less than `quarantine_days` (default `3`) ago. See [Quarantine](#quarantine). | | `mirrorlist` / `mirror_strategy` | rpm/deb/alpine only. Upstream pool is `[base_url] + mirrorlist`; `round_robin` (default) rotates the starting mirror, `least_conn` starts at the one with the fewest in-flight requests. Network errors and 5xx fail over to the next mirror; other statuses return as-is. | ## Precedence @@ -52,6 +52,24 @@ So `blocklist` beats the allowlist, and `immutable_patterns` beats Concurrent misses on one path share a fetch lock. Waiters poll the store for up to 5s instead of all going upstream. +## Quarantine + +With `quarantine_enabled`, an uncached **immutable** path whose upstream copy +was published less than `quarantine_days` ago answers `403` and is not cached: + +```json +{"error": "quarantined: published 2026-10-24T09:00:00Z, available from 2026-10-27T09:00:00Z", + "published": "2026-10-24T09:00:00Z", "available": "2026-10-27T09:00:00Z"} +``` + +- `quarantine_days` must be at least `1` when quarantine is enabled. +- The publish time is the upstream `Last-Modified` header (capped at now), else the first time artifactapi was asked for the path. +- If that first-seen lookup fails, the path answers `503` with `Retry-After` instead. +- Mutable paths (indexes, metadata) are never quarantined. +- Paths already cached keep serving, including ones cached before quarantine was enabled. +- A virtual redirects to the member that owns the path, so its `403` reaches the client. A goproxy virtual probing for an unlisted version skips a quarantining member and only redirects to it when no other member serves the version. +- Indexes still list fresh versions, so a client resolving "latest" may pick one and get the `403`. + ## Eviction ```sh diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index 9c6ad33..a80f737 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -2,6 +2,7 @@ package v1 import ( "bytes" + "encoding/json" "errors" "fmt" "io" @@ -11,6 +12,7 @@ import ( "net/url" "strconv" "strings" + "time" "github.com/go-chi/chi/v5" @@ -121,7 +123,7 @@ func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote if err != nil { var proxyErr *proxy.ProxyError if errors.As(err, &proxyErr) { - http.Error(w, proxyErr.Message, proxyErr.Status) + writeProxyError(w, proxyErr) return } slog.Error("proxy fetch failed", "remote", remote.Name, "path", path, "error", err) @@ -149,6 +151,24 @@ func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote io.Copy(w, body) } +// writeProxyError answers a quarantine refusal as JSON and anything else as text. +func writeProxyError(w http.ResponseWriter, pe *proxy.ProxyError) { + if pe.RetryAfter > 0 { + w.Header().Set("Retry-After", strconv.Itoa(pe.RetryAfter)) + } + if pe.Published.IsZero() { + http.Error(w, pe.Message, pe.Status) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(pe.Status) + _ = json.NewEncoder(w).Encode(map[string]string{ + "error": pe.Message, + "published": pe.Published.Format(time.RFC3339), + "available": pe.Available.Format(time.RFC3339), + }) +} + func rewritesMutable(prov provider.Provider, path string) bool { _, sr := prov.(provider.ServeRewriter) _, rr := prov.(provider.ResponseRewriter) @@ -176,7 +196,7 @@ func (h *ProxyHandler) handleProxyHead(w http.ResponseWriter, r *http.Request) { if err != nil { var proxyErr *proxy.ProxyError if errors.As(err, &proxyErr) { - http.Error(w, proxyErr.Message, proxyErr.Status) + writeProxyError(w, proxyErr) return } slog.Error("proxy head failed", "remote", remote.Name, "path", path, "error", err) diff --git a/internal/api/v1/proxyerror_test.go b/internal/api/v1/proxyerror_test.go new file mode 100644 index 0000000..009b9c5 --- /dev/null +++ b/internal/api/v1/proxyerror_test.go @@ -0,0 +1,16 @@ +package v1 + +import ( + "net/http/httptest" + "testing" + + "git.unkin.net/unkin/artifactapi/internal/proxy" +) + +func TestWriteProxyErrorRetryAfter(t *testing.T) { + w := httptest.NewRecorder() + writeProxyError(w, &proxy.ProxyError{Status: 503, Message: "quarantine check unavailable", RetryAfter: 30}) + if w.Code != 503 || w.Header().Get("Retry-After") != "30" { + t.Errorf("code %d, Retry-After %q", w.Code, w.Header().Get("Retry-After")) + } +} diff --git a/internal/api/v2/remotes.go b/internal/api/v2/remotes.go index b5c3a82..5eb530e 100644 --- a/internal/api/v2/remotes.go +++ b/internal/api/v2/remotes.go @@ -72,7 +72,7 @@ func (h *RemotesHandler) get(w http.ResponseWriter, r *http.Request) { const defaultMutableTTL = 3600 func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) { - remote := models.Remote{MutableTTL: defaultMutableTTL} + remote := models.Remote{MutableTTL: defaultMutableTTL, QuarantineDays: models.DefaultQuarantineDays} if err := json.NewDecoder(r.Body).Decode(&remote); err != nil { http.Error(w, "invalid json", http.StatusBadRequest) return @@ -104,6 +104,10 @@ func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } + if err := remote.ValidateQuarantine(); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } if err := h.db.CreateRemote(r.Context(), &remote); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -126,6 +130,7 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) { var remote models.Remote if oldKnown { remote.MutableTTL = existing.MutableTTL + remote.QuarantineDays = existing.QuarantineDays } if err := json.NewDecoder(r.Body).Decode(&remote); err != nil { http.Error(w, "invalid json", http.StatusBadRequest) @@ -144,6 +149,10 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } + if err := remote.ValidateQuarantine(); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } if err := h.db.UpdateRemote(r.Context(), &remote); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/internal/api/v2/remotes_ttl_test.go b/internal/api/v2/remotes_ttl_test.go index 7beb859..8f55e97 100644 --- a/internal/api/v2/remotes_ttl_test.go +++ b/internal/api/v2/remotes_ttl_test.go @@ -55,3 +55,67 @@ func TestMutableTTLOmittedVsZero(t *testing.T) { t.Errorf("explicit 0 stored as %d", got) } } + +func TestQuarantineDaysDefault(t *testing.T) { + if testDSN == "" { + t.Skip("Docker unavailable") + } + db, err := database.New(testDSN) + if err != nil { + t.Fatal(err) + } + defer db.Close() + h := NewRemotesHandler(db, nil, nil).Routes() + days := func() int { + t.Helper() + r, err := db.GetRemote(context.Background(), "q-days") + if err != nil { + t.Fatal(err) + } + return r.QuarantineDays + } + const body = `"package_type":"npm","base_url":"https://example.com","quarantine_enabled":true` + for i, s := range []struct { + method, path, json string + want int + }{ + {"POST", "/", `{"name":"q-days",` + body + `}`, 3}, + {"PUT", "/q-days", `{` + body + `,"quarantine_days":14}`, 14}, + {"PUT", "/q-days", `{` + body + `}`, 14}, + } { + if c := do(t, h, s.method, s.path, s.json); c/100 != 2 { + t.Fatalf("step %d = %d", i, c) + } + if got := days(); got != s.want { + t.Errorf("step %d: quarantine_days = %d, want %d", i, got, s.want) + } + } +} + +func TestQuarantineDaysValidation(t *testing.T) { + if testDSN == "" { + t.Skip("Docker unavailable") + } + db, err := database.New(testDSN) + if err != nil { + t.Fatal(err) + } + defer db.Close() + h := NewRemotesHandler(db, nil, nil).Routes() + const body = `"package_type":"npm","base_url":"https://example.com"` + for i, s := range []struct { + method, path, json string + want int + }{ + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_enabled":true,"quarantine_days":0}`, 400}, + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_enabled":true,"quarantine_days":-1}`, 400}, + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_days":0}`, 201}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true}`, 400}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true,"quarantine_days":1}`, 200}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true,"quarantine_days":0}`, 400}, + } { + if c := do(t, h, s.method, s.path, s.json); c != s.want { + t.Errorf("step %d %s %s = %d, want %d", i, s.method, s.path, c, s.want) + } + } +} diff --git a/internal/cache/redis.go b/internal/cache/redis.go index c120745..844d939 100644 --- a/internal/cache/redis.go +++ b/internal/cache/redis.go @@ -84,6 +84,20 @@ func (r *Redis) GetETag(ctx context.Context, remote, path string) (string, error return val, err } +// FirstSeen records now as the first sighting of path unless one is already +// stored, and returns the stored sighting. +func (r *Redis) FirstSeen(ctx context.Context, remote, path string, ttl time.Duration) (time.Time, error) { + key := fmt.Sprintf("firstseen:%s:%s", remote, path) + if err := r.client.SetNX(ctx, key, time.Now().Unix(), ttl).Err(); err != nil { + return time.Time{}, err + } + sec, err := r.client.Get(ctx, key).Int64() + if err != nil { + return time.Time{}, err + } + return time.Unix(sec, 0), nil +} + func (r *Redis) GetToken(ctx context.Context, key string) (string, error) { val, err := r.client.Get(ctx, "token:"+key).Result() if err == redis.Nil { diff --git a/internal/proxy/engine.go b/internal/proxy/engine.go index 779bec9..aef9118 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -325,12 +325,12 @@ func (e *Engine) Head(ctx context.Context, remote models.Remote, path string, pr return &HeadResult{ContentType: info.ContentType, Size: info.Size, Source: "cache"}, nil } - return e.headUpstream(ctx, remote, path, prov) + return e.headUpstream(ctx, remote, path, prov, class) } // headUpstream issues an upstream HEAD, load-balancing across the remote's base // URLs and failing over to the next mirror on a network error or 5xx. -func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path string, prov provider.Provider) (*HeadResult, error) { +func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path string, prov provider.Provider, class Classification) (*HeadResult, error) { order := e.baseURLAttemptOrder(remote) if len(order) == 0 { return nil, &ProxyError{Status: http.StatusBadGateway, Message: "no upstream base_url configured"} @@ -338,7 +338,7 @@ func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path st var lastErr error for i, url := range order { ctr := e.beginAttempt(remote, url) - result, err := e.headUpstreamOnce(ctx, withBaseURL(remote, url), path, prov) + result, err := e.headUpstreamOnce(ctx, withBaseURL(remote, url), path, prov, class) endAttempt(ctr) if err == nil { return result, nil @@ -353,7 +353,7 @@ func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path st return nil, lastErr } -func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, path string, prov provider.Provider) (*HeadResult, error) { +func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, path string, prov provider.Provider, class Classification) (*HeadResult, error) { url := prov.UpstreamURL(remote, path) authHeaders, err := prov.AuthHeaders(ctx, remote) @@ -400,6 +400,11 @@ func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, pat if resp.StatusCode != http.StatusOK { return nil, &ProxyError{Status: resp.StatusCode, Message: fmt.Sprintf("upstream returned %d", resp.StatusCode)} } + if class == ClassImmutable { + if err := e.checkQuarantine(ctx, remote, path, resp.Header); err != nil { + return nil, err + } + } contentType := prov.ContentType(path) if ct := resp.Header.Get("Content-Type"); ct != "" { @@ -527,6 +532,11 @@ func (e *Engine) fetchFromUpstreamOnce(ctx context.Context, remote models.Remote }, nil } + if err := e.checkQuarantine(ctx, remote, path, resp.Header); err != nil { + _ = resp.Body.Close() + return nil, err + } + // Immutable blobs are streamed through the content-addressable store // (tempfile -> sha256 -> S3) so arbitrarily large artifacts never sit // fully in memory. Immutable content is never rewritten in the proxy path. @@ -663,6 +673,43 @@ func (e *Engine) checkUpstreamOnce(ctx context.Context, remote models.Remote, pa return resp.StatusCode == http.StatusNotModified, nil } +// checkQuarantine refuses an uncached immutable artifact published less than +// quarantine_days ago. The publish time is the upstream Last-Modified, capped +// at now, else the first time this path was seen. +func (e *Engine) checkQuarantine(ctx context.Context, remote models.Remote, path string, upstream http.Header) error { + if !remote.QuarantineEnabled { + return nil + } + if a, err := e.db.GetArtifact(ctx, remote.Name, path); err == nil && a != nil { + return nil + } + days := remote.QuarantineDays + if days < 1 { + days = models.DefaultQuarantineDays + } + window := time.Duration(days) * 24 * time.Hour + now := time.Now() + published, err := http.ParseTime(upstream.Get("Last-Modified")) + if err != nil { + if published, err = e.cache.FirstSeen(ctx, remote.Name, path, window+24*time.Hour); err != nil { + slog.Warn("first-seen lookup failed", "remote", remote.Name, "path", path, "error", err) + return &ProxyError{Status: http.StatusServiceUnavailable, Message: "quarantine check unavailable", RetryAfter: 30} + } + } + if published.After(now) { + published = now + } + if until := published.Add(window); now.Before(until) { + return &ProxyError{ + Status: http.StatusForbidden, + Message: fmt.Sprintf("quarantined: published %s, available from %s", published.UTC().Format(time.RFC3339), until.UTC().Format(time.RFC3339)), + Published: published.UTC(), + Available: until.UTC(), + } + } + return nil +} + // etagTTL keeps a mutable path's ETag well past its freshness key so an expired // path revalidates with a conditional HEAD, while idle paths still age out. func etagTTL(ttl time.Duration) time.Duration { @@ -821,10 +868,21 @@ func fetchBearerToken(ctx context.Context, wwwAuth string, remote models.Remote) type ProxyError struct { Status int Message string + // Published and Available are set only on a quarantine refusal. + Published time.Time + Available time.Time + // RetryAfter, in seconds, is sent as Retry-After when set. + RetryAfter int } func (e *ProxyError) Error() string { return e.Message } +// Quarantined reports whether err is a quarantine refusal. +func Quarantined(err error) bool { + var pe *ProxyError + return errors.As(err, &pe) && !pe.Published.IsZero() +} + type UpstreamError struct { Err error } diff --git a/internal/proxy/engine_test.go b/internal/proxy/engine_test.go index 447de34..25371a0 100644 --- a/internal/proxy/engine_test.go +++ b/internal/proxy/engine_test.go @@ -227,7 +227,7 @@ func TestHeadUpstreamUsesRemoteTimeout(t *testing.T) { r := models.Remote{Name: "slow-head", PackageType: models.PackageGeneric, BaseURL: slow.URL, UpstreamResponseHeaderTimeout: 1} start := time.Now() - _, err := (&Engine{}).headUpstreamOnce(context.Background(), r, "blob.bin", prov(t, models.PackageGeneric)) + _, err := (&Engine{}).headUpstreamOnce(context.Background(), r, "blob.bin", prov(t, models.PackageGeneric), ClassImmutable) if err == nil || time.Since(start) > 2*time.Second { t.Fatalf("HEAD ignored the remote's response header timeout: err=%v after %s", err, time.Since(start)) } diff --git a/internal/proxy/quarantine_test.go b/internal/proxy/quarantine_test.go new file mode 100644 index 0000000..eb4c1c9 --- /dev/null +++ b/internal/proxy/quarantine_test.go @@ -0,0 +1,186 @@ +package proxy + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "git.unkin.net/unkin/artifactapi/internal/cache" + "git.unkin.net/unkin/artifactapi/pkg/models" + goredis "github.com/redis/go-redis/v9" +) + +func must[T any](v T, err error) T { + if err != nil { + panic(err) + } + return v +} + +func quarantineUpstream(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/fresh.tgz": + w.Header().Set("Last-Modified", time.Now().Add(-time.Hour).UTC().Format(http.TimeFormat)) + case "/old.tgz": + w.Header().Set("Last-Modified", time.Now().Add(-30*24*time.Hour).UTC().Format(http.TimeFormat)) + case "/future.tgz": + w.Header().Set("Last-Modified", time.Now().Add(30*24*time.Hour).UTC().Format(http.TimeFormat)) + case "/undated.tgz", "/pkg": + default: + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte("body " + r.URL.Path)) + })) + t.Cleanup(srv.Close) + return srv +} + +func quarantineRemote(t *testing.T, name string, enabled bool) models.Remote { + r := models.Remote{Name: name, PackageType: models.PackageNPM, RepoType: models.RepoTypeRemote, + BaseURL: quarantineUpstream(t).URL, QuarantineEnabled: enabled, QuarantineDays: 3} + return seed(t, r) +} + +func wantQuarantined(t *testing.T, err error) *ProxyError { + t.Helper() + var pe *ProxyError + if !errors.As(err, &pe) || pe.Status != http.StatusForbidden || !Quarantined(err) { + t.Fatalf("want quarantine 403, got %v", err) + } + return pe +} + +func TestQuarantineBlocksFreshArtifact(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-fresh", true) + pe := wantQuarantined(t, func() error { _, err := testEngine.Fetch(ctx, r, "fresh.tgz", prov(t, models.PackageNPM)); return err }()) + if d := time.Until(pe.Available); d < 70*time.Hour || d > 72*time.Hour { + t.Errorf("available in %s, want ~71h", d) + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "fresh.tgz"); a != nil { + t.Error("quarantined artifact was cached") + } + _, err := testEngine.Head(ctx, r, "fresh.tgz", prov(t, models.PackageNPM)) + _ = wantQuarantined(t, err) +} + +func TestQuarantineServesOldArtifact(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-old", true) + res, err := testEngine.Fetch(context.Background(), r, "old.tgz", prov(t, models.PackageNPM)) + if err != nil { + t.Fatal(err) + } + if got := readAll(t, res); got != "body /old.tgz" { + t.Errorf("body = %q", got) + } +} + +func TestQuarantineFallsBackToFirstSeen(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-undated", true) + p := prov(t, models.PackageNPM) + _, err := testEngine.Fetch(ctx, r, "undated.tgz", p) + first := wantQuarantined(t, err).Published + if time.Since(first) > time.Minute { + t.Fatalf("first seen %s, want now", first) + } + _, err = testEngine.Fetch(ctx, r, "undated.tgz", p) + if again := wantQuarantined(t, err).Published; !again.Equal(first) { + t.Errorf("first seen moved: %s -> %s", first, again) + } + + r.QuarantineDays = 1 + c := goredis.NewClient(must(goredis.ParseURL(testRedis))) + defer func() { _ = c.Close() }() + if err := c.Set(ctx, "firstseen:q-undated:undated.tgz", time.Now().Add(-48*time.Hour).Unix(), time.Hour).Err(); err != nil { + t.Fatal(err) + } + if _, err := testEngine.Fetch(ctx, r, "undated.tgz", p); err != nil { + t.Errorf("seen 48h ago with 1 day quarantine: %v", err) + } +} + +func TestQuarantineSkipsMutable(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-mutable", true) + if _, err := testEngine.Fetch(context.Background(), r, "pkg", prov(t, models.PackageNPM)); err != nil { + t.Errorf("mutable index blocked: %v", err) + } +} + +func TestQuarantineDisabled(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-off", false) + if _, err := testEngine.Fetch(context.Background(), r, "fresh.tgz", prov(t, models.PackageNPM)); err != nil { + t.Errorf("disabled quarantine blocked: %v", err) + } +} + +func TestQuarantineKeepsServingCached(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-cached", false) + p := prov(t, models.PackageNPM) + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err != nil { + t.Fatal(err) + } + r.QuarantineEnabled = true + expire(t, r.Name, "fresh.tgz") + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err != nil { + t.Errorf("artifact cached before quarantine refused on refetch: %v", err) + } +} + +func TestQuarantineCapsFutureLastModified(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-future", true) + _, err := testEngine.Fetch(context.Background(), r, "future.tgz", prov(t, models.PackageNPM)) + pe := wantQuarantined(t, err) + if d := time.Until(pe.Available); d < 71*time.Hour || d > 72*time.Hour { + t.Errorf("available in %s, want ~72h", d) + } +} + +func TestQuarantineRedisErrorUnavailable(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-redis-down", true) + r.StaleOnError = true + down := must(cache.NewRedis(testRedis)) + _ = down.Close() + eng := NewEngine(testDB, down, testEngine.store) + _, err := eng.Fetch(ctx, r, "undated.tgz", prov(t, models.PackageNPM)) + var pe *ProxyError + if !errors.As(err, &pe) || pe.Status != http.StatusServiceUnavailable || pe.RetryAfter == 0 || Quarantined(err) { + t.Fatalf("want 503 with Retry-After, got %#v", err) + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "undated.tgz"); a != nil { + t.Error("artifact cached while quarantine check unavailable") + } +} + +func TestQuarantineStaleOnErrorServesNothingUncached(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-stale", true) + r.StaleOnError = true + p := prov(t, models.PackageNPM) + _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p) + _ = wantQuarantined(t, err) + r.BaseURL = "http://127.0.0.1:1" + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err == nil { + t.Error("stale_on_error served an uncached quarantined path") + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "fresh.tgz"); a != nil { + t.Error("quarantined artifact was cached") + } +} diff --git a/internal/virtual/goproxy_merger.go b/internal/virtual/goproxy_merger.go index f502631..634cfcc 100644 --- a/internal/virtual/goproxy_merger.go +++ b/internal/virtual/goproxy_merger.go @@ -194,10 +194,18 @@ func (e *Engine) goList(ctx context.Context, virt models.Virtual, escMod string) return nil, ErrNotFound } -// goProbe returns the first member, in member order, that serves path. +// goProbe returns the first member, in member order, that serves path, else +// the first member quarantining it so the client sees that refusal. func (e *Engine) goProbe(ctx context.Context, virt models.Virtual, path string) (string, error) { + held := "" for _, name := range virt.Members { m, err := e.goMember(ctx, name, path) + if proxy.Quarantined(err) { + if held == "" { + held = name + } + continue + } if err != nil { return "", fmt.Errorf("virtual %q: member %q: %v", virt.Name, name, err) } @@ -205,6 +213,9 @@ func (e *Engine) goProbe(ctx context.Context, virt models.Virtual, path string) return name, nil } } + if held != "" { + return held, nil + } return "", ErrNotFound } diff --git a/internal/virtual/goproxy_merger_test.go b/internal/virtual/goproxy_merger_test.go index 0d99d19..088f9d7 100644 --- a/internal/virtual/goproxy_merger_test.go +++ b/internal/virtual/goproxy_merger_test.go @@ -3,11 +3,15 @@ package virtual import ( "context" "errors" + "fmt" + "net/http" + "strings" "sync" "testing" "time" "git.unkin.net/unkin/artifactapi/internal/database" + "git.unkin.net/unkin/artifactapi/internal/proxy" "git.unkin.net/unkin/artifactapi/internal/testsupport" "git.unkin.net/unkin/artifactapi/pkg/models" ) @@ -181,3 +185,32 @@ func TestGoProxyLocalMemberNotFound(t *testing.T) { t.Fatalf("module only absent from a local member must be not-found, got %v", err) } } + +func TestGoProbeFallsThroughQuarantine(t *testing.T) { + quarantined := &proxy.ProxyError{Status: http.StatusForbidden, Message: "quarantined", Published: time.Now()} + served := map[string]bool{} + e := &Engine{ + mergeTTL: time.Minute, + getRemote: func(_ context.Context, name string) (*models.Remote, error) { + return &models.Remote{Name: name, RepoType: models.RepoTypeRemote}, nil + }, + goMember: func(_ context.Context, name, path string) (*GoMember, error) { + if name == "internal" && strings.HasSuffix(path, ".info") { + return nil, fmt.Errorf("fetch: %w", quarantined) + } + if served[name] { + return &GoMember{RemoteName: name, Body: []byte("{}")}, nil + } + return &GoMember{RemoteName: name}, nil + }, + } + zip := "example.com/mod/@v/v0.0.0-20260101-abcdef.zip" + served["goproxy"] = true + if _, loc, err := e.GoProxy(context.Background(), goVirt(), zip, ""); err != nil || loc != "/api/v1/remote/goproxy/"+zip { + t.Errorf("next member serving: %q %v", loc, err) + } + served["goproxy"] = false + if _, loc, err := e.GoProxy(context.Background(), goVirt(), zip, ""); err != nil || loc != "/api/v1/remote/internal/"+zip { + t.Errorf("only quarantined member: %q %v, want redirect to its refusal", loc, err) + } +} diff --git a/migrations/0006_quarantine_days.sql b/migrations/0006_quarantine_days.sql new file mode 100644 index 0000000..5a72bf5 --- /dev/null +++ b/migrations/0006_quarantine_days.sql @@ -0,0 +1 @@ +UPDATE remotes SET quarantine_days = 3 WHERE quarantine_days IS NULL OR quarantine_days <= 0; diff --git a/pkg/models/remote.go b/pkg/models/remote.go index fd27aa6..2a6ef95 100644 --- a/pkg/models/remote.go +++ b/pkg/models/remote.go @@ -149,6 +149,17 @@ func (r *Remote) ValidateMirrorStrategy() error { return nil } +// DefaultQuarantineDays matches the remotes.quarantine_days column default. +const DefaultQuarantineDays = 3 + +// ValidateQuarantine rejects an enabled quarantine with no window. +func (r *Remote) ValidateQuarantine() error { + if r.QuarantineEnabled && r.QuarantineDays < 1 { + return fmt.Errorf("quarantine_days must be at least 1 when quarantine_enabled is set") + } + return nil +} + // ValidatePatterns ensures every configured regex compiles. Storing an // invalid pattern would otherwise be silently dropped at match time, which // for the blocklist is a fail-open: a mistyped deny rule becomes a no-op. From f0ee5931649d9ddf6231b68c327852a9cae36924 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 15:17:32 +1100 Subject: [PATCH 3/3] Gate local repo writes behind OIDC tokens (#169) Local repo writes (file uploads/deletes, object evictions, native publishes, docker push) are open to anyone who can reach the service; only remote/virtual config is token-gated. - add `ARTIFACTAPI_AUTH_UPLOADS` (default false) gating every local write path behind the OIDC verifier - accept the token as Bearer, Token, Basic password (any user) or bare header value on every gated path, including remote/virtual config writes - serve a `/v2/token` Bearer challenge flow so `docker login` with the token enables push; missing, expired or invalid credentials get an anonymous token so pulls stay open - document minting a Vault identity token and passing it per client Reviewed-on: https://git.unkin.net/unkin/artifactapi/pulls/169 Co-authored-by: unkin-agent Co-committed-by: unkin-agent --- README.md | 4 +- docs/ansible/local.md | 2 +- docs/common/auth.md | 53 +++++++++++++++++ docs/docker/local.md | 3 + docs/gems/local.md | 2 +- docs/npm/local.md | 3 +- internal/auth/oidc.go | 110 +++++++++++++++++++++++++++++------ internal/auth/oidc_test.go | 88 ++++++++++++++++++++++++++++ internal/config/env.go | 10 ++++ internal/config/env_test.go | 20 +++++++ internal/server/oidc_test.go | 96 ++++++++++++++++++++++++++++++ internal/server/server.go | 22 +++++-- 12 files changed, 386 insertions(+), 27 deletions(-) create mode 100644 docs/common/auth.md diff --git a/README.md b/README.md index 7ca3d60..5e55941 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ API: `http://localhost:8000` | Frontend: `http://localhost:5173` Providers classify paths automatically. Users only configure what to proxy and TTLs. Classification precedence, TTLs and eviction for every remote: [docs/common/caching.md](docs/common/caching.md). +Write authentication and per-client token usage: [docs/common/auth.md](docs/common/auth.md). Each type's paths, Terraform examples and local/virtual behaviour are in `docs//`: @@ -208,8 +209,9 @@ additive and idempotent for the same reason; a test enforces it. | `GITHUB_APP_INSTALLATION_ID` | | GitHub App installation id | | `GITHUB_APP_PRIVATE_KEY` | | GitHub App private key, inline PEM | | `GITHUB_APP_PRIVATE_KEY_PATH` | | GitHub App private key, file path (alternative to inline PEM) | -| `ARTIFACTAPI_OIDC_ISSUER` | | OIDC issuer URL (e.g. Vault `https://vault.service.consul:8200/v1/identity/oidc`). When set, `POST /api/v2/remotes`, `POST /api/v2/virtuals` and `PUT`/`DELETE` on `/api/v2/{remotes,virtuals}/{name}` require `Authorization: Bearer ` verified against the issuer's JWKS. Uploads, object/file deletes, probe, reads and `/v2` stay open. Unset = no auth | +| `ARTIFACTAPI_OIDC_ISSUER` | | OIDC issuer URL (e.g. Vault `https://vault.example.com/v1/identity/oidc`). When set, `POST /api/v2/remotes`, `POST /api/v2/virtuals` and `PUT`/`DELETE` on `/api/v2/{remotes,virtuals}/{name}` require a token verified against the issuer's JWKS. Unset = no auth. See [docs/common/auth.md](docs/common/auth.md) | | `ARTIFACTAPI_OIDC_AUDIENCE` | | Comma-separated allowed `aud` values (e.g. the Vault OIDC role's client_id). Required when `ARTIFACTAPI_OIDC_ISSUER` is set | +| `ARTIFACTAPI_AUTH_UPLOADS` | `false` | Also require a token for every local repo write: file uploads/deletes, object evictions, `/api/v1/local` publishes and `/v2` push. Requires `ARTIFACTAPI_OIDC_ISSUER` | ## Development diff --git a/docs/ansible/local.md b/docs/ansible/local.md index 33bd7ad..66a2d96 100644 --- a/docs/ansible/local.md +++ b/docs/ansible/local.md @@ -2,7 +2,7 @@ A local `ansible` repo is a Galaxy v3 server: `ansible-galaxy collection publish` and `ansible-galaxy collection install` work against `/api/v1/local/`. -`publish` needs an API key; any value works. +`publish` needs an API key; any value works unless [uploads are gated](../common/auth.md). ```bash ansible-galaxy collection build diff --git a/docs/common/auth.md b/docs/common/auth.md new file mode 100644 index 0000000..9b31aaf --- /dev/null +++ b/docs/common/auth.md @@ -0,0 +1,53 @@ +# Write authentication + +artifactapi verifies writes against an OIDC issuer's JWKS, such as a Vault +identity OIDC role. Reads, probes and anonymous pulls never need a token. + +| Variable | Gates | +|---|---| +| `ARTIFACTAPI_OIDC_ISSUER` + `ARTIFACTAPI_OIDC_AUDIENCE` | creating, updating and deleting remotes and virtuals | +| `ARTIFACTAPI_AUTH_UPLOADS=true` | also every local repo write: `PUT`/`DELETE /api/v2/remotes//files/*`, `DELETE /api/v2/{remotes,locals}//objects/*`, `PUT`/`POST`/`DELETE /api/v1/local//*` (npm publish, gem push/yank, ansible-galaxy publish) and Docker Registry V2 push/delete under `/v2` | + +Any token from the issuer whose `aud` is in `ARTIFACTAPI_OIDC_AUDIENCE` may +write to every repo. + +## Minting a token + +```sh +vault read -field=token identity/oidc/token/ > token +``` + +The role's `client_id` must be one of the configured audiences. + +## Passing the token + +The token is accepted as `Authorization: Bearer `, as `Token `, +as the password of HTTP Basic credentials with any username, or as a bare +header value. + +```sh +# curl +curl -fsSL -H "Authorization: Bearer $(cat token)" --upload-file f.tgz \ + https://artifactapi.example.com/api/v2/remotes//files/f.tgz +curl -fsSL -u "ci:$(cat token)" --upload-file f.tgz \ + https://artifactapi.example.com/api/v2/remotes//files/f.tgz + +# docker / podman / helm (OCI): any username, token as password +docker login artifactapi.example.com -u ci --password-stdin < token +helm registry login artifactapi.example.com -u ci --password-stdin < token + +# npm (.npmrc) +//artifactapi.example.com/api/v1/local//:_authToken= + +# gem +GEM_HOST_API_KEY="$(cat token)" gem push --host https://artifactapi.example.com/api/v1/local/ mygem-1.0.0.gem + +# ansible-galaxy +ansible-galaxy collection publish c.tar.gz \ + --server https://artifactapi.example.com/api/v1/local// --api-key "$(cat token)" +``` + +With uploads gated, `/v2/` answers anonymous requests with a Bearer challenge +pointing at `/v2/token`. That endpoint returns the verified Basic password as +the registry token, or an anonymous token that only reads, so `docker pull` +keeps working without a login. diff --git a/docs/docker/local.md b/docs/docker/local.md index 54d7a55..749c88d 100644 --- a/docs/docker/local.md +++ b/docs/docker/local.md @@ -10,6 +10,9 @@ docker push artifactapi.example.com/docker-internal/myapp:latest docker pull artifactapi.example.com/docker-internal/myapp:latest ``` +With [uploads gated](../common/auth.md), `docker login` with any username and +a token as the password before pushing; pulls stay anonymous. + The first path segment after `/v2/` is the artifactapi repo name; the remainder is the image name. Blobs and manifests are stored through the shared content-addressable store (deduplicated by digest, reaped by GC once diff --git a/docs/gems/local.md b/docs/gems/local.md index caf720c..c019418 100644 --- a/docs/gems/local.md +++ b/docs/gems/local.md @@ -2,7 +2,7 @@ A local `gems` repo is a RubyGems source: `gem push`, `gem yank`, `gem install` and `bundle install` work against `/api/v1/local/`. `gem push` needs an -API key; any value works. +API key; any value works unless [uploads are gated](../common/auth.md). ```bash GEM_HOST_API_KEY=unused gem push --host https://artifactapi.example.com/api/v1/local/ mygem-1.0.0.gem diff --git a/docs/npm/local.md b/docs/npm/local.md index 798a48b..22b42b5 100644 --- a/docs/npm/local.md +++ b/docs/npm/local.md @@ -2,7 +2,8 @@ A local `npm` repo is a real npm registry: `npm publish` (scoped and unscoped) and `npm install` work against `/api/v1/local//`. npm requires an -`_authToken` for the registry before it will publish; any value works. +`_authToken` for the registry before it will publish; any value works unless +[uploads are gated](../common/auth.md). ```ini # .npmrc diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index d544bd0..b3f0fd8 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -2,7 +2,9 @@ package auth import ( "context" + "encoding/json" "errors" + "fmt" "log/slog" "net/http" "slices" @@ -104,42 +106,116 @@ func (b *BearerOIDC) discoverOnce(done chan struct{}) (v *oidc.IDTokenVerifier, // Require gates every request through next on a valid token. A nil receiver // passes everything through. func (b *BearerOIDC) Require(next http.Handler) http.Handler { + return b.require(next, "Bearer") +} + +func (b *BearerOIDC) require(next http.Handler, challenge string) http.Handler { if b == nil { return next } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - scheme, raw, _ := strings.Cut(strings.TrimSpace(r.Header.Get("Authorization")), " ") - raw = strings.TrimSpace(raw) - if !strings.EqualFold(scheme, "Bearer") || raw == "" { - unauthorized(w, "missing bearer token") + raw := token(r) + if raw == "" { + unauthorized(w, challenge, "missing token") return } - v, err := b.getVerifier(r.Context()) - if err != nil { + sub, err := b.verify(r.Context(), raw) + if errors.Is(err, errIssuerUnavailable) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusServiceUnavailable) _, _ = w.Write([]byte(`{"error":"token issuer unavailable"}`)) return } - tok, err := v.Verify(r.Context(), raw) if err != nil { slog.Info("bearer token rejected", "error", err) - unauthorized(w, "invalid token") + unauthorized(w, challenge, "invalid token") return } - if len(b.audiences) > 0 && !slices.ContainsFunc(tok.Audience, func(a string) bool { return slices.Contains(b.audiences, a) }) { - slog.Info("bearer token rejected", "error", "audience not allowed", "aud", tok.Audience) - unauthorized(w, "invalid token") - return - } - slog.Info("bearer token accepted", "sub", tok.Subject, "method", r.Method, "path", r.URL.Path) + slog.Info("bearer token accepted", "sub", sub, "method", r.Method, "path", r.URL.Path) next.ServeHTTP(w, r) }) } +func (b *BearerOIDC) verify(ctx context.Context, raw string) (string, error) { + v, err := b.getVerifier(ctx) + if err != nil { + return "", errIssuerUnavailable + } + tok, err := v.Verify(ctx, raw) + if err != nil { + return "", err + } + if len(b.audiences) > 0 && !slices.ContainsFunc(tok.Audience, func(a string) bool { return slices.Contains(b.audiences, a) }) { + return "", fmt.Errorf("audience %v not allowed", tok.Audience) + } + return tok.Subject, nil +} + +// token reads the JWT from `Bearer`/`Token` credentials, the password of +// `Basic` credentials (any username), or a bare header value as `gem push` +// sends its API key. +func token(r *http.Request) string { + if _, pass, ok := r.BasicAuth(); ok { + return pass + } + scheme, raw, found := strings.Cut(strings.TrimSpace(r.Header.Get("Authorization")), " ") + if !found { + return scheme + } + if strings.EqualFold(scheme, "Bearer") || strings.EqualFold(scheme, "Token") { + return strings.TrimSpace(raw) + } + return "" +} + // RequireWrites gates only mutating methods. func (b *BearerOIDC) RequireWrites(next http.Handler) http.Handler { - gated := b.Require(next) + return writesOnly(next, b.Require(next)) +} + +// RequireRegistryWrites gates Docker Registry V2 writes behind the token flow +// Docker clients need: an unauthenticated ping gets a Bearer challenge naming +// realm, which serves the Basic password back as the token once verified, or +// an anonymous token so pulls stay open. +func (b *BearerOIDC) RequireRegistryWrites(next http.Handler, realm func(*http.Request) string) http.Handler { + if b == nil { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + challenge := `Bearer realm="` + realm(r) + `",service="artifactapi"` + hasAuth := r.Header.Get("Authorization") != "" + switch { + case r.URL.Path == "/v2/token": + writeToken(w, b.registryToken(r)) + case r.URL.Path == "/v2/" && !hasAuth: + unauthorized(w, challenge, "authentication required") + default: + writesOnly(next, b.require(next, challenge)).ServeHTTP(w, r) + } + }) +} + +// registryToken echoes a valid credential back as the registry token. Missing +// or invalid credentials (e.g. an expired `docker login`) get the anonymous +// token so pulls keep working; writes are still verified per request. +func (b *BearerOIDC) registryToken(r *http.Request) string { + raw := token(r) + if raw == "" { + return "anonymous" + } + if _, err := b.verify(r.Context(), raw); err != nil { + slog.Info("registry credential rejected, issuing anonymous token", "error", err) + return "anonymous" + } + return raw +} + +func writeToken(w http.ResponseWriter, tok string) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"token": tok, "access_token": tok}) +} + +func writesOnly(next, gated http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet, http.MethodHead, http.MethodOptions: @@ -150,9 +226,9 @@ func (b *BearerOIDC) RequireWrites(next http.Handler) http.Handler { }) } -func unauthorized(w http.ResponseWriter, msg string) { +func unauthorized(w http.ResponseWriter, challenge, msg string) { w.Header().Set("Content-Type", "application/json") - w.Header().Set("WWW-Authenticate", "Bearer") + w.Header().Set("WWW-Authenticate", challenge) w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte(`{"error":"` + msg + `"}`)) } diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index c7e9c6d..3654194 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "strings" "sync" "sync/atomic" "testing" @@ -253,3 +254,90 @@ func TestRequireWritesLeavesReadsOpen(t *testing.T) { } } } + +func basic(user, pass string) string { + return "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":"+pass)) +} + +func TestTokenSchemes(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + h := NewBearerOIDC(iss.URL, []string{"artifactapi"}).RequireWrites(ok) + valid := iss.Token(nil, "artifactapi", "ci", time.Hour) + cases := []struct { + name string + authz string + want int + }{ + {"bearer", "Bearer " + valid, http.StatusNoContent}, + {"token scheme", "Token " + valid, http.StatusNoContent}, + {"basic password", basic("anyone", valid), http.StatusNoContent}, + {"basic empty user", basic("", valid), http.StatusNoContent}, + {"bare jwt", valid, http.StatusNoContent}, + {"basic wrong aud", basic("ci", iss.Token(nil, "nope", "ci", time.Hour)), http.StatusUnauthorized}, + {"basic token as user", basic(valid, "x"), http.StatusUnauthorized}, + {"bare garbage", "garbage", http.StatusUnauthorized}, + {"unknown scheme", "Digest " + valid, http.StatusUnauthorized}, + } + for _, c := range cases { + if got := serveAuth(h, http.MethodPut, c.authz); got != c.want { + t.Errorf("%s: status %d, want %d", c.name, got, c.want) + } + } +} + +func TestRequireRegistryWrites(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + h := NewBearerOIDC(iss.URL, []string{"artifactapi"}).RequireRegistryWrites(ok, func(*http.Request) string { return "https://reg.example/v2/token" }) + valid := iss.Token(nil, "artifactapi", "ci", time.Hour) + do := func(method, path, authz string) *httptest.ResponseRecorder { + r := httptest.NewRequest(method, path, nil) + if authz != "" { + r.Header.Set("Authorization", authz) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w + } + challenge := `Bearer realm="https://reg.example/v2/token",service="artifactapi"` + + w := do(http.MethodGet, "/v2/", "") + if w.Code != http.StatusUnauthorized || w.Header().Get("WWW-Authenticate") != challenge { + t.Errorf("anonymous ping: %d %q", w.Code, w.Header().Get("WWW-Authenticate")) + } + if w := do(http.MethodGet, "/v2/", "Bearer anonymous"); w.Code != http.StatusNoContent { + t.Errorf("ping with token: %d", w.Code) + } + if w := do(http.MethodGet, "/v2/token", ""); w.Code != http.StatusOK || !strings.Contains(w.Body.String(), `"token":"anonymous"`) { + t.Errorf("anonymous token: %d %s", w.Code, w.Body) + } + if w := do(http.MethodGet, "/v2/token", basic("ci", valid)); w.Code != http.StatusOK || !strings.Contains(w.Body.String(), `"token":"`+valid+`"`) { + t.Errorf("token exchange: %d %s", w.Code, w.Body) + } + expired := iss.Token(nil, "artifactapi", "ci", -time.Minute) + for _, authz := range []string{basic("ci", "bogus"), basic("ci", expired), "Bearer " + expired} { + if w := do(http.MethodGet, "/v2/token", authz); w.Code != http.StatusOK || !strings.Contains(w.Body.String(), `"token":"anonymous"`) { + t.Errorf("rejected credential %q: %d %s", authz, w.Code, w.Body) + } + } + for _, m := range []string{http.MethodGet, http.MethodHead} { + if w := do(m, "/v2/repo/img/manifests/t", ""); w.Code != http.StatusNoContent { + t.Errorf("anonymous %s: %d", m, w.Code) + } + } + for _, m := range []string{http.MethodPost, http.MethodPatch, http.MethodPut, http.MethodDelete} { + w := do(m, "/v2/repo/img/blobs/uploads/", "Bearer anonymous") + if w.Code != http.StatusUnauthorized || w.Header().Get("WWW-Authenticate") != challenge { + t.Errorf("anonymous %s: %d %q", m, w.Code, w.Header().Get("WWW-Authenticate")) + } + for _, authz := range []string{"Bearer " + valid, basic("ci", valid)} { + if w := do(m, "/v2/repo/img/blobs/uploads/", authz); w.Code != http.StatusNoContent { + t.Errorf("authenticated %s: %d", m, w.Code) + } + } + } + if got := serve(NewBearerOIDC("", nil).RequireRegistryWrites(ok, nil), http.MethodPut, ""); got != http.StatusNoContent { + t.Errorf("disabled: status %d", got) + } +} diff --git a/internal/config/env.go b/internal/config/env.go index 527d247..150cce5 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -61,6 +61,8 @@ type Config struct { // Bearer-JWT auth on configuration writes; empty issuer disables it. OIDCIssuer string OIDCAudiences []string + // AuthUploads extends that auth to local repo uploads, deletes and evictions. + AuthUploads bool } func (c *Config) DatabaseDSN() string { @@ -105,6 +107,13 @@ func Load() (*Config, error) { if oidcIssuer != "" && len(oidcAudiences) == 0 { return nil, fmt.Errorf("ARTIFACTAPI_OIDC_AUDIENCE is required when ARTIFACTAPI_OIDC_ISSUER is set") } + authUploads, err := strconv.ParseBool(getenv("ARTIFACTAPI_AUTH_UPLOADS", "false")) + if err != nil { + return nil, fmt.Errorf("invalid ARTIFACTAPI_AUTH_UPLOADS: %w", err) + } + if authUploads && oidcIssuer == "" { + return nil, fmt.Errorf("ARTIFACTAPI_OIDC_ISSUER is required when ARTIFACTAPI_AUTH_UPLOADS is set") + } cfg := &Config{ ListenAddr: getenv("LISTEN_ADDR", ":8000"), @@ -142,6 +151,7 @@ func Load() (*Config, error) { OIDCIssuer: oidcIssuer, OIDCAudiences: oidcAudiences, + AuthUploads: authUploads, } return cfg, nil diff --git a/internal/config/env_test.go b/internal/config/env_test.go index 3e09284..a6e39c4 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -81,3 +81,23 @@ func TestLoadOIDC(t *testing.T) { t.Error("expected error for issuer without audience") } } + +func TestLoadAuthUploads(t *testing.T) { + cfg, err := Load() + if err != nil || cfg.AuthUploads { + t.Fatalf("default: %v, AuthUploads=%v", err, cfg.AuthUploads) + } + t.Setenv("ARTIFACTAPI_AUTH_UPLOADS", "true") + if _, err := Load(); err == nil { + t.Error("expected error for AUTH_UPLOADS without issuer") + } + t.Setenv("ARTIFACTAPI_OIDC_ISSUER", "https://vault.example/v1/identity/oidc") + t.Setenv("ARTIFACTAPI_OIDC_AUDIENCE", "artifactapi") + if cfg, err := Load(); err != nil || !cfg.AuthUploads { + t.Fatalf("enabled: %v, AuthUploads=%v", err, cfg.AuthUploads) + } + t.Setenv("ARTIFACTAPI_AUTH_UPLOADS", "maybe") + if _, err := Load(); err == nil { + t.Error("expected error for invalid ARTIFACTAPI_AUTH_UPLOADS") + } +} diff --git a/internal/server/oidc_test.go b/internal/server/oidc_test.go index c9ed292..61aedb5 100644 --- a/internal/server/oidc_test.go +++ b/internal/server/oidc_test.go @@ -1,6 +1,7 @@ package server import ( + "encoding/base64" "io" "net/http" "net/http/httptest" @@ -78,3 +79,98 @@ func TestServerOIDCWriteAuth(t *testing.T) { expect(204, "DELETE", "/api/v2/virtuals/srv-oidc-v", "", good) expect(204, "DELETE", "/api/v2/remotes/srv-oidc", "", good) } + +func TestServerAuthUploads(t *testing.T) { + requireStack(t) + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + + cfg := *testSrv.cfg + cfg.OIDCIssuer = iss.URL + cfg.OIDCAudiences = []string{"artifactapi"} + cfg.AuthUploads = true + s := *testSrv + s.cfg = &cfg + ts := httptest.NewServer(s.routes()) + defer ts.Close() + + good := iss.Token(nil, "artifactapi", "ci", time.Hour) + do := func(method, path, body, authz string) int { + t.Helper() + rq, _ := http.NewRequest(method, ts.URL+path, strings.NewReader(body)) + rq.Header.Set("Content-Type", "application/json") + if authz != "" { + rq.Header.Set("Authorization", authz) + } + resp, err := http.DefaultClient.Do(rq) + if err != nil { + t.Fatalf("%s %s: %v", method, path, err) + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + return resp.StatusCode + } + bearer := "Bearer " + good + for _, typ := range []string{"generic", "docker", "npm", "gems"} { + body := `{"name":"srv-up-` + typ + `","package_type":"` + typ + `","repo_type":"local"}` + if got := do("POST", "/api/v2/remotes", body, bearer); got != 201 { + t.Fatalf("create %s: status %d", typ, got) + } + } + + writes := []struct{ method, path, body string }{ + {"PUT", "/api/v2/remotes/srv-up-generic/files/a.bin", "payload"}, + {"DELETE", "/api/v2/remotes/srv-up-generic/files/a.bin", ""}, + {"DELETE", "/api/v2/remotes/srv-up-generic/objects/a.bin", ""}, + {"DELETE", "/api/v2/locals/srv-up-generic/objects/a.bin", ""}, + {"PUT", "/api/v1/local/srv-up-npm/pkg", "{}"}, + {"POST", "/api/v1/local/srv-up-gems/api/v1/gems", "x"}, + {"DELETE", "/api/v1/local/srv-up-gems/api/v1/gems/yank", ""}, + {"POST", "/v2/srv-up-docker/img/blobs/uploads/", ""}, + {"PATCH", "/v2/srv-up-docker/img/blobs/uploads/x", ""}, + {"PUT", "/v2/srv-up-docker/img/manifests/t", "{}"}, + {"DELETE", "/v2/srv-up-docker/img/manifests/t", ""}, + } + for _, w := range writes { + if got := do(w.method, w.path, w.body, ""); got != 401 { + t.Errorf("anonymous %s %s: status %d, want 401", w.method, w.path, got) + } + } + + if got := do("PUT", "/api/v2/remotes/srv-up-generic/files/a.bin", "payload", bearer); got != 201 { + t.Errorf("bearer upload: status %d", got) + } + if got := do("PUT", "/api/v2/remotes/srv-up-generic/files/b.bin", "payload", basic(good)); got != 201 { + t.Errorf("basic upload: status %d", got) + } + if got := do("POST", "/v2/srv-up-docker/img/blobs/uploads/", "", basic(good)); got != 202 { + t.Errorf("basic docker upload: status %d", got) + } + expectOpen := func(method, path string) { + t.Helper() + if got := do(method, path, "", ""); got >= 400 { + t.Errorf("anonymous %s %s: status %d", method, path, got) + } + } + expectOpen("GET", "/api/v2/remotes/srv-up-generic/files/a.bin") + expectOpen("GET", "/api/v2/remotes/srv-up-generic/objects") + expectOpen("GET", "/api/v1/local/srv-up-generic/a.bin") + expectOpen("GET", "/v2/token") + if got := do("GET", "/v2/", "", ""); got != 401 { + t.Errorf("anonymous ping: status %d, want 401 challenge", got) + } + if got := do("POST", "/api/v2/probe", `{}`, ""); got == 401 { + t.Error("probe gated") + } + + if got := do("DELETE", "/api/v2/remotes/srv-up-generic/files/a.bin", "", bearer); got >= 400 { + t.Errorf("bearer delete: status %d", got) + } + for _, typ := range []string{"generic", "docker", "npm", "gems"} { + do("DELETE", "/api/v2/remotes/srv-up-"+typ, "", bearer) + } +} + +func basic(tok string) string { + return "Basic " + base64.StdEncoding.EncodeToString([]byte("ci:"+tok)) +} diff --git a/internal/server/server.go b/internal/server/server.go index 5cdacc0..b4021c9 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -169,8 +169,17 @@ func (s *Server) routes() chi.Router { s.virtEngine.NPMLocal = proxyHandler.LocalNPMPackument s.virtEngine.GemsLocal = proxyHandler.LocalGemsIndex s.virtEngine.AnsibleLocal = proxyHandler.LocalAnsible - r.Mount("/api/v1", proxyHandler.Routes()) - r.Mount("/v2", proxyHandler.DockerV2Routes()) + writeAuth := auth.NewBearerOIDC(s.cfg.OIDCIssuer, s.cfg.OIDCAudiences) + uploadAuth := passthrough + dockerV2 := http.Handler(proxyHandler.DockerV2Routes()) + if s.cfg.AuthUploads { + uploadAuth = writeAuth.RequireWrites + dockerV2 = writeAuth.RequireRegistryWrites(dockerV2, func(r *http.Request) string { + return v1.Scheme(r) + "://" + r.Host + "/v2/token" + }) + } + r.Mount("/api/v1", uploadAuth(proxyHandler.Routes())) + r.Mount("/v2", dockerV2) remotesHandler := v2.NewRemotesHandler(s.db, s.cache, map[models.PackageType]v2.Primer{ models.PackageGitHubRPM: s.syncer, @@ -182,7 +191,6 @@ func (s *Server) routes() chi.Router { statsHandler := v2.NewStatsHandler(s.db) eventsHandler := v2.NewEventsHandler() probeHandler := v2.NewProbeHandler(s.engine, s.db) - writeAuth := auth.NewBearerOIDC(s.cfg.OIDCIssuer, s.cfg.OIDCAudiences) r.Route("/api/v2", func(r chi.Router) { r.Mount("/remotes", writeAuth.RequireWrites(remotesHandler.Routes())) @@ -192,19 +200,19 @@ func (s *Server) routes() chi.Router { r.Mount("/events", eventsHandler.Routes()) r.Mount("/probe", probeHandler.Routes()) - r.Route("/remotes/{name}/objects", func(r chi.Router) { + r.With(uploadAuth).Route("/remotes/{name}/objects", func(r chi.Router) { objHandler := v2.NewObjectsHandler(s.db) r.Get("/", objHandler.Routes(s.engine).ServeHTTP) r.Delete("/*", objHandler.Routes(s.engine).ServeHTTP) }) - r.Route("/locals/{name}/objects", func(r chi.Router) { + r.With(uploadAuth).Route("/locals/{name}/objects", func(r chi.Router) { objHandler := v2.NewObjectsHandler(s.db) r.Get("/", objHandler.LocalRoutes().ServeHTTP) r.Delete("/*", objHandler.LocalRoutes().ServeHTTP) }) - r.Route("/remotes/{name}/files", func(r chi.Router) { + r.With(uploadAuth).Route("/remotes/{name}/files", func(r chi.Router) { r.Put("/*", s.localHandler.Routes().ServeHTTP) r.Get("/*", s.localHandler.Routes().ServeHTTP) r.Delete("/*", s.localHandler.Routes().ServeHTTP) @@ -214,6 +222,8 @@ func (s *Server) routes() chi.Router { return r } +func passthrough(h http.Handler) http.Handler { return h } + func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK)