Allow the use of SSL connections to the postgres database.

[monotok]

Hello,

fixes #902

Just throwing it out there, I have never coded in typescript before so wasn't exactly sure what I was doing as I code mainly in python/C.

I tweaked the code slightly so allow untrusted SSL connections or no SSL. I tested it with the docker-compose-dev deployment and also changing the environment parameters to point to my crunchydata postgres instance within K8s (which requires SSL). It all seemed to work.

I have made this a draft as I am not sure if this is a good way of doing it and I haven't changed/written any tests for it.

PS. I haven't tested immich yet as I am migrating stuff to K8s and want to deploy it there but it looks very promising; exactly what I have been looking for! Thank you for creating this.

[vercel]

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

Name	Status	Preview	Comments	Updated
immich	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 16, 2023 at 11:33PM (UTC)
immich-code-coverage	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 16, 2023 at 11:33PM (UTC)

[alextran1502]

Hello, can you add the environment variable to docker/.env.example under Optional Database settings:? as a commented-out option?

[jrasm91]

I believe that ssl settings can be included in the connection string itself. I would prefer if the ssl settings were read from that instead of a separate/additional env variable.

Did you look at the package/method mentioned in #902?

[monotok]

Hello, can you add the environment variable to docker/.env.example under Optional Database settings:? as a commented-out option?

Done

[monotok]

I believe that ssl settings can be included in the connection string itself. I would prefer if the ssl settings were read from that instead of a separate/additional env variable.

Did you look at the package/method mentioned in #902?

I did but I am not very familiar with typescript. I was reading this: https://node-postgres.com/features/ssl

If I understand correctly we don't use a connectionstring at the moment.

I couldn't get something like below to work:

let host = process.env.DB_HOSTNAME || 'immich_postgres'
let port = parseInt(process.env.DB_PORT || '5432')
let username = process.env.DB_USERNAME
let password = process.env.DB_PASSWORD
let database = process.env.DB_DATABASE_NAME

let baseDatabaseConfig: PostgresConnectionOptions = {
  type: 'postgres',
  entities: [__dirname + '/../**/*.entity.{js,ts}'],
  synchronize: false,
  migrations: [__dirname + '/../migrations/*.{js,ts}'],
  migrationsRun: true,
  url: `postgres://${username}:${password}@${host}:${port}/${database}?sslmode=prefer`,
};

However I did find a neater way of implementing what I already did. Please see latest commit.

[jrasm91]

That's probably good enough for now and we can refactor to a connection string in another PR.

[monotok]

Okay :) What is the main advantage of a connection string? I would have thought it is a bit more fragile as you are building a string.
If it gets refactored to a connection string then I think it needs to be able to ignore untrusted CA's and add custom CA's.

[jrasm91]

You would not manually construct it like that, but instead let the user provide the whole thing. They aren't restriced by the options we choose to provide values for, etc. So remove all the env variables and use only DB_URL instead.

…

On Thu, Jan 5, 2023, 7:18 PM Hammer ***@***.***> wrote: Okay :) What is the main advantage of a connection string? I would have thought it is a bit more fragile as you are building a string. If it gets refactored to a connection string then I think it needs to be able to ignore untrusted CA's and add custom CA's. — Reply to this email directly, view it on GitHub <#1256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBCE5B7AED6PVGTDD35S5LWQ5QEVANCNFSM6AAAAAATRMVEZY> . You are receiving this because you commented.Message ID: ***@***.***>

[jrasm91]

Look at all these options that would "just work"

https://github.com/brianc/node-postgres/blob/master/packages/pg-connection-string/test/parse.js#L220-L288

[monotok]

That sounds like a better approach 👍

[monotok]

This does work for the local environment however it requires a bit more work to remove all the dependencies on the existing DB env vars. I think that is probably why it doesn't quite work with my external postgres.

import { PostgresConnectionOptions } from 'typeorm/driver/postgres/PostgresConnectionOptions';
import { DataSource } from 'typeorm';

