-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add configuration for SSL database connection #140
Conversation
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.
Great job! I just wrote some little (and opinionated) comments 😄
@@ -10,4 +11,23 @@ export const dataSourceOptions: DataSourceOptions = { | |||
url: process.env.MIGRATIONS_DATABASE_URL, | |||
entities: [Webhook], | |||
migrations: [__dirname + '/../migrations/**/*{.ts,.js}'], | |||
}; | |||
... (process.env.DB_SSL_ENABLE == 'true' ? { |
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.
I think the spread operator is not strictly required here. If I'm not wrong, the expression can be simplified:
ssl: process.env.DB_SSL_ENABLE == 'true',
extra: process.env.DB_CA_PATH
? {
ssl: {
ca: readFileSync(process.env.DB_CA_PATH),
rejectUnauthorized: true,
},
}
: {
ssl: {
rejectUnauthorized: false,
},
},
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.
We can remove extra option and just keep ssl with the configuration.
https://typeorm.io/data-source-options#postgres--cockroachdb-data-source-options
155ca44
to
6d4629f
Compare
.env.sample
Outdated
@@ -7,4 +7,6 @@ ADMIN_PASSWORD=password | |||
WEBHOOKS_CACHE_TTL=300000 | |||
NODE_ENV=dev | |||
SSE_AUTH_TOKEN=aW5mcmFAc2FmZS5nbG9iYWw6YWJjMTIz | |||
DB_SSL_ENABLE=false |
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.
I would use 0 or 1 instead of a string like false
.
Also we should use the same naming, as the original variable is DATABASE_URL
this should be DATABASE_SSL_ENABLED
, same for the CA path
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.
@Uxio0 Done
closes #132
Description
This PR adds the needed configuration to enable SSL connection with database and optional configuration of database certificates.
New environment vars
DB_SSL_ENABLE
if is unset or false SSL won't be used but if true then the connection will use SSL.DB_CA_PATH
path of database certificate, when is configured the connection will validate the database certificate during the connection.