Add ip subnet whitelist for bypassing webui auth by Piccirello · Pull Request #6779 · qbittorrent/qBittorrent

Piccirello · 2017-05-13T21:40:05Z

Requested in #924.
Provides a more permanent solution than this wiki page while also allowing the web API auth to be disabled

This feature is useful when qBitttorrent is used behind a reverse proxy with its own authentication scheme, or when it's running on a local network.

Piccirello · 2017-06-14T00:59:34Z

@evsh, @glassez , @Chocobo1, @sledgehammer999: would someone be able to review this PR, or at least comment on it? This is a highly requested feature and I know a lot of people would benefit from having it.

Chocobo1 · 2017-06-14T04:31:27Z

This is a highly requested feature

Don't think so, care to point it out?

I know a lot of people would benefit from having it.

How so? having a simple/mindless password is better than none.
~~Or do you mind to talk about your specific usage that will need this PR?~~

Chocobo1 · 2017-06-14T04:46:58Z

This feature is useful when qBitttorrent is used behind a reverse proxy with its own authentication scheme, or when it's running on a local network.

I agree we could have an option for bypassing auth for users on LAN, for WAN, it's best to keep auth on.

Piccirello · 2017-06-14T05:19:37Z

Don't think so, care to point it out?

#924
#4581
#6518
Disable authentification of webGUI
[Solved] Disable Webui Authorization

A method of disabling webgui auth is even provided in the Compiling qBittorrent on Debian and Ubuntu guide. However, that method requires users to modify their login.html file.

Chocobo1 · 2017-06-14T06:16:38Z

And if users disable authentication, while on public Wi-Fi, how does that protect them then?

I don't get what you're trying to say.
It seems you're talking about different layers of security measures (e.g. public wifi, wifi with WPA2 or not, qbt session auth, http or https).
While a security measure prevents possible exploits (as side effect or not), you shouldn't mix it (multiple security measures) up when thinking about disabling a specific measure.

@Piccirello
Thank you.
However, after skimming through those issues, I read that bypassing LAN is the common request, not bypassing all.

If I wasn't clear enough, I only support the idea of adding a checkbox stated: "Bypass authentication for users on LAN".

glassez · 2017-06-14T08:30:47Z

the idea of adding a checkbox stated: "Bypass authentication for users on LAN".

But this checkbox exists, isn't it?

Chocobo1 · 2017-06-14T08:40:21Z

But this checkbox exists, isn't it?

No, the one exists is Bypass authentication for localhost.

Piccirello · 2017-06-14T17:14:35Z

I agree that the "disable for LAN" option makes more sense from a security standpoint. I'll modify my changes and update when finished.

Piccirello · 2017-06-21T22:00:23Z

I see that env_.clientAddress is storing my ipv4 address 10.0.0.3 in the ipv6 format ::ffff:10.0.0.3. Will this behavior be exhibited by all clients, or will env_.clientAddress ever be in ipv4 format for some users?

glassez · 2017-06-22T03:12:35Z

Shouldn't be OR in this place?

There are many conditions here so it would be better to have two helper functions: isLoopbackAddress and isLANAddress.

Shouldn't be OR in this place?

The LAN conditionals return true if the address isn't localhost OR if local auth is enabled. Therefore if an OR is used and a user connects with a LAN ip, the function would return true regardless of the LAN conditionals.

That being said, development is still underway, so don't start picking apart my changes just yet ;)

Once I'm finished I'll squash the changes and update this PR.

Chocobo1 · 2017-06-22T04:48:09Z

or will env_.clientAddress ever be in ipv4 format for some users?

you'll need to handle 3 formats:

ipv6
ipv4 mapped ipv6, see: https://en.wikipedia.org/wiki/IPv6#IPv4-mapped_IPv6_addresses
ipv4

Piccirello · 2017-06-25T05:40:11Z

Ok, all three scenarios are now handled by the LAN check. Commits are also squashed. What're your thoughts?

Chocobo1 · 2017-06-25T06:08:24Z

I would prefer const QHostAddress &clientAddress

Chocobo1 · 2017-06-25T06:08:29Z

I would prefer const QHostAddress &clientAddress

Chocobo1 · 2017-06-25T06:18:30Z

These 2 can move to src\base\utils\net if you want

At least it shouldn't be member of AbstractWebApplication.

Chocobo1 · 2017-06-25T06:26:32Z

please use:

if () else if () else

No. Don't do it!
It is considered "bad form" to use else after return.

OK, but at least it should be

if () return ... else if () return ... return true;

to save 1 comparison.

You are again mistaken. It saves nothing.

The second comparison will be executed only if the first returns false, because otherwise we will just return from the function.

You are again mistaken. It saves nothing.

You're right. I my head was obsessed with something else.

Chocobo1 · 2017-06-25T06:57:09Z

0a00 and a00 are equivalent.

I know, the suggestion is for clarity.

Chocobo1 · 2017-06-25T07:10:51Z

