Improve channel password handling #3929

Krzmbrzl · 2019-12-28T15:22:51Z

This is aimed to fix #3857 and to fix #3856

Changes of this PR:

Add 2 fields to the MumbleProto::channelState msg indicating whether a channel is protected by ACLs denying ENTER privilege and whether the user the message is being sent to has the permissions to enter the channel
Implement the backend to process the abovementioned fields (Mumble + Murmur)
Added the equivalents of those message-fields as fields in the channel class inside the Mumble-client
UI: Show lock-icon if channel is access-restricted (locked lock for the case in which the local user can't enter the respective channel and unlocked lock for the case in which the local user has the necessary privileges to enter the channel noetheless)
UI (kinda): Changed permission denied message for ENTER or MOVE requests, if the channel has access-restrictions (the message now states that a missing password/access token might be the cause for the denial)

Krzmbrzl · 2019-12-29T11:16:29Z

I implemented #3856 and this is how it looks now:

Note that there are 2 different symbols: A closed (red) lock and an opened (green) lock. The former is shown, if the channel ~~is password-protected and you don't have a matching access token set~~ is enter-restricted and you don't have ENTER privilege. The latter is shown, if the channel ~~is password-protected, but you have a matching access token configured~~ is enter-restricted, but the local client has ENTER privileges in it. These locks don't actually indicate whether you can enter those channels though as they don't take other ACLs into account (clearly seen by the fact that even the SuperUser is presented with a red lock).

EDIT: After the refactor, the locks indicate indeed whether you can or cannot enter a channel.

What do you think about this feature?

Krzmbrzl · 2019-12-29T12:01:26Z

Btw.: This is how it looks for the classic theme. I used the icons suggest in #3872 (comment) for it.

Krzmbrzl · 2019-12-29T12:35:34Z

I guess this PR is pretty much done for now. Once #3927 gets merged, I could add a popup asking for a password once the permission denied message (most likely associated with invalid password / access token) is received and then try again with that password set...

Johni0702 · 2020-01-02T14:36:32Z

I strongly disagree with sending password hashes to all clients as that'll allow any client to crack them by offline brute force.
As is, a client would have to send two packets (one for setting its tokens and one for testing if they worked, latter ones afaict have a strict rate limit) per X passwords they'd like to try (limited by packet size iirc), making brute force infeasible (or at least non-trivial). An offline attack is multiple orders of magnitude faster (e.g. this SO post names ~100M sha512 hashes per second, it's from 2012) making it almost trivial to brute force channel passwords (which generally won't be particularly strong by their nature).

Less relevant given the above is already a deal breaker but still worth mentioning:

The salt seems fairly pointless given it's the same every time
Do not use SHA512 for password hashing, it's fast by design
Unless you know exactly what you're doing, don't roll your own crypto

Instead I'd suggest adding two bool flags (e.g. "has_lock", "is_locked") to the ChannelState message and comparing tokens on the server-side. And make sure there's no way to update them without a rate limit (probably best to just add a rate limit to the Authenticate message).

Krzmbrzl · 2020-01-02T14:52:18Z

Thanks for your input.

First of all, let me state that I don't see channel passwords to be such a valuable target that someone might actually go through and bruteforce them (and discussions like #3926 suggest to me that they aren't considered that valuable to others)

That being said though, I agree that if we manage to get the features with a safer approach, that's definitely better.

The salt seems fairly pointless given it's the same every time

As the client needs to be able to also compute the hash, I don't see a good way of making a variable salt (unless maybe randomly generated and transmitted to the client on connect. Though that of course kinda breaks the purpose of a salt, I guess). I added the salt simply to prevent someone not affiliated with Mumble's code base from using rainbow tables to crack the hash. I'm far from being an expert on these things though, so feel free to correct me in that (and anything that follows).

Do not use SHA512 for password hashing, it's fast by design

What would you suggest instead?

Unless you know exactly what you're doing, don't roll your own crypto

Does the current state if this PR fall under this? 🤔

Instead I'd suggest adding two bool flags (e.g. "has_lock", "is_locked") to the ChannelState message and comparing tokens on the server-side.

So every time ACL change, the server shall update all clients with new, personalized channel info? I guess that could work as well. Dunno if this could lead to too much overhead on the server-side, though I can't really imagine this getting a problem.
I'll give this some thought, but on first glance, this definitely is a good alternative.
Plus it has the advantage of also being able to respect ACLs other than the access tokens, making the approach much more universal :)

