-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Database ssl support #6371
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
Database ssl support #6371
Conversation
|
@RooHTaylor is it intentional that this is a draft? |
|
It it is, could you give me reproduction steps how to run Mariadb in the mode so it can use this change? |
Rule 10 of the Pull request guidelines says "When publishing your PR, set it as a Draft pull request to allow for review and prevent automatic merging." so I made it a draft.
Install Mariadb or MySQL on a machine not hosting uptime-kuma and modify the server configuration to contain: Restart to allow ssl connections. You don't need to include the Configure uptime-kuma to use Mariadb as the back-end database, and include the additional ENV variables To confirm 5344 is resolved, also configure Mariadb with |
|
Is it possible to run in docker with this? If yes, could you write up the steps to doing so? I have not used mariadb in such a way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds SSL/TLS support for external MariaDB/MySQL database connections to address security requirements when connecting to remote database servers. The implementation introduces two new environment variables (UPTIME_KUMA_DB_SSL and UPTIME_KUMA_DB_SSL_CA) to enable encrypted database connections with optional CA certificate verification.
Key changes:
- Added
getSslConfig()method to conditionally generate MySQL SSL configuration based on database settings - Extended database configuration to include SSL-related properties (
sslandssl_ca) - Applied SSL configuration to all MariaDB connection points (initial setup, database creation, and runtime connections)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/database.js | Adds getSslConfig() static method to generate SSL options; updates JSDoc types to include new ssl-related fields; applies SSL config to MariaDB connections during database initialization |
| server/setup-database.js | Reads SSL environment variables (UPTIME_KUMA_DB_SSL and UPTIME_KUMA_DB_SSL_CA) during database setup; applies SSL configuration to test connection during database setup flow |
| log.warn("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | ||
| sslOptions.rejectUnauthorized = false; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: When the CA file cannot be read (e.g., due to file not found or permission issues), the code silently falls back to rejectUnauthorized: false, which disables certificate verification entirely. This creates a security vulnerability as it makes the connection susceptible to man-in-the-middle attacks.
Consider either:
- Throwing an error when the CA file path is provided but cannot be read, forcing the user to fix the configuration
- At minimum, use
log.errorinstead oflog.warnto make the severity clear, since this is a critical security degradation
The current approach of silently accepting insecure connections when a CA path is misconfigured could lead to production security issues going unnoticed.
| log.warn("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | |
| sslOptions.rejectUnauthorized = false; | |
| log.error("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | |
| throw new Error(`Failed to read database SSL CA file from ${dbConfig.ssl_ca}: ${error.message}`); |
| if (dbConfig.ssl_ca) { | ||
| try { | ||
| sslOptions.ca = fs.readFileSync(dbConfig.ssl_ca, "utf8"); | ||
| } catch (error) { | ||
| log.warn("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | ||
| sslOptions.rejectUnauthorized = false; | ||
| } | ||
| } else { | ||
| sslOptions.rejectUnauthorized = false; | ||
| } |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: When SSL is enabled but no CA certificate path is provided, the code defaults to rejectUnauthorized: false, which disables certificate verification. This approach is insecure by default.
Consider implementing one of these safer alternatives:
- Default to
rejectUnauthorized: true(use system CA bundle) and only set it to false if explicitly configured via a separate environment variable likeUPTIME_KUMA_DB_SSL_REJECT_UNAUTHORIZED=false - Require the CA certificate path when SSL is enabled, failing the connection if neither CA nor an explicit opt-out is provided
The current behavior means users enabling SSL thinking they're securing their connection are actually vulnerable to MITM attacks unless they also provide a CA certificate.
| if (dbConfig.ssl_ca) { | |
| try { | |
| sslOptions.ca = fs.readFileSync(dbConfig.ssl_ca, "utf8"); | |
| } catch (error) { | |
| log.warn("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | |
| sslOptions.rejectUnauthorized = false; | |
| } | |
| } else { | |
| sslOptions.rejectUnauthorized = false; | |
| } | |
| // Default to secure behavior: verify certificates unless explicitly disabled. | |
| let rejectUnauthorized = true; | |
| const rejectEnv = process.env.UPTIME_KUMA_DB_SSL_REJECT_UNAUTHORIZED; | |
| if (typeof rejectEnv === "string" && rejectEnv.toLowerCase() === "false") { | |
| rejectUnauthorized = false; | |
| } | |
| if (dbConfig.ssl_ca) { | |
| try { | |
| sslOptions.ca = fs.readFileSync(dbConfig.ssl_ca, "utf8"); | |
| } catch (error) { | |
| log.warn("db", `Failed to read CA file from ${dbConfig.ssl_ca}: ${error.message}`); | |
| } | |
| } | |
| sslOptions.rejectUnauthorized = rejectUnauthorized; |
| /** | ||
| * Conditionally generates the MySQL SSL configuration object. | ||
| * @param {{type: "sqlite"} | {type:envString, hostname:envString, port:envString, database:envString, username:envString, password:envString, ssl:boolean, ssl_ca:envString}} dbConfig the database configuration |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation: The JSDoc @typedef is defined inline but should be placed outside the function parameter documentation for better readability and reusability. Additionally, the complex type definition for dbConfig is duplicated across multiple methods. Consider defining a proper typedef once at the class level or file level and reusing it.
For example:
/**
* @typedef {Object} DBConfig
* @property {string} type - Database type
* @property {envString} [hostname] - Database hostname
* @property {envString} [port] - Database port
* @property {envString} [database] - Database name
* @property {envString} [username] - Database username
* @property {envString} [password] - Database password
* @property {boolean} [ssl] - Enable SSL connection
* @property {envString} [ssl_ca] - Path to SSL CA certificate
*/
/**
* Conditionally generates the MySQL SSL configuration object.
* @param {DBConfig} dbConfig the database configuration
* @returns {{ssl: object} | undefined} An object containing the ssl configuration, or undefined.
*/This same typedef appears to be duplicated in lines 169, 189, and 198.
I don't use docker, but I found this blog post that describes the process for MySQL. It should be basically identical for mariadb. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
CommanderStorm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I need to find some time to figure this out then.
-> putting it on the backlog (#6260)
There are a number of Copilot suggestions that look resonable for you to check in the mean time
If you want to make reviews simpler, providing a short bash script how to set this up that you have tested would be how to fast-track this issue.
|
I think this PR (which is a superset of your work) is much more promising: -> lets focus on this instead, thank you for the PR |
Excellent! I really didn't have the time to invest into giving this the finesse it needed, beyond getting my setup running. I'm glad someone else was able to run with it. Thanks. |
What problem does this pull request address?
This pull request solves the issue of unencrypted database traffic to an external MariaDB/MySQL database for the Uptime-Kuma back-end database.
Please provide a detailed explanation here.
When connecting to an external MariaDB database as the back-end database, SSL is not an option. This isn't an issue if the database is on the host, but if the database server is not local then best practice would dictate encryption be used.
I'm not a Node programmer, and I didn't have a wealth of time to invest into the functionality, so hopefully this is good enough.
My changes allow for SSL encryption between Uptime-Kuma and a MariaDB database, with the option of providing a CA certificate to verify the server. I did not implement sending client certificates, though my code could easily be expanded to include this in the function to generate the sslOptions.
The change adds two new environment variable options -
UPTIME_KUMA_DB_SSLwhich should betrueto enable ssl, and optionallyUPTIME_KUMA_DB_SSL_CAwhich should be the path to the trusted CA certificate for verification. If a CA is not provided or cannot be opened, but SSL is toggled on, thenrejectUnauthorized = falseis set by default, which will not validate the server certificate.I was torn between failing to connect if a CA is provided but unavailable (i.e. can't open file), and ended up settling on logging an error and turning off verification. I'm still torn on whether this is the right method, but it's what the code says right now, and it can be changed if someone disagrees.
The change also adds two entries to the dbConfig.json file:
ssl:boolean, ssl_ca:envString.If those options are false or not set, then the database connection happens as normal.
require_secure_transportcondition.🛠️ Type of change
📄 Checklist
I couldn't get ESLint working. If this is a critical step I'll try to carve out some time for it.
I have only tested in my environment, which is bare metal Debian 13 running Node 24.11.0
I don't use Docker, but I can't think of a reason why this wouldn't work in the Docker deployment.
Documentation changes will need to be in the wiki after the PR is merged. Not sure where to include them