Repository navigation
fix: trust traefik's X-Forwarded-Proto so pagination links use https - #9
Conversation
TLS ends at traefik, so the app sees plain http and builds next/previous links starting http://. uvicorn already reads X-Forwarded-Proto, but only from 127.0.0.1; traefik connects from a pod IP. Set FORWARDED_ALLOW_IPS to the cluster's pod network.
Same chart shape as CLEANDB-api#28, so the two APIs configure this the same way.
matthewberry
left a comment
There was a problem hiding this comment.
Looks good. Checked:
helm template(afterhelm dependency build) putsFORWARDED_ALLOW_IPSin the right place in all three cases: afterenvFromwithexistingSecretset, afterOED_DB_NAMEwith it unset, and noenv:at all whenextraEnvis empty.- uvicorn 0.34.2 has proxy headers on by default, and
FORWARDED_ALLOW_IPSaccepts a CIDR range (uvicorn/middleware/proxy_headers.py:98). Without the setting it defaults to127.0.0.1(config.py:334). - Nothing in
app/readsrequest.clientorX-Forwarded-*(grepped both). So a pod that spoofs these headers can only change the scheme in generated links and the address in the access log. The tradeoff note in the PR body is accurate.
One thing to check before merging: this PR says mmli2-mmli-oed-api has no automated sync and needs a manual Argo sync. #7 said Argo tracks main with automated sync. One of them is wrong. I couldn't open mmli2's charts/apps/values.yaml to check.
|
#9 is right; #7 was wrong. In mmli2 ( The cluster agrees: 16 minutes after #7 merged, the Deployment still had no So after merging this, someone needs to sync |
Why
The
next/previouslinks in/api/v1/dataresponses (and, since #8, the CSVLinkheader) start withhttp://:TLS ends at traefik, so the app receives plain HTTP, and the links are built from
request.url. This predates #8.uvicorn 0.34.2 (from
uv.lock) already appliesX-Forwarded-Proto, but only from addresses inforwarded_allow_ips, which defaults toFORWARDED_ALLOW_IPSor127.0.0.1(uvicorn/config.py:334). Traefik connects from a pod IP (10.42.4.120in the access log), so the header is ignored.What
Same fix and chart shape as moleculemaker/CLEANDB-api#28:
deployment.yamlrenders a generalcontroller.extraEnvlist ({name, value}entries) in both config modes: afterenvFromwhenexistingSecretis set (as deployed), and after the plaintextOED_DB_*values otherwise.values.yamlsetsFORWARDED_ALLOW_IPS: "10.42.0.0/16"there, mmli2's pod network. Every node'spodCIDRis inside it (10.42.0.0/24through10.42.11.0/24), including the nodes traefik runs on. Unlike CLEANDB-api, this chart has no separate mmli2 values file, so the value goes invalues.yaml.The range is cluster-specific, so it lives in the chart rather than the Dockerfile
CMD. No image rebuild is needed.Trusting the whole pod network means any pod in the cluster, not only traefik, could set
X-Forwarded-ProtoorX-Forwarded-Foron a request it sends to this Service; outside clients can't. Not"*", which would log the firstX-Forwarded-Forentry, one an outside client can set.Testing
helm templaterendersFORWARDED_ALLOW_IPSafterenvFromwithexistingSecretset, afterOED_DB_NAMEwith it unset, and noenv:at all withextraEnv: [].X-Forwarded-Proto: httpsfrom a non-loopback address gives scope schemehttpwithFORWARDED_ALLOW_IPSunset, andhttpswith it set to a range containing that address.X-Forwarded-Proto: https. After deploying,curl -s 'https://openenzymedb-api.platform.moleculemaker.org/api/v1/data?limit=6000&columns=ec' | jq -r .nextshould start withhttps://.Deploying
mmli2-mmli-oed-apihas no automated sync (nosync: trueundermoldb.oedapiin mmli2'scharts/apps/values.yaml), so this needs a manual Argo sync after merging. The env change restarts the pod.