Skip to content
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

PDO - support username & password specified in DSN #2684

Conversation

@SjonHortensius
Copy link
Contributor

commented Aug 14, 2017

This implements a new feature where PDO (for mysql) supports user and password being specified in the DSN instead of separate constructor params. This is backwards compatible, it ignores the credentials in DSN when separate params are passed.

This additionally allows a security feature where user-space is unable to read the credentials but PDO is able to connect anyway.

I have tested this with named DSN's as well

@kelunik

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

How does moving the credentials from separate parameters to the DSN allow hiding the credentials from user-space?

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

@kelunik That is a separate feature; not the primary aim of this change.

It is prevented when using named dsns, see the manual on ini.pdo.dsn These are unreadable from userspace; see pdo_mysql___construct_ini.phpt

@KalleZ

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

I don't think the other drivers should be left out, like PDO_OCI, PDO_PGSQL etc

@SjonHortensius SjonHortensius force-pushed the SjonHortensius:pdo_mysql_dsn_credentials_support branch from d2572e6 to 000071c Aug 14, 2017

@morrisonlevi

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

@KalleZ Maybe I'm misunderstanding something here but doesn't PGSQL already support username and password in the DSN? https://secure.php.net/manual/en/ref.pdo-pgsql.connection.php

@KalleZ

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

@morrisonlevi ah, I'm not too familiar with the other drivers, but if PGSQL supports then I think its fairly reasonable to let this go into master, tho I would personally like to see other drivers support it as well for unification, PDO is already fairly split as it is

@krakjoe krakjoe added the Enhancement label Aug 15, 2017

@SjonHortensius SjonHortensius force-pushed the SjonHortensius:pdo_mysql_dsn_credentials_support branch from 000071c to d5748e7 Aug 15, 2017

@KalleZ

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

The following should unify PDO_OCI, PDO_DBLIB and PDO_FIREBIRD with this PR: https://gist.github.com/KalleZ/2cea919fe1e8f4e1d1d8cbfd2afe0c43

I left out ODBC intentionally as it needs some more work as a username is passed as UID there, so it may not make that much sense to do it there.

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2017

@KalleZ Thanks, I think it would be great to introduce this feature across all popular pdo drivers at once. I've appended your patch (with proper attribution) to this PR

@adambaratz

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

Basic change looks fine. My only request would be to create a generic test in ext/pdo/tests instead of the driver-specific one. This will make this functionality is validated for all drivers.

@KalleZ

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@SjonHortensius cheers :)

@adambaratz agreed, tho its kinda bad we don't have a way of exposing a DSN for a PDO instance

@SjonHortensius SjonHortensius force-pushed the SjonHortensius:pdo_mysql_dsn_credentials_support branch 2 times, most recently from 9716575 to 2619c7b Aug 16, 2017

@morrisonlevi

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

Can someone please officially review this for PHP 7.3? This is not just convenience; by specifying the username and password in an ini file it won't show up in backtraces.

if (dbh->username) {
if(FAIL == DBSETLUSER(H->login, dbh->username)) {
goto cleanup;
}
}

