Updates password/token_auth hashing #10740

mneudert · 2016-10-14T18:52:38Z

Updates password hashing
Updates token_auth hashing

work in progress...

mneudert · 2016-10-14T18:55:08Z

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
users.

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.

testing

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...

token_auth

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 · 2016-10-31T00:46:05Z

core/Updates/3.0.0-b2.php

+
+            $db->query(
+                'UPDATE `' . $userTable . '`'
+                . ' SET `password` = :password, `password_hashed` = :password_hashed'


Will there be also an update for token_auth = hash('sha256, token_auth)? sha2 was only added recently in MySQL 5.5 so we might not be able to use it actually. Will need to do it in PHP unfortunately

Actually, I understand the problem is then we can no longer show the token in the UI so we can do this in a separate PR 👍

tsteur · 2016-10-31T00:47:53Z

core/Updates/3.0.0-b2.php

+    private function getUserDatabaseMigrations($queries)
+    {
+        $queries[] = $this->migration->db->changeColumn($this->userTable, 'password', 'password', 'VARCHAR(255) NOT NULL');
+        $queries[] = $this->migration->db->addColumn($this->userTable, 'password_hashed', 'VARCHAR(255) NOT NULL', 'password');


I'm not quite sure, do we need the password_hashed actually? Or could we just upgrade all password fields and then try 1) password_verify($password), and if it fails try password_verify(md5(password)) or as we know by the option table which ones were upgraded we would know directly whether to apply md5 or not?

I would love to remove that field. The problem is that this would break at least the UsersManager.getTokenAuth API used in piwik-log-analytics script (line 919 onwards) when using the --password parameter.

It being used during testing is probably easily changed. The only occurrence I found was in the ManySitesImportedLogs fixture.

But there may be other things that rely on calling UsersManager.getTokenAuth so to me it looks like it might be a necessary evil for the time being.

I understand now. I will have a think about it as I am using this method in Piwik Mobile as well.

So my thoughts on this: Can we maybe change getTokenAuth to return the current auth_token for a user? This may be the hashed sha256(token_auth) version or as long as it is still saved as md5 the actual token. The method would try to log in the user and if username / password combination is correct return that token.

One problem might be that we are expecting an md5 password there whereas new passwords are stored as password_hash($rawPassword). So now I am thinking would it be so bad to instead always save passwords as password_hash(md5($rawPassword)). So all user passwords would be immediately hashed, no option entry is needed and we stay backwards compatible with that getTokenAuth etc. I think that might make everything more easy

BTW: We would then need a new method generateTokenAuth but this would be likely not needed in the API :) getTokenAuth() would then do what it actually says meaning returning a token, not generating it :)

I think getTokenAuth and generateTokenAuth require the separation you mentioned already for being able to include any random data. Before accessing the token the authentication should have already happened so it should be perfectly fine to just access the user record and return what is found there.

The generation API might still need some sort of public interface as with random generation there is no need to reference the password anymore. So the token could be generated completely independent from password changes. The first password change should automatically do the migration away from the old MD5 token with an updated warning in the interface.

Regarding the passwords I also thought about just applying bcrypt around already hashed MD5 passwords. Besides rendering the legacy option obsolete it would ensure full backwards compatibility without the current "hack".

It leaves some of the problems with MD5 in place like collision vulnerability or the fact that bcrypt now works on a fixed length hash instead of potential 72 chars. But that is already a problem due to the API requirements for the MD5 login. At least it made the problem obvious :)

+1 to no longer regenerate token when password changes but then we need to have a feature in the UI to generate a new token. I'd say we could just create a new issue for this to not generate a new token when password changes to keep things easy and doable for now. I'd still include the username in the generation but password not needed anymore indeed.

Regarding the passwords I also thought about just applying bcrypt around already hashed MD5 passwords.
I really like that idea. In Piwik 4 we could then still think about removing the md5($password) and breaking the API.

Will you work on this in the next days so we can get rid of password_hashed column and maybe merge soon? :)

