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

Add 2-factor authentication support for phpMyAdmin. #13495

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Achilles-96
Contributor

Achilles-96 commented Jul 16, 2017

Signed-off-by: Raghuram raghuram.4350@gmail.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
@Achilles-96

This comment has been minimized.

Show comment
Hide comment
@Achilles-96

Achilles-96 Jul 16, 2017

Contributor

I described the procedure for setting it up 2-factor authentication in my blog post.

As I mentioned there, I need advice on where to place the link to access setup2FA.php.

Contributor

Achilles-96 commented Jul 16, 2017

I described the procedure for setting it up 2-factor authentication in my blog post.

As I mentioned there, I need advice on where to place the link to access setup2FA.php.

@nijel

I've quickly gone through your code, some general remarks:

  • Please include documentation, having it in the blog is great, but it really should be part of our documentation.
  • Any reason for not integrating 2FA into existing Settings tab?
Show outdated Hide outdated js/get_scripts.js.php
Show outdated Hide outdated libraries/phpqrcode.php
Show outdated Hide outdated libraries/session.inc.php
Show outdated Hide outdated libraries/plugins/auth/AuthenticationCookie.php
Show outdated Hide outdated libraries/plugins/auth/AuthenticationCookie.php
Show outdated Hide outdated libraries/plugins/auth/AuthenticationCookie.php
Show outdated Hide outdated setup2FA.php
Show outdated Hide outdated setup2FA.php
Show outdated Hide outdated setup2FA.php
@Achilles-96

This comment has been minimized.

Show comment
Hide comment
@Achilles-96

Achilles-96 Jul 18, 2017

Contributor

@nijel, I addressed most of the issues you raised. I left comments for the rest. Please verify.

Contributor

Achilles-96 commented Jul 18, 2017

@nijel, I addressed most of the issues you raised. I left comments for the rest. Please verify.

