From 909e2e464abe1a3193ef31bbf2f72c50c6b0fac5 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Mon, 19 Oct 2026 00:02:05 -0400 Subject: [PATCH 1/8] fix(s3): signatures for alternate hosts --- config.example.yaml | 9 ++-- internal/handler/handler.go | 28 +--------- internal/handler/handler_test.go | 68 ------------------------- internal/server/server.go | 7 ++- internal/storage/blob.go | 87 +++++++++++++++++++++++++++++--- internal/storage/blob_test.go | 62 +++++++++++++++++++++++ 6 files changed, 151 insertions(+), 110 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index e43e4346..1967213d 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -133,12 +133,9 @@ storage: # How long presigned URLs remain valid (e.g. "5m", "1h"). Default: "15m". direct_serve_ttl: "15m" - # Public base URL to substitute into presigned URLs. Set this when the - # proxy reaches storage at an internal address (127.0.0.1, a Docker - # service name) but clients must use a public hostname. Only scheme and - # host are used; the signed path and query are preserved. For S3/MinIO - # the reverse proxy at this address must forward requests with the - # internal Host header or the SigV4 signature will not validate. + # Public base URL for presigned URLs. Set this when the proxy reaches + # storage at an internal address (127.0.0.1, a Docker service name, or + # VPC DNS) but clients must use a public hostname or CDN. # direct_serve_base_url: "https://minio.example.com" # Database configuration diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 125b1174..6d38118d 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -240,11 +240,7 @@ type Proxy struct { NPMFullMetadata bool DirectServe bool DirectServeTTL time.Duration - // DirectServeBaseURL, if set, replaces the scheme and host of presigned - // URLs so clients receive a public address even when the proxy reaches - // storage at an internal one. - DirectServeBaseURL string - HTTPClient *http.Client + HTTPClient *http.Client // RateLimits is the gate under the upstream clients. Artifact fetches // check it before calling Fetcher, so held hosts never reach its breaker. @@ -548,7 +544,7 @@ func (p *Proxy) readStored(ctx context.Context, pkgPURL, versionPURL, filename s if p.DirectServe { signed, err := p.Storage.SignedURL(ctx, artifact.StoragePath, p.DirectServeTTL) if err == nil { - result.RedirectURL = rewriteSignedURLHost(signed, p.DirectServeBaseURL) + result.RedirectURL = signed p.recordCacheHit(artifact.Ecosystem, versionPURL, filename) return result, nil } @@ -595,26 +591,6 @@ func (p *Proxy) readStored(ctx context.Context, pkgPURL, versionPURL, filename s return result, nil } -// rewriteSignedURLHost replaces the scheme and host of a signed URL with those -// from baseURL, preserving the path and query (which carry the signature). -// Returns signed unchanged if baseURL is empty or either URL fails to parse. -func rewriteSignedURLHost(signed, baseURL string) string { - if baseURL == "" { - return signed - } - s, err := url.Parse(signed) - if err != nil { - return signed - } - b, err := url.Parse(baseURL) - if err != nil || b.Scheme == "" || b.Host == "" { - return signed - } - s.Scheme = b.Scheme - s.Host = b.Host - return s.String() -} - func (p *Proxy) recordCacheHit(ecosystem, versionPURL, filename string) { _ = p.DB.RecordArtifactHit(versionPURL, filename) metrics.RecordArtifactCacheHit(ecosystem) diff --git a/internal/handler/handler_test.go b/internal/handler/handler_test.go index 8ce636c9..3ae885e1 100644 --- a/internal/handler/handler_test.go +++ b/internal/handler/handler_test.go @@ -638,74 +638,6 @@ func TestGetOrFetchArtifact_DirectServe_Redirect(t *testing.T) { } } -func TestGetOrFetchArtifact_DirectServe_BaseURLRewrite(t *testing.T) { - proxy, db, store, _ := setupTestProxy(t) - seedPackage(t, db, store, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz", "cached content") - - proxy.DirectServe = true - proxy.DirectServeBaseURL = "https://cdn.example.com" - store.signedURL = "http://127.0.0.1:9000/bucket/npm/lodash?X-Amz-Signature=abc&X-Amz-Expires=900" - - result, err := proxy.GetOrFetchArtifact(context.Background(), "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - want := "https://cdn.example.com/bucket/npm/lodash?X-Amz-Signature=abc&X-Amz-Expires=900" - if result.RedirectURL != want { - t.Errorf("RedirectURL = %q, want %q", result.RedirectURL, want) - } -} - -func TestRewriteSignedURLHost(t *testing.T) { - tests := []struct { - name string - signed string - baseURL string - want string - }{ - { - "empty base url is no-op", - "http://127.0.0.1:9000/bucket/key?sig=abc", - "", - "http://127.0.0.1:9000/bucket/key?sig=abc", - }, - { - "replaces scheme and host", - "http://127.0.0.1:9000/bucket/key?sig=abc", - "https://cdn.example.com", - "https://cdn.example.com/bucket/key?sig=abc", - }, - { - "preserves path and query", - "http://minio:9000/bucket/npm/lodash/4.17.21/lodash.tgz?X-Amz-Signature=abc&X-Amz-Date=20260101", - "https://files.example.com", - "https://files.example.com/bucket/npm/lodash/4.17.21/lodash.tgz?X-Amz-Signature=abc&X-Amz-Date=20260101", - }, - { - "ignores base url path", - "http://127.0.0.1:9000/bucket/key?sig=abc", - "https://cdn.example.com/ignored", - "https://cdn.example.com/bucket/key?sig=abc", - }, - { - "invalid base url is no-op", - "http://127.0.0.1:9000/bucket/key?sig=abc", - "://bad", - "http://127.0.0.1:9000/bucket/key?sig=abc", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := rewriteSignedURLHost(tt.signed, tt.baseURL) - if got != tt.want { - t.Errorf("rewriteSignedURLHost(%q, %q) = %q, want %q", tt.signed, tt.baseURL, got, tt.want) - } - }) - } -} - func TestGetOrFetchArtifact_DirectServe_FallbackOnUnsupported(t *testing.T) { proxy, db, store, _ := setupTestProxy(t) seedPackage(t, db, store, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz", "cached content") diff --git a/internal/server/server.go b/internal/server/server.go index 55fe597b..4466d06f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -189,7 +189,11 @@ func New(cfg *config.Config, logger *slog.Logger, buildInfo BuildInfo) (*Server, // Fall back to file:// with Path storageURL = "file://" + cfg.Storage.Path //nolint:staticcheck // backwards compat } - store, err := storage.OpenBucket(context.Background(), storageURL) + var storageOpts []storage.Option + if cfg.Storage.DirectServe && cfg.Storage.DirectServeBaseURL != "" { + storageOpts = append(storageOpts, storage.WithSignEndpoint(cfg.Storage.DirectServeBaseURL)) + } + store, err := storage.OpenBucket(context.Background(), storageURL, storageOpts...) if err != nil { _ = db.Close() return nil, fmt.Errorf("initializing storage: %w", err) @@ -317,7 +321,6 @@ func (s *Server) serve(listener net.Listener) error { proxy.GradleMaxUploadSize = s.cfg.ParseGradleBuildCacheMaxUploadSize() proxy.DirectServe = s.cfg.Storage.DirectServe proxy.DirectServeTTL = s.cfg.ParseDirectServeTTL() - proxy.DirectServeBaseURL = s.cfg.Storage.DirectServeBaseURL proxy.StreamArtifacts = !s.cfg.Storage.CacheArtifacts if err := ConfigureDenylist(s.cfg, proxy); err != nil { return err diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 81af78f2..cf3ee0f6 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path/filepath" "runtime" @@ -30,11 +31,13 @@ const osWindows = "windows" const ( uploadBufferSize = 5 << 20 uploadConcurrency = 2 -) -// attrsExt is fileblob's sidecar suffix, kept only to clear sidecars an -// earlier version wrote. -const attrsExt = ".attrs" + s3Scheme = "s3://" + + // attrsExt is fileblob's sidecar suffix, kept only to clear sidecars an + // earlier version wrote. + attrsExt = ".attrs" +) // Blob implements Storage using gocloud.dev/blob. // Supports local filesystem (file://) and S3 (s3://) URLs. @@ -42,11 +45,29 @@ type Blob struct { bucket *blob.Bucket url string + // signBucket, when set, is used by SignedURL to sign S3 presigned URLs for + // a public endpoint rather than the internal storage endpoint. + signBucket *blob.Bucket + // fileRoot is the directory backing a file:// bucket, empty for cloud // backends. Used only to clear sidecars an earlier version wrote. fileRoot string } +// Option configures OpenBucket. +type Option func(*options) + +type options struct { + signEndpoint string +} + +// WithSignEndpoint configures S3 presigned URLs to be signed for the scheme and +// host of signEndpoint (e.g. "https://storage.example.com") instead of the +// storage endpoint, since SigV4 includes the Host header in the signature. +func WithSignEndpoint(signEndpoint string) Option { + return func(o *options) { o.signEndpoint = signEndpoint } +} + // OpenBucket opens a blob bucket from a URL. // // Supported URL schemes: @@ -60,7 +81,12 @@ type Blob struct { // For local filesystem, the directory is created if it doesn't exist. // //nolint:ireturn // The URL scheme selects the storage implementation. -func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { +func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, error) { + var o options + for _, opt := range opts { + opt(&o) + } + if strings.HasPrefix(urlStr, "gs://") { return OpenGCS(ctx, urlStr) } @@ -124,7 +150,42 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return nil, fmt.Errorf("opening bucket: %w", err) } - return &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot}, nil + b := &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot} + + if o.signEndpoint != "" && strings.HasPrefix(urlStr, s3Scheme) { + signBucket, err := openSignBucket(ctx, urlStr, o.signEndpoint) + if err != nil { + _ = bucket.Close() + return nil, err + } + b.signBucket = signBucket + } + + return b, nil +} + +// openSignBucket opens an S3 bucket with the endpoint overridden to signEndpoint +// while preserving all other storage URL parameters (region, use_path_style, etc.). +func openSignBucket(ctx context.Context, urlStr, signEndpoint string) (*blob.Bucket, error) { + sign, err := url.Parse(signEndpoint) + if err != nil || sign.Scheme == "" || sign.Host == "" { + return nil, fmt.Errorf("invalid sign endpoint %q: must be an absolute URL", signEndpoint) + } + + u, err := url.Parse(urlStr) + if err != nil { + return nil, fmt.Errorf("parsing storage URL: %w", err) + } + endpoint := sign.Scheme + "://" + sign.Host + q := u.Query() + q.Set("endpoint", endpoint) + u.RawQuery = q.Encode() + + bucket, err := blob.OpenBucket(ctx, u.String()) + if err != nil { + return nil, fmt.Errorf("opening bucket at sign endpoint %s: %w", endpoint, err) + } + return bucket, nil } // legacySidecarPath gives the ".attrs" path an earlier version wrote for key, @@ -299,7 +360,11 @@ func (b *Blob) Delete(ctx context.Context, path string) error { } func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { - url, err := b.bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ + bucket := b.bucket + if b.signBucket != nil { + bucket = b.signBucket + } + url, err := bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ Method: http.MethodGet, Expiry: expiry, }) @@ -371,7 +436,13 @@ func (b *Blob) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, err } func (b *Blob) Close() error { - return b.bucket.Close() + err := b.bucket.Close() + if b.signBucket != nil { + if signErr := b.signBucket.Close(); err == nil { + err = signErr + } + } + return err } func (b *Blob) URL() string { diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 1a764de7..4811c7eb 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -7,6 +7,7 @@ import ( "errors" "io" "io/fs" + "net/url" "os" "path/filepath" "runtime" @@ -260,6 +261,67 @@ func TestBlobSignedURLUnsupported(t *testing.T) { } } +func TestBlobSignedURLSignEndpoint(t *testing.T) { + t.Setenv("AWS_ACCESS_KEY_ID", "test") + t.Setenv("AWS_SECRET_ACCESS_KEY", "test") + + const ( + storageURL = "s3://bucket?region=us-1&endpoint=http://127.0.0.1:9000&use_path_style=true" + signEndpoint = "https://cdn.example.com" + artifactPath = "npm/lodash/4.17.21/lodash-4.17.21.tgz" + ) + + tests := []struct { + name string + opts []Option + host string + }{ + { + "storage endpoint without a sign endpoint", + nil, + "127.0.0.1:9000", + }, + { + "configured sign endpoint, reached over another host", + []Option{WithSignEndpoint(signEndpoint)}, + "cdn.example.com", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + b, err := OpenBucket(ctx, storageURL, tt.opts...) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + signed, err := b.SignedURL(ctx, artifactPath, time.Minute) + if err != nil { + t.Fatalf("SignedURL failed: %v", err) + } + + u, err := url.Parse(signed) + if err != nil { + t.Fatalf("parsing signed URL %q: %v", signed, err) + } + if u.Host != tt.host { + t.Errorf("signed URL %q host = %q, want %q", signed, u.Host, tt.host) + } + // The bucket and key stay where the storage URL puts them, so the + // request the client makes still names the object. + if want := "/bucket/" + artifactPath; u.Path != want { + t.Errorf("signed URL path = %q, want %q", u.Path, want) + } + if u.Query().Get("X-Amz-Signature") == "" { + t.Errorf("signed URL %q carries no signature", signed) + } + }) + } +} + func TestBlobOverwrite(t *testing.T) { b := createTestBlob(t) ctx := context.Background() From f82ec690068dce9ec4e5edcad407ea35795b8025 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Fri, 6 Nov 2026 20:06:36 -0500 Subject: [PATCH 2/8] fix(storage): restore Azure/GCS direct serve signing --- internal/storage/blob.go | 62 +++++++++++++++++++++------ internal/storage/blob_test.go | 79 +++++++++++++++++++++++++++++++++++ internal/storage/gcs.go | 10 +++-- internal/storage/gcs_test.go | 56 +++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 15 deletions(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index cf3ee0f6..eeb8b3b9 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -49,6 +49,10 @@ type Blob struct { // a public endpoint rather than the internal storage endpoint. signBucket *blob.Bucket + // signEndpoint, when set on non-S3 backends, replaces the scheme and host + // of presigned URLs returned by SignedURL. + signEndpoint string + // fileRoot is the directory backing a file:// bucket, empty for cloud // backends. Used only to clear sidecars an earlier version wrote. fileRoot string @@ -61,9 +65,11 @@ type options struct { signEndpoint string } -// WithSignEndpoint configures S3 presigned URLs to be signed for the scheme and -// host of signEndpoint (e.g. "https://storage.example.com") instead of the -// storage endpoint, since SigV4 includes the Host header in the signature. +// WithSignEndpoint configures presigned URLs to use the scheme and host of +// signEndpoint (e.g. "https://storage.example.com") instead of the storage +// endpoint. For S3, the URL is signed directly against signEndpoint so the +// SigV4 Host signature matches; for other backends, the signed URL's scheme +// and host are rewritten. func WithSignEndpoint(signEndpoint string) Option { return func(o *options) { o.signEndpoint = signEndpoint } } @@ -88,7 +94,12 @@ func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, er } if strings.HasPrefix(urlStr, "gs://") { - return OpenGCS(ctx, urlStr) + g, err := OpenGCS(ctx, urlStr) + if err != nil { + return nil, err + } + g.signEndpoint = o.signEndpoint + return g, nil } var fileRoot string @@ -152,13 +163,17 @@ func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, er b := &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot} - if o.signEndpoint != "" && strings.HasPrefix(urlStr, s3Scheme) { - signBucket, err := openSignBucket(ctx, urlStr, o.signEndpoint) - if err != nil { - _ = bucket.Close() - return nil, err + if o.signEndpoint != "" { + if strings.HasPrefix(urlStr, s3Scheme) { + signBucket, err := openSignBucket(ctx, urlStr, o.signEndpoint) + if err != nil { + _ = bucket.Close() + return nil, err + } + b.signBucket = signBucket + } else { + b.signEndpoint = o.signEndpoint } - b.signBucket = signBucket } return b, nil @@ -364,7 +379,7 @@ func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) if b.signBucket != nil { bucket = b.signBucket } - url, err := bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ + signed, err := bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ Method: http.MethodGet, Expiry: expiry, }) @@ -374,7 +389,30 @@ func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) } return "", fmt.Errorf("signing URL: %w", err) } - return url, nil + if b.signBucket == nil && b.signEndpoint != "" { + signed = rewriteSignedURLHost(signed, b.signEndpoint) + } + return signed, nil +} + +// rewriteSignedURLHost replaces the scheme and host of a signed URL with those +// from baseURL, preserving the path and query (which carry the signature). +// Returns signed unchanged if baseURL is empty or either URL fails to parse. +func rewriteSignedURLHost(signed, baseURL string) string { + if baseURL == "" { + return signed + } + s, err := url.Parse(signed) + if err != nil { + return signed + } + b, err := url.Parse(baseURL) + if err != nil || b.Scheme == "" || b.Host == "" { + return signed + } + s.Scheme = b.Scheme + s.Host = b.Host + return s.String() } func (b *Blob) Size(ctx context.Context, path string) (int64, error) { diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 4811c7eb..bffe09b0 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -322,6 +322,85 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { } } +func TestAzureSignedURLSignEndpoint(t *testing.T) { + t.Setenv("AZURE_STORAGE_ACCOUNT", "devstoreaccount1") + t.Setenv("AZURE_STORAGE_KEY", "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==") + t.Setenv("AZURE_STORAGE_PROTOCOL", "http") + t.Setenv("AZURE_STORAGE_IS_LOCAL_EMULATOR", "true") + + ctx := context.Background() + b, err := OpenBucket(ctx, "azblob://bucket?domain=127.0.0.1:10000", WithSignEndpoint("https://cdn.example.com")) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + signed, err := b.SignedURL(ctx, "npm/lodash/4.17.21/lodash-4.17.21.tgz", time.Minute) + if err != nil { + t.Fatalf("SignedURL failed: %v", err) + } + + u, err := url.Parse(signed) + if err != nil { + t.Fatalf("parsing signed URL %q: %v", signed, err) + } + if u.Scheme != "https" || u.Host != "cdn.example.com" { + t.Errorf("signed URL = %q, want https://cdn.example.com", signed) + } + if u.Query().Get("sig") == "" { + t.Errorf("signed URL %q carries no SAS signature", signed) + } +} + +func TestRewriteSignedURLHost(t *testing.T) { + tests := []struct { + name string + signed string + baseURL string + want string + }{ + { + "empty base url is no-op", + "http://127.0.0.1:9000/bucket/key?sig=abc", + "", + "http://127.0.0.1:9000/bucket/key?sig=abc", + }, + { + "replaces scheme and host", + "http://127.0.0.1:9000/bucket/key?sig=abc", + "https://cdn.example.com", + "https://cdn.example.com/bucket/key?sig=abc", + }, + { + "preserves path and query", + "http://minio:9000/bucket/npm/lodash/4.17.21/lodash.tgz?X-Amz-Signature=abc&X-Amz-Date=20260101", + "https://files.example.com", + "https://files.example.com/bucket/npm/lodash/4.17.21/lodash.tgz?X-Amz-Signature=abc&X-Amz-Date=20260101", + }, + { + "ignores base url path", + "http://127.0.0.1:9000/bucket/key?sig=abc", + "https://cdn.example.com/ignored", + "https://cdn.example.com/bucket/key?sig=abc", + }, + { + "invalid base url is no-op", + "http://127.0.0.1:9000/bucket/key?sig=abc", + "://bad", + "http://127.0.0.1:9000/bucket/key?sig=abc", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := rewriteSignedURLHost(tt.signed, tt.baseURL) + if got != tt.want { + t.Errorf("rewriteSignedURLHost(%q, %q) = %q, want %q", tt.signed, tt.baseURL, got, tt.want) + } + }) + } +} + func TestBlobOverwrite(t *testing.T) { b := createTestBlob(t) ctx := context.Background() diff --git a/internal/storage/gcs.go b/internal/storage/gcs.go index a096c18c..43b14e55 100644 --- a/internal/storage/gcs.go +++ b/internal/storage/gcs.go @@ -13,8 +13,9 @@ import ( // GCS adapts a Google Cloud Storage bucket to Storage. type GCS struct { - bucket *gcstorage.Bucket - url string + bucket *gcstorage.Bucket + url string + signEndpoint string } // OpenGCS opens a Google Cloud Storage bucket from a gs:// URL. @@ -64,7 +65,10 @@ func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) if errors.Is(err, gcstorage.ErrSignedURLUnsupported) { return "", ErrSignedURLUnsupported } - return u, err + if err != nil { + return "", err + } + return rewriteSignedURLHost(u, g.signEndpoint), nil } func (g *GCS) UsedSpace(ctx context.Context) (int64, error) { diff --git a/internal/storage/gcs_test.go b/internal/storage/gcs_test.go index 1bd8a407..461fb866 100644 --- a/internal/storage/gcs_test.go +++ b/internal/storage/gcs_test.go @@ -2,14 +2,20 @@ package storage import ( "context" + "crypto/rand" + "crypto/rsa" "crypto/sha256" + "crypto/x509" "encoding/hex" "encoding/json" + "encoding/pem" "errors" "io" "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" "sort" "strconv" "strings" @@ -89,6 +95,56 @@ func TestOpenBucketGCSRoundTripWithEmulator(t *testing.T) { } } +func TestGCSSignedURLSignEndpoint(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating RSA key: %v", err) + } + pemBytes := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + credsJSON, err := json.Marshal(map[string]string{ + "type": "service_account", + "client_email": "test@example.iam.gserviceaccount.com", + "private_key": string(pemBytes), + }) + if err != nil { + t.Fatalf("marshaling creds: %v", err) + } + credsFile := filepath.Join(t.TempDir(), "creds.json") + if err := os.WriteFile(credsFile, credsJSON, 0o600); err != nil { + t.Fatalf("writing creds: %v", err) + } + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", credsFile) + t.Setenv("STORAGE_EMULATOR_HOST", "") + + ctx := context.Background() + store, err := OpenBucket(ctx, "gs://test-bucket", WithSignEndpoint("https://cdn.example.com")) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = store.Close() }() + + signed, err := store.SignedURL(ctx, "npm/pkg/file.tgz", time.Minute) + if err != nil { + t.Fatalf("SignedURL failed: %v", err) + } + u, err := url.Parse(signed) + if err != nil { + t.Fatalf("parsing signed URL %q: %v", signed, err) + } + if u.Scheme != "https" || u.Host != "cdn.example.com" { + t.Errorf("signed URL = %q, want https://cdn.example.com", signed) + } + if u.Path != "/test-bucket/npm/pkg/file.tgz" { + t.Errorf("signed URL path = %q, want /test-bucket/npm/pkg/file.tgz", u.Path) + } + if u.Query().Get("Signature") == "" { + t.Errorf("signed URL %q carries no signature", signed) + } +} + type fakeGCSServer struct { t *testing.T objects map[string]string From 5c1eaa9fe5a1189476089a414178da0c910690db Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sat, 7 Nov 2026 13:57:40 -0500 Subject: [PATCH 3/8] chore: test reliability / godoc on methods Code review feedback --- internal/storage/blob.go | 11 +++++++++++ internal/storage/blob_test.go | 14 ++++++++++++++ internal/storage/gcs.go | 11 +++++++++++ 3 files changed, 36 insertions(+) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index eeb8b3b9..7519da8e 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -286,6 +286,7 @@ func (b *Blob) clearLegacySidecar(key string) { } } +// Store writes the contents of r to path and returns the byte count and hex SHA-256 digest. func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { b.clearLegacySidecar(path) @@ -322,6 +323,7 @@ func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, stri return size, hash, nil } +// Open returns a reader for the object stored at path. func (b *Blob) Open(ctx context.Context, path string) (io.ReadCloser, error) { if localPath := b.localPath(path); localPath != "" { r, err := os.Open(localPath) @@ -349,6 +351,7 @@ func (b *Blob) Open(ctx context.Context, path string) (io.ReadCloser, error) { // otherwise. func (b *Blob) ReadsSeekable() bool { return true } +// Exists reports whether an object exists at path. func (b *Blob) Exists(ctx context.Context, path string) (bool, error) { exists, err := b.bucket.Exists(ctx, path) if err != nil { @@ -374,6 +377,10 @@ func (b *Blob) Delete(ctx context.Context, path string) error { return nil } +// SignedURL returns a presigned URL granting time-limited GET access to path. +// When signBucket is configured (S3 with WithSignEndpoint), it signs against +// the public endpoint; when signEndpoint is set on other backends (e.g. Azure), +// it rewrites the signed URL's scheme and host. func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { bucket := b.bucket if b.signBucket != nil { @@ -415,6 +422,7 @@ func rewriteSignedURLHost(signed, baseURL string) string { return s.String() } +// Size returns the size in bytes of the object at path. func (b *Blob) Size(ctx context.Context, path string) (int64, error) { attrs, err := b.bucket.Attributes(ctx, path) if err != nil { @@ -426,6 +434,7 @@ func (b *Blob) Size(ctx context.Context, path string) (int64, error) { return attrs.Size, nil } +// UsedSpace returns the total bytes stored across all objects in the bucket. func (b *Blob) UsedSpace(ctx context.Context) (int64, error) { var total int64 @@ -473,6 +482,7 @@ func (b *Blob) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, err return objects, nil } +// Close closes the underlying storage bucket and any separate signing bucket. func (b *Blob) Close() error { err := b.bucket.Close() if b.signBucket != nil { @@ -483,6 +493,7 @@ func (b *Blob) Close() error { return err } +// URL returns the storage backend URL. func (b *Blob) URL() string { return b.url } diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index bffe09b0..7a46413b 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -262,6 +262,19 @@ func TestBlobSignedURLUnsupported(t *testing.T) { } func TestBlobSignedURLSignEndpoint(t *testing.T) { + awsDir := t.TempDir() + awsConfigFile := filepath.Join(awsDir, "config") + awsCredsFile := filepath.Join(awsDir, "credentials") + if err := os.WriteFile(awsConfigFile, nil, 0o600); err != nil { + t.Fatalf("writing empty AWS config file: %v", err) + } + if err := os.WriteFile(awsCredsFile, nil, 0o600); err != nil { + t.Fatalf("writing empty AWS credentials file: %v", err) + } + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_DEFAULT_PROFILE", "") + t.Setenv("AWS_CONFIG_FILE", awsConfigFile) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", awsCredsFile) t.Setenv("AWS_ACCESS_KEY_ID", "test") t.Setenv("AWS_SECRET_ACCESS_KEY", "test") @@ -323,6 +336,7 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { } func TestAzureSignedURLSignEndpoint(t *testing.T) { + t.Setenv("AZURE_STORAGE_SAS_TOKEN", "") t.Setenv("AZURE_STORAGE_ACCOUNT", "devstoreaccount1") t.Setenv("AZURE_STORAGE_KEY", "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==") t.Setenv("AZURE_STORAGE_PROTOCOL", "http") diff --git a/internal/storage/gcs.go b/internal/storage/gcs.go index 43b14e55..c2c98c0b 100644 --- a/internal/storage/gcs.go +++ b/internal/storage/gcs.go @@ -27,6 +27,7 @@ func OpenGCS(ctx context.Context, urlStr string) (*GCS, error) { return &GCS{bucket: bucket, url: urlStr}, nil } +// Store writes the contents of r to path and returns the byte count and hex SHA-256 digest. func (g *GCS) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { h := sha256.New() size, err := g.bucket.Write(ctx, path, io.TeeReader(r, h)) @@ -36,6 +37,7 @@ func (g *GCS) Store(ctx context.Context, path string, r io.Reader) (int64, strin return size, hex.EncodeToString(h.Sum(nil)), nil } +// Open returns a reader for the object stored at path. func (g *GCS) Open(ctx context.Context, path string) (io.ReadCloser, error) { r, err := g.bucket.Open(ctx, path) if errors.Is(err, gcstorage.ErrNotFound) { @@ -44,14 +46,17 @@ func (g *GCS) Open(ctx context.Context, path string) (io.ReadCloser, error) { return r, err } +// Exists reports whether an object exists at path. func (g *GCS) Exists(ctx context.Context, path string) (bool, error) { return g.bucket.Exists(ctx, path) } +// Delete removes the object at path. func (g *GCS) Delete(ctx context.Context, path string) error { return g.bucket.Delete(ctx, path) } +// Size returns the size in bytes of the object at path. func (g *GCS) Size(ctx context.Context, path string) (int64, error) { size, err := g.bucket.Size(ctx, path) if errors.Is(err, gcstorage.ErrNotFound) { @@ -60,6 +65,8 @@ func (g *GCS) Size(ctx context.Context, path string) (int64, error) { return size, err } +// SignedURL returns a presigned URL granting time-limited GET access to path, +// rewriting the scheme and host to signEndpoint when configured. func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { u, err := g.bucket.SignedURL(ctx, path, expiry) if errors.Is(err, gcstorage.ErrSignedURLUnsupported) { @@ -71,10 +78,12 @@ func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) return rewriteSignedURLHost(u, g.signEndpoint), nil } +// UsedSpace returns the total bytes stored across all objects in the bucket. func (g *GCS) UsedSpace(ctx context.Context) (int64, error) { return g.bucket.UsedSpace(ctx) } +// ListPrefix returns object metadata for keys under a prefix. func (g *GCS) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, error) { objects, err := g.bucket.ListPrefix(ctx, prefix) if err != nil { @@ -92,10 +101,12 @@ func (g *GCS) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, erro return result, nil } +// Close releases any resources held by the GCS client. func (g *GCS) Close() error { return nil } +// URL returns the storage backend URL. func (g *GCS) URL() string { return g.url } From e4bc1f5f11b32fad5acf60e62636b3fc28e96bb2 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sat, 7 Nov 2026 16:50:24 -0500 Subject: [PATCH 4/8] fix: do not prepend host on signing --- internal/server/server_test.go | 57 ++++++++++++++++++++++++++++++ internal/storage/blob.go | 41 +++++++++++++++------- internal/storage/blob_test.go | 64 ++++++++++++++++++++++++++++------ 3 files changed, 140 insertions(+), 22 deletions(-) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index e2d22648..93328acc 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1655,6 +1655,63 @@ func TestNewServer_StorageConnectivityCheck(t *testing.T) { _ = srv.db.Close() } +func TestNewServer_DirectServeBaseURL(t *testing.T) { + tempDir := t.TempDir() + awsConfigFile := filepath.Join(tempDir, "aws-config") + awsCredsFile := filepath.Join(tempDir, "aws-credentials") + if err := os.WriteFile(awsConfigFile, nil, 0o600); err != nil { + t.Fatalf("writing empty AWS config file: %v", err) + } + if err := os.WriteFile(awsCredsFile, nil, 0o600); err != nil { + t.Fatalf("writing empty AWS credentials file: %v", err) + } + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_DEFAULT_PROFILE", "") + t.Setenv("AWS_CONFIG_FILE", awsConfigFile) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", awsCredsFile) + t.Setenv("AWS_ACCESS_KEY_ID", "test") + t.Setenv("AWS_SECRET_ACCESS_KEY", "test") + + s3Srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer s3Srv.Close() + + cfg := &config.Config{ + Listen: ":0", + BaseURL: "http://localhost:8080", + Storage: config.StorageConfig{ + URL: "s3://bucket?region=us-1&endpoint=" + s3Srv.URL + "&use_path_style=true", + DirectServe: true, + DirectServeBaseURL: "https://cdn.example.com", + }, + Database: config.DatabaseConfig{Path: filepath.Join(tempDir, "test.db")}, + } + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + srv, err := New(cfg, logger, BuildInfo{}) + if err != nil { + t.Fatalf("New() failed: %v", err) + } + defer func() { + _ = srv.storage.Close() + _ = srv.db.Close() + }() + + signed, err := srv.storage.SignedURL(context.Background(), "npm/lodash/4.17.21/lodash-4.17.21.tgz", time.Minute) + if err != nil { + t.Fatalf("SignedURL() failed: %v", err) + } + + u, err := url.Parse(signed) + if err != nil { + t.Fatalf("parsing signed URL %q: %v", signed, err) + } + if u.Scheme != "https" || u.Host != "cdn.example.com" { + t.Errorf("signed URL = %q, want scheme https and host cdn.example.com", signed) + } +} + func TestNewServer_InvalidAccessLogFailsBeforeDatabaseInit(t *testing.T) { tempDir := t.TempDir() dbPath := filepath.Join(tempDir, "test.db") diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 7519da8e..06685397 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -14,6 +14,9 @@ import ( "strings" "time" + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/aws/smithy-go/middleware" + smithyhttp "github.com/aws/smithy-go/transport/http" "gocloud.dev/blob" _ "gocloud.dev/blob/azureblob" _ "gocloud.dev/blob/fileblob" @@ -179,27 +182,41 @@ func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, er return b, nil } -// openSignBucket opens an S3 bucket with the endpoint overridden to signEndpoint -// while preserving all other storage URL parameters (region, use_path_style, etc.). +// openSignBucket opens an S3 bucket that signs presigned URLs against the +// scheme and host of signEndpoint while preserving the storage URL's +// addressing mode and object path. func openSignBucket(ctx context.Context, urlStr, signEndpoint string) (*blob.Bucket, error) { sign, err := url.Parse(signEndpoint) if err != nil || sign.Scheme == "" || sign.Host == "" { return nil, fmt.Errorf("invalid sign endpoint %q: must be an absolute URL", signEndpoint) } - u, err := url.Parse(urlStr) + bucket, err := blob.OpenBucket(ctx, urlStr) if err != nil { - return nil, fmt.Errorf("parsing storage URL: %w", err) + return nil, fmt.Errorf("opening bucket at sign endpoint %s: %w", signEndpoint, err) + } + + var s3Client *s3.Client + if bucket.As(&s3Client) { + *s3Client = *s3.New(s3Client.Options(), func(o *s3.Options) { + o.APIOptions = append(o.APIOptions, func(stack *middleware.Stack) error { + return stack.Finalize.Insert(middleware.FinalizeMiddlewareFunc( + "signEndpointHost", + func(ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( + middleware.FinalizeOutput, middleware.Metadata, error, + ) { + if req, ok := in.Request.(*smithyhttp.Request); ok { + req.URL.Scheme = sign.Scheme + req.URL.Host = sign.Host + req.Host = sign.Host + } + return next.HandleFinalize(ctx, in) + }, + ), "PresignHTTPRequest", middleware.Before) + }) + }) } - endpoint := sign.Scheme + "://" + sign.Host - q := u.Query() - q.Set("endpoint", endpoint) - u.RawQuery = q.Encode() - bucket, err := blob.OpenBucket(ctx, u.String()) - if err != nil { - return nil, fmt.Errorf("opening bucket at sign endpoint %s: %w", endpoint, err) - } return bucket, nil } diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 7a46413b..233b501c 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -7,6 +7,7 @@ import ( "errors" "io" "io/fs" + "net/http" "net/url" "os" "path/filepath" @@ -17,6 +18,8 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" + awssigv4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" "gocloud.dev/blob" ) @@ -279,25 +282,39 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { t.Setenv("AWS_SECRET_ACCESS_KEY", "test") const ( - storageURL = "s3://bucket?region=us-1&endpoint=http://127.0.0.1:9000&use_path_style=true" + pathStyleURL = "s3://bucket?region=us-1&endpoint=http://127.0.0.1:9000&use_path_style=true" + vhostURL = "s3://bucket?region=us-1&endpoint=http://127.0.0.1:9000" signEndpoint = "https://cdn.example.com" artifactPath = "npm/lodash/4.17.21/lodash-4.17.21.tgz" ) tests := []struct { - name string - opts []Option - host string + name string + storageURL string + opts []Option + host string + path string }{ { "storage endpoint without a sign endpoint", + pathStyleURL, nil, "127.0.0.1:9000", + "/bucket/" + artifactPath, }, { "configured sign endpoint, reached over another host", + pathStyleURL, []Option{WithSignEndpoint(signEndpoint)}, "cdn.example.com", + "/bucket/" + artifactPath, + }, + { + "configured sign endpoint with virtual-hosted storage URL", + vhostURL, + []Option{WithSignEndpoint(signEndpoint)}, + "cdn.example.com", + "/" + artifactPath, }, } @@ -305,7 +322,7 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - b, err := OpenBucket(ctx, storageURL, tt.opts...) + b, err := OpenBucket(ctx, tt.storageURL, tt.opts...) if err != nil { t.Fatalf("OpenBucket failed: %v", err) } @@ -325,16 +342,43 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { } // The bucket and key stay where the storage URL puts them, so the // request the client makes still names the object. - if want := "/bucket/" + artifactPath; u.Path != want { - t.Errorf("signed URL path = %q, want %q", u.Path, want) - } - if u.Query().Get("X-Amz-Signature") == "" { - t.Errorf("signed URL %q carries no signature", signed) + if u.Path != tt.path { + t.Errorf("signed URL path = %q, want %q", u.Path, tt.path) } + assertSigV4Signature(t, u, "us-1", aws.Credentials{AccessKeyID: "test", SecretAccessKey: "test"}) }) } } +func assertSigV4Signature(t *testing.T, u *url.URL, region string, creds aws.Credentials) { + t.Helper() + + q := u.Query() + signTime, err := time.Parse("20060102T150405Z", q.Get("X-Amz-Date")) + if err != nil { + t.Fatalf("parsing X-Amz-Date from %q: %v", u, err) + } + q.Del("X-Amz-Signature") + unsigned := &url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path, RawQuery: q.Encode()} + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, unsigned.String(), nil) + if err != nil { + t.Fatalf("creating request: %v", err) + } + wantSigned, _, err := awssigv4.NewSigner(func(o *awssigv4.SignerOptions) { + o.DisableURIPathEscaping = true + }).PresignHTTP(context.Background(), creds, req, "UNSIGNED-PAYLOAD", "s3", region, signTime) + if err != nil { + t.Fatalf("PresignHTTP failed: %v", err) + } + wantURL, err := url.Parse(wantSigned) + if err != nil { + t.Fatalf("parsing expected signed URL %q: %v", wantSigned, err) + } + if gotSig, wantSig := u.Query().Get("X-Amz-Signature"), wantURL.Query().Get("X-Amz-Signature"); gotSig != wantSig { + t.Errorf("X-Amz-Signature = %q, want %q (for host %q)", gotSig, wantSig, u.Host) + } +} + func TestAzureSignedURLSignEndpoint(t *testing.T) { t.Setenv("AZURE_STORAGE_SAS_TOKEN", "") t.Setenv("AZURE_STORAGE_ACCOUNT", "devstoreaccount1") From dadc94dd62e47d4a52e6ae49c42b7e2e86ae92d2 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sat, 7 Nov 2026 17:06:44 -0500 Subject: [PATCH 5/8] chore: add some godoc on modified complex funcs --- internal/server/server.go | 1 + internal/server/server_test.go | 2 ++ internal/storage/blob.go | 6 ------ internal/storage/blob_test.go | 7 +++++++ internal/storage/gcs.go | 9 --------- internal/storage/gcs_test.go | 2 ++ 6 files changed, 12 insertions(+), 15 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index 4466d06f..c10d1c5a 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -283,6 +283,7 @@ func (s *Server) configureGitCache(client *http.Client) error { return nil } +// serve wires up HTTP routes, background workers, and serves requests on listener. func (s *Server) serve(listener net.Listener) error { metadataClient, artifactClient := s.newUpstreamClients(s.cfg.ParseArtifactTimeout()) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 93328acc..830fb1ed 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1655,6 +1655,8 @@ func TestNewServer_StorageConnectivityCheck(t *testing.T) { _ = srv.db.Close() } +// TestNewServer_DirectServeBaseURL verifies that New passes DirectServeBaseURL +// to storage.OpenBucket when DirectServe is enabled. func TestNewServer_DirectServeBaseURL(t *testing.T) { tempDir := t.TempDir() awsConfigFile := filepath.Join(tempDir, "aws-config") diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 06685397..930bd5d8 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -303,7 +303,6 @@ func (b *Blob) clearLegacySidecar(key string) { } } -// Store writes the contents of r to path and returns the byte count and hex SHA-256 digest. func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { b.clearLegacySidecar(path) @@ -340,7 +339,6 @@ func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, stri return size, hash, nil } -// Open returns a reader for the object stored at path. func (b *Blob) Open(ctx context.Context, path string) (io.ReadCloser, error) { if localPath := b.localPath(path); localPath != "" { r, err := os.Open(localPath) @@ -368,7 +366,6 @@ func (b *Blob) Open(ctx context.Context, path string) (io.ReadCloser, error) { // otherwise. func (b *Blob) ReadsSeekable() bool { return true } -// Exists reports whether an object exists at path. func (b *Blob) Exists(ctx context.Context, path string) (bool, error) { exists, err := b.bucket.Exists(ctx, path) if err != nil { @@ -439,7 +436,6 @@ func rewriteSignedURLHost(signed, baseURL string) string { return s.String() } -// Size returns the size in bytes of the object at path. func (b *Blob) Size(ctx context.Context, path string) (int64, error) { attrs, err := b.bucket.Attributes(ctx, path) if err != nil { @@ -451,7 +447,6 @@ func (b *Blob) Size(ctx context.Context, path string) (int64, error) { return attrs.Size, nil } -// UsedSpace returns the total bytes stored across all objects in the bucket. func (b *Blob) UsedSpace(ctx context.Context) (int64, error) { var total int64 @@ -510,7 +505,6 @@ func (b *Blob) Close() error { return err } -// URL returns the storage backend URL. func (b *Blob) URL() string { return b.url } diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 233b501c..cac35f62 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -264,6 +264,8 @@ func TestBlobSignedURLUnsupported(t *testing.T) { } } +// TestBlobSignedURLSignEndpoint verifies that S3 presigned URLs are signed for +// the configured public host while preserving the storage URL's path style. func TestBlobSignedURLSignEndpoint(t *testing.T) { awsDir := t.TempDir() awsConfigFile := filepath.Join(awsDir, "config") @@ -350,6 +352,8 @@ func TestBlobSignedURLSignEndpoint(t *testing.T) { } } +// assertSigV4Signature recomputes the SigV4 presigned signature for u and +// verifies it matches u's X-Amz-Signature query parameter. func assertSigV4Signature(t *testing.T, u *url.URL, region string, creds aws.Credentials) { t.Helper() @@ -379,6 +383,8 @@ func assertSigV4Signature(t *testing.T, u *url.URL, region string, creds aws.Cre } } +// TestAzureSignedURLSignEndpoint verifies that Azure presigned URLs have their +// scheme and host rewritten to the configured sign endpoint. func TestAzureSignedURLSignEndpoint(t *testing.T) { t.Setenv("AZURE_STORAGE_SAS_TOKEN", "") t.Setenv("AZURE_STORAGE_ACCOUNT", "devstoreaccount1") @@ -410,6 +416,7 @@ func TestAzureSignedURLSignEndpoint(t *testing.T) { } } +// TestRewriteSignedURLHost verifies scheme and host substitution on signed URLs. func TestRewriteSignedURLHost(t *testing.T) { tests := []struct { name string diff --git a/internal/storage/gcs.go b/internal/storage/gcs.go index c2c98c0b..313caccb 100644 --- a/internal/storage/gcs.go +++ b/internal/storage/gcs.go @@ -27,7 +27,6 @@ func OpenGCS(ctx context.Context, urlStr string) (*GCS, error) { return &GCS{bucket: bucket, url: urlStr}, nil } -// Store writes the contents of r to path and returns the byte count and hex SHA-256 digest. func (g *GCS) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { h := sha256.New() size, err := g.bucket.Write(ctx, path, io.TeeReader(r, h)) @@ -37,7 +36,6 @@ func (g *GCS) Store(ctx context.Context, path string, r io.Reader) (int64, strin return size, hex.EncodeToString(h.Sum(nil)), nil } -// Open returns a reader for the object stored at path. func (g *GCS) Open(ctx context.Context, path string) (io.ReadCloser, error) { r, err := g.bucket.Open(ctx, path) if errors.Is(err, gcstorage.ErrNotFound) { @@ -46,17 +44,14 @@ func (g *GCS) Open(ctx context.Context, path string) (io.ReadCloser, error) { return r, err } -// Exists reports whether an object exists at path. func (g *GCS) Exists(ctx context.Context, path string) (bool, error) { return g.bucket.Exists(ctx, path) } -// Delete removes the object at path. func (g *GCS) Delete(ctx context.Context, path string) error { return g.bucket.Delete(ctx, path) } -// Size returns the size in bytes of the object at path. func (g *GCS) Size(ctx context.Context, path string) (int64, error) { size, err := g.bucket.Size(ctx, path) if errors.Is(err, gcstorage.ErrNotFound) { @@ -78,12 +73,10 @@ func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) return rewriteSignedURLHost(u, g.signEndpoint), nil } -// UsedSpace returns the total bytes stored across all objects in the bucket. func (g *GCS) UsedSpace(ctx context.Context) (int64, error) { return g.bucket.UsedSpace(ctx) } -// ListPrefix returns object metadata for keys under a prefix. func (g *GCS) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, error) { objects, err := g.bucket.ListPrefix(ctx, prefix) if err != nil { @@ -101,12 +94,10 @@ func (g *GCS) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, erro return result, nil } -// Close releases any resources held by the GCS client. func (g *GCS) Close() error { return nil } -// URL returns the storage backend URL. func (g *GCS) URL() string { return g.url } diff --git a/internal/storage/gcs_test.go b/internal/storage/gcs_test.go index 461fb866..92cf3fde 100644 --- a/internal/storage/gcs_test.go +++ b/internal/storage/gcs_test.go @@ -95,6 +95,8 @@ func TestOpenBucketGCSRoundTripWithEmulator(t *testing.T) { } } +// TestGCSSignedURLSignEndpoint verifies that GCS presigned URLs have their +// scheme and host rewritten to the configured sign endpoint. func TestGCSSignedURLSignEndpoint(t *testing.T) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { From 9f400c34af35c5b814486950db5cb22545ee0472 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sat, 7 Nov 2026 17:13:27 -0500 Subject: [PATCH 6/8] fix: compile error in cooldown after merge (remove direct serve URL) --- internal/handler/profile.go | 2 +- internal/handler/profile_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/handler/profile.go b/internal/handler/profile.go index 233e7bbb..52369798 100644 --- a/internal/handler/profile.go +++ b/internal/handler/profile.go @@ -14,7 +14,7 @@ func (p *Proxy) WithCooldown(policy CooldownPolicy, shortest time.Duration) *Pro GradleReadOnly: p.GradleReadOnly, HostedEnabled: p.HostedEnabled, GradleMaxUploadSize: p.GradleMaxUploadSize, NPMFullMetadata: p.NPMFullMetadata, DirectServe: p.DirectServe, DirectServeTTL: p.DirectServeTTL, - DirectServeBaseURL: p.DirectServeBaseURL, HTTPClient: p.HTTPClient, + HTTPClient: p.HTTPClient, RateLimits: p.RateLimits, HostedNPMScopes: p.HostedNPMScopes, HostedComposerVendors: p.HostedComposerVendors, StreamArtifacts: p.StreamArtifacts, Scanners: p.Scanners, ScanSigningKey: p.ScanSigningKey, ScanFetchBaseURL: p.ScanFetchBaseURL, diff --git a/internal/handler/profile_test.go b/internal/handler/profile_test.go index 5caf7b86..070b85bf 100644 --- a/internal/handler/profile_test.go +++ b/internal/handler/profile_test.go @@ -18,6 +18,8 @@ import ( registryclient "github.com/git-pkgs/registries/client" ) +// TestWithCooldownCopiesProxyFields verifies that WithCooldown copies all +// non-isolated Proxy fields onto the returned view. func TestWithCooldownCopiesProxyFields(t *testing.T) { p, _, _, _ := setupTestProxy(t) p.RouteFor = func(string, string) *ArtifactRoute { return nil } @@ -34,7 +36,6 @@ func TestWithCooldownCopiesProxyFields(t *testing.T) { p.NPMFullMetadata = true p.DirectServe = true p.DirectServeTTL = time.Minute - p.DirectServeBaseURL = "https://storage.example.com" p.RateLimits = registryclient.NewRateLimitGate(http.DefaultTransport) p.HostedNPMScopes = []string{"@example"} p.HostedComposerVendors = []string{"example"} From ae0be48b3688a88fec2aa8a6f74d2b1b5d3d918c Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sat, 7 Nov 2026 18:29:31 -0500 Subject: [PATCH 7/8] chore: clean-up/clarity around signing across providers --- internal/storage/blob.go | 103 +++++++++++++++------------------------ internal/storage/gcs.go | 14 ++++-- 2 files changed, 48 insertions(+), 69 deletions(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 930bd5d8..41e12831 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -35,8 +35,6 @@ const ( uploadBufferSize = 5 << 20 uploadConcurrency = 2 - s3Scheme = "s3://" - // attrsExt is fileblob's sidecar suffix, kept only to clear sidecars an // earlier version wrote. attrsExt = ".attrs" @@ -48,13 +46,11 @@ type Blob struct { bucket *blob.Bucket url string - // signBucket, when set, is used by SignedURL to sign S3 presigned URLs for - // a public endpoint rather than the internal storage endpoint. - signBucket *blob.Bucket - - // signEndpoint, when set on non-S3 backends, replaces the scheme and host - // of presigned URLs returned by SignedURL. - signEndpoint string + // postSignRewriteEndpoint, when set, replaces the scheme and host of + // presigned URLs after signing for backends that do not include the host + // in their signature (e.g. Azure). S3 rewrites the host before signing via + // configureS3SignEndpoint so the SigV4 Host signature matches. + postSignRewriteEndpoint string // fileRoot is the directory backing a file:// bucket, empty for cloud // backends. Used only to clear sidecars an earlier version wrote. @@ -101,7 +97,7 @@ func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, er if err != nil { return nil, err } - g.signEndpoint = o.signEndpoint + g.postSignRewriteEndpoint = o.signEndpoint return g, nil } @@ -167,57 +163,50 @@ func OpenBucket(ctx context.Context, urlStr string, opts ...Option) (Storage, er b := &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot} if o.signEndpoint != "" { - if strings.HasPrefix(urlStr, s3Scheme) { - signBucket, err := openSignBucket(ctx, urlStr, o.signEndpoint) - if err != nil { + var s3Client *s3.Client + if bucket.As(&s3Client) { + if err := configureS3SignEndpoint(s3Client, o.signEndpoint); err != nil { _ = bucket.Close() return nil, err } - b.signBucket = signBucket } else { - b.signEndpoint = o.signEndpoint + b.postSignRewriteEndpoint = o.signEndpoint } } return b, nil } -// openSignBucket opens an S3 bucket that signs presigned URLs against the -// scheme and host of signEndpoint while preserving the storage URL's -// addressing mode and object path. -func openSignBucket(ctx context.Context, urlStr, signEndpoint string) (*blob.Bucket, error) { +// configureS3SignEndpoint registers a middleware on s3Client that rewrites +// presigned request URLs to the scheme and host of signEndpoint before SigV4 +// signing, while leaving regular S3 operations and object paths unchanged. +func configureS3SignEndpoint(s3Client *s3.Client, signEndpoint string) error { sign, err := url.Parse(signEndpoint) if err != nil || sign.Scheme == "" || sign.Host == "" { - return nil, fmt.Errorf("invalid sign endpoint %q: must be an absolute URL", signEndpoint) - } - - bucket, err := blob.OpenBucket(ctx, urlStr) - if err != nil { - return nil, fmt.Errorf("opening bucket at sign endpoint %s: %w", signEndpoint, err) + return fmt.Errorf("invalid sign endpoint %q: must be an absolute URL", signEndpoint) } - var s3Client *s3.Client - if bucket.As(&s3Client) { - *s3Client = *s3.New(s3Client.Options(), func(o *s3.Options) { - o.APIOptions = append(o.APIOptions, func(stack *middleware.Stack) error { - return stack.Finalize.Insert(middleware.FinalizeMiddlewareFunc( - "signEndpointHost", - func(ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( - middleware.FinalizeOutput, middleware.Metadata, error, - ) { - if req, ok := in.Request.(*smithyhttp.Request); ok { - req.URL.Scheme = sign.Scheme - req.URL.Host = sign.Host - req.Host = sign.Host - } - return next.HandleFinalize(ctx, in) - }, - ), "PresignHTTPRequest", middleware.Before) - }) + *s3Client = *s3.New(s3Client.Options(), func(o *s3.Options) { + o.APIOptions = append(o.APIOptions, func(stack *middleware.Stack) error { + if _, ok := stack.Finalize.Get("PresignHTTPRequest"); !ok { + return nil + } + return stack.Finalize.Insert(middleware.FinalizeMiddlewareFunc( + "signEndpointHost", + func(ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( + middleware.FinalizeOutput, middleware.Metadata, error, + ) { + if req, ok := in.Request.(*smithyhttp.Request); ok { + req.URL.Scheme = sign.Scheme + req.URL.Host = sign.Host + req.Host = sign.Host + } + return next.HandleFinalize(ctx, in) + }, + ), "PresignHTTPRequest", middleware.Before) }) - } - - return bucket, nil + }) + return nil } // legacySidecarPath gives the ".attrs" path an earlier version wrote for key, @@ -392,15 +381,8 @@ func (b *Blob) Delete(ctx context.Context, path string) error { } // SignedURL returns a presigned URL granting time-limited GET access to path. -// When signBucket is configured (S3 with WithSignEndpoint), it signs against -// the public endpoint; when signEndpoint is set on other backends (e.g. Azure), -// it rewrites the signed URL's scheme and host. func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { - bucket := b.bucket - if b.signBucket != nil { - bucket = b.signBucket - } - signed, err := bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ + signed, err := b.bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ Method: http.MethodGet, Expiry: expiry, }) @@ -410,8 +392,8 @@ func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) } return "", fmt.Errorf("signing URL: %w", err) } - if b.signBucket == nil && b.signEndpoint != "" { - signed = rewriteSignedURLHost(signed, b.signEndpoint) + if b.postSignRewriteEndpoint != "" { + signed = rewriteSignedURLHost(signed, b.postSignRewriteEndpoint) } return signed, nil } @@ -494,15 +476,8 @@ func (b *Blob) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, err return objects, nil } -// Close closes the underlying storage bucket and any separate signing bucket. func (b *Blob) Close() error { - err := b.bucket.Close() - if b.signBucket != nil { - if signErr := b.signBucket.Close(); err == nil { - err = signErr - } - } - return err + return b.bucket.Close() } func (b *Blob) URL() string { diff --git a/internal/storage/gcs.go b/internal/storage/gcs.go index 313caccb..675a1015 100644 --- a/internal/storage/gcs.go +++ b/internal/storage/gcs.go @@ -13,9 +13,13 @@ import ( // GCS adapts a Google Cloud Storage bucket to Storage. type GCS struct { - bucket *gcstorage.Bucket - url string - signEndpoint string + bucket *gcstorage.Bucket + url string + + // postSignRewriteEndpoint replaces the scheme and host of presigned URLs + // after signing (GCS signs against storage.googleapis.com and does not + // include the rewritten host in its V2 signature). + postSignRewriteEndpoint string } // OpenGCS opens a Google Cloud Storage bucket from a gs:// URL. @@ -61,7 +65,7 @@ func (g *GCS) Size(ctx context.Context, path string) (int64, error) { } // SignedURL returns a presigned URL granting time-limited GET access to path, -// rewriting the scheme and host to signEndpoint when configured. +// rewriting the scheme and host to postSignRewriteEndpoint when configured. func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { u, err := g.bucket.SignedURL(ctx, path, expiry) if errors.Is(err, gcstorage.ErrSignedURLUnsupported) { @@ -70,7 +74,7 @@ func (g *GCS) SignedURL(ctx context.Context, path string, expiry time.Duration) if err != nil { return "", err } - return rewriteSignedURLHost(u, g.signEndpoint), nil + return rewriteSignedURLHost(u, g.postSignRewriteEndpoint), nil } func (g *GCS) UsedSpace(ctx context.Context) (int64, error) { From bb6fb2bb372055b39a2cf3f9583a9e19b0edf736 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Tue, 10 Nov 2026 16:26:30 +0000 Subject: [PATCH 8/8] Document public-host signing for direct_serve_base_url Also marks the AWS modules as direct dependencies and drops comments that restate function names. --- config.example.yaml | 8 +++++++- go.mod | 6 +++--- internal/config/config.go | 8 ++++---- internal/handler/profile_test.go | 2 -- internal/server/server.go | 1 - internal/server/server_test.go | 2 -- internal/storage/blob_test.go | 5 ----- internal/storage/gcs_test.go | 2 -- 8 files changed, 14 insertions(+), 20 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index 56c02f7d..98e9b908 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -135,7 +135,13 @@ storage: # Public base URL for presigned URLs. Set this when the proxy reaches # storage at an internal address (127.0.0.1, a Docker service name, or - # VPC DNS) but clients must use a public hostname or CDN. + # VPC DNS) but clients must use a public hostname. Only scheme and host + # are used. For S3/MinIO the URL is signed for this host, so the server + # there must receive that Host header: a reverse proxy or CDN that sends + # the internal or bucket host to the origin fails signature validation. + # With virtual-hosted addressing (no use_path_style) the URL carries no + # bucket name, so this must be the bucket's own hostname. For GCS and + # Azure the host is replaced after signing. # direct_serve_base_url: "https://minio.example.com" # Database configuration diff --git a/go.mod b/go.mod index dce58411..f1cb4f7a 100644 --- a/go.mod +++ b/go.mod @@ -5,8 +5,11 @@ go 1.27.3 require ( github.com/BurntSushi/toml v1.6.0 github.com/Masterminds/semver/v3 v3.5.0 + github.com/aws/aws-sdk-go-v2 v1.47.5 github.com/aws/aws-sdk-go-v2/config v1.33.12 github.com/aws/aws-sdk-go-v2/service/ecr v1.67.1 + github.com/aws/aws-sdk-go-v2/service/s3 v1.111.0 + github.com/aws/smithy-go v1.28.7 github.com/git-pkgs/archives v0.10.0 github.com/git-pkgs/artifacts v0.2.1 github.com/git-pkgs/cooldown v0.3.1 @@ -51,7 +54,6 @@ require ( github.com/PuerkitoBio/purell v1.1.1 // indirect github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect github.com/apapsch/go-jsonmerge/v2 v2.0.0 // indirect - github.com/aws/aws-sdk-go-v2 v1.47.5 // indirect github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.20 // indirect github.com/aws/aws-sdk-go-v2/credentials v1.20.12 // indirect github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.20.5 // indirect @@ -63,12 +65,10 @@ require ( github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.11.2 // indirect github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.14.8 // indirect github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.20.2 // indirect - github.com/aws/aws-sdk-go-v2/service/s3 v1.111.0 // indirect github.com/aws/aws-sdk-go-v2/service/signin v1.12.0 // indirect github.com/aws/aws-sdk-go-v2/service/sso v1.39.0 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.44.0 // indirect github.com/aws/aws-sdk-go-v2/service/sts v1.51.6 // indirect - github.com/aws/smithy-go v1.28.7 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/cenk/backoff v2.2.1+incompatible // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect diff --git a/internal/config/config.go b/internal/config/config.go index 6f8171fa..ddfbb938 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -519,10 +519,10 @@ type StorageConfig struct { // Uses Go duration syntax (e.g. "5m", "1h"). Default: "15m". DirectServeTTL string `json:"direct_serve_ttl" yaml:"direct_serve_ttl"` - // DirectServeBaseURL overrides the scheme and host of presigned URLs - // before returning them to clients. Useful when the proxy reaches - // storage at an internal address (e.g. 127.0.0.1 or a Docker hostname) - // but clients must use a public one. + // DirectServeBaseURL sets the scheme and host of presigned URLs. Useful + // when the proxy reaches storage at an internal address (e.g. 127.0.0.1 + // or a Docker hostname) but clients must use a public one. S3 URLs are + // signed for this host; GCS and Azure URLs have it replaced after signing. DirectServeBaseURL string `json:"direct_serve_base_url" yaml:"direct_serve_base_url"` // CacheArtifacts stores fetched artifacts so later downloads are served diff --git a/internal/handler/profile_test.go b/internal/handler/profile_test.go index baa179d5..a85d6ca3 100644 --- a/internal/handler/profile_test.go +++ b/internal/handler/profile_test.go @@ -18,8 +18,6 @@ import ( registryclient "github.com/git-pkgs/registries/client" ) -// TestWithCooldownCopiesProxyFields verifies that WithCooldown copies all -// non-isolated Proxy fields onto the returned view. func TestWithCooldownCopiesProxyFields(t *testing.T) { p, _, _, _ := setupTestProxy(t) p.RouteFor = func(string, string) *ArtifactRoute { return nil } diff --git a/internal/server/server.go b/internal/server/server.go index fedf11be..59717d44 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -283,7 +283,6 @@ func (s *Server) configureGitCache(client *http.Client) error { return nil } -// serve wires up HTTP routes, background workers, and serves requests on listener. func (s *Server) serve(listener net.Listener) error { metadataClient, artifactClient := s.newUpstreamClients(s.cfg.ParseArtifactTimeout()) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index c1e5b4a2..d631b9a8 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1740,8 +1740,6 @@ func TestNewServer_StorageConnectivityCheck(t *testing.T) { _ = srv.db.Close() } -// TestNewServer_DirectServeBaseURL verifies that New passes DirectServeBaseURL -// to storage.OpenBucket when DirectServe is enabled. func TestNewServer_DirectServeBaseURL(t *testing.T) { tempDir := t.TempDir() awsConfigFile := filepath.Join(tempDir, "aws-config") diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index dd357b0d..debb16d5 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -266,8 +266,6 @@ func TestBlobSignedURLUnsupported(t *testing.T) { } } -// TestBlobSignedURLSignEndpoint verifies that S3 presigned URLs are signed for -// the configured public host while preserving the storage URL's path style. func TestBlobSignedURLSignEndpoint(t *testing.T) { awsDir := t.TempDir() awsConfigFile := filepath.Join(awsDir, "config") @@ -385,8 +383,6 @@ func assertSigV4Signature(t *testing.T, u *url.URL, region string, creds aws.Cre } } -// TestAzureSignedURLSignEndpoint verifies that Azure presigned URLs have their -// scheme and host rewritten to the configured sign endpoint. func TestAzureSignedURLSignEndpoint(t *testing.T) { t.Setenv("AZURE_STORAGE_SAS_TOKEN", "") t.Setenv("AZURE_STORAGE_ACCOUNT", "devstoreaccount1") @@ -418,7 +414,6 @@ func TestAzureSignedURLSignEndpoint(t *testing.T) { } } -// TestRewriteSignedURLHost verifies scheme and host substitution on signed URLs. func TestRewriteSignedURLHost(t *testing.T) { tests := []struct { name string diff --git a/internal/storage/gcs_test.go b/internal/storage/gcs_test.go index 92cf3fde..461fb866 100644 --- a/internal/storage/gcs_test.go +++ b/internal/storage/gcs_test.go @@ -95,8 +95,6 @@ func TestOpenBucketGCSRoundTripWithEmulator(t *testing.T) { } } -// TestGCSSignedURLSignEndpoint verifies that GCS presigned URLs have their -// scheme and host rewritten to the configured sign endpoint. func TestGCSSignedURLSignEndpoint(t *testing.T) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil {