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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5a3c1bf
Add ssl: :verify_full
josevalim May 17, 2024
0437449
Update lib/postgrex/protocol.ex
josevalim May 17, 2024
8d2ca0c
Update postgrex.ex
josevalim May 17, 2024
b850f96
Update lib/postgrex.ex
josevalim May 17, 2024
d697e78
Update postgrex.ex
josevalim May 17, 2024
dc832ba
Streamline :ssl
josevalim May 17, 2024
8a9d6d1
Use Logger.warning
josevalim May 17, 2024
112161f
Update lib/postgrex.ex
josevalim May 17, 2024
80f748c
Update lib/postgrex/protocol.ex
josevalim May 17, 2024
325a7e2
Always set server_name_indicator based on hostname
josevalim May 17, 2024
1dffd59
More fixes
josevalim May 17, 2024
08604bd
Fix
josevalim May 17, 2024
bd498b6
Remove deprecation warnings
josevalim May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions lib/postgrex.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ defmodule Postgrex do
| {:connect_timeout, timeout}
| {:handshake_timeout, timeout}
| {:ping_timeout, timeout}
| {:ssl, boolean}
| {:ssl_opts, [:ssl.tls_client_option()]}
| {:ssl, boolean | [:ssl.tls_client_option()]}
| {:socket_options, [:gen_tcp.connect_option()]}
| {:prepare, :named | :unnamed}
| {:transactions, :strict | :naive}
Expand Down Expand Up @@ -118,11 +117,10 @@ defmodule Postgrex do
* `:idle_interval` - Ping connections after a period of inactivity in milliseconds.
Defaults to 1000ms;

* `:ssl` - Set to `true` if ssl should be used (default: `false`);

* `:ssl_opts` - A list of ssl options, see the
[`tls_client_option`](http://erlang.org/doc/man/ssl.html#type-tls_client_option)
from the ssl docs;
* `:ssl` - Enables SSL. Setting it to `true` enables SSL without host verification,
which emits a warning. Instead, prefer to set it to a keyword list, with either
`:cacerts` or `:cacertfile` pointing to the server certificate, to enable hostname
verification. Defaults to `false`;
josevalim marked this conversation as resolved.
Show resolved Hide resolved

* `:socket_options` - Options to be given to the underlying socket
(applies to both TCP and UNIX sockets);
Expand Down
50 changes: 40 additions & 10 deletions lib/postgrex/protocol.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,29 @@ defmodule Postgrex.Protocol do
timeout = opts[:timeout] || @timeout
ping_timeout = Keyword.get(opts, :ping_timeout, timeout)
sock_opts = [send_timeout: timeout] ++ (opts[:socket_options] || [])
ssl? = opts[:ssl] || false
types_mod = Keyword.fetch!(opts, :types)
disconnect_on_error_codes = opts[:disconnect_on_error_codes] || []
target_server_type = opts[:target_server_type] || :any
disable_composite_types = opts[:disable_composite_types] || false

{ssl_opts, opts} =
case Keyword.pop(opts, :ssl, false) do
{false, opts} ->
{nil, opts}

{true, opts} ->
Logger.warning(
"setting ssl: true on your database connection offers only limited protection, " <>
"as the hostname is not verified. Set \"ssl: [cacertfile: path/to/file]\" instead"
josevalim marked this conversation as resolved.
Show resolved Hide resolved
)

# Read ssl_opts for backwards compatibility
Keyword.pop(opts, :ssl_opts, [])
Copy link

@ruslandoga ruslandoga Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

👋

Sorry for posting on an old PR, but shouldn't the default here be verify: :verify_none (according to the warning)?

Keyword.pop(opts, :ssl_opts, verify: :verify_none)

It seems like -- by default -- :ssl still tries to verify the peer.

iex> :ssl.start()
#==> :ok

iex> :ssl.connect(~c"google.com", 443, [])
#==> {:error, {:options, :incompatible, [verify: :verify_peer, cacerts: :undefined]}}

iex> :ssl.connect(~c"google.com", 443, verify: :verify_none)
#==> {:ok,
#==>  {:sslsocket, {:gen_tcp, #Port<0.5>, :tls_connection, :undefined},
#==>   [#PID<0.139.0>, #PID<0.138.0>]}}

Copy link
Member

@wojtekmach wojtekmach Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think previous OTP defaulted to :verify_none and more recent to :verify_peer and so I believe the idea was for this legacy ssl: true to keep whatever that particular OTP defaulted to, however ssl: list would force secure defaults.

Copy link

@ruslandoga ruslandoga Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, but the warning kind of implies that no verification happens anyway when ssl: true:

Logger.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"
)

