feat: Make Database SSL Configurable through files #6892

chriswk · 2024-04-19T08:02:25Z

This makes it configurable either through a single JSON file with all three certificates as separate keys or via separate files per ca/cert/key key.

fixes #6718

vercel · 2024-04-19T08:02:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 19, 2024 10:45am
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 19, 2024 10:45am

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 1 findings(s) 🚩
Improving Code Health: 1 findings(s) ✅

View detailed results in CodeScene

This makes it configurable either through a single JSON file with all three certificates as separate keys or via separate files per ca/cert/key key. fixes #6718

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 1 findings(s) 🚩
Improving Code Health: 1 findings(s) ✅

View detailed results in CodeScene

sighphyre · 2024-04-19T09:19:41Z

src/lib/create-config.ts

+        return JSON.parse(process.env.DATABASE_SSL);
+    } else if (
+        process.env.DATABASE_SSL_CA_CONFIG != null &&
+        filesExists(process.env.DATABASE_SSL_CA_CONFIG)


Not convinced about the file existence checks here. Is there a place where we want the file check to return true but return an empty object if it's unreadable or contains broken JSON?

Maybe it's the Python dev in me but I'd lean on just reading the thing and trapping the exception case

I think that's a fair point, I think failing on non-existing files if you use the env vars is fair enough

sighphyre · 2024-04-19T09:21:02Z

src/lib/create-config.ts

+    try {
+        return readFileSync(caFilePath).toJSON();
+    } catch (e) {
+        return {};


I'd be tempted to just bubble here. If you asked for this to happen and it didn't, I'd be rather surprised to find a quiet fail without a log

Agreed. Let's fail hard, we do in other paths for config.

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 1 findings(s) 🚩
Improving Code Health: 1 findings(s) ✅

View detailed results in CodeScene

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 1 findings(s) 🚩

View detailed results in CodeScene

codescene-delta-analysis · 2024-04-19T10:17:17Z

src/lib/create-config.ts

@@ -1,25 +1,26 @@
 import { parse } from 'pg-connection-string';
 import merge from 'deepmerge';
 import * as fs from 'fs';
+import { readFileSync } from 'fs';


ℹ Getting worse: Overall Code Complexity
The mean cyclomatic complexity increases from 5.24 to 5.28, threshold = 4

codescene-delta-analysis

✅ Code Health Quality Gates: OK

View detailed results in CodeScene

alvinometric · 2024-04-19T10:54:14Z

Just to confirm, this doesn't change the default username and passwords that are setup when self-hosting, right? It just lets you configure it.

chriswk · 2024-04-19T11:59:17Z

Just to confirm, this doesn't change the default username and passwords that are setup when self-hosting, right? It just lets you configure it.

It doesn't touch anything for default username and password, it's concerned with configuring a secure connection to the database.
I see the autoformatter has touched quite a bit of the configuring-unleash file that did not receive any actual eyeballs on it. Just to confirm that the new keys are

DATABASE_SSL_CA_FILE: sets ssl: { ca }
DATABASE_SSL_KEY_FILE: sets ssl: { key }
DATABASE_SSL_CERT_FILE: sets ssl: { cert }

DATABASE_SSL_CA_CONFIG: expects a path to a json file with { rejectUnauthorized: bool, ca: string, key: string, cert: string }, currently, the DATABASE_SSL option expects a stringified json object with the same keys, set in an environment variable. This is often hard to manage in various kubernetes setups. it's better to be able to write an actual json file to disk, and then read this in to memory in Unleash.

DATABASE_SSL_REJECT_UNAUTHORIZED: sets ssl: { rejectUnauthorized }

alvinometric · 2024-04-19T12:10:11Z

Perfect, thanks

sighphyre

Ye! Code looks good, so I'm gonna tack on an approve. I'll put comments on the docs if I spot anything

Wow, IntelliJ got a little intense with the whitespace here but the docs look good at a glance

egor-xyz · 2024-05-06T11:41:37Z

Just to confirm, this doesn't change the default username and passwords that are setup when self-hosting, right? It just lets you configure it.

It doesn't touch anything for default username and password, it's concerned with configuring a secure connection to the database. I see the autoformatter has touched quite a bit of the configuring-unleash file that did not receive any actual eyeballs on it. Just to confirm that the new keys are

DATABASE_SSL_CA_FILE: sets ssl: { ca }

DATABASE_SSL_KEY_FILE: sets ssl: { key }

DATABASE_SSL_CERT_FILE: sets ssl: { cert }

DATABASE_SSL_CA_CONFIG: expects a path to a json file with { rejectUnauthorized: bool, ca: string, key: string, cert: string }, currently, the DATABASE_SSL option expects a stringified json object with the same keys, set in an environment variable. This is often hard to manage in various kubernetes setups. it's better to be able to write an actual json file to disk, and then read this in to memory in Unleash.

DATABASE_SSL_REJECT_UNAUTHORIZED: sets ssl: { rejectUnauthorized }

Can I set only the CA file path without DATABASE_SSL_KEY_FILE and DATABASE_SSL_CERT_FILE? AWS provides me only one CA file, and it's enough for connection to DB

Regarding ticket #6892: I would like to enable the use of a CA certificate without requiring other certificates. This would be useful for AWS Helm, as AWS only provides a single PEM file for DB connections.

chriswk requested review from Tymek and sighphyre April 19, 2024 08:02

chriswk self-assigned this Apr 19, 2024

codescene-delta-analysis bot reviewed Apr 19, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend April 19, 2024 08:04 View deployment

feat: Make Database SSL Configurable

c30898b

This makes it configurable either through a single JSON file with all three certificates as separate keys or via separate files per ca/cert/key key. fixes #6718

chriswk force-pushed the task/makeDbSslConfigurableFromFile branch from 192914d to c30898b Compare April 19, 2024 08:06

codescene-delta-analysis bot reviewed Apr 19, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend April 19, 2024 08:07 View deployment

chriswk mentioned this pull request Apr 19, 2024

Easier DB CA certificate configuration #6718

Closed

sighphyre reviewed Apr 19, 2024

View reviewed changes

don't check file existence

10d5f0e

chriswk force-pushed the task/makeDbSslConfigurableFromFile branch from 78bd728 to 10d5f0e Compare April 19, 2024 10:16

codescene-delta-analysis bot reviewed Apr 19, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend April 19, 2024 10:18 View deployment

fix: SSL config doesn't necessarily need a CA

da4eb13

chriswk requested review from alvinometric and nnennandukwe as code owners April 19, 2024 10:41

codescene-delta-analysis bot approved these changes Apr 19, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend April 19, 2024 10:42 View deployment

vercel bot deployed to Preview – unleash-docs April 19, 2024 10:45 View deployment

sighphyre approved these changes Apr 19, 2024

View reviewed changes

chriswk merged commit ff6297d into main Apr 19, 2024
15 checks passed

chriswk deleted the task/makeDbSslConfigurableFromFile branch April 19, 2024 12:38

egor-xyz mentioned this pull request May 6, 2024

feat: Allow to use CA certificate file path for DB #6985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make Database SSL Configurable through files #6892

feat: Make Database SSL Configurable through files #6892

chriswk commented Apr 19, 2024

vercel bot commented Apr 19, 2024 •

edited

Loading

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot left a comment

sighphyre Apr 19, 2024

chriswk Apr 19, 2024

sighphyre Apr 19, 2024

chriswk Apr 19, 2024

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot Apr 19, 2024

codescene-delta-analysis bot left a comment

alvinometric commented Apr 19, 2024

chriswk commented Apr 19, 2024 •

edited

Loading

alvinometric commented Apr 19, 2024

sighphyre left a comment •

edited

Loading

egor-xyz commented May 6, 2024

feat: Make Database SSL Configurable through files #6892

feat: Make Database SSL Configurable through files #6892

Conversation

chriswk commented Apr 19, 2024

vercel bot commented Apr 19, 2024 • edited Loading

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

sighphyre Apr 19, 2024

Choose a reason for hiding this comment

chriswk Apr 19, 2024

Choose a reason for hiding this comment

sighphyre Apr 19, 2024

Choose a reason for hiding this comment

chriswk Apr 19, 2024

Choose a reason for hiding this comment

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

codescene-delta-analysis bot Apr 19, 2024

Choose a reason for hiding this comment

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

alvinometric commented Apr 19, 2024

chriswk commented Apr 19, 2024 • edited Loading

alvinometric commented Apr 19, 2024

sighphyre left a comment • edited Loading

Choose a reason for hiding this comment

egor-xyz commented May 6, 2024

vercel bot commented Apr 19, 2024 •

edited

Loading

chriswk commented Apr 19, 2024 •

edited

Loading

sighphyre left a comment •

edited

Loading