Kissaki · 2020-01-03T17:46:00Z

If possible please keep PRs as distinctive and simple as possible. This now seems to tackle multiple things in multiple commits and without an explanation in the PR description making it harder to review and get into/understand. It also makes it impossible to land partials/selected parts.

Krzmbrzl · 2020-01-03T18:43:11Z

This now seems to tackle multiple things in multiple commits and without an explanation in the PR description making it harder to review and get into/understand

Yes it does 2 things. But the second one is simply changing the error message if ENTER privliges were denied in certain cases and this change is only possible because of the changes im the first one.
Therefore the only option would have been to create 3 PRs: backend, feature1, feature2
I don't think that this would contribute a lot to readability of the PR, if one starts to make multiple PRs that are dependent on one another...

The very short "description" I provided was originally intended to be expanded, but I forgot. Sorry for that (I'll do that right now).

Krzmbrzl · 2020-01-03T18:49:26Z

Oh and the dependency to #3933 is gone in the latest refactoring. Hopefully I'll find the time to squash the commits in this PR tomorrow to clean up the current mess xD

@Johni0702 I implemented your proposal and removed the password-hashing :)

Krzmbrzl · 2020-01-04T11:36:09Z

@Kissaki I restructured my commits and from the commit messages alongside what else is written in this PR, I hope it will be relatively easy to follow the changes along.

Note also that the build fails, because mumble-voip/mumble-theme#21 is not merged yet. once that is merged, the code in this PR should build just fine (as shown by the CI of the second last commit.

davidebeatrici · 2020-01-28T22:36:50Z

Is backwards compatibility maintained?

Krzmbrzl · 2020-01-29T06:58:50Z

I don't see how it would break it. The whole thing is implemented by extending the protocol by two fields. A server that doesn't know it, won't populate the fields in the first place and a client that doesn't know them won't read them. Each of these cases will result in the icons not being shown, but as far as I can tell, there shouldn't be a problem other than that.

I haven't actually tested the above though. Might be worth a try. However it might take a bit until I'll be able to do so, as my beloved Linux notebook is in repair and I just don't want to fiddle around with setting up a build environment on Windows xD

src/ACL.cpp

Krzmbrzl · 2020-02-27T15:57:31Z

So I did test connecting to a normal server and there was no problem doing that. And I just can't imagine that there is a problem for a client connecting to a Server running with this patch as it simply won't process the extra information the Server now provides :)

…hannelState message in order for the protocol to be able to tell clients about which channels they will be able to enter

… grant enter rights to the channel, in the permission denied message, if the denied permission is ENTER and the channel has (to the client's knowledge) enter restrictions

…d or an unlocked one, depending on whether the user can enter the channel or not)

toby63 · 2020-06-09T16:39:21Z

Sry but as I already said in mumble-voip/mumble-theme#17 (comment) I think this implementation is too confusing.
If a user sees the green symbol they assume that they can enter the channel, but are asked for a password, this is inconsistent design.

I think about two solutions:

For channels which are access-restricted by ACL, Hide Channel for Groups #3057 should be applied, so making them invisible for normal users (or everyone who is not admin or group member).
For channels which are only access-restricted via password, use a lock-symbol to indicate that it is access-restricted.

To seperate these two scenarios better from each other, you might apply a different ACL-system.
We would have two groups:

Exclusive Groups (apply to scenario 1 see above): they cannot be joined via a password, only via an acl that is set by an admin or group leader or group member etc.
Channel Groups (aka non-exclusive groups) (apply to scenario 2 see above): they can be joined via a password and are visible to all normal users.

Maybe technology-wise my concept is already (partly) implemented, but the UI should also represent this.

Krzmbrzl · 2020-06-09T16:40:44Z

If a user sees the green symbol they assume that they can enter the channel, but are asked for a password, this is inconsistent design.

Where did you get that from?
If the lock is green, the user can enter the channel without any further actions. Only when it's red, that user has to configure the access tokens (password) accordingly in order to access the channel.

Krzmbrzl · 2020-06-09T16:42:26Z

For channels which are access-restricted by ACL, #3057 should be applied, so making them invisible for normal users (or everyone who is not admin or group member).