So, for the warning message to be true, setting verify: :verify_none seems safe (i.e. no surprises to the users).

Right now, with the current :ssl and :ssl_opts defaults it just doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this change broke any apps? Cause if the warning implies less security we should change the warning not intentionally make things less secure. Sorry I don't have an easy way to check how it all works right now.

Copy link

@ruslandoga ruslandoga Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, I don't think it broke any apps. My suggestion was purely a (questionable) QoL improvement :)

Also it seems like right now the warning might be a false positive in this scenario:

config :app, Repo,
  ssl: true,
  ssl_opts: [cacertfile: "..."]

Maybe some other wording could be used to highlight that:

  • ssl_opts is deprecated and should be replaced with ssl: list
  • just setting ssl: true doesn't work out-of-the-box and ssl: [verify: :verify_none] can be used to replicate ?sslmode=require
  • ssl: [cacertfile: ...] is the actual suggested approach


{ssl_opts, opts} when is_list(ssl_opts) ->
{Keyword.merge(default_ssl_opts(opts), ssl_opts), opts}
end

transactions =
case opts[:transactions] || :naive do
:naive -> :naive
Expand Down Expand Up @@ -117,14 +134,27 @@ defmodule Postgrex.Protocol do
types_lock: nil,
prepare: prepare,
messages: [],
ssl: ssl?,
ssl: ssl_opts,
target_server_type: target_server_type,
search_path: opts[:search_path]
}

connect_endpoints(endpoints, sock_opts ++ @sock_opts, connect_timeout, s, status, [])
end

defp default_ssl_opts(opts) do
case Keyword.fetch(opts, :hostname) do
josevalim marked this conversation as resolved.
Show resolved Hide resolved
{:ok, hostname} -> [server_name_indication: String.to_charlist(hostname)]
:error -> []
end ++
[
verify: :verify_peer,
customize_hostname_check: [
match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
]
]
end

defp endpoints(opts) do
port = opts[:port] || 5432

Expand Down Expand Up @@ -733,22 +763,22 @@ defmodule Postgrex.Protocol do
:ok
end

defp do_handshake(s, %{ssl: true} = status), do: ssl(s, status)
defp do_handshake(s, %{ssl: false} = status), do: startup(s, status)
defp do_handshake(s, %{ssl: nil} = status), do: startup(s, status)
defp do_handshake(s, %{ssl: ssl_opts} = status), do: ssl(s, status, ssl_opts)

## ssl

defp ssl(s, status) do
defp ssl(s, status, ssl_opts) do
case msg_send(s, msg_ssl_request(), "") do
:ok -> ssl_recv(s, status)
:ok -> ssl_recv(s, status, ssl_opts)
{:disconnect, _, _} = dis -> dis
end
end

defp ssl_recv(%{sock: {:gen_tcp, sock}} = s, status) do
defp ssl_recv(%{sock: {:gen_tcp, sock}} = s, status, ssl_opts) do
case :gen_tcp.recv(sock, 1, :infinity) do
{:ok, <<?S>>} ->
ssl_connect(s, status)
ssl_connect(s, status, ssl_opts)

{:ok, <<?N>>} ->
disconnect(s, %Postgrex.Error{message: "ssl not available"}, "")
Expand All @@ -770,8 +800,8 @@ defmodule Postgrex.Protocol do
end
end

defp ssl_connect(%{sock: {:gen_tcp, sock}, timeout: timeout} = s, status) do
case :ssl.connect(sock, status.opts[:ssl_opts] || [], timeout) do
defp ssl_connect(%{sock: {:gen_tcp, sock}, timeout: timeout} = s, status, ssl_opts) do
case :ssl.connect(sock, ssl_opts, timeout) do
{:ok, ssl_sock} ->
startup(%{s | sock: {:ssl, ssl_sock}}, status)

Expand Down
Loading