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(server): sslmode not working #15587

Merged
merged 9 commits into from
Jan 24, 2025
Merged

fix(server): sslmode not working #15587

merged 9 commits into from
Jan 24, 2025

Conversation

mertalev
Copy link
Contributor

Description

Postgres.js has its own connection parsing logic that is not as robust as the previous driver. This PR uses the pg-connection-string library to do the parsing and passes the results to the Postgres.js driver.

Fixes #15566

@mertalev mertalev force-pushed the fix/server-pg-sslmode branch from 5773454 to 9f7399a Compare January 24, 2025 06:45
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Please write some tests for this

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Looks good. Can you add an error for an invalid ssl value as well?

@mertalev
Copy link
Contributor Author

Looks good. Can you add an error for an invalid ssl value as well?

There's an error for it already

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 24, 2025

Looks good. Can you add an error for an invalid ssl value as well?

There's an error for it already

Sorry, I meant add a test for the error case.

@mertalev
Copy link
Contributor Author

Oh, there is a test for it at the bottom

@alextran1502 alextran1502 merged commit ba01b40 into main Jan 24, 2025
36 checks passed
@alextran1502 alextran1502 deleted the fix/server-pg-sslmode branch January 24, 2025 19:01
@dotlambda
Copy link
Contributor

Afaict this doesn't deal with sockets, the option path is never set.

@mertalev
Copy link
Contributor Author

Afaict this doesn't deal with sockets, the option path is never set.

pg-connection-stringdoes support sockets, see this code.

@dotlambda
Copy link
Contributor

dotlambda commented Jan 24, 2025

I know, but it never sets path:

> parse('socket:/run/postgresql?db=immich')
{
  db: 'immich',
  user: '',
  password: '',
  host: '/run/postgresql',
  database: 'immich',
  client_encoding: null
}

What I was missing though is that the postgres library checks for a slash in host: https://github.com/porsager/postgres/blob/089214e85c23c90cf142d47fb30bd03f42874984/src/index.js#L466
Do you mind if I add two test cases for socket connections?

@mertalev
Copy link
Contributor Author

I know, but it never sets path:

> parse('socket:/run/postgresql?db=immich')
{
  db: 'immich',
  user: '',
  password: '',
  host: '/run/postgresql',
  database: 'immich',
  client_encoding: null
}

What I was missing though is that the postgres library checks for a slash in host: porsager/postgres@089214e/src/index.js#L466 Do you mind if I add two test cases for socket connections?

Go for it! I'm also happy to review any changes needed to support sockets if they still don't work.

@voc0der
Copy link

voc0der commented Jan 24, 2025

Where is it expecting your ssl certificates ? (fullchain / and or cert + pem / privkey) to be?

Mine fails when I set verify-ca for the obvious reason that I have not figured out where to mount the cert?

@mmomjian
Copy link
Contributor

Where is it expecting your ssl certificates ? (fullchain / and or cert + pem / privkey) to be?

Mine fails when I set verify-ca for the obvious reason that I have not figured out where to mount the cert?

I don't think that we have ever supported verification even before kysely.

@voc0der
Copy link

voc0der commented Jan 24, 2025

Where is it expecting your ssl certificates ? (fullchain / and or cert + pem / privkey) to be?
Mine fails when I set verify-ca for the obvious reason that I have not figured out where to mount the cert?

I don't think that we have ever supported verification even before kysely.

Good to know I'm not crazy. In the docs, it's a bit ambiguous, made me feel like I was doing it wrong.

DB_URL must be in the format postgresql://immichdbusername:immichdbpassword@postgreshost:postgresport/immichdatabasename. You can require SSL by adding ?sslmode=require to the end of the DB_URL string, or require SSL and skip certificate verification by adding ?sslmode=require&sslmode=no-verify.

@mertalev
Copy link
Contributor Author

mertalev commented Jan 24, 2025

I believe you have to set sslcert, sslkey, and/or sslrootcert to the file path within the container. The library reads the files at these paths when parsing if specified.

@voc0der
Copy link

voc0der commented Jan 24, 2025

I believe you have to set sslcert, sslkey, and/or sslrootcert to the file path within the container. The library reads the files at these paths when parsing if specified.

Environment variables which correspond to the mounting? That sounds like the solution, although I'm not seeing it when I search the code in github so I'm still skeptical?

@mertalev
Copy link
Contributor Author

I believe you have to set sslcert, sslkey, and/or sslrootcert to the file path within the container. The library reads the files at these paths when parsing if specified.

Environment variables which correspond to the mounting? That sounds like the solution, although I'm not seeing it when I search the code in github so I'm still skeptical?

No, these are query parameters in the DB_URL

@voc0der
Copy link

voc0der commented Jan 24, 2025

That makes sense. And thanks, that fixed my issue on top of this.

vladd11 pushed a commit to vladd11/immich that referenced this pull request Jan 25, 2025
* parse db url before passing it to the driver

* don't be lazy

* simplify

* simplify

* add tests

* update sql sync script

* update mock

* remove unused import

* remove unused imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.125.1: Postgres sslmode not working anymore
6 participants