@mneudert opened this Pull Request on October 14th 2016 Member

Refs #5728:

  • Updates password hashing
  • Updates token_auth hashing

work in progress...

@mneudert commented on October 14th 2016 Member

I dropped most of the travis testing work to not have it completely run all the time. That is somewhat a development artifact and of course will be removed before a merge is considered. If the actual pull request should do the full testing cycle I can easily remove it and just keep it for development in different branches (as to not completely mess up notifications/comments here).

For the mentioned caveats/quirks:

Password is wrapped with bcrypt during update and an option called
${login}_legacy_password takes care of later identifying those migrated

Upon regular login this flag triggers an automatic update. That is, password
gets re-hashed with pure bcrypt and option removed.

password_hash configuration

Currently only the default bcrypt configuration is used. This should of course
move to some config flags. Including a change of the actual hashing algorithm
to whatever password_hash supports.

password_hashed database field

As the login mechanisms allow passing an MD5 hashed password (like the
python log importer) there is a secondary storage under password_hashed
that wraps the MD5 password with a layer of bcrypt.

Upon every password change this field also gets a new round of hashing.

The only way I know to work around this double storage is to completely wrap
the password with bcrypt around MD5. That would also allow us to re-hash the
password during url authentication.


As the passwords are now non-deterministic most of the XML fixtures make the
tests fail. The fixture check could replace the password fields with some value
to prevent these failures. However that would mean any test using a fixture to
check a change in the password would never detect any failure...


Only changed the database field to a length viable to accomodate a SHA256 hash.

Every new auth token would directly be generated with that length while the old
ones would stay in place and working.

Regenerating that one single token could be done using a simple button in the
admin interface. Imho the first password change while still having an old token
should also regenerate the hash with a new one and then drop that connection.

@tsteur commented on October 31st 2016 Member

Looks promising so far 👍 What would be nice to have in this PR already is to generate the auth token randomly instead of based on username + password. I can't point you to where it is done right now as I cannot run my IDE right now but I'm sure you'll find it. Let me know if not

@mneudert commented on November 1st 2016 Member

The token hashing is something I am working on locally. I will include it here once I have something working well enough to receive a look.

Just changing it (plugins/UsersManager/API.php should be the place) looks promising, but the calls are quite scattered. Running the tests is taking quite some time to find everything :D

@tsteur commented on November 1st 2016 Member

BTW: With token hash generation I mean https://github.com/piwik/piwik/blob/2.17.0/plugins/UsersManager/API.php#L793

Here it may be fine to do just something like return md5($userLogin . $md5Password . microtime() . \Piwik\Common::generateUniqId() . Config::getInstance()->General['salt']) instead for now :)

@tsteur commented on November 18th 2016 Member

@mneudert are you done with your changes so far? I'd have a look at the weekend and give it a test / review as we want to release Piwik 3 in the near future :)

@mneudert commented on November 18th 2016 Member

It is missing the "click on a button and renew your token" UI-Stuff. However everything non-visible should be at least ready for review.

@tsteur commented on November 21st 2016 Member

@mattab ok to have the UI feature to re-generate token at any time and to no longer show the token in the UI? This is pretty much standard behaviour nowadays on all secure platforms / services. This way we get secure tokens and secure passwords in DB finally while existing tokens would still work. We would just need to update FAQs etc. @mneudert did you solve the problem that we send along the auth token in the UI when issueing API requests via XHR? Or did I understand it wrong and tokens are pretty much still stored in plain text?

@mneudert commented on November 21st 2016 Member

The tokens are still stored plaintext but disconnected from the passwords. Having temporary tokens for UI (and external tools like the python-log-importer) would of course be nice, but I think that is more fit for a separate PR working only on that part.

@tsteur commented on November 21st 2016 Member

:+1: Awesome, I hope to have a look today

@tsteur commented on November 22nd 2016 Member

Note to myself: We need to add a note to developer changelog that after this update there is no "going back" possible. This can be tricky when switching between branches regularly. In my case on my local machine I made a copy of the user table to be able to switch between branches more easily in the future. We might need a command for this in the future or so. Any ideas appreciated @sgiehl @mattab

@tsteur commented on November 22nd 2016 Member

Looks promising, great work 👍 Mentioned a few things but they are probably "easy" to change

@mattab commented on November 22nd 2016 Member

ok to have the UI feature to re-generate token at any time and to no longer show the token in the UI? This is pretty much standard behaviour nowadays on all secure platforms / services.

@tsteur Interesting point. In the "Manage users" listing we only show the first few characters of each token_auth, and the full token on click (to avoid people over-looking or even screenshots leaking tokens). In the But the token_auth is also displayed in other places for example "API" page we list the full token_auth which is not consistent, we should also list only first chars. (but still leave it visible if needed as it is transmitted in the page for other reasons anyway).

I guess what you mean is to make Piwik more secure by also completely remove token_auth from the page html output: would prevent stealing passwords over HTTP. but this would be extra work as the token_auth is used in the JS a few times, likely out of scope for this PR?

@tsteur commented on November 22nd 2016 Member

It is actually not implement yet in this PR and we need to implement this next. Either for 3.0 or for 3.X (or at the very latest 4.0). We can never show the token anywhere in the UI. Only after the token generation or re-generation. Otherwise we store the token in plain text and this is always insecure.

@tsteur commented on November 22nd 2016 Member

For JS requests etc we need to create temporary tokens or something. But this will be all a separate PR

@tsteur commented on November 23rd 2016 Member

@mneudert will you have a chance to work on it this week / weekend? I would otherwise try to finish it on Friday or so

@tsteur commented on November 23rd 2016 Member

Actually, would you be able to work on it by Friday? I can otherwise have a look at it as some of the changes need to be quickly done. Because this change is quite big we want to get the change out soon before 3.0 RC to have enough people testing it. No worries at all if you don't find the time

@mneudert commented on November 24th 2016 Member

I think I have addressed all comments (at least I marked them for myself being addressed). Reaching a "nearly mergeable" state I also removed my temporary modifications to the travis configuration. Should all be fine for another review :+1:

@tsteur commented on November 25th 2016 Member

automated tests on travis seem to pretty much work 👍 Only few random test failures because token/password is now random but that's fine. We can mark those fields to be ignored. Gonna have a look at PR again

@tsteur commented on November 25th 2016 Member

Nice 👍 we are getting there 👍 A few minor tweaks missing only I'd say. Maybe @mattab can have a look as well, not sure what else we need to test but worked for me after fixing the update

@tsteur commented on November 26th 2016 Member

@mneudert 👍 well done. Only have 2 minor translation updates I think. Ideally we do not show the auth tokens of all users anymore in the UI but we can do this in a separate PR / issue as it is not related to this PR

@tsteur commented on November 27th 2016 Member

Looks good to merge to me. One translation change is needed but can do this later. @mattab ok to merge and then release a beta version?

@tsteur commented on November 30th 2016 Member

@mneudert for now the only change needed would be to move sha256 back to md5 so we don't break APIs and SDK. Any chance you can work on this in the next 24 hours? As It is a small change I reckon we could otherwise also merge this PR and I apply the change afterwards

@tsteur commented on November 30th 2016 Member

Merging @mneudert Kudos !!! so good to have secure passwords. I will make the changes here and switch to md5 quickly

This Pull Request was closed on November 30th 2016
Powered by GitHub Issue Mirror