$_SESSION['2FAVerified'] = true;
echo 'true';
} else {
echo htmlspecialchars('Sorry. The key does not match. Please ensure that server time and your app\'s time are in sync.');

This comment has been minimized.

@Achilles-96

Achilles-96 Jul 18, 2017

Contributor

By this time, translations may not be possible.

@Achilles-96

Achilles-96 Jul 18, 2017

Contributor

By this time, translations may not be possible.

This comment has been minimized.

@ibennetch

ibennetch Jul 18, 2017

Member

Could you please add a comment to the code as well stating that translations may not be possible?

@ibennetch

ibennetch Jul 18, 2017

Member

Could you please add a comment to the code as well stating that translations may not be possible?

This comment has been minimized.

@nijel

nijel Jul 21, 2017

Member

Hmm, maybe this should be part of the setup script in the end (which we indeed do not localize)?

Edit: I think there is no reason why the localization wound not work here, why it doesn't work here? You should just include common.inc.php (what you should do anyway for authentication and CSRF protection) and localization will work.

Also it should probably display proper error message, not just plain text...

@nijel

nijel Jul 21, 2017

Member

Hmm, maybe this should be part of the setup script in the end (which we indeed do not localize)?

Edit: I think there is no reason why the localization wound not work here, why it doesn't work here? You should just include common.inc.php (what you should do anyway for authentication and CSRF protection) and localization will work.

Also it should probably display proper error message, not just plain text...

This comment has been minimized.

@Achilles-96

Achilles-96 Jul 22, 2017

Contributor

By this time, the user would not have been completely authenticated i.e., he/she is in the middle of authentication. Including common.inc.php is not possible because it will circularly check for 2-factor authentication again.

This part is accessed only by ajax request and the error message is displayed in standard PMA error dialog once received.

@Achilles-96

Achilles-96 Jul 22, 2017

Contributor

By this time, the user would not have been completely authenticated i.e., he/she is in the middle of authentication. Including common.inc.php is not possible because it will circularly check for 2-factor authentication again.

This part is accessed only by ajax request and the error message is displayed in standard PMA error dialog once received.

This comment has been minimized.

@nijel

nijel Jul 24, 2017

Member

Maybe it would be better to closer integrate this to the authentication so that it can use the authentication API to show the form

@nijel

nijel Jul 24, 2017

Member

Maybe it would be better to closer integrate this to the authentication so that it can use the authentication API to show the form

This comment has been minimized.

@Achilles-96

Achilles-96 Jul 25, 2017

Contributor

Authentication form is shown by auth2FA function in AuthenticationCookie.php. This is for verification. Do you mean I should move verification to a different file.

@Achilles-96

Achilles-96 Jul 25, 2017

Contributor

Authentication form is shown by auth2FA function in AuthenticationCookie.php. This is for verification. Do you mean I should move verification to a different file.

Show outdated Hide outdated setup2FA.php
@ibennetch

This comment has been minimized.

Show comment
Hide comment
@ibennetch

ibennetch Jul 18, 2017

Member

I tried opening my main phpMyAdmin after pulling your branch (without configuring any user for 2FA or setting up config.inc.php) and got a number of errors. We should be more graceful about failing if the user hasn't configured their system for this.

Error
SQL query: DocumentationEdit Edit

SELECT `pma_user` FROM `` WHERE `pma_user` = 'root';
MySQL said: Documentation

#1103 - Incorrect table name ''
Open new phpMyAdmin window
Notice in ./libraries/plugins/auth/AuthenticationCookie.php#570
Undefined index: 2fa_secrets

Backtrace

./libraries/plugins/auth/AuthenticationCookie.php#547: PMA\libraries\plugins\auth\AuthenticationCookie->check2FA()
./libraries/common.inc.php#656: PMA\libraries\plugins\auth\AuthenticationCookie->authCheck()
./index.php#22: require_once(./libraries/common.inc.php)
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#437: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#447: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#449: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()

Member

ibennetch commented Jul 18, 2017

I tried opening my main phpMyAdmin after pulling your branch (without configuring any user for 2FA or setting up config.inc.php) and got a number of errors. We should be more graceful about failing if the user hasn't configured their system for this.

Error
SQL query: DocumentationEdit Edit

SELECT `pma_user` FROM `` WHERE `pma_user` = 'root';
MySQL said: Documentation

#1103 - Incorrect table name ''
Open new phpMyAdmin window
Notice in ./libraries/plugins/auth/AuthenticationCookie.php#570
Undefined index: 2fa_secrets

Backtrace

./libraries/plugins/auth/AuthenticationCookie.php#547: PMA\libraries\plugins\auth\AuthenticationCookie->check2FA()
./libraries/common.inc.php#656: PMA\libraries\plugins\auth\AuthenticationCookie->authCheck()
./index.php#22: require_once(./libraries/common.inc.php)
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#437: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#447: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()
Warning in ./libraries/dbi/DBIMysqli.php#556
mysqli_real_escape_string() expects parameter 1 to be mysqli, boolean given

Backtrace

./libraries/dbi/DBIMysqli.php#556: mysqli_real_escape_string(
boolean false,
string '',
)
./libraries/classes/DatabaseInterface.php#2743: PMA\libraries\dbi\DBIMysqli->escapeString(
boolean false,
string '
',
)
./libraries/navigation/nodes/Node.php#449: PhpMyAdmin\DatabaseInterface->escapeString(string '_')
./libraries/navigation/NavigationTree.php#289: PMA\libraries\navigation\nodes\Node->getData(
string 'databases',
integer 0,
string '',
)
./libraries/navigation/NavigationTree.php#847: PMA\libraries\navigation\NavigationTree->_buildPath()
./libraries/navigation/Navigation.php#45: PMA\libraries\navigation\NavigationTree->renderState()
./libraries/classes/Header.php#433: PMA\libraries\navigation\Navigation->getDisplay()
./libraries/classes/Response.php#266: PhpMyAdmin\Header->getDisplay()
./libraries/classes/Response.php#279: PhpMyAdmin\Response->_getDisplay()
./libraries/classes/Response.php#438: PhpMyAdmin\Response->_htmlResponse()
PhpMyAdmin\Response->response()

@ibennetch

This comment has been minimized.

Show comment
Hide comment
@ibennetch

ibennetch Jul 19, 2017

Member

We'll have to deal gracefully with situations where the user either hasn't configured it or the table doesn't exist.

You can also see for instance libraries/classes/Bookmark.php:114 for an example of how we might deal with this. We have to test the array to make sure $cfgRelation['2fa_secret'] (or whatever configuration directive you use) isn't empty first, before trying to use it. We initialize it as part of the default configuration to an empty value, so before using we'll test if the user has configured it.

