Fix: adding "APP_TRUSTED_PROXIES" variable

[LoveSkylark]

This change adds support for APP_TRUSTED_PROXIES to the container startup configuration so LibreNMS automatically trusts requests coming through common Docker and Kubernetes proxy networks without manual setup: the script now defines a default trusted proxy list (172.17.0.0/16,10.42.0.0/16), writes that value into the generated LibreNMS .env file.

[murrant]

10.42.0.0/16 doesn't seem like it should be there.

[Yoyasp]

If the APP_TRUSTED_PORXIES variable is prefilled anywhere it should be in the traefik example .env AND also define the same subnet for traefik in the compose.yml

[LoveSkylark]

If the APP_TRUSTED_PORXIES variable is prefilled anywhere it should be in the traefik example .env AND also define the same subnet for traefik in the compose.yml

Ok K3S removed

[murrant]

I strongly agree with @Yoyasp it should only be set in the traefik example and make sure the network matches.

[LoveSkylark]

am I doing something wrong why isn this being added to the build?

Every time docker is update right now LibreNMS breaks and you have to go into the docker and manually change it back.

[murrant]

Yes, you change examples that don't use a proxy.

[LoveSkylark]

So let me make sure I understand the situation. I'm a low-level, infrequent LibreNMS contributor. I hit an issue after a security update, submitted a fix, it went through review, and I made the requested adjustments.

Since my own systems are running fine with a workaround, I stopped watching this closely and moved on to other work.

Meanwhile, the fix has been sitting here waiting for some final guideline touches — and I'm guessing nobody else is working on a fix, because one already exists in the pipeline.

So, honest question: in general, how should occasional contributors handle situations like this in LibreNMS? Is it better for us to step back and let core maintainers take over, so fixes don't stall while waiting on us?

Because as far as I can tell, anyone with a legitimate Docker deployment hasn't had a working container since that security update.

[murrant]

Your description could be improved here. Calling out Laravel is probably confusing for most users.
"What is a trusted proxy?"

[murrant]

Something like this, but I think my wording still could be improved:
"Allow given IP(s) to inject headers into the HTTP request. This is required to forward the real client IP from reverse proxies. IPs are allowed to inject any header even authentication headers, so take care to restrict this to your actual reverse proxies"

[murrant]

Maybe this?
IP(s) or CIDR ranges allowed to set forwarding headers (X-Forwarded-For, X-Forwarded-Proto, etc.) so the real client IP and protocol survive behind a reverse proxy. These IPs can inject any header, including auth headers, so restrict this to your actual proxies only.

[murrant]

What do we think about this as a default? I'm unsure since it differs from upstream. But at least you document it.

[LoveSkylark]

Your are right this range is wrong, back in the old days I used to do exactly this: everything landed on the default bridge, so whitelisting 172.17.0.0/16 gave you a simple, ready-to-go path between your reverse proxy and any other container you spun up on the host. That's was the intent here.

Looking at the example in this repo, we're not even separating the reverse proxy from the LibreNMS server (Traefik and LibreNMS share one compose network), which will be 172.18.0.0/16 most of the time on a basic host. If we want a default to cover more than just the example scenario, it should be 172.16.0.0/12, which covers all the /16 networks Docker hands out on a host of this size.

That said, I don't have a strong stake in how this lands, I won't be using it myself (I run this in Kubernetes), I was just trying to make this easier for the most basic Docker deployments.

I appreciate your dedication to LibreNMS. Feel free to make whatever adjustments you want to this PR — or close it if that's easier. I don't plan to spend more time on it myself.

This wouldn't be the first fix I've submitted that stalled out in rounds of small tweaks. Honestly, I don't think senior contributors like you should have to wait on a drive-by contributor like me for changes you already know should be in there — just make them directly. I'm fine with whatever the result looks like.

[murrant]

Hmm, I just noticed. REAL_IP_FROM Is this a duplicate? does it clash?
Also, APP_TRUSTED_PROXIES means trust the headers passed to me (not real ip specifcally, but it includes that)

[LoveSkylark]

Im baffled,
A change was implement the breaks docker deployment with any kind of proxy in front, and we are tunnel visioning on what is the best default setting instead of implementing the fix.

