feat: extract nginx into separate container#430
Conversation
Signed-off-by: Pascal Iske <info@pascaliske.dev>
|
Great work on this PR, @pascaliske! The separation of nginx into its own container is a much-needed architectural improvement and the security hardening (non-root, read-only fs, dropped capabilities) is excellent. The move to nginxinc/nginx-unprivileged with envsubst-based templates is much cleaner than the old sed-based approach.
|
|
Thanks for the feedback - I will have a look into and address those points. |
|
Sounds good, thanks! Let me know if you want me to re-review once you've pushed updates |
|
@mortezamirkar I addressed your points:
The removal of ports
Forgot to switch back after debugging, I set this to error again - easy fix 😄.
The FastCGI params are actually included via
Forgot to add this back as well. Actually I just
I re-added the logic for supporting IPv6 and for disabling it via
I moved the FPM status page route alongside the nginx stub status to a separate port which is only reachable from within the nginx container (
Yeah makes sense - I rewrote the health check to check against the status page. This prevents unnecessary restarts of nginx if the actual FPM container fails - static assets are still served.
Done. 🙂 Could you please assist in re-testing the PR? Thank you! |
Signed-off-by: Pascal Iske <info@pascaliske.dev>
c71739d to
fca2ae8
Compare
|
Just a quick comment (especially related to the TLS part): we still need this to be a zero to hero container, so TLS part needs to be handled in one way or another. Also we need to make this backwards compatible in some way. Can't we add another container taking care of that part? |
I already thought about making it based on the presence of cert & key files. Basically if those two files are present it also enables serving via 8443 with those cert in-place. In the What do you think about this idea @ostefano? |
| ARG DOCKER_HUB_PROXY="" | ||
|
|
||
| # --- sources stage | ||
| FROM "${DOCKER_HUB_PROXY}alpine:3.23" AS sources |
There was a problem hiding this comment.
Not alpine. We want something debian based here, or if not, something officially released by nginx.
There was a problem hiding this comment.
Why not alpine? That's just the stage for fetching the static assets and alpine is small.
But I can also just use the same image as the final stage:
FROM ghcr.io/nginx/nginx-unprivileged:1.31.2-alpine-slim AS sources
| tags = flatten(["${NAMESPACE}/misp-nginx:latest", "${NAMESPACE}/misp-nginx:${COMMIT_HASH}", NGINX_TAG != "" ? ["${NAMESPACE}/misp-nginx:${NGINX_TAG}"] : []]) | ||
| args = { | ||
| "NGINX_TAG": "${NGINX_TAG}", | ||
| "NGINX_COMMIT": "${NGINX_COMMIT}", |
There was a problem hiding this comment.
I don't understand what is the role of NGINX_COMMIT.
Should the only variable here be version of the base image?
There was a problem hiding this comment.
Yes, it's just for referencing the source code in the Dockerfile. Should I just use CORE_TAG & CORE_COMMIT here as well?
Hi MISP-team,
Thank you for your work in this repository.
This PR moves nginx from the
misp-corecontainer to a new separatemisp-nginxcontainer.The intention behind it is:
→ The nginx process now fully runs with a non-root user in a read-only root filesystem
→ Linux capabilities are dropped completely
→ Privileges are limited to a minimum
→ Separation of concerns, leaving grouping / orchestration to infra-tooling (Docker Compose / Kubernetes / etc.)
→ Easier horizontal scaling due to separation
Please note: the high number of line changes in the misp-core Dockerfile are due to some re-ordering and whitespace / indentation changes I made, I hope this is fine for you.
Closes #340.