export const databaseConfig: PostgresConnectionOptions = {
  type: 'postgres',
  url: process.env.DB_URL || 'postgres://postgres:postgres@immich_postgres:5432/immich?sslmode=disable',
  entities: [__dirname + '/../**/*.entity.{js,ts}'],
  synchronize: false,
  migrations: [__dirname + '/../migrations/*.{js,ts}'],
  migrationsRun: true,
};

export const dataSource = new DataSource(databaseConfig);

Another PR to refactor would be good.

[bo0tzz]

I think supporting both an URL and the existing env vars would be best, for backwards compatability and maximum versatility. A choice to make there is whether we want to allow both at the same time - in the library we're using, any options that are set explicitly will override those from the connection URL. I think there are two options there:

1. If URL is set, use only that and ignore all the other env vars
2. Only use the env vars that are explicitly set, and remove the default hardcoded host and port (otherwise they'll override the URL)

Option 1 would be the easiest, and I expect it'd be fine to ignore the other env vars, at the cost of a little bit of flexibility.

fwiw: I think removing the hardcoded defaults is probably a good idea either way. The default host is already in the standard .env, and I believe port defaults to 5432 internally if we don't set anything.

[monotok]

@bo0tzz I have got the URL string working now so can implement option 1 as a connection string seems to be the preferred way.

[monotok]

I have now changed it to use a DB URL.

A few questions:

server/libs/common/src/config/app.config.ts

The validation still looks for these env variables even if we set the DB_URL and they are ignored. I have just added a warning not to remove them in the env.example file.

    DB_USERNAME: Joi.string().required(),
    DB_PASSWORD: Joi.string().required(),
    DB_DATABASE_NAME: Joi.string().required(),

The env variables are still used by the docker container in the docker-compose file to set the passwords etc. If you are setting the DB_URL anyway you are unlikely to be using that container anyway. Just wanted to flag it.

I think this is ready now.

[jrasm91]

Looks better. We can make the validation conditional. So connection string OR the other ones.

Also, the config merge can probably be simplified a bit by still having one exported base option that merges in a second one that's set using a ternary.

So like

const url = process.env.DB_URL;
const connection = url ? ({ url }) : ({ password: process.env.DB_PASSWORD, etc. })

export const dbConfig = {
  Original options,
  ...connection,

[jrasm91]

Here's an example of dynamic validation:

immich/server/libs/common/src/config/app.config.ts

Lines 19 to 45 in d3c35ec

	const WHEN_OAUTH_ENABLED = Joi.when('OAUTH_ENABLED', {
	is: true,
	then: Joi.string().required(),
	otherwise: Joi.string().optional(),
	});
	export const immichAppConfig: ConfigModuleOptions = {
	envFilePath: '.env',
	isGlobal: true,
	validationSchema: Joi.object({
	NODE_ENV: Joi.string().required().valid('development', 'production', 'staging').default('development'),
	DB_USERNAME: Joi.string().required(),
	DB_PASSWORD: Joi.string().required(),
	DB_DATABASE_NAME: Joi.string().required(),
	JWT_SECRET: Joi.string().required().custom(jwtSecretValidator),
	DISABLE_REVERSE_GEOCODING: Joi.boolean().optional().valid(true, false).default(false),
	REVERSE_GEOCODING_PRECISION: Joi.number().optional().valid(0, 1, 2, 3).default(3),
	LOG_LEVEL: Joi.string().optional().valid('simple', 'verbose').default('simple'),
	OAUTH_ENABLED: Joi.bool().valid(true, false).default(false),
	OAUTH_BUTTON_TEXT: Joi.string().optional().default('Login with OAuth'),
	OAUTH_AUTO_REGISTER: Joi.bool().valid(true, false).default(true),
	OAUTH_ISSUER_URL: WHEN_OAUTH_ENABLED,
	OAUTH_SCOPE: Joi.string().optional().default('openid email profile'),
	OAUTH_CLIENT_ID: WHEN_OAUTH_ENABLED,
	OAUTH_CLIENT_SECRET: WHEN_OAUTH_ENABLED,
	}),
	};

[bo0tzz]

Just a few small nits from me. Thank you :)

[monotok]

Just a few small nits from me. Thank you :)