I disagree 100%. If I don't even see the channel, I would never think about setting my access tokens. This would confuse the hell out of our users

For channels which are only access-restricted via password, use a lock-symbol to indicate that it is access-restricted.

The password is in fact also only an ACL

toby63 · 2020-06-09T16:49:13Z

Very confusing. I don't have the time to dig into that. If you think that everyone can understand that, then let it be.

I would never think about setting my access tokens

What should that mean?
Either you are the admin and can set access tokens or not.

The password is in fact also only an ACL

I know that.
But as I already clarified:

Maybe technology-wise my concept is already (partly) implemented, but the UI should also represent this.

this is about meta-functionality, if you can follow me.
Because thats what is needed for people to understand the system correctly.

toby63 · 2020-06-09T17:05:51Z

If a user sees the green symbol they assume that they can enter the channel, but are asked for a password, this is inconsistent design.

Where did you get that from?
If the lock is green, the user can enter the channel without any further actions. Only when it's red, that user has to configure the access tokens (password) accordingly in order to access the channel.

I see, I missunderstood this whole thing then.
I thought the unlocked-icon would indicate that you have enter permission as a normal user, but would need to type in a password to join the channel.
And the locked-icon indicates that you can't join it, because you are not part of the right group and also cannot join via a password.

Nonetheless you should streamline this concept of access-tokens, passwords and acls, it's very confusing as it is now.
And to remind once again, it is not necessarily about changes behind the scenes, but UI changes etc. are useful to get a better concept of this.

Krzmbrzl · 2020-06-09T17:35:37Z

Nonetheless you should streamline this concept of access-tokens, passwords and acls, it's very confusing as it is now.

green & unlocked == you can enter
red & locked == you can't enter

what is confusing about this?

toby63 · 2020-06-09T21:40:36Z

As I said I missunderstood the functionality, still this is not perfect.

red & locked == you can't enter

Thats for example a potencial point of confusion.
A user might ask "is it impossible to join the channel? or is it just password-protected?".
With only one neutral lock symbol (like the golden symbol already mentioned) this is not so confusing,

what is confusing about this?

Well this specific lock symbol concept is not perfect, but somewhat ok.
What I mean in the first place is the overall concept.
This mix of ACL, Groups, Password, Access Tokens etc.
I think it should be changed to be more clear.
#3247 is one point.
Another point is the confusing mix of password, access tokens and groups.
But I don't really know a way to solve this, without making it more complicated.
I thought about a pure password option (groups can still be used in the background, but would be deleted on password change).
But that would complicate it, because of one more option.

Anyway I also don't fully understand the whole functionality, so as of now I am no help in "redesigning" it.

Krzmbrzl · 2020-06-10T06:03:32Z

With only one neutral lock symbol (like the golden symbol already mentioned) this is not so confusing,

One could argue that the user could ask himself the same question here. Additionally that user might be wondering if he can join the channel right now.

Besides: Whether you can't join due to not knowing the password or due to groups doesn't actually matter. The effect is the same. And once somebody gives you the password, you add it to your access tokens and then you'll see what channels it will grant access to.

Krzmbrzl · 2020-06-10T06:04:33Z

Oh and just to avoid misunderstandings: This PR of mine didn't introduce channel passwords or the way restricted channels are handled. I merely added an indication of it to the UI ☝️

streaps · 2020-06-10T15:41:54Z

This implementation that turns tokens into "passwords", if they have a specific set of permissions, is confusing.

Channels can have more than one token (for different sets of permissions), tokens might even deny permissions. There might be Enter restrictions that are completely irrelevant for the user. E.g. the ACL group very_annoying_users that only denies 3 users out of 200 the permission to enter a channel.

I don't believe this PR really improves "password" handling, because user don't use passwords for channel access, only tokens. "Passwords" are nothing more than some cruel UX horror in Channel Edit window.

The lock icon also does not reliably indicate that a room is locked or unlocked by a token. It's nearly impossible for an average user to form a stable mental model about this lock icon (without knowing how ACLs work in detail).

Improving the token system for the user is a lot more complicated than that.

Krzmbrzl · 2020-06-10T17:11:47Z

This implementation that turns tokens into "passwords", if they have a specific set of permissions, is confusing.

I know. But its better to have a function for that than simply implicitly knowing that if a group is formed in a certain way, we'll treat it as a password (which was the case before this PR)

