Add WebUI reverse proxy source IP resolution #15047

HiFiPhile · 2021-05-31T19:46:44Z

This PR is a revive of #9176 .

Since the orignal author @qix67 is no longer maintaining the PR and based on the discussion some collaborators are also in favor of this feature, I rebased the PR to latest master.

It also includes some refactor and renaming.

Co-authored-by: qix67
Co-authored-by: HiFiPhile admin@hifiphile.com

glassez · 2021-06-01T10:49:01Z

@HiFiPhile
If you want to contribute to qBittorrent you should consider our Coding Guidelines and follow them in your contributions.

thalieht

A few things not mentioned in coding guidelines

src/base/http/connection.cpp

src/gui/optionsdialog.ui

src/webui/www/private/views/preferences.html

src/base/http/connection.cpp

src/webui/webapplication.h

src/webui/webapplication.cpp

glassez · 2021-06-05T03:37:11Z

src/webui/webapplication.cpp

+            {
+                if (clientAddress.setAddress(remoteIp) && clientAddress.isGlobal())
+                {
+                    hasGlobalIp = true;


You can return clientAddress; here.

So what about changing it?

PS I hope that your disagreements with @qix67 do not mean that this PR is not ready?

So what about changing it?

I've changed it in the last commit.

PS I hope that your disagreements with @qix67 do not mean that this PR is not ready?

No we only have some differences in proxy configuration, this PR can fit both cases.

glassez · 2021-06-05T03:39:28Z

src/webui/webapplication.cpp

+
+        if (!forwardedFor.isEmpty())
+        {
+            // client address is the 1st global IP in X-Forwarded-For or, if none available, the 1st IP in the list


IIRC, client address is always first. Any subsequent addresses become to midway proxies. Am I wrong?

Yes, you are wrong but it is a fairly common error.

When I access to qbittorrent from my work, I have the following the X-forwarded-for
172.29.225.237,194.254.4.90,10.0.3.254,127.0.0.1

First IP is the local IP of my desktop PC at work.

2nd IP is the public IP used by my work proxy.

3rd IP is the local IP of my reverse proxy at home. This reverse proxy runs on my gateway and performs SSL termination.

4th IP is added by nginx running on the same computer as qbittorrent (proxy_pass).

The correct IP is always the last public IP, here, the 2nd IP.

Yes, you are wrong but it is a fairly common error.

I was only guided by https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#syntax

It is correct, the first IP is the real client IP but it does not mean it is a public IP, that's why it is kinda misleading.

The worst case I ever seen is something like that (combination of proxy, VPN and reverse proxies):
192.168.0.1, public_IP, 10.0.23.1, public_IP2, 172.29.226.1, 127.0.0.1

In this case, the correct public IP is public_IP2 (last public IP). I admit, it is a very unusual settings but it is a valid value.

OK, thanks.

One more question.
We specify the address of the proxy server to make sure that the request (containing X-Forwarded-For header) is received from it. This assumes that requests can come from both the proxy server address and another address. Is it a valid usecase to have a configured proxy server and at the same time some other access channels for untrusted clients?

I think for a safe reverse proxy configuration, it's better to delete X-Forwarded-For at frontend, so someone from the
outside cannot spoof this header. It also ensures the first IP in the header is the client IP.

I think for a safe reverse proxy configuration, it's better to delete X-Forwarded-For at frontend, so someone from the
outside cannot spoof this header. It also ensures the first IP in the header is the client IP.

I don't think it will change anything as the frontend will always add the incoming IP address at the end of X-Forwarded-For string. Anything existing before should be assumed unsafe and purely informational. That's why taking the last public IP is important.

Moreover, it is not always possible to reconfigure the reverse proxy.

We specify the address of the proxy server to make sure that the request (containing X-Forwarded-For header) is received from it. This assumes that requests can come from both the proxy server address and another address. Is it a valid usecase to have a configured proxy server and at the same time some other access channels for untrusted clients?

The point is that if it is only available through a proxy (as it should be, IMO; i.e., if we have a proxy, then it should only be available through it), then we do not need to have a proxy address check, but just specify whether it should resolve the client address or not.

It looks like we should still allow user to explicitly set the proxy address for better security. But maybe we can be more lenient in another case, i.e. allow the user to enable client address resolution without explicitly specifying the proxy address?

But maybe we can be more lenient in another case, i.e. allow the user to enable client address resolution without explicitly specifying the proxy address?

It's hard to say, people do it differently.
For example Gitea allows client address resolution from 127.0.0.0 by default, SFTPGo needs to specify proxy address whitelist.

It's just that we could provide more options to the user by placing more responsibility on them (of course, by warning them about this). If he is confident in the security of his network environment settings, then allow him not to explicitly specify the proxy server address.

In any case, this is not a blocking comment. So if you don't agree, you can continue as is and wait for user feedback.

glassez · 2021-06-07T10:11:19Z

src/base/preferences.cpp

@@ -761,6 +761,26 @@ void Preferences::setWebUICustomHTTPHeaders(const QString &headers)
    setValue("Preferences/WebUI/CustomHTTPHeaders", headers);
 }

+bool Preferences::isWebUIReverseProxyEnabled() const
+{
+    return value("Preferences/WebUI/ReverseProxyEnabled", false).toBool();


IMO, "reverse proxy enabled" confuses. One may think that there is some built-in "reverse proxy" feature.
Maybe "forwarded client IP enabled/used/allowed"?

Maybe "Reverse proxy support" ? I think the word "reverse proxy" makes the purpose clear.

glassez · 2021-06-07T10:14:04Z

src/base/preferences.cpp

+
+QString Preferences::getWebUIReverseProxyAddress() const
+{
+    return value("Preferences/WebUI/ReverseProxyAddress", "").toString().trimmed();


Can't it be used with some kind of "forward" proxy as well? I would avoid explicitly specifying "reverse" term.

I think a list of "trusted_proxies" is not a bad idea.

src/webui/webapplication.cpp

src/base/preferences.cpp

src/base/http/types.h

src/base/preferences.cpp

src/webui/api/appcontroller.cpp

src/webui/www/private/views/preferences.html

src/webui/webapplication.cpp

Walter-o · 2021-06-14T00:28:52Z

My input:

The idea of a "Trusted proxies list" is good in case people have multiple proxies

The warning for configuring is also in the right place since misconfiguration can lead to IP impersonation

1 thing i would propose as a tiny bonus is being able to manually enter the header name that is checked for IP.
Some proxies are already configured or forced to use a single header like "X-Forwarded-For" or "CF-Client-IP",

Personally i have been using "X-Forwarded-For" but there is a chance some other people might be unhappy.

Anyways this pull request is already a banger i can recommend

src/webui/webapplication.cpp

Co-authored-by: qix67 Co-authored-by: HiFiPhile <admin@hifiphile.com>

Co-authored-by: Vladimir Golovnev <glassez@yandex.ru>

Chocobo1

Only one last comment, I'm fine with the rest.

src/webui/webapplication.cpp

glassez · 2021-06-23T06:01:48Z

Thank you!

glassez · 2021-06-23T06:03:40Z

@HiFiPhile
I hope you intend to maintain it if there are any related problems in the future.

HiFiPhile · 2021-06-23T06:23:34Z

I hope you intend to maintain it if there are any related problems in the future.

Yes in general I maintain my commits.

Co-authored-by: qix67 Co-authored-by: HiFiPhile <admin@hifiphile.com>

FranciscoPombal added Network Issues related to network connectivity WebUI WebUI-related issues/changes labels May 31, 2021

HiFiPhile force-pushed the proxy branch 2 times, most recently from a325aa2 to d2dcc47 Compare May 31, 2021 21:30

glassez removed the WebUI WebUI-related issues/changes label Jun 1, 2021

thalieht reviewed Jun 1, 2021

View reviewed changes

src/base/http/connection.cpp Outdated Show resolved Hide resolved

src/base/http/connection.cpp Outdated Show resolved Hide resolved

src/gui/optionsdialog.ui Outdated Show resolved Hide resolved

src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved

glassez reviewed Jun 1, 2021

View reviewed changes

src/base/http/connection.cpp Outdated Show resolved Hide resolved

HiFiPhile force-pushed the proxy branch from 021494a to 0251843 Compare June 2, 2021 21:47

glassez changed the title ~~Add WebUI reverse proxy source ip resolution.~~ Add WebUI reverse proxy source ip resolution Jun 4, 2021

glassez reviewed Jun 4, 2021

View reviewed changes

src/webui/webapplication.h Outdated Show resolved Hide resolved

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

HiFiPhile force-pushed the proxy branch 2 times, most recently from 68cf37c to 3a1ccf4 Compare June 4, 2021 07:47

glassez reviewed Jun 4, 2021

View reviewed changes

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

HiFiPhile force-pushed the proxy branch from 685b74f to f232019 Compare June 4, 2021 17:53

glassez changed the title ~~Add WebUI reverse proxy source ip resolution~~ Add WebUI reverse proxy source IP resolution Jun 5, 2021

glassez reviewed Jun 5, 2021

View reviewed changes

glassez reviewed Jun 7, 2021

View reviewed changes

glassez reviewed Jun 9, 2021

View reviewed changes

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

glassez requested a review from Chocobo1 June 10, 2021 06:25

glassez added this to the 4.4.0 milestone Jun 10, 2021

Chocobo1 reviewed Jun 12, 2021

View reviewed changes

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

HiFiPhile mentioned this pull request Jun 13, 2021

qbittorrent-nox + Nginx (config from wiki) + 5 failed logins = ban of 127.0.0.1 #11908

Closed

glassez requested changes Jun 14, 2021

View reviewed changes

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

HiFiPhile added 4 commits June 15, 2021 22:09

Add WebUI reverse proxy source ip resolution.

b3565d8

Co-authored-by: qix67 Co-authored-by: HiFiPhile <admin@hifiphile.com>

Add enable checkbox, code formatting.

f4336df

Move reverse proxy address resolution to WebApplication class.

3b04460

Code clean-up

e845b56

HiFiPhile and others added 7 commits June 15, 2021 22:09

Move condition check into resolveClientAddress(), rename peer to client

7c3b751

Add missing replacement

b41d554

Code refactor

80e8cd7

Renaming options, add proxy list support.

7901275

Fix coding style

c393e3f

Update regarding review.

77ec7f6

Use Qt::SkipEmptyParts

2c4233c

Co-authored-by: Vladimir Golovnev <glassez@yandex.ru>

HiFiPhile force-pushed the proxy branch from a6b4602 to 2c4233c Compare June 15, 2021 20:16

glassez requested review from Chocobo1 and glassez June 20, 2021 03:48

Chocobo1 reviewed Jun 20, 2021

View reviewed changes

src/webui/webapplication.cpp Outdated Show resolved Hide resolved

Use connection IP in CSR & host header validation

c18be67

HiFiPhile requested a review from Chocobo1 June 20, 2021 14:48

Chocobo1 approved these changes Jun 21, 2021

View reviewed changes

glassez approved these changes Jun 23, 2021

View reviewed changes

glassez merged commit f5315d9 into qbittorrent:master Jun 23, 2021

IceCodeNew pushed a commit to IceCodeNew/qBittorrent-Enhanced-Edition that referenced this pull request Jun 23, 2021

Add WebUI reverse proxy source IP resolution (qbittorrent#15047)

2faebcb

Co-authored-by: qix67 Co-authored-by: HiFiPhile <admin@hifiphile.com>

AbeniMatteo mentioned this pull request Jul 4, 2021

WebAPI: Provide filtering torrents by tag #15152

Merged

FranciscoPombal mentioned this pull request Jul 4, 2021

Document missing WebAPI 2.8.3 changes in the wiki #15158

Open

4 tasks

likol mentioned this pull request Oct 16, 2021

WebUI authentication can not bypass by whitelist when service behind reverse proxy #15582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebUI reverse proxy source IP resolution #15047

Add WebUI reverse proxy source IP resolution #15047

HiFiPhile commented May 31, 2021

glassez commented Jun 1, 2021

thalieht left a comment

glassez Jun 5, 2021

glassez Jun 6, 2021

HiFiPhile Jun 6, 2021

glassez Jun 5, 2021

qix67 Jun 5, 2021

glassez Jun 5, 2021

qix67 Jun 5, 2021

glassez Jun 5, 2021

HiFiPhile Jun 5, 2021

qix67 Jun 6, 2021

glassez Jun 6, 2021

HiFiPhile Jun 6, 2021

glassez Jun 6, 2021

glassez Jun 7, 2021

HiFiPhile Jun 7, 2021

glassez Jun 7, 2021

HiFiPhile Jun 7, 2021

Walter-o commented Jun 14, 2021

Chocobo1 left a comment

glassez commented Jun 23, 2021

glassez commented Jun 23, 2021

HiFiPhile commented Jun 23, 2021

Add WebUI reverse proxy source IP resolution #15047

Add WebUI reverse proxy source IP resolution #15047

Conversation

HiFiPhile commented May 31, 2021

glassez commented Jun 1, 2021

thalieht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Walter-o commented Jun 14, 2021

Chocobo1 left a comment

Choose a reason for hiding this comment

glassez commented Jun 23, 2021

glassez commented Jun 23, 2021

HiFiPhile commented Jun 23, 2021