use isInSubnet(const QHostAddress &subnet, int netmask)?
i.e. isInSubnet(QLatin1String("10.0.0.0"), 8)
and below

Why? They are equivalent.

They are equivalent.

to save 1 function call in our code?

isInSubnet(QLatin1String("10.0.0.0"), 8)

What's the purpose of using QLatin1String in this instance? It's not currently used for creating the ipv6 mapped 127.0.0.1 address.

What's the purpose of using QLatin1String in this instance?

QHostAddress is constructed via QHostAddress(const QString &address) and then from http://doc.qt.io/qt-5/qlatin1string.html#details :

This is a bit longer to type, but it provides exactly the same benefits as the first version of the code, and is faster than converting the Latin-1 strings using QString::fromLatin1().

Chocobo1 · 2017-06-25T07:21:48Z

clientAddress -> addr or some other name
there is no concept of "client" in these 2 helper functions

Done.

I still see "clientAddress"...

Chocobo1 · 2017-06-25T07:43:00Z

there are non-trivial computation in this func, maybe it make sense to add the following?

if (clientAddress.isInSubnet(QLatin1String("0.0.0.0"), 0)) { // ipv4 } else { // ipv6 }

UPDATE: example updated.

example updated

Chocobo1 · 2017-06-25T08:42:38Z

we use implicit type conversion in our codebase, so I'll say there is no need for explicit QHostAddress() construction here.

I can't compile without explicitly passing QHostAddress

base/utils/net.cpp:51:60: error: no matching function for call to ‘QHostAddress::isInSubnet(QLatin1String, int) const’ if (addr.isInSubnet(QLatin1String("0.0.0.0"), 0)) { ^ base/utils/net.cpp:51:60: note: candidates are: In file included from /opt/qt55/include/QtNetwork/QHostAddress:1:0, from base/utils/net.cpp:30: /opt/qt55/include/QtNetwork/qhostaddress.h:116:10: note: bool QHostAddress::isInSubnet(const QHostAddress&, int) const bool isInSubnet(const QHostAddress &subnet, int netmask) const; ^ /opt/qt55/include/QtNetwork/qhostaddress.h:116:10: note: no known conversion for argument 1 from ‘QLatin1String’ to ‘const QHostAddress&’ /opt/qt55/include/QtNetwork/qhostaddress.h:117:10: note: bool QHostAddress::isInSubnet(const QPair<QHostAddress, int>&) const bool isInSubnet(const QPair<QHostAddress, int> &subnet) const; ^ /opt/qt55/include/QtNetwork/qhostaddress.h:117:10: note: candidate expects 1 argument, 2 provided base/utils/net.cpp:62:9: warning: control reaches end of non-void function [-Wreturn-type] } ^

hmmm, OK then.

Chocobo1 · 2017-06-25T08:59:28Z

my review is finished.
👍 for this PR.

glassez · 2017-06-25T10:12:44Z

Please drop else branch and place return statement outside it.

glassez · 2017-06-25T10:13:45Z

Done.

I still see "clientAddress"...

glassez · 2017-06-25T10:15:30Z

Move this include to .cpp

glassez · 2017-06-25T10:20:42Z

And one more question. Is bypass auth for lan but require it for localhost the expected behavior?

glassez · 2017-11-05T05:13:58Z

@Piccirello, I've done it for you (rebased and squashed). You need to force update your local branch.

glassez · 2017-11-05T05:18:19Z

GitHub's "Resolve conflicts" editor is just too tempting!

You shouldn't use GitHub editor. When you need to resolve conflicts in your PR, you need to rebase your local branch on top of current qBittorrent master (manually resolving some conflicts if required) and then force push it on GitHub.

Piccirello · 2017-11-06T01:26:29Z

Thanks @glassez.

glassez · 2017-11-07T14:49:37Z

Thanks @glassez.

@Piccirello, what the hell are you doing? Are you kidding me?
I squashed your fixup commits for you, I even fixed the remaining coding style issues for you... You don't have to do anything anymore. The only thing you had to do is "to force update your local branch" (as I suggested you), to make it identical with PR branch on GitHub. But what did you do? You did the opposite! You just returned everything as it was (with the exception of fixed conflict). I don't even know what to say... Maybe "Please squash your commits and fix the coding style".
(I'm sorry if I was overly emotional).

glassez · 2017-11-07T15:53:56Z

Please squash your commits and fix the coding style

Ok. You can forget about commits squashing since they are unrelated. But coding style issues are still be fixed.

glassez

Fix coding style.
The rest is ok.

Piccirello · 2017-11-07T16:01:47Z

Fix coding style.
The rest is ok.

Care to point out the style issues? I re-pushed the repo because you squashed unrelated commits.

glassez · 2017-11-08T07:08:46Z

Wrong curly brace position.

glassez · 2017-11-08T07:11:43Z

Inconsistent else block. Curly braces are required because of curly braces in if.

Piccirello · 2017-11-19T21:02:14Z

@evsh @Chocobo1 Any further changes required?

WolfganP · 2017-11-20T18:20:22Z

Great work @Piccirello
I hope this great addition is promptly merged into master.

glassez · 2017-11-20T18:54:07Z

@Piccirello, thank you!
Thanks for all reviewers as well!

Piccirello · 2017-11-20T23:01:47Z

Woohoo! Glad we were able to get this merged.

Thanks for the back and forth over many months @Chocobo1 @glassez @evsh

glassez · 2018-01-17T17:12:28Z

+    QStringList authSubnetWhitelistStringList;
+    for (const Utils::Net::Subnet &subnet : pref->getWebUiAuthSubnetWhitelist())
+        authSubnetWhitelistStringList << Utils::Net::subnetToString(subnet);
+    data["bypass_auth_subnet_whitelist"] = authSubnetWhitelistStringList.join("\n");


Unfortunately I noticed it only now... Why have you chosen this out-of-context way to encode this list? We should send it as (json) list, isn't it? I'm sure this should be fixed soon.

nomandera · 2018-07-23T06:18:53Z

Posting for posterity. If you wish to turn of authentication completely, say in the scenario where the web interface is protected by a separate TLS nginx/apache/trafik/haproxy reverse proxy you can add 0.0.0.0\0 to allow the entire IPv4 space which includes the internet.

Insert obvious warning: dont do this if dont know what you are doing, authentication is there for a reason.

Piccirello force-pushed the disable-web-auth branch 2 times, most recently from b4041ff to 32a342f Compare May 19, 2017 03:50

This was referenced May 28, 2017

[Bug] localhost without authentication banned from WebUI #6860

Closed

WHY do I have to use a username and password for LAN WEBSERVER ACCESS? #6518

Closed

Piccirello changed the title ~~Add option for disabling web authentication~~ Add option for disabling webui LAN authentication Jun 20, 2017

glassez requested changes Jun 22, 2017

View reviewed changes

Piccirello force-pushed the disable-web-auth branch from 981b740 to 9ee2abf Compare June 25, 2017 05:23

Chocobo1 reviewed Jun 25, 2017

View reviewed changes

glassez requested changes Jun 25, 2017

View reviewed changes

glassez force-pushed the disable-web-auth branch from d5b6bfd to 2b43372 Compare November 5, 2017 05:10

glassez force-pushed the disable-web-auth branch from 2b43372 to 09d9436 Compare November 5, 2017 07:05

Piccirello force-pushed the disable-web-auth branch from 09d9436 to 04d2e53 Compare November 6, 2017 03:53

Chocobo1 previously approved these changes Nov 7, 2017

View reviewed changes

zeule previously approved these changes Nov 7, 2017

View reviewed changes

glassez requested changes Nov 7, 2017

View reviewed changes

glassez requested changes Nov 8, 2017

View reviewed changes

Piccirello added 3 commits November 8, 2017 22:39

Add ip subnet whitelist for bypassing webui auth

95bf633

Enable preferences Apply button when ip banlist is modified

da0db92

Remove duplicate header

0de2f17

Piccirello dismissed stale reviews from zeule and Chocobo1 via 0de2f17 November 9, 2017 03:39

Piccirello force-pushed the disable-web-auth branch from 04d2e53 to 0de2f17 Compare November 9, 2017 03:39

glassez approved these changes Nov 11, 2017

View reviewed changes

zeule approved these changes Nov 19, 2017

View reviewed changes

Chocobo1 approved these changes Nov 20, 2017

View reviewed changes

glassez merged commit f41cb80 into qbittorrent:master Nov 20, 2017

glassez reviewed Jan 17, 2018

View reviewed changes

Piccirello deleted the disable-web-auth branch July 15, 2019 00:46

Uh oh!

Uh oh!

Conversation

Piccirello commented May 13, 2017

Uh oh!

Piccirello commented Jun 14, 2017

Uh oh!

Chocobo1 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chocobo1 commented Jun 14, 2017

Uh oh!

Piccirello commented Jun 14, 2017

Uh oh!

Chocobo1 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glassez commented Jun 14, 2017

Uh oh!

Chocobo1 commented Jun 14, 2017

Uh oh!

Piccirello commented Jun 14, 2017

Uh oh!

Piccirello commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chocobo1 commented Jun 22, 2017

Uh oh!

Piccirello commented Jun 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piccirello Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chocobo1 Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Chocobo1 commented Jun 14, 2017 •

edited

Loading

Chocobo1 commented Jun 14, 2017 •

edited

Loading

Piccirello commented Jun 21, 2017 •

edited

Loading

Piccirello Jun 25, 2017 •

edited

Loading

Chocobo1 Jun 25, 2017 •

edited

Loading

Chocobo1 Jun 25, 2017 •

edited

Loading

Chocobo1 Jun 25, 2017 •

edited

Loading