ktorio / ktor

Framework for quickly creating connected applications in Kotlin with minimal effort
https://ktor.io
Apache License 2.0
12.71k stars 1.04k forks source link

XForwardedHeaderSupport should let you specify which index (from end) to choose #1305

Closed glasser closed 1 year ago

glasser commented 4 years ago

Ktor Version and Engine Used (client or server and name) ktor-server-core 1.2.3

Describe the bug The XForwardedHeaderSupport feature assigns the first entry in X-Forwarded-For to call.request.origin.remoteHost. However, typically the way that proxies work is that they accept arbitrary X-Forwarded-For values from clients and add their own IPs to the end of the list. So to properly figure out a client's IP address, one needs to know how many proxies one is behind, and count that far from the end of the list.

Now, to be fair, the docs on remoteHost say to not use it for user authentication. But if you do know how many proxies you are behind, it should be safe to use it for user authentication, by counting that many IPs from the end. So it would be good if you could do this easily!

So I guess I have two separate concerns: (a) Today's behavior is surprisingly insecure, and though it's documented on RequestConnectionPoint it should be documented on https://ktor.io/servers/features/forward-headers.html

(b) Ideally you should be able to configure XForwardedHeader with something like

install(XForwardedHeaderSupport) {
  // Given `x-forwarded-for: GARBAGE1, GARBAGE2, REALCLIENTIP1, PROXY1` and request.local being the second proxy, this will get REALCLIENTIP1
  proxyCount = 2
}

ie, "behind one proxy" means the last IP address is correct, "behind two proxies" means the second-to-last, etc.

To Reproduce

Expected behavior It should be possible to configure XForwardedHeaderSupport to reliably ascertain the remoteHost.

Happy to send in a PR, esp if people like the proposed variable name.

glasser commented 4 years ago

I guess issue 3 is that it's annoying that you can't implement this logic by yourself because ApplicationCall.mutableOriginConnectionPoint is internval.

oxc commented 4 years ago

It's really very common to configure these values in a way that allows trusting them.

The way that ktor configures this feature by default, however, contradicts this in two ways:

Here is an excerpt from the Apache HTTPd mod_remoteip module documentation, which is comparable to this Feature. It allows you to specify a list of trust proxies, and then processes the header values in the following way:

When multiple, comma delimited useragent IP addresses are listed in the header value, they are processed in Right-to-Left order. Processing halts when a given useragent IP address is not trusted to present the preceding IP address. The header field is updated to this remaining list of unconfirmed IP addresses, or if all IP addresses were trusted, this header is removed from the request altogether.

Perhaps there is no suitable default configuration that can be used for trusted setups, but please at least provide a way to implement this logic ourselves by making the necessary API public.

oxc commented 4 years ago

It's really very common to configure these values in a way that allows trusting them.

I would even go so far and claim that the feature is not much use otherwise in the first place.

Nobody wants untrusted values in their request origin properties.

oleg-larshin commented 4 years ago

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.