Added TOTP feature #173

yuki · 2017-12-18T21:23:46Z

Now there's a new option (disabled by default) to create a new
password, based on the TOTP module for openldap, that the user
must have installed and configured
https://github.com/openldap/openldap/tree/master/contrib/slapd-modules/passwd/totp

To get the new password, the user must reset it by "resetbytoken" in
the new menu option, and after click the link, the user will obtain
a QR code that must scan with an app (freeOTP, for example) before
submit the form.

Now there's a new option (disabled by default) to create a new password, based on the TOTP module for openldap, that the user must have installed and configured https://github.com/openldap/openldap/tree/master/contrib/slapd-modules/passwd/totp To get the new password, the user must reset it by "resetbytoken" in the new menu option, and after click the link, the user will obtain a QR code that must scan with an app (freeOTP, for example) before submit the form.

yuki · 2017-12-18T21:34:55Z

I haven't seen that there were some tests :-/ I have tested the option I have added and it's working. I have added two needed git submodules (for base32, needed for TOTP module in openldap) and the one to create QRcode.

coudot · 2017-12-19T07:53:48Z

Indeed, you must set the translation strings in all lang files. Put the english translation string everywhere you are not able to translate.

I will need some time to test this PR. I let also @plewin review the code.

Added translations strings used for TOTP feature into all langs

yuki · 2017-12-19T11:01:34Z

Added the translations. The errors from Codacy tests are from twig section, but not sure how to solve it.

plewin

I think the implementation in SSP is smart, it uses efficiently what we have today.

plewin · 2017-12-19T11:35:46Z

.gitmodules

@@ -0,0 +1,6 @@
+[submodule "lib/vendor/base32"]
+	path = lib/vendor/base32
+	url = https://github.com/tuupola/base32.git


How did you choose this library ?
Based on https://packagist.org/?q=base32
I would have prefered
https://github.com/paragonie/constant_time_encoding
https://github.com/ChristianRiesen/base32

plewin · 2017-12-19T11:37:05Z

.gitmodules

+	url = https://github.com/tuupola/base32.git
+[submodule "lib/vendor/phpqrcode"]
+	path = lib/vendor/phpqrcode
+	url = https://git.code.sf.net/p/phpqrcode/git


I am ok with this library.
We may need to switch to a fork or a replacement if SSP adopt composer.

plewin · 2017-12-19T11:40:09Z

README.md

+```
+git submodule update --init lib/vendor/phpqrcode
+git submodule update --init lib/vendor/base32
+```


Please add a git checkout to a specific commit (plus a comment on version tested) to each of these submodules.
We never know if any of these libraries start adding commits that will break your code or add backdoors.

plewin · 2017-12-19T11:42:57Z

README.md

@@ -39,6 +39,14 @@ It has the following features:
 * valid PHP mail server configuration (reset mail)
 * valid PHP session configuration (reset mail)



php-gd2 is required for this feature and should be mentionned

plewin · 2017-12-19T11:48:41Z

pages/changetotp.php

@@ -0,0 +1,232 @@
+<?php


This whole file has only 10 characters difference with sendtoken.php, not including a comment line
I understand that the code in sendtoken.php is not reusable without duplicating.
Maybe include()ing sendtoken.php in changetotp would prevent this duplication.
A variable can be added in changetotp.php and detected in sendtoken.php to change the url to the changetotp's one in the mail.

plewin · 2017-12-19T11:56:07Z

pages/resetbytoken.php

+                require_once("lib/vendor/base32/src/Base32.php");
+                require_once("lib/vendor/base32/src/Base32/PhpEncoder.php");
+                $base32 = new Tuupola\Base32;
+                $randomString = $base32->encode(bin2hex(openssl_random_pseudo_bytes (15)));


Please do not use openssl_random_pseudo_bytes()
Use random_bytes() already provided by random_compat/defuse-crypto (there are used in functions.php)

plewin · 2017-12-19T12:01:14Z

pages/resetbytoken.php