Should all be resolved now. Where do we change the documentation for the new DB_URL?

[jrasm91]

It doesn't look like we have a dedicated page for environment variables yet, just the env file copy/pasted here:

https://immich.app/docs/install/docker-compose

[jrasm91]

Looks good, thanks!

[alextran1502]

Can you please help fix the unit test

Thank you

[monotok]

Unfortunately I can't get my build environment working properly so not able to run tests locally etc. I must be missing something. Is there any setup instructions to get everything installed etc?

[bo0tzz]

https://immich.app/docs/developer/setup should help

[monotok]

Thanks for the link but I have been looking at that. Make dev works fine. I am struggling with getting npm install to work.
To install the packages there is a 'package.json' file in the server directory however npm install only works in the top level checkout, not within the server directory.

1145 info run bcrypt@5.0.1 install node_modules/bcrypt node-pre-gyp install --fallback-to-build
1146 info run diskusage@1.1.3 install node_modules/diskusage node-gyp rebuild
1147 info run msgpackr-extract@1.0.16 install node_modules/msgpackr-extract node-gyp-build
1148 info run sharp@0.28.3 install node_modules/sharp (node install/libvips && node install/dll-copy && prebuild-install) || (node install/can-compile && node-gyp rebuild && node install/dll-copy)
1149 info run bcrypt@5.0.1 install { code: 1, signal: null }
1150 info run diskusage@1.1.3 install { code: 1, signal: null }
1151 info run msgpackr-extract@1.0.16 install { code: 0, signal: null }
1152 timing build:run:install:node_modules/msgpackr-extract Completed in 418ms
1153 info run sharp@0.28.3 install { code: 1, signal: null }
1154 timing reify:rollback:createSparse Completed in 2240ms
1155 timing reify:rollback:retireShallow Completed in 29ms
1156 timing command:install Completed in 15561ms
1157 verbose stack Error: command failed
1157 verbose stack     at ChildProcess.<anonymous> (/snap/node/6895/lib/node_modules/npm/node_modules/@npmcli/promise-spawn/lib/index.js:63:27)
1157 verbose stack     at ChildProcess.emit (node:events:513:28)
1157 verbose stack     at maybeClose (node:internal/child_process:1100:16)
1157 verbose stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)
1158 verbose pkgid bcrypt@5.0.1
1159 verbose cwd /home/hammer/SoftwareDevelopment/Projects/immich/server
1160 verbose Linux 5.15.0-57-generic
1161 verbose node v16.18.1
1162 verbose npm  v8.19.2
1163 error code 1
1164 error path /home/hammer/SoftwareDevelopment/Projects/immich/server/node_modules/bcrypt
1165 error command failed
1166 error command sh -c -- node-pre-gyp install --fallback-to-build
1167 verbose exit 1

[jrasm91]

You need to run the command on the docker container itself. So something like:

docker exec -it immich_server bash

Then npm install.

Similar to https://immich.app/docs/features/server-commands#examples

[monotok]

You need to run the command on the docker container itself. So something like:

docker exec -it immich_server bash

Then npm install.

Similar to https://immich.app/docs/features/server-commands#examples

Right okay; doing that works fine. I just thought that was for testing the application manually in the browser etc.

All tests etc pass so fingers crossed it is okay now. Just rebased the fork as it was getting redis errors (from previous rebase) and it is clean.

[monotok]

Is the docker login something I have missed?

[bo0tzz]

Is the docker login something I have missed?

Nope, no worries.

@alextran1502 can you take another look at this?