two factor auth #24559

ChristophWurst · 2016-05-11T09:40:51Z

This PR adds two factor authorization to all requests using the app framework. Unless at least one 2FA app is enabled (install this https://github.com/owncloud/twofactor_email app for testing), nothing changes with respect to authentication. The dummy app included prompts the user for a token sent by email, which is the hard-coded value 'passme'. This 2FA system architecture allows app developers to write own 2FA providers which can be plugged in as apps.

fixes #12102

TODO - basic

TODO - email 2FA app

dummy validation (always enforce + hard-coded token)
extract demo app into it's own repo

TODO - enhanced (done in separate PRs if possible)

if 2FA is used, clients can only log in with client-specific passwords -> Forbid password login on APIs if user is two-factor authenticated #24768
send out real email with a generated temporary password
remember redirect_url across requests

Other ideas

make it possible to cancel 2FA to log in as an other user

mention-bot · 2016-05-11T09:40:53Z

By analyzing the blame information on this pull request, we identified @bantu, @DeepDiver1975 and @icewind1991 to be potential reviewers

DeepDiver1975 · 2016-05-11T09:44:18Z

apps/twofactor_email/appinfo/info.xml

+	</two-factor-providers>
+
+	<dependencies>
+		<owncloud min-version="9.1" />


max-version is required as well

ChristophWurst · 2016-05-11T18:58:52Z

apps/twofactor_email/tests/bootstrap.php

+ * This file is licensed under the Affero General Public License version 3 or
+ * later. See the COPYING file.
+ *
+ * @author Christoph Wurst <[email protected]>


TODO: change license header email address 🙈

LukasReschke · 2016-05-11T19:46:51Z

Can't enable 😢

LukasReschke · 2016-05-11T19:52:05Z

apps/twofactor_email/appinfo/info.xml

@@ -0,0 +1,23 @@
+<?xml version="1.0"?>
+<info>
+	<id>twofactoremail</id>


id needs to be twofactor_email otherwise installation will fail 😉

installation via occ works 😉

LukasReschke · 2016-05-11T19:57:39Z

Easily bypassed using the following URL:

http://localhost/index.php/login?redirect_url=%252Foc%252Findex.php%252Fapps%252Ffiles%252F

Needs some more state checks 😄

ChristophWurst · 2016-05-12T12:55:08Z

BUG: emtpy can not be used for return values

Parse error: ./lib/private/Authentication/TwoFactorAuth/Manager.php:58

    56|      */

    57|     public function isTwoFactorAuthenticated(IUser $user) {

  > 58|         return !empty($this->getProviders($user));

    59|     }

    60| 

Fatal error: Can't use method return value

ChristophWurst · 2016-05-18T08:17:23Z

ChristophWurst · 2016-05-18T08:32:45Z

2FA can be disabled/enabled via OCC:

$ php occ twofactor:disable admin
2FA disabled for user admin

$ php occ twofactor:enable admin
2FA enabled for user admin

Note that 2FA is enforced for all users by default if at least one 2FA provider app is enabled.

PVince81 · 2016-05-20T12:20:19Z

About the CI error not sure, maybe something went wrong with all the recent merges.
That sqlite issue doesn't seem to happen on master: https://ci.owncloud.org/job/server-master-linux/
(there's a mysql error "too many connections" on master, unrelated)

So not sure, did you try running the sqlite tests locally ?

PVince81 · 2016-05-20T12:21:22Z

Reproducible on your branch:

% ./autotest.sh sqlite
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Installing ....
The process control (PCNTL) extensions are required in case you want to interrupt long running commands - see http://php.net/manual/en/book.pcntl.php
ownCloud is not installed - only a limited number of commands are available
{"reqId":"gRq23Q1VbUONT8pIzumE","remoteAddr":"","app":"PHP","message":"file_put_contents(\/srv\/www\/htdocs\/owncloud\/data\/.htaccess): failed to open stream: Permission denied at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Setup.php#498","level":3,"time":"2016-05-20T12:20:41+00:00","method":"--","url":"--","user":"--"}
{"reqId":"gRq23Q1VbUONT8pIzumE","remoteAddr":"","app":"PHP","message":"file_put_contents(\/srv\/www\/htdocs\/owncloud\/data\/index.html): failed to open stream: Permission denied at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Setup.php#499","level":3,"time":"2016-05-20T12:20:41+00:00","method":"--","url":"--","user":"--"}
creating sqlite db
ownCloud was successfully installed
{"reqId":"gRq23Q1VbUONT8pIzumE","remoteAddr":"","app":"PHP","message":"chmod(): Operation not permitted at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Log\/Owncloud.php#114","level":3,"time":"2016-05-20T12:20:41+00:00","method":"--","url":"--","user":"admin"}
Testing with sqlite ...
No coverage
/home/vincent/opt/bin/phpunit --configuration phpunit-autotest.xml --log-junit autotest-results-sqlite.xml  
PHP Fatal error:  Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1 no such table: oc_appconfig' in /srv/www/htdocs/owncloud/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:104
Stack trace:
#0 /srv/www/htdocs/owncloud/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php(104): PDO->query('SELECT * FROM "...')
#1 /srv/www/htdocs/owncloud/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(833): Doctrine\DBAL\Driver\PDOConnection->query('SELECT * FROM "...')
#2 /srv/www/htdocs/owncloud/lib/private/DB/Connection.php(184): Doctrine\DBAL\Connection->executeQuery('SELECT * FROM "...', Array, Array, NULL)
#3 /srv/www/htdocs/owncloud/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php(206): OC\DB\Connection->executeQuery('SELECT * FROM `...', Array, Array)
#4 /srv/www/htdocs/owncloud/lib/private/DB/QueryBuilder/QueryBuilder.php(141): Doctrine\DBAL\Query\QueryBuilder->execute()
#5 /srv/www/htdocs/owncloud/lib/private/AppConfig.php(274): OC\DB\QueryBuilder\Que in /srv/www/htdocs/owncloud/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php on line 58

PVince81 · 2016-05-20T12:21:54Z

Cannot reproduce on master. Weird.

ChristophWurst · 2016-05-20T12:22:34Z

opened an issue for canceling 2fa: #24745

ChristophWurst · 2016-05-20T12:24:37Z

Reproducible locally

./autotest.sh sqlite                                                                                                                                                                           
Using PHP executable /usr/bin/php                                                                                                                                                                                                 
Parsing all files in lib/public for the presence of @since or @deprecated on each method...                                                                                                                                       

Using database oc_autotest                                                                                                                                                                                                        
Setup environment for sqlite testing on local storage ...                                                                                                                                                                         
Installing ....                                                                                                                                                                                                                   
ownCloud is not installed - only a limited number of commands are available                                                                                                                                                       
creating sqlite db                                                                                                                                                                                                                
Error while trying to create admin user: An exception occurred while executing 'CREATE TABLE "oc_appconfig" ("appid" VARCHAR(32) DEFAULT '' NOT NULL, "configkey" VARCHAR(64) DEFAULT '' NOT NULL, "configvalue" CLOB DEFAULT NULL, PRIMARY KEY("appid", "configkey"))':

SQLSTATE[HY000]: General error: 1 table "oc_appconfig" already exists

I have no idea where that comes from…

PVince81 · 2016-05-20T12:26:01Z

@ChristophWurst it is likely that some of your code is running very early, even before the database tables are created. The code is question is likely the part that reads the two factor auth flags ?

PVince81 · 2016-05-20T12:26:50Z

There might also be some config code that runs whenever you call \OC::$server->getXXXConfig() and preloads all config values. Maybe this is happening too early.

ChristophWurst · 2016-05-23T08:30:03Z

sqlite error fixed.

@LukasReschke @PVince81 @rullzer @nickvergessen @BernhardPosselt review please :)

PVince81 · 2016-05-23T08:59:01Z

👍

ChristophWurst · 2016-05-23T09:12:50Z

Unit tests fail…

ChristophWurst · 2016-05-23T11:22:42Z

Unrelated?

05:26:15 There was 1 failure:
05:26:15 
05:26:15 1) Test\Files\Utils\ScannerTest::testReuseExistingRoot
05:26:15 Failed asserting that two objects are equal.
05:26:15 --- Expected
05:26:15 +++ Actual
05:26:15 @@ @@
05:26:15          'encrypted' => false
05:26:15 -        'etag' => '5742cc2463af2'
05:26:15 +        'etag' => '5742cc2464566'
05:26:15          'permissions' => 23
05:26:15          'checksum' => ''
05:26:15          'encryptedVersion' => 0
05:26:15      )
05:26:15  )

