6.0.0-git
2021-01-18

[#7972] SQL SSL support
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

History
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 03:41:44 BryanRJ (at) gmail (dot) com Comment #16
New Attachment: ssl2.patch Download
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 Download
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]
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-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 Download
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.

Saved Queries