ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Key the client address on the connection, not a header the caller sends

Browse Source 
client_ip() read the first X-Forwarded-For hop unconditionally. The
login rate limit and the account lockout are keyed on that value and the
audit log records it, so rotating the header per request bought an
unbounded budget and an untraceable trail.

This walked around TRUSTED_PROXY_COUNT, which exists to decide exactly
this. The app leaves ProxyFix off by default and says why, ten lines of
comment away: ProxyFix trusts X-Forwarded-*, a client can send those
itself, and the login limiter is keyed on the address. Then client_ip
read the header anyway, so the stated protection never applied.

Resolution is now from remote_addr, which ProxyFix has already filled
from the header wherever a proxy is genuinely configured. Both cases end
at the same trustworthy source, and the port still comes off because ARR
forwards clientip:port and ProxyFix passes it through verbatim.

geenforce and tools had each hand-rolled the same mistake for their own
limiters. Rather than duplicate security-relevant logic a third time,
client_ip and strip_port join the shopdb.api surface (contract 0.28.0).
geenforce's _trusted_client_ip, which already refused the raw header for
its IP allowlist, is untouched and was the model for this.

The stability table was also missing rows for 0.26.0 and 0.27.0.

INSTALL-WINDOWS-IIS.md never mentioned TRUSTED_PROXY_COUNT, so a site
installed from it runs behind IIS with the setting at 0: every request
looks like it came from IIS, the whole site shares one rate-limit
bucket, and the audit log records the proxy for every action. It now
says so, and says not to set it when nothing is in front.

Three test modules simulated separate callers by setting
X-Forwarded-For, which is the header a real attacker rotates to escape
the limit. One of them, test_client_ip_prefers_forwarded_first_hop_
stripped, asserted the defect outright. The suite could not have caught
this. They now drive REMOTE_ADDR, with a regression guard that forging
the header buys nothing.

Scope: product
(cherry picked from commit 7260244b7e)
This commit is contained in:
cproudlock
2026-08-31 07:02:17 -04:00
parent d1a7907a2c
commit ccd8221adf
10 changed files with 212 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
CHANGELOG.md
View File

@@ -154,6 +154,19 @@ ADR-007 and ADR-002.
refresh token that minted new ones for thirty days. `/api/auth/refresh` and
the API-token path already checked `isactive`; the loader now does too.
- **The login rate limit could be reset by a header the caller writes.**
`client_ip()` read the first `X-Forwarded-For` hop unconditionally, so
rotating it per request bought an unbounded budget, and every audit row
recorded the forged address. This walked around `TRUSTED_PROXY_COUNT`, which
exists to decide whether a proxy is really in front: the app deliberately
leaves ProxyFix off by default for exactly this reason, and then read the
header anyway. Resolution is now from `remote_addr`, which ProxyFix has
already filled from the header where a proxy is configured. Two plugins had
hand-rolled the same mistake for their own limiters and now share the helper,
which is on `shopdb.api` so the third one does not. **A site behind IIS must
set `TRUSTED_PROXY_COUNT`** or its whole floor shares one rate-limit bucket;
INSTALL-WINDOWS-IIS.md now says so.
- **The published site's API documentation page had no spec to show.**
`shopdb/core/api/docs.py` reads `docs/openapi.json` at runtime to serve
`/api/docs`, and publication excluded the whole of `docs/` because the prose

3
docs/CONTRACT-STABILITY.md
View File