I don't believe this PR really improves "password" handling, because user don't use passwords for channel access, only tokens. "Passwords" are nothing more than some cruel UX horror in Channel Edit window.

So? The title of this PR is not reflected in the code, so what exactly is your point here?

Improving the token system for the user is a lot more complicated than that.

I never claimed I fixed all problems.

The lock icon also does not reliably indicate that a room is locked or unlocked by a token. It's nearly impossible for an average user to form a stable mental model about this lock icon (without knowing how ACLs work in detail).

Green = locked, but can enter
Red = can't enter
I repeat myself but I think this is about as simple as it gets. If a user may enter because he is not affected by the ACLs, then the lock will be green.
Please elaborate on a situation in which there is potential for confusion (and be explicit, please).

With the introduction of mumble-voip#3929 a channel's description would not be accessible by clicking on the comment icon next to the channel's name but rather by clicking to the lock icon to its right (if there was one). This was due to some implicit assumptions about the comment icon's position in the code that is responsible for showing the description. Thus the fix was simply to let that part of the code account for the possible new icon as well. Fixes mumble-voip#4685 Co-Authored-By: Davide Beatrici <git@davidebeatrici.dev>

The following icons were added (with a description) to Qt's "What's this?" help system: - padlocks (mumble-voip#3929) - talking state for locally muted users (mumble-voip#4322) - listener symbol (mumble-voip#4011) Fixes mumble-voip#4686

…k icon With the introduction of #3929 a channel's description would not be accessible by clicking on the comment icon next to the channel's name but rather by clicking to the lock icon to its right (if there was one). This was due to some implicit assumptions about the comment icon's position in the code that is responsible for showing the description. Thus the fix was simply to let that part of the code account for the possible new icon as well. Fixes #4685

The following icons were added (with a description) to Qt's "What's this?" help system: - padlocks (#3929) - talking state for locally muted users (#4322) - listener symbol (#4011) Fixes #4686

Krzmbrzl mentioned this pull request Dec 29, 2019

Feat password at connect dialog iss3856 #3872

Closed

Krzmbrzl mentioned this pull request Dec 29, 2019

Added icons for enter-restricted channels mumble-voip/mumble-theme#21

Merged

Krzmbrzl force-pushed the channel-password-handling branch 2 times, most recently from 7743a1c to befd16c Compare December 29, 2019 12:32

Krzmbrzl changed the title ~~[WIP]: Improve channel password handling~~ Improve channel password handling Dec 29, 2019

Krzmbrzl mentioned this pull request Dec 29, 2019

Pull request for issue #3857 #3915

Closed

Krzmbrzl force-pushed the channel-password-handling branch from 94d1c13 to bb50aa6 Compare January 4, 2020 10:57

Krzmbrzl force-pushed the channel-password-handling branch 2 times, most recently from 2f299fb to 2385c34 Compare January 28, 2020 07:47

Krzmbrzl requested a review from davidebeatrici January 28, 2020 07:47

Krzmbrzl added client needs-testing A feature or fix that is ready but still needs testing before it can be used. labels Feb 8, 2020

Krzmbrzl commented Feb 13, 2020

View reviewed changes

src/ACL.cpp Show resolved Hide resolved

Krzmbrzl force-pushed the channel-password-handling branch from 6d0b1f1 to c885ab3 Compare February 27, 2020 16:00

Krzmbrzl added 4 commits February 28, 2020 18:41

ACL: Added isPassword() function and made use of it in ACLEditor

7f7cc6c

Protocol: Added fields 'is_enter_restricted' and 'can_enter' to the C…

69fe7c6

…hannelState message in order for the protocol to be able to tell clients about which channels they will be able to enter

src/mumble/Messages.cpp: Include hint that adding access tokens might…

dfa90fc

