This adds a new database configuration mysql.use-tls which will require the connection to the database to use TLS. The default value is false for backwards compatibility.
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15045: Support SSL/TLS for MariaDB connections
I set up mariadb to require connections using TLS, with key and self-signed cert. With mysql.use-tls unspecified I verified that initial startup of phorge failed with an error connecting to the database. I changed the config to be true and verified that phorge started up and ran database migrations.
Diff Detail
- Repository
- rP Phorge
- Branch
- cspeck-mysql-ssl
- Lint
Lint Passed Severity Location Code Message Advice src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php:89 XHP16 TODO Comment Advice src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php:92 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 687 Build 687: arc lint + arc unit
Event Timeline
scripts/sql/manage_storage.php | ||
---|---|---|
151 | getUseTLS looks better than getUseTls to me. It's also the style used in PhutilLDAPAuthAdapter.php (setLDAPStartTLS). | |
src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php | ||
99 | Some database providers use custom CAs to verify the server, and it's more common than client certs as far as I know. I think we should make CA certificates configurable as well. |
Opening this up from draft if communication/reviews are happening
src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php | ||
---|---|---|
99 | Yeah I think I'd like both arguments here to be used. What I prefer to happen is for these options to essentially be configured by php.ini rather than phabricator configurations/settings. I haven't really looked much into it though. |
I basically completely trust speck if they say this works with TLS, and I verified this works without TLS. Tried with / without this configuration.
I agree on the undone inline comments. Please check them before landing. Thank you!
I think there’s still a chunk to do here, including additional documentation during setup