Tickets :: [#7972] SQL SSL support

6.0.0-git

2021-01-18

Summary	SQL SSL support
Queue	Horde Framework Packages
Queue Version	FRAMEWORK_3
Type	Enhancement
State	Resolved
Priority	1. Low
Owners	chuck (at) horde (dot) org
Requester	BryanRJ (at) gmail (dot) com
Created	2009-02-11 (4359 days ago)
Due
Updated	2009-02-25 (4345 days ago)
Assigned	2009-02-12 (4358 days ago)
Resolved	2009-02-25 (4345 days ago)
Milestone	3.3.4
Patch	No

2009-02-25 05:30:05	Chuck Hagenbuch	Comment #18 State ⇒ Resolved	Reply to this comment
That patch looks great - committed, thanks!

2009-02-25 05:29:34	CVS Commit	Comment #17	Reply to this comment
Changes have been made in CVS for this ticket: http://cvs.horde.org/diff.php/framework/Alarm/Alarm/sql.php?rt=horde&r1=1.18&r2=1.19&ty=u http://cvs.horde.org/diff.php/framework/Auth/Auth/sql.php?rt=horde&r1=1.109&r2=1.110&ty=u http://cvs.horde.org/diff.php/framework/Cache/Cache/sql.php?rt=horde&r1=1.23&r2=1.24&ty=u http://cvs.horde.org/diff.php/framework/DataTree/DataTree/sql.php?rt=horde&r1=1.247&r2=1.248&ty=u http://cvs.horde.org/diff.php/framework/Group/Group/sql.php?rt=horde&r1=1.11&r2=1.12&ty=u http://cvs.horde.org/diff.php/framework/Horde/Horde/Config.php?rt=horde&r1=1.147&r2=1.148&ty=u http://cvs.horde.org/diff.php/framework/Lock/Lock/sql.php?rt=horde&r1=1.16&r2=1.17&ty=u http://cvs.horde.org/diff.php/framework/Perms/Perms/sql.php?rt=horde&r1=1.9&r2=1.10&ty=u http://cvs.horde.org/diff.php/framework/Prefs/Prefs/sql.php?rt=horde&r1=1.130&r2=1.131&ty=u

2009-02-25 03:41:44	BryanRJ (at) gmail (dot) com	Comment #16 New Attachment: ssl2.patch	Reply to this comment
This patch is still against the lib/ directory, which makes it really hard to apply to CVS (as opposed to a packaged release). It also fails to apply in most cases for me if i supply the paths manually. Can you please do this patch against a CVS HEAD/git checkout (http://horde.org/source/)? Also, you seem to be introducing tabs and whitespace inconsistencies - if those could be ironed out it would speed up applying (http://horde.org/horde/docs/?f=CODING_STANDARDS.html). Thanks! Patch against CVS HEAD fixing problems?

2009-02-17 18:05:08	Chuck Hagenbuch	Comment #15	Reply to this comment
This patch is still against the lib/ directory, which makes it really hard to apply to CVS (as opposed to a packaged release). It also fails to apply in most cases for me if i supply the paths manually. Can you please do this patch against a CVS HEAD/git checkout (http://horde.org/source/)? Also, you seem to be introducing tabs and whitespace inconsistencies - if those could be ironed out it would speed up applying (http://horde.org/horde/docs/?f=CODING_STANDARDS.html). Thanks!

2009-02-16 22:53:14	BryanRJ (at) gmail (dot) com	Comment #14 New Attachment: ssl.patch	Reply to this comment
Note that "foo" is still set to true. The analogy to the current situation is thus: if a user wants to use a persistent connection for the RW database and NOT use one for the RO database, they set the "persistent" flag in the RW options and set no such option in the RO. They should set it to false in RO. Fine so long as users always do explicitly set the attribute. Attached: updated patch that uses the merged array where appropriate.

2009-02-14 20:07:27	Chuck Hagenbuch	Comment #13	Reply to this comment
Note that "foo" is still set to true. The analogy to the current situation is thus: if a user wants to use a persistent connection for the RW database and NOT use one for the RO database, they set the "persistent" flag in the RW options and set no such option in the RO. They should set it to false in RO.

2009-02-14 05:33:01	BryanRJ (at) gmail (dot) com	Comment #12	Reply to this comment
[Show Quoted Text - 11 lines][Hide Quoted Text] Perhaps this is a topic for another bug, but the handling of split reads appears to be broken in all of these places - array_merge is called to merge the original parameters with the read-only DB ones. However, both the persistent connections option and this new SSL one check !empty, so in the case where an SSL connection is desired for the original database but not for the new one you will get incorrect behavior. This would be a separate issue. But why? A false key in the new array should override a true key in the original one just fine. This function, I do not think it does what you think it does ;-P. Example code: <?php $foo = array('foo'=>true, 'bar'=>'baz'); $bazza = array('bar'=>'yar'); var_dump(array_merge($foo,$bazza)); ?> This code prints: array(2) { ["foo"]=> bool(true) ["bar"]=> string(3) "yar" } Note that "foo" is still set to true. The analogy to the current situation is thus: if a user wants to use a persistent connection for the RW database and NOT use one for the RO database, they set the "persistent" flag in the RW options and set no such option in the RO. So you call array_merge($rwopts,$roopts) and get an array which has "persistent" nonempty, because it was nonempty in $rwopts... Get it? Also, the references to "$this->_params" inside the splitreads block should be just "$params". That was from your patch - can you provide an updated one for just that issue? Will do, give me a day or two. But like I said it's not a big deal since the $params array is not correct anyhow and so using it won't change anything. How do I go about getting this same change made to Imp/Turba/Kronolith/etc? Pretty much every application also needs it. Patches would be great, or at least tickets to remind us. Thanks! OK, I'll add a few more tickets.

2009-02-14 05:15:38	Chuck Hagenbuch	Comment #11 State ⇒ Feedback	Reply to this comment
Perhaps this is a topic for another bug, but the handling of split reads appears to be broken in all of these places - array_merge is called to merge the original parameters with the read-only DB ones. However, both the persistent connections option and this new SSL one check !empty, so in the case where an SSL connection is desired for the original database but not for the new one you will get incorrect behavior. This would be a separate issue. But why? A false key in the new array should override a true key in the original one just fine. Also, the references to "$this->_params" inside the splitreads block should be just "$params". That was from your patch - can you provide an updated one for just that issue? How do I go about getting this same change made to Imp/Turba/Kronolith/etc? Pretty much every application also needs it. Patches would be great, or at least tickets to remind us. Thanks!

2009-02-13 20:59:34	BryanRJ (at) gmail (dot) com	Comment #10	Reply to this comment
Committed for 3.3.4, thanks! Thank you for the quick commit! Perhaps this is a topic for another bug, but the handling of split reads appears to be broken in all of these places - array_merge is called to merge the original parameters with the read-only DB ones. However, both the persistent connections option and this new SSL one check !empty, so in the case where an SSL connection is desired for the original database but not for the new one you will get incorrect behavior. Also, the references to "$this->_params" inside the splitreads block should be just "$params". How do I go about getting this same change made to Imp/Turba/Kronolith/etc? Pretty much every application also needs it.

2009-02-13 05:47:53	Chuck Hagenbuch	Comment #9 State ⇒ Resolved Milestone ⇒ 3.3.4	Reply to this comment
Committed for 3.3.4, thanks!

2009-02-13 05:35:09	CVS Commit	Comment #8	Reply to this comment
Changes have been made in CVS for this ticket: http://cvs.horde.org/diff.php/framework/Alarm/Alarm/sql.php?rt=horde&r1=1.17&r2=1.18&ty=u http://cvs.horde.org/diff.php/framework/Auth/Auth/sql.php?rt=horde&r1=1.108&r2=1.109&ty=u http://cvs.horde.org/diff.php/framework/Cache/Cache/sql.php?rt=horde&r1=1.22&r2=1.23&ty=u http://cvs.horde.org/diff.php/framework/DataTree/DataTree/sql.php?rt=horde&r1=1.246&r2=1.247&ty=u http://cvs.horde.org/diff.php/framework/Group/Group/sql.php?rt=horde&r1=1.10&r2=1.11&ty=u http://cvs.horde.org/diff.php/framework/Horde/Horde/Config.php?rt=horde&r1=1.146&r2=1.147&ty=u http://cvs.horde.org/diff.php/framework/Lock/Lock/sql.php?rt=horde&r1=1.15&r2=1.16&ty=u http://cvs.horde.org/diff.php/framework/Perms/Perms/sql.php?rt=horde&r1=1.8&r2=1.9&ty=u http://cvs.horde.org/diff.php/framework/Prefs/Prefs/sql.php?rt=horde&r1=1.129&r2=1.130&ty=u http://cvs.horde.org/diff.php/framework/SessionHandler/SessionHandler/sql.php?rt=horde&r1=1.49&r2=1.50&ty=u http://cvs.horde.org/diff.php/framework/Token/Token/sql.php?rt=horde&r1=1.43&r2=1.44&ty=u http://cvs.horde.org/diff.php/framework/VFS/lib/VFS/sql.php?rt=horde&r1=1.5&r2=1.6&ty=u http://cvs.horde.org/diff.php/framework/VFS/lib/VFS/sql_file.php?rt=horde&r1=1.1&r2=1.2&ty=u http://cvs.horde.org/diff.php/horde/docs/CHANGES?rt=horde&r1=1.1204&r2=1.1205&ty=u

2009-02-12 22:51:39	Chuck Hagenbuch	Comment #7 Assigned to Chuck Hagenbuch State ⇒ Assigned	Reply to this comment
Okay, thanks. I'll try and apply this soon.

2009-02-12 04:38:32	BryanRJ (at) gmail (dot) com	Comment #6	Reply to this comment
Okay, but what was the bit about Rdo? The Db lib in Horde 4 (http://git.horde.org/co.php/framework/Db/lib/Horde/Db/Adapter/Mysqli.php?r=9115f57f8f3da190a9f9f05b0db55135f51ac4d7&rt=horde-git) has SSL support in the mysqli adapter already. Is this only relevant for mysqli? I built this patch against the latest released horde 3.3 series. I didn't realize that your Rdo code wasn't used - I saw calls to connect, so I rewrote them to accept additional arguments. This code in theory applies to all SQL versions, but I'm unsure what PEAR DB does with the ssl=>true setting with backends other than mysqli.

2009-02-12 04:35:03	Chuck Hagenbuch	Comment #5	Reply to this comment
Okay, but what was the bit about Rdo? The Db lib in Horde 4 (http://git.horde.org/co.php/framework/Db/lib/Horde/Db/Adapter/Mysqli.php?r=9115f57f8f3da190a9f9f05b0db55135f51ac4d7&rt=horde-git) has SSL support in the mysqli adapter already. Is this only relevant for mysqli?

2009-02-11 23:02:56	BryanRJ (at) gmail (dot) com	Comment #4 New Attachment: ssl	Reply to this comment
And if you already have jumped through all the hoops, why don't you provide a patch? How's this?

2009-02-11 22:11:02	Chuck Hagenbuch	Comment #3	Reply to this comment
Also, FW_3 code doesn't use Rdo.

2009-02-11 22:09:03	Jan Schneider	Comment #2 State ⇒ Feedback	Reply to this comment
And if you already have jumped through all the hoops, why don't you provide a patch?

2009-02-11 20:07:21	BryanRJ (at) gmail (dot) com	Comment #1 Type ⇒ Enhancement State ⇒ New Priority ⇒ 1. Low Summary ⇒ SQL SSL support Queue ⇒ Horde Framework Packages Milestone ⇒ Patch ⇒ No	Reply to this comment
Horde as it stands cannot make a secured connection to a remote SQL server. This is an easy feature to implement: mysqli and PEAR DB both support it already. All that needs to be done is changing /lib/Horde/Rdo/Adapter/Mysqli.php on line 171 to use mysqli_init, mysqli_ssl_set, and mysqli_real_connect (there is already a comment there to that effect) and then going through every call to DB::connect (such as the one on nag/lib/Driver/sql.php around line 590) in all horde applications and changing two things: a) add a "ca" to _params b) add "ssl"=>true in the options array passed to DB::connect So, steps to implement this: 1) Add the user interface to accept a SSL CA against which to verify the server (and, if you like, a client cert/key to use) and a checkbox for enabling SSL 2) Modify the MySQLi Rdo adaptor to use these options 3) Modify each call to DB::connect to use these options I completed these steps myself and verified that horde and its applications can now access a database where I gave the horde user "grant all privileges ... require ssl" permissions. Previously the database connection failed. All told, this work took twenty minutes, although I hardcoded my CA cert instead of actually adding an option to conf.php.