Fix: adding "APP_TRUSTED_PROXIES" variable #569
Conversation
|
10.42.0.0/16 doesn't seem like it should be there. |
|
If the |
Ok K3S removed |
|
I strongly agree with @Yoyasp it should only be set in the traefik example and make sure the network matches. |
|
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. |
|
Yes, you change examples that don't use a proxy. |
Remove APP_TRUSTED_PROXIES environment variable.
Remove APP_TRUSTED_PROXIES configuration from librenms.env
|
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. |
| * `REAL_IP_FROM`: Trusted addresses that are known to send correct replacement addresses (default `0.0.0.0/32`) | ||
| * `REAL_IP_HEADER`: Request header field whose value will be used to replace the client address (default `X-Forwarded-For`) | ||
| * `LOG_IP_VAR`: Use another variable to retrieve the remote IP address for access [log_format](http://nginx.org/en/docs/http/ngx_http_log_module.html#log_format) on Nginx. (default `remote_addr`) | ||
| * `APP_TRUSTED_PROXIES`: Trusted proxies for Laravel / LibreNMS proxy awareness (default `172.17.0.0/16`) |
There was a problem hiding this comment.
Your description could be improved here. Calling out Laravel is probably confusing for most users.
"What is a trusted proxy?"
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
| DB_TIMEOUT=${DB_TIMEOUT:-30} | ||
|
|
||
| LIBRENMS_BASE_URL=${LIBRENMS_BASE_URL:-/} | ||
| APP_TRUSTED_PROXIES=${APP_TRUSTED_PROXIES:-"172.17.0.0/16"} |
There was a problem hiding this comment.
What do we think about this as a default? I'm unsure since it differs from upstream. But at least you document it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
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. |
3 participants
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.