? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
21 Sep 2019

Pull Request for Issue # .

Summary of Changes

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.

Testing Instructions

Pre-condition: Set up your database server to support encrypted connections.

Links to some helpful documentation:

Method 1 - this requires composer to be installed

  1. Apply this Pull Request (PR) on an existing installation of a clean 4.0-dev, using the database server prepared in step 1.

  2. Do a composer update joomla/database to update the database framework package to the latest version.

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

  4. Check the System Information view.

Result: See section "Expected result" below.

  1. Go to Global Cofiguration, tab "Server" and check the options for database connection encryption.

Result: See section "Expected result" below.

  1. Add the Privacy Status module to one of the dashboards, e.g. the "Users" dashboard, then check display of database connection encryption status in that module.

Result: See section "Expected result" below.

  1. Play around with the options. Note that this SSL stiff is a bit tricky, so first read all available documentation linked above and research for your db server and server operating system before blaming this PR doing something wrong. Check the system info after changes.

Result: See section "Expected result" below.

Method 2 - use a patched nightly built

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

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

  3. Check the System Information view.

Result: See section "Expected result" below.

  1. Go to Global Cofiguration, tab "Server" and check the options for database connection encryption.

Result: See section "Expected result" below.

  1. Add the Privacy Status module to one of the dashboards, e.g. the "Users" dashboard, then check display of database connection encryption status in that module.

Result: See section "Expected result" below.

  1. Play around with the options. Note that this SSL stiff is a bit tricky, so first read all available documentation linked above and research for your db server and server operating system before blaming this PR doing something wrong. Check the system info after changes.

Result: See section "Expected result" below.

Expected result

System Information

  1. No encryption active:

snap-6

  1. Encryption active:

snap-10

Privacy Status module

  1. No encryption active:

db-tls-mod-priv-sts-disabled

  1. Encryption active:

db-tls-mod-priv-sts-enabled

Global Configuration, tab "Server", section "Database Settings"

  1. Default, i.e. encryption is controlled by the server:

snap-7

  1. One way encryption requested by the client:

snap-8

  1. Two way encryption requested by the client:

snap-9

Actual result

Nothing of the above.

Documentation Changes Required

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.

avatar richard67 richard67 - open - 21 Sep 2019
avatar richard67 richard67 - change - 21 Sep 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Sep 2019
Category Administration com_admin com_config Language & Strings Libraries Front End Plugins
avatar richard67 richard67 - change - 21 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 21 Sep 2019
avatar richard67 richard67 - change - 21 Sep 2019
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 21 Sep 2019
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
avatar richard67 richard67 - change - 21 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 21 Sep 2019
avatar richard67 richard67 - change - 21 Sep 2019
Title
[4.0] [WiP] [New feature] Add tls transfer encryption for database connections
[4.0] [New feature] Add tls transfer encryption for database connections
avatar richard67 richard67 - edited - 21 Sep 2019
avatar richard67 richard67 - change - 21 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 21 Sep 2019
avatar richard67
richard67 - comment - 21 Sep 2019

@alikon @andrepereiradasilva @twister65 Could you test this PR?

avatar richard67 richard67 - change - 21 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 21 Sep 2019
avatar andrepereiradasilva
andrepereiradasilva - comment - 22 Sep 2019

thanks. Will test when possible. Just some early coments:

  1. "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

  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

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

avatar richard67
richard67 - comment - 22 Sep 2019
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?

avatar richard67 richard67 - change - 22 Sep 2019
Labels Added: ?
avatar infograf768
infograf768 - comment - 22 Sep 2019

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"
		/>
avatar richard67
richard67 - comment - 22 Sep 2019

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

avatar richard67
richard67 - comment - 22 Sep 2019

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

avatar richard67
richard67 - comment - 22 Sep 2019

Thanks @brianteeman for language strings review.

avatar richard67 richard67 - change - 22 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 22 Sep 2019
avatar richard67 richard67 - change - 22 Sep 2019
The description was changed
avatar richard67 richard67 - edited - 22 Sep 2019
avatar Quy
Quy - comment - 25 Sep 2019

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()

avatar richard67
richard67 - comment - 25 Sep 2019

@Quy Did you do step 3 composer update to get the latest version of the database framework package?

avatar Quy
Quy - comment - 25 Sep 2019

Yes,

avatar richard67
richard67 - comment - 25 Sep 2019

@Quy Could you check composer.lock, if it contains same version as the one in this PR for the database package?

avatar richard67
richard67 - comment - 25 Sep 2019

@Quy If yes and composer.lock is ok, then it needs maybe a composer install instead of a composer update joomla/database?

avatar richard67
richard67 - comment - 25 Sep 2019

@Quy That error normally comes only when the database package is not up to date enough to have that function, like it was for me before I did the composer update. Could you try composer install? Maybe update does nothing if composer.lock is already updated?

avatar Quy
Quy - comment - 25 Sep 2019

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
avatar Quy
Quy - comment - 25 Sep 2019

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: '::'.
avatar richard67
richard67 - comment - 25 Sep 2019

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

