diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..bf1be13 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,4 @@ +# The build context only needs the module file and the Go sources. +* +!go.mod +!*.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index df1b133..e2748eb 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,6 +2,7 @@ name: build on: push: + branches: [main] pull_request: permissions: @@ -19,7 +20,7 @@ jobs: - run: go vet ./... - - run: go test ./... + - run: go test -race -count=1 ./... # Keeps the Dockerfile validated on pull requests, which no longer reach # the publish job. No registry credentials needed. @@ -28,12 +29,25 @@ jobs: context: . push: false + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v7 + with: + go-version: stable + + - uses: golangci/golangci-lint-action@v9 + with: + version: v2.12.2 + publish: # Only pushes to main publish, so pull request runs never receive a token # with packages: write — a branch PR can change this workflow and its code # runs with whatever permissions the run was granted. if: github.event_name == 'push' && github.ref == 'refs/heads/main' - needs: test + needs: [test, lint] runs-on: ubuntu-latest permissions: contents: read diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9fd428c..54d38e3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2,7 +2,8 @@ name: release on: push: - tags: ['v*'] + # Both cases: the existing V1.0 tag would not have matched a 'v*' glob. + tags: ['v*', 'V*'] permissions: contents: read @@ -21,7 +22,7 @@ jobs: - run: go vet ./... - - run: go test ./... + - run: go test -race -count=1 ./... - name: Build run: | @@ -36,6 +37,7 @@ jobs: done cd dist && sha256sum ./*.tar.gz > checksums.txt - - run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes + # --verify-tag refuses to invent a tag if the ref is somehow absent. + - run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes --verify-tag env: GH_TOKEN: ${{ github.token }} diff --git a/.golangci.yml b/.golangci.yml index 1f8dfdb..4faadd5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,14 +1,12 @@ +version: "2" + run: timeout: 3m linters: + # errcheck, govet, ineffassign, staticcheck and unused are enabled by + # default. gosimple was folded into staticcheck, and tenv into usetesting. enable: - - errcheck - - gosimple - - govet - - ineffassign - - staticcheck - - unused - asciicheck - bodyclose - errorlint @@ -17,13 +15,23 @@ linters: - noctx - prealloc - revive - - tenv - unconvert + - usetesting - wastedassign + settings: + errcheck: + # Closing a response body cannot fail in a way the handler can act on. + exclude-functions: + - (io.ReadCloser).Close + exclusions: + rules: + - path: _test\.go + linters: + - errcheck + - gosec + # Test requests are synthetic and never escape the process. + - noctx -issues: - exclude-rules: - - path: _test\.go - linters: - - errcheck - - gosec +formatters: + enable: + - gofmt diff --git a/Dockerfile b/Dockerfile index ed845c4..18438b3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,8 +1,11 @@ FROM golang:1.26-alpine AS builder WORKDIR /build -COPY go.mod main.go ./ -RUN CGO_ENABLED=0 go build -ldflags="-s -w" -o image-proxy . +# Listing sources explicitly meant a new .go file was silently left out of +# the image. .dockerignore keeps the context to go.mod and the sources. +COPY go.mod ./ +COPY *.go ./ +RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o image-proxy . # distroless static: CA certs included, runs as non-root FROM gcr.io/distroless/static:nonroot diff --git a/README.md b/README.md index 8199105..2a1b84c 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,6 @@ [![Go version](https://img.shields.io/badge/Go-1.25-00ADD8?logo=go)](https://go.dev) [![Build](https://github.com/schiz0x00/image-proxy/actions/workflows/build.yml/badge.svg)](https://github.com/schiz0x00/image-proxy/actions/workflows/build.yml) -[![CodeQL](https://github.com/schiz0x00/image-proxy/actions/workflows/codeql.yml/badge.svg)](https://github.com/schiz0x00/image-proxy/actions/workflows/codeql.yml) [![License](https://img.shields.io/github/license/schiz0x00/image-proxy)](LICENSE) [![Release](https://img.shields.io/github/v/release/schiz0x00/image-proxy)](https://github.com/schiz0x00/image-proxy/releases) @@ -23,7 +22,7 @@ docker run -p 8080:8080 image-proxy ## Releases -Pushing a `v*` tag builds static binaries for linux and darwin on amd64 and arm64, and attaches them plus `checksums.txt` to a GitHub release. +Pushing a `v*` or `V*` tag builds static binaries for linux and darwin on amd64 and arm64, and attaches them plus `checksums.txt` to a GitHub release. ## Behavior @@ -53,6 +52,18 @@ Pushing a `v*` tag builds static binaries for linux and darwin on amd64 and arm6 | `ALLOWED_HOSTS` | unset (any public host) | Comma-separated hostname allowlist; subdomains are included | | `TRUSTED_PROXY_HOPS` | `0` | Number of reverse proxies in front; enables `X-Forwarded-For` parsing | +## Development + +```sh +make test # go test -race +make lint # golangci-lint (config in .golangci.yml) +make vet fmt # go vet / go fmt +make coverage # writes coverage.html +make build # local binary +``` + +`make lint` needs [golangci-lint](https://golangci-lint.run) v2; the config uses the v2 schema and will not load under v1. + ## Contributing See [CONTRIBUTING.md](CONTRIBUTING.md). Bug reports and feature requests welcome via [issues](https://github.com/schiz0x00/image-proxy/issues). diff --git a/main.go b/main.go index 799c610..12e8674 100644 --- a/main.go +++ b/main.go @@ -1,3 +1,8 @@ +// Command image-proxy is a stateless streaming image proxy. It fetches an +// image from an upstream URL and streams it back to the caller without +// caching, storing, or transforming it. Fetching attacker-supplied URLs is the +// point of the service, so the SSRF, size, and rate controls in this file are +// the security boundary rather than incidental hardening. package main import ( @@ -35,12 +40,23 @@ const ( var inFlight = make(chan struct{}, maxInFlight) -// redactURL strips query parameters from a URL for safe logging, keeping -// the scheme, host, and path visible. +// redactURL strips query parameters from a URL for safe logging, keeping the +// scheme, host, and path visible. Control characters are removed from whatever +// it returns: the input is caller-supplied, and a bare newline in it would let +// the caller forge log lines. url.ParseRequestURI already rejects such input on +// the request path, so this is defence in depth for the error branch below, +// which returns the string unparsed. func redactURL(rawURL string) string { - u, err := url.Parse(rawURL) + clean := strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f { + return -1 + } + return r + }, rawURL) + + u, err := url.Parse(clean) if err != nil || u.Query().Encode() == "" { - return rawURL + return clean } u.RawQuery = "redacted" return u.String() @@ -88,7 +104,7 @@ func hostAllowed(host string) bool { // the hostname up front cannot prevent DNS rebinding, because the client // resolves the name a second time when it dials. Applies to every hop, // including redirects. -func ssrfControl(network, address string, _ syscall.RawConn) error { +func ssrfControl(_, address string, _ syscall.RawConn) error { if disableSSRFCheck { return nil } @@ -377,13 +393,13 @@ func main() { } } -func healthHandler(w http.ResponseWriter, r *http.Request) { +func healthHandler(w http.ResponseWriter, _ *http.Request) { writeSecurityHeaders(w) w.Header().Set("Content-Type", "text/plain; charset=utf-8") - w.Write([]byte("OK")) + _, _ = w.Write([]byte("OK")) } -func optionsHandler(w http.ResponseWriter, r *http.Request) { +func optionsHandler(w http.ResponseWriter, _ *http.Request) { writeCORS(w) writeSecurityHeaders(w) w.WriteHeader(http.StatusNoContent) @@ -400,6 +416,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { return } if len(imageURL) > maxURLLength { + // #nosec G706 -- only the length is logged, never the URL itself. log.Printf("URL too long: %d bytes", len(imageURL)) http.Error(w, "URL too long", http.StatusBadRequest) return @@ -412,6 +429,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { } if !hostAllowed(u.Host) { + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("Blocked host not in ALLOWED_HOSTS: url=%s", redactURL(imageURL)) http.Error(w, "Blocked: target host is not allowed", http.StatusForbidden) return @@ -419,6 +437,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { if !disableSSRFCheck { if blocked, reason := isBlockedHost(u.Host); blocked { + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("Blocked SSRF attempt: url=%s reason=%s", redactURL(imageURL), reason) http.Error(w, "Blocked: target host is not allowed", http.StatusForbidden) return @@ -436,6 +455,9 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { return } + // #nosec G704 -- fetching a caller-supplied URL is what this service does. + // The target has already passed hostAllowed and isBlockedHost, and the + // dialer re-checks the resolved IP in ssrfControl on every hop. originReq, err := http.NewRequestWithContext(r.Context(), http.MethodGet, imageURL, nil) if err != nil { log.Printf("Failed to create origin request: %v", err) @@ -446,6 +468,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { originReq.Header.Set(k, v) } + // #nosec G704 -- see above; originClient pins the SSRF check to the dial. resp, err := originClient.Do(originReq) if err != nil { status := http.StatusBadGateway @@ -489,6 +512,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("Error streaming response body: %v", err) } + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("%s status=%d bytes=%d duration=%s", redactURL(imageURL), resp.StatusCode, written, time.Since(start)) } diff --git a/main_test.go b/main_test.go index d754a32..86fe20e 100644 --- a/main_test.go +++ b/main_test.go @@ -105,7 +105,7 @@ func TestSecurityHeaders(t *testing.T) { "Referrer-Policy": "no-referrer", } - origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "image/svg+xml") w.Write([]byte(``)) })) @@ -214,7 +214,7 @@ func TestClientIP(t *testing.T) { } func TestOversizedContentLengthRejected(t *testing.T) { - origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "image/png") w.Header().Set("Content-Length", "999999999") // ~1 GB, past the 50 MB cap w.WriteHeader(200) @@ -226,6 +226,21 @@ func TestOversizedContentLengthRejected(t *testing.T) { } } +func TestRedactURL(t *testing.T) { + // Query strings hold tokens and must never reach the log. + if got := redactURL("https://cdn.example/a.png?sig=secret"); got != "https://cdn.example/a.png?redacted" { + t.Errorf("query not redacted: %q", got) + } + // A newline would let a caller forge log lines. + if got := redactURL("https://cdn.example/a.png\n2026-01-01 forged entry"); strings.ContainsAny(got, "\r\n") { + t.Errorf("control characters survived: %q", got) + } + // Unparseable input still gets scrubbed rather than returned as-is. + if got := redactURL("::not a url::\x00\n"); strings.ContainsAny(got, "\r\n\x00") { + t.Errorf("control characters survived the error branch: %q", got) + } +} + func TestIsPrivateIP(t *testing.T) { blocked := []string{ "127.0.0.1", "::1", "0.0.0.0", "::",