Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Blind) Trusting user names for authentication #4960

Closed
EP-u-NW opened this issue May 1, 2021 · 10 comments
Closed

(Blind) Trusting user names for authentication #4960

EP-u-NW opened this issue May 1, 2021 · 10 comments
Labels
feature-request This issue or PR deals with a new feature server

Comments

@EP-u-NW
Copy link
Contributor

EP-u-NW commented May 1, 2021

Murmurs current authentication system either uses a password or a certificate hash to authenticate a user with the backend. If murmur trusts a certificate, because it was issued from a certificate authority that itself trusts (what will be possibly with the MCTS, see #3523) AND if murmur can guarantee that a user is who he is (what will be possibly with "forceUsernameCertSubjectEquality", see #4940), murmur can authenticate that user purly based on the username.

I want to introduce a new murmur.ini option:

blindTrust (default: false)
This option is very dangerous! If blind trust is set to true, murmur trusts the user to be who his username says he is. The only practial case this is useful is if "forceUsernameCertSubjectEquality" is also true, and the MCTS is configured correctly and strongly. However, we won't prevent you from enabeling blindTrust, even if "forceUsernameCertSubjectEquality" is false or your MCTS is not set up correctly, since open source software means freedom, and you are free to do as you please.

We could achive this by altering the authenticate method in ServerDB.cpp slightly, I think of something like this (for line 1321):

		if (bBlindTrust) {
			// We trust the user to be who his name says. This is very dangerous if used incorrectly by server admins!
			name = query.value(1).toString();
			res  = query.value(0).toInt();
		} else if (!storedPasswordHash.isEmpty()) {
			// A user has password authentication enabled if there is a password hash.
			... continue as normal

Why don't just use an external authentication system? The system could return the userid based on the certificate and we would not alter murmur.
BUT user authenticated using an external system are (as of my understanding from this wiki text) members of a temporary group and thus not differentiable when it comes to ACLs. Secondly, using an external authentication system will introduce unnecessary overhead to the system.

EP-u-NW pushed a commit to EP-u-NW/mumble that referenced this issue May 1, 2021
@Krzmbrzl Krzmbrzl added feature-request This issue or PR deals with a new feature server labels May 1, 2021
@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 1, 2021

The question that I pose myself here is: Given that it can be dangerous to enable this option, why would you want to have it in the first place? Aka: What is the actual advantage? Less overhead during registration?

Also I always was under the impression that a certificate check was all that was required to authenticate on most servers anyways. Is this then specifically for servers that have passwords in addition to certificate checks? 🤔

@EP-u-NW
Copy link
Contributor Author

EP-u-NW commented May 1, 2021

Maybe things get clearer if I just explain our overall usecase 😅 :
We want to use mumble for internal communication and restrict usage to internal users. Since all our other internal services use a certificate based authenticaten system (instead of a less secure password based one), our users are given certificates from our own CA. The twist is, that a user can have multiple devices, so the same user will have different certificates issued to him, so different cerficiates with same subject common name but different hashes. Registering a user using one certificate will block any authentication attempt with an other valid certificate the user posseses on an other device. BUT registering is required, since we need ACLs for our users. Now, since we can configure our murmur server to only accept certificates from ower own CA, all users that enter the server are certified by us.
The idea is now the follwoing:

  1. A user posseses a valid certificate, so he is allowed to join the server
  2. The mumble user name matches the name given in the certificate
  3. This is enough to know, that the user is really who he claims to be
  4. We don't need to do murmur user database checks with the certificate hash now, because if we did, the certificate might be rejected since it was not the same as used for registering (different hash) BUT the user is actually the same

I hope it got clearer now :)

@Kissaki
Copy link
Member

Kissaki commented May 2, 2021

From what I have read and understood, this seems a fitting candidate for an Ice authenticator.

This authorization scheme is very specific and special, and you formulated necessary warnings on related settings which have to taken into account to safely use them. So this seems error prone and complicated. Both in program logic we would have to add, as well as for users.

I don’t really see what you see as the problem with using an authenticator. You definitely can configure and apply ACL rules just like normal. I’ve been using a password DB backed authenticator on a server with ACL groups and member accounts in those groups for a long time. Users authenticate to an account, and the account is accessible in ACL.

The wiki link you provide does not talk about temporary data. Maybe you are talking about specific authenticator scripts that are linked from there? As there is a mumble user DB and authenticator backed user DB to authenticate against, IIRC the user DB IDs receive IDs in a much higher range to not cause conflicts in the smf script. But that does not influence the username based ACL handling.

For reference, here is the Ice authenticate function

@Kissaki
Copy link
Member

Kissaki commented May 2, 2021

In your example you could say the certificates themselves are the backing DB.

Your authenticator would not have a backing DB to check against, but check the CA validity, and then accept the authentication with the username in the certificate.

If I understand correctly.

@Kissaki
Copy link
Member

Kissaki commented May 2, 2021

To clarify to be technically correct from the top of my head I’m not actually sure about the username basis of ACLs. But either way from experience it works with authenticator authenticated users too.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 3, 2021

The core functionality you are after is already requested in #2107.

I think the proper way to implement this would be to add real support for having multiple certs per user instead of adding a workaround to skip the cert-hash check.

@EP-u-NW
Copy link
Contributor Author

EP-u-NW commented May 3, 2021

The core functionality you are after is already requested in #2107.

Hm #2107 is close, but not the exact same thing: The issue describes that a user should be able to multi-log (online with multiple devices the same time), we actually prefer the way it is currently (that the "old" user is disconnected). But such details could be solved with an other configuration variable I guess 😬.

Unfortunaly, adding a real "multi certificate infrastructure" is currently out-of-scope for me 🙈.

You definitely can configure and apply ACL rules just like normal.

It's good to hear that users authenticated by an external system can be targeted by ACLs, I think I misunderstood that in the wiki. While I agree with you that it might be error porn in a way a admin could missconfigure the option (thus that much warnings), I tend to disagree that it would introduce problems from the program logic:
https://github.com/EPNW/mumble/blob/7eee5caeb44eb37f127bbe3a3e021c8c2eb61cff/src/murmur/ServerDB.cpp#L1321-L1325

From what I have read and understood, this seems a fitting candidate for an Ice authenticator.

To be totaly honest, it is much more of a hassle for me to learn how to use the Ice authenticator and get it working than adding this three lines of code...
Beside from the effort in creating such an authenticator, it will introduce overhead, both in runtime (making an async call using ice) and in configuration.
An other "hack" I could use to achive the desired behaviour is to add the password "1234" to every user. I think the code here is better suited to explain why this would work then my words 😅. But I don't like this solution since it's messy and undocumented and might change and it also introduces overhead by doing password hashing.

Oh and btw: #2107 also mentions certificate revocations, what is a desirable thing. Unfortunaly, it is currently not possible to add CRLs (certificate revocation lists) to a server. The problem here is that Qt doesn't allow adding a crl to a QSslConfiguration or QSslSocket. I opend a new issue for that, see #4964.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 3, 2021

While I can see how you would benefit from this feature being implemented in Mumble directly, I don't think that this is something that a lot of other users will benefit from as well. Even the contrary might be the case if one deals with server admins that recklessly enable this option without properly understanding the consequences.
The latter wouldn't be a big deal if I saw a wider audience for this feature, but as I said: I don't 🤷

For these reasons I would vote to close this request as out-of-scope for Mumble (at least in the approach described here).
@Kissaki @davidebeatrici your thoughts?

@Kissaki
Copy link
Member

Kissaki commented May 8, 2021

As I wrote above I agree with that reasoning. Very niche functionality, with potential confusion/accidental misuse. At the same time it has a good Ice authenticator alternative that can be used.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 9, 2021

Alright then for the reasons stated above, I will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue or PR deals with a new feature server
Projects
None yet
Development

No branches or pull requests

2 participants