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

Fix #5344 Add the ability to provide and use a CA file for mysql servers with require_secure_transport=on #5418

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

GABRIELNGBTUC
Copy link

@GABRIELNGBTUC GABRIELNGBTUC commented Dec 6, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #5344

This PR adds the following:

  • A file input on the database setup UI for mariadb/mysql to optionally upload a PEM file
  • The setup database endpoint has been modified to attempt to connect to the database with this PEM file when provided
  • The writeDBConfig function has been modified to move a provided CA file (from either the environment variable or the setup through the UI) to a file name mariadb-ca.pem inside the data directory
  • db-config.json can now contains a new property caFilePath with the path of the CA file to use when using secure connections
  • The connect function has been modified to use secure connections when the property caFilePath is present in the db-config.json

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image

// Move CA file to the data directory
if (dbConfig.caFilePath) {
const dataCaFilePath = path.join(Database.dataDir, "mariadb-ca.pem");
fs.renameSync(dbConfig.caFilePath, dataCaFilePath);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternate approach shall be , using a generic CA bundle : Eg: Mozilla. Then provide an env variable to override it, either as a path or the ca bundle content itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first attempt at implementing this was to manage this entirely through environment variables but then I read from the contibuting.md the following:

I personally do not like something that requires a lot of configuration before you can finally start the app. The goal is to make the Uptime Kuma installation as easy as installing a mobile app.

And

Settings should be configurable in the frontend. Environment variables are discouraged, unless it is related to startup such as DATA_DIR

I believe this is something that should be configured through environment variables but if we want to respect the whishes of the maintainer, the user upload has to be there with all the complications it implies.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again just opinion :)

If we look at various feature requests for external Db, I believe it was not implemented early to keep the simplicity you quoted. A user base who may use a dedicated DB would be certainly having the knowledge and willingness to get it done, by reading docs and Configure As Code , even if there are efforts involved.

In that context, if we add a UI element, theoretically it is cluttering AND/OR complicating the experience of the user base who needs just out of the box experience.

On the other hand, if we do it via configuration those who need it can implement in a CAC fashion, hopefully it is efficient for those user base.

In a nutshell , if we do it via config, we may able to stretch the functionality without affecting the core slogan of simplicity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@louislam Would it be possible to have your input on this as I do agree with @vsmanu proposal.

Changes are minimal and simpler with an environment variable only configuration but this would only be possible if Quote: Settings should be configurable in the frontend. is not an hard requirement.

@GABRIELNGBTUC GABRIELNGBTUC marked this pull request as draft December 18, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot connect to azure mysql flexible server while --require_secure_transport=ON
2 participants