Strip default away set the description to what ever you like, just get the fix through then we can start tweaking that the bets setting is and how it is best described.

This is like not fixing a broken window because you cant decide what color curtains will go with the new window :)

[Yoyasp]

You do realize nothing is broken right?
Its a setting you change... Set an env var, edit the example compose, do what you need. The reason it changed is because it allowed man in the middle attacks...

I get your frustration with getting the change through, but its better to stay on the side of safely failing on boot rather than exposing company data because someone forgot to change a default setting

[murrant]

Better analogy, it is like leaving the window open because the window is broken and you don't want people to get in through the broken window. Also, change the window to a door and add a lock... ok I'm lost now.

Just like this PR. I don't really use the docker in the real world which is why I ask so many questions. I don't want to break user's installs or expose their data. We are trying to walk a fine line. Changing defaults has much more wide reaching impact and we have to be careful.

Not having APP_TRUSTED_PROXIES in the docs was a major oversight and I would have merged that almost right away.

Again, I have no idea what is going on with the docker image and a lot of people come in with incorrect assumptions (me included) and have different environments. We can get silly things like duplicate variables. Which is very hard to undo.

[LoveSkylark]

You do realize nothing is broken right? Its a setting you change... Set an env var, edit the example compose, do what you need. The reason it changed is because it allowed man in the middle attacks...

I get your frustration with getting the change through, but its better to stay on the side of safely failing on boot rather than exposing company data because someone forgot to change a default setting

@Yoyasp have you tested this?

It's true that APP_TRUSTED_PROXIES gets passed into the containe, you can see it with env from a shell inside. The problem is that PHP-FPM strips environment variables from its worker processes as a security measure (clear_env = yes by default), so the web UI never sees the value even though the container has it.

This can be bypassed with the CLEAR_ENV variable the image exposes, but that disables the scrubbing entirely, exposing every container variable, including DB_PASSWORD, to all web-facing PHP code. Arguably a broader security tradeoff than just setting APP_TRUSTED_PROXIES="*", but it's what I'm currently running to keep my deployments working.

Alternatively, PHP-FPM supports whitelisting individual variables through the security filter by adding env[APP_TRUSTED_PROXIES] = $APP_TRUSTED_PROXIES to the pool config (www.conf). But that config is regenerated on container start, and its path changes with PHP version bumps and third-party modifications, so the customization can silently get lost.

The original image sidesteps this for its core settings by writing them into Laravel's .env at container start via the deployment script (rootfs/etc/cont-init.d/03-config.sh).

cat >${LIBRENMS_PATH}/.env <<EOL
APP_URL=${LIBRENMS_BASE_URL}
DB_HOST=${DB_HOST}
DB_PORT=${DB_PORT}
DB_DATABASE=${DB_NAME}
DB_USERNAME=${DB_USER}
DB_PASSWORD="${DB_PASSWORD}"
EOL

This is where my pull request went off the rails. My first thought was to simply add APP_TRUSTED_PROXIES=${APP_TRUSTED_PROXIES} into the .env file . But then anyone not setting the variable would get APP_TRUSTED_PROXIES= and an empty value there overrides LibreNMS's built-in default rather than falling back to it. My workaround was to ship default values covering the internal IP ranges Docker and Kubernetes use, unreachable from outside the cluster, analogous to trusting 127.0.0.1 on a bare-metal install. That sidetracked the whole PR into a discussion about which internal ranges belong in the defaults.

So in my latest commit I've taken APP_TRUSTED_PROXIES out of the heredoc entirely and instead append it to .env only when the variable is actually set. The script already uses this exact pattern for RRDCACHED_SERVER, so it follows the file's existing convention rather than inventing a new one.

[LoveSkylark]

Ok I removed default values, I pulled it out of the .env build and append it so it would not overwrite LibreNMS defaults if nothing was configured.

[murrant]

I spent some time to build up an environment and test. With the default container (no code changes), just setting APP_TRUSTED_PROXIES in my compose file worked.

    environment:
      - "APP_TRUSTED_PROXIES=172.18.0.0/16"

[murrant]

If you think additional changes are needed, send a new PR and we can try to figure that out.