fix: serve security headers from app nginx instead of ingress custom-headers#4186
fix: serve security headers from app nginx instead of ingress custom-headers#4186devthejo wants to merge 3 commits into
Conversation
…headers The frontend, portail-admins and portail-usagers apps relied on the `nginx.ingress.kubernetes.io/custom-headers` annotation to set their security headers (CSP, X-Frame-Options, Referrer-Policy, ...). This couples the apps to the ingress controller honouring that annotation. On the legacy cluster the edge controller was swapped to Traefik, whose nginx-compat provider rejects custom-headers whose names are not in its `globalAllowedResponseHeaders` allowlist — dropping the whole route and returning `503 no available server` even though the pods were healthy. Move the headers into each app's own nginx config (the nginx4spa image supports overriding `/etc/nginx/nginx.conf`) and drop the ingress customHeaders blocks. The apps now emit their security headers themselves, independently of the ingress controller in front of them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
revu-bot
left a comment
revu-bot
left a comment
There was a problem hiding this comment.
Summary
This PR moves security headers from Kubernetes ingress customHeaders annotations into per-app nginx configurations, fixing a 503 regression caused by Traefik's kubernetesingressnginx compatibility provider not honouring arbitrary response headers. The approach is sound and the fix is correct.
Overall quality: Good. The configs are validated, headers are faithfully ported, and the always flag ensures headers are sent on error responses too. A few issues worth addressing:
| File | Lines | Severity | Issue |
|---|---|---|---|
All 3 nginx.conf |
28–35 | IMPORTANT | add_header in server block is silently overridden by any location-level add_header — security headers will be dropped on static assets |
All 3 nginx.conf |
14 | MINOR | error_log declared twice (http + top-level) |
packages/frontend/nginx.conf |
38 | IMPORTANT | Missing Permissions-Policy header — CSP alone doesn't restrict browser feature APIs |
All 3 nginx.conf |
19 | MINOR | keepalive_timeout 3000 is 50 minutes — almost certainly a copy-paste error (should be 65s) |
The most impactful issue is the nginx add_header inheritance trap: because location / and location /live both exist without their own add_header directives, the server-block headers will be inherited — but if the upstream nginx4spa base image's included configs add any add_header inside a location block, all server-block headers are silently dropped for that location. This is a well-known nginx footgun. The safe fix is to move all add_header directives into every location block, or use proxy_hide_header + add_header at the http level (not applicable here), or — best for this use case — use a dedicated include snippet.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4186 +/- ##
========================================
Coverage 61.31% 61.31%
========================================
Files 1030 1030
Lines 20860 20860
Branches 4076 4076
========================================
Hits 12791 12791
- Misses 7783 7986 +203
+ Partials 286 83 -203 see 77 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
nginx does not inherit `add_header` directives into a location block that defines its own. To keep the security headers robust against future location blocks (and any header set by an included/base-image snippet), extract them into a per-app `security-headers.conf` and `include` it both at server level and inside every location block, keeping a single source of truth per app. Addresses review feedback on the add_header inheritance trap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Vérification e2e sur env de review ✅Env de review déployé depuis cette branche (cluster
Headers émis une seule fois (pas de doublon), validé aussi par
|
revu-bot
left a comment
revu-bot
left a comment
There was a problem hiding this comment.
Summary
This PR correctly solves a real production incident (503s caused by Traefik's incompatibility with nginx.ingress.kubernetes.io/custom-headers) by moving security headers into each app's nginx configuration. The approach is sound and the fix is well-motivated.
Key observations:
- The three
nginx.conffiles are byte-for-byte identical — only thesecurity-headers.conffiles differ per app. This creates a maintainability burden. - The
includeat theserver {}level is redundant given the per-location includes, and the comment explaining this is slightly misleading. - The
Cache-Control: no-storeheader is applied to all routes including static assets (/live,/ready,/50x.html), which is overly broad but not a regression from the previous behaviour. - Missing
Permissions-Policyheader (not a regression, but worth noting). - The
keepalive_timeout 3000value (seconds, not ms) is very high — likely a copy from the upstream image, not introduced here.
| File | Lines | Severity | Issue |
|---|---|---|---|
packages/*/nginx.conf (all 3) |
1–76 | IMPORTANT | Identical files — DRY violation, future drift risk |
packages/*/nginx.conf |
31 | MINOR | Server-level include is redundant when all locations also include |
packages/frontend/security-headers.conf |
5 | IMPORTANT | Cache-Control: no-store applied to health/error endpoints |
The frontend, portail-admins and portail-usagers nginx configs were byte-for-byte identical (only the per-app security-headers.conf differs). Collapse them into a single packages/nginx.conf copied by each Dockerfile, keeping the per-app security-headers.conf, to avoid three-way drift. Also clarify the server-level security-headers include is only a fallback for requests matching no location. Addresses review feedback (DRY across the three nginx.conf). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
3 participants
Problème
La preprod (
domifa-preprod.ovh.fabrique.social.gouv.fr/manage) renvoyait503 no available serveralors que les pods étaient sains et le déploiement « passait ».Cause : l'edge ingress du cluster legacy
ovh-deva été basculé de nginx-ingress vers Traefik. Le provider de compatkubernetesingressnginxde Traefik n'honore l'annotationnginx.ingress.kubernetes.io/custom-headersque pour les en-têtes présents dans son allowlistglobalAllowedResponseHeaders(vide par défaut). Tout en-tête hors allowlist (CSP, Referrer-Policy, Cache-Control…) fait échouer la résolution de la route entière → 503, alors quefrontend,portail-adminsetportail-usagersposaient leurs en-têtes de sécu via cette annotation.Changement
On déplace les en-têtes de sécurité dans la conf nginx de chaque app (l'image
nginx4spapermet de surcharger/etc/nginx/nginx.conf) et on retire les blocsingress.customHeadersdu.kontinuous/values.yaml.Résultat : les apps émettent elles-mêmes leurs en-têtes de sécu, indépendamment de l'ingress controller placé devant. Plus de couplage à un comportement spécifique de l'edge.
packages/{frontend,portail-admins,portail-usagers}/nginx.conf: conf basée sur celle de l'imagenginx4spa:8.2.4+ en-têtes de sécu (CSP par app, X-Frame-Options, X-Content-Type-Options, X-XSS-Protection, Referrer-Policy, Cache-Control, + X-Robots-Tag pour admins). En-têtes enalways(envoyés aussi sur les réponses d'erreur).COPY ./packages/<app>/nginx.conf /etc/nginx/nginx.conf..kontinuous/values.yaml: suppression desingress.customHeaders.Les en-têtes (noms et valeurs, CSP incluse) sont repris à l'identique de ce qui était défini dans
values.yaml.Validation
nginx -tOK sur les 3 confs contre l'image réelleghcr.io/socialgouv/docker/nginx4spa:8.2.4.🤖 Generated with Claude Code