From 638931d7eda1b498c1b3ca19d6a770ccbb2902ae Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 14 Oct 2026 22:22:59 +1100 Subject: [PATCH 1/4] Require OIDC bearer tokens on remote/virtual config writes when an issuer is set --- README.md | 2 + go.mod | 3 + go.sum | 6 ++ internal/auth/oidc.go | 105 ++++++++++++++++++++++++++++++++++ internal/auth/oidc_test.go | 81 ++++++++++++++++++++++++++ internal/config/env.go | 19 ++++++ internal/config/env_test.go | 17 ++++++ internal/server/middleware.go | 2 +- internal/server/oidc_test.go | 80 ++++++++++++++++++++++++++ internal/server/server.go | 6 +- internal/testsupport/oidc.go | 68 ++++++++++++++++++++++ 11 files changed, 386 insertions(+), 3 deletions(-) create mode 100644 internal/auth/oidc.go create mode 100644 internal/auth/oidc_test.go create mode 100644 internal/server/oidc_test.go create mode 100644 internal/testsupport/oidc.go diff --git a/README.md b/README.md index 7d2fcf3..660a658 100644 --- a/README.md +++ b/README.md @@ -371,6 +371,8 @@ 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_AUDIENCE` | | Comma-separated allowed `aud` values (e.g. the Vault OIDC role's client_id). Required when `ARTIFACTAPI_OIDC_ISSUER` is set | ## Development diff --git a/go.mod b/go.mod index 6583252..73858b7 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,9 @@ require ( github.com/cavaliergopher/rpm v1.3.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 + github.com/coreos/go-oidc/v3 v3.21.0 github.com/go-chi/chi/v5 v5.3.0 + github.com/go-jose/go-jose/v4 v4.1.4 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.10.0 github.com/klauspost/compress v1.19.2 @@ -102,6 +104,7 @@ require ( go.uber.org/atomic v1.11.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/net v0.56.0 // indirect + golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect gopkg.in/ini.v1 v1.67.2 // indirect diff --git a/go.sum b/go.sum index b77ce4b..c0b4cad 100644 --- a/go.sum +++ b/go.sum @@ -44,6 +44,8 @@ github.com/containerd/log v0.1.0 h1:TCJt7ioM2cr/tfR8GPbGf9/VRAX8D2B4PjzCpfX540I= github.com/containerd/log v0.1.0/go.mod h1:VRRf09a7mHDIRezVKTRCrOq78v577GXq3bSa3EhrzVo= github.com/containerd/platforms v0.2.1 h1:zvwtM3rz2YHPQsF2CHYM8+KtB5dvhISiXh5ZpSBQv6A= github.com/containerd/platforms v0.2.1/go.mod h1:XHCb+2/hzowdiut9rkudds9bE5yJ7npe7dG/wG+uFPw= +github.com/coreos/go-oidc/v3 v3.21.0 h1:wZo4Q9Pum8dYEj0eMUPrqR+kvuGkeUplbLpNCkBqoWM= +github.com/coreos/go-oidc/v3 v3.21.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= @@ -67,6 +69,8 @@ github.com/felixge/httpsnoop v1.1.0 h1:3YtUj32ZZkqZtt3sZZsClsymw/QDuVfpNhoA31zeO github.com/felixge/httpsnoop v1.1.0/go.mod h1:Zqxgdd+1Rkcz8euOqdr7lqgCRJztwr5hp9vDSi5UZCE= github.com/go-chi/chi/v5 v5.3.0 h1:halUjDxhshgXHMrao5bB8eNBXo/rnzwr8m5m36glehM= github.com/go-chi/chi/v5 v5.3.0/go.mod h1:R+tYY2hNuVUUjxoPtqUdgBqevM9s9njzkTLutVsOCto= +github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= +github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= @@ -225,6 +229,8 @@ golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= +golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= +golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go new file mode 100644 index 0000000..a663a8e --- /dev/null +++ b/internal/auth/oidc.go @@ -0,0 +1,105 @@ +package auth + +import ( + "context" + "log/slog" + "net/http" + "slices" + "strings" + "sync" + "time" + + "github.com/coreos/go-oidc/v3/oidc" +) + +// BearerOIDC verifies `Authorization: Bearer ` against an OIDC issuer's +// JWKS. Discovery is lazy and retried per request so startup never depends on +// the issuer being reachable. +type BearerOIDC struct { + issuer string + audiences []string + + mu sync.Mutex + verifier *oidc.IDTokenVerifier +} + +// NewBearerOIDC returns nil when issuer is empty, meaning auth is disabled. +func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { + if issuer == "" { + return nil + } + return &BearerOIDC{issuer: issuer, audiences: audiences} +} + +func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, error) { + b.mu.Lock() + defer b.mu.Unlock() + if b.verifier != nil { + return b.verifier, nil + } + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + // The provider keeps this context for JWKS refreshes; it must outlive the request. + p, err := oidc.NewProvider(oidc.ClientContext(context.WithoutCancel(ctx), http.DefaultClient), b.issuer) + if err != nil { + return nil, err + } + b.verifier = p.Verifier(&oidc.Config{SkipClientIDCheck: true}) + return b.verifier, nil +} + +// 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 { + if b == nil { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + raw, ok := strings.CutPrefix(r.Header.Get("Authorization"), "Bearer ") + if !ok || raw == "" { + unauthorized(w, "missing bearer token") + return + } + v, err := b.getVerifier(r.Context()) + if err != nil { + slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) + 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") + 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) + next.ServeHTTP(w, r) + }) +} + +// RequireWrites gates only mutating methods. +func (b *BearerOIDC) RequireWrites(next http.Handler) http.Handler { + gated := b.Require(next) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions: + next.ServeHTTP(w, r) + default: + gated.ServeHTTP(w, r) + } + }) +} + +func unauthorized(w http.ResponseWriter, msg string) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("WWW-Authenticate", "Bearer") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"` + msg + `"}`)) +} diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go new file mode 100644 index 0000000..8fff37b --- /dev/null +++ b/internal/auth/oidc_test.go @@ -0,0 +1,81 @@ +package auth + +import ( + "crypto/rand" + "crypto/rsa" + "net/http" + "net/http/httptest" + "testing" + "time" + + "git.unkin.net/unkin/artifactapi/internal/testsupport" +) + +func serve(h http.Handler, method, token string) int { + r := httptest.NewRequest(method, "/x", nil) + if token != "" { + r.Header.Set("Authorization", "Bearer "+token) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w.Code +} + +var ok = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }) + +func TestBearerOIDC(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + h := NewBearerOIDC(iss.URL, []string{"other", "artifactapi"}).Require(ok) + + otherKey, _ := rsa.GenerateKey(rand.Reader, 2048) + cases := []struct { + name string + token string + want int + }{ + {"valid", iss.Token(nil, "artifactapi", "ci", time.Hour), http.StatusNoContent}, + {"missing", "", http.StatusUnauthorized}, + {"garbage", "not.a.jwt", http.StatusUnauthorized}, + {"bad signature", iss.Token(otherKey, "artifactapi", "ci", time.Hour), http.StatusUnauthorized}, + {"wrong aud", iss.Token(nil, "nope", "ci", time.Hour), http.StatusUnauthorized}, + {"expired", iss.Token(nil, "artifactapi", "ci", -time.Hour), http.StatusUnauthorized}, + } + for _, c := range cases { + if got := serve(h, http.MethodPost, c.token); got != c.want { + t.Errorf("%s: status %d, want %d", c.name, got, c.want) + } + } +} + +func TestBearerOIDCIssuerDown(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + b := NewBearerOIDC(iss.URL, []string{"artifactapi"}) + tok := iss.Token(nil, "artifactapi", "ci", time.Hour) + iss.Close() + if got := serve(b.Require(ok), http.MethodPost, tok); got != http.StatusServiceUnavailable { + t.Errorf("issuer down: status %d, want 503", got) + } +} + +func TestBearerOIDCDisabled(t *testing.T) { + if got := serve(NewBearerOIDC("", nil).RequireWrites(ok), http.MethodDelete, ""); got != http.StatusNoContent { + t.Errorf("disabled: status %d, want 204", got) + } +} + +func TestRequireWritesLeavesReadsOpen(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + h := NewBearerOIDC(iss.URL, []string{"artifactapi"}).RequireWrites(ok) + for _, m := range []string{http.MethodGet, http.MethodHead, http.MethodOptions} { + if got := serve(h, m, ""); got != http.StatusNoContent { + t.Errorf("%s: status %d, want 204", m, got) + } + } + for _, m := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { + if got := serve(h, m, ""); got != http.StatusUnauthorized { + t.Errorf("%s: status %d, want 401", m, got) + } + } +} diff --git a/internal/config/env.go b/internal/config/env.go index 80ac39f..527d247 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "strconv" + "strings" ) type Config struct { @@ -56,6 +57,10 @@ type Config struct { GitHubAppInstallationID string GitHubAppPrivateKey string GitHubAppPrivateKeyPath string + + // Bearer-JWT auth on configuration writes; empty issuer disables it. + OIDCIssuer string + OIDCAudiences []string } func (c *Config) DatabaseDSN() string { @@ -90,6 +95,17 @@ func Load() (*Config, error) { return nil, fmt.Errorf("invalid GITHUB_SYNC_POLL_INTERVAL: %w", err) } + oidcIssuer := getenv("ARTIFACTAPI_OIDC_ISSUER", "") + var oidcAudiences []string + for _, a := range strings.Split(getenv("ARTIFACTAPI_OIDC_AUDIENCE", ""), ",") { + if a = strings.TrimSpace(a); a != "" { + oidcAudiences = append(oidcAudiences, a) + } + } + if oidcIssuer != "" && len(oidcAudiences) == 0 { + return nil, fmt.Errorf("ARTIFACTAPI_OIDC_AUDIENCE is required when ARTIFACTAPI_OIDC_ISSUER is set") + } + cfg := &Config{ ListenAddr: getenv("LISTEN_ADDR", ":8000"), @@ -123,6 +139,9 @@ func Load() (*Config, error) { GitHubAppInstallationID: getenv("GITHUB_APP_INSTALLATION_ID", ""), GitHubAppPrivateKey: getenv("GITHUB_APP_PRIVATE_KEY", ""), GitHubAppPrivateKeyPath: getenv("GITHUB_APP_PRIVATE_KEY_PATH", ""), + + OIDCIssuer: oidcIssuer, + OIDCAudiences: oidcAudiences, } return cfg, nil diff --git a/internal/config/env_test.go b/internal/config/env_test.go index 179d314..3e09284 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -64,3 +64,20 @@ func TestLoadInvalidPort(t *testing.T) { t.Error("expected error for invalid DBPORT") } } + +func TestLoadOIDC(t *testing.T) { + t.Setenv("ARTIFACTAPI_OIDC_ISSUER", "https://vault.example/v1/identity/oidc") + t.Setenv("ARTIFACTAPI_OIDC_AUDIENCE", " a, ,b ") + cfg, err := Load() + if err != nil { + t.Fatalf("load: %v", err) + } + if len(cfg.OIDCAudiences) != 2 || cfg.OIDCAudiences[0] != "a" || cfg.OIDCAudiences[1] != "b" { + t.Errorf("audiences = %q", cfg.OIDCAudiences) + } + + t.Setenv("ARTIFACTAPI_OIDC_AUDIENCE", "") + if _, err := Load(); err == nil { + t.Error("expected error for issuer without audience") + } +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index a8ec248..8cc6a0d 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -12,7 +12,7 @@ func cors(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Access-Control-Allow-Origin", "*") w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") - w.Header().Set("Access-Control-Allow-Headers", "Content-Type") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization") if r.Method == http.MethodOptions { w.WriteHeader(http.StatusNoContent) return diff --git a/internal/server/oidc_test.go b/internal/server/oidc_test.go new file mode 100644 index 0000000..c9ed292 --- /dev/null +++ b/internal/server/oidc_test.go @@ -0,0 +1,80 @@ +package server + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "git.unkin.net/unkin/artifactapi/internal/testsupport" +) + +func TestServerOIDCWriteAuth(t *testing.T) { + requireStack(t) + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + + cfg := *testSrv.cfg + cfg.OIDCIssuer = iss.URL + cfg.OIDCAudiences = []string{"artifactapi"} + 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, token string) int { + t.Helper() + rq, _ := http.NewRequest(method, ts.URL+path, strings.NewReader(body)) + rq.Header.Set("Content-Type", "application/json") + if token != "" { + rq.Header.Set("Authorization", "Bearer "+token) + } + 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 + } + expect := func(want int, method, path, body, token string) { + t.Helper() + if got := do(method, path, body, token); got != want { + t.Errorf("%s %s: status %d, want %d", method, path, got, want) + } + } + + remote := `{"name":"srv-oidc","package_type":"generic","repo_type":"local"}` + virtual := `{"name":"srv-oidc-v","package_type":"generic","members":["srv-oidc"]}` + + expect(401, "POST", "/api/v2/remotes", remote, "") + expect(401, "POST", "/api/v2/remotes", remote, iss.Token(nil, "other", "ci", time.Hour)) + expect(201, "POST", "/api/v2/remotes", remote, good) + expect(401, "PUT", "/api/v2/remotes/srv-oidc", `{"package_type":"generic"}`, "") + expect(401, "DELETE", "/api/v2/remotes/srv-oidc", "", "") + expect(401, "POST", "/api/v2/virtuals", virtual, "") + expect(201, "POST", "/api/v2/virtuals", virtual, good) + expect(401, "PUT", "/api/v2/virtuals/srv-oidc-v", virtual, "") + expect(401, "DELETE", "/api/v2/virtuals/srv-oidc-v", "", "") + + expect(200, "GET", "/api/v2/remotes", "", "") + expect(200, "GET", "/api/v2/remotes/srv-oidc", "", "") + expect(200, "GET", "/api/v2/virtuals/srv-oidc-v", "", "") + expect(201, "PUT", "/api/v2/remotes/srv-oidc/files/a.bin", "payload", "") + expect(200, "GET", "/api/v2/remotes/srv-oidc/files/a.bin", "", "") + if got := do("DELETE", "/api/v2/remotes/srv-oidc/files/a.bin", "", ""); got >= 400 { + t.Errorf("delete file: status %d", got) + } + if got := do("POST", "/api/v2/probe", `{}`, ""); got == 401 { + t.Error("probe gated") + } + if got := do("GET", "/v2/", "", ""); got == 401 { + t.Error("docker /v2 gated") + } + + expect(204, "DELETE", "/api/v2/virtuals/srv-oidc-v", "", good) + expect(204, "DELETE", "/api/v2/remotes/srv-oidc", "", good) +} diff --git a/internal/server/server.go b/internal/server/server.go index a2e85cb..88d12ac 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -15,6 +15,7 @@ import ( tfregistry "git.unkin.net/unkin/artifactapi/internal/api/terraform" v1 "git.unkin.net/unkin/artifactapi/internal/api/v1" v2 "git.unkin.net/unkin/artifactapi/internal/api/v2" + "git.unkin.net/unkin/artifactapi/internal/auth" "git.unkin.net/unkin/artifactapi/internal/cache" "git.unkin.net/unkin/artifactapi/internal/config" "git.unkin.net/unkin/artifactapi/internal/database" @@ -186,10 +187,11 @@ 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", remotesHandler.Routes()) - r.Mount("/virtuals", virtualsHandler.Routes()) + r.Mount("/remotes", writeAuth.RequireWrites(remotesHandler.Routes())) + r.Mount("/virtuals", writeAuth.RequireWrites(virtualsHandler.Routes())) r.Mount("/health", healthHandler.Routes()) r.Mount("/stats", statsHandler.Routes()) r.Mount("/events", eventsHandler.Routes()) diff --git a/internal/testsupport/oidc.go b/internal/testsupport/oidc.go new file mode 100644 index 0000000..0ea124a --- /dev/null +++ b/internal/testsupport/oidc.go @@ -0,0 +1,68 @@ +package testsupport + +import ( + "crypto/rand" + "crypto/rsa" + "encoding/json" + "net/http" + "net/http/httptest" + "time" + + "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4/jwt" +) + +// OIDCIssuer is a minimal OIDC issuer serving discovery and JWKS, signing +// RS256 tokens with a generated key. +type OIDCIssuer struct { + *httptest.Server + key *rsa.PrivateKey +} + +func NewOIDCIssuer() *OIDCIssuer { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + panic(err) + } + iss := &OIDCIssuer{key: key} + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": iss.URL, + "jwks_uri": iss.URL + "/keys", + "id_token_signing_alg_values_supported": []string{"RS256"}, + }) + }) + mux.HandleFunc("/keys", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{ + {Key: &key.PublicKey, KeyID: "k1", Algorithm: "RS256", Use: "sig"}, + }}) + }) + iss.Server = httptest.NewServer(mux) + return iss +} + +// Token signs a token for aud and sub. Pass a nil key to use the issuer's own; +// expiry is relative to now. +func (o *OIDCIssuer) Token(key *rsa.PrivateKey, aud, sub string, expiry time.Duration) string { + if key == nil { + key = o.key + } + sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.RS256, Key: key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "k1")) + if err != nil { + panic(err) + } + now := time.Now() + tok, err := jwt.Signed(sig).Claims(jwt.Claims{ + Issuer: o.URL, + Subject: sub, + Audience: jwt.Audience{aud}, + IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), + Expiry: jwt.NewNumericDate(now.Add(expiry)), + }).Serialize() + if err != nil { + panic(err) + } + return tok +} -- 2.47.3 From 51a77e3446742d5f1514156606dc1e771bf6f900 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 14 Oct 2026 22:27:20 +1100 Subject: [PATCH 2/4] Bound OIDC discovery with a client timeout and failure backoff --- internal/auth/oidc.go | 56 ++++++++++++----- internal/auth/oidc_test.go | 117 ++++++++++++++++++++++++++++++++++- internal/testsupport/oidc.go | 21 ++++--- 3 files changed, 167 insertions(+), 27 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index a663a8e..47cafea 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "log/slog" "net/http" "slices" @@ -13,35 +14,56 @@ import ( ) // BearerOIDC verifies `Authorization: Bearer ` against an OIDC issuer's -// JWKS. Discovery is lazy and retried per request so startup never depends on -// the issuer being reachable. +// JWKS. Discovery is lazy so startup never depends on the issuer being +// reachable; a failed discovery is not retried until the backoff elapses. type BearerOIDC struct { issuer string audiences []string + timeout time.Duration + backoff time.Duration - mu sync.Mutex - verifier *oidc.IDTokenVerifier + mu sync.Mutex + verifier *oidc.IDTokenVerifier + discovering bool + failedAt time.Time } +var errIssuerUnavailable = errors.New("oidc issuer unavailable") + // NewBearerOIDC returns nil when issuer is empty, meaning auth is disabled. func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { if issuer == "" { return nil } - return &BearerOIDC{issuer: issuer, audiences: audiences} + return &BearerOIDC{issuer: issuer, audiences: audiences, timeout: 10 * time.Second, backoff: 15 * time.Second} } -func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, error) { +// getVerifier runs at most one discovery at a time without holding the lock +// across network I/O; callers arriving mid-discovery or within the backoff fail fast. +func (b *BearerOIDC) getVerifier() (*oidc.IDTokenVerifier, error) { + b.mu.Lock() + if b.verifier != nil || b.discovering || time.Since(b.failedAt) < b.backoff { + v := b.verifier + b.mu.Unlock() + if v == nil { + return nil, errIssuerUnavailable + } + return v, nil + } + b.discovering = true + b.mu.Unlock() + + // The provider keeps this context and client for JWKS refreshes, so the + // deadline lives on the client rather than the context. + client := &http.Client{Timeout: b.timeout} + p, err := oidc.NewProvider(oidc.ClientContext(context.Background(), client), b.issuer) + b.mu.Lock() defer b.mu.Unlock() - if b.verifier != nil { - return b.verifier, nil - } - ctx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - // The provider keeps this context for JWKS refreshes; it must outlive the request. - p, err := oidc.NewProvider(oidc.ClientContext(context.WithoutCancel(ctx), http.DefaultClient), b.issuer) + b.discovering = false if err != nil { + b.failedAt = time.Now() + slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) return nil, err } b.verifier = p.Verifier(&oidc.Config{SkipClientIDCheck: true}) @@ -55,14 +77,14 @@ func (b *BearerOIDC) Require(next http.Handler) http.Handler { return next } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - raw, ok := strings.CutPrefix(r.Header.Get("Authorization"), "Bearer ") - if !ok || raw == "" { + 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") return } - v, err := b.getVerifier(r.Context()) + v, err := b.getVerifier() if err != nil { - slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusServiceUnavailable) _, _ = w.Write([]byte(`{"error":"token issuer unavailable"}`)) diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 8fff37b..38ea85a 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -3,24 +3,67 @@ package auth import ( "crypto/rand" "crypto/rsa" + "encoding/base64" "net/http" "net/http/httptest" + "strconv" + "sync/atomic" "testing" "time" + "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4/jwt" + "git.unkin.net/unkin/artifactapi/internal/testsupport" ) func serve(h http.Handler, method, token string) int { + if token == "" { + return serveAuth(h, method, "") + } + return serveAuth(h, method, "Bearer "+token) +} + +func serveAuth(h http.Handler, method, authz string) int { r := httptest.NewRequest(method, "/x", nil) - if token != "" { - r.Header.Set("Authorization", "Bearer "+token) + if authz != "" { + r.Header.Set("Authorization", authz) } w := httptest.NewRecorder() h.ServeHTTP(w, r) return w.Code } +func claims(iss string) jwt.Claims { + now := time.Now() + return jwt.Claims{ + Issuer: iss, + Subject: "ci", + Audience: jwt.Audience{"artifactapi"}, + IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + } +} + +func hs256Token(t *testing.T, c jwt.Claims) string { + sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.HS256, Key: []byte("0123456789abcdef0123456789abcdef")}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "k1")) + if err != nil { + t.Fatal(err) + } + tok, err := jwt.Signed(sig).Claims(c).Serialize() + if err != nil { + t.Fatal(err) + } + return tok +} + +func noneToken(iss string) string { + enc := base64.RawURLEncoding.EncodeToString + return enc([]byte(`{"alg":"none","typ":"JWT","kid":"k1"}`)) + "." + + enc([]byte(`{"iss":"`+iss+`","sub":"ci","aud":"artifactapi","exp":`+strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10)+`}`)) + "." +} + var ok = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }) func TestBearerOIDC(t *testing.T) { @@ -40,12 +83,82 @@ func TestBearerOIDC(t *testing.T) { {"bad signature", iss.Token(otherKey, "artifactapi", "ci", time.Hour), http.StatusUnauthorized}, {"wrong aud", iss.Token(nil, "nope", "ci", time.Hour), http.StatusUnauthorized}, {"expired", iss.Token(nil, "artifactapi", "ci", -time.Hour), http.StatusUnauthorized}, + {"wrong iss", iss.Sign(nil, claims("https://evil.example")), http.StatusUnauthorized}, + {"hs256", hs256Token(t, claims(iss.URL)), http.StatusUnauthorized}, + {"alg none", noneToken(iss.URL), http.StatusUnauthorized}, } for _, c := range cases { if got := serve(h, http.MethodPost, c.token); got != c.want { t.Errorf("%s: status %d, want %d", c.name, got, c.want) } } + + valid := iss.Token(nil, "artifactapi", "ci", time.Hour) + headers := []struct { + name string + authz string + want int + }{ + {"lowercase scheme", "bearer " + valid, http.StatusNoContent}, + {"extra whitespace", " Bearer " + valid + " ", http.StatusNoContent}, + {"empty value", "Bearer ", http.StatusUnauthorized}, + {"scheme only", "Bearer", http.StatusUnauthorized}, + {"basic scheme", "Basic " + valid, http.StatusUnauthorized}, + } + for _, c := range headers { + if got := serveAuth(h, http.MethodPost, c.authz); got != c.want { + t.Errorf("%s: status %d, want %d", c.name, got, c.want) + } + } +} + +func TestBearerOIDCHangingIssuerBacksOff(t *testing.T) { + var hits atomic.Int32 + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + select { + case <-r.Context().Done(): + case <-release: + } + })) + defer srv.Close() + defer close(release) + + b := NewBearerOIDC(srv.URL, nil) + b.timeout = 100 * time.Millisecond + b.backoff = time.Hour + h := b.Require(ok) + + start := time.Now() + if got := serve(h, http.MethodPost, "x.y.z"); got != http.StatusServiceUnavailable { + t.Fatalf("hanging issuer: status %d, want 503", got) + } + if d := time.Since(start); d > 2*time.Second { + t.Fatalf("hanging issuer: took %s, timeout not applied", d) + } + if got := serve(h, http.MethodPost, "x.y.z"); got != http.StatusServiceUnavailable { + t.Fatalf("during backoff: status %d, want 503", got) + } + if n := hits.Load(); n != 1 { + t.Fatalf("issuer hit %d times, want 1", n) + } +} + +func TestBearerOIDCRetriesAfterBackoff(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + b := NewBearerOIDC(iss.URL, nil) + b.failedAt = time.Now() + b.backoff = 50 * time.Millisecond + tok := iss.Token(nil, "artifactapi", "ci", time.Hour) + if got := serve(b.Require(ok), http.MethodPost, tok); got != http.StatusServiceUnavailable { + t.Fatalf("within backoff: status %d, want 503", got) + } + time.Sleep(60 * time.Millisecond) + if got := serve(b.Require(ok), http.MethodPost, tok); got != http.StatusNoContent { + t.Fatalf("after backoff: status %d, want 204", got) + } } func TestBearerOIDCIssuerDown(t *testing.T) { diff --git a/internal/testsupport/oidc.go b/internal/testsupport/oidc.go index 0ea124a..0acf546 100644 --- a/internal/testsupport/oidc.go +++ b/internal/testsupport/oidc.go @@ -45,6 +45,18 @@ func NewOIDCIssuer() *OIDCIssuer { // Token signs a token for aud and sub. Pass a nil key to use the issuer's own; // expiry is relative to now. func (o *OIDCIssuer) Token(key *rsa.PrivateKey, aud, sub string, expiry time.Duration) string { + now := time.Now() + return o.Sign(key, jwt.Claims{ + Issuer: o.URL, + Subject: sub, + Audience: jwt.Audience{aud}, + IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), + Expiry: jwt.NewNumericDate(now.Add(expiry)), + }) +} + +// Sign signs arbitrary claims with RS256. Pass a nil key to use the issuer's own. +func (o *OIDCIssuer) Sign(key *rsa.PrivateKey, claims jwt.Claims) string { if key == nil { key = o.key } @@ -53,14 +65,7 @@ func (o *OIDCIssuer) Token(key *rsa.PrivateKey, aud, sub string, expiry time.Dur if err != nil { panic(err) } - now := time.Now() - tok, err := jwt.Signed(sig).Claims(jwt.Claims{ - Issuer: o.URL, - Subject: sub, - Audience: jwt.Audience{aud}, - IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), - Expiry: jwt.NewNumericDate(now.Add(expiry)), - }).Serialize() + tok, err := jwt.Signed(sig).Claims(claims).Serialize() if err != nil { panic(err) } -- 2.47.3 From 98ad145b6f79a4e41490db9ef20cf822f2e8793f Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 14 Oct 2026 22:31:34 +1100 Subject: [PATCH 3/4] Wait on in-flight OIDC discovery instead of returning 503 --- internal/auth/oidc.go | 45 ++++++++++++++++++++++++++------------ internal/auth/oidc_test.go | 36 ++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 47cafea..86c63d9 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -22,10 +22,10 @@ type BearerOIDC struct { timeout time.Duration backoff time.Duration - mu sync.Mutex - verifier *oidc.IDTokenVerifier - discovering bool - failedAt time.Time + mu sync.Mutex + verifier *oidc.IDTokenVerifier + inflight chan struct{} + failedAt time.Time } var errIssuerUnavailable = errors.New("oidc issuer unavailable") @@ -39,18 +39,34 @@ func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { } // getVerifier runs at most one discovery at a time without holding the lock -// across network I/O; callers arriving mid-discovery or within the backoff fail fast. -func (b *BearerOIDC) getVerifier() (*oidc.IDTokenVerifier, error) { +// across network I/O; concurrent callers wait for it, and only callers within +// the post-failure backoff fail fast. +func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, error) { b.mu.Lock() - if b.verifier != nil || b.discovering || time.Since(b.failedAt) < b.backoff { - v := b.verifier + if v := b.verifier; v != nil { b.mu.Unlock() - if v == nil { - return nil, errIssuerUnavailable - } return v, nil } - b.discovering = true + if done := b.inflight; done != nil { + b.mu.Unlock() + select { + case <-done: + case <-ctx.Done(): + return nil, ctx.Err() + } + b.mu.Lock() + defer b.mu.Unlock() + if b.verifier == nil { + return nil, errIssuerUnavailable + } + return b.verifier, nil + } + if time.Since(b.failedAt) < b.backoff { + b.mu.Unlock() + return nil, errIssuerUnavailable + } + done := make(chan struct{}) + b.inflight = done b.mu.Unlock() // The provider keeps this context and client for JWKS refreshes, so the @@ -60,7 +76,8 @@ func (b *BearerOIDC) getVerifier() (*oidc.IDTokenVerifier, error) { b.mu.Lock() defer b.mu.Unlock() - b.discovering = false + b.inflight = nil + defer close(done) if err != nil { b.failedAt = time.Now() slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) @@ -83,7 +100,7 @@ func (b *BearerOIDC) Require(next http.Handler) http.Handler { unauthorized(w, "missing bearer token") return } - v, err := b.getVerifier() + v, err := b.getVerifier(r.Context()) if err != nil { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusServiceUnavailable) diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 38ea85a..8c552b6 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "sync" "sync/atomic" "testing" "time" @@ -145,6 +146,41 @@ func TestBearerOIDCHangingIssuerBacksOff(t *testing.T) { } } +func TestBearerOIDCConcurrentFirstRequestsWaitForDiscovery(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + var hits atomic.Int32 + inner := iss.Config.Handler + iss.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + hits.Add(1) + time.Sleep(200 * time.Millisecond) + } + inner.ServeHTTP(w, r) + }) + + h := NewBearerOIDC(iss.URL, nil).Require(ok) + tok := iss.Token(nil, "artifactapi", "ci", time.Hour) + var wg sync.WaitGroup + codes := make([]int, 20) + for i := range codes { + wg.Add(1) + go func() { + defer wg.Done() + codes[i] = serve(h, http.MethodPost, tok) + }() + } + wg.Wait() + for i, c := range codes { + if c != http.StatusNoContent { + t.Errorf("request %d: status %d, want 204", i, c) + } + } + if n := hits.Load(); n != 1 { + t.Fatalf("discovery hit %d times, want 1", n) + } +} + func TestBearerOIDCRetriesAfterBackoff(t *testing.T) { iss := testsupport.NewOIDCIssuer() defer iss.Close() -- 2.47.3 From 4d9f2bb6937a536ca6882da63e5f92970356946a Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 14 Oct 2026 22:36:00 +1100 Subject: [PATCH 4/4] Release OIDC discovery waiters when discovery panics --- internal/auth/oidc.go | 40 +++++++++++++++++++++++++------------- internal/auth/oidc_test.go | 25 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 86c63d9..d544bd0 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -21,6 +21,7 @@ type BearerOIDC struct { audiences []string timeout time.Duration backoff time.Duration + discover func(context.Context, string) (*oidc.Provider, error) mu sync.Mutex verifier *oidc.IDTokenVerifier @@ -35,7 +36,7 @@ func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { if issuer == "" { return nil } - return &BearerOIDC{issuer: issuer, audiences: audiences, timeout: 10 * time.Second, backoff: 15 * time.Second} + return &BearerOIDC{issuer: issuer, audiences: audiences, timeout: 10 * time.Second, backoff: 15 * time.Second, discover: oidc.NewProvider} } // getVerifier runs at most one discovery at a time without holding the lock @@ -69,22 +70,35 @@ func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, er b.inflight = done b.mu.Unlock() - // The provider keeps this context and client for JWKS refreshes, so the - // deadline lives on the client rather than the context. - client := &http.Client{Timeout: b.timeout} - p, err := oidc.NewProvider(oidc.ClientContext(context.Background(), client), b.issuer) - - b.mu.Lock() - defer b.mu.Unlock() - b.inflight = nil - defer close(done) + v, err := b.discoverOnce(done) if err != nil { - b.failedAt = time.Now() slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) return nil, err } - b.verifier = p.Verifier(&oidc.Config{SkipClientIDCheck: true}) - return b.verifier, nil + return v, nil +} + +// discoverOnce releases inflight and waiters on every exit, a panic included, +// recording failedAt unless discovery succeeded. +func (b *BearerOIDC) discoverOnce(done chan struct{}) (v *oidc.IDTokenVerifier, err error) { + defer func() { + b.mu.Lock() + b.inflight = nil + if v == nil { + b.failedAt = time.Now() + } + b.verifier = v + b.mu.Unlock() + close(done) + }() + // The provider keeps this context and client for JWKS refreshes, so the + // deadline lives on the client rather than the context. + client := &http.Client{Timeout: b.timeout} + p, err := b.discover(oidc.ClientContext(context.Background(), client), b.issuer) + if err != nil { + return nil, err + } + return p.Verifier(&oidc.Config{SkipClientIDCheck: true}), nil } // Require gates every request through next on a valid token. A nil receiver diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 8c552b6..c7e9c6d 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -1,6 +1,7 @@ package auth import ( + "context" "crypto/rand" "crypto/rsa" "encoding/base64" @@ -12,6 +13,7 @@ import ( "testing" "time" + "github.com/coreos/go-oidc/v3/oidc" "github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4/jwt" @@ -146,6 +148,29 @@ func TestBearerOIDCHangingIssuerBacksOff(t *testing.T) { } } +func TestBearerOIDCPanickingDiscoveryBacksOff(t *testing.T) { + b := NewBearerOIDC("http://issuer.invalid", nil) + b.backoff = time.Hour + b.discover = func(context.Context, string) (*oidc.Provider, error) { panic("boom") } + h := b.Require(ok) + + func() { + defer func() { _ = recover() }() + serve(h, http.MethodPost, "x.y.z") + }() + + got := make(chan int, 1) + go func() { got <- serve(h, http.MethodPost, "x.y.z") }() + select { + case code := <-got: + if code != http.StatusServiceUnavailable { + t.Fatalf("after panic: status %d, want 503", code) + } + case <-time.After(2 * time.Second): + t.Fatal("after panic: request hung on stale inflight discovery") + } +} + func TestBearerOIDCConcurrentFirstRequestsWaitForDiscovery(t *testing.T) { iss := testsupport.NewOIDCIssuer() defer iss.Close() -- 2.47.3