lock · 2019-08-05T19:02:22Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ChristophWurst added enhancement 2 - Developing security labels May 11, 2016

ChristophWurst added this to the 9.1-current milestone May 11, 2016

ChristophWurst force-pushed the 2fa branch from 098680d to 14e781b Compare May 11, 2016 09:41

DeepDiver1975 reviewed May 11, 2016
View reviewed changes

ChristophWurst force-pushed the 2fa branch 2 times, most recently from f3bde39 to 092ffaf Compare May 11, 2016 18:57

ChristophWurst reviewed May 11, 2016
View reviewed changes

LukasReschke reviewed May 11, 2016
View reviewed changes

ChristophWurst force-pushed the 2fa branch 2 times, most recently from 1ce7d72 to 4e1d677 Compare May 12, 2016 10:37

ChristophWurst force-pushed the 2fa branch 5 times, most recently from eecd262 to 288730f Compare May 18, 2016 08:13

ChristophWurst force-pushed the 2fa branch 3 times, most recently from 238464a to 1801123 Compare May 23, 2016 08:28

ChristophWurst mentioned this pull request May 23, 2016

Forbid password login on APIs if user is two-factor authenticated #24768

Closed

ChristophWurst force-pushed the 2fa branch from 1801123 to 847bbc5 Compare May 23, 2016 09:20