@@ -45,6 +45,9 @@ Recorded in the comment block in `shopdb/__init__.py`:
| 0.23.0 | Added `mint_service_token`, `list_service_tokens` and `revoke_service_tokens` so a plugin can provision, list and revoke a single-scope managed service token without importing core token internals. The endpoints plugin uses them for a UI-managed, regenerable agent key scoped to `collector.ingest.endpoints` alone - narrow on purpose, so rotating or revoking it cannot disturb the computers or GE-Enforce collectors, and revocation is an instant kill switch on agent ingest | additive surface (minor) |
| 0.24.0 | Added `collector_authorized(pluginname)` so a plugin can gate an agent-facing route that lives OUTSIDE the generic collector blueprint on the same per-plugin key and kill switch the ingest route uses. The endpoints command channel needs it: the agent long-polls `/api/endpoints/commands/next` and posts results back, and those are agent callbacks rather than JWT routes. Without it a plugin would reach into core collector internals to authorize them | additive surface (minor) |
| 0.25.0 | Added `caller_holds(permission)`, which answers whether the current caller holds a permission WITHOUT refusing the request - the complement to `require_permission` for a route that serves everyone but should only offer what the caller can act on. The endpoints services panel is the case: it emitted Start/Stop row actions to every reader, because the emission never consulted the caller. Clicking them was always refused server-side, so nothing was exploitable, but offering a read-only viewer controls over a shop-floor PC is the wrong signal, and it becomes a real problem once a site grants that panel to guests. Guests are included in the answer, since a signed-out visitor holds whatever the guest role was granted | additive surface (minor) |
| 0.26.0 | Dashboard cards may declare `shortname`. The dashboard merged its per-source cards into one worst-first queue, and a card's `title` is written as a heading ("Endpoints with crashes or shutdowns") which as a row badge ran wider than the hostname it qualified. Optional, defaults to the title, so a plugin that does not declare one is unaffected | additive field (minor) |
| 0.27.0 | Added `Script` so a plugin can read the ShopDB-hosted script library that backs endpoint `run_script` delivery without importing core model internals. The endpoints plugin resolves a delivered script by name and serves its body by digest to the agent | additive surface (minor) |
| 0.28.0 | Added `client_ip` and `strip_port` so a plugin does not hand-roll caller-IP extraction. Two plugins had, and both read the first `X-Forwarded-For` hop, which the caller supplies - so a rate limit keyed on it was defeated by rotating the header. The helper resolves from `remote_addr`, which ProxyFix has already filled from the header when `TRUSTED_PROXY_COUNT` says a proxy is really in front | additive surface (minor) |
The source comment block documents 0.3.0, 0.4.0, 0.6.0, 0.7.0, 0.9.0, 0.10.0,
0.11.0, 0.16.0, 0.18.0 and 0.19.0; 0.12.0 through 0.15.0 and 0.17.0 are recorded

16
docs/INSTALL-WINDOWS-IIS.md
View File

