PDO - support username & password specified in DSN

[SjonHortensius]

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]

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

[SjonHortensius]

@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]

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

[morrisonlevi]

@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]

@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

[KalleZ]

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]

@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]

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]

@SjonHortensius cheers :)

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

[morrisonlevi]

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.

[SpyroTEQ]

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

[KalleZ]

@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

[SpyroTEQ]

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

[SjonHortensius]

@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]

@petk would you please consider pulling this for 7.4?

[petk]

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

[peter279k]

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

[petk]

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]

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

[petk]

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?

[KalleZ]

@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]

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]

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

Merge coming up then 🎉

[petk]

And just one more:

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

[KalleZ]

@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]

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]

@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]

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

[SjonHortensius]

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]

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]

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

[SjonHortensius]

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]

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]

@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]

Should we use URL-encoding?

[SpyroTEQ]

@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]

@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]

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