Add support for secure connections to the database
Changes PlannedPublic
Actions

Authored by speck on Jun 3 2023, 21:34.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15045: Support SSL/TLS for MariaDB connections

Summary

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.

Test Plan

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

speck created this revision.Jun 3 2023, 21:34

speck held this revision as a draft.

Owners added a reviewer: O1: Blessed Committers.Jun 3 2023, 21:34

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJun 3 2023, 21:34

Harbormaster completed remote builds in B511: Diff 886.Jun 3 2023, 21:34

speck mentioned this in T15045: Support SSL/TLS for MariaDB connections.Jun 3 2023, 21:35

Add the mysql.use-tls option

Harbormaster completed remote builds in B515: Diff 890.Jun 5 2023, 01:32

valerio.bozzolan added a task: T15045: Support SSL/TLS for MariaDB connections.Jun 16 2023, 09:22

Remove client side verification and cipher specification for now

Harbormaster completed remote builds in B687: Diff 1151.Jul 22 2023, 04:03

speck edited the summary of this revision. (Show Details)Jul 22 2023, 04:13

speck edited the test plan for this revision. (Show Details)

• l2dy subscribed.Nov 9 2023, 11:50

• l2dy added inline comments.

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.