avatar Quy
Quy - comment - 25 Sep 2019

The first output is from XAMPP Control Panel to launch MySQL.

avatar richard67
richard67 - comment - 25 Sep 2019

No idea how that could come, that a composer install or update kills MySQL on Windows.

avatar Quy
Quy - comment - 25 Sep 2019

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
avatar richard67
richard67 - comment - 26 Sep 2019

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

avatar richard67
richard67 - comment - 26 Sep 2019

@Quy Maybe the MySQL log shows more details about what happened?

avatar richard67
richard67 - comment - 26 Sep 2019

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

avatar joomla-cms-bot joomla-cms-bot - change - 5 Oct 2019
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
avatar joomla-cms-bot joomla-cms-bot - change - 5 Oct 2019
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
avatar richard67 richard67 - change - 5 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 5 Oct 2019
avatar richard67 richard67 - change - 5 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 5 Oct 2019
avatar richard67 richard67 - change - 5 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 5 Oct 2019
a70adf7 5 Oct 2019 avatar richard67 CS
avatar richard67
richard67 - comment - 5 Oct 2019

Whoever has interest in this feature: Could you test again? Thanks to @andrepereiradasilva error there have been made some improvements.

avatar richard67
richard67 - comment - 5 Oct 2019

@brianteeman Could you review language stings again? Some new ones have been added with new commits. Thanks in advance.

avatar twister65
twister65 - comment - 11 Oct 2019

I can't apply this patch with the latest nightly build:

The file marked for modification does not exist: composer.lock

avatar richard67
richard67 - comment - 11 Oct 2019

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

avatar twister65
twister65 - comment - 11 Oct 2019

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.

avatar richard67
richard67 - comment - 11 Oct 2019

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

avatar twister65
twister65 - comment - 11 Oct 2019

Yes it does, I checked it before. PR's joomla-framework/database#177 and joomla-framework/database#183 have been merged.

avatar SharkyKZ
SharkyKZ - comment - 11 Oct 2019

joomla-framework/database#183 is not included in nightly.

avatar twister65
twister65 - comment - 11 Oct 2019

Exactly @SharkyKZ , I'm waiting tomorrow morning then...

avatar SharkyKZ
SharkyKZ - comment - 11 Oct 2019

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.

avatar richard67
richard67 - comment - 11 Oct 2019

Thanks @SharkyKZ for helping with explanations. Am at work right now and so have only limited possibilities.

avatar twister65
twister65 - comment - 11 Oct 2019

I hope?

avatar richard67 richard67 - change - 12 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 12 Oct 2019
avatar richard67 richard67 - change - 12 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 12 Oct 2019
avatar richard67 richard67 - change - 12 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 12 Oct 2019
avatar richard67
richard67 - comment - 12 Oct 2019

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

avatar richard67
richard67 - comment - 12 Oct 2019

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

avatar richard67 richard67 - change - 13 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 13 Oct 2019
avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar brianteeman
brianteeman - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

@brianteeman Thanks for the feedback and review. I will fix remaining stuff soon.

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar brianteeman
brianteeman - comment - 13 Oct 2019

so will you be doing the conditional display?

avatar richard67
richard67 - comment - 13 Oct 2019

@brianteeman Am trying. Not sure if showon="databaseconnection:mysql,mysqli[AND]dbencryption:2" will work.

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Oct 2019

@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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar brianteeman
brianteeman - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar richard67
richard67 - comment - 13 Oct 2019

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

avatar brianteeman
brianteeman - comment - 13 Oct 2019

@Quy please add the documentation required label

avatar twister65
twister65 - comment - 14 Oct 2019

It would be great to add a RFT (Ready For Test) label for Joomla's PRs ?

avatar richard67
richard67 - comment - 14 Oct 2019

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

avatar richard67 richard67 - change - 14 Oct 2019
Labels Added: ?
avatar richard67 richard67 - change - 14 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 14 Oct 2019
avatar richard67
richard67 - comment - 14 Oct 2019

I think texts should be ok now. Thanks @brianteeman for the reviews.

avatar richard67
richard67 - comment - 14 Oct 2019

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

avatar richard67
richard67 - comment - 19 Oct 2019

Solved conflicts.

avatar wilsonge wilsonge - close - 19 Oct 2019
avatar wilsonge wilsonge - merge - 19 Oct 2019
avatar wilsonge wilsonge - change - 19 Oct 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-10-19 22:15:00
Closed_By wilsonge
avatar wilsonge
wilsonge - comment - 19 Oct 2019

Thanks!

avatar richard67 richard67 - change - 1 Nov 2019
The description was changed
avatar richard67 richard67 - edited - 1 Nov 2019
avatar richard67
richard67 - comment - 1 Nov 2019

Updated section "Documentation Changes Required" of the description by what I think that is necessary.

Add a Comment

Login with GitHub to post a comment