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

Synchronize root and admin password #527

Merged
merged 13 commits into from Nov 4, 2018

Conversation

Projects
None yet
4 participants
@zamentur
Contributor

zamentur commented Aug 29, 2018

The problem

We suspect some instance have 'yunohost' as password. More generally, when people set or change the admin password, it does not affect the root password, which leads to : 1) more confusion between 'what's the admin password' and 'what's the root password', and 2) leaves hole open if the user forgets to change the default root password ...

Solution

Add a migration that check if the root password is 'yunohost' : if so, the root password is automatically replaced by the admin password. Otherwise, the migration is manual and will sync root and admin password. Also, using yunohost tools adminpw will now also impacts the root password.

PR Status

Ready

How to test

# Put yunohost as password
passwd
yunohost  tools migrations migrate --debug
# Check password has changed
su
yunohost tools adminpw --debug
# Check password has changed
su

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Psycojoker

This comment has been minimized.

Member

Psycojoker commented Aug 30, 2018

I have to admit that this migration looks super tricky and breaking prone to me, I don't feel really confident about having it 😨

I know this is frustrating to re-do some existing work but I think it would be way better if we had a mechanism on authentication to check if the password in a blacklist and require a modification on the password at that moment. That would be way more flexible and futur proof because we could extend this blacklist very easily.

Also this PR seems to contain a totally very different modification that what is announce 😐 2d5077e That should be in another PR, definitively.

@zamentur

This comment has been minimized.

Contributor

zamentur commented Aug 30, 2018

I know this is frustrating to re-do some existing work but I think it would be way better if we had a mechanism on authentication to check if the password in a blacklist and require a modification on the password at that moment.

So the main problem for you is the way I add a second password on ldap ?
Note : This process is already used in a lot of case with the yunohost-reset-ldap-password command.

Until today, i never saw someone on forum/irc saying this manipulation break something. I know it's quite tricky.

Also this PR seems to contain a totally very different modification that what is announce neutral_face 2d5077e That should be in another PR, definitively.

yunohost tools adminpw was announced in this PR, see the solution description. The title of this PR is voluntary not very clear, because i didn't want to publish all details in a title...

To explain a little bit, we need during postinstall to change root password with the admin's one. It's the easiest way i found to be sure root password will not be a default password from our images.

In more, having different password between root/admin users is not so simple to understand for users. So it's a solution pretty well to fix that too.

To understand completely the issue, you should read this PR too: #518

That would be way more flexible and futur proof because we could extend this blacklist very easily.

That's not the goal of this PR. To accept good password or not, there is already an other PR #196 .

I already reduce the list of password tested in this PR to the strict minimal, because other contributors here say it's a different case if the user put a weak password voluntarily...

6aef80c

@zamentur zamentur requested review from alexAubin and Psycojoker Sep 6, 2018

@zamentur

This comment has been minimized.

Contributor

zamentur commented Sep 6, 2018

7 days, this fix correct a big issue...

@alexAubin

After a short review : I shall test this tomorrow, but in the meantime, agreed on the principle.

logger.debug('Stop slapd and backup its conf')
run_commands([
# Stop slapd service...
'systemctl stop slapd',

This comment has been minimized.

@alexAubin

alexAubin Sep 6, 2018

Member

Hmm that one looks kinda touchy 😛 I wonder if that's really a good idea (though there are not so many alternatives). I'm guessing that the fact that we're logged as admin (e.g. from the webadmin) is cached somewhere (unscd?) so that should be okay ...

But we should reaaaally make sure that slapd gets back on at some point.... c.f. the "slapd start" at the end of the finally block... I wonder what happens if one of the previous command fails for some reason.

This comment has been minimized.

@zamentur

zamentur Sep 20, 2018

Contributor

run_commands raise a CalledProcessError at the first command that fail. We can disable this behavious by passing a specific callback that don't raise error.

"""
Ask for admin hash the ldap db
Note: to do that like we don't know the admin password we add a second
password

This comment has been minimized.

@alexAubin

alexAubin Sep 20, 2018

Member

I'm testing this and : is this method really needed to get the admin hash ?

I just realized that, as root, you can run slapcat from which you may extract the relevant info. Here's a quick and dirty command to do that (we might want to have a more robust way to do that though) :

slapcat | grep "dn: cn=admin,dc=yunohost,dc=org" -A20 | grep userPassword -A2 | tr -d '\n ' | tr ':' ' ' | awk '{print $2}' | base64 -d

if self._is_root_pwd_listed(SMALL_PWD_LIST):
new_hash = self._get_admin_hash()
self._replace_root_hash(new_hash)

This comment has been minimized.

@alexAubin

alexAubin Oct 24, 2018

Member

Rethinking about this, I think we need to clarify the purpose of this migration ? (Or maybe I'm kinda lost about it)

It seems like this PR does two different things :

  • check if the root password is "bad", and if so, resync it with the admin password
  • change tools_adminpw such that it also changes the root password each time the admin password is modified...

But for existing instance on which the root password ain't "bad", after this migration, we still end up with the root password and admin password being desynchronized. Shouldn't we sync the root and admin password in any case so that we end up with consistent setups in all cases ?

@alexAubin alexAubin changed the title from Migrate pwd to Synchronize root and admin password Oct 25, 2018

@alexAubin alexAubin added this to the 3.x milestone Oct 25, 2018

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

Note that since we are gonna have a few manual migrations now, we should check how this behaves exactly. I'm thinking that if you have several pending migrations (manual or auto), running yunohost tools migrations migrate --accept-disclaimer might run all of them whereas we might want them to be ran one-by-one.

@alexAubin alexAubin removed the work needed label Oct 25, 2018

@alexAubin

Tested and works OK

@Josue-T

This comment has been minimized.

Contributor

Josue-T commented Oct 25, 2018

Will we be able to keep a independent root password after this migration ?

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

Yes and no : the spirit of this PR is that root and admin password shall be identical and shall be resynchronized each time yunohost tools adminpw is ran. You could still have a different root password by running "passwd" as root, but that's not really the spirit.

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 27, 2018

Proposing to merge in a few days ... any more feedback on this ?

@alexAubin alexAubin modified the milestones: 3.x, 3.3.x Oct 31, 2018

@alexAubin alexAubin merged commit 4999cd8 into stretch-unstable Nov 4, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@alexAubin alexAubin deleted the migrate-pwd branch Nov 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment