karlheyes / icecast-kh

KH branch of icecast
GNU General Public License v2.0
299 stars 107 forks source link

Icecast-KH7 release doesn't appear to honor the X-Forwarded-For header #195

Closed BusterNeece closed 4 years ago

BusterNeece commented 6 years ago

Hello! The radio management software I build uses icecast-kh built from the latest release here on Github, and also offers support for proxying listener traffic through nginx, with unique listener data preserved for licensing and royalty purposes.

Prior to the latest update, my software was pointing at the KH5 release. The listener data API endpoints in Icecast properly reported the forwarded IP from nginx. The recent update to KH7 appears to have reverted this functionality, instead showing only the direct IP (in this case, the nginx docker container's IP)

The relevant nginx proxy configuration is below:

proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $remote_addr;

I also included the "X-Real-IP" header as this is often used in other software.

If there is something I should be changing on my end to properly reflect the proxied listener IP, please let me know. Thank you!

karlheyes commented 6 years ago

I don't see any changes relating to that header between those versions. I've just tried here with my latest work using the tag in as before and the access log shows the expected IP (if present in the request).

karl.

BusterNeece commented 6 years ago

@karlheyes The endpoint I'm calling for the detailed listener statistics is /admin/listclients?mount=/mountname.mp3, and it's returning a record with a single IP, which is the actual remote address of the user, not the contents of the X-Forwarded-For or X-Real-IP headers.

karlheyes commented 6 years ago

The X-Real-IP header is never checked by icecast but X-Forwarded-For is (provided the xml tag is in place). I checked the listclients page as well and that seems to work fine. You are sure the xml x-forward-for tag is set correctly?, setting level 4 will report any changes for the forwarding header when a new connection comes in

kar

BusterNeece commented 6 years ago

@karlheyes I've never actually set that config variable in the XML...is it a KH-specific thing? I never saw it in the upstream documentation, so there's a very real possibility I've been doing it wrong this whole time. :(

Thank you for the heads up!

karlheyes commented 6 years ago

you have to have a trusted IP for the proxy or log both IPs, or else you will get fake addesses used. I chose the former as you'll get a lot of duplicated content over time.

karl.

BusterNeece commented 6 years ago

@karlheyes Makes sense to me, though it may be difficult to pinpoint exactly what IP another docker container will have at a given moment (and it may have multiple internals). Can you specify a subnet or range with that config?

karlheyes commented 6 years ago

you can specify multiple x-forwarded-for tags, but not a range as of yet.

karl.

BusterNeece commented 6 years ago

@karlheyes Sorry to be a bother on this, but after adding multiple x-forwarded-for tags, it appears it's still not respecting this and instead returning the IP, even though it's one of the ones specified.

Basically, the XML file being generated looks like:

<paths>
        <basedir>(basedir)</basedir>
        <webroot>(webroot)</webroot>
        <x-forwarded-for>172.18.0.1</x-forwarded-for>
        <x-forwarded-for>172.18.0.2</x-forwarded-for>
        ...
        <x-forwarded-for>172.18.0.20</x-forwarded-for>
</paths>

Is this how I should be doing that? I realize it's not graceful, but without the application having control over the Docker socket directly, there's no simple way to enumerate which IPs are actually the ones being used by the nginx container(s).

karlheyes commented 6 years ago

that would be the idea. set the log level to 4 and just try connecting via the proxy, then look at the log file for 'forward'. I presume you are saying that those 172.18.x.x addresses are appearing in your access log

karl.

BusterNeece commented 6 years ago

@karlheyes Sorry, it turns out my dev and staging environments have a slight variation, in that one assigns internal Docker IPs starting at 172.18.0.1 and the other uses 172.20.0.1.

I'm worried that this whitelist approach to the XFF header, sans any sort of wildcards, will basically make this impractical or impossible to use in this setting, given the inherent volatility of the IP range in a scalable environment like that.

Can we reopen this as an enhancement request, to either add support for wildcards in the X-Forwarded-For whitelist, or to allow 0.0.0.0 or equivalent in settings where access will always be proxied through a local service like nginx?

BusterNeece commented 6 years ago

@karlheyes Sorry to bother on this subject, but per my previous reply, can we reopen this issue as an enhancement request along the lines of enabling either an IP block/wildcard setting for the X-Forwarded-For configuration value, or allowing 0.0.0.0 as a valid option in the case where all traffic is proxied? The current configuration setup is unworkable in any environment where the nginx proxy IPs will always be within a single block, but may rotate or change at any time without the radio service knowing, as would happen when scaling with Docker containers.

jesseorr commented 6 years ago

@SlvrEagle23 As a work around, you can utilize the same configuration element repeatedly in the XML to specify each IP. If the block is small that may suffice for now.

BusterNeece commented 6 years ago

@karlheyes I'm not sure if there has been any update on this issue, but after discovering that Docker's internal DNS can use an entire range of IPs, not just a small, narrow range that could be enumerated through multiple X-Forwarded-For XML entries, there's effectively no way for me to make Icecast sitting in front of an nginx forward proxy work in the Docker environment, unless support was added for a range of IP addresses or a wildcard specifier.

Until recently, I've advised the small subset of affected users of my software to use SHOUTcast as the frontend for their stations, but with SHOUTcast going to a "freemium" model with an even more aggressive licensing scheme, I can no longer include or bundle support for that in my software.

If we could work together to get this issue figured out, it would have a huge positive impact on the increasing group of people using Docker for their station infrastructure. Thanks!

CodeSteele commented 6 years ago

I currently have a branch that allows us to configure a flag to basically allow all x-forwarded-for headers, which is pretty much required if you stick this behind a load balancer or in this case systems like Docker:

https://github.com/CodeSteele/icecast-kh/commit/fb24d1800b0cfb6676941c192f0ae536646c1bd9

Eyeball it and let me know if you're happy with it and are fine with a PR for this functionality.

CodeSteele commented 4 years ago

Should be able to close this now.