My plan was to have everything integrated and updated during the weekend. Ideally with the added token_auth changes including the adaptations to the admin area.

tsteur · 2016-10-31T00:51:03Z

plugins/Login/Auth.php

+            return new AuthResult(AuthResult::FAILURE, $login, null);
+        }
+
+        $legacyOption = sprintf('%s_legacy_password', $user['login']);


I wonder if we need the option or whether we could always try password_verify($password, $user['password']) || password_verify(md5($password), $user['password']) . It may be slightly less secure as it could match when someone enters a wrong password but the chances for this are quite unlikely. Probably it is better to have the option entry 👍

tsteur · 2016-10-31T00:53:10Z

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 · 2016-11-01T17:06:26Z

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 · 2016-11-01T20:55:35Z

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 · 2016-11-18T03:54:24Z

@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 · 2016-11-18T08:41:11Z

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 · 2016-11-21T03:33:51Z

@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 · 2016-11-21T09:38:54Z

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 · 2016-11-21T19:46:34Z

👍 Awesome, I hope to have a look today

tsteur · 2016-11-22T00:08:15Z

core/Updates/3.0.0-b4.php

+
+        foreach ($users as $user) {
+            $db->query(
+                'UPDATE `' . $userTable . '`'


I'm not sure if we should show those update statements in the UI @mattab ? There shouldn't be any sensitive password leak

@mneudert we could add them to the migrations via $this->migration->db->boundSql($query, $bind)

FYI: Usually via use ? instead of :password. Not sure if there would be any problems with any DB adapters or so. Maybe better to use ? instead.

✅ (marking for myself before updating the PR)

If the question mark is the usual style we should stick with it. No need to risk any DB adapter to go haywire.

I decided to not include them in the migration listing thinking about the "query per user" that would appear. It might not be a problem at all but it might just make the migration log completely unusable when having too many users.

The only update I can find using the direct query style is `2.10.0-b5'. Should the updater take care of the queries even if they won't get shown for compatibility/style reasons here?

@mattab do you have thoughts on whether we need to show the queries? Probably we don't need to show them as they would be executed afterwards anyway when they perform the update so probably better to maybe not show them.

Updater will take care of them anyway 👍

Agreed it is not needed to display them during updater in this case

tsteur · 2016-11-22T00:16:02Z

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 · 2016-11-22T00:18:33Z

plugins/UsersManager/angularjs/personal-settings/personal-settings.controller.js

@@ -7,13 +7,13 @@
 (function () {
    angular.module('piwikApp').controller('PersonalSettingsController', PersonalSettingsController);

-    PersonalSettingsController.$inject = ['piwikApi'];
+    PersonalSettingsController.$inject = ['piwikApi', 'window'];


Typo: should be $window. Causes the personal settings to not be shown

✅ (marking for myself before updating the PR)

tsteur · 2016-11-22T00:20:05Z

plugins/UsersManager/angularjs/manage-users/manage-users.controller.js

@@ -176,6 +180,23 @@
            }});
        };

+        this.regenerateUserTokenAuth = function (userLogin) {
+            console.log('foo');


console.log should be removed

✅ (marking for myself before updating the PR)

tsteur · 2016-11-22T00:20:57Z

plugins/UsersManager/templates/userSettings.twig

@@ -107,6 +107,15 @@
    </form>
 </div>

+<div piwik-content-block
+     content-title="{{ 'UsersManager_TokenAuth'|translate|e('html_attr') }}">
+    <p id="token_auth_user" data-token="{{ userTokenAuth }}">{{ userTokenAuth }}</p>


Can we wrap the token in <pre piwik-select-on-focus>...</pre>? Then it will be formatted as "code" and be selectable with just one click

Do we need the token in the attribute again or could we just read $('#token_auth_user').text()? Not important though I think

✅ (marking for myself before updating the PR)

tsteur · 2016-11-22T00:22:48Z

plugins/UsersManager/templates/userSettings.twig

+
+    <button class="btn btn-link"
+            ng-controller="PersonalSettingsController as personalSettings"
+            ng-click="personalSettings.regenerateTokenAuth()">{{ 'UsersManager_RegenerateTitle'|translate }}</button>


After regenerating the token I got logged out. We should mention this in a <p>...</p> before. Eg When you re-generate your token you will be logged out and need to log in again

Maybe we could also add something like "Once the token has been regenerated don't forget to update the token in case you are using the token to export reports via the reporting API, to track data or to embed widgets"

Is there any chance to update the token without logging a user out?

Having the regenerateTokenAuth API return the new token it might be interesting to extend the piwik JS-service to allow updating the token. Updating ones token might be a rather rare action to take but logouts are never a nice response.

Or just stick with the current logout? (perhaps just for now)

I have no clue honestly, I don't mind people being logged out we would only need to mention it

✅ (marking for myself before updating the PR)

tsteur · 2016-11-22T00:28:19Z

plugins/UsersManager/templates/index.twig

@@ -178,6 +178,7 @@
                {% if showLastSeen is defined and showLastSeen %}
                <th>{{ 'UsersManager_LastSeen'|translate }}</th>
                {% endif %}
+                <th>token_auth</th>


Can we maybe remove this column and instead use an icon in front of the shown token? The button is quite big in there and rarely needed. eg

<span class="icon-reload"></span> token... We could maybe also find a new icon and add it

Yeah, that part is a rather ugly one. Personally I would like to completely separate the token management from the user management, like just having a separate piwik-content-block only listing the tokens and the regenerate buttons next to them.

The warning message for passwords should not feel to confusing having separate blocks. From a responsive layout perspective it might also be easier on the eyes to have those long strings separated if there are any plans to go like below 540px for everything (currently the users block needs like 9xx pixels and displaying the data veritcally (each cell as a row) might be nicer with less data per user).

We could have a separate content block for it but it could be a long list when there are > 30k users. We would need to have a user select field or a user login auto complete field similar as we have for "give view access" feature under "Manage access". It is only visible to users with admin access but not to users with super user access. We could later reuse the same logic from "give view access" .

The whole users manager needs a big re-design eventually. For now it would be good to just move that big button out and maybe replace it with a tiny icon in the existing column. In another PR we could add a new content block where users can type in the login name or email address and then re-generate their token

✅ (marking for myself before updating the PR)

Using.icon-reload seems to fit nicely (displaying it already before token expansion).

mattab · 2016-11-29T11:09:13Z

plugins/UsersManager/lang/en.json

@@ -8,7 +8,8 @@
        "AnonymousUserHasViewAccess2": "Your analytics reports and your visitors information are publicly viewable.",
        "ApplyToAllWebsites": "Apply to all websites",
        "ChangeAllConfirm": "Are you sure you want to give '%s' access to all the websites?",
-        "ChangePasswordConfirm": "Changing the password will also change the user's token_auth. Do you really want to continue?",
+        "ChangePasswordConfirm": "Changing the password will also change the user's old API authentication token which may be used to access the Piwik APIs. This token will be re-generated only once. This is a security measure to keep your Piwik data safe. Do you really want to continue?",
+        "ChangePasswordConfirmSelf": "Changing the password will also change your old API authentication token which may be used to access the Piwik APIs. This token will be re-generated only once. This is a security measure to keep your Piwik data safe. Proceeding may require you to log in again. Do you really want to continue?",


@mneudert it would be great to refactor translation strings so that each sub-string ideally only appears once. This has many advantages especially around making text consistent in different languages and keeping translators working more efficiently :-) For example here, the string can be Proceeding may require you to log in again. and reuse ChangePasswordConfirm

mattab · 2016-11-29T11:11:23Z

plugins/UsersManager/lang/en.json

@@ -63,6 +64,11 @@
        "SuperUserAccessManagementMainDescription": "Super users have the highest permissions. They can perform all administrative tasks such as adding new websites to monitor, adding users, changing user permissions, activating and deactivating plugins and even installing new plugins from the Marketplace.",
        "TheLoginScreen": "The login screen",
        "ThereAreCurrentlyNRegisteredUsers": "There are currently %s registered users.",
+        "TokenAuth": "API Authentication Token",
+        "TokenRegenerateConfirm": "Changing the API authentication token will invalidate the user's current token. If the user is currently logged in, he may be required to log in again. Do you really want to continue?",
+        "TokenRegenerateConfirmSelf": "Changing the API authentication token will invalidate your own token. You may be required to log in again if you proceed. Do you really want to change your authentication token?",


here you could reuse the string TokenRegenerateConfirmSelf within TokenRegenerateConfirmSelf (removing the substring You may be required to log in again if you proceed.)

IMO it's not needed. It totally depends on the translation etc. and splitting it up in too many parts is sometimes just not practical. In this part also the first part is different etc and having different keys allows us to use better, more user friendly wording.

tsteur · 2016-11-30T20:40:59Z

@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 · 2016-11-30T23:43:44Z

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

mattab modified the milestone: 3.0.0-b2 Oct 17, 2016

mattab added the c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. label Oct 30, 2016

mattab modified the milestones: 3.0.0-b2, 3.0.0-b3 Oct 30, 2016

tsteur reviewed Oct 31, 2016

View reviewed changes

mattab modified the milestones: 3.0.0-b3, 3.0.0-b4 Nov 15, 2016

tsteur reviewed Nov 22, 2016

View reviewed changes

mneudert added 18 commits November 27, 2016 20:39

converts user.password to VARCHAR(255)

594afa8

re-hashes existing passwords with bcrypt

07201d7

re-hashes legacy password after first login

b5c4804

converts user.token_auth to CHAR(64)

b80e144

creates/updates users with randomized token_auth

1360622

performs authentication for getTokenAuth

6fc4d41

shows authentication token on personal settings page

ea4afae

hides password change confirmation for modern token_auth

4c7cb0a

adds hint about token regeneration to api page

2649573

allows users to regenerate their token_auth

cf1f5e5

allows admins to regenerate users token_auth

8078ab9

cleans up unused "use" statements

9f09424

extracts password interaction to separate class

b2dcb47

improves "password changes token" confirmation wording

8e8e883

requires confirmation before token regeneration

9380a31

removes Password class from UsersManager model

0447092

cleans up testing environment (passwords / tokens)

14107b7

bumps version to 3.0.0-b4

bf9c59f

mattab reviewed Nov 29, 2016

View reviewed changes

tsteur changed the base branch from 3.x-dev to password3x November 30, 2016 23:42

tsteur merged commit c2d50bd into matomo-org:password3x Nov 30, 2016

tsteur mentioned this pull request Dec 1, 2016

Password hashing #10926

Merged

This was referenced Dec 1, 2016

Super User should not be able to see token_auth of other users #10938

Closed

Update FAQ: How do I change the Super User’s password when password recovery emails are not sent? #10941

Closed

c-prompt mentioned this pull request Dec 26, 2016

Clarify FAQ instructions for logme mechanism (automatically logging in users when they click from my app to Piwik) #11094

Closed

mneudert deleted the issue-5728 branch July 9, 2017 17:48

sgiehl mentioned this pull request Feb 4, 2018

"Wrong Username and password combination" after upgrade to 3.x #12536

Closed

Updates password/token_auth hashing #10740

Updates password/token_auth hashing #10740

Conversation

mneudert commented Oct 14, 2016

mneudert commented Oct 14, 2016

password_hash configuration

password_hashed database field

testing

token_auth

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur Oct 31, 2016 • edited

Choose a reason for hiding this comment

tsteur commented Oct 31, 2016

mneudert commented Nov 1, 2016

tsteur commented Nov 1, 2016 • edited

tsteur commented Nov 18, 2016

mneudert commented Nov 18, 2016

tsteur commented Nov 21, 2016

mneudert commented Nov 21, 2016

tsteur commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Nov 30, 2016

tsteur commented Nov 30, 2016

tsteur Oct 31, 2016 •

edited

tsteur commented Nov 1, 2016 •

edited