Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] [New feature] Add tls transfer encryption for database connections #26375

Merged

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Sep 21, 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.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Sep 21, 2019
@richard67 richard67 changed the title [4.0] [WiP] [New feature] Add tls transfer encryption for database connections [4.0] [New feature] Add tls transfer encryption for database connections Sep 21, 2019
@richard67
Copy link
Member Author

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

@andrepereiradasilva
Copy link
Contributor

andrepereiradasilva commented Sep 22, 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.

@richard67
Copy link
Member Author

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?

@infograf768
Copy link
Member

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"
		/>

Co-Authored-By: Brian Teeman <brian@teeman.net>
Co-Authored-By: Brian Teeman <brian@teeman.net>
@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

Thanks @brianteeman for language strings review.

@Quy
Copy link
Contributor

Quy commented Sep 25, 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()

@richard67
Copy link
Member Author

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

@Quy
Copy link
Contributor

Quy commented Sep 25, 2019

Yes,

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@Quy
Copy link
Contributor

Quy commented Sep 25, 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

@Quy
Copy link
Contributor

Quy commented Sep 25, 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: '::'.

Co-Authored-By: Brian Teeman <brian@teeman.net>
@richard67
Copy link
Member Author

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

@brianteeman
Copy link
Contributor

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@brianteeman
Copy link
Contributor

@Quy please add the documentation required label

@twister65
Copy link
Contributor

twister65 commented Oct 14, 2019

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

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

@richard67
Copy link
Member Author

Solved conflicts.

@wilsonge wilsonge merged commit ca73cb9 into joomla:4.0-dev Oct 19, 2019
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 19, 2019
@richard67 richard67 deleted the 4.0-dev-tls-encryption-for-db-connections branch October 19, 2019 22:26
@richard67
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants