diff --git a/TODO.md b/TODO.md index 2f56525..f02b7c0 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,27 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-26 the image request holds only what names the cached object (closes + #314): its source and its transform, which the variant key covers. The signed + URL format, the signing algorithm and every route's answers are unchanged. + - The `/v1/image/` handler checks `allowlist_hosts` and verifies the + signature itself, with `Signer.Verify(req, sig, expires)`. + - Each image handler keeps its URL's expiry as a value of its own, for + `Cache-Control`. `encurl`'s `Generate` takes the expiry and its `Parse` + returns it. + - The fetcher takes the source and builds its URL with `Source.URL`, over + http only when the fetcher's own `AllowHTTP` is set. `validateURL` takes + that URL, so no URL string is built and parsed again. + - `chooseAutoFormat` returns the format it chooses, and the handler puts it + into the request it parsed. + - The image service serves images through `Get`, and has `WaitForProcessing` + for shutdown. + - Removed: + - `Signature`, `Expires`, `AllowHTTP` and `SourceURL` of the image + request + - `Service.ValidateRequest`, and the service's signer, allowlist and + `AllowHTTP` + - `extractHost` of the fetcher and `urlToFSPath` of the mock fetcher - 2026-10-26 pixad's fx graph builds the image pipeline (closes #313). `database.New` now opens the database, so the parts below can read it while they are built; its stop hook still runs after the cache's. The cache's diff --git a/internal/encurl/encurl.go b/internal/encurl/encurl.go index e754ec9..2f06726 100644 --- a/internal/encurl/encurl.go +++ b/internal/encurl/encurl.go @@ -59,12 +59,14 @@ func NewGenerator(signingKey string) (*Generator, error) { return &Generator{key: key}, nil } -// Generate creates an encrypted URL token from the request's source, -// transform and expiry, each as the request holds it; an empty format, a -// quality of 0 or an empty fit mode is left out of the token, and a zero -// Expires makes a token that never expires. The token is CBOR-encoded, +// Generate creates an encrypted URL token from the request's source and +// transform, each as the request holds it, and from expires; an empty format, +// a quality of 0 or an empty fit mode is left out of the token, and a zero +// expires makes a token that never expires. The token is CBOR-encoded, // encrypted with NaCl secretbox, and base64url-encoded. -func (g *Generator) Generate(req *imagerequest.Request) (string, error) { +func (g *Generator) Generate( + req *imagerequest.Request, expires time.Time, +) (string, error) { p := &payload{ SourceHost: req.Source.Host, SourcePath: req.Source.Path, @@ -76,8 +78,8 @@ func (g *Generator) Generate(req *imagerequest.Request) (string, error) { FitMode: req.Transform.FitMode, } - if !req.Expires.IsZero() { - p.ExpiresAt = req.Expires.Unix() + if !expires.IsZero() { + p.ExpiresAt = expires.Unix() } // CBOR encode the payload @@ -91,19 +93,19 @@ func (g *Generator) Generate(req *imagerequest.Request) (string, error) { } // Parse decrypts and validates an encrypted URL token and returns the -// request it holds, with default values for omitted optional fields. A token -// that never expires gives the zero Expires. +// request it holds, with default values for omitted optional fields, and its +// expiry. A token that never expires gives the zero time. // Returns ErrExpired if the token has expired, or other errors for invalid tokens. -func (g *Generator) Parse(token string) (*imagerequest.Request, error) { +func (g *Generator) Parse(token string) (*imagerequest.Request, time.Time, error) { // Decrypt data, err := seal.Decrypt(g.key, token) if err != nil { if errors.Is(err, seal.ErrDecryptionFailed) || errors.Is(err, seal.ErrInvalidPayload) { - return nil, ErrDecryptFailed + return nil, time.Time{}, ErrDecryptFailed } - return nil, err + return nil, time.Time{}, err } // CBOR decode @@ -111,12 +113,12 @@ func (g *Generator) Parse(token string) (*imagerequest.Request, error) { err = cbor.Unmarshal(data, &p) if err != nil { - return nil, ErrInvalidFormat + return nil, time.Time{}, ErrInvalidFormat } // Check expiration (0 = never expires) if p.ExpiresAt != 0 && time.Now().Unix() > p.ExpiresAt { - return nil, ErrExpired + return nil, time.Time{}, ErrExpired } format := p.Format @@ -151,6 +153,5 @@ func (g *Generator) Parse(token string) (*imagerequest.Request, error) { Quality: quality, FitMode: fitMode, }, - Expires: expires, - }, nil + }, expires, nil } diff --git a/internal/encurl/encurl_test.go b/internal/encurl/encurl_test.go index 338f678..9ce3100 100644 --- a/internal/encurl/encurl_test.go +++ b/internal/encurl/encurl_test.go @@ -36,10 +36,10 @@ func TestGenerator_GenerateAndParse(t *testing.T) { Quality: 90, FitMode: imagerequest.FitContain, }, - Expires: time.Now().Add(time.Hour), } + expires := time.Now().Add(time.Hour) - token, err := gen.Generate(req) + token, err := gen.Generate(req, expires) if err != nil { t.Fatalf("Generate() error = %v", err) } @@ -48,7 +48,7 @@ func TestGenerator_GenerateAndParse(t *testing.T) { t.Fatal("Generate() returned empty token") } - parsed, err := gen.Parse(token) + parsed, parsedExpires, err := gen.Parse(token) if err != nil { t.Fatalf("Parse() error = %v", err) } @@ -91,8 +91,8 @@ func TestGenerator_GenerateAndParse(t *testing.T) { parsed.Transform.FitMode, req.Transform.FitMode) } - if parsed.Expires.Unix() != req.Expires.Unix() { - t.Errorf("ExpiresAt = %d, want %d", parsed.Expires.Unix(), req.Expires.Unix()) + if parsedExpires.Unix() != expires.Unix() { + t.Errorf("ExpiresAt = %d, want %d", parsedExpires.Unix(), expires.Unix()) } } @@ -106,15 +106,14 @@ func TestGenerator_Parse_Expired(t *testing.T) { Host: testSourceHost, Path: testSourcePath, }, - Expires: time.Now().Add(-time.Hour), // Already expired } - token, err := gen.Generate(req) + token, err := gen.Generate(req, time.Now().Add(-time.Hour)) // Already expired if err != nil { t.Fatalf("Generate() error = %v", err) } - _, err = gen.Parse(token) + _, _, err = gen.Parse(token) if err == nil { t.Error("Parse() should fail for expired token") } @@ -129,7 +128,7 @@ func TestGenerator_Parse_InvalidToken(t *testing.T) { gen, _ := encurl.NewGenerator("test-signing-key-12345") - _, err := gen.Parse("not-a-valid-token") + _, _, err := gen.Parse("not-a-valid-token") if err == nil { t.Error("Parse() should fail for invalid token") } @@ -145,10 +144,9 @@ func TestGenerator_Parse_TamperedToken(t *testing.T) { Host: testSourceHost, Path: testSourcePath, }, - Expires: time.Now().Add(time.Hour), } - token, _ := gen.Generate(req) + token, _ := gen.Generate(req, time.Now().Add(time.Hour)) // Tamper with the token tampered := []byte(token) @@ -156,7 +154,7 @@ func TestGenerator_Parse_TamperedToken(t *testing.T) { tampered[10] ^= 0x01 } - _, err := gen.Parse(string(tampered)) + _, _, err := gen.Parse(string(tampered)) if err == nil { t.Error("Parse() should fail for tampered token") } @@ -173,12 +171,11 @@ func TestGenerator_Parse_WrongKey(t *testing.T) { Host: testSourceHost, Path: testSourcePath, }, - Expires: time.Now().Add(time.Hour), } - token, _ := gen1.Generate(req) + token, _ := gen1.Generate(req, time.Now().Add(time.Hour)) - _, err := gen2.Parse(token) + _, _, err := gen2.Parse(token) if err == nil { t.Error("Parse() should fail with different signing key") } @@ -195,13 +192,12 @@ func TestGenerator_Parse_Defaults(t *testing.T) { Host: testSourceHost, Path: testSourcePath, }, - Expires: time.Now().Add(time.Hour), - }) + }, time.Now().Add(time.Hour)) if err != nil { t.Fatalf("Generate() error = %v", err) } - req, err := gen.Parse(token) + req, _, err := gen.Parse(token) if err != nil { t.Fatalf("Parse() error = %v", err) } @@ -232,10 +228,9 @@ func TestGenerator_TokenIsURLSafe(t *testing.T) { Host: testSourceHost, Path: testSourcePath, }, - Expires: time.Now().Add(time.Hour), } - token, _ := gen.Generate(req) + token, _ := gen.Generate(req, time.Now().Add(time.Hour)) // Check that token only contains URL-safe characters for _, c := range token { diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index a5a6702..1dda36d 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -153,20 +153,20 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { return } - // The image URL must fetch the source URL as it was written - req.AllowHTTP = s.config.AllowHTTP - fetchURL := req.SourceURL() + // The image URL must fetch the source URL as it was written. The + // fetcher fetches over http only when allow_http is set. + fetchURL := req.Source.URL(s.config.AllowHTTP) if !fetchedAsWritten(parsed, fetchURL) { s.renderGeneratorWithForm(w, r, "Source URL cannot be fetched as "+ - "written: pixa would fetch "+fetchURL, + "written: pixa would fetch "+fetchURL.String(), r.Form, http.StatusBadRequest) return } // Generate encrypted token - token, err := s.encGen.Generate(req) + token, err := s.encGen.Generate(req, expiresAt) if err != nil { s.log.Error("failed to generate encrypted URL", "error", err) s.renderGeneratorWithForm(w, r, "Failed to generate URL", r.Form, @@ -226,25 +226,23 @@ func parseSourceURL(sourceURL string) (*url.URL, error) { // in RawPath when it is not the decoded path percent-encoded again. // EscapedPath ignores RawPath when it also holds a character net/url encodes, // such as a space, so RawPath is compared itself. -func fetchedAsWritten(source *url.URL, fetchURL string) bool { +func fetchedAsWritten(source, fetchURL *url.URL) bool { writtenPath := source.RawPath if writtenPath == "" { writtenPath = source.EscapedPath() } - fetched, err := url.Parse(fetchURL) - - return err == nil && source.User == nil && !source.ForceQuery && - fetched.EscapedPath() == writtenPath && - fetched.RawQuery == source.RawQuery + return source.User == nil && !source.ForceQuery && + fetchURL.EscapedPath() == writtenPath && + fetchURL.RawQuery == source.RawQuery } // buildGenerateRequest parses the numeric form fields and assembles the -// image request the encrypted URL's token is made from. ttl=0 means never -// expires (Expires stays the zero time). A non-numeric or out-of-range -// width, height, quality or ttl, or an unrecognized fit mode, is a client -// error naming the offending field. The format field is passed through -// unchecked. +// image request the encrypted URL's token is made from, and its expiry. +// ttl=0 means never expires (the expiry is the zero time). A non-numeric or +// out-of-range width, height, quality or ttl, or an unrecognized fit mode, is +// a client error naming the offending field. The format field is passed +// through unchecked. func buildGenerateRequest( parsed *url.URL, form url.Values, ) (*imagerequest.Request, time.Time, int, error) { @@ -294,7 +292,6 @@ func buildGenerateRequest( Quality: quality, FitMode: fitMode, }, - Expires: expiresAt, } return req, expiresAt, ttl, nil diff --git a/internal/handlers/cache_hit_internal_test.go b/internal/handlers/cache_hit_internal_test.go index 56d1dda..bbc16e1 100644 --- a/internal/handlers/cache_hit_internal_test.go +++ b/internal/handlers/cache_hit_internal_test.go @@ -19,11 +19,13 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" ) // countingHost is the allowlisted host whose photoPath the server of @@ -137,8 +139,6 @@ func newCountingDatabaseServer( MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: testSigningKey, - Allowlist: []string{countingHost}, MaxResponseSize: 50 << 20, Logger: slog.New(slog.DiscardHandler), }) @@ -159,15 +159,17 @@ func newCountingDatabaseServer( Quality: 85, FitMode: imagerequest.FitCover, }, - }) + }, time.Time{}) if err != nil { tb.Fatalf("encGen.Generate() error = %v", err) } h := &Handlers{ - log: slog.New(slog.DiscardHandler), - imgSvc: svc, - encGen: encGen, + log: slog.New(slog.DiscardHandler), + imgSvc: svc, + encGen: encGen, + signer: signature.New(testSigningKey), + allowlist: allowlist.New([]string{countingHost}), } r := chi.NewRouter() diff --git a/internal/handlers/first_bytes_internal_test.go b/internal/handlers/first_bytes_internal_test.go index 4a31a7b..69a3751 100644 --- a/internal/handlers/first_bytes_internal_test.go +++ b/internal/handlers/first_bytes_internal_test.go @@ -13,11 +13,13 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" ) // mismatchedSourcePath is the path of the source that @@ -27,8 +29,9 @@ import ( const mismatchedSourcePath = "/images/source.png" // newRoutesFetchingThrough returns both image routes, and the Handlers behind -// them, whose image service fetches through fetcher and has allowlistedHost on -// its allowlist. The handlers and the image service log to log. +// them, whose image service fetches through fetcher, and which have +// allowlistedHost on their allowlist. The handlers and the image service log +// to log. func newRoutesFetchingThrough( t *testing.T, fetcher httpfetcher.Fetcher, log *slog.Logger, ) (*Handlers, http.Handler) { @@ -50,8 +53,6 @@ func newRoutesFetchingThrough( MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: testSigningKey, - Allowlist: []string{allowlistedHost}, MaxResponseSize: 50 << 20, Logger: log, }) @@ -64,7 +65,13 @@ func newRoutesFetchingThrough( t.Fatalf("encurl.NewGenerator() error = %v", err) } - h := &Handlers{log: log, imgSvc: svc, encGen: encGen} + h := &Handlers{ + log: log, + imgSvc: svc, + encGen: encGen, + signer: signature.New(testSigningKey), + allowlist: allowlist.New([]string{allowlistedHost}), + } r := chi.NewRouter() r.Get("/v1/image/*", h.HandleImage()) @@ -93,7 +100,7 @@ func mismatchedSourceEncURL(t *testing.T, h *Handlers) string { Size: imagerequest.Size{Width: 50, Height: 50}, Format: imagerequest.FormatJPEG, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/format_auto.go b/internal/handlers/format_auto.go index 0dda8f8..96a1b06 100644 --- a/internal/handlers/format_auto.go +++ b/internal/handlers/format_auto.go @@ -18,16 +18,17 @@ var ( "image/jxl, image/avif, image/webp or image/jpeg") ) -// chooseAutoFormat replaces the format auto in req with the format -// formatForAccept chooses from r's Accept header, and adds Vary: Accept to the +// chooseAutoFormat returns the format to serve for the requested format: for +// the format auto, the format formatForAccept chooses from r's Accept header, +// and any other format as it is. For auto it adds Vary: Accept to the // response, which then depends on that header. It answers 400 for an Accept // header that is not valid and 406 for one that allows none of the formats, -// and reports whether req can be served. Any other format is left as it is. +// and reports whether the image can be served. func (s *Handlers) chooseAutoFormat( - w http.ResponseWriter, r *http.Request, req *imagerequest.Request, -) bool { - if req.Transform.Format != imagerequest.FormatAuto { - return true + w http.ResponseWriter, r *http.Request, requested imagerequest.Format, +) (imagerequest.Format, bool) { + if requested != imagerequest.FormatAuto { + return requested, true } w.Header().Add("Vary", "Accept") @@ -36,18 +37,16 @@ func (s *Handlers) chooseAutoFormat( if errors.Is(err, errNotAcceptable) { s.respondError(w, err.Error(), http.StatusNotAcceptable) - return false + return "", false } if err != nil { s.respondError(w, err.Error(), http.StatusBadRequest) - return false + return "", false } - req.Transform.Format = format - - return true + return format, true } // formatForAccept returns the format an auto URL is served in for the Accept diff --git a/internal/handlers/format_auto_internal_test.go b/internal/handlers/format_auto_internal_test.go index c543902..5a99f1e 100644 --- a/internal/handlers/format_auto_internal_test.go +++ b/internal/handlers/format_auto_internal_test.go @@ -75,9 +75,9 @@ func TestFormatForAccept(t *testing.T) { // autoPhotoURLs returns a signed /v1/image/ URL and an encrypted /v1/e/ URL, // both valid for a minute, for the JPEG at photoPath on signedHost at 50x50 in -// the format auto. The first is signed with testSigningKey, the key of the -// image service newSignedHostServer builds, and the second is made with h's -// generator. +// the format auto. The first is signed with testSigningKey, the key the +// handlers that newSignedHostServer builds check signatures with. The second +// is made with h's generator. func autoPhotoURLs(t *testing.T, h *Handlers) (string, string) { t.Helper() @@ -101,8 +101,7 @@ func autoPhotoURLs(t *testing.T, h *Handlers) (string, string) { Size: imagerequest.Size{Width: 50, Height: 50}, Format: imagerequest.FormatAuto, }, - Expires: time.Now().Add(time.Minute), - }) + }, time.Now().Add(time.Minute)) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 555e9c6..db24c4f 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -16,6 +16,7 @@ import ( "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/logger" "sneak.berlin/go/pixa/internal/session" + "sneak.berlin/go/pixa/internal/signature" ) // Params defines dependencies for Handlers. @@ -41,6 +42,11 @@ type Handlers struct { encGen *encurl.Generator csrfProtect func(http.Handler) http.Handler + // signer checks the signature of a /v1/image/ URL with signing_key. + signer *signature.Signer + // allowlist matches the hosts of allowlist_hosts, which a /v1/image/ + // URL may name without a signature. + allowlist *allowlist.HostAllowList // refererBlocklist matches the hosts of referer_blocklist; its IsAllowed // reports whether a URL's host is on that list. refererBlocklist *allowlist.HostAllowList @@ -61,6 +67,8 @@ func New(_ fx.Lifecycle, params Params) (*Handlers, error) { sessMgr: params.SessionManager, encGen: params.EncryptedURLGenerator, csrfProtect: csrfProtect, + signer: signature.New(params.Config.SigningKey), + allowlist: allowlist.New(params.Config.AllowlistHosts), refererBlocklist: allowlist.New(params.Config.RefererBlocklist), }, nil } diff --git a/internal/handlers/handlers_internal_test.go b/internal/handlers/handlers_internal_test.go index e463dda..9568849 100644 --- a/internal/handlers/handlers_internal_test.go +++ b/internal/handlers/handlers_internal_test.go @@ -19,10 +19,13 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" ) // testFixtures contains test data. @@ -62,8 +65,6 @@ func setupTestHandler(t *testing.T) *testFixtures { MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: "test-signing-key-must-be-32-chars", - Allowlist: []string{goodHost}, MaxResponseSize: 50 << 20, }) if err != nil { @@ -71,8 +72,10 @@ func setupTestHandler(t *testing.T) *testFixtures { } h := &Handlers{ - imgSvc: svc, - log: slog.Default(), + imgSvc: svc, + log: slog.Default(), + signer: signature.New("test-signing-key-must-be-32-chars"), + allowlist: allowlist.New([]string{goodHost}), } return &testFixtures{ @@ -127,10 +130,9 @@ func newMockFetcher(fs fs.FS) *mockFetcher { } func (f *mockFetcher) Fetch( - _ context.Context, url string, + _ context.Context, source imagerequest.Source, ) (*httpfetcher.FetchResult, error) { - // Remove https:// prefix - path := url[8:] // Remove "https://" + path := source.Host + source.Path data, err := fs.ReadFile(f.fs, path) if err != nil { diff --git a/internal/handlers/image.go b/internal/handlers/image.go index b11b0f5..12665e1 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "time" "github.com/go-chi/chi/v5/middleware" @@ -27,13 +28,13 @@ func (s *Handlers) HandleImage() http.HandlerFunc { return } - req, ok := s.parseImageRequest(w, r) + req, sig, expires, ok := s.parseImageRequest(w, r) if !ok { return } // Validate signature if required - err := s.imgSvc.ValidateRequest(req) + err := s.validateSignature(req, sig, expires) if err != nil { s.log.Warn("signature validation failed", "host", req.Source.Host, @@ -46,10 +47,13 @@ func (s *Handlers) HandleImage() http.HandlerFunc { } // The signature covers the format auto, not the format chosen - if !s.chooseAutoFormat(w, r, req) { + format, ok := s.chooseAutoFormat(w, r, req.Transform.Format) + if !ok { return } + req.Transform.Format = format + // Get cache key for logging cacheKey := imagerequest.CacheKey(req) @@ -65,7 +69,7 @@ func (s *Handlers) HandleImage() http.HandlerFunc { defer func() { _ = resp.Content.Close() }() - s.writeImageResponse(w, r, req, resp, cacheKey, startTime) + s.writeImageResponse(w, r, req, expires, resp, cacheKey, startTime) } } @@ -82,11 +86,11 @@ func (s *Handlers) HandleRobotsTxt() http.HandlerFunc { } // parseImageRequest parses the image URL's path and query parameters into -// an image request. On invalid input it writes an error response and -// returns false. +// an image request, and returns it with the URL's signature and expiry. On +// invalid input it writes an error response and returns false. func (s *Handlers) parseImageRequest( w http.ResponseWriter, r *http.Request, -) (*imagerequest.Request, bool) { +) (*imagerequest.Request, string, time.Time, bool) { // The path is parsed decoded once, as r.URL.Path holds it, and not from // chi's wildcard: chi routes on the path as the client wrote it whenever // that is not Go's own encoding of it, so its wildcard would still hold @@ -100,7 +104,7 @@ func (s *Handlers) parseImageRequest( ) s.respondError(w, "invalid image URL: "+err.Error(), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } // Parse signature params from query string. r.URL.Query() would silently @@ -113,7 +117,7 @@ func (s *Handlers) parseImageRequest( s.respondError(w, fmt.Sprintf("invalid query string %q: %v", r.URL.RawQuery, err), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } for name, values := range query { @@ -121,17 +125,15 @@ func (s *Handlers) parseImageRequest( s.respondError(w, fmt.Sprintf("invalid %s: given more than once", name), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } } - req.Signature = query.Get("sig") - - req.Expires, err = parseExpires(query) + expires, err := parseExpires(query) if err != nil { s.respondError(w, err.Error(), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } // Parse optional quality and fit params. Only a q missing from the URL is @@ -143,7 +145,7 @@ func (s *Handlers) parseImageRequest( s.respondError(w, `invalid q: not a number, got ""`, http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } req.Transform.Quality, err = parseFormInt(query, "q", @@ -152,7 +154,7 @@ func (s *Handlers) parseImageRequest( s.respondError(w, fmt.Sprintf("%v, got %q", err, qStr), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } // Only a fit missing from the URL is cover. A fit in the URL that is not a @@ -162,7 +164,7 @@ func (s *Handlers) parseImageRequest( if query.Has("fit") && req.Transform.FitMode == "" { s.respondError(w, `invalid fit: not a fit mode, got ""`, http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } // Default fit mode if not set @@ -179,10 +181,35 @@ func (s *Handlers) parseImageRequest( s.respondError(w, "invalid image request: "+err.Error(), http.StatusBadRequest) - return nil, false + return nil, "", time.Time{}, false } - return req, true + return req, query.Get("sig"), expires, true +} + +// validateSignature returns nil when the source host of req is on +// allowlist_hosts and names no port. Otherwise it verifies sig, the URL's +// signature of req, and expires, the URL's expiry. +func (s *Handlers) validateSignature( + req *imagerequest.Request, sig string, expires time.Time, +) error { + // An allowlist entry names no port, so a host followed by ":", with or + // without a port, needs a signature. Go's URL type reads the port as the + // fetcher does: in an IPv6 address written without brackets, such as + // 2001:db8::1:8443, it is what follows the last ":". This is checked here + // and not in allowlist.IsAllowed, which also matches referer_blocklist, + // where a Referer must be blocked whatever its port. Port and IsAllowed + // read only the host of a URL. + sourceURL := &url.URL{Host: req.Source.Host} + hostHasPort := sourceURL.Port() != "" || + strings.HasSuffix(sourceURL.Host, ":") + + if !hostHasPort && s.allowlist.IsAllowed(sourceURL) { + return nil + } + + // Signature required for non-allowed hosts + return s.signer.Verify(req, sig, expires) } // parseExpires reads the exp query parameter, a Unix time in seconds. An exp @@ -341,10 +368,11 @@ func notModified(w http.ResponseWriter, r *http.Request, etag string) bool { } // writeImageResponse writes headers and streams the image content, -// handling conditional and HEAD requests. +// handling conditional and HEAD requests. expires is the URL's expiry, which +// ends the Cache-Control max-age. func (s *Handlers) writeImageResponse( w http.ResponseWriter, r *http.Request, - req *imagerequest.Request, resp *imgcache.ImageResponse, + req *imagerequest.Request, expires time.Time, resp *imgcache.ImageResponse, cacheKey imagerequest.VariantKey, startTime time.Time, ) { // Set response headers @@ -355,7 +383,7 @@ func (s *Handlers) writeImageResponse( } // Cache control headers - w.Header().Set("Cache-Control", cacheControl(req.Expires)) + w.Header().Set("Cache-Control", cacheControl(expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) if notModified(w, r, resp.ETag) { diff --git a/internal/handlers/image_cache_control_internal_test.go b/internal/handlers/image_cache_control_internal_test.go index b27465d..7059ac7 100644 --- a/internal/handlers/image_cache_control_internal_test.go +++ b/internal/handlers/image_cache_control_internal_test.go @@ -14,10 +14,12 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" ) // photoPath is the path of the JPEG that newSignedHostServer serves. @@ -52,7 +54,6 @@ func newSignedHostServer( MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: testSigningKey, MaxResponseSize: 50 << 20, Logger: log, }) @@ -66,9 +67,11 @@ func newSignedHostServer( } h := &Handlers{ - log: log, - imgSvc: svc, - encGen: encGen, + log: log, + imgSvc: svc, + encGen: encGen, + signer: signature.New(testSigningKey), + allowlist: allowlist.New(nil), } r := chi.NewRouter() @@ -202,8 +205,7 @@ func TestHandleImageEnc_MaxAge(t *testing.T) { Size: imagerequest.Size{Width: 50, Height: 50}, Format: imagerequest.FormatJPEG, }, - Expires: tt.expires, - }) + }, tt.expires) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index e4143ef..0591044 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -14,6 +14,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" @@ -26,7 +27,7 @@ import ( const allowlistedHost = "allowed.example.com" // newImageRoute returns the image route of a Handlers whose service fetches -// with fetcher and checks signatures with testSigningKey. +// with fetcher, and which checks signatures with testSigningKey. func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { t.Helper() @@ -46,15 +47,18 @@ func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: testSigningKey, - Allowlist: []string{allowlistedHost}, MaxResponseSize: 50 << 20, }) if err != nil { t.Fatalf("failed to create service: %v", err) } - h := &Handlers{imgSvc: svc, log: slog.New(slog.DiscardHandler)} + h := &Handlers{ + imgSvc: svc, + log: slog.New(slog.DiscardHandler), + signer: signature.New(testSigningKey), + allowlist: allowlist.New([]string{allowlistedHost}), + } r := chi.NewRouter() r.Get("/v1/image/*", h.HandleImage()) @@ -63,7 +67,7 @@ func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { } // newPhotoFetcher returns a mock fetcher that serves a JPEG at photoPath on -// each of hosts, and answers any other URL with an upstream error. +// each of hosts, and answers any other source with an upstream error. func newPhotoFetcher(t *testing.T, hosts ...string) *httpfetcher.MockFetcher { t.Helper() @@ -90,10 +94,10 @@ func photoURLWithSig(host, sig string, expires time.Time) string { return fmt.Sprintf("%s?sig=%s&exp=%d", photoURL(host), sig, expires.Unix()) } -// photoSignature returns the signature of photoURL(host) at the default -// quality and fit, made with key and expiring at expires. -func photoSignature(key, host string, expires time.Time) string { - return signature.New(key).Sign(&imagerequest.Request{ +// photoRequest returns the image request of photoURL(host) at the default +// quality and fit. +func photoRequest(host string) *imagerequest.Request { + return &imagerequest.Request{ Source: imagerequest.Source{ Host: host, Path: photoPath, @@ -104,8 +108,13 @@ func photoSignature(key, host string, expires time.Time) string { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + } +} + +// photoSignature returns the signature of photoURL(host) at the default +// quality and fit, made with key and expiring at expires. +func photoSignature(key, host string, expires time.Time) string { + return signature.New(key).Sign(photoRequest(host), expires) } // sendGet sends a GET for target to route and returns the response. @@ -286,10 +295,9 @@ func TestHandleImage_AllowlistOrSignature(t *testing.T) { // host can be, and checks that each is refused with 400 naming the host as // decoded: allowlistedHost with a character after it that a URL host cannot // hold, percent-encoded, an IPv6 address without brackets, and brackets -// around something that is not an IPv6 address. A "?" or "#" after -// allowlistedHost would otherwise end the host when the source URL is read, -// so the request would pass the allowlist and be fetched and cached under a -// host that is not the one fetched. +// around something that is not an IPv6 address. Later steps put the host +// unparsed into the allowlist check, the cache key and the upstream URL, so +// such a host is refused first. func TestHandleImage_InvalidHost_Returns400(t *testing.T) { t.Parallel() @@ -360,10 +368,9 @@ func TestHandleImage_HostForms_Served(t *testing.T) { // TestHandleImage_HashInSourcePathOrQuery_Returns400 sends image URLs whose // source path or source query holds a "#", written as %23, and checks that -// each is refused with 400, the one signed for that source path included. The -// source URL would otherwise end at the "#" when it is fetched, so each text -// after it would fetch the image at photoPath and cache it under keys of its -// own. +// each is refused with 400, the one signed for that source path included. +// A "#" starts a URL's fragment, so neither the source path nor the source +// query may hold one. func TestHandleImage_HashInSourcePathOrQuery_Returns400(t *testing.T) { t.Parallel() @@ -381,8 +388,7 @@ func TestHandleImage_HashInSourcePathOrQuery_Returns400(t *testing.T) { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: exp, - }) + }, exp) tests := []struct { name string diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index 3d2b766..eaed6ee 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -38,40 +38,44 @@ func getImage(t *testing.T, fix *testFixtures, target string) int { } // generateSignedURL returns the /v1/image/ URL of req, signed with key and -// expiring after ttl, and sets req's expiry. An unset quality or fit of req is -// first set to 85 or cover, the values the route uses when a URL has no q or -// fit. The URL carries q and fit next to sig and exp, so the route verifies it -// for the quality and fit it was signed with. +// expiring after ttl. It signs a copy of req, so req itself is not changed. +// An unset quality or fit in the copy is set to 85 or cover, the values the +// route uses when a URL has no q or fit. The URL carries q and fit next to sig +// and exp, so the route verifies it for the quality and fit it was signed +// with. func generateSignedURL( key string, req *imagerequest.Request, ttl time.Duration, ) string { - if req.Transform.Quality == 0 { - req.Transform.Quality = 85 + signed := *req + + if signed.Transform.Quality == 0 { + signed.Transform.Quality = 85 } - if req.Transform.FitMode == "" { - req.Transform.FitMode = imagerequest.FitCover + if signed.Transform.FitMode == "" { + signed.Transform.FitMode = imagerequest.FitCover } - req.Expires = time.Now().Add(ttl) - sig := signature.New(key).Sign(req) + expires := time.Now().Add(ttl) + sig := signature.New(key).Sign(&signed, expires) // A source query follows the source path after a "?". The image route // reads the path decoded once, so the whole path is written in Go's own // percent-encoding of a URL path, which writes "?" as %3F and "%" as %25, // and leaves "/", ";", ",", "=" and "&" as they are. - sourcePathAndQuery := req.Source.Path - if req.Source.Query != "" { - sourcePathAndQuery += "?" + req.Source.Query + sourcePathAndQuery := signed.Source.Path + if signed.Source.Query != "" { + sourcePathAndQuery += "?" + signed.Source.Query } unescapedPath := fmt.Sprintf("/v1/image/%s%s/%dx%d.%s", - req.Source.Host, sourcePathAndQuery, - req.Transform.Size.Width, req.Transform.Size.Height, req.Transform.Format) + signed.Source.Host, sourcePathAndQuery, + signed.Transform.Size.Width, signed.Transform.Size.Height, + signed.Transform.Format) return fmt.Sprintf("%s?sig=%s&exp=%d&q=%d&fit=%s", - (&url.URL{Path: unescapedPath}).EscapedPath(), sig, req.Expires.Unix(), - req.Transform.Quality, req.Transform.FitMode) + (&url.URL{Path: unescapedPath}).EscapedPath(), sig, expires.Unix(), + signed.Transform.Quality, signed.Transform.FitMode) } // TestHandleImage_SignatureCoversQualityAndFit signs a URL for quality 85 @@ -93,8 +97,7 @@ func TestHandleImage_SignatureCoversQualityAndFit(t *testing.T) { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + }, expires) signedURL := fmt.Sprintf("/v1/image/%s/images/photo.jpg/50x50.jpeg?sig=%s&exp=%d", signedHost, sig, expires.Unix()) @@ -277,8 +280,7 @@ func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + }, expires) tests := []struct { name string @@ -344,8 +346,7 @@ func TestHandleImage_SourceURLHoldingPercent25Verifies(t *testing.T) { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + }, expires) target := fmt.Sprintf("/v1/image/%s%s/50x50.jpeg?sig=%s&exp=%d", signedHost, tt.writtenPathAndQuery, sig, expires.Unix()) diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index e5a94d4..e823498 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -31,11 +31,18 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { ctx := r.Context() start := time.Now() - req, ok := s.parseImageEncRequest(w, r) - if !ok || !s.chooseAutoFormat(w, r, req) { + req, expires, ok := s.parseImageEncRequest(w, r) + if !ok { return } + format, ok := s.chooseAutoFormat(w, r, req.Transform.Format) + if !ok { + return + } + + req.Transform.Format = format + // Log the request s.log.Debug("encrypted image request", "host", req.Source.Host, @@ -64,7 +71,7 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { } // Cache headers: max-age ends at the URL's expiry - w.Header().Set("Cache-Control", cacheControl(req.Expires)) + w.Header().Set("Cache-Control", cacheControl(expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) if notModified(w, r, resp.ETag) { @@ -102,34 +109,34 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { } // parseImageEncRequest decrypts the token of an encrypted image URL into an -// image request and checks it. On a token that is missing, does not decrypt, -// has expired or asks for something not valid, it writes an error response -// and returns false. +// image request and checks it, and returns it with the token's expiry. On a +// token that is missing, does not decrypt, has expired or asks for something +// not valid, it writes an error response and returns false. func (s *Handlers) parseImageEncRequest( w http.ResponseWriter, r *http.Request, -) (*imagerequest.Request, bool) { +) (*imagerequest.Request, time.Time, bool) { // Extract token from URL token := chi.URLParam(r, "token") if token == "" { s.respondError(w, "missing token", http.StatusBadRequest) - return nil, false + return nil, time.Time{}, false } // Decrypt and validate the payload - req, err := s.encGen.Parse(token) + req, expires, err := s.encGen.Parse(token) if err != nil { if errors.Is(err, encurl.ErrExpired) { s.log.Debug("encrypted URL expired", "error", err) s.respondError(w, "URL has expired", http.StatusGone) - return nil, false + return nil, time.Time{}, false } s.log.Debug("failed to decrypt URL", "error", err) s.respondError(w, "invalid encrypted URL", http.StatusBadRequest) - return nil, false + return nil, time.Time{}, false } // Apply the same dimension and fit-mode bounds as the plain image @@ -142,10 +149,10 @@ func (s *Handlers) parseImageEncRequest( s.respondError(w, "invalid encrypted URL: "+err.Error(), http.StatusBadRequest) - return nil, false + return nil, time.Time{}, false } - return req, true + return req, expires, true } // handleImageError converts image service errors to HTTP responses. diff --git a/internal/handlers/imageenc_internal_test.go b/internal/handlers/imageenc_internal_test.go index ec034c5..e939e13 100644 --- a/internal/handlers/imageenc_internal_test.go +++ b/internal/handlers/imageenc_internal_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/go-chi/chi/v5" @@ -62,7 +63,7 @@ func TestHandleImageEnc_OverLimitDimension_Returns400(t *testing.T) { Transform: imagerequest.Transform{ Size: imagerequest.Size{Width: 100000, Height: 100000}, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } @@ -91,7 +92,7 @@ func TestHandleImageEnc_InvalidFitMode_Returns400(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, FitMode: imagerequest.Fit("bogus"), }, - }) + }, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/imageenc_token_internal_test.go b/internal/handlers/imageenc_token_internal_test.go index f647a11..920cc12 100644 --- a/internal/handlers/imageenc_token_internal_test.go +++ b/internal/handlers/imageenc_token_internal_test.go @@ -70,15 +70,12 @@ func TestHandleImageEnc_RejectedToken(t *testing.T) { }, } - valid, err := h.encGen.Generate(&photo) + valid, err := h.encGen.Generate(&photo, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } - expiredPhoto := photo - expiredPhoto.Expires = time.Now().Add(-time.Minute) - - expired, err := h.encGen.Generate(&expiredPhoto) + expired, err := h.encGen.Generate(&photo, time.Now().Add(-time.Minute)) if err != nil { t.Fatalf("Generate() error = %v", err) } @@ -88,7 +85,7 @@ func TestHandleImageEnc_RejectedToken(t *testing.T) { t.Fatalf("encurl.NewGenerator() error = %v", err) } - otherKey, err := otherGen.Generate(&photo) + otherKey, err := otherGen.Generate(&photo, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/jpegxl_default_internal_test.go b/internal/handlers/jpegxl_default_internal_test.go index 219c0f0..9a5502e 100644 --- a/internal/handlers/jpegxl_default_internal_test.go +++ b/internal/handlers/jpegxl_default_internal_test.go @@ -69,8 +69,7 @@ func TestImageWithoutFormat_SignedAsJXL(t *testing.T) { Quality: encurl.DefaultQuality, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + }, expires) query := fmt.Sprintf("?sig=%s&exp=%d", sig, expires.Unix()) for _, size := range []string{"50x50.jxl", "50x50"} { @@ -94,7 +93,7 @@ func TestImageEncWithoutFormat_ServesJPEGXL(t *testing.T) { Transform: imagerequest.Transform{ Size: imagerequest.Size{Width: 50, Height: 50}, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/referer_blocklist_internal_test.go b/internal/handlers/referer_blocklist_internal_test.go index f3160a5..a81a7f5 100644 --- a/internal/handlers/referer_blocklist_internal_test.go +++ b/internal/handlers/referer_blocklist_internal_test.go @@ -15,7 +15,9 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/signature" ) // blockedReferer is a page on leech.example, which newRefererRoutes puts on @@ -31,11 +33,11 @@ type countingFetcher struct { // Fetch counts the fetch and passes it on. func (f *countingFetcher) Fetch( - ctx context.Context, url string, + ctx context.Context, source imagerequest.Source, ) (*httpfetcher.FetchResult, error) { f.fetches.Add(1) - return f.Fetcher.Fetch(ctx, url) + return f.Fetcher.Fetch(ctx, source) } // newRefererRoutes returns both image routes of a Handlers whose @@ -65,8 +67,6 @@ func newRefererRoutes(t *testing.T) (http.Handler, *Handlers, *countingFetcher) MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: testSigningKey, - Allowlist: []string{allowlistedHost}, MaxResponseSize: 50 << 20, }) if err != nil { @@ -79,9 +79,11 @@ func newRefererRoutes(t *testing.T) (http.Handler, *Handlers, *countingFetcher) } h := &Handlers{ - log: slog.New(slog.DiscardHandler), - imgSvc: svc, - encGen: encGen, + log: slog.New(slog.DiscardHandler), + imgSvc: svc, + encGen: encGen, + signer: signature.New(testSigningKey), + allowlist: allowlist.New([]string{allowlistedHost}), refererBlocklist: allowlist.New( []string{"leech.example", ".hotlinker.example"}), } diff --git a/internal/handlers/request_id_internal_test.go b/internal/handlers/request_id_internal_test.go index 110d45c..72da7e6 100644 --- a/internal/handlers/request_id_internal_test.go +++ b/internal/handlers/request_id_internal_test.go @@ -18,8 +18,8 @@ import ( // signedPhotoURL returns a signed /v1/image/ URL, valid for a minute, for the // JPEG at photoPath on signedHost at 50x50. It is signed with testSigningKey, -// the key of the image service newSignedHostServer builds. It takes a Handlers -// it does not use, so that it has the type of encPhotoURL. +// the key the handlers that newSignedHostServer builds check signatures with. +// It takes a Handlers it does not use, so that it has the type of encPhotoURL. func signedPhotoURL(t *testing.T, _ *Handlers) string { t.Helper() @@ -49,7 +49,7 @@ func encPhotoURL(t *testing.T, h *Handlers) string { Size: imagerequest.Size{Width: 50, Height: 50}, Format: imagerequest.FormatJPEG, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("Generate() error = %v", err) } diff --git a/internal/handlers/validate_signature_internal_test.go b/internal/handlers/validate_signature_internal_test.go new file mode 100644 index 0000000..0b13996 --- /dev/null +++ b/internal/handlers/validate_signature_internal_test.go @@ -0,0 +1,211 @@ +package handlers + +import ( + "testing" + "time" + + "sneak.berlin/go/pixa/internal/allowlist" + "sneak.berlin/go/pixa/internal/signature" +) + +// Addresses the port tests below put on the allowlist. +const ( + allowlistedIPv4 = "203.0.113.5" + allowlistedIPv6 = "2001:db8::1" +) + +// newSignatureChecker returns Handlers that check signatures with +// testSigningKey and have allowlistHosts on their allowlist, as New builds +// them from signing_key and allowlist_hosts. +func newSignatureChecker(allowlistHosts ...string) *Handlers { + return &Handlers{ + signer: signature.New(testSigningKey), + allowlist: allowlist.New(allowlistHosts), + } +} + +func TestValidateSignature_NonAllowlistedHost_NoSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedHost) + + // Should fail validation - not allowlisted and no signature + err := h.validateSignature(photoRequest(signedHost), "", time.Time{}) + if err == nil { + t.Error("validateSignature() expected error for non-allowlisted " + + "host without signature") + } +} + +func TestValidateSignature_NonAllowlistedHost_ValidSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedHost) + req := photoRequest(signedHost) + + // Generate a valid signature + expires := time.Now().Add(time.Hour) + sig := signature.New(testSigningKey).Sign(req, expires) + + // Should pass validation + err := h.validateSignature(req, sig, expires) + if err != nil { + t.Errorf("validateSignature() error = %v", err) + } +} + +func TestValidateSignature_NonAllowlistedHost_ExpiredSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedHost) + req := photoRequest(signedHost) + + // Generate an expired signature + expires := time.Now().Add(-time.Hour) + sig := signature.New(testSigningKey).Sign(req, expires) + + // Should fail validation + err := h.validateSignature(req, sig, expires) + if err == nil { + t.Error("validateSignature() expected error for expired signature") + } +} + +func TestValidateSignature_NonAllowlistedHost_InvalidSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedHost) + + // Should fail validation + err := h.validateSignature(photoRequest(signedHost), "invalid-signature", + time.Now().Add(time.Hour)) + if err == nil { + t.Error("validateSignature() expected error for invalid signature") + } +} + +// TestValidateSignature_NoAllowlist checks that with no host on the +// allowlist, a request without a signature fails validation. +func TestValidateSignature_NoAllowlist(t *testing.T) { + t.Parallel() + + h := newSignatureChecker() + + err := h.validateSignature(photoRequest(signedHost), "", time.Time{}) + if err == nil { + t.Error("validateSignature() expected error when no host is allowlisted") + } +} + +// TestValidateSignature_SignatureExactHostMatch verifies that +// validateSignature enforces exact host matching for signatures. A +// signature for one host must not verify for a different host, even +// if they share a domain suffix. +func TestValidateSignature_SignatureExactHostMatch(t *testing.T) { + t.Parallel() + + h := newSignatureChecker() + + // Sign a request for signedHost + signedReq := photoRequest(signedHost) + expires := time.Now().Add(time.Hour) + sig := signature.New(testSigningKey).Sign(signedReq, expires) + + // The original request should pass validation + t.Run("exact host passes", func(t *testing.T) { + t.Parallel() + + err := h.validateSignature(signedReq, sig, expires) + if err != nil { + t.Errorf("validateSignature() exact host failed: %v", err) + } + }) + + // Try to reuse the signature with different hosts + tests := []struct { + name string + host string + }{ + {"parent domain", "example.com"}, + {"sibling subdomain", "images.example.com"}, + {"deeper subdomain", "a." + signedHost}, + {"evil suffix domain", signedHost + ".evil.com"}, + {"prefixed host", "evil" + signedHost}, + } + + for _, tt := range tests { + t.Run(tt.name+" rejected", func(t *testing.T) { + t.Parallel() + + err := h.validateSignature(photoRequest(tt.host), sig, expires) + if err == nil { + t.Errorf("validateSignature() should reject signature for "+ + "host %q (signed for %q)", tt.host, signedHost) + } + }) + } +} + +// TestValidateSignature_HostWithPortNeedsSignature verifies that a host +// followed by a port, or by a colon with no port, matches no allowlist entry, +// so that a request for it without a signature is refused, while the same +// host with no port is allowlisted. +func TestValidateSignature_HostWithPortNeedsSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedHost, allowlistedIPv4, allowlistedIPv6) + + tests := []struct { + host string + allowlisted bool + }{ + {allowlistedHost, true}, + {allowlistedHost + ":8443", false}, + {allowlistedHost + ":443", false}, + {allowlistedHost + ":", false}, + {allowlistedIPv4, true}, + {allowlistedIPv4 + ":8080", false}, + {"[" + allowlistedIPv6 + "]", true}, + {"[" + allowlistedIPv6 + "]:8443", false}, + } + + for _, tt := range tests { + t.Run(tt.host, func(t *testing.T) { + t.Parallel() + + err := h.validateSignature(photoRequest(tt.host), "", time.Time{}) + t.Logf("validateSignature() for host %q with no signature: %v", + tt.host, err) + + if allowlisted := err == nil; allowlisted != tt.allowlisted { + t.Errorf("host %q served without a signature = %v, want %v", + tt.host, allowlisted, tt.allowlisted) + } + }) + } +} + +// TestValidateSignature_UnbracketedIPv6PortNeedsSignature verifies that an +// allowlisted IPv6 address written without brackets and followed by a port, +// or by a colon with no port, needs a signature. Go's URL type reads +// 2001:db8::1:8443 as the host 2001:db8::1 with the port 8443. +func TestValidateSignature_UnbracketedIPv6PortNeedsSignature(t *testing.T) { + t.Parallel() + + h := newSignatureChecker(allowlistedIPv6) + + for _, host := range []string{allowlistedIPv6 + ":8443", allowlistedIPv6 + ":"} { + t.Run(host, func(t *testing.T) { + t.Parallel() + + err := h.validateSignature(photoRequest(host), "", time.Time{}) + t.Logf("validateSignature() for host %q with no signature: %v", + host, err) + + if err == nil { + t.Errorf("host %q served without a signature, want refused", + host) + } + }) + } +} diff --git a/internal/httpfetcher/dial_context_internal_test.go b/internal/httpfetcher/dial_context_internal_test.go index 036bd78..b632a1f 100644 --- a/internal/httpfetcher/dial_context_internal_test.go +++ b/internal/httpfetcher/dial_context_internal_test.go @@ -51,12 +51,12 @@ func TestDialContextReplacesOnlyTheDialer(t *testing.T) { t.Errorf("body = %q, want %q", body, imagePayload) } - _, err := f.Fetch(testContext(t), "http://127.0.0.1/image") + _, err := f.Fetch(testContext(t), imageSourceOn("127.0.0.1")) if !errors.Is(err, ErrSSRFBlocked) { t.Errorf("Fetch(loopback URL) error = %v, want ErrSSRFBlocked", err) } - _, err = f.Fetch(testContext(t), upstreamURL("/redirect/private")) + _, err = f.Fetch(testContext(t), upstreamSource("/redirect/private")) if !errors.Is(err, ErrSSRFBlocked) { t.Errorf("Fetch(/redirect/private) error = %v, want ErrSSRFBlocked", err) } diff --git a/internal/httpfetcher/fetch_internal_test.go b/internal/httpfetcher/fetch_internal_test.go index f2f3e83..0d515d7 100644 --- a/internal/httpfetcher/fetch_internal_test.go +++ b/internal/httpfetcher/fetch_internal_test.go @@ -14,6 +14,8 @@ import ( "sync" "testing" "time" + + "sneak.berlin/go/pixa/internal/imagerequest" ) // testPublicHost is a TEST-NET-1 (RFC 5737) literal. isPrivateIP treats it as @@ -29,9 +31,9 @@ const imagePayload = "fake-jpeg-bytes" // would mean SSRF protection let a forbidden target reach the transport. var errUnexpectedDial = errors.New("unexpected dial target") -// upstreamURL builds a fetch URL on the fake public host for the given path. -func upstreamURL(path string) string { - return "http://" + testPublicHost + path +// upstreamSource returns the source at path on the fake public host. +func upstreamSource(path string) imagerequest.Source { + return imagerequest.Source{Host: testPublicHost, Path: path} } // recordingDialer records every address the transport asks it to dial and @@ -158,7 +160,7 @@ func testContext(t *testing.T) context.Context { func fetchImage(t *testing.T, f *HTTPFetcher, path string) *FetchResult { t.Helper() - res, err := f.Fetch(testContext(t), upstreamURL(path)) + res, err := f.Fetch(testContext(t), upstreamSource(path)) if err != nil { t.Fatalf("Fetch(%s) error = %v", path, err) } @@ -170,7 +172,7 @@ func fetchImage(t *testing.T, f *HTTPFetcher, path string) *FetchResult { func fetchExpectError(t *testing.T, f *HTTPFetcher, path string) error { t.Helper() - res, err := f.Fetch(testContext(t), upstreamURL(path)) + res, err := f.Fetch(testContext(t), upstreamSource(path)) if err == nil { _ = res.Content.Close() @@ -222,7 +224,7 @@ func TestFetchRedirectToPrivateIPBlocked(t *testing.T) { srv := startUpstream(t) f, dialer := newServerFetcher(t, srv, nil) - _, err := f.Fetch(testContext(t), upstreamURL("/redirect/private")) + _, err := f.Fetch(testContext(t), upstreamSource("/redirect/private")) if !errors.Is(err, ErrSSRFBlocked) { t.Fatalf("Fetch() error = %v, want ErrSSRFBlocked", err) } @@ -590,3 +592,53 @@ func TestSSRFSafeDialerAllowsPublicTarget(t *testing.T) { t.Errorf("public target was SSRF-blocked: %v", err) } } + +// TestFetchSchemeFollowsAllowHTTP checks that the fetcher fetches a source +// over https, on port 443, unless AllowHTTP is set, and then over http, on +// port 80. The test server speaks plain HTTP, so only the fetch over http +// succeeds. +func TestFetchSchemeFollowsAllowHTTP(t *testing.T) { + t.Parallel() + + tests := []struct { + allowHTTP bool + wantAddr string + }{ + {false, net.JoinHostPort(testPublicHost, "443")}, + {true, net.JoinHostPort(testPublicHost, "80")}, + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("AllowHTTP %v", tt.allowHTTP), func(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + dialer := &recordingDialer{target: srv.Listener.Addr().String()} + + f := New(&Config{ + Timeout: 30 * time.Second, + MaxResponseSize: 50 << 20, + AllowHTTP: tt.allowHTTP, + MaxConnectionsPerHost: 20, + MaxConnections: 64, + DialContext: dialer.dialContext, + }) + + res, err := f.Fetch(testContext(t), upstreamSource("/image")) + if err == nil { + _ = res.Content.Close() + } + + dialed := dialer.dialedAddrs() + t.Logf("Fetch(): error %v, dialed %v", err, dialed) + + if served := err == nil; served != tt.allowHTTP { + t.Errorf("served = %v, want %v", served, tt.allowHTTP) + } + + if len(dialed) != 1 || dialed[0] != tt.wantAddr { + t.Errorf("dialed %v, want only %s", dialed, tt.wantAddr) + } + }) + } +} diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index c9d2eae..08eeee1 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -20,6 +20,7 @@ import ( "time" "github.com/go-chi/chi/v5/middleware" + "sneak.berlin/go/pixa/internal/imagerequest" ) // Fetcher configuration constants. @@ -91,8 +92,8 @@ var ( // Fetcher retrieves content from upstream origins. type Fetcher interface { - // Fetch retrieves content from the given URL. - Fetch(ctx context.Context, url string) (*FetchResult, error) + // Fetch retrieves the content of the source image. + Fetch(ctx context.Context, source imagerequest.Source) (*FetchResult, error) } // FetchResult contains the result of fetching from upstream. @@ -139,7 +140,8 @@ type Config struct { Timeout time.Duration // MaxResponseSize is the maximum allowed response body size. MaxResponseSize int64 - // AllowHTTP allows non-TLS connections (for testing only). + // AllowHTTP makes the fetcher fetch sources over http instead of https, + // and follow redirects to http (for testing only). AllowHTTP bool // MaxConnectionsPerHost limits concurrent connections to each upstream host. MaxConnectionsPerHost int @@ -211,7 +213,7 @@ func New(config *Config) *HTTPFetcher { } // Validate the redirect target - err := validateURL(req.Context(), req.URL.String(), config.AllowHTTP) + err := validateURL(req.Context(), req.URL, config.AllowHTTP) if err != nil { return fmt.Errorf("redirect blocked: %w", err) } @@ -229,18 +231,22 @@ func New(config *Config) *HTTPFetcher { } } -// Fetch retrieves content from the given URL with SSRF protection. When -// MaxConnections fetches are already in progress, it waits up to -// ConnectionWaitTimeout for one to finish, then fails with -// ErrTooManyConnections. -func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, error) { +// Fetch retrieves the source image with SSRF protection, over https, or over +// http when AllowHTTP is set. When MaxConnections fetches are already in +// progress, it waits up to ConnectionWaitTimeout for one to finish, then +// fails with ErrTooManyConnections. +func (f *HTTPFetcher) Fetch( + ctx context.Context, source imagerequest.Source, +) (*FetchResult, error) { + sourceURL := source.URL(f.config.AllowHTTP) + // Validate URL before making request - err := validateURL(ctx, url, f.config.AllowHTTP) + err := validateURL(ctx, sourceURL, f.config.AllowHTTP) if err != nil { return nil, err } - release, err := f.acquireConnection(ctx, extractHost(url)) + release, err := f.acquireConnection(ctx, source.Host) if err != nil { return nil, err } @@ -254,14 +260,9 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro } }() - parsedURL, err := neturl.Parse(url) - if err != nil { - return nil, fmt.Errorf("failed to parse URL: %w", err) - } - req := &http.Request{ Method: http.MethodGet, - URL: parsedURL, + URL: sourceURL, Header: make(http.Header), } @@ -455,19 +456,14 @@ func (f *HTTPFetcher) isAllowedContentType(contentType string) bool { } // validateURL checks if a URL is safe to fetch (not internal/private). -func validateURL(ctx context.Context, rawURL string, allowHTTP bool) error { - if !allowHTTP && !strings.HasPrefix(rawURL, "https://") { +func validateURL(ctx context.Context, fetchURL *neturl.URL, allowHTTP bool) error { + if !allowHTTP && fetchURL.Scheme != "https" { return ErrUnsupportedScheme } - parsedURL, err := neturl.Parse(rawURL) - if err != nil { - return fmt.Errorf("%w: %w", ErrInvalidHost, err) - } - // Hostname drops any port, and the brackets of an IPv6 address whether // or not a port follows them - host := parsedURL.Hostname() + host := fetchURL.Hostname() if host == "" { return ErrInvalidHost } @@ -493,25 +489,6 @@ func validateURL(ctx context.Context, rawURL string, allowHTTP bool) error { return nil } -// extractHost extracts the host from a URL string. -func extractHost(rawURL string) string { - // Simple extraction without full URL parsing - url := rawURL - if idx := strings.Index(url, "://"); idx != -1 { - url = url[idx+3:] - } - - if idx := strings.Index(url, "/"); idx != -1 { - url = url[:idx] - } - - if idx := strings.Index(url, "?"); idx != -1 { - url = url[:idx] - } - - return url -} - // isLocalhost checks if the host is localhost. func isLocalhost(host string) bool { host = strings.ToLower(host) diff --git a/internal/httpfetcher/httpfetcher_internal_test.go b/internal/httpfetcher/httpfetcher_internal_test.go index 7341d28..8b71419 100644 --- a/internal/httpfetcher/httpfetcher_internal_test.go +++ b/internal/httpfetcher/httpfetcher_internal_test.go @@ -5,9 +5,12 @@ import ( "errors" "io" "net" + "net/url" "testing" "testing/fstest" "time" + + "sneak.berlin/go/pixa/internal/imagerequest" ) // testHost is the hostname used by mock fetch tests. @@ -49,33 +52,6 @@ func TestIsAllowedContentType(t *testing.T) { } } -func TestExtractHost(t *testing.T) { - t.Parallel() - - tests := []struct { - url string - want string - }{ - {"https://example.com/path", testHost}, - {"http://example.com:8080/path", "example.com:8080"}, - {"https://example.com", testHost}, - {"https://example.com?q=1", testHost}, - {"example.com/path", testHost}, - {"", ""}, - } - - for _, tc := range tests { - t.Run(tc.url, func(t *testing.T) { - t.Parallel() - - got := extractHost(tc.url) - if got != tc.want { - t.Errorf("extractHost(%q) = %q, want %q", tc.url, got, tc.want) - } - }) - } -} - func TestIsLocalhost(t *testing.T) { t.Parallel() @@ -188,7 +164,7 @@ func TestIsPrivateIPJudgesEmbeddedIPv4Address(t *testing.T) { func TestValidateURL_RejectsNonHTTPS(t *testing.T) { t.Parallel() - err := validateURL(t.Context(), "http://example.com/path", false) + err := validateURL(t.Context(), &url.URL{Scheme: "http", Host: testHost}, false) if !errors.Is(err, ErrUnsupportedScheme) { t.Errorf("validateURL http = %v, want ErrUnsupportedScheme", err) } @@ -198,7 +174,8 @@ func TestValidateURL_AllowsHTTPWhenConfigured(t *testing.T) { t.Parallel() // Use a host that won't resolve (explicit .invalid TLD) so we don't hit DNS. - err := validateURL(t.Context(), "http://nonexistent.invalid/path", true) + err := validateURL(t.Context(), + &url.URL{Scheme: "http", Host: "nonexistent.invalid"}, true) // We expect a host resolution error, not ErrUnsupportedScheme. if errors.Is(err, ErrUnsupportedScheme) { t.Error("validateURL with AllowHTTP should not return ErrUnsupportedScheme") @@ -208,7 +185,7 @@ func TestValidateURL_AllowsHTTPWhenConfigured(t *testing.T) { func TestValidateURL_RejectsLocalhost(t *testing.T) { t.Parallel() - err := validateURL(t.Context(), "https://localhost/path", false) + err := validateURL(t.Context(), &url.URL{Scheme: "https", Host: "localhost"}, false) if !errors.Is(err, ErrSSRFBlocked) { t.Errorf("validateURL localhost = %v, want ErrSSRFBlocked", err) } @@ -217,7 +194,7 @@ func TestValidateURL_RejectsLocalhost(t *testing.T) { func TestValidateURL_EmptyHost(t *testing.T) { t.Parallel() - err := validateURL(t.Context(), "https:///path", false) + err := validateURL(t.Context(), &url.URL{Scheme: "https"}, false) if !errors.Is(err, ErrInvalidHost) { t.Errorf("validateURL empty host = %v, want ErrInvalidHost", err) } @@ -232,7 +209,8 @@ func TestMockFetcher_FetchesFile(t *testing.T) { m := NewMock(mockFS) - result, err := m.Fetch(context.Background(), "https://example.com/images/photo.jpg") + result, err := m.Fetch(context.Background(), + imagerequest.Source{Host: testHost, Path: "/images/photo.jpg"}) if err != nil { t.Fatalf("Fetch() error = %v", err) } @@ -263,7 +241,8 @@ func TestMockFetcher_MissingFileReturnsUpstreamError(t *testing.T) { mockFS := fstest.MapFS{} m := NewMock(mockFS) - _, err := m.Fetch(context.Background(), "https://example.com/missing.jpg") + _, err := m.Fetch(context.Background(), + imagerequest.Source{Host: testHost, Path: "/missing.jpg"}) if !errors.Is(err, ErrUpstreamError) { t.Errorf("Fetch() error = %v, want ErrUpstreamError", err) } @@ -280,7 +259,7 @@ func TestMockFetcher_RespectsContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := m.Fetch(ctx, "https://example.com/photo.jpg") + _, err := m.Fetch(ctx, imagerequest.Source{Host: testHost, Path: "/photo.jpg"}) if !errors.Is(err, context.Canceled) { t.Errorf("Fetch() error = %v, want context.Canceled", err) } diff --git a/internal/httpfetcher/max_connections_internal_test.go b/internal/httpfetcher/max_connections_internal_test.go index d414178..bb80228 100644 --- a/internal/httpfetcher/max_connections_internal_test.go +++ b/internal/httpfetcher/max_connections_internal_test.go @@ -8,14 +8,20 @@ import ( "sync" "testing" "time" + + "sneak.berlin/go/pixa/internal/imagerequest" ) -// imageURLOnPort is the fake upstream's image route on testPublicHost at +// imageSourceOn is the fake upstream's image route on host. +func imageSourceOn(host string) imagerequest.Source { + return imagerequest.Source{Host: host, Path: "/image"} +} + +// imageSourceOnPort is the fake upstream's image route on testPublicHost at // port. Each port is a different host to the per-host limit, while the test // dialer sends every port to the one test server. -func imageURLOnPort(port int) string { - return "http://" + net.JoinHostPort(testPublicHost, strconv.Itoa(port)) + - "/image" +func imageSourceOnPort(port int) imagerequest.Source { + return imageSourceOn(net.JoinHostPort(testPublicHost, strconv.Itoa(port))) } // TestFetchLimitsConnectionsToAllHostsTogether checks that MaxConnections @@ -39,12 +45,12 @@ func TestFetchLimitsConnectionsToAllHostsTogether(t *testing.T) { f, _ := newServerFetcher(t, srv, cfg) f.connectionWaitTimeout = 100 * time.Millisecond - first, err := f.Fetch(testContext(t), imageURLOnPort(81)) + first, err := f.Fetch(testContext(t), imageSourceOnPort(81)) if err != nil { t.Fatalf("first Fetch() error = %v", err) } - second, err := f.Fetch(testContext(t), imageURLOnPort(82)) + second, err := f.Fetch(testContext(t), imageSourceOnPort(82)) if err != nil { t.Fatalf("second Fetch() error = %v", err) } @@ -53,7 +59,7 @@ func TestFetchLimitsConnectionsToAllHostsTogether(t *testing.T) { start := time.Now() - _, err = f.Fetch(testContext(t), imageURLOnPort(83)) + _, err = f.Fetch(testContext(t), imageSourceOnPort(83)) if !errors.Is(err, ErrTooManyConnections) { t.Fatalf("third Fetch() error = %v, want ErrTooManyConnections", err) } @@ -72,7 +78,7 @@ func TestFetchLimitsConnectionsToAllHostsTogether(t *testing.T) { t.Fatalf("close first body: %v", err) } - third, err := f.Fetch(testContext(t), imageURLOnPort(83)) + third, err := f.Fetch(testContext(t), imageSourceOnPort(83)) if err != nil { t.Fatalf("Fetch() after a response was closed: error = %v", err) } @@ -99,7 +105,7 @@ func TestFetchFreesHostSlotWhenContextEndsWaitingForConnection(t *testing.T) { f, _ := newServerFetcher(t, srv, cfg) - first, err := f.Fetch(testContext(t), imageURLOnPort(81)) + first, err := f.Fetch(testContext(t), imageSourceOnPort(81)) if err != nil { t.Fatalf("first Fetch() error = %v", err) } @@ -109,7 +115,7 @@ func TestFetchFreesHostSlotWhenContextEndsWaitingForConnection(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) defer cancel() - _, err = f.Fetch(ctx, imageURLOnPort(82)) + _, err = f.Fetch(ctx, imageSourceOnPort(82)) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("second Fetch() error = %v, want context.DeadlineExceeded", err) } @@ -134,7 +140,7 @@ func TestFetchRemovesIdleHostSemaphores(t *testing.T) { for i := range 100 { wg.Go(func() { - res, err := f.Fetch(ctx, imageURLOnPort(1+i%50)) + res, err := f.Fetch(ctx, imageSourceOnPort(1+i%50)) if err != nil { t.Errorf("Fetch() error = %v", err) @@ -173,12 +179,12 @@ func TestFetchRemovesHostSemaphoreWhenNoConnection(t *testing.T) { f, _ := newServerFetcher(t, srv, cfg) f.connectionWaitTimeout = 100 * time.Millisecond - open, err := f.Fetch(testContext(t), imageURLOnPort(81)) + open, err := f.Fetch(testContext(t), imageSourceOnPort(81)) if err != nil { t.Fatalf("first Fetch() error = %v", err) } - _, err = f.Fetch(testContext(t), imageURLOnPort(82)) + _, err = f.Fetch(testContext(t), imageSourceOnPort(82)) if !errors.Is(err, ErrTooManyConnections) { t.Fatalf("Fetch() from another host: error = %v, "+ "want ErrTooManyConnections", err) @@ -187,7 +193,7 @@ func TestFetchRemovesHostSemaphoreWhenNoConnection(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) defer cancel() - _, err = f.Fetch(ctx, imageURLOnPort(81)) + _, err = f.Fetch(ctx, imageSourceOnPort(81)) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("Fetch() from the busy host: error = %v, "+ "want context.DeadlineExceeded", err) @@ -199,7 +205,7 @@ func TestFetchRemovesHostSemaphoreWhenNoConnection(t *testing.T) { ctx, cancel = context.WithTimeout(t.Context(), 100*time.Millisecond) defer cancel() - _, err = f.Fetch(ctx, imageURLOnPort(83)) + _, err = f.Fetch(ctx, imageSourceOnPort(83)) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("Fetch() from a host with nothing open: error = %v, "+ "want context.DeadlineExceeded", err) @@ -222,15 +228,15 @@ func TestFetchReleasesConnectionOnError(t *testing.T) { t.Parallel() cases := []struct { - name string - url string - want error + name string + source imagerequest.Source + want error }{ - {"upstream answers 500", upstreamURL("/status/500"), ErrUpstreamError}, - {"upstream sends HTML", upstreamURL("/html"), ErrInvalidContentType}, + {"upstream answers 500", upstreamSource("/status/500"), ErrUpstreamError}, + {"upstream sends HTML", upstreamSource("/html"), ErrInvalidContentType}, // 198.51.100.7 (TEST-NET-2) passes the SSRF checks, and the test // dialer refuses every host but testPublicHost. - {"connecting fails", "http://198.51.100.7/image", errUnexpectedDial}, + {"connecting fails", imageSourceOn("198.51.100.7"), errUnexpectedDial}, } for _, tc := range cases { @@ -249,7 +255,7 @@ func TestFetchReleasesConnectionOnError(t *testing.T) { f, _ := newServerFetcher(t, srv, cfg) f.connectionWaitTimeout = 100 * time.Millisecond - _, err := f.Fetch(testContext(t), tc.url) + _, err := f.Fetch(testContext(t), tc.source) if !errors.Is(err, tc.want) { t.Fatalf("Fetch() error = %v, want %v", err, tc.want) } diff --git a/internal/httpfetcher/mock.go b/internal/httpfetcher/mock.go index 2a3b11b..40ac3e6 100644 --- a/internal/httpfetcher/mock.go +++ b/internal/httpfetcher/mock.go @@ -7,14 +7,14 @@ import ( "io/fs" "net/http" "strings" -) -// errEmptyURLPath is returned when a mock URL has no usable path. -var errEmptyURLPath = errors.New("empty URL path") + "sneak.berlin/go/pixa/internal/imagerequest" +) // MockFetcher implements Fetcher using an embedded filesystem. // Files are organized as: hostname/path/to/file.ext -// URLs like https://example.com/images/photo.jpg map to example.com/images/photo.jpg. +// The source with host example.com and path /images/photo.jpg maps to +// example.com/images/photo.jpg; the source query is ignored. type MockFetcher struct { fs fs.FS } @@ -24,8 +24,10 @@ func NewMock(fsys fs.FS) *MockFetcher { return &MockFetcher{fs: fsys} } -// Fetch retrieves content from the mock filesystem. -func (m *MockFetcher) Fetch(ctx context.Context, url string) (*FetchResult, error) { +// Fetch retrieves the source's content from the mock filesystem. +func (m *MockFetcher) Fetch( + ctx context.Context, source imagerequest.Source, +) (*FetchResult, error) { // Check context cancellation select { case <-ctx.Done(): @@ -33,11 +35,7 @@ func (m *MockFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro default: } - // Parse URL to get filesystem path - path, err := urlToFSPath(url) - if err != nil { - return nil, err - } + path := source.Host + source.Path // Open the file f, err := m.fs.Open(path) @@ -68,32 +66,6 @@ func (m *MockFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro }, nil } -// urlToFSPath converts a URL to a filesystem path. -// https://example.com/images/photo.jpg -> example.com/images/photo.jpg -func urlToFSPath(rawURL string) (string, error) { - // Strip scheme - url := rawURL - if idx := strings.Index(url, "://"); idx != -1 { - url = url[idx+3:] - } - - // Remove query string - if idx := strings.Index(url, "?"); idx != -1 { - url = url[:idx] - } - - // Remove fragment - if idx := strings.Index(url, "#"); idx != -1 { - url = url[:idx] - } - - if url == "" { - return "", errEmptyURLPath - } - - return url, nil -} - // detectContentTypeFromPath returns the MIME type based on file extension. func detectContentTypeFromPath(path string) string { path = strings.ToLower(path) diff --git a/internal/httpfetcher/request_id_internal_test.go b/internal/httpfetcher/request_id_internal_test.go index a3c998e..62a654d 100644 --- a/internal/httpfetcher/request_id_internal_test.go +++ b/internal/httpfetcher/request_id_internal_test.go @@ -34,7 +34,7 @@ func TestFetchSendsRequestID(t *testing.T) { ctx := context.WithValue(testContext(t), middleware.RequestIDKey, requestID) - res, err := f.Fetch(ctx, upstreamURL("/image")) + res, err := f.Fetch(ctx, upstreamSource("/image")) if err != nil { t.Fatalf("Fetch() error = %v", err) } diff --git a/internal/imagerequest/imagerequest.go b/internal/imagerequest/imagerequest.go index ecc86b4..d9b2bac 100644 --- a/internal/imagerequest/imagerequest.go +++ b/internal/imagerequest/imagerequest.go @@ -1,13 +1,12 @@ // Package imagerequest holds the image request type, its parts, its checks, -// the parsing of the /v1/image/ path, the variant key and the source key. It -// imports no other package of this repo, so every package that handles an -// image request can import it. +// the parsing of the /v1/image/ path, the variant key, the source key and the +// source's upstream URL. It imports no other package of this repo, so every +// package that handles an image request can import it. package imagerequest import ( "errors" "net/url" - "time" ) // ErrInvalidFitMode is returned when an invalid fit mode is provided. @@ -85,19 +84,13 @@ func ValidateImageRequest(req *Request) error { } // Request represents a request for a processed image: the source to fetch -// and the transform to apply to it. +// and the transform to apply to it. It holds exactly what the variant key +// covers. type Request struct { // Source is what to fetch Source Source // Transform is what to do to it Transform Transform - // Signature is the HMAC signature for non-allowlisted hosts - Signature string - // Expires is when the URL expires: the exp of a signed URL, or the expiry - // of an encrypted URL; the zero time if it has none - Expires time.Time - // AllowHTTP indicates whether HTTP (non-TLS) is allowed for this request - AllowHTTP bool } // Source is the image a request fetches from the origin. @@ -125,23 +118,20 @@ type Transform struct { FitMode Fit } -// SourceURL returns the full upstream URL to fetch: the source path -// percent-encoded again, as net/url writes a path, so each source path is one -// upstream path, and the source query as it is, with the characters -// escapeQuery encodes percent-encoded. Uses http:// scheme when AllowHTTP is -// true, otherwise https://. -func (r *Request) SourceURL() string { +// URL returns the source's upstream URL, with the scheme http when allowHTTP +// is true and https otherwise. net/url writes its path percent-encoded again, +// so each source path is one upstream path. Its query is the source query as +// it is, with the characters escapeQuery encodes percent-encoded. +func (s Source) URL(allowHTTP bool) *url.URL { scheme := "https" - if r.AllowHTTP { + if allowHTTP { scheme = "http" } - sourceURL := url.URL{ + return &url.URL{ Scheme: scheme, - Host: r.Source.Host, - Path: r.Source.Path, - RawQuery: escapeQuery(r.Source.Query), + Host: s.Host, + Path: s.Path, + RawQuery: escapeQuery(s.Query), } - - return sourceURL.String() } diff --git a/internal/imagerequest/sourceurl_internal_test.go b/internal/imagerequest/sourceurl_internal_test.go index 6e6b671..0b953fd 100644 --- a/internal/imagerequest/sourceurl_internal_test.go +++ b/internal/imagerequest/sourceurl_internal_test.go @@ -2,57 +2,49 @@ package imagerequest import "testing" -func TestRequest_SourceURL_DefaultHTTPS(t *testing.T) { +func TestSource_URL_DefaultHTTPS(t *testing.T) { t.Parallel() - req := &Request{ - Source: Source{ - Host: testHostCDN, - Path: testPathCat, - Query: "v=2", - }, + source := Source{ + Host: testHostCDN, + Path: testPathCat, + Query: "v=2", } - got := req.SourceURL() + got := source.URL(false).String() want := "https://cdn.example.com/photos/cat.jpg?v=2" if got != want { - t.Errorf("SourceURL() = %q, want %q", got, want) + t.Errorf("URL() = %q, want %q", got, want) } } -func TestRequest_SourceURL_AllowHTTP(t *testing.T) { +func TestSource_URL_AllowHTTP(t *testing.T) { t.Parallel() - req := &Request{ - Source: Source{ - Host: "localhost:8080", - Path: testPathCat, - }, - AllowHTTP: true, + source := Source{ + Host: "localhost:8080", + Path: testPathCat, } - got := req.SourceURL() + got := source.URL(true).String() want := "http://localhost:8080/photos/cat.jpg" if got != want { - t.Errorf("SourceURL() = %q, want %q", got, want) + t.Errorf("URL() = %q, want %q", got, want) } } -func TestRequest_SourceURL_AllowHTTPFalse(t *testing.T) { +func TestSource_URL_AllowHTTPFalse(t *testing.T) { t.Parallel() - req := &Request{ - Source: Source{ - Host: testHostCDN, - Path: "/img.jpg", - }, - AllowHTTP: false, + source := Source{ + Host: testHostCDN, + Path: "/img.jpg", } - got := req.SourceURL() + got := source.URL(false).String() if got != "https://cdn.example.com/img.jpg" { - t.Errorf("SourceURL() = %q, want https scheme", got) + t.Errorf("URL() = %q, want https scheme", got) } } diff --git a/internal/imagerequest/urlparser.go b/internal/imagerequest/urlparser.go index 9e8cc4c..09d7bd5 100644 --- a/internal/imagerequest/urlparser.go +++ b/internal/imagerequest/urlparser.go @@ -126,15 +126,15 @@ func parseImageComponents(remainder string) (*Request, error) { return nil, ErrMissingHost } - // Only the forms hostRegex describes are accepted. A "?", "#" or "@" in - // the host would make Go's URL parser, which the allowlist check and the - // fetcher use, read a different host from the one allowlisted and cached + // Later steps put the host unparsed into the allowlist check, the cache key + // and the upstream URL, so only the forms hostRegex describes are accepted hostMatch := hostRegex.FindStringSubmatch(host) if hostMatch == nil { return nil, fmt.Errorf("%w: %q", ErrInvalidHost, host) } - // Go's URL parser refuses brackets around anything but an IPv6 address + // The allowlist check and the fetcher drop the brackets, so brackets + // around anything but an IPv6 address would give a host a second spelling if inBrackets := hostMatch[1]; inBrackets != "" { addr, err := netip.ParseAddr(inBrackets) if err != nil || !addr.Is6() { @@ -147,9 +147,8 @@ func parseImageComponents(remainder string) (*Request, error) { // host host = strings.ToLower(host) - // Go's URL parser, which the fetcher uses, reads a "#" as the start of a - // fragment and drops the rest, so each text after it would be another - // spelling, cached under its own keys, of one upstream request + // A "#" starts a URL's fragment, so neither the source path nor the source + // query may hold one if strings.Contains(path, "#") { return nil, fmt.Errorf( `%w: the source path or query holds a "#"`, ErrInvalidPath) diff --git a/internal/imgcache/concurrent_misses_internal_test.go b/internal/imgcache/concurrent_misses_internal_test.go index 226e79e..594e4fb 100644 --- a/internal/imgcache/concurrent_misses_internal_test.go +++ b/internal/imgcache/concurrent_misses_internal_test.go @@ -36,7 +36,7 @@ type heldFetcher struct { } func (f *heldFetcher) Fetch( - ctx context.Context, url string, + ctx context.Context, source imagerequest.Source, ) (*httpfetcher.FetchResult, error) { f.fetches.Add(1) @@ -48,7 +48,7 @@ func (f *heldFetcher) Fetch( return nil, ctx.Err() } - return f.upstream.Fetch(ctx, url) + return f.upstream.Fetch(ctx, source) } // releaseFetches lets every held fetch, and every later one, go on. @@ -79,8 +79,8 @@ func setupHeldFetchService(t *testing.T) (*Service, *TestFixtures, *heldFetcher) } // photoVariant asks for the test photo, 100x100, at 50x25 as a JPEG of the -// given quality and fit mode. Each call returns a new request, as Get writes -// to the request it is given. +// given quality and fit mode. Each call returns a new request, which a test +// may change. func photoVariant( fixtures *TestFixtures, quality int, fit imagerequest.Fit, ) *imagerequest.Request { @@ -489,7 +489,7 @@ func TestService_Get_WaiterGetsProcessingTimedOut(t *testing.T) { type panickingFetcher struct{} func (panickingFetcher) Fetch( - context.Context, string, + context.Context, imagerequest.Source, ) (*httpfetcher.FetchResult, error) { panic("upstream fetcher panicked") } diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index ece9311..35d762a 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -7,21 +7,17 @@ import ( "fmt" "io" "log/slog" - "net/url" "runtime/debug" - "strings" "time" "github.com/dustin/go-humanize" "github.com/getsentry/sentry-go" "github.com/go-chi/chi/v5/middleware" "golang.org/x/sync/singleflight" - "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/magic" - "sneak.berlin/go/pixa/internal/signature" ) // Service orchestrates the cache, fetcher, and processor. @@ -29,10 +25,7 @@ type Service struct { cache *Cache fetcher httpfetcher.Fetcher processor *imageprocessor.ImageProcessor - signer *signature.Signer - allowlist *allowlist.HostAllowList log *slog.Logger - allowHTTP bool maxResponseSize int64 // variantsInProgress lets the requests that miss the same variant at the // same time share one fetch and one transcode. @@ -47,13 +40,6 @@ type ServiceConfig struct { Fetcher httpfetcher.Fetcher // Processor converts and resizes the images Processor *imageprocessor.ImageProcessor - // SigningKey is the HMAC signing key (empty disables signing) - SigningKey string - // Allowlist is the list of hosts that don't require signatures - Allowlist []string - // AllowHTTP makes the service fetch sources over http instead of - // https (testing only), so the fetcher must allow http too - AllowHTTP bool // MaxResponseSize is the largest source accepted, in bytes; a cached // source larger than this is fetched again MaxResponseSize int64 @@ -61,11 +47,8 @@ type ServiceConfig struct { Logger *slog.Logger } -// Static errors for service construction. -var ( - errCacheRequired = errors.New("cache is required") - errSigningKeyRequired = errors.New("signing key is required") -) +// errCacheRequired is returned when the service is built without a cache. +var errCacheRequired = errors.New("cache is required") // NewService creates a new image service. func NewService(cfg *ServiceConfig) (*Service, error) { @@ -73,10 +56,6 @@ func NewService(cfg *ServiceConfig) (*Service, error) { return nil, errCacheRequired } - if cfg.SigningKey == "" { - return nil, errSigningKeyRequired - } - log := cfg.Logger if log == nil { log = slog.Default() @@ -93,10 +72,7 @@ func NewService(cfg *ServiceConfig) (*Service, error) { cache: cfg.Cache, fetcher: cfg.Fetcher, processor: cfg.Processor, - signer: signature.New(cfg.SigningKey), - allowlist: allowlist.New(cfg.Allowlist), log: log, - allowHTTP: cfg.AllowHTTP, maxResponseSize: cfg.MaxResponseSize, }, nil } @@ -123,9 +99,6 @@ var ErrProcessingTimedOut = errors.New( func (s *Service) Get( ctx context.Context, req *imagerequest.Request, ) (*ImageResponse, error) { - // Propagate AllowHTTP setting to the request - req.AllowHTTP = s.allowHTTP - cacheKey := imagerequest.CacheKey(req) reader, size, contentType, err := s.cache.GetVariant(cacheKey) @@ -169,33 +142,6 @@ func (s *Service) WaitForProcessing(ctx context.Context) int { return s.processor.WaitForProcessing(ctx) } -// ValidateRequest validates the request signature if required. -func (s *Service) ValidateRequest(req *imagerequest.Request) error { - // Check if host is allowed (no signature required) - sourceURL := req.SourceURL() - - parsedURL, err := url.Parse(sourceURL) - if err != nil { - return fmt.Errorf("invalid source URL: %w", err) - } - - // An allowlist entry names no port, so a host followed by ":", with or - // without a port, needs a signature. The port is read from the parsed - // URL, as the fetcher reads it: in an IPv6 address written without - // brackets, such as 2001:db8::1:8443, it is what follows the last ":". - // This is checked here and not in allowlist.IsAllowed, which also matches - // referer_blocklist, where a Referer must be blocked whatever its port. - hostHasPort := parsedURL.Port() != "" || - strings.HasSuffix(parsedURL.Host, ":") - - if !hostHasPort && s.allowlist.IsAllowed(parsedURL) { - return nil - } - - // Signature required for non-allowed hosts - return s.signer.Verify(req) -} - // errPanicked is returned when processing a variant panicked. var errPanicked = errors.New("panic while processing image") @@ -388,11 +334,10 @@ func (s *Service) fetchAndProcess( } // Fetch from upstream - sourceURL := req.SourceURL() + s.log.Debug("fetching from upstream", "host", req.Source.Host, + "path", req.Source.Path, "query", req.Source.Query) - s.log.Debug("fetching from upstream", "url", sourceURL) - - fetchResult, err := s.fetcher.Fetch(ctx, sourceURL) + fetchResult, err := s.fetcher.Fetch(ctx, req.Source) if err != nil { // Store negative cache for certain errors if isNegativeCacheable(err) { diff --git a/internal/imgcache/service_internal_test.go b/internal/imgcache/service_internal_test.go index 31573e1..ed8ae02 100644 --- a/internal/imgcache/service_internal_test.go +++ b/internal/imgcache/service_internal_test.go @@ -9,17 +9,15 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/magic" - "sneak.berlin/go/pixa/internal/signature" ) // Test data literals used repeatedly in this file (goconst). const ( testPathPhoto = "/images/photo.jpg" testPathUpload = "/uploads/image.jpg" - testSigningKey = "test-signing-key-12345" ) -func TestService_Get_AllowlistedHost(t *testing.T) { +func TestService_Get_GoodHost(t *testing.T) { t.Parallel() svc, fixtures := SetupTestService(t) @@ -60,36 +58,12 @@ func TestService_Get_AllowlistedHost(t *testing.T) { } } -func TestService_Get_NonAllowlistedHost_NoSignature(t *testing.T) { +// TestService_Get_OtherHost checks that the image service serves a second host +// as it serves GoodHost. +func TestService_Get_OtherHost(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t, WithSigningKey("test-key")) - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: fixtures.OtherHost, - Path: testPathUpload, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - // Should fail validation - not allowlisted and no signature - err := svc.ValidateRequest(req) - if err == nil { - t.Error("ValidateRequest() expected error for non-allowlisted host without signature") - } -} - -func TestService_Get_NonAllowlistedHost_ValidSignature(t *testing.T) { - t.Parallel() - - signingKey := testSigningKey - svc, fixtures := SetupTestService(t, WithSigningKey(signingKey)) + svc, fixtures := SetupTestService(t) ctx := context.Background() req := &imagerequest.Request{ @@ -105,17 +79,6 @@ func TestService_Get_NonAllowlistedHost_ValidSignature(t *testing.T) { }, } - // Generate a valid signature - signer := signature.New(signingKey) - req.Expires = time.Now().Add(time.Hour) - req.Signature = signer.Sign(req) - - // Should pass validation - err := svc.ValidateRequest(req) - if err != nil { - t.Errorf("ValidateRequest() error = %v", err) - } - // Should fetch successfully resp, err := svc.Get(ctx, req) if err != nil { @@ -134,244 +97,6 @@ func TestService_Get_NonAllowlistedHost_ValidSignature(t *testing.T) { } } -func TestService_Get_NonAllowlistedHost_ExpiredSignature(t *testing.T) { - t.Parallel() - - signingKey := testSigningKey - svc, fixtures := SetupTestService(t, WithSigningKey(signingKey)) - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: fixtures.OtherHost, - Path: testPathUpload, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - // Generate an expired signature - signer := signature.New(signingKey) - req.Expires = time.Now().Add(-time.Hour) // Already expired - req.Signature = signer.Sign(req) - - // Should fail validation - err := svc.ValidateRequest(req) - if err == nil { - t.Error("ValidateRequest() expected error for expired signature") - } -} - -func TestService_Get_NonAllowlistedHost_InvalidSignature(t *testing.T) { - t.Parallel() - - signingKey := testSigningKey - svc, fixtures := SetupTestService(t, WithSigningKey(signingKey)) - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: fixtures.OtherHost, - Path: testPathUpload, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - // Set an invalid signature - req.Signature = "invalid-signature" - req.Expires = time.Now().Add(time.Hour) - - // Should fail validation - err := svc.ValidateRequest(req) - if err == nil { - t.Error("ValidateRequest() expected error for invalid signature") - } -} - -// TestService_ValidateRequest_SignatureExactHostMatch verifies that -// ValidateRequest enforces exact host matching for signatures. A -// signature for one host must not verify for a different host, even -// if they share a domain suffix. -func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) { - t.Parallel() - - signingKey := "test-signing-key-must-be-32-chars" - svc, _ := SetupTestService(t, - WithSigningKey(signingKey), - WithNoAllowlist(), - ) - - signer := signature.New(signingKey) - - // Sign a request for "cdn.example.com" - signedReq := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: testHostCDN, - Path: testPathCat, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - Expires: time.Now().Add(time.Hour), - } - signedReq.Signature = signer.Sign(signedReq) - - // The original request should pass validation - t.Run("exact host passes", func(t *testing.T) { - t.Parallel() - - err := svc.ValidateRequest(signedReq) - if err != nil { - t.Errorf("ValidateRequest() exact host failed: %v", err) - } - }) - - // Try to reuse the signature with different hosts - tests := []struct { - name string - host string - }{ - {"parent domain", testHostExample}, - {"sibling subdomain", "images.example.com"}, - {"deeper subdomain", "a.cdn.example.com"}, - {"evil suffix domain", "cdn.example.com.evil.com"}, - {"prefixed host", "evilcdn.example.com"}, - } - - for _, tt := range tests { - t.Run(tt.name+" rejected", func(t *testing.T) { - t.Parallel() - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: tt.host, - Path: signedReq.Source.Path, - Query: signedReq.Source.Query, - }, - Transform: signedReq.Transform, - Expires: signedReq.Expires, - Signature: signedReq.Signature, - } - - err := svc.ValidateRequest(req) - if err == nil { - t.Errorf( - "ValidateRequest() should reject signature for host %q (signed for %q)", - tt.host, signedReq.Source.Host) - } - }) - } -} - -// TestService_ValidateRequest_HostWithPortNeedsSignature verifies that a host -// followed by a port, or by a colon with no port, matches no allowlist entry, -// so that a request for it without a signature is refused, while the same -// host with no port is allowlisted. -func TestService_ValidateRequest_HostWithPortNeedsSignature(t *testing.T) { - t.Parallel() - - svc, _ := SetupTestService(t, - WithSigningKey(testSigningKey), - WithAllowlist(testHostCDN, "203.0.113.5", "2001:db8::1"), - ) - - tests := []struct { - host string - allowlisted bool - }{ - {testHostCDN, true}, - {"cdn.example.com:8443", false}, - {"cdn.example.com:443", false}, - {"cdn.example.com:", false}, - {"203.0.113.5", true}, - {"203.0.113.5:8080", false}, - {"[2001:db8::1]", true}, - {"[2001:db8::1]:8443", false}, - } - - for _, tt := range tests { - t.Run(tt.host, func(t *testing.T) { - t.Parallel() - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: tt.host, - Path: testPathCat, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - err := svc.ValidateRequest(req) - t.Logf("ValidateRequest() for host %q with no signature: %v", - tt.host, err) - - if allowlisted := err == nil; allowlisted != tt.allowlisted { - t.Errorf("host %q served without a signature = %v, want %v", - tt.host, allowlisted, tt.allowlisted) - } - }) - } -} - -// TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature verifies that -// an allowlisted IPv6 address written without brackets and followed by a -// port, or by a colon with no port, needs a signature. Go's URL parser, which -// the fetcher uses, reads 2001:db8::1:8443 as the host 2001:db8::1 with the -// port 8443. -func TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature( - t *testing.T, -) { - t.Parallel() - - svc, _ := SetupTestService(t, - WithSigningKey(testSigningKey), - WithAllowlist("2001:db8::1"), - ) - - for _, host := range []string{"2001:db8::1:8443", "2001:db8::1:"} { - t.Run(host, func(t *testing.T) { - t.Parallel() - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: host, - Path: testPathCat, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - err := svc.ValidateRequest(req) - t.Logf("ValidateRequest() for host %q with no signature: %v", - host, err) - - if err == nil { - t.Errorf("host %q served without a signature, want refused", - host) - } - }) - } -} - func TestService_Get_InvalidFile(t *testing.T) { t.Parallel() @@ -692,33 +417,6 @@ func TestService_Get_DifferentSizes(t *testing.T) { } } -func TestService_ValidateRequest_NoSigningKey(t *testing.T) { - t.Parallel() - - // Service with no signing key - all non-allowlisted requests should fail - svc, fixtures := SetupTestService(t, WithNoAllowlist()) - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: fixtures.OtherHost, - Path: testPathUpload, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - } - - err := svc.ValidateRequest(req) - if err == nil { - t.Error( - "ValidateRequest() expected error when no signing key and host not allowlisted", - ) - } -} - func TestService_Get_ContextCancellation(t *testing.T) { t.Parallel() diff --git a/internal/imgcache/stats_internal_test.go b/internal/imgcache/stats_internal_test.go index 6a90fbe..6e4912a 100644 --- a/internal/imgcache/stats_internal_test.go +++ b/internal/imgcache/stats_internal_test.go @@ -93,7 +93,7 @@ type fakeUpstream struct { } func (u *fakeUpstream) Fetch( - context.Context, string, + context.Context, imagerequest.Source, ) (*httpfetcher.FetchResult, error) { return &httpfetcher.FetchResult{ Content: io.NopCloser(u), diff --git a/internal/imgcache/testutil_internal_test.go b/internal/imgcache/testutil_internal_test.go index 31c2c79..ec44fb3 100644 --- a/internal/imgcache/testutil_internal_test.go +++ b/internal/imgcache/testutil_internal_test.go @@ -31,11 +31,11 @@ const ( // TestFixtures contains paths to test files in the mock filesystem. type TestFixtures struct { // Valid image files - GoodHostJPEG string // allowlisted host, valid JPEG - GoodHostPNG string // allowlisted host, valid PNG - GoodHostGIF string // allowlisted host, valid GIF - OtherHostJPEG string // non-allowlisted host, valid JPEG - OtherHostPNG string // non-allowlisted host, valid PNG + GoodHostJPEG string // valid JPEG on GoodHost + GoodHostPNG string // valid PNG on GoodHost + GoodHostGIF string // valid GIF on GoodHost + OtherHostJPEG string // valid JPEG on OtherHost + OtherHostPNG string // valid PNG on OtherHost // Invalid/edge case files InvalidFile string // file with wrong magic bytes @@ -43,8 +43,8 @@ type TestFixtures struct { TextFile string // text file masquerading as image // Hostnames - GoodHost string // allowlisted hostname - OtherHost string // non-allowlisted hostname + GoodHost string // a host the mock fetcher serves + OtherHost string // a second host the mock fetcher serves } // DefaultFixtures returns the standard test fixture paths. @@ -161,22 +161,11 @@ func generateTestGIF(t *testing.T, width, height int, c color.Color) []byte { } // SetupTestService creates a Service with mock fetcher for testing. -func SetupTestService( - t *testing.T, opts ...TestServiceOption, -) (*Service, *TestFixtures) { +func SetupTestService(t *testing.T) (*Service, *TestFixtures) { t.Helper() mockFS, fixtures := NewTestFS(t) - cfg := &testServiceConfig{ - allowlist: []string{fixtures.GoodHost}, - signingKey: "test-signing-key-must-be-32-chars", - } - - for _, opt := range opts { - opt(cfg) - } - // Create temp directory for cache tmpDir := t.TempDir() @@ -199,8 +188,6 @@ func SetupTestService( MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 2, }), - SigningKey: cfg.signingKey, - Allowlist: cfg.allowlist, MaxResponseSize: 50 << 20, }) if err != nil { @@ -228,32 +215,3 @@ func setupServiceTestDB(t *testing.T) *sql.DB { return db } - -type testServiceConfig struct { - allowlist []string - signingKey string -} - -// TestServiceOption configures the test service. -type TestServiceOption func(*testServiceConfig) - -// WithAllowlist sets the allowlist for the test service. -func WithAllowlist(hosts ...string) TestServiceOption { - return func(c *testServiceConfig) { - c.allowlist = hosts - } -} - -// WithSigningKey sets the signing key for the test service. -func WithSigningKey(key string) TestServiceOption { - return func(c *testServiceConfig) { - c.signingKey = key - } -} - -// WithNoAllowlist removes all allowlisted hosts. -func WithNoAllowlist() TestServiceOption { - return func(c *testServiceConfig) { - c.allowlist = nil - } -} diff --git a/internal/providers/providers.go b/internal/providers/providers.go index b098b4e..7d2a951 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -212,21 +212,15 @@ type imageServiceParams struct { } // newImageService builds the image service from the cache, the fetcher -// and the processor, with the signing key, the allowlist and the upstream -// settings of the config. +// and the processor, with the largest upstream response the config allows. func newImageService( _ fx.Lifecycle, params imageServiceParams, ) (*imgcache.Service, error) { - cfg := params.Config - return imgcache.NewService(&imgcache.ServiceConfig{ Cache: params.Cache, Fetcher: params.Fetcher, Processor: params.Processor, - SigningKey: cfg.SigningKey, - Allowlist: cfg.AllowlistHosts, - AllowHTTP: cfg.AllowHTTP, - MaxResponseSize: cfg.UpstreamMaxResponseSize, + MaxResponseSize: params.Config.UpstreamMaxResponseSize, Logger: params.Logger.Get(), }) } diff --git a/internal/server/computed_side_internal_test.go b/internal/server/computed_side_internal_test.go index ad81740..95fb696 100644 --- a/internal/server/computed_side_internal_test.go +++ b/internal/server/computed_side_internal_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imagerequest" @@ -76,7 +77,7 @@ func TestImageProxyRefusesComputedSideAboveMaximum(t *testing.T) { Size: imagerequest.Size{Width: tt.width, Height: tt.height}, Format: imagerequest.FormatJPEG, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("encurl Generate: %v", err) } diff --git a/internal/server/fit_outside_internal_test.go b/internal/server/fit_outside_internal_test.go index cbc6a80..1c095fd 100644 --- a/internal/server/fit_outside_internal_test.go +++ b/internal/server/fit_outside_internal_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imagerequest" @@ -66,7 +67,7 @@ func TestImageProxyRefusesFitOutsideAboveMaximum(t *testing.T) { Format: imagerequest.FormatJPEG, FitMode: imagerequest.FitOutside, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("encurl Generate: %v", err) } diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index 550b8e5..68676d8 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -524,8 +524,7 @@ func signedImageURL(t *testing.T, sourceHost string) string { Quality: 85, FitMode: imagerequest.FitCover, }, - Expires: expires, - }) + }, expires) // Go's percent-encoding of a URL path writes the brackets of the IPv6 // host as %5B and %5D. @@ -549,7 +548,7 @@ func encryptedImageURL(t *testing.T, sourceHost string) string { token, err := generator.Generate(&imagerequest.Request{ Source: imagerequest.Source{Host: sourceHost, Path: ipv6SourcePath}, Transform: imagerequest.Transform{Format: imagerequest.FormatPNG}, - }) + }, time.Time{}) if err != nil { t.Fatalf("encurl Generate: %v", err) } diff --git a/internal/server/svg_as_avif_internal_test.go b/internal/server/svg_as_avif_internal_test.go index af75b24..0993c16 100644 --- a/internal/server/svg_as_avif_internal_test.go +++ b/internal/server/svg_as_avif_internal_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imagerequest" @@ -45,7 +46,7 @@ func TestImageProxyRefusesSVGServedAsAVIF(t *testing.T) { Size: imagerequest.Size{Width: 32, Height: 24}, Format: format, }, - }) + }, time.Time{}) if err != nil { t.Fatalf("encurl Generate: %v", err) } diff --git a/internal/signature/escaping_test.go b/internal/signature/escaping_test.go index 032df24..30d9603 100644 --- a/internal/signature/escaping_test.go +++ b/internal/signature/escaping_test.go @@ -28,20 +28,20 @@ func TestSigner_Verify_ColonCannotMovePathQueryBoundary(t *testing.T) { Quality: 85, FitMode: testFitCover, }, - Expires: time.Now().Add(1 * time.Hour), } - signed.Signature = signer.Sign(signed) + expires := time.Now().Add(1 * time.Hour) + sig := signer.Sign(signed, expires) replayed := *signed replayed.Source.Path = "/a" replayed.Source.Query = "b:" t.Logf("signature for path %q query %q: %s", - signed.Source.Path, signed.Source.Query, signed.Signature) + signed.Source.Path, signed.Source.Query, sig) t.Logf("signature for path %q query %q: %s", - replayed.Source.Path, replayed.Source.Query, signer.Sign(&replayed)) + replayed.Source.Path, replayed.Source.Query, signer.Sign(&replayed, expires)) - err := signer.Verify(&replayed) + err := signer.Verify(&replayed, sig, expires) if !errors.Is(err, signature.ErrInvalid) { t.Errorf("Verify() of path %q query %q with the signature for "+ "path %q = %v, want %v", @@ -106,10 +106,9 @@ func TestSigner_EscapedGoldenVectors(t *testing.T) { Quality: 85, FitMode: testFitCover, }, - Expires: time.Unix(goldenExpiresUnix, 0), } - gotSignature := signer.Sign(req) + gotSignature := signer.Sign(req, time.Unix(goldenExpiresUnix, 0)) t.Logf("Sign() of path %q query %q = %s", tt.path, tt.query, gotSignature) if gotSignature != tt.wantSignature { diff --git a/internal/signature/golden_test.go b/internal/signature/golden_test.go index f3d6e16..9447c9b 100644 --- a/internal/signature/golden_test.go +++ b/internal/signature/golden_test.go @@ -19,8 +19,8 @@ type goldenVector struct { name string req imagerequest.Request // wantSignature is the exact base64url (RFC 4648 URL-safe, - // padded) HMAC-SHA256 signature for the request with Expires - // set to goldenExpiresUnix. + // padded) HMAC-SHA256 signature for the request expiring at + // goldenExpiresUnix. wantSignature string } @@ -124,9 +124,8 @@ func TestSigner_GoldenVectors(t *testing.T) { t.Parallel() signReq := tt.req - signReq.Expires = time.Unix(goldenExpiresUnix, 0) - gotSignature := signer.Sign(&signReq) + gotSignature := signer.Sign(&signReq, time.Unix(goldenExpiresUnix, 0)) if gotSignature != tt.wantSignature { t.Errorf("Sign() = %q, want %q (signed byte format changed?)", gotSignature, tt.wantSignature) diff --git a/internal/signature/quality_fit_test.go b/internal/signature/quality_fit_test.go index 8821b7b..ff27efa 100644 --- a/internal/signature/quality_fit_test.go +++ b/internal/signature/quality_fit_test.go @@ -9,10 +9,12 @@ import ( "sneak.berlin/go/pixa/internal/signature" ) -// signedQualityFitRequest returns a request signed for quality 85 and fit -// mode "cover", the effective defaults the handler applies before -// verification. -func signedQualityFitRequest(signer *signature.Signer) *imagerequest.Request { +// signedQualityFitRequest returns a request, its signature for quality 85 and +// fit mode "cover", the effective defaults the handler applies before +// verification, and the expiry it was signed with. +func signedQualityFitRequest( + signer *signature.Signer, +) (*imagerequest.Request, string, time.Time) { req := &imagerequest.Request{ Source: imagerequest.Source{ Host: testHost, @@ -24,11 +26,10 @@ func signedQualityFitRequest(signer *signature.Signer) *imagerequest.Request { Quality: 85, FitMode: testFitCover, }, - Expires: time.Now().Add(1 * time.Hour), } - req.Signature = signer.Sign(req) + expires := time.Now().Add(1 * time.Hour) - return req + return req, signer.Sign(req, expires), expires } // TestSigner_Verify_QualityAndFitAreSigned proves that quality and fit are @@ -60,10 +61,10 @@ func TestSigner_Verify_QualityAndFitAreSigned(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - req := signedQualityFitRequest(signer) + req, sig, expires := signedQualityFitRequest(signer) tt.tamper(req) - err := signer.Verify(req) + err := signer.Verify(req, sig, expires) if !errors.Is(err, signature.ErrInvalid) { t.Errorf("Verify() = %v, want %v", err, signature.ErrInvalid) } diff --git a/internal/signature/signature.go b/internal/signature/signature.go index 6982756..6ec50aa 100644 --- a/internal/signature/signature.go +++ b/internal/signature/signature.go @@ -34,14 +34,14 @@ func New(secretKey string) *Signer { } } -// Sign generates an HMAC-SHA256 signature for the given request. -// The signature covers: host + path + query + width + height + format + -// expiration + quality + fit. The quality and fit mode are the effective -// values the request resolves to: callers pass the defaults when the request -// omits those parameters, so an omitted quality or fit mode signs +// Sign generates an HMAC-SHA256 signature for the given request, expiring at +// expires. The signature covers: host + path + query + width + height + +// format + expiration + quality + fit. The quality and fit mode are the +// effective values the request resolves to: callers pass the defaults when +// the request omits those parameters, so an omitted quality or fit mode signs // identically to that same value stated explicitly. -func (s *Signer) Sign(req *imagerequest.Request) string { - data := s.buildSignatureData(req) +func (s *Signer) Sign(req *imagerequest.Request, expires time.Time) string { + data := s.buildSignatureData(req, expires) mac := hmac.New(sha256.New, s.secretKey) mac.Write([]byte(data)) sig := mac.Sum(nil) @@ -49,28 +49,31 @@ func (s *Signer) Sign(req *imagerequest.Request) string { return base64.URLEncoding.EncodeToString(sig) } -// Verify checks if the signature on the request is valid and not expired. +// Verify checks that sig is the signature of the request expiring at expires, +// and that expires has not passed. // Signatures are exact-match only: every component of the signed data // (host, path, query, dimensions, format, expiration, quality, fit) must // match exactly. // No suffix matching, wildcard matching, or partial matching is supported. // A signature for "cdn.example.com" will NOT verify for "example.com" or // "other.cdn.example.com", and vice versa. -func (s *Signer) Verify(req *imagerequest.Request) error { +func (s *Signer) Verify( + req *imagerequest.Request, sig string, expires time.Time, +) error { // Check expiration first - if req.Expires.IsZero() { + if expires.IsZero() { return ErrMissingExpiration } - if time.Now().After(req.Expires) { + if time.Now().After(expires) { return ErrExpired } // Compute expected signature - expected := s.Sign(req) + expected := s.Sign(req, expires) // Constant-time comparison to prevent timing attacks - if !hmac.Equal([]byte(req.Signature), []byte(expected)) { + if !hmac.Equal([]byte(sig), []byte(expected)) { return ErrInvalid } @@ -85,7 +88,9 @@ func (s *Signer) Verify(req *imagerequest.Request) error { // expansion is performed. Quality and fit are the effective transform // values, so replaying a signed URL with a different quality or fit mode // fails verification. -func (s *Signer) buildSignatureData(req *imagerequest.Request) string { +func (s *Signer) buildSignatureData( + req *imagerequest.Request, expires time.Time, +) string { return fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d:%d:%s", req.Source.Host, escapePercentAndColon(req.Source.Path), @@ -93,7 +98,7 @@ func (s *Signer) buildSignatureData(req *imagerequest.Request) string { req.Transform.Size.Width, req.Transform.Size.Height, req.Transform.Format, - req.Expires.Unix(), + expires.Unix(), req.Transform.Quality, req.Transform.FitMode, ) diff --git a/internal/signature/signature_test.go b/internal/signature/signature_test.go index dfe08a2..b8b2c75 100644 --- a/internal/signature/signature_test.go +++ b/internal/signature/signature_test.go @@ -34,11 +34,11 @@ func TestSigner_Sign(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Unix(1704067200, 0), // Fixed timestamp for reproducibility } + expires := time.Unix(1704067200, 0) // Fixed timestamp for reproducibility - sig1 := signer.Sign(req) - sig2 := signer.Sign(req) + sig1 := signer.Sign(req, expires) + sig2 := signer.Sign(req, expires) // Same input should produce same signature if sig1 != sig2 { @@ -62,18 +62,17 @@ func TestSigner_Sign(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Unix(1704067200, 0), } - sig3 := signer.Sign(req2) + sig3 := signer.Sign(req2, expires) if sig1 == sig3 { t.Error("Sign() produced same signature for different input") } } -// validVerifyRequest returns a fully-populated request that verifies -// successfully once signed. -func validVerifyRequest() *imagerequest.Request { +// validVerifyRequest returns a fully-populated request, and an expiry an hour +// ahead, that verify successfully once signed. +func validVerifyRequest() (*imagerequest.Request, time.Time) { return &imagerequest.Request{ Source: imagerequest.Source{ Host: testHost, @@ -83,13 +82,13 @@ func validVerifyRequest() *imagerequest.Request { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Now().Add(1 * time.Hour), - } + }, time.Now().Add(1 * time.Hour) } type verifyCase struct { - name string - setup func() *imagerequest.Request + name string + // setup returns the request, the signature and the expiry to verify + setup func() (*imagerequest.Request, string, time.Time) wantErr error } @@ -97,54 +96,49 @@ func verifyCases(signer *signature.Signer) []verifyCase { return []verifyCase{ { name: "valid signature", - setup: func() *imagerequest.Request { - req := validVerifyRequest() - req.Signature = signer.Sign(req) + setup: func() (*imagerequest.Request, string, time.Time) { + req, expires := validVerifyRequest() - return req + return req, signer.Sign(req, expires), expires }, wantErr: nil, }, { name: "expired signature", - setup: func() *imagerequest.Request { - req := validVerifyRequest() - req.Expires = time.Now().Add(-1 * time.Hour) - req.Signature = signer.Sign(req) + setup: func() (*imagerequest.Request, string, time.Time) { + req, _ := validVerifyRequest() + expires := time.Now().Add(-1 * time.Hour) - return req + return req, signer.Sign(req, expires), expires }, wantErr: signature.ErrExpired, }, { name: "invalid signature", - setup: func() *imagerequest.Request { - req := validVerifyRequest() - req.Signature = "invalid-signature" + setup: func() (*imagerequest.Request, string, time.Time) { + req, expires := validVerifyRequest() - return req + return req, "invalid-signature", expires }, wantErr: signature.ErrInvalid, }, { name: "missing expiration", - setup: func() *imagerequest.Request { - req := validVerifyRequest() - req.Expires = time.Time{} - req.Signature = "some-signature" + setup: func() (*imagerequest.Request, string, time.Time) { + req, _ := validVerifyRequest() - return req + return req, "some-signature", time.Time{} }, wantErr: signature.ErrMissingExpiration, }, { name: "tampered request", - setup: func() *imagerequest.Request { - req := validVerifyRequest() - req.Signature = signer.Sign(req) + setup: func() (*imagerequest.Request, string, time.Time) { + req, expires := validVerifyRequest() + sig := signer.Sign(req, expires) req.Source.Path = "/photos/secret.jpg" - return req + return req, sig, expires }, wantErr: signature.ErrInvalid, }, @@ -160,8 +154,8 @@ func TestSigner_Verify(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - req := tt.setup() - err := signer.Verify(req) + req, sig, expires := tt.setup() + err := signer.Verify(req, sig, expires) if tt.wantErr == nil { if err != nil { @@ -254,8 +248,9 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { signer := signature.New("test-secret-key") - // Base request that we'll sign, then tamper with individual fields. - baseReq := func() *imagerequest.Request { + // Base request that we'll sign, then tamper with individual fields; it + // is returned with its signature and expiry. + baseReq := func() (*imagerequest.Request, string, time.Time) { req := &imagerequest.Request{ Source: imagerequest.Source{ Host: testHost, @@ -266,21 +261,20 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Now().Add(1 * time.Hour), } - req.Signature = signer.Sign(req) + expires := time.Now().Add(1 * time.Hour) - return req + return req, signer.Sign(req, expires), expires } for _, tt := range exactMatchTamperCases() { t.Run(tt.name, func(t *testing.T) { t.Parallel() - req := baseReq() + req, sig, expires := baseReq() tt.tamper(req) - err := signer.Verify(req) + err := signer.Verify(req, sig, expires) if !errors.Is(err, signature.ErrInvalid) { t.Errorf("Verify() = %v, want %v", err, signature.ErrInvalid) } @@ -291,9 +285,9 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { t.Run("unmodified request passes", func(t *testing.T) { t.Parallel() - req := baseReq() + req, sig, expires := baseReq() - err := signer.Verify(req) + err := signer.Verify(req, sig, expires) if err != nil { t.Errorf("Verify() unmodified request failed: %v", err) } @@ -329,10 +323,9 @@ func TestSigner_Sign_ExactHostInData(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Unix(1704067200, 0), } - sig := signer.Sign(req) + sig := signer.Sign(req, time.Unix(1704067200, 0)) if existing, ok := sigs[sig]; ok { t.Errorf("hosts %q and %q produced the same signature", existing, host) } @@ -356,20 +349,20 @@ func TestSigner_DifferentKeys(t *testing.T) { Size: imagerequest.Size{Width: 800, Height: 600}, Format: testFormatWebP, }, - Expires: time.Now().Add(1 * time.Hour), } + expires := time.Now().Add(1 * time.Hour) // Sign with key 1 - req.Signature = signer1.Sign(req) + sig := signer1.Sign(req, expires) // Verify with key 1 should succeed - err := signer1.Verify(req) + err := signer1.Verify(req, sig, expires) if err != nil { t.Errorf("Verify() with same key failed: %v", err) } // Verify with key 2 should fail - err = signer2.Verify(req) + err = signer2.Verify(req, sig, expires) if !errors.Is(err, signature.ErrInvalid) { t.Errorf("Verify() with different key should fail, got: %v", err) } @@ -395,12 +388,11 @@ func TestSign_ReadmeQueryExample(t *testing.T) { Quality: 85, FitMode: testFitCover, }, - Expires: time.Unix(1704067200, 0), } const wantSig = "37aFa9GSVCZM1kRY9SzMOk2fOg_M7Wt5gKNR4xM-FYA=" - gotSig := signer.Sign(&req) + gotSig := signer.Sign(&req, time.Unix(1704067200, 0)) if gotSig != wantSig { t.Errorf("Sign() = %q, want %q", gotSig, wantSig) }