Skip to content

feat: extract nginx into separate container#430

Open
pascaliske wants to merge 2 commits into
MISP:masterfrom
pascaliske:feature/extract-nginx-from-core-image
Open

feat: extract nginx into separate container#430
pascaliske wants to merge 2 commits into
MISP:masterfrom
pascaliske:feature/extract-nginx-from-core-image

Conversation

@pascaliske

@pascaliske pascaliske commented Jul 5, 2026

Copy link
Copy Markdown

Hi MISP-team,

Thank you for your work in this repository.

This PR moves nginx from the misp-core container to a new separate misp-nginx container.

The intention behind it is:

  • Improving the runtime security of nginx
    → 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
  • Following containerization best practices
    → Separation of concerns, leaving grouping / orchestration to infra-tooling (Docker Compose / Kubernetes / etc.)
    → Easier horizontal scaling due to separation
  • Reducing the size of the core image which improves deployment and operational processes
  • Reducing the amount of scripted config changes - the official image has builtin support for variable substitutions
  • Preparing the core images (and the others) for further improvements and tooling coming soon... 😁

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.

Signed-off-by: Pascal Iske <info@pascaliske.dev>
@mortezamirkar

Copy link
Copy Markdown
Contributor

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.
I've done a thorough review and have a few items to flag before this can merge:
Critical:

  1. TLS/SSL support is missing — The old architecture served HTTPS on port 443 with self-signed cert generation. The new misp-nginx only listens on HTTP port 8080 with no TLS configuration at all. The ssl volume (./ssl/:/etc/nginx/certs/:Z) also remains orphaned on misp-core. If TLS termination is intentionally being pushed to an external reverse proxy, this should be explicitly documented. Otherwise, SSL support needs to be re-added to misp.conf.
  2. error_log /dev/stderr debug in misp.conf — This will produce extremely verbose logs in production. Should default to error or warn, not debug.
  3. Missing include snippets/fastcgi-php.conf — The old config included this for standard FastCGI params (QUERY_STRING, REQUEST_URI, etc.). The new config only sets SCRIPT_FILENAME manually. This could cause subtle PHP routing issues — worth validating.
    High priority:
  4. Submodule init is commented out in the nginx Dockerfile — If MISP stores any web assets in submodules under app/webroot, they'll be missing. Either uncomment git submodule update --init --recursive . or verify it's not needed.
  5. IPv6 support was dropped — DISABLE_IPV6 env var is still exported but unused. The old config had IPv6 toggle support for listeners.
  6. /status location may conflict with legitimate MISP routes — Old code used a separate localhost port for the FPM status page. Consider namespacing this differently.
  7. Health check may not work reliably — wget --spider against / will get a 302 redirect to /users/login, which some implementations treat as failure.
    Nit: The /etc/nginx/certs volume mount on misp-core is now dead weight and should be removed.
    Overall the direction is great — just need these points addressed before merge. Happy to help test once updated.

@pascaliske

Copy link
Copy Markdown
Author

Thanks for the feedback - I will have a look into and address those points.

@mortezamirkar

Copy link
Copy Markdown
Contributor

Sounds good, thanks! Let me know if you want me to re-review once you've pushed updates

@pascaliske

pascaliske commented Jul 9, 2026

Copy link
Copy Markdown
Author

@mortezamirkar I addressed your points:

  1. TLS/SSL support is missing — The old architecture served HTTPS on port 443 with self-signed cert generation. The new misp-nginx only listens on HTTP port 8080 with no TLS configuration at all. The ssl volume (./ssl/:/etc/nginx/certs/:Z) also remains orphaned on misp-core. If TLS termination is intentionally being pushed to an external reverse proxy, this should be explicitly documented. Otherwise, SSL support needs to be re-added to misp.conf.

The removal of ports 80 / 443 was intentional because non-root containers can't listen to ports below 1024. Also I wouldn't generate self-signed certificates during image build because in production those won't be used anyway. So yes I would propose to push TLS to an external reverse proxy if that's fine for you.

  1. error_log /dev/stderr debug in misp.conf — This will produce extremely verbose logs in production. Should default to error or warn, not debug.

Forgot to switch back after debugging, I set this to error again - easy fix 😄.

  1. Missing include snippets/fastcgi-php.conf — The old config included this for standard FastCGI params (QUERY_STRING, REQUEST_URI, etc.). The new config only sets SCRIPT_FILENAME manually. This could cause subtle PHP routing issues — worth validating.

The FastCGI params are actually included via include fastcgi_params;. This file is present in the unprivileged image it just has a different name and path than the one in the original core image. The only directives not included are fastcgi_split_path_info and PATH_INFO. AFAICT those are only really required in really old PHP apps because newer apps rely on REQUEST_URI instead for their routing. It is even better to remove them security wise because that eliminates another attack vector. I double checked with CakePHP docs which doesn't use them as well: https://book.cakephp.org/2.x/installation/url-rewriting.html#pretty-urls-on-nginx

  1. Submodule init is commented out in the nginx Dockerfile — If MISP stores any web assets in submodules under app/webroot, they'll be missing. Either uncomment git submodule update --init --recursive . or verify it's not needed.

Forgot to add this back as well. Actually I just COPY the actual webroot directory for serving static assets therefore I restricted the recursive submodule update to app/webroot which speeds up image builds a bit.

  1. IPv6 support was dropped — DISABLE_IPV6 env var is still exported but unused. The old config had IPv6 toggle support for listeners.

I re-added the logic for supporting IPv6 and for disabling it via DISABLE_IPV6.

  1. /status location may conflict with legitimate MISP routes — Old code used a separate localhost port for the FPM status page. Consider namespacing this differently.

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 (wget -qO- http://127.0.0.1:8081/status / wget -qO- http://127.0.0.1:8081/fpm-status). This should prevent any conflicts with MISP routes.

  1. Health check may not work reliably — wget --spider against / will get a 302 redirect to /users/login, which some implementations treat as failure.

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.

Nit: The /etc/nginx/certs volume mount on misp-core is now dead weight and should be removed.

Done. 🙂

Could you please assist in re-testing the PR? Thank you!

Signed-off-by: Pascal Iske <info@pascaliske.dev>
@pascaliske pascaliske force-pushed the feature/extract-nginx-from-core-image branch from c71739d to fca2ae8 Compare July 9, 2026 03:45
@ostefano

ostefano commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

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?

@pascaliske

Copy link
Copy Markdown
Author

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 docker-compose.yml we could then just map 808080 / 4438443.

What do you think about this idea @ostefano?

Comment thread nginx/Dockerfile
ARG DOCKER_HUB_PROXY=""

# --- sources stage
FROM "${DOCKER_HUB_PROXY}alpine:3.23" AS sources

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not alpine. We want something debian based here, or if not, something officially released by nginx.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread docker-bake.hcl
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}",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what is the role of NGINX_COMMIT.

Should the only variable here be version of the base image?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's just for referencing the source code in the Dockerfile. Should I just use CORE_TAG & CORE_COMMIT here as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Seperate nginx into a single container

3 participants