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

Disable SuperUser #1779

Closed
wants to merge 2 commits into from

Conversation

@ghost
Copy link

commented Aug 9, 2015

The account "SuperUser" is known to exist on every server. It is also known this account is secured only by a password. No matter how strong the password is choosen it is only my second choice if I can delegate administration to a cert-authed user and have access to the server process (to re-enable the SuperUser password if it is really necessary). Looking for a way to disable SuperUser account (or at least the password) I got to this feature request: https://sourceforge.net/p/mumble/feature-requests/1130/.
Reading the code doing the authentication I learned it is sufficient to set SuperUser password in the DB to NULL. The attached changes deliver an UI to do that.

9x6 added 2 commits Aug 9, 2015
murmurd: command line argument "-disablesu" disables SuperUser
Password login was always allowed for SuperUser. -disablesu
removes SuperUser password. Certificate based authentication
is never allowed for SuperUser. In consequence SuperUser can
authenticate neither with password nor with certificate.
Set new password to allow login for SuperUser again.
ServerDB.cpp: refactor code writing SuperUser password into DB
setSUPW and disableSU used same code to write into database.
This duplication is removed and replaced with private function
writeSUPW.
@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

I like this feature.

Curious what other people think.

Code looks fine at a glance. Will take a deeper dive soon. (Should also consider whether nulling the fields is a feature or an accident. Should probably be documented at password check time as well.)

@hacst

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

I think there are some practical reasons for SuperUser only having password auth at this point. For one the account might reasonably be accessed by multiple people. Also - at least for normal users - murmur pretty much ignores the username and uses the cert hash to look up what account is the right one. Not sure whether that would backfire if SuperUser started to use a certificate. At the moment we have no sane way to easily switch to different certificates for different accounts in the client.

In general I of course agree that cert auth is stronger and preferrable.

@ghost

This comment has been minimized.

Copy link
Author

commented Aug 9, 2015

I totally agree that SuperUser should start as an active account with password authentication. But after initial setup I'd like to administrate with another account. ACLs make this easily possible.
So there is no need for cert-auth'ing SuperUser. But there is also no need for an active SuperUser account during normal operation - let alone an active one where a password can be guessed/brute forced.

The other point, that there is no sane way to switch to different certificates for different accounts in the client, is true; that feature would still be nice if I manage the server using different accounts. But that's a solvable problem in the client which should not stop an admin to get some extra security in the server. And, the admin has actively act to disable SuperUser, so this issue won't affect anybody who hasn't choose to run into it.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

Being able to unset the superuser password, and by that effectively disabling the superuser account is definitely a good idea.

The superuser is meant as a fallback and initial setup account, which is only to be used in rare and very specific circumstances.
As such, it should (as it currently is) not use user certificate authentication, but stay (potentially automatically disabling) fallback account that gives other access and allows retrieving access from an overtake - by being able to set a superuser PW via the command line (or other adequate measures, like Ice / administration webinterface).

@Kissaki

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

kudos for splitting refactoring and logical change into two commits :)

In several places (when introducing new lines) spaces are used for indentation instead of tabs.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

I guess if the superuser user does not exist yet in the DB; we do not automatically create it on login.
So I would have questioned actually inserting the superuser account with a NULL password.

However, as that logic is not in the helper method setSUPW (second commit) that’s fine I guess.

Changes look good to me (apart from the minor indentation issue).
I did not compile or run these changes.

@Kissaki Kissaki added the murmur label Oct 3, 2015

@Kissaki

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

I guess Ice can already set NULL values on the superuser, so we do not need a disablesuperuser function?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2015

@9x6 I was about to merge this, but found that your commits have no email address attached to them. We very much prefer having an email address on file, if at all possible.

If you reply with it here, I can fix up the commits to have the new address when landing.

Also, sorry for the wait. The patch is pretty much perfect as-is, as far as I am concerned. Thanks!

@ghost

This comment has been minimized.

Copy link
Author

commented Oct 20, 2015

I'm pretty busy at the moment. I'll try to adress both issues till this weekend:

a) Tab-indentation, right?
b) I'll create a new branch, you can pull from with e-mail adrdress.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

We can fix indentation for you. No problem.

If you just provide the email here, we can fix that as well.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Nov 21, 2015

Ping.

@9x6 If you provide an email address, we can make the necessary changes and land this. Thanks! :-)

@mkrautz mkrautz self-assigned this Apr 26, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Apr 26, 2016

Any news on this?

We've begun not requiring email addresses for contributions. We can pretty much land this now.

But since it's been a while, I'd like to take one last look before we land, which is why I assigned myself.

@9x6 If you want an email credited for this, you can always have it added later via the .mailmap file.

mkrautz added a commit that referenced this pull request May 22, 2016
@mkrautz

This comment has been minimized.

Copy link
Member

commented May 22, 2016

Landed via f990b90.

@mkrautz mkrautz closed this May 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.