@@ -88,12 +88,28 @@ SECRET_KEY=<64+ random chars>
JWT_SECRET_KEY=<another 64+ random chars>
DATABASE_URL=mysql+pymysql://shopdb:CHANGE_ME@<mysql-host>:3306/shopdb_flask?charset=utf8mb4
CORS_ORIGINS=http://<the site's own hostname-or-ip:port>
TRUSTED_PROXY_COUNT=1
```
Generate a key: `venv\Scripts\python -c "import secrets;print(secrets.token_urlsafe(64))"`.
Production **refuses to boot** if any of `SECRET_KEY`, `JWT_SECRET_KEY`,
`DATABASE_URL`, `CORS_ORIGINS` is missing or a dev default.
**`TRUSTED_PROXY_COUNT` matters on this install and nowhere else.** IIS is a
reverse proxy, so the app sees IIS's connection rather than the browser's. Set
it to the number of proxies actually in front (1 for a single IIS), and the app
enables ProxyFix, which resolves the real client address, scheme and host from
the `X-Forwarded-*` headers IIS sets.
Leave it at 0 behind IIS and the app is not wrong about who the client is so
much as blind to it: every request looks like it came from IIS itself. The login
rate limit is keyed on the client address, so the whole site shares one bucket,
and the audit log records the proxy for every action.
Do NOT set it when nothing is in front of the app. ProxyFix trusts those headers,
and a caller can send them - which is the reason it is off by default rather
than always on.
---
## 5. Preflight (catch problems before installing)

16
plugins/geenforce/api/routes.py
View File

@@ -21,7 +21,7 @@ from sqlalchemy.exc import IntegrityError
from shopdb.api import (
db, cache, success_response, error_response, ErrorCodes, require_permission,
require_any_permission,
authorized_service_token, Setting, Application,
authorized_service_token, Setting, Application, client_ip,
)
SHAREROOT_SETTING = 'geenforce_share_root'
@@ -207,12 +207,14 @@ PAYLOAD_DOWNLOAD_RATELIMIT_WINDOW_SECONDS = 60
def _client_ip():
"""Caller IP for rate limiting, honoring the first X-Forwarded-For hop
(mirrors shopdb.core.api.auth._login_ip)."""
forwarded = request.headers.get('X-Forwarded-For')
if forwarded:
return forwarded.split(',')[0].strip()
return request.remote_addr or 'unknown'
"""Caller IP for rate limiting.
Uses the shared helper, which resolves from remote_addr. The raw
X-Forwarded-For hop is client-supplied, so keying a rate limit on it lets a
caller rotate the header and get an unbounded budget - the same reasoning
_trusted_client_ip() above already applies to the allowlist.
"""
return client_ip(request) or 'unknown'
def _payload_max_bytes():

7
plugins/tools/api/routes.py
View File

@@ -37,7 +37,7 @@ from flask import current_app
from flask_jwt_extended import jwt_required, get_jwt_identity, current_user
from shopdb.api import (
db, cache, Setting,
db, cache, Setting, client_ip,
success_response, error_response, ErrorCodes,
)
@@ -117,8 +117,9 @@ def _guest_ratelimited():
person has more addresses - but it stops an open endpoint being an easy
place to park data or fill a disk.
"""
address = (request.headers.get('X-Forwarded-For', '').split(',')[0].strip()
or request.remote_addr or 'unknown')
# Not the raw X-Forwarded-For hop: the caller sends that, so rotating it
# gives an unbounded budget and the limit stops being a limit.
address = client_ip(request) or 'unknown'
bucket = int(_utcnow().timestamp()) // GUEST_CREATE_WINDOW_SECONDS
key = f'securesendguest:{address}:{bucket}'
used = cache.get(key) or 0

8
shopdb/api/__init__.py
View File

@@ -104,6 +104,12 @@ def collector_authorized(pluginname: str):
# as script in this origin - see shopdb/utils/uploads.py.
from shopdb.utils.uploads import send_upload, harden_upload_response
# Caller IP for rate limiting and audit. On the surface so a plugin does not
# reimplement it: the obvious hand-rolled version reads X-Forwarded-For, which
# the caller supplies, so a rate limit keyed on it can be rotated away to
# nothing. See shopdb/utils/clientip.py.
from shopdb.utils.clientip import client_ip, strip_port
# Import-mode helpers: preserve legacy timestamps during a bulk data import
from shopdb.utils.import_mode import (
apply_import_timestamps,
@@ -346,6 +352,8 @@ __all__ = [
# Serving uploads
'send_upload',
'harden_upload_response',
'client_ip',
'strip_port',
# Import-mode helpers
'apply_import_timestamps',
'import_mode_active',

32
shopdb/utils/clientip.py
View File

@@ -6,6 +6,23 @@ every connection - so it must be stripped before the value is stored (audit),
matched (dashboard IP fallback), or used as a rate-limit key (login). Left in,
it makes every request look like a different client.
X-Forwarded-For is NOT read here. A client can send that header itself, so
trusting it unconditionally hands any caller a free identity: the login rate
limit and the account lockout are keyed on this value, and the audit log
records it. Reading it directly also walked around TRUSTED_PROXY_COUNT, which
exists precisely to decide whether a proxy is really in front (see the ProxyFix
block in shopdb/__init__.py).
The trusted case is already handled upstream. When TRUSTED_PROXY_COUNT is set,
ProxyFix resolves X-Forwarded-For into remote_addr before the request reaches a
view, so remote_addr is the real client. When it is unset, remote_addr is the
peer that actually connected. Either way remote_addr is the trustworthy source,
which is why this module only strips the port from it.
A site behind IIS or nginx MUST set TRUSTED_PROXY_COUNT (1 for a single proxy),
or every request will key on the proxy's own address and the per-client rate
limit becomes one shared bucket.
Port stripping is IPv6-aware: a bracketed [2001:db8::1]:443 loses its port; a
bare 2001:db8::1 (many colons, no brackets) is returned untouched.
"""
@@ -26,14 +43,11 @@ def strip_port(ipaddress):
def client_ip(request_obj):
"""First-hop client IP behind one proxy, port stripped.
"""Client IP, port stripped, from the connecting peer.
Honors the first X-Forwarded-For hop, else remote_addr. Returns None only
when neither is present (callers that need a non-null key coerce it).
Resolved from remote_addr, which ProxyFix has already rewritten from
X-Forwarded-For when TRUSTED_PROXY_COUNT says a proxy is really in front.
Returns None only when remote_addr is absent (callers that need a non-null
key coerce it).
"""
forwarded = request_obj.headers.get('X-Forwarded-For')
if forwarded:
raw = forwarded.split(',')[0].strip()
else:
raw = request_obj.remote_addr
return strip_port(raw)
return strip_port(request_obj.remote_addr)

11
tests/test_core/test_auth_ratelimit.py
View File

@@ -32,11 +32,18 @@ def ratelimit_on(app):
def _bad_login(client, ip):
"""Attempt a login for a non-existent user from a given source IP."""
"""Attempt a login for a non-existent user from a given source IP.
The caller is set through REMOTE_ADDR, the peer that actually connected,
because that is what the limiter keys on. It used to be sent as
X-Forwarded-For, which meant these tests changed identity using the very
header a real attacker would rotate to escape the limit - so the suite
could not have told the difference.
"""
return client.post(
'/api/auth/login',
json={'username': 'ghost', 'password': 'wrong'},
headers={'X-Forwarded-For': ip},
environ_base={'REMOTE_ADDR': ip},
)

98
tests/test_core/test_clientip.py
View File

@@ -1,4 +1,12 @@
"""strip_port / client_ip normalization (ARR forwards clientip:port)."""
"""client_ip: which address the app treats as the caller, and port normalization.
The answer is remote_addr. X-Forwarded-For is written by the caller, and the
login rate limit, the account lockout and the audit log all key on this value,
so honoring the header let a caller rotate it for an unbounded budget and an
untraceable trail. Where a proxy really is in front, TRUSTED_PROXY_COUNT turns
on ProxyFix and the header arrives already resolved into remote_addr - ports
included, because ARR forwards clientip:port.
"""
from shopdb.utils.clientip import strip_port, client_ip
@@ -34,12 +42,94 @@ class _FakeRequest:
self.remote_addr = remote_addr
def test_client_ip_prefers_forwarded_first_hop_stripped():
request_obj = _FakeRequest(headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'},
remote_addr='127.0.0.1')
def test_client_ip_ignores_the_forwarded_header():
"""The caller sends X-Forwarded-For, so it cannot decide who the caller is.
This test used to assert the opposite. The login rate limit, the account
lockout and the audit log all key on client_ip, so honoring the header let
a caller rotate it for an unbounded budget and an untraceable trail. Where
a proxy really is in front, TRUSTED_PROXY_COUNT turns on ProxyFix and the
header reaches us already resolved into remote_addr.
"""
request_obj = _FakeRequest(
headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'},
remote_addr='127.0.0.1')
assert client_ip(request_obj) == '127.0.0.1'
def test_client_ip_strips_a_port_proxyfix_passed_through():
"""ARR writes clientip:port into the header and ProxyFix copies it verbatim
into remote_addr, so the port still has to come off."""
request_obj = _FakeRequest(headers={}, remote_addr='10.0.0.5:51000')
assert client_ip(request_obj) == '10.0.0.5'
def test_client_ip_falls_back_to_remote_addr_stripped():
request_obj = _FakeRequest(headers={}, remote_addr='192.168.1.9:60000')
assert client_ip(request_obj) == '192.168.1.9'
# ---------------------------------------------------------------------------
# Through a real request, and through the login limiter that keys on this.
# ---------------------------------------------------------------------------
def test_the_forwarded_header_does_not_decide_the_client_ip(app):
from shopdb.utils.clientip import client_ip
with app.test_request_context(
'/', headers={'X-Forwarded-For': '10.0.0.99'},
environ_base={'REMOTE_ADDR': '203.0.113.7'}):
from flask import request
assert client_ip(request) == '203.0.113.7'
def test_the_peer_address_decides_the_client_ip(app):
from shopdb.utils.clientip import client_ip
with app.test_request_context(
'/', environ_base={'REMOTE_ADDR': '203.0.113.8'}):
from flask import request
assert client_ip(request) == '203.0.113.8'
def test_a_forwarded_port_is_still_stripped(app):
"""IIS ARR writes "clientip:port" and ProxyFix passes it through verbatim,
so the port has to come off remote_addr or every request looks like a new
client."""
from shopdb.utils.clientip import client_ip
with app.test_request_context(
'/', environ_base={'REMOTE_ADDR': '203.0.113.9:54321'}):
from flask import request
assert client_ip(request) == '203.0.113.9'
def test_rotating_the_forwarded_header_does_not_reset_the_login_limiter(app,
client,
db):
"""The budget follows the connection, not a header the caller writes.
Sprayed across many usernames on purpose: the per-account lockout bounds
guessing at ONE account, so the IP limiter is the only thing standing
between a caller and unlimited attempts spread across the directory. That
is what rotating the header used to defeat.
"""
caller = {'REMOTE_ADDR': '203.0.113.50'}
previous = dict(app.config)
app.config['AUTH_RATELIMIT_ENABLED'] = True
app.config['AUTH_RATELIMIT_MAX'] = 3
try:
seen = set()
for hop in range(10):
response = client.post(
'/api/auth/login',
headers={'X-Forwarded-For': '198.51.100.{}'.format(hop)},
environ_base=caller,
json={'username': 'nobody{}'.format(hop), 'password': 'wrong'})
seen.add(response.status_code)
if response.status_code == 429:
break
finally:
app.config['AUTH_RATELIMIT_ENABLED'] = previous.get(
'AUTH_RATELIMIT_ENABLED', False)
app.config['AUTH_RATELIMIT_MAX'] = previous.get('AUTH_RATELIMIT_MAX', 30)
assert 429 in seen, 'rotating X-Forwarded-For bought an unbounded budget'

40
tests/test_plugins/test_geenforce_payload_hardening.py
View File

@@ -107,21 +107,47 @@ def test_payload_download_is_rate_limited(client, app, auth_headers):
def test_rate_limit_is_per_ip(client, app, auth_headers):
"""A second caller (different X-Forwarded-For) has its own budget: one IP
being throttled must not throttle everyone."""
"""A second caller has its own budget: one IP being throttled must not
throttle everyone.
Callers are distinguished by REMOTE_ADDR, the peer that actually connected,
which is what ProxyFix fills from X-Forwarded-For when a proxy is really in
front of this instance.
"""
sha = _store(app, b'per-ip blob')
key = _fetch_key(client, auth_headers)
hot = {**key, 'X-Forwarded-For': '203.0.113.202'}
cool = {**key, 'X-Forwarded-For': '203.0.113.203'}
hot = {'REMOTE_ADDR': '203.0.113.202'}
cool = {'REMOTE_ADDR': '203.0.113.203'}
with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1,
GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300):
assert client.get(f'/api/geenforce/payload/{sha}',
headers=hot).status_code == 200
headers=key, environ_base=hot).status_code == 200
assert client.get(f'/api/geenforce/payload/{sha}',
headers=hot).status_code == 429
headers=key, environ_base=hot).status_code == 429
# Fresh IP still gets its first request through.
assert client.get(f'/api/geenforce/payload/{sha}',
headers=cool).status_code == 200
headers=key, environ_base=cool).status_code == 200
def test_rate_limit_cannot_be_reset_by_forging_a_forwarded_header(
client, app, auth_headers):
"""The budget follows the connection, not a header the caller writes.
The limiter used to key on the first X-Forwarded-For hop, so rotating that
header per request bought an unbounded budget and the limit stopped being
a limit.
"""
sha = _store(app, b'forged header blob')
key = _fetch_key(client, auth_headers)
caller = {'REMOTE_ADDR': '203.0.113.210'}
with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1,
GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300):
assert client.get(f'/api/geenforce/payload/{sha}', headers=key,
environ_base=caller).status_code == 200
for hop in ('198.51.100.1', '198.51.100.2', '198.51.100.3'):
spoofed = {**key, 'X-Forwarded-For': hop}
assert client.get(f'/api/geenforce/payload/{sha}', headers=spoofed,
environ_base=caller).status_code == 429
def test_rate_limit_can_be_disabled(client, app, auth_headers):