@@ -181,7 +192,18 @@

 # Change password
 if ($result === "") {
-    $result = change_password($ldap, $userdn, $newpassword, $ad_mode, $ad_options, $samba_mode, $samba_options, $shadow_options, $hash, $hash_options, "", "");
+    if ($type === "totp") {


I have a problem with how "type" is handled.
The admin can disable resetpassword by email but users can still call this feature by omiting the "type=totp" from the url. I am not sure how to prevent this.

plewin · 2017-12-19T12:06:45Z

pages/resetbytoken.php

    'action' => $action,
-    'source' => $source,


I think there is a problem with the menu, it showed me multiple highlighted links.

plewin · 2017-12-19T12:17:16Z

Hi @yuki,

Thank you a lot for the PR. Do not worry about Codacy, it is not well configured.

There is something I do not understand, after you used your changetotp feature, do you have 2 entries of userPassword in your ldap ?
I think I misunderstand something, is the code in ssp replacing your password by the totp secret, making it impossible to connect to ldap with a password but only the totp token ?

yuki · 2017-12-22T09:38:18Z

Hi @plewin

I'll try to answer all your questions.

I choose this base32 library because I see that was very simple, and solved what I need. I didn't want to take a big project that has more functionalities that base32 (for base64 PHP has its own functions).

I copy the file sendtoken.php to create changetotp.php. The basic functionality is the same, but because of the code, I didn't see any function to be reused, so I decided to copy the file and make minor changes. We could delete the file changetotp.php and use sendtoken.php and make inside the changes using an URL variable (similar to the resetbytoken.php).

About "type" variable in resetbytoken.php, you're right. I wasn't sure how to make it in the right way. We could take care about the global variables.

About the menu, I saw that there were 2 highlighted links, not sure why (the sms and mail ones). I wasn't sure if it was a bug, so I added to highlight the TOTP. I can change that.

The TOTP plugin in LDAP (right now is in beta mode) does the magic with the "{TOTP1}base32hash" in the password field. As you can see, I forced to be a clear password, so with a "slapcat" an administrator can see the base32hash of the user. With this base32 in your app (freetotp, for example) it generates the "one time password", so when you try to authenticate against the LDAP, the LDAP plugin does the same algorithm to be sure that the digits you introduce and what the plugin returns are the same.

If a user has 2 different passwords (one based on SSHA and othe in TOTP modes), if the user tries to authenticate, it can use any of both passwords, the TOTP one or the SSHA. I'm not sure if we can force to authenticate against one password and not the other :-/ So, my idea is that my users will have a new user (or the same user, but in other branch of the LDAP tree) with only TOTP password, and my app will only use this branch in the authentication, to be sure that they must use TOTP password.

I have done this modifications because I need them for my users, and because I didn't see nothing to make it. If you're working hard in the new branch for 2.0 to refactor code and so on, we can put this pull-request in "sleep" mode and put it in your new branch in the future. If you need some help, I'll try to help you in the new branch if you want. If you have some ideas of how the project will evolve (in the wiki, for example), please let me know and I'll try to help.

By the way, if you're ok, I'll try to make the modifications this night with your suggestions, and I'll check if there's another base32 library with "composer" mode.

Thanks for this project! :D

plewin · 2017-12-22T14:01:08Z

I choose this base32 library because I see that was very simple, and solved what I need. I didn't want to take a big project that has more functionalities that base32 (for base64 PHP has its own functions).

Okay, I think we can keep it for now.

I copy the file sendtoken.php to create changetotp.php. The basic functionality is the same, but because of the code, I didn't see any function to be reused, so I decided to copy the file and make minor changes. We could delete the file changetotp.php and use sendtoken.php and make inside the changes using an URL variable (similar to the resetbytoken.php).

I believe keeping the file changetotp.php it is fine so we don't have to change how url -> action -> pages are handled. sendtoken.php yes will need to detect if it was included by changetotp.php and change its url variable.

About "type" variable in resetbytoken.php, you're right. I wasn't sure how to make it in the right way. We could take care about the global variables.

I think it is okay to call die() at the top of resetbytoken.php if available_actions does not contain sendtoken and does not contain also sendsms and type is not present.

About the menu, I saw that there were 2 highlighted links, not sure why (the sms and mail ones). I wasn't sure if it was a bug, so I added to highlight the TOTP. I can change that.

Now that you mention it, yes it is a visual bug I introduced when the templates got extracted and converted to twig. You do not have to handle it. I have a fix in a other branch so it will be solved if it gets merged, if it is not merged, i will port the fix for master.

The TOTP plugin in LDAP (right now is in beta mode) does the magic with the "{TOTP1}base32hash" in the password field. As you can see, I forced to be a clear password, so with a "slapcat" an administrator can see the base32hash of the user. With this base32 in your app (freetotp, for example) it generates the "one time password", so when you try to authenticate against the LDAP, the LDAP plugin does the same algorithm to be sure that the digits you introduce and what the plugin returns are the same.

I understand this but I am suprised that the php ldap code in change_password does not overwrite the normal password. I guess I have to test the topt module myself and make some experiments.

If a user has 2 different passwords (one based on SSHA and othe in TOTP modes), if the user tries to authenticate, it can use any of both passwords, the TOTP one or the SSHA. I'm not sure if we can force to authenticate against one password and not the other :-/

My understanding is that is it not possible to test against one or another with a bind. Applications that want to use totp should add a new textfield in the login page and bind test both password and totp code.

I guess applications should also assert password input != totp code to avoid tricking the system.
I hope there is a way to specifically test the TOTP otherwise php applications won't be able to use totp in two steps forms.

So, my idea is that my users will have a new user (or the same user, but in other branch of the LDAP tree) with only TOTP password, and my app will only use this branch in the authentication, to be sure that they must use TOTP password.

This sounds strange to me, "only use this branch in the authentication" " with only TOTP password", so you want a 1FA with totp ? 6 digits pin code is only 10**6 = 1000000 possibilities.

I have done this modifications because I need them for my users, and because I didn't see nothing to make it. If you're working hard in the new branch for 2.0 to refactor code and so on, we can put this pull-request in "sleep" mode and put it in your new branch in the future. If you need some help, I'll try to help you in the new branch if you want. If you have some ideas of how the project will evolve (in the wiki, for example), please let me know and I'll try to help.

Do not worry about the 2.0 proposal. If this PR gets in master i will port it to 2.0 proposal too.

By the way, if you're ok, I'll try to make the modifications this night with your suggestions, and I'll check if there's another base32 library with "composer" mode.

The library is okay do not worry, there is no rush.

yuki · 2017-12-22T14:27:19Z

I understand this but I am suprised that the php ldap code in change_password does not overwrite the normal password. I guess I have to test the topt module myself and make some experiments.

I don't understand what you mean with this, sorry. The function I used "change_password" is the one of your code. I only force to set "$hash" as "clear" and add the header based of the totp algorithm ("{TOTP256}" if sha256, or "{TOTP512}" if sha512), and it overwrites my previous password (doesn't care if it was SSHA, clear, TOTP, or MD5). Here you have a slapcat of my test user, so maybe you can make an idea:

dn: cn=yuki,ou=groups,dc=example,dc=com
uidNumber: 1000
gidNumber: 1000
cn: rgomez
homeDirectory: /home/yuki
sn: san
uid: yuki
mail: [email protected]
givenName: yuki
objectClass: posixAccount
objectClass: inetOrgPerson
objectClass: top
userPassword: {TOTP1}GE2TKOBYMM4DGZRVG5QWIOBTGMYTCM3CMM4WKMTEGQ4DKMTG

My real user, as "userPassword" has a value like:

userPassword: {CRYPT}$6$X5$SAxgZVXqX..3oolAcPnkHaFddkfqKfOU3...

This sounds strange to me, "only use this branch in the authentication" " with only TOTP password", so you want a 1FA with totp ? 6 digits pin code is only 10**6 = 1000000 possibilities.

My web-app is only accesible via VPN at this moment. The idea is that the users doesn't have the "password remember" of the web browser, because we see as a security breach inside the office and outside. Right now we prefer a 6 digits password (random and only usable for 30 seconds) than a very long difficult password but the the browser has saved. BTW, the TOTP module for LDAP, allows 8 or 10 digits passwords, but it must change the code before compilation. I compiled the TOTP module against Debian's OpenLDAP's code. I have a "how I did" the compilation, so if you need to make your test ask for it and I'll translate into english and put somewhere.

As I mentioned, I'll try to make the changes of your suggestions tonight.

stevleibelt · 2023-02-14T07:01:32Z

Is this still relevant or should you close it @coudot?

coudot requested review from plewin and coudot December 19, 2017 07:52

Added TOTP translations into all langs

4822b2a

Added translations strings used for TOTP feature into all langs

plewin requested changes Dec 19, 2017

View reviewed changes

coudot added this to the Future milestone Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TOTP feature #173

Added TOTP feature #173

yuki commented Dec 18, 2017

yuki commented Dec 18, 2017

coudot commented Dec 19, 2017

yuki commented Dec 19, 2017

plewin left a comment

plewin Dec 19, 2017

plewin Dec 19, 2017

plewin Dec 19, 2017

plewin Dec 19, 2017

plewin Dec 19, 2017 •

edited

Loading

plewin Dec 19, 2017

plewin Dec 19, 2017

plewin Dec 19, 2017

plewin commented Dec 19, 2017 •

edited

Loading

yuki commented Dec 22, 2017

plewin commented Dec 22, 2017

yuki commented Dec 22, 2017

stevleibelt commented Feb 14, 2023

		@@ -39,6 +39,14 @@ It has the following features:
		* valid PHP mail server configuration (reset mail)
		* valid PHP session configuration (reset mail)

Added TOTP feature #173

Are you sure you want to change the base?

Added TOTP feature #173

Conversation

yuki commented Dec 18, 2017

yuki commented Dec 18, 2017

coudot commented Dec 19, 2017

yuki commented Dec 19, 2017

plewin left a comment

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin Dec 19, 2017

Choose a reason for hiding this comment

plewin commented Dec 19, 2017 • edited Loading

yuki commented Dec 22, 2017

plewin commented Dec 22, 2017

yuki commented Dec 22, 2017

stevleibelt commented Feb 14, 2023

plewin Dec 19, 2017 •

edited

Loading

plewin commented Dec 19, 2017 •

edited

Loading