if (!dbh->password && vars[7].optval) {

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ Feb 19, 2019

This might be a dumb question (as I'm new here, still trying to make & run tests properly): how will this behave for new PDO("...;user=root;password=shouldbeoverwritten", 'overuser', '')?
As far as I get the key idea here, the 2nd and 3rd construct parameters prevail, so user should be overuser and password should be empty. But will the test here consider that since password (from 3rd param) is empty, then the one in DSN will be used and so, instead of "overuser"+"" empty password, we get "overuser"+"shouldbeoverwritten" ?

The same question goes for all other places this test is used

This comment has been minimized.

Copy link
@KalleZ

KalleZ May 3, 2019

Member

@SpyroTEQ Sorry for the long time between replies, as this PR seems inactive. The idea is that if a username and password is set for the constructor to PDO (argument 2 and 3), then it will override the ones sent in the DSN, this is how other PDO drivers did it so I wrote it like that (in this case for pdo_dblib) to keep the drivers somewhat consistent

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 28, 2019

Got it (sorry from my side too to take so long to answer), but I was actually focusing on how you test that the password has been set in DSN and how you test that password has been set as 3rd argument. What I mean is that !dbh->password might be true if password is not set, but is it also true if password is set to empty? Same question for vars[7].optval?

In other words, these cases might be worth being tested (or must match the tests used, considering empty/not set values as different):

  • new PDO("...;password=shouldbeoverwritten", 'overuser', '') : password must be the empty one
  • new PDO("...;password=shouldbeoverwritten", 'overuser', '000') : password must be '000'
  • new PDO("...;password=shouldbeoverwritten", 'overuser', 'false') : password must be 'false' (pretty dumb password but still)
  • new PDO("...;password=", 'overuser', 'xxx') : password must be 'xxx'
  • new PDO("...;password=000", 'overuser', 'xxx') : password must be 'xxx'
  • new PDO("...;password=inuse", 'overuser') : password must be 'inuse'
  • new PDO("...;password=", 'overuser') : password must be empty one
  • new PDO("...;", 'overuser') : password must be backward compatible

Maybe this is an absurd question in C code, but in PHP code itself, it's often easy to mistake between empty string, not set string (null), 0-filled string, false literal, etc

This comment has been minimized.

Copy link
@SjonHortensius

SjonHortensius May 28, 2019

Author Contributor

@SpyroTEQ while that is indeed interesting - it is currently impossible to test like that. We have one valid set of credentials for testing and I don't think it's productive to start issuing SET PASSWORD calls just for this. Since the implementation is already in C - I'm not sure testing for weird PHP behavior is productive here. I do invite you to test the examples you specified and report back if you find any discrepancies.

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@petk would you please consider pulling this for 7.4?

@petk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@petk would you please consider pulling this for 7.4?

Looks good feature indeed. I'm not sure why this wasn't added before for such a long time. Of course, we can merge this to PHP-7.4. Let me test this a bit if everything working ok and merge coming up...

@petk petk self-assigned this May 3, 2019

@peter279k

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I just note that this will have some related descriptions in php.net doc after this PR is accepted and merged.

@petk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I just note that this will have some related descriptions in php.net doc after this PR is accepted and merged.

Definitely. Hopefully someone will add that there. I've uninstalled SVN very long time ago so I'm waiting for a Git repository to be available and contributions possible over pull requests.

@Girgias

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I can always write the doc for this when it gets merged just throw me a ping/email.

@petk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Hello, after a quick glance I think it is ok. But I possibly cannot test all these databases to be completely sure so I'll blindly trust that the pull request is ok... These are 3 databases here to test actually.

Should we merge this then?

@petk petk removed their assignment May 3, 2019

@KalleZ

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@petk There is actually 4 (+2): pdo_dblib (either of its 3 build flavors should be enough), pdo_oci, pdo_mysql and pdo_firebird.

There is one thing I would like to know before we eventually merge this, are we gonna look at pdo_odbc too or let it be the special kid in the yard?

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Should we merge this then?

I'm not sure who you expect to reply here, as others have (basically) said the same thing. There has been no reason not to merge it in the last 2 years

@petk

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

There has been no reason not to merge it in the last 2 years

Merge coming up then 🎉

@SjonHortensius SjonHortensius force-pushed the SjonHortensius:pdo_mysql_dsn_credentials_support branch from 62d4a5d to dd96d1d May 16, 2019

@petk

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

And just one more:

  • Windows tests seem to fail for the file ext/pdo_mysql/tests/pdo_mysql___construct.phpt
@KalleZ

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@petk This might be because of the configuration of AppVeyor, see here:
https://github.com/php/php-src/blob/master/appveyor/test_task.bat#L30

Currently in master we are sending user and password in the DSN, even though those two values are not legal directives for ext/pdo_mysql:
https://github.com/php/php-src/blob/master/ext/pdo_mysql/mysql_driver.c#L568

I'm sorry I don't have more time to debug this currently.

Removing those two directives from the generated DSN environment variable and merging master into the branch should resolve it (maybe?).

@petk

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Thank you @KalleZ for this info... I'm circling in loops here a bit what to fix now in the tests. Yes, this seems to be the case. To not invest too much time into this, can maybe @SjonHortensius help us again by checking this out for the failing tests what would be a good fix for this?

However, I also think something else here - the attached test file gets skipped. Is this something to fix maybe?

Thank you for this pull request and the feature implementation, @SjonHortensius. Using username and pass in the DSN string is useful feature nonetheless...

SjonHortensius added a commit to SjonHortensius/php-src that referenced this pull request May 17, 2019

appveyor - don't include credentials in DSN
as the tests don't expect it to do anything - which it does since PR php#2684

SjonHortensius added some commits May 17, 2019

appveyor - don't include credentials in DSN
as the tests don't expect it to do anything - which it does since PR #2684
@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@KalleZ is correct - what's happening is that the appveyor/test_task.bat prematurely (this was ignored until this PR) added user/pwd to the dsn - and then expected the tests to fail - which they no longer do with this PR.

Therefore I removed the credentials from appveyor to match the 'normal' test run on *nix

@petk

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Thanks for the update. What about the following? The added phpt file is now skipped on all of these CI environments PHP uses. And it was skipped before the last commit also...

Travis:
SKIP PDO Common: Pass credentials in dsn instead of constructor params [ext/pdo/tests/pdo_dsn_containing_credentials.phpt] reason: no driver

AppVeyor:
SKIP PDO Common: Pass credentials in dsn instead of constructor params [C:\projects\php-src\ext\pdo\tests\pdo_dsn_containing_credentials.phpt] reason: no driver

Fix test - was moved in dd96d1d but not correctly refactored
since PDOTest::factory has a different factory then MySQLPDOTest this
test was effectively useless

@SjonHortensius SjonHortensius force-pushed the SjonHortensius:pdo_mysql_dsn_credentials_support branch from 6678fad to e6e64e4 May 18, 2019

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

I'm pretty sure the pdo common tests skip anything that requires a driver - which this change does as well.

You did make me look at the test again btw - and I fixed an embarrassing issue that was caused by the move from mysql>pdo

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

FYI - I just realized an accompanying FR in the bugtracker is required to make this change appear in the changelog - so I logged https://bugs.php.net/bug.php?id=78033

@KalleZ

KalleZ approved these changes May 22, 2019

@@ -27,7 +27,7 @@ set PDO_MYSQL_TEST_USER=%MYSQL_TEST_USER%
set PDO_MYSQL_TEST_PASS=%MYSQL_PWD%
set PDO_MYSQL_TEST_HOST=%MYSQL_TEST_HOST%
set PDO_MYSQL_TEST_PORT=%MYSQL_TEST_PORT%
set PDO_MYSQL_TEST_DSN=mysql:host=%PDO_MYSQL_TEST_HOST%;port=%PDO_MYSQL_TEST_PORT%;dbname=test;user=%PDO_MYSQL_TEST_USER%;password=%MYSQL_PW%
set PDO_MYSQL_TEST_DSN=mysql:host=%PDO_MYSQL_TEST_HOST%;port=%PDO_MYSQL_TEST_PORT%;dbname=test

This comment has been minimized.

Copy link
@KalleZ

KalleZ May 22, 2019

Member

I think this can be done in a separate PR and then merged to PHP-7.2 and upwards

This comment has been minimized.

Copy link
@SjonHortensius

SjonHortensius May 24, 2019

Author Contributor

While I wouldn't mind; this change isn't necessary for branches that don't parse credentials in the dsn - so it's not really needed, do you disagree? It'll require some git-fu to undo the merge and keep this branch merge-able

@SpyroTEQ

This comment has been minimized.

Copy link

commented May 28, 2019

I guess user in DSN will often be [a-zA-Z0-9_-]+ but what if password holds = or ;? It will break DSN password, so should it be said "password in DSN must contain no = nor ;" in the doc (in this case, PHP coder should rely on the usual "password" parameter), or is there an encoding that should be used in DSN values in general (if so which one)?
(I don't see this in the talks here, but maybe I've misssed it)

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@SpyroTEQ this is true for any parameter in the dsn and not specific to this feature. I assume that anyone who sees invalid user x after trying to connect as dbname=asdf;user=x;things will understand what the problem is

@kelunik

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Should we use URL-encoding?

@SpyroTEQ

This comment has been minimized.

Copy link

commented Jun 5, 2019

@SjonHortensius This might prevent generic code like new PDO('mysql:...;user=$user;password=$password') where user & password come from configuration files. This issue wasn't really a probelm with user and db names (usually A-Z0-9_) but might be with password. It can still be considered as a "usage issue", but I guess it could be worth a note in the doc at least

@kelunik I have no idea what encoding should be used. It should be BC I suppose (unless this comes with PHP8 or so): using urlencoding, any DSN already containing a %[0-9A-F]{2,} will be broken (I suppose it's not actually very used, but again, maybe worth a release note?)

Looking at https://social.msdn.microsoft.com/forums/sqlserver/en-US/06103cd7-3d07-4a1a-a028-4de053db3eb7/how-can-we-put-semi-colon-in-webconfig-connectionstrings answer, I see that password="lalala" is used; " is used because inside XML so the actualy DSN is xxx:...;password="password"` This resolves the ";" issue, but now, how would quotes be escaped inside the password?! : )

Note that maybe I'm overly-thinking, these are only "curiosity" questions that probably do not prevent the feature from being merged.

@SjonHortensius

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@KalleZ can you update the tags of this PR to reflect the current state? Imo this is still ready to be pulled as-is

@nikic nikic removed the Waiting on Author label Jul 2, 2019

@nikic

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Merged as a7881df into 7.4. Thanks, and sorry for the delay :)

@nikic nikic closed this Jul 2, 2019

SjonHortensius added a commit to SjonHortensius/php-src that referenced this pull request Jul 2, 2019

SjonHortensius added a commit to SjonHortensius/php-src that referenced this pull request Jul 2, 2019

php-pulls pushed a commit that referenced this pull request Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.