Also, you should add the new directive to libraries/config.default.php with a blank assignment such as $cfg['Servers'][$i]['2fa_secret'] = '';. You can add it right after the export_template area around line 503.

We should also add this new directive to libraries/relation.lib.php around line 353; this file is used by chk_rel.php which warns the user if their table structure is outdated.

Can you add the structure for the 2fa_secrets table to sql/create_tables.sql?

Instead of manually running the SQL, it might be more efficient to use PMA_queryAsControlUser such as

$sql_query
    = " SELECT `tables` FROM " . $this->_getPmaTable() .
    " WHERE `username` = '" . $GLOBALS['dbi']->escapeString($GLOBALS['cfg']['Server']['user']) . "'";
$result = PMA_queryAsControlUser($sql_query, false);
Member

ibennetch commented Jul 19, 2017

We'll have to deal gracefully with situations where the user either hasn't configured it or the table doesn't exist.

You can also see for instance libraries/classes/Bookmark.php:114 for an example of how we might deal with this. We have to test the array to make sure $cfgRelation['2fa_secret'] (or whatever configuration directive you use) isn't empty first, before trying to use it. We initialize it as part of the default configuration to an empty value, so before using we'll test if the user has configured it.

Also, you should add the new directive to libraries/config.default.php with a blank assignment such as $cfg['Servers'][$i]['2fa_secret'] = '';. You can add it right after the export_template area around line 503.

We should also add this new directive to libraries/relation.lib.php around line 353; this file is used by chk_rel.php which warns the user if their table structure is outdated.

Can you add the structure for the 2fa_secrets table to sql/create_tables.sql?

Instead of manually running the SQL, it might be more efficient to use PMA_queryAsControlUser such as

$sql_query
    = " SELECT `tables` FROM " . $this->_getPmaTable() .
    " WHERE `username` = '" . $GLOBALS['dbi']->escapeString($GLOBALS['cfg']['Server']['user']) . "'";
$result = PMA_queryAsControlUser($sql_query, false);

@ibennetch ibennetch self-assigned this Jul 19, 2017

use RobThree\Auth\Providers\Qr\IQRCodeProvider;
use PHPQRCode;
class PMAQRProvider implements IQRCodeProvider {

This comment has been minimized.

@nijel

nijel Jul 21, 2017

Member

Can you please place the class to separate file and include tests for it?

@nijel

nijel Jul 21, 2017

Member

Can you please place the class to separate file and include tests for it?

@ibennetch

This comment has been minimized.

Show comment
Hide comment
@ibennetch

ibennetch Aug 23, 2017

Member

Could you rebase this against current master to reflect the changes Maurício has done?

Member

ibennetch commented Aug 23, 2017

Could you rebase this against current master to reflect the changes Maurício has done?

Check if session already started
This is required because we start in setup2FA.php while checking for 2-factor
authentication. session variable for accessing secret. We also need that
libraries/common.inc.php should not be loaded by this time.

Signed-off-by: Raghuram <raghuram.4350@gmail.com>
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 23, 2017

Codecov Report

Merging #13495 into master will decrease coverage by 0.2%.
The diff coverage is 2.09%.

@@            Coverage Diff             @@
##           master   #13495      +/-   ##
==========================================
- Coverage   53.17%   52.96%   -0.21%     
==========================================
  Files         468      470       +2     
  Lines       81619    81953     +334     
==========================================
+ Hits        43404    43410       +6     
- Misses      38215    38543     +328

codecov bot commented Aug 23, 2017

Codecov Report

Merging #13495 into master will decrease coverage by 0.2%.
The diff coverage is 2.09%.

@@            Coverage Diff             @@
##           master   #13495      +/-   ##
==========================================
- Coverage   53.17%   52.96%   -0.21%     
==========================================
  Files         468      470       +2     
  Lines       81619    81953     +334     
==========================================
+ Hits        43404    43410       +6     
- Misses      38215    38543     +328
Add 2-factor authentication support for phpMyAdmin.
Signed-off-by: Raghuram <raghuram.4350@gmail.com>
@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Oct 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot commented Oct 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 23, 2017

@stale stale bot closed this Oct 30, 2017

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