ChristophWurst added 2 commits May 23, 2016 11:21

Add two factor auth to core

dfb4d42

add OCC command to enable/disable 2FA for a user

847bbc5

ChristophWurst mentioned this pull request May 23, 2016

Test\Files\Utils\ScannerTest::testReuseExistingRoot fails on master #24774

Closed

ChristophWurst added comp:authentication and removed security labels May 23, 2016

PVince81 added 4 - To release and removed 3 - To Review labels May 23, 2016

PVince81 merged commit 87fa86a into master May 23, 2016

PVince81 deleted the 2fa branch May 23, 2016 18:50

ChristophWurst mentioned this pull request May 25, 2016

Remember redirect_url when logging in with a second factor #24850

Closed

ChristophWurst mentioned this pull request Jun 8, 2016

remove app type to allow enabling for groups nextcloud/twofactor_totp#15

Merged

InQuize mentioned this pull request Sep 1, 2016

Restoring access nextcloud/twofactor_totp#53

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

two factor auth #24559

two factor auth #24559

ChristophWurst commented May 11, 2016 •

edited

Loading

mention-bot commented May 11, 2016

DeepDiver1975 May 11, 2016

ChristophWurst May 11, 2016 •

edited

Loading

LukasReschke commented May 11, 2016

LukasReschke May 11, 2016

ChristophWurst May 12, 2016

LukasReschke commented May 11, 2016

ChristophWurst commented May 12, 2016 •

edited

Loading

ChristophWurst commented May 18, 2016

ChristophWurst commented May 18, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

ChristophWurst commented May 20, 2016

ChristophWurst commented May 20, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

ChristophWurst commented May 23, 2016

PVince81 commented May 23, 2016

ChristophWurst commented May 23, 2016

ChristophWurst commented May 23, 2016

lock bot commented Aug 5, 2019

two factor auth #24559

two factor auth #24559

Conversation

ChristophWurst commented May 11, 2016 • edited Loading

mention-bot commented May 11, 2016

DeepDiver1975 May 11, 2016

Choose a reason for hiding this comment

ChristophWurst May 11, 2016 • edited Loading

Choose a reason for hiding this comment

LukasReschke commented May 11, 2016

LukasReschke May 11, 2016

Choose a reason for hiding this comment

ChristophWurst May 12, 2016

Choose a reason for hiding this comment

LukasReschke commented May 11, 2016

ChristophWurst commented May 12, 2016 • edited Loading

ChristophWurst commented May 18, 2016

ChristophWurst commented May 18, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

ChristophWurst commented May 20, 2016

ChristophWurst commented May 20, 2016

PVince81 commented May 20, 2016

PVince81 commented May 20, 2016

ChristophWurst commented May 23, 2016

PVince81 commented May 23, 2016

ChristophWurst commented May 23, 2016

ChristophWurst commented May 23, 2016

lock bot commented Aug 5, 2019

ChristophWurst commented May 11, 2016 •

edited

Loading

ChristophWurst May 11, 2016 •

edited

Loading

ChristophWurst commented May 12, 2016 •

edited

Loading