… grant enter rights to the channel, in the permission denied message, if the denied permission is ENTER and the channel has (to the client's knowledge) enter restrictions

Theme (Classic): Added icons for locked and unlocked lock

047c5c0

Krzmbrzl added 2 commits February 28, 2020 18:41

Theme (Mumble): Updated submodule to include new images

3a3644b

UI: Show lock icons next to enter restricted channels (either a locke…

81c0e93

…d or an unlocked one, depending on whether the user can enter the channel or not)

Krzmbrzl force-pushed the channel-password-handling branch from c885ab3 to 81c0e93 Compare February 28, 2020 17:43

Krzmbrzl merged commit 5a4f529 into mumble-voip:master Feb 28, 2020

Krzmbrzl removed the needs-testing A feature or fix that is ready but still needs testing before it can be used. label Mar 2, 2020

Krzmbrzl mentioned this pull request Mar 2, 2020

Possibly wrong description #3928

Closed

This was referenced Mar 31, 2020

Channel and User Group Flags (Icons) #1632

Open

Ask for password upon joining a password-protected channel #4028

Open

Krzmbrzl deleted the channel-password-handling branch April 4, 2020 11:54

Krzmbrzl mentioned this pull request Jun 9, 2020

Theme variants using FontAwesome mumble-voip/mumble-theme#17

Open

toby63 mentioned this pull request Jun 9, 2020

Replace lock icons mumble-voip/mumble-theme#28

Open

toby63 mentioned this pull request Dec 6, 2020

Adjust channel lock icons to adhere to the MetroMumble colour scheme mumble-voip/mumble-theme#31

Merged

Krzmbrzl mentioned this pull request Jan 14, 2021

FIX(client,ui): Channel description via lock icon #4688

Merged

Krzmbrzl mentioned this pull request Jan 14, 2021

FEAT(client): Add new icons to "What's this?" #4690

Merged

Improve channel password handling #3929

Improve channel password handling #3929

Krzmbrzl commented Dec 28, 2019 •

edited

Krzmbrzl commented Dec 29, 2019 •

edited

Krzmbrzl commented Dec 29, 2019

Krzmbrzl commented Dec 29, 2019

Johni0702 commented Jan 2, 2020

Krzmbrzl commented Jan 2, 2020

Kissaki commented Jan 3, 2020

Krzmbrzl commented Jan 3, 2020

Krzmbrzl commented Jan 3, 2020 •

edited

Krzmbrzl commented Jan 4, 2020

davidebeatrici commented Jan 28, 2020

Krzmbrzl commented Jan 29, 2020 •

edited

Krzmbrzl commented Feb 27, 2020

toby63 commented Jun 9, 2020 •

edited

Krzmbrzl commented Jun 9, 2020

Krzmbrzl commented Jun 9, 2020

toby63 commented Jun 9, 2020 •

edited

toby63 commented Jun 9, 2020 •

edited

Krzmbrzl commented Jun 9, 2020

toby63 commented Jun 9, 2020 •

edited

Krzmbrzl commented Jun 10, 2020

Krzmbrzl commented Jun 10, 2020

streaps commented Jun 10, 2020

Krzmbrzl commented Jun 10, 2020

Improve channel password handling #3929

Improve channel password handling #3929

Conversation

Krzmbrzl commented Dec 28, 2019 • edited

Krzmbrzl commented Dec 29, 2019 • edited

Krzmbrzl commented Dec 29, 2019

Krzmbrzl commented Dec 29, 2019

Johni0702 commented Jan 2, 2020

Krzmbrzl commented Jan 2, 2020

Kissaki commented Jan 3, 2020

Krzmbrzl commented Jan 3, 2020

Krzmbrzl commented Jan 3, 2020 • edited

Krzmbrzl commented Jan 4, 2020

davidebeatrici commented Jan 28, 2020

Krzmbrzl commented Jan 29, 2020 • edited

Krzmbrzl commented Feb 27, 2020

toby63 commented Jun 9, 2020 • edited

Krzmbrzl commented Jun 9, 2020

Krzmbrzl commented Jun 9, 2020

toby63 commented Jun 9, 2020 • edited

toby63 commented Jun 9, 2020 • edited

Krzmbrzl commented Jun 9, 2020

toby63 commented Jun 9, 2020 • edited

Krzmbrzl commented Jun 10, 2020

Krzmbrzl commented Jun 10, 2020

streaps commented Jun 10, 2020

Krzmbrzl commented Jun 10, 2020

Krzmbrzl commented Dec 28, 2019 •

edited

Krzmbrzl commented Dec 29, 2019 •

edited

Krzmbrzl commented Jan 3, 2020 •

edited

Krzmbrzl commented Jan 29, 2020 •

edited

toby63 commented Jun 9, 2020 •

edited

toby63 commented Jun 9, 2020 •

edited

toby63 commented Jun 9, 2020 •

edited

toby63 commented Jun 9, 2020 •

edited