Repository navigation
feat: support deriving the client address from a trusted proxy - #58
Merged
Merged
Conversation
The image never enabled mod_remoteip. The module ships in the base image but nothing links it into mods-enabled and no RemoteIP directive is shipped, so Apache attributes every request to the TCP peer. Behind a reverse proxy -- the shape this image is built for, given it already carries OIDCXForwardedHeaders support -- %h, %a and REMOTE_ADDR are the proxy on every line, X-Forwarded-For arrives and is never consulted, and AllowOverride None on the document root leaves no way to correct it from outside the image. Add a remote-ip.conf following the global-server-name.conf idiom: ship the conf and enable it at boot only when configured. HTTPD_GLOBAL_REMOTE_IP_HEADER names the header and defaults to X-Forwarded-For; HTTPD_GLOBAL_REMOTE_IP_INTERNAL_PROXY and HTTPD_GLOBAL_REMOTE_IP_TRUSTED_PROXY declare which peers may set it, and whichever is configured is uncommented before the module is enabled. Presence of a proxy variable is deliberately the enabling condition, with no separate on/off flag. A flag would permit the one configuration that must not exist: the module active with no declared trusted peers. mod_remoteip trusting every peer lets any client set its own apparent address through the header, turning a logging gap into a spoofing primitive, so the trust list being the enabling condition removes that state structurally rather than by documentation. Leaving the variables unset keeps the module unlinked, so existing deployments are unaffected. Verified against the published 0.4.2 image with these files overlaid. With no variables set the module stays out of mods-enabled and %h is the peer, unchanged from today. With an internal proxy covering the peer, %h becomes the X-Forwarded-For value; with a trusted proxy and HEADER set to CF-Connecting-IP it becomes that header's value. With an internal proxy that does not cover the peer a spoofed header is correctly ignored and %h stays the peer. apache2ctl configtest reports Syntax OK with the module enabled, and shellcheck reports nothing beyond the pre-existing SC1091. One side effect is worth recording. mod_remoteip consumes the header it reads, and mod_auth_openidc hashes the left-most X-Forwarded-For value into the OIDC state, so enabling this drops that component from the hash and the state stops being sensitive to a proxy address that differs between the authorization request and the callback. A flow whose X-Forwarded-For changes between the two legs now completes where it previously failed with "calculated state from cookie does not match state parameter passed back in URL". Fixes #57 Signed-off-by: Nathan Klick <nathan@swirldslabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #57.
What this changes
The image never enabled
mod_remoteip. It ships in the base image, but nothing links it intomods-enabledand noRemoteIP*directive is shipped, so Apache attributes every request to the TCP peer —%h,%aandREMOTE_ADDRare all the proxy.X-Forwarded-Forarrives and is never consulted, andAllowOverride Noneon the document root leaves no way to correct it from outside the image.This adds a
remote-ip.conffollowing theglobal-server-name.confidiom already in this image: ship the conf, enable it at boot only when configured.HTTPD_GLOBAL_REMOTE_IP_HEADERX-Forwarded-ForHTTPD_GLOBAL_REMOTE_IP_INTERNAL_PROXYHTTPD_GLOBAL_REMOTE_IP_TRUSTED_PROXYWhichever proxy directive is configured gets uncommented, then
a2enmod remoteipanda2enconf remote-ip.Why there is no on/off flag
Presence of a proxy variable is deliberately the enabling condition. A separate flag would permit the one configuration that must not exist: the module active with no declared trusted peers.
mod_remoteiptrusting every peer lets any client set its own apparent address through the header — that turns a logging gap into a spoofing primitive, strictly worse than the status quo. Making the trust list the enabling condition removes that state structurally rather than by documentation.Backward compatibility
Leaving the variables unset leaves the module unlinked from
mods-enabled, exactly as today. The default is off.Verification
Against the published
0.4.2image with these files overlaid:%hloggedmods-enabledINTERNAL_PROXYcovers peer,X-Forwarded-For: 203.0.113.10203.0.113.10TRUSTED_PROXYcovers peer,HEADER=CF-Connecting-IP198.51.100.5INTERNAL_PROXYdoes not cover peer, spoofed headerapache2ctl configtestreportsSyntax OKwith the module enabled.shellcheckon both scripts reports nothing beyond the pre-existingSC1091for thelogger.shsource.Note the full image build could not be exercised locally —
repro-sources-list.shplus the pinned snapshot repos fail to resolve here. That is pre-existing and unrelated to this change; CI builds it. The overlay approach is the same one used to verify #54 and #56.Side effect worth recording
mod_remoteipconsumes the header it reads.mod_auth_openidchashes the left-mostX-Forwarded-Forvalue into the OIDCstate(oidc_get_browser_state_hash, withOIDCStateInputHeadersdefaulting touser-agentplusx-forwarded-for), so enabling this drops that component from the hash. A flow whoseX-Forwarded-Fordiffers between the authorization request and the callback now completes, where it previously failed withcalculated state from cookie does not match state parameter passed back in URL.That is a real fix for deployments behind a CDN, where the proxy address varies per request, but it is a side effect rather than the intent. Deployments wanting the fingerprint pinned deliberately should set
OIDCStateInputHeaders, which this image does not yet expose.