elixir-ecto / postgrex

PostgreSQL driver for Elixir
http://hexdocs.pm/postgrex/
1.1k stars 270 forks source link

Set "ssl: [cacertfile: path/to/file]" instead #681

Closed dar-datsystems closed 2 months ago

dar-datsystems commented 2 months ago

Elixir version

1.16.3

Database and Version

PostrgeSQL 16

Postgrex Version

0.18.0

Current behavior

I am conecting to Neon with SSL and have 1 master and 1 read replica

for {repo, hostname} <- replicas do
    config :maverick, repo,
      username: System.get_env("NEON_DB_USER") || "postgres",
      password: System.get_env("NEON_DB_PASSWORD") || "postgres",
      hostname: hostname || "localhost",
      database: System.get_env("NEON_DB") || "maverick_dev",
      stacktrace: true,
      show_sensitive_data_on_connection_error: true,
      pool_size: 20,
      queue_target: 45_000,
      timeout: 60_000,
      connect_timeout: 60_000,
      ownership_timeout: 60_000,
      prepare: :unnamed,
      ssl: true,
      ssl_opts: [
        server_name_indication: String.to_charlist(hostname) || ~c"localhost",
        cacertfile: CAStore.file_path(),
        verify_type: :verify_peer,
        customize_hostname_check: [
          match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
        ]
      ],
      socket_options: maybe_ipv6
  end

I keep seeing below warning in the logs.

setting
 ssl: true on your database connection offers only limited protection, 
as the server's certificate is not verified. Set \"ssl: [cacertfile: 
path/to/file]\" instead" pid=#PID<0.3344.0> application=postgrex 
mfa=Postgrex.Protocol.connect/1

Expected behavior

As per the ecto postgres adapter docs the value for ssl is true or false and all options are to be passed in ssl_opts. Is there any change which is not reflected in the docs ?

dar-datsystems commented 2 months ago

found the change https://github.com/elixir-ecto/postgrex/blob/cb9c213e5913f884e68b9690e3ac462999b5bb29/CHANGELOG.md?plain=1#L6

docs should reflect that https://hexdocs.pm/ecto_sql/Ecto.Adapters.Postgres.html

wojtekmach commented 2 months ago

We've updated Neon docs, https://neon.tech/docs/guides/elixir-ecto#configure-ecto, but forgot to update Ecto.SQL. A PR would be appreciated!

mrcnkoba commented 2 months ago

This is a breaking change, not a deprecation though.

josevalim commented 2 months ago

How is it a breaking change? Passing ssl_opts should still work the same as before?

mrcnkoba commented 2 months ago

Hi @josevalim. I guess it could be a combination of our settings with the deprecation. Our Backend just fails to start. We get the aforementioned warning and this error message:

WARNING setting ssl: true on your database connection offers only limited protection, as the server's certificate is not verified. Set "ssl: [cacertfile: path/to/file]" instead
ERROR Postgrex.Protocol (#PID<0.168.0>) failed to connect: ** (DBConnection.ConnectionError) ssl connect: TLS client: In state wait_cert at ssl_handshake.erl:2135 generated CLIENT ALERT: Fatal - Handshake Failure
 {bad_cert,hostname_check_failed} - {:tls_alert, {:handshake_failure, ~c"TLS client: In state wait_cert at ssl_handshake.erl:2135 generated CLIENT ALERT: Fatal - Handshake Failure\n {bad_cert,hostname_check_failed}"}}

Our config taken from: OurApp.Repo.config() is as follows:

[
  ssl_opts: [
    verify: :verify_peer,
    cacertfile: ~c"/app/lib/backend-0.0.1/priv/certificates/ca_cert.pem"
  ],
  timeout: 15000,
  hostname: <REDACTED>,
  port: <REDACTED>,
  migration_port: <REDACTED>,
  username: <REDACTED>,
  password: <REDACTED>,
  database: <REDACTED>,
  pool_size: 10,
  ssl: true,
  ssl_mode: :"verify-full",
  migration_timestamps: [type: :naive_datetime_usec],
  queue_target: 1000,
  queue_interval: 6000,
  idle_interval: 10000,
  prepare: :named,
  migration_lock: :pg_advisory_lock
]
josevalim commented 2 months ago

Did you also update the OTP version?

The only possible failure I can think of is that we are now inserting the server_name_indication: https://github.com/elixir-ecto/postgrex/commit/de665e40e34fc1a0e88b14c09ed0912ec477cf68

Perhaps you could add server_name_indication: :disabled to your ssl_opts? But I would also try to leave it as is and make it work with the server name indication for extra security.

mrcnkoba commented 2 months ago

@josevalim We are still on OTP 25. Ecto update didn't correspond to the update of the OTP.

bugnano commented 2 months ago

It was the server name indication indeed. Setting server_name_indication: :disable in the ssl options fixed the issue. I think this is a breaking change.

josevalim commented 2 months ago

You are correct. This is still pre-1.0, so it is still fine (this and support for Elixir duration are the pending changes before 1.0) but we need update the CHANGELOG appropriately.