Drop WebUI default credentials

[glassez]

This is how it is supposed to behave:

1. WebUI is still disabled by default for regular build and enabled for no-GUI one.
2. There are no default credentials for now (default username "admin" is still supported). no-GUI build uses temporary auto generated password until user sets custom one. Existing regular installations that had enabled WebUI with default credentials will fail to start WebUI until user sets custom password.

Note that credentials explicitly set to "admin:adminadmin" are considered as valid.

[sledgehammer999]

Two quick comments. Not a full review.

[staze]

Note that credentials explicitly set to "admin:adminadmin" are considered as valid.

Will this prevent previous default credentials from working?

[glassez]

Note that credentials explicitly set to "admin:adminadmin" are considered as valid.

Will this prevent previous default credentials from working?

Sorry, I don't understand the essence of the question...
The behavior is described in the OP. qBittorrent will no longer have default credentials. "admin:adminadmin" won't work anymore unless you explicitly configured qBittorrent to have such credentials.

[staze]

Note that credentials explicitly set to "admin:adminadmin" are considered as valid.

Will this prevent previous default credentials from working?

Sorry, I don't understand the essence of the question... The behavior is described in the OP. qBittorrent will no longer have default credentials. "admin:adminadmin" won't work anymore unless you explicitly configured qBittorrent to have such credentials.

On new installs, makes perfect sense, there's no default, it'll be randomized, etc.

But if you updated from a previous version, and have web enabled, the default credentials exist and are "valid"... during the upgrade, does it check the hashed value of that password, say "it's adminadmin, must be default" and disable it? I'm trying to determine if the update process to whatever version will "close" the security vulnerability.

[glassez]

But if you updated from a previous version, and have web enabled, the default credentials exist

You don't seem to understand what the "default credentials" means. This only applies if there are no credentials explicitly specified in the configuration file.

[staze]

But if you updated from a previous version, and have web enabled, the default credentials exist

You don't seem to understand what the "default credentials" means. This only applies if there are no credentials explicitly specified in the configuration file.

I do. I just don't know how they're implemented in qbittorrent.
If you don't set credentials on old versions, did it leave the credentials null and only accepted admin:adminadmin, or did it set them to admin:adminadmin? If the latter, then they're "set in the configuration file" and won't be invalidated by this patch... since you've said "admin:adminadmin" if explicitly set, is valid.

I'm not sure how else to word this. If you can manually set "admin:adminadmin" as credentials (which, to me seems incorrect and should be prevented since it's a known, default, credential), how are we preventing the old default credentials from continuing to work if the user just upgraded from an old version rather than a new install?

[glassez]

I just don't know how they're implemented in qbittorrent.

If you don't set it there should be nothing in config file. Just test it yourself.

[staze]

gotcha. so the config file being null for that setting means it's not overriding the built in credentials. understood.

would still argue that admin:adminadmin should be disallowed, but probably a discussion for another day. Thanks for the answers! =)

[glassez]

would still argue that admin:adminadmin should be disallowed, but probably a discussion for another day.

#18735 (comment)

[glassez]

PR is rebased to resolve conflicts.

[sledgehammer999]

I have no idea on how to handle the WebUI-HTML part.

In case @Chocobo1 (or someone else) doesn't propose a reasonable solution, I am not against merging this PR as-is.

I don't know how best to do the same on the WebUI. I should at least send there information about whether there is password configured at the moment (aka has_webui_password_configured).

Well, seems there is no such problem for WebUI. After all, in order to change these settings from the WebUI, the user must be logged in, i.e. some password is configured in any case (or a temporary one is used).

Unless, he accessed the webui because auth was disabled for local access and then he disabled that setting via the webui.
PS: I wouldn't mind the has_webui_password_configured solution if nothing else is proposed.

[glassez]

Unless, he accessed the webui because auth was disabled for local access and then he disabled that setting via the webui.

I thought about this case. It still has a temporary password.
IMO, anything else is beyond the scope of this PR.

[sledgehammer999]

IMO, anything else is beyond the scope of this PR.

Can you consider the following change?
In

qBittorrent/src/webui/api/authcontroller.cpp

Line 86 in 28b5d72

setResult(u"Fails."_s);

Change the current "Fails." string to "Invalid Username or Password."
Then add an additional specialized check for failure while on tempPassword. In that case the string should be something like "A temporary password has been generated because no password has been configured. Find it in the console output. Restarting qBittorrent will generate a new temporary password."

And finally change

qBittorrent/src/webui/www/public/scripts/login.js

Line 51 in 28b5d72

errorMsgElement.textContent = 'QBT_TR(Invalid Username or Password.)QBT_TR[CONTEXT=HttpServer]';

to

errorMsgElement.textContent = xhr.responseText;

This way the user won't be dumbfounded on why he can't login anymore.

[glassez]

This way the user won't be dumbfounded on why he can't login anymore.

I wouldn't do anything more than announce on the official website that "default" credentials are no longer supported.
Moreover, your proposal will lead to incompatible API changes in a minor update which is bad idea.
@Chocobo1, what do you think?

[sledgehammer999]

I don't think it is an incompatible API change. Especially if you look the JS code around the linked line.
Currently the API responses:

1. HTTP 403 + (may have response text attached, I didn't check code)-> User's IP is banned for too many failed login attempts
2. HTPP 200 + Response text "Ok." -> logged in
3. HTPP 200 + Response text "Fails." -> log failure, presumably invalid user/pass

New:
4. HTPP 200 + Response text "Specialized error string" -> log failure, explained in response text

The breakage would happen if we changed the log success response.

PS: I amend my previous comment. Fails. should stay. Only add a specialized response for temp password failure.

[glassez]

The breakage would happen if we changed the log success response.

Some WebAPI users can legitimately count on the fact that it has only two options, Ok or Fails, and, for example, determine an unsuccessful result only by testing it for Fails. So these are incompatible changes if it will return something else.
And, by the way, it is incorrect to judge such things based on the built-in WebUI. If we only cared about it, we would not need to talk about any compatibility of the API changes.

[sledgehammer999]

Sadly you're correct:
https://github.com/bill-ahmed/qbit-matUI/blob/34610c40289ef8c107fad4e06c8d840c5069ae5b/projects/login/src/app/login-page/login-page.component.ts#L74-L75

[sledgehammer999]

Closes #13833
Closes #16529
Closes #18731
Closes #19738