User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Add option to global config to enable and configure data in transit encryption with TLS in MySQLi/PDO MySQL/PDO PostgreSQL drivers.
Add info about this encryption to sysinfo and privacy status module.
Update database framework package to the latest version which supports this feature.
Thanks to @andrepereiradasilva for the implementation the support for this feature in the database framework (PR's joomla-framework/database#177 and joomla-framework/database#183) and for almost all of the code for this PR.
Pre-condition: Set up your database server to support encrypted connections.
Links to some helpful documentation:
Method 1 - this requires composer to be installed
Apply this Pull Request (PR) on an existing installation of a clean 4.0-dev, using the database server prepared in step 1.
Do a composer update joomla/database
to update the database framework package to the latest version.
Login to backend and change database server in global config server tab from localhost to the full hostname (fqdn) of that server and make sure it can be accessed in that way. E.g. it might be necessary to add it to your hosts file or dns for proper name to ip resolving. The reason for this is that server certificates will normally not be made for localhost.
Check the System Information view.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
Method 2 - use a patched nightly built
Download the nightly build package of today, October 12, 2019, patched with the changes of this PR plus the updated database package from framework here: https://test5.richard-fath.de/Joomla_4.0.0-alpha12-dev-Development-Full_Package_2019-10-14_pr-26375.zip.
Install Joomla with the package downloaded in step 1. When specifying the database server don't use localhost, use the full hostname (fqdn) of that server and make sure it can be accessed in that way. E.g. it might be necessary to add it to your hosts file or dns for proper name to ip resolving. The reason for this is that server certificates will normally not be made for localhost.
Check the System Information view.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
Result: See section "Expected result" below.
System Information
Privacy Status module
Global Configuration, tab "Server", section "Database Settings"
Nothing of the above.
It needs at least to extend the documentation of the server section of Global Configuration, the Privacy Dashboard Module and the System Information view by the new fields.
In addition a documentation about setting up the particular kinds of encrypted server connections could be helpful.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_admin com_config Language & Strings Libraries Front End Plugins |
Labels |
Added:
?
?
|
Category | Administration com_admin com_config Language & Strings Libraries Front End Plugins | ⇒ | Administration com_admin com_config Language & Strings External Library Composer Change Libraries Front End Plugins |
Title |
|
thanks. Will test when possible. Just some early coments:
"No encryption" value is not right. At least in PostgreSQL you can force encryption just in server side. So maybe "Default (server prefered)" or (better) have a new field that, for that option value, shows the current encryption status. That new field could also show only "Not suported" if the DB server doesn't support encrypted connections.
As an example, see the database conectors field https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Field/DatabaseconnectionField.php
"Verify" should be "Verify Server Certificate". In 2 way connection you have a server and a client Certificate so it should be clear that, in this case, is the db server certificate that is being veryfied
IMHO whether the db connection is encrypted or not, is also a data privacy question, so this information should be also in the privacy component. If you have a database connection to other server or to a cloud database as a service, to protect privacy data-in-transit should to be encrypted.
But this is also true for HTTPS, so maybe for other PR.
2. "Verify" should be "Verify Server Certificate". In 2 way connection you have a server and a client Certificate so it should be clear that, in this case, is the db server certificate that is being veryfied
@andrepereiradasilva That's why I've added this information to the "..._DESCR" language strings. But you are right, the title could be changed. Will do that, especially because it seems to me that these "..._DESCR" language strings aren't used anywhere.
Maybe @brianteeman could have a look at the language strings?
Labels |
Added:
?
|
Suggestion:
Prevent "Connexion Encryption" field to be displayed by itself facing database prefix with Verify field not facing it.
It's just a matter of adding a spacer field after the dbprefix
field.
<field
type="spacer"
/>
@infograf768 Would be the first spacer in that xml, and it does not look nice on small screens (mobile). So I leave that decision for later.
@infograf768 The whole Global Settings would look better if the switchers and text fields and drop down selects all would have the same height. But that's another issue .. it just becomes very visible when I apply your suggestion so the "Verify .." switcher is right beside the "Connection Encryption" drop down on a large screen.
Thanks @brianteeman for language strings review.
I don't have step 1 configured. Performed step 2-4.
Here is the error for step 4:
Call to undefined method Joomla\Database\Mysqli\MysqliDriver::isConnectionEncryptionSupported()
Yes,
I just did composer install
and composer update joomla/database
and it killed MySQL. Let me play some more and get back to you.
11:38:27 AM [mysql] Error: MySQL shutdown unexpectedly.
11:38:27 AM [mysql] This may be due to a blocked port, missing dependencies,
11:38:27 AM [mysql] improper privileges, a crash, or a shutdown by another method.
11:38:27 AM [mysql] Press the Logs button to view error logs and check
11:38:27 AM [mysql] the Windows Event Viewer for more clues
11:38:27 AM [mysql] If you need more help, copy and post this
11:38:27 AM [mysql] entire log window on the forums
Here is from the Log:
2019-09-25 11:41:59 0 [Note] InnoDB: Mutexes and rw_locks use Windows interlocked functions
2019-09-25 11:41:59 0 [Note] InnoDB: Uses event mutexes
2019-09-25 11:41:59 0 [Note] InnoDB: Compressed tables use zlib 1.2.11
2019-09-25 11:41:59 0 [Note] InnoDB: Number of pools: 1
2019-09-25 11:41:59 0 [Note] InnoDB: Using SSE2 crc32 instructions
2019-09-25 11:41:59 0 [Note] InnoDB: Initializing buffer pool, total size = 16M, instances = 1, chunk size = 16M
2019-09-25 11:41:59 0 [Note] InnoDB: Completed initialization of buffer pool
2019-09-25 11:42:00 0 [Note] InnoDB: 128 out of 128 rollback segments are active.
2019-09-25 11:42:00 0 [Note] InnoDB: Creating shared tablespace for temporary tables
2019-09-25 11:42:00 0 [Note] InnoDB: Setting file 'C:\xampp\mysql\data\ibtmp1' size to 12 MB. Physically writing the file full; Please wait ...
2019-09-25 11:42:00 0 [Note] InnoDB: File 'C:\xampp\mysql\data\ibtmp1' size is now 12 MB.
2019-09-25 11:42:00 0 [Note] InnoDB: Waiting for purge to start
2019-09-25 11:42:00 0 [Note] InnoDB: 10.3.16 started; log sequence number 1143440372; transaction id 1748347
2019-09-25 11:42:00 0 [Note] InnoDB: Loading buffer pool(s) from C:\xampp\mysql\data\ib_buffer_pool
2019-09-25 11:42:00 0 [Note] InnoDB: Buffer pool(s) load completed at 190925 11:42:00
2019-09-25 11:42:00 0 [Note] Plugin 'FEEDBACK' is disabled.
2019-09-25 11:42:00 0 [Note] Server socket created on IP: '::'.
@Quy Ok, if necessary I'm available at Glip, too. MySQL version and MySQLi client version and so on would be good to know if problem persists. The 2nd log does not look strange, but the first one does. I have no idea yet what the problem could be, except of what the 1st log sais: " ... blocked port, missing dependencies, improper privileges ...". Maybe MySQL on Windows needs some additional SSL lib for encryption? I'll see if I can find something in the docs.
The first output is from XAMPP Control Panel to launch MySQL.
No idea how that could come, that a composer install or update kills MySQL on Windows.
10.4.6-MariaDB
After selecting One-way encryption
and clicking Save
, I get a white screen with the following errors:
Warning: session_start(): Failed to initialize storage module: user (path: C:\xampp\tmp) in \libraries\vendor\joomla\session\src\Storage\NativeStorage.php on line 478
Warning: mysqli::prepare(): invalid object or resource mysqli in \libraries\vendor\joomla\database\src\Mysqli\MysqliStatement.php on line 145
Warning: mysqli::prepare(): invalid object or resource mysqli in \libraries\vendor\joomla\database\src\Mysqli\MysqliStatement.php on line 145
Error: MySQL server has gone away: Could not connect to database: MySQL server has gone away
Warning: mysqli::prepare(): invalid object or resource mysqli in \libraries\vendor\joomla\database\src\Mysqli\MysqliStatement.php on line 145
Warning: session_write_close(): Failed to write session data using user defined save handler. (session.save_path: C:\xampp\tmp) in \libraries\vendor\joomla\session\src\Storage\NativeStorage.php on line 114
@Quy Sorry I was lazy with testing instructions. Please check description at joomla-framework/database#177 . It’s important not to run with socket connection but with TCP/IP, and to use the host for which the certs are made, and that is possibly not localhost event if dB runs on same server as the site. It is all a tricky thing, and it seems some error handling should be added ... suggestions are welcome.
@wilsonge Maybe I should flag this PR as draft or RFC or even close it? It is more a prototype than ready for use, and I can’t do much for it because have other tasks, so it is more a kind of a starting point for people to work on it than a thing which I could make ready for use soon all alone.
Category | Administration com_admin com_config Language & Strings Libraries Front End Plugins External Library Composer Change | ⇒ | Administration com_admin com_config Language & Strings External Library Composer Change Installation Libraries Front End Plugins |
Category | Administration com_admin com_config Language & Strings Libraries Front End Plugins External Library Composer Change Installation | ⇒ | Administration com_admin com_config Language & Strings Modules External Library Composer Change Installation Libraries Front End Plugins |
Whoever has interest in this feature: Could you test again? Thanks to @andrepereiradasilva error there have been made some improvements.
@brianteeman Could you review language stings again? Some new ones have been added with new commits. Thanks in advance.
I can't apply this patch with the latest nightly build:
The file marked for modification does not exist: composer.lock
@twister65 That's why the testing instructions say in step 2: "Apply this Pull Request (PR) on a 4.0-dev ...", and in step 3: "Do a composer update joomla/database
". This means you need to test on a clone of the Joomla CMS' GitHub repository on the 4.0-dev branch, and your testing environment has to be set up as described here: https://docs.joomla.org/J4.x:Setting_Up_Your_Local_Environment.
Is there a reason for this ? Because the nightly build also includes the latest joomla database framework. And I use Patchtester to test PR, so convenient.
@twister65 No, the nightly build does not include the latest state of the database framework, that's why this PR changes composer.lock to fetch the newer version. Version does not mean any release version. is is always 2.0-dev, and the commit checksum is what makes the "version". You can see that when expanding the diff for composer.lock in the list of changed files of this PR here.
Yes it does, I checked it before. PR's joomla-framework/database#177 and joomla-framework/database#183 have been merged.
joomla-framework/database#183 is not included in nightly.
It won't be included tomorrow either, unless someone submits a separate PR to update Composer dependencies (all or just joomla/database
). This is already done in this PR.
I hope
@twister65 and other potential testers: I've updated my testing instructions in the description above by a 2nd test method, which uses a patched nightly build of today. You can find the link for download in the testing instructions for that method. The patched nighly build contains the changes of this PR plus the updated database package from framework.
@twister65 and other potential testers: Please wait with testing, I will add some more changes tonight or tomorrow, German time. I'll let you know when ready.
@twister65 and other potential testers: I've updated the zip package for testing to the state of right now. PR is ready for review and tests.
@brianteeman Could you review the changed language stings and what is much more important, review following point regarding UX consistency? I would be really happy if you could do that.
Following is the UX consistency issue: Up to now, we don't use the hint
property in the text fields of the Global Configuration form. We use it in a few other forms in backend, but not in Global config. The hint
is shown in a grey color inside the field and serves as a hint or example how to fill in the field. Now this PR introduces the usage of hints in Global Config, and because the grey color enough is not really sufficient to show that the content is not a valid default value but a hint, e.g. with an example in case of this PR, the hint texts are starting with 'ex: ', which stands for example. But this has been done nowhere else in fields in backend where hints are used, no prefix for such purpose.
What do you think? Shall we drop the hints from this PR? Or change 'ex: ' to 'example: '? Or change it to 'hint: '? Or maybe make another PR which adds a static text prefix 'hint: ' (or course translatable) to all texts of those hints?
Your recommendation would be very welcome.
Hints are bad for accessibility for several reasons and are also bad for ux as they disappear as soon as you focus on the input.
If you absolutely need the information that you are displaying in the hint then move it to the description. I suspect that you really don't
Ex - is not a valid abbreviation. Use eg
see https://developer.joomla.org/en-gb-user-interface-text-guidelines/punctuation.html
@brianteeman Thanks for the feedback and review. I will fix remaining stuff soon.
@brianteeman I've removed the hints and added their info to the descriptions. I still have 2 questions, see comments to code above. Could you check again? Thanks in advance and thanks for review up to now.
@brianteeman Except of the conditional display (showon) for the MySQL-specific field, all should be done as you suggested. I hope I haven't missed or misunderstood anything.
so will you be doing the conditional display?
@brianteeman Am trying. Not sure if showon="databaseconnection:mysql,mysqli[AND]dbencryption:2"
will work.
@brianteeman Am testing the showon right now, and if it will not work I'll undo. Do you know if I have to be careful with something regarding showon? Am not very familiar with it.
@brianteeman Am testing the showon right now, and if it will not work I'll undo. Do you know if I have to be careful with something regarding showon? Am not very familiar with it.
Should be: showon="dbtype:mysql,mysqli[AND]dbencryption:2"
there are actually two fields (cipher and capath) that only work in mysqli adn PDO mysql, not in postgresql
https://github.com/joomla-framework/database/blob/2.0-dev/src/Pdo/PdoDriver.php#L280-L286
@andrepereiradasilva Thanks, will change the showon for both. @brianteeman Should I leave the '(MySQL only)' in the text despite of the showon, just in case? Or shall I remove that because showon is enough?
@andrepereiradasilva You are right that some information has to be documented somewhere. We have to do that at the help pages or on the docs wiki or both. The _DESCR
texts are the wrong place for too long explanations, I agree with @brianteeman at this point. Unfortunately I share your fear that this documentation might never happen.
The documentation will happen if you write it - you guys are the best people to write it because you are the ones who understand it the most.
Ideally any new feature should have documentation with it as a requirement before it can be finally merged
@brianteeman I agree, but I would not be the ideal writer. @andrepereiradasilva knows the feature better than I do. Maybe we can do that together and I can put it on the wiki. For today I have to finish, gotta work tomorrow morning. And then I have primary focus still on database datetime stuff. Next weekend I could find time, I hope. We stay in contact, Andre.
@twister65 and other potential testers: I've updated the patched full install zip package linked in the testing instructions for those who do not have a git repository clone with composer and so on.
PR should be ready for testing now.
It would be great to add a RFT (Ready For Test) label for Joomla's PRs
@twister65 Well, if I had known that it takes several iterations to make it ready for testing, I would have made this PR as draft PR first and then later at the end marked it as ready for review, which means "undraft". But if that happens too early, there is no way back to switch from normal PR back to draft, and so you are again where we are now. This switching back from draft to not draft is a functionality requested by users of GitHub but not impemented yet.
Labels |
Added:
?
|
I think texts should be ok now. Thanks @brianteeman for the reviews.
@andrepereiradasilva I will try to find time on weekend to write some documentation. If not this weekend, then the next one. I'll ping you for a review at the end before I'll publish, but you shouldn't have much work with it.
Solved conflicts.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-10-19 22:15:00 |
Closed_By | ⇒ | wilsonge |
Thanks!
Updated section "Documentation Changes Required" of the description by what I think that is necessary.
@alikon @andrepereiradasilva @twister65 Could you test this PR?