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

Streamline SSL experience #677

Merged
merged 13 commits into from
May 18, 2024
Merged

Streamline SSL experience #677

merged 13 commits into from
May 18, 2024

Conversation

josevalim
Copy link
Member

@chrismccord, let me know if this works. For now, this means you need to explicitly set ssl: :verify_full in your config file but we can support "ssl=verify_full" via query string too.

Copy link
Member

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

Just one comment about charlist and this is good to go. I tested it on DigitalOcean and it worked!

Mix.install([
  {:postgrex, github: "elixir-ecto/postgrex", branch: "jv-add-ssl-verify-full"}
])

{:ok, pid} =
  Postgrex.start_link(
    hostname: System.fetch_env!("DO_HOST"),
    port: 25060,
    username: "doadmin",
    password: System.fetch_env!("DO_PASSWORD"),
    database: "defaultdb",
    ssl: :verify_full,
    ssl_opts: [
      cacertfile: "/Users/wojtek/Downloads/ca-certificate.crt"
    ]
  )

IO.inspect(Postgrex.query!(pid, "SELECT NOW()", []))

lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex.ex Outdated Show resolved Hide resolved
@josevalim josevalim changed the title Add ssl: :verify_full Streamline SSL experience May 17, 2024
@josevalim
Copy link
Member Author

@voltone, please let me know if you have thoughts here, the goal is to mirror this in myxql as well.

Copy link

@chrismccord chrismccord left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@voltone voltone left a comment

Choose a reason for hiding this comment

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

There are a few more places in the docs where ssl options are described, including for example the endpoints list under "Failover with SSL support". I don't have time right now to trace the behavior of endpoints and understand the implications to propose updates, sorry. Thought I'd give my feedback now before this gets merged. I might have time tonight...

lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member Author

Thank you @voltone, I believe I have addressed all of your concerns. However, there is one problem, we are always setting server_name_indicator, even in the cases where we would previously set only ssl: true (i.e. when verify: :verify_none). Let me know if this can be a concern because then we can set server_name_indicator: disabled instead.

@josevalim
Copy link
Member Author

However, there is one problem, we are always setting server_name_indicator, even in the cases where we would previously set only ssl: true (i.e. when verify: :verify_none)

From some tests this does not seem to be an issue and, if it happens, folks can always set it to :disabled.

Copy link
Member

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

I tested this on Supabase, CockroachDB, Neon, DigitalOcean PostgreSQL, and PlanetScale (which is MySQL, but same idea) and it works well! I'll work on similar change for MyXQL soon.

@josevalim josevalim merged commit de665e4 into master May 18, 2024
9 checks passed
@josevalim josevalim deleted the jv-add-ssl-verify-full branch May 18, 2024 09:10
@josevalim
Copy link
Member Author

💚 💙 💜 💛 ❤️

@ruslandoga
Copy link
Contributor

ruslandoga commented Aug 21, 2024

👋

It seems like setting ?ssl=true overwrites the keyword options provided in config. E.g.

config :app, Repo,
  url: "ecto://postgresql://your_username:[email protected]:25060/defaultdb?ssl=true",
  ssl: [cacertfile: "/path/to/cacert.pem"]

results in

Postgrex.child_spec([repo: Repo, ssl: true, ...])

and a warning

[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

I wonder if URL parsing logic should be updated to avoid overwriting ssl cacerts?

@josevalim
Copy link
Member Author

Yes, we should probably fix it in Ecto. Maybe we ignore ssl: true if there is already a list in place (and we emit a warning).

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

Successfully merging this pull request may close these issues.

5 participants