From 28f65b1e82959fdd10c8bf54fa7be87483a51d35 Mon Sep 17 00:00:00 2001 From: Thomas de Jong Date: Fri, 29 May 2026 07:16:36 +0200 Subject: [PATCH 1/5] fix: fail closed on unexpected source errors --- extractor.go | 2 +- extractor_orchestration_test.go | 35 +++++++++++ resolver.go | 10 +++- resolver_test.go | 101 ++++++++++++++++++++++++++++++++ source_chain_extract.go | 5 +- source_chain_extract_test.go | 45 ++++++++++++++ source_execution.go | 24 -------- 7 files changed, 194 insertions(+), 28 deletions(-) diff --git a/extractor.go b/extractor.go index eca407c..fd73974 100644 --- a/extractor.go +++ b/extractor.go @@ -134,7 +134,7 @@ func (e *extractor) extractRequestView(r requestView) (Extraction, error) { return result, nil } - if sourceIsTerminalError(err) { + if !errors.Is(err, ErrSourceUnavailable) { if !result.Source.valid() { result.Source = source.source } diff --git a/extractor_orchestration_test.go b/extractor_orchestration_test.go index 2374079..1cd529f 100644 --- a/extractor_orchestration_test.go +++ b/extractor_orchestration_test.go @@ -25,6 +25,41 @@ func TestExtract_AllSourcesUnavailableReturnsLastSource(t *testing.T) { } } +func TestExtract_UnknownSourceErrorIsTerminal(t *testing.T) { + unexpectedErr := errors.New("unexpected extractor failure") + extractor := &extractor{ + config: &config{sourceHeaderKeys: []string{"X-Forwarded-For"}}, + sources: []configuredSource{ + { + source: SourceXForwardedFor, + chain: chainExtractor{policy: chainPolicy{ + headerName: "X-Forwarded-For", + parseValues: func([]string) ([]string, error) { + return nil, unexpectedErr + }, + }}, + }, + { + source: SourceRemoteAddr, + remote: remoteAddrExtractor{}, + }, + }, + } + + result, err := extractor.Extract(&http.Request{ + RemoteAddr: "8.8.8.8:8080", + Header: http.Header{ + "X-Forwarded-For": {"1.1.1.1"}, + }, + }) + if !errors.Is(err, unexpectedErr) { + t.Fatalf("error = %v, want %v", err, unexpectedErr) + } + if got, want := result.Source, SourceXForwardedFor; got != want { + t.Fatalf("source = %q, want %q", got, want) + } +} + func TestExtractInput_ContextCanceledBeforeFallbackSource(t *testing.T) { cfg := defaultOptions() cfg.TrustedProxyPrefixes = LoopbackProxyPrefixes() diff --git a/resolver.go b/resolver.go index 0e0fc63..7e47468 100644 --- a/resolver.go +++ b/resolver.go @@ -55,6 +55,8 @@ const ( FallbackReasonSourceUnavailable // FallbackReasonInvalidIP indicates fallback was used because the extracted IP failed client-IP policy. FallbackReasonInvalidIP + // FallbackReasonUnknown indicates fallback was used because strict extraction returned an unclassified error. + FallbackReasonUnknown ) // String returns the stable label for r. @@ -70,6 +72,8 @@ func (r FallbackReason) String() string { return "source_unavailable" case FallbackReasonInvalidIP: return "invalid_ip" + case FallbackReasonUnknown: + return "unknown" default: return "unknown" } @@ -291,6 +295,8 @@ func (r *Resolver) applyFallback(remoteAddr string, fallback Fallback, strictErr func fallbackReasonFromError(err error) FallbackReason { switch ClassifyError(err) { + case ResultSuccess, ResultCanceled, ResultFallback: + return FallbackReasonNone case ResultUntrusted: return FallbackReasonUntrustedProxy case ResultMalformed: @@ -299,8 +305,10 @@ func fallbackReasonFromError(err error) FallbackReason { return FallbackReasonSourceUnavailable case ResultInvalid: return FallbackReasonInvalidIP + case ResultUnknown: + return FallbackReasonUnknown default: - return FallbackReasonNone + return FallbackReasonUnknown } } diff --git a/resolver_test.go b/resolver_test.go index 4a1b58b..2275b16 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -146,6 +146,7 @@ func TestFallbackReasonString(t *testing.T) { {reason: FallbackReasonMalformedHeader, want: "malformed_header"}, {reason: FallbackReasonSourceUnavailable, want: "source_unavailable"}, {reason: FallbackReasonInvalidIP, want: "invalid_ip"}, + {reason: FallbackReasonUnknown, want: "unknown"}, {reason: FallbackReason(255), want: "unknown"}, } @@ -156,6 +157,72 @@ func TestFallbackReasonString(t *testing.T) { } } +func TestFallbackReasonFromError(t *testing.T) { + tests := []struct { + name string + err error + want FallbackReason + }{ + {name: "nil", want: FallbackReasonNone}, + {name: "canceled", err: context.Canceled, want: FallbackReasonNone}, + {name: "unknown", err: errors.New("unexpected extractor failure"), want: FallbackReasonUnknown}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := fallbackReasonFromError(tt.err); got != tt.want { + t.Fatalf("fallbackReasonFromError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestResolveOperational_UnknownStrictErrorUsesUnknownFallbackReason(t *testing.T) { + unexpectedErr := errors.New("unexpected extractor failure") + resolver := &Resolver{extractor: &extractor{ + config: &config{ + sourceHeaderKeys: []string{"X-Forwarded-For"}, + logger: noopLogger{}, + loggerNoop: true, + observer: noopObserver{}, + }, + sources: []configuredSource{ + { + source: SourceXForwardedFor, + chain: chainExtractor{policy: chainPolicy{ + headerName: "X-Forwarded-For", + parseValues: func([]string) ([]string, error) { + return nil, unexpectedErr + }, + }}, + }, + }, + }} + + result := resolver.ResolveOperational(&http.Request{ + RemoteAddr: "8.8.8.8:443", + Header: http.Header{ + "X-Forwarded-For": {"1.1.1.1"}, + }, + }, RemoteAddrFallback()) + + if result.Err != nil { + t.Fatalf("ResolveOperational() error = %v, want nil", result.Err) + } + if !result.FallbackUsed { + t.Fatal("ResolveOperational() FallbackUsed = false, want true") + } + if got, want := result.FallbackReason, FallbackReasonUnknown; got != want { + t.Fatalf("FallbackReason = %v, want %v", got, want) + } + if got, want := result.Source, SourceRemoteAddr; got != want { + t.Fatalf("Source = %q, want %q", got, want) + } + if got, want := result.IP, netip.MustParseAddr("8.8.8.8"); got != want { + t.Fatalf("IP = %v, want %v", got, want) + } +} + func TestResolveOperational_ContextErrorsRemainTerminal(t *testing.T) { resolver, err := New() if err != nil { @@ -265,3 +332,37 @@ func TestResolveInputAndHeaders(t *testing.T) { t.Fatalf("ResolveHeaders() IP = %v, want 8.8.8.8", result.IP) } } + +func TestResolveInputOperational_RemoteAddrFallback(t *testing.T) { + resolver, err := New( + WithTrustedProxies(netip.MustParsePrefix("127.0.0.0/8")), + WithSources(SourceXForwardedFor), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + + input := Input{ + RemoteAddr: "203.0.113.10:443", + Headers: http.Header{"X-Forwarded-For": {"8.8.8.8"}}, + } + result := resolver.ResolveInputOperational(input, RemoteAddrFallback()) + if result.Err != nil { + t.Fatalf("ResolveInputOperational() error = %v, want nil", result.Err) + } + if !result.FallbackUsed { + t.Fatal("ResolveInputOperational() FallbackUsed = false, want true") + } + if got, want := result.FallbackReason, FallbackReasonUntrustedProxy; got != want { + t.Fatalf("FallbackReason = %v, want %v", got, want) + } + if got, want := result.Source, SourceRemoteAddr; got != want { + t.Fatalf("Source = %q, want %q", got, want) + } + if got, want := result.IP, netip.MustParseAddr("203.0.113.10"); got != want { + t.Fatalf("IP = %v, want %v", got, want) + } + if got, want := result.Classify(), ResultFallback; got != want { + t.Fatalf("Classify() = %v, want %v", got, want) + } +} diff --git a/source_chain_extract.go b/source_chain_extract.go index b1e982b..a8e5500 100644 --- a/source_chain_extract.go +++ b/source_chain_extract.go @@ -2,6 +2,7 @@ package clientip import ( "net/netip" + "slices" "strings" ) @@ -88,9 +89,9 @@ func (e chainExtractor) extract(req requestView, source Source) (Extraction, *ex // DebugInfo is success-only so failed requests do not carry extra // parsed attacker-controlled chain details through Result by default. result.DebugInfo = &ChainDebugInfo{ - FullChain: parts, + FullChain: slices.Clone(parts), ClientIndex: analysis.ClientIndex, - TrustedIndices: analysis.TrustedIndices, + TrustedIndices: slices.Clone(analysis.TrustedIndices), } } diff --git a/source_chain_extract_test.go b/source_chain_extract_test.go index 3c693a0..40e0c11 100644 --- a/source_chain_extract_test.go +++ b/source_chain_extract_test.go @@ -3,6 +3,7 @@ package clientip import ( "errors" "net/netip" + "slices" "strings" "testing" ) @@ -73,6 +74,50 @@ func TestChainExtractor_SingleValidValue(t *testing.T) { } } +func TestChainExtractor_DebugInfoClonesChainMetadata(t *testing.T) { + parsedValues := []string{"8.8.8.8", "10.0.0.1"} + trustedCIDR := netip.MustParsePrefix("10.0.0.0/8") + ext := chainExtractor{policy: chainPolicy{ + headerName: "X-Forwarded-For", + parseValues: func([]string) ([]string, error) { + return parsedValues, nil + }, + parseClientIP: parseIP, + trustedProxy: proxyPolicy{ + TrustedProxyCIDRs: []netip.Prefix{trustedCIDR}, + TrustedProxyMatch: newPrefixMatcher([]netip.Prefix{trustedCIDR}), + }, + selection: RightmostUntrustedIP, + collectDebugInfo: true, + }} + + req := requestView{ + remoteAddrValue: "10.0.0.2:443", + headerMap: map[string][]string{ + "X-Forwarded-For": {"8.8.8.8, 10.0.0.1"}, + }, + } + + result, failure, err := ext.extract(req, SourceXForwardedFor) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if failure != nil { + t.Fatalf("unexpected failure: %+v", failure) + } + if result.DebugInfo == nil { + t.Fatal("DebugInfo = nil") + } + if got, want := result.DebugInfo.TrustedIndices, []int{1}; !slices.Equal(got, want) { + t.Fatalf("DebugInfo.TrustedIndices = %v, want %v", got, want) + } + + parsedValues[0] = "1.1.1.1" + if got, want := result.DebugInfo.FullChain[0], "8.8.8.8"; got != want { + t.Fatalf("DebugInfo.FullChain[0] = %q, want %q", got, want) + } +} + func TestChainExtractor_ChainWithTrustedProxies(t *testing.T) { trustedCIDR := netip.MustParsePrefix("10.0.0.0/8") ext := chainExtractor{policy: chainPolicy{ diff --git a/source_execution.go b/source_execution.go index 7a56cd1..a822bda 100644 --- a/source_execution.go +++ b/source_execution.go @@ -1,7 +1,6 @@ package clientip import ( - "context" "errors" "fmt" ) @@ -56,29 +55,6 @@ func (e *extractor) extractRemoteAddrSource(r requestView, source *configuredSou return result, nil } -// sourceIsTerminalError defines when extractor orchestration may continue to -// the next configured source. Only source-unavailable is a normal miss; trust, -// syntax, policy, chain-length, invalid-IP, and context failures stop -// resolution. -func sourceIsTerminalError(err error) bool { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return true - } - - if errors.Is(err, ErrSourceUnavailable) { - return false - } - - return errors.Is(err, ErrInvalidIP) || - errors.Is(err, ErrMultipleSingleIPHeaders) || - errors.Is(err, ErrUntrustedProxy) || - errors.Is(err, ErrNoTrustedProxies) || - errors.Is(err, ErrTooFewTrustedProxies) || - errors.Is(err, ErrTooManyTrustedProxies) || - errors.Is(err, ErrChainTooLong) || - errors.Is(err, ErrInvalidForwardedHeader) -} - // logSecurityWarning emits stable base attributes with the request context so // caller-provided loggers can attach trace/span metadata. func (e *extractor) logSecurityWarning(r requestView, source Source, event, msg string, attrs ...any) { From 95dc0cbfcaeb0cd1c7b71eefabc763331173b6ff Mon Sep 17 00:00:00 2001 From: Thomas de Jong Date: Fri, 29 May 2026 07:23:17 +0200 Subject: [PATCH 2/5] ci: add codeql, coverage, fuzz and scorecard workflows --- .github/workflows/ci.yml | 30 ++++---- .github/workflows/coverage.yml | 126 ++++++++++++++++++++++++++++++++ .github/workflows/fuzz.yml | 33 +++++++++ .github/workflows/scorecard.yml | 29 ++++++++ .github/workflows/security.yml | 4 +- Justfile | 2 +- 6 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 .github/workflows/coverage.yml create mode 100644 .github/workflows/fuzz.yml create mode 100644 .github/workflows/scorecard.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b4c58b0..84768fb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,8 +22,8 @@ jobs: - go-version: "1.26.x" go-label: go126 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: ${{ matrix.go-version }} cache: true @@ -36,7 +36,7 @@ jobs: - run: go test -race ./... - run: go test -coverprofile=coverage.out ./... - run: go tool cover -func=coverage.out - - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: coverage-root-${{ matrix.go-label }} path: coverage.out @@ -53,8 +53,8 @@ jobs: - go-version: "1.26.x" go-label: go126 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: ${{ matrix.go-version }} cache: true @@ -67,7 +67,7 @@ jobs: - run: GOWORK=off go -C observe/prometheus test -race ./... - run: GOWORK=off go -C observe/prometheus test -coverprofile=../../coverage-prometheus.out ./... - run: GOWORK=off go -C observe/prometheus tool cover -func=../../coverage-prometheus.out - - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: coverage-prometheus-${{ matrix.go-label }} path: coverage-prometheus.out @@ -76,8 +76,8 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: "1.26.x" cache: true @@ -86,15 +86,15 @@ jobs: go.sum observe/prometheus/go.mod observe/prometheus/go.sum - - uses: extractions/setup-just@v4 + - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4 - run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.8.0 - run: just lint actionlint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: "1.26.x" cache: true @@ -103,7 +103,7 @@ jobs: go.sum observe/prometheus/go.mod observe/prometheus/go.sum - - uses: extractions/setup-just@v4 + - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4 - run: go install github.com/rhysd/actionlint/cmd/actionlint@v1.7.10 - run: just actionlint @@ -121,10 +121,10 @@ jobs: group: benchmark-compare-${{ github.event.pull_request.number }} cancel-in-progress: true steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 with: fetch-depth: 0 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: "1.26.x" cache: true @@ -179,7 +179,7 @@ jobs: fi } >> "$GITHUB_STEP_SUMMARY" - - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 if: always() with: name: benchmark-compare diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml new file mode 100644 index 0000000..c281752 --- /dev/null +++ b/.github/workflows/coverage.yml @@ -0,0 +1,126 @@ +name: Coverage + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +concurrency: + group: coverage-pages-${{ github.ref }} + cancel-in-progress: true + +jobs: + coverage: + runs-on: ubuntu-latest + env: + GOWORK: off + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 + with: + go-version: "1.26.x" + cache: true + cache-dependency-path: | + go.mod + go.sum + observe/prometheus/go.mod + observe/prometheus/go.sum + + - name: Run coverage + run: | + set -euo pipefail + go test -coverprofile=coverage.out ./... + GOWORK=off go -C observe/prometheus test -coverprofile=../../coverage-prometheus.out ./... + + - name: Build coverage site + run: | + set -euo pipefail + mkdir -p public/coverage + + go tool cover -func=coverage.out | tee public/coverage/root.txt + go tool cover -html=coverage.out -o public/coverage/root.html + GOWORK=off go -C observe/prometheus tool cover -func=../../coverage-prometheus.out | tee public/coverage/prometheus.txt + GOWORK=off go -C observe/prometheus tool cover -html=../../coverage-prometheus.out -o ../../public/coverage/prometheus.html + + root_pct=$(awk '/^total:/ {print $3}' public/coverage/root.txt) + prometheus_pct=$(awk '/^total:/ {print $3}' public/coverage/prometheus.txt) + root_num=${root_pct%%%} + root_int=${root_num%.*} + + color="#e05d44" + if [ "$root_int" -ge 90 ]; then + color="#4c1" + elif [ "$root_int" -ge 75 ]; then + color="#dfb317" + fi + + cat > public/coverage.svg < + + + + + + + + + + + + + coverage + + $root_pct + + + EOF + + cat > public/coverage/index.html < + + + + + clientip coverage + + +

clientip coverage

+

coverage: $root_pct

+
    +
  • Root module: $root_pct (HTML, text)
    • +
  • Prometheus adapter: $prometheus_pct (HTML, text)
    • +
+ + + EOF + + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: coverage-site + path: public/ + retention-days: 14 + + deploy: + if: github.event_name == 'push' + needs: coverage + runs-on: ubuntu-latest + permissions: + pages: write + id-token: write + environment: + name: github-pages + url: ${{ steps.deployment.outputs.page_url }} + steps: + - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7 + with: + name: coverage-site + path: public/ + - uses: actions/configure-pages@45bfe0192ca1faeb007ade9deae92b16b8254a0d # v6 + - uses: actions/upload-pages-artifact@7b1f4a764d45c48632c6b24a0339c27f5614fb0b # v4 + with: + path: public/ + - id: deployment + uses: actions/deploy-pages@d6db90164ac5ed86f2b6aed7e0febac5b3c0c03e # v4 diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml new file mode 100644 index 0000000..71d3b9d --- /dev/null +++ b/.github/workflows/fuzz.yml @@ -0,0 +1,33 @@ +name: Fuzz + +on: + pull_request: + schedule: + - cron: "15 4 * * 1" + workflow_dispatch: + +permissions: + contents: read + +jobs: + fuzz-smoke: + runs-on: ubuntu-latest + env: + GOWORK: off + FUZZTIME: 30s + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 + with: + go-version: "1.26.x" + cache: true + cache-dependency-path: | + go.mod + go.sum + - name: Fuzz parser targets + run: | + set -euo pipefail + go test -run '^$' -fuzz '^FuzzParseIP_RoundTripNormalization$' -fuzztime "$FUZZTIME" . + go test -run '^$' -fuzz '^FuzzParseRemoteAddr_RoundTripNormalization$' -fuzztime "$FUZZTIME" . + go test -run '^$' -fuzz '^FuzzParseXFFValues_ErrorShapeAndOutput$' -fuzztime "$FUZZTIME" . + go test -run '^$' -fuzz '^FuzzParseForwardedValues_ErrorShapeAndOutput$' -fuzztime "$FUZZTIME" . diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml new file mode 100644 index 0000000..6d365f6 --- /dev/null +++ b/.github/workflows/scorecard.yml @@ -0,0 +1,29 @@ +name: Scorecard + +on: + branch_protection_rule: + push: + branches: [main] + schedule: + - cron: "45 3 * * 2" + +permissions: + contents: read + security-events: write + id-token: write + +jobs: + scorecard: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + with: + persist-credentials: false + - uses: ossf/scorecard-action@05b42c624433fc40578a4040d5cf5e36ddca8cde # v2.4.2 + with: + results_file: scorecard-results.sarif + results_format: sarif + publish_results: true + - uses: github/codeql-action/upload-sarif@7211b7c8077ea37d8641b6271f6a365a22a5fbfa # v4 + with: + sarif_file: scorecard-results.sarif diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 7e70e6c..b8f81d5 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -13,8 +13,8 @@ jobs: govulncheck: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 with: go-version: "1.26.x" cache: true diff --git a/Justfile b/Justfile index 3a9af52..ecef221 100644 --- a/Justfile +++ b/Justfile @@ -6,7 +6,7 @@ adapter_gowork := env_var_or_default("CLIENTIP_ADAPTER_GOWORK", "off") default: @just --list -ci: test race coverage lint tidy-check actionlint +ci: test race coverage lint tidy-check actionlint security fmt: gofumpt -extra -w . From 2fbdba15157c126b0a916cfe9d66c4822d18cf63 Mon Sep 17 00:00:00 2001 From: Thomas de Jong Date: Fri, 29 May 2026 07:24:24 +0200 Subject: [PATCH 3/5] docs: add detailed docs and updates to README --- CHANGELOG.md | 13 +++++++ CONTRIBUTING.md | 4 +++ README.md | 73 +++++++------------------------------ docs/release.md | 36 +++++++++++++++++++ docs/threat-model.md | 51 ++++++++++++++++++++++++++ docs/trusted-proxies.md | 79 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 195 insertions(+), 61 deletions(-) create mode 100644 docs/release.md create mode 100644 docs/threat-model.md create mode 100644 docs/trusted-proxies.md diff --git a/CHANGELOG.md b/CHANGELOG.md index f230611..bef6ecd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ The format is based on Keep a Changelog and this project follows Semantic Versio ## [Unreleased] +### Added + +- Added `FallbackReasonUnknown` for operational fallbacks triggered by unclassified strict errors. +- Added threat-model, trusted-proxy configuration, and release checklist documentation. +- Added CodeQL, OpenSSF Scorecard, fuzz smoke, and coverage publishing workflows with actions pinned by commit SHA. + +### Changed + +- README now documents the intended `v0.1.0` compatibility posture and links to generated coverage reports. +- Unexpected extractor errors now fail closed instead of allowing source fallback. +- Chain debug metadata now clones parsed chain slices before storing them on `Result`. +- `just ci` now includes `govulncheck` through the `security` task. + ## [0.0.8] - 2026-05-22 ### Changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8ea1cbe..406f58a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -105,6 +105,10 @@ CLIENTIP_ADAPTER_GOWORK=auto just test - README changes should stay user-facing and concise; detailed contributor guidance belongs here. - Public behavior changes should update `CHANGELOG.md` under `[Unreleased]`. +## Releases + +See [docs/release.md](docs/release.md) for release tagging conventions, including the nested Prometheus adapter module tag format. + ## Pull Requests - Describe the trust-boundary impact of the change, even when it is "none". diff --git a/README.md b/README.md index cc23ea3..1734218 100644 --- a/README.md +++ b/README.md @@ -3,6 +3,7 @@ [![Go Reference](https://pkg.go.dev/badge/github.com/abczzz13/clientip.svg)](https://pkg.go.dev/github.com/abczzz13/clientip) [![CI](https://github.com/abczzz13/clientip/actions/workflows/ci.yml/badge.svg)](https://github.com/abczzz13/clientip/actions/workflows/ci.yml) [![Security](https://github.com/abczzz13/clientip/actions/workflows/security.yml/badge.svg)](https://github.com/abczzz13/clientip/actions/workflows/security.yml) +[![Coverage](https://abczzz13.github.io/clientip/coverage.svg)](https://abczzz13.github.io/clientip/coverage/) [![License](https://img.shields.io/github/license/abczzz13/clientip)](LICENSE) Secure client IP resolution for `net/http` and framework-agnostic request inputs with trusted proxy validation, explicit source modeling, and operational fallback. @@ -19,13 +20,17 @@ Secure client IP resolution for `net/http` and framework-agnostic request inputs - [Common Deployments](#common-deployments) - [Error Handling](#error-handling) - [Presets](#presets) +- [Advanced Proxy Configuration](#advanced-proxy-configuration) - [Observability](#observability) - [Security Rules](#security-rules) +- [Threat Model](#threat-model) - [Contributing](#contributing) ## Stability -This project is pre-`v0.1.0`; public APIs may change before stabilization. +Starting with `v0.1.0`, public APIs are intended to preserve compatibility according to Semantic Versioning. + +Before a `v1.0.0` release, minor-version releases may still add APIs or refine behavior where SemVer allows. ## Install @@ -212,67 +217,9 @@ Generic option presets are available: - `PresetLoopbackReverseProxy()` trusts loopback proxies and uses `X-Forwarded-For`, then `RemoteAddr`. - `PresetVMReverseProxy()` trusts loopback/private proxy ranges and uses `X-Forwarded-For`, then `RemoteAddr`. -### Vendor And Cloud Proxy Ranges - -When your service is behind a load balancer, CDN, or reverse proxy, `RemoteAddr` is usually that proxy, not the original client. The proxy may add headers such as `X-Forwarded-For`, `CF-Connecting-IP`, or other vendor-specific client IP headers. Those headers are plain HTTP headers and are spoofable unless the immediate peer is a proxy you trust to set or append them. - -`WithTrustedProxies` is the allowlist for trusted ingress peers and, for chain headers, trusted forwarding hops. Single-IP headers such as `CF-Connecting-IP` are accepted only when the immediate `RemoteAddr` peer is trusted; chain headers such as `X-Forwarded-For` use the trusted suffix of proxy hops to select the client candidate. Only include ranges that can actually connect to your service. Provider IP ranges change over time, and dynamic range fetching introduces application-specific policy around refresh intervals, startup failure, caching, and regional filtering, so keep the trusted proxy ranges in your own configuration where they can follow your deployment process. - -Recommended workflow: - -- Fetch the ranges from the provider's official source. -- Filter to the product, region, VPC, subnet, load balancer, or CDN edge that can actually connect to your service. -- Parse the CIDRs with `clientip.ParseCIDRs`. -- Pass those prefixes to `clientip.WithTrustedProxies`. -- Refresh the ranges on your own deploy or configuration-management schedule. - -Common provider range sources, useful as starting points before product/service/region filtering: - -- AWS: `https://ip-ranges.amazonaws.com/ip-ranges.json` -- Azure: `https://www.microsoft.com/download/details.aspx?id=56519` -- Google Cloud: `https://www.gstatic.com/ipranges/cloud.json` -- Google Cloud default domains: `https://www.gstatic.com/ipranges/goog.json` -- Cloudflare: `https://www.cloudflare.com/ips-v4` and `https://www.cloudflare.com/ips-v6` -- Fastly: `https://api.fastly.com/public-ip-list` - -Do not treat broad cloud-provider feeds as ready-to-use trusted proxy lists. Some feeds describe public service ranges and may not represent the immediate proxy peers that connect to your application. - -Example configuration shape: - -```go -trustedProxyCIDRs := []string{ - // Replace these documentation prefixes with your filtered proxy CIDRs. - "203.0.113.0/24", - "2001:db8:1234::/48", -} - -trustedProxies, err := clientip.ParseCIDRs(trustedProxyCIDRs...) -if err != nil { - log.Fatal(err) -} -``` - -Store the CIDR strings in your own config after fetching and filtering the provider list. Avoid embedding every published provider range unless every one of those ranges is a valid ingress path to your service. +## Advanced Proxy Configuration -Cloudflare-style configuration should trust `CF-Connecting-IP` only when the immediate peer is in Cloudflare's published CIDRs: - -```go -resolver, err := clientip.New( - clientip.WithTrustedProxies(trustedProxies...), - clientip.WithSources(clientip.HeaderSource("CF-Connecting-IP"), clientip.SourceRemoteAddr), -) -``` - -AWS ALB append mode typically uses `X-Forwarded-For`. Trust the narrowest ingress range that can reach your targets, such as explicit proxy addresses or ALB target subnets protected by security groups. Published AWS public service ranges are usually not the right trust boundary for private ALB-to-target traffic: - -```go -resolver, err := clientip.New( - clientip.WithTrustedProxies(trustedProxies...), - clientip.WithSources(clientip.SourceXForwardedFor, clientip.SourceRemoteAddr), -) -``` - -CloudFront and other CDN-origin configurations follow the same rule: a single-IP header is only trustworthy when the connecting peer is verified as that CDN. If the origin is reachable directly, a client can spoof headers such as `CF-Connecting-IP`, `True-Client-IP`, `Fastly-Client-IP`, or `X-Forwarded-For`; block direct origin access with firewall rules, security groups, or equivalent network policy. +Provider and cloud proxy ranges need application-specific filtering before they are trusted. See [Trusted Proxy Configuration](docs/trusted-proxies.md) for provider range sources, CDN header examples, ALB/X-Forwarded-For guidance, and refresh workflow recommendations. ## Observability @@ -299,6 +246,10 @@ Prometheus support lives in the optional `github.com/abczzz13/clientip/observe/p - Do not use operational fallback for security decisions. - Count-only proxy trust is intentionally unsupported: `WithMinTrustedProxies` / `WithMaxTrustedProxies` validate CIDR-trusted hop counts and do not by themselves make a header source trusted. +## Threat Model + +See [Threat Model](docs/threat-model.md) for security goals, trust assumptions, non-goals, failure behavior, and privacy notes. + ## Contributing See [CONTRIBUTING.md](CONTRIBUTING.md) for local setup, test commands, documentation expectations, and pull request guidance. diff --git a/docs/release.md b/docs/release.md new file mode 100644 index 0000000..6ac0e80 --- /dev/null +++ b/docs/release.md @@ -0,0 +1,36 @@ +# Release Checklist + +## Tags + +This repository contains two Go modules: + +- root module: `github.com/abczzz13/clientip` +- Prometheus adapter module: `github.com/abczzz13/clientip/observe/prometheus` + +Tag the root module with normal semantic-version tags: + +```bash +git tag -a vX.Y.Z -m "release: vX.Y.Z" +git push origin vX.Y.Z +``` + +Tag the nested Prometheus adapter module with a path-prefixed tag: + +```bash +git tag -a observe/prometheus/vX.Y.Z -m "release(prometheus): vX.Y.Z" +git push origin observe/prometheus/vX.Y.Z +``` + +The older `prometheus/v0.0.x` tags belong to the previous adapter module path and should be left in place for historical users. Do not reuse or move published tags. + +## Before Tagging + +1. Ensure `CHANGELOG.md` has a dated release section. +2. Run `just ci` locally when the required tools are installed. +3. Confirm GitHub Actions are green on `main`. +4. Confirm `go list -m -versions github.com/abczzz13/clientip` includes the intended root version after tagging. +5. Confirm `go list -m -versions github.com/abczzz13/clientip/observe/prometheus` includes the intended adapter version after tagging. + +## v0.1 Compatibility Posture + +Starting with `v0.1.0`, public APIs should preserve compatibility according to Semantic Versioning. Breaking API changes should wait for an appropriate SemVer boundary and should be clearly documented in the changelog. diff --git a/docs/threat-model.md b/docs/threat-model.md new file mode 100644 index 0000000..db19e8e --- /dev/null +++ b/docs/threat-model.md @@ -0,0 +1,51 @@ +# Threat Model + +This document describes what `clientip` is designed to protect, what it assumes, and what it deliberately does not try to solve. + +## Security Goals + +`clientip` helps applications choose a client IP address for decisions such as: + +- rate limiting and abuse controls +- ACLs and authorization policy +- audit trails and incident investigation +- security-sensitive logging and metrics labels + +The primary goal is to avoid trusting spoofable forwarding headers unless the request came through a proxy that the application explicitly trusts. + +## Trust Boundary + +`RemoteAddr` is the only source that comes from the direct network peer observed by the HTTP server. Forwarding headers such as `Forwarded`, `X-Forwarded-For`, `X-Real-IP`, `CF-Connecting-IP`, and vendor-specific headers are plain HTTP headers. A client can spoof them unless an upstream proxy removes or rewrites untrusted values before forwarding the request. + +For header-based sources, the application must configure `WithTrustedProxies` with the narrowest CIDR ranges that can actually connect to the service. These are usually load-balancer, reverse-proxy, CDN edge, or private ingress ranges, not broad cloud-provider address feeds. + +## Assumptions + +`clientip` assumes that: + +- configured trusted proxies are controlled by the operator or provider and enforce the expected header behavior +- direct origin access is blocked when a CDN or edge proxy is supposed to be the only ingress path +- the application keeps trusted proxy CIDRs current through its own deployment or configuration process +- framework integrations preserve repeated header-line values in order +- the caller treats `Resolve` results differently from operational fallback results + +## Non-Goals + +`clientip` does not: + +- make arbitrary forwarding headers trustworthy without CIDR-validated trusted proxies +- fetch or refresh provider IP ranges automatically +- implement count-only proxy trust +- authenticate proxies cryptographically +- decide application response status codes or rejection bodies +- anonymize or redact IP addresses in caller logs, metrics, or storage + +## Failure Behavior + +Strict resolution fails closed. Source absence can allow the next configured source to run, but malformed headers, invalid IPs, proxy-trust failures, chain-limit failures, context cancellation, and unexpected extractor errors are terminal. + +Operational fallback is intentionally separate. `ResolveOperational` can return a best-effort value for analytics or logging, but fallback results should not be used for authorization, ACLs, rate-limit identity, or other trust-boundary decisions. + +## Privacy Notes + +IP addresses can be personal data. Avoid logging full addresses unless needed, define retention periods, and consider redaction or aggregation for analytics. `clientip` exposes logging and observation hooks, but privacy policy and storage behavior are application responsibilities. diff --git a/docs/trusted-proxies.md b/docs/trusted-proxies.md new file mode 100644 index 0000000..07f438f --- /dev/null +++ b/docs/trusted-proxies.md @@ -0,0 +1,79 @@ +# Trusted Proxy Configuration + +This guide expands on the README's deployment examples. + +## Core Rule + +Only trust proxy ranges that can actually connect to your application. A broad provider range is usually too large to be a trust boundary. + +Forwarding headers are HTTP headers. They become meaningful only when the immediate peer is a trusted proxy that sets, appends, or sanitizes them according to your deployment contract. + +## Recommended Workflow + +1. Fetch ranges from the provider's official source. +2. Filter to the product, region, VPC, subnet, load balancer, CDN edge, or proxy fleet that can actually reach your service. +3. Store the filtered CIDR strings in your application configuration. +4. Parse them with `clientip.ParseCIDRs`. +5. Pass the resulting prefixes to `clientip.WithTrustedProxies`. +6. Refresh ranges on your deploy or configuration-management schedule. + +```go +trustedProxyCIDRs := []string{ + // Replace these documentation prefixes with your filtered proxy CIDRs. + "203.0.113.0/24", + "2001:db8:1234::/48", +} + +trustedProxies, err := clientip.ParseCIDRs(trustedProxyCIDRs...) +if err != nil { + log.Fatal(err) +} + +resolver, err := clientip.New( + clientip.WithTrustedProxies(trustedProxies...), + clientip.WithSources(clientip.SourceXForwardedFor, clientip.SourceRemoteAddr), +) +``` + +## Common Provider Range Sources + +Use these as starting points before product/service/region filtering: + +- AWS: `https://ip-ranges.amazonaws.com/ip-ranges.json` +- Azure: `https://www.microsoft.com/download/details.aspx?id=56519` +- Google Cloud: `https://www.gstatic.com/ipranges/cloud.json` +- Google Cloud default domains: `https://www.gstatic.com/ipranges/goog.json` +- Cloudflare: `https://www.cloudflare.com/ips-v4` and `https://www.cloudflare.com/ips-v6` +- Fastly: `https://api.fastly.com/public-ip-list` + +Do not treat broad cloud-provider feeds as ready-to-use trusted proxy lists. Some feeds describe public service ranges and may not represent the immediate proxy peers that connect to your application. + +## CDN Single-IP Headers + +Single-IP headers such as `CF-Connecting-IP`, `True-Client-IP`, or `Fastly-Client-IP` are trusted only when the connecting peer is verified as the expected CDN or edge proxy. + +```go +resolver, err := clientip.New( + clientip.WithTrustedProxies(trustedCDNPrefixes...), + clientip.WithSources(clientip.HeaderSource("CF-Connecting-IP"), clientip.SourceRemoteAddr), +) +``` + +If the origin is reachable directly, clients can spoof these headers. Block direct origin access with firewall rules, security groups, private networking, or equivalent network policy. + +## Load Balancers And X-Forwarded-For + +ALBs and reverse proxies commonly append to `X-Forwarded-For`. Trust the narrowest ingress range that can reach your targets, such as explicit proxy addresses or load-balancer target subnets protected by security groups. + +```go +resolver, err := clientip.New( + clientip.WithTrustedProxies(trustedIngressPrefixes...), + clientip.WithSources(clientip.SourceXForwardedFor, clientip.SourceRemoteAddr), +) +``` + +Published cloud public-service ranges are usually not the right trust boundary for private load-balancer-to-target traffic. + +## Count-Only Trust + +`clientip` intentionally does not support count-only proxy trust. `WithMinTrustedProxies` and `WithMaxTrustedProxies` validate how many CIDR-trusted hops were observed; they do not make a header source trusted without `WithTrustedProxies` and a trusted immediate peer. From c72a144a6dda30f9536f77d802b29e5d45a3b09e Mon Sep 17 00:00:00 2001 From: Thomas de Jong Date: Fri, 29 May 2026 07:26:32 +0200 Subject: [PATCH 4/5] release: v0.1.0 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bef6ecd..e073e34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on Keep a Changelog and this project follows Semantic Versio ## [Unreleased] +## [0.1.0] - 2026-05-29 + ### Added - Added `FallbackReasonUnknown` for operational fallbacks triggered by unclassified strict errors. From d373e2fcbf4f22b4688a2c178f890ebbdc1a639d Mon Sep 17 00:00:00 2001 From: Thomas de Jong Date: Fri, 29 May 2026 10:52:46 +0200 Subject: [PATCH 5/5] test: improve fuzzing a bit --- .github/workflows/fuzz.yml | 27 ++++++++++++++++++++----- parse_fuzz_test.go | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index 71d3b9d..99f3b69 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -5,6 +5,15 @@ on: schedule: - cron: "15 4 * * 1" workflow_dispatch: + inputs: + fuzz-depth: + description: "How long to run each fuzz target" + required: true + default: smoke + type: choice + options: + - smoke + - deep permissions: contents: read @@ -14,7 +23,9 @@ jobs: runs-on: ubuntu-latest env: GOWORK: off - FUZZTIME: 30s + PR_FUZZTIME: 30s + DEEP_FUZZTIME: 5m + DEEP_FUZZ_MAX_HEADER_VALUE_LEN: "1048576" steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6 @@ -27,7 +38,13 @@ jobs: - name: Fuzz parser targets run: | set -euo pipefail - go test -run '^$' -fuzz '^FuzzParseIP_RoundTripNormalization$' -fuzztime "$FUZZTIME" . - go test -run '^$' -fuzz '^FuzzParseRemoteAddr_RoundTripNormalization$' -fuzztime "$FUZZTIME" . - go test -run '^$' -fuzz '^FuzzParseXFFValues_ErrorShapeAndOutput$' -fuzztime "$FUZZTIME" . - go test -run '^$' -fuzz '^FuzzParseForwardedValues_ErrorShapeAndOutput$' -fuzztime "$FUZZTIME" . + fuzztime="$PR_FUZZTIME" + if [ "${{ github.event_name }}" = "schedule" ] || [ "${{ inputs.fuzz-depth || 'smoke' }}" = "deep" ]; then + fuzztime="$DEEP_FUZZTIME" + export CLIENTIP_FUZZ_MAX_HEADER_VALUE_LEN="$DEEP_FUZZ_MAX_HEADER_VALUE_LEN" + fi + echo "Running parser fuzz targets for $fuzztime each with max header value length ${CLIENTIP_FUZZ_MAX_HEADER_VALUE_LEN:-default}" + go test -run '^$' -fuzz '^FuzzParseIP_RoundTripNormalization$' -fuzztime "$fuzztime" . + go test -run '^$' -fuzz '^FuzzParseRemoteAddr_RoundTripNormalization$' -fuzztime "$fuzztime" . + go test -run '^$' -fuzz '^FuzzParseXFFValues_ErrorShapeAndOutput$' -fuzztime "$fuzztime" . + go test -run '^$' -fuzz '^FuzzParseForwardedValues_ErrorShapeAndOutput$' -fuzztime "$fuzztime" . diff --git a/parse_fuzz_test.go b/parse_fuzz_test.go index dc79b64..291aa3a 100644 --- a/parse_fuzz_test.go +++ b/parse_fuzz_test.go @@ -2,15 +2,49 @@ package clientip import ( "errors" + "os" + "strconv" "testing" ) +// defaultFuzzMaxHeaderValueLen keeps PR fuzz smoke near common proxy/request +// header limits. Typical defaults range from about 8-16 KiB per field to around +// 60-64 KiB total headers, while Go's net/http default total header cap is 1 +// MiB. Use 64 KiB by default so PR fuzzing still covers realistic high-end +// proxy inputs without spending CI time on oversized strings. Deep fuzzing can +// raise this with CLIENTIP_FUZZ_MAX_HEADER_VALUE_LEN. +const defaultFuzzMaxHeaderValueLen = 64 * 1024 + +var fuzzMaxHeaderValueLen = configuredFuzzMaxHeaderValueLen() + +func configuredFuzzMaxHeaderValueLen() int { + raw := os.Getenv("CLIENTIP_FUZZ_MAX_HEADER_VALUE_LEN") + if raw == "" { + return defaultFuzzMaxHeaderValueLen + } + + n, err := strconv.Atoi(raw) + if err != nil || n <= 0 { + panic("CLIENTIP_FUZZ_MAX_HEADER_VALUE_LEN must be a positive integer byte count") + } + return n +} + +func skipOversizedFuzzInput(t *testing.T, raw string) { + t.Helper() + if len(raw) > fuzzMaxHeaderValueLen { + t.Skipf("skipping oversized fuzz input with %d bytes", len(raw)) + } +} + func FuzzParseIP_RoundTripNormalization(f *testing.F) { for _, seed := range []string{"1.1.1.1", " 1.1.1.1 ", "1.1.1.1:443", "[2606:4700:4700::1]:443", `"1.1.1.1"`, `'1.1.1.1'`, "not-an-ip", ""} { f.Add(seed) } f.Fuzz(func(t *testing.T, raw string) { + skipOversizedFuzzInput(t, raw) + parsed := parseIP(raw) if !parsed.IsValid() { return @@ -33,6 +67,8 @@ func FuzzParseRemoteAddr_RoundTripNormalization(f *testing.F) { } f.Fuzz(func(t *testing.T, raw string) { + skipOversizedFuzzInput(t, raw) + parsed := parseRemoteAddr(raw) if !parsed.IsValid() { return @@ -55,6 +91,8 @@ func FuzzParseXFFValues_ErrorShapeAndOutput(f *testing.F) { } f.Fuzz(func(t *testing.T, raw string) { + skipOversizedFuzzInput(t, raw) + valueSets := [][]string{{raw}, {raw, raw}, {"1.1.1.1", raw}, {raw, "8.8.8.8"}} for _, values := range valueSets { @@ -90,6 +128,8 @@ func FuzzParseForwardedValues_ErrorShapeAndOutput(f *testing.F) { } f.Fuzz(func(t *testing.T, raw string) { + skipOversizedFuzzInput(t, raw) + valueSets := [][]string{{raw}, {raw, raw}, {"for=1.1.1.1", raw}, {raw, "for=8.8.8.8"}} for _, values := range valueSets {