feat: added tls config #5530
feat: added tls config #5530
Conversation
ErikMichelson
added
the
type: feature enhancement
Avi98
force-pushed
the
tls-config
branch
3 times, most recently
from
bf90dbd
to
92f2d0d
Compare
Erik, I have added an env flag
Tested joi validation for following condition
Screen.Recording.2024-03-08.at.6.07.25.PM.mov |
There was a problem hiding this comment.
This gets in the right direction! :)
What still surprises me is the reason we need completely separate environment variables for postgres and maraidb/mysql? I get that you intended to make explicit which options are only relevant for the chosen database type. But having for example both HD_SQL_SSL_CA_PATH and HD_PG_SSL_CA_PATH seems a bit unnecessary to me. For changing that the code doesn't need greater changes, it's more a thing of renaming the variables and updating the documentation.
| const databaseSchema = Joi.object({ | ||
| type: Joi.string() | ||
| .valid(...Object.values(DatabaseType)) | ||
| .label('HD_DATABASE_TYPE'), | ||
| needsSSL: Joi.string().valid('0', '1').label('HD_DATABASE_SSL'), |
There was a problem hiding this comment.
Why isn't this just a boolean (Joi.boolean().default(false))?
| const getTlsConfig = (dbType: DatabaseType, needsSSL: boolean) => { | ||
| if (!needsSSL) return; | ||
|
|
||
| const postgresTlsConfig = dbType === DatabaseType.POSTGRES && { | ||
| ssl: { | ||
| ca: getSecret(process.env.HD_PG_SSL_CA_PATH), | ||
| rejectUnauthorized: process.env.HD_PG_SSL_REJECT_UNAUTHORIZED === 'true', | ||
| key: getSecret(process.env.HD_PG_SSL_KEY_PATH), | ||
| cert: getSecret(process.env.HD_PG_SSL_CERT_PATH), | ||
| }, | ||
| }; | ||
|
|
||
| const sqlTlsConfig = [DatabaseType.MARIADB, DatabaseType.MYSQL].includes( | ||
| dbType, | ||
| ) && { | ||
| ssl: { | ||
| ca: getSecret(process.env.HD_SQL_SSL_CA_PATH), | ||
| key: getSecret(process.env.HD_SQL_SSL_KEY_PATH), | ||
| cert: getSecret(process.env.HD_SQL_SSL_CERT_PATH), | ||
| rejectUnauthorized: process.env.HD_SQL_SSL_REJECT_UNAUTHORIZED === 'true', | ||
| ciphers: process.env.HD_SQL_SSL_CIPHERS, | ||
| maxVersion: process.env.HD_SQL_SSL_MAX_VERSION, | ||
| minVersion: process.env.HD_SQL_SSL_MIN_VERSION, | ||
| passphrase: process.env.HD_SQL_SSL_PASSPHRASE, | ||
| }, | ||
| }; | ||
|
|
||
| return sqlTlsConfig || postgresTlsConfig; | ||
| }; |
There was a problem hiding this comment.
As this method is rather crucial to database security options, it should be unit-tested. The getSecret method could be mocked for that.
| }); | ||
|
|
||
| const getTlsConfig = (dbType: DatabaseType, needsSSL: boolean) => { | ||
| if (!needsSSL) return; |
There was a problem hiding this comment.
| if (!needsSSL) return; | |
| if (!needsSSL) { | |
| return; | |
| } |
Although it may seem unnecessary, please use explicit closures for if statements. By being explicit in all places, we should be avoiding things like apple's gotofail bug.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5530 +/- ##
============================================
+ Coverage 57.66% 87.65% +29.99%
============================================
Files 418 173 -245
Lines 12058 4561 -7497
Branches 1007 492 -515
============================================
- Hits 6953 3998 -2955
+ Misses 5048 538 -4510
+ Partials 57 25 -32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Avinash <avinash.kumar.cs92@gmail.com>
Component/Part
backend -> Database cofig
Description
This PR fixes #3063
Steps
made sure that:
masterfor 1.x & docs,developfor 2.x