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

fix: handle SSL errors in client handler #551

Merged
merged 1 commit into from
Jan 20, 2025
+31 −1
Merged

fix: handle SSL errors in client handler #551

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion VERSION
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2.0.15
2.0.16
5 changes: 5 additions & 0 deletions lib/supavisor/client_handler.ex
View file
Original file line number Diff line number Diff line change
@@ -334,7 +334,7 @@
Cachex.get(Supavisor.Cache, key) == {:ok, nil} do
case auth_secrets(info, data.user, key, 15_000) do
{:ok, {method2, secrets2}} = value ->
if method != method2 or Map.delete(secrets.(), :client_key) != secrets2.() do

Check warning on line 337 in lib/supavisor/client_handler.ex

GitHub Actions / Code style 

Function body is nested too deep (max depth is 2, was 4).
Logger.warning("ClientHandler: Update secrets and terminate pool")

Cachex.update(
@@ -531,6 +531,11 @@
def handle_event(:info, {:parameter_status, ps}, :exchange, _),
do: {:keep_state_and_data, {:next_event, :internal, {:greetings, ps}}}

def handle_event(:info, {:ssl_error, sock, reason}, _, %{sock: {_, sock}}) do
Logger.error("ClientHandler: SSL error #{inspect(reason)}")
{:stop, {:shutdown, :ssl_error}}
end

# client closed connection
def handle_event(_, {closed, _}, _, data)
when closed in [:tcp_closed, :ssl_closed] do
@@ -574,7 +579,7 @@
db_pid = handle_db_pid(data.mode, data.pool, data.db_pid)

{_, stats} =
if not data.local,

Check warning on line 582 in lib/supavisor/client_handler.ex

GitHub Actions / Code style 

Avoid negated conditions in if-else blocks.
do: Telem.network_usage(:client, data.sock, data.id, data.stats),
else: {nil, data.stats}

25 changes: 25 additions & 0 deletions test/supavisor/client_handler_test.exs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,28 @@
defmodule Supavisor.ClientHandlerTest do
use ExUnit.Case, async: true
alias Supavisor.ClientHandler

test "handle ssl_error" do
sock =
{:sslsocket,
{
:gen_tcp,
:some_port,
:tls_connection,
[session_id_tracker: :some_pid]
}, [:some_pid]}

error =
{:ssl_error, sock,
{
:tls_alert,
{:user_canceled,
~c"TLS server: In state connection received CLIENT ALERT: Fatal - User Canceled\n"}
}}

data = %{sock: {:ssl, sock}}

assert {:stop, {:shutdown, :ssl_error}} =
ClientHandler.handle_event(:info, error, nil, data)
end
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is the proper way to test it. I do not even think that it is path that should be tested at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client handler has many cases, and with this test, we can ensure that the function returns the proper response to incoming messages.

Copy link
Member

Choose a reason for hiding this comment

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

I still think it is improper way to handle it. I will probably merge it, but I think that this test should be scrapped in favour of doing real TLS connection that is killed before finishing handshake or killing the connection without proper exit messages.

end

Unchanged files with check annotations Beta

View file
test/support/e2e_case.ex
defmodule Supavisor.E2ECase do

Check warning on line 1 in test/support/e2e_case.ex

GitHub Actions / Code style 

Modules should have a @moduledoc tag.
use ExUnit.CaseTemplate
@repo Supavisor.Repo
end
def unboxed(fun) do
Ecto.Adapters.SQL.Sandbox.unboxed_run(@repo, fun)

Check warning on line 23 in test/support/e2e_case.ex

GitHub Actions / Code style 

Nested modules could be aliased at the top of the invoking module.
end
def create_instance(external_id) do
View file
lib/supavisor/helpers.ex
@spec validate_name(String.t()) :: boolean()
def validate_name(name) do
# 1-63 characters, starting with a lowercase letter or underscore, and containing only alphanumeric characters, underscores, dollar signs, and hyphens. Names with spaces or uppercase letters must be enclosed in double quotes.

Check warning on line 374 in lib/supavisor/helpers.ex

GitHub Actions / Code style 

Line is too long (max is 120, was 229).
String.length(name) <= 63 and
name =~ ~r/^(?:[a-z_][a-z0-9_$\- ]*|"[a-zA-Z0-9_$\- ]+")$/ and
name != ~s/""/
View file
lib/supavisor/protocol/server.ex
def application_name, do: @application_name
@spec terminate_message() :: binary
def terminate_message(), do: @terminate_message

Check warning on line 477 in lib/supavisor/protocol/server.ex

GitHub Actions / Code style 

Do not use parentheses when defining a function which has no arguments.
end
View file
lib/supavisor/db_handler.ex
tenant = if data.proxy, do: Supavisor.tenant(data.id)
search_path = Supavisor.search_path(data.id)
case send_startup(sock, auth, tenant, search_path) do

Check warning on line 129 in lib/supavisor/db_handler.ex

GitHub Actions / Code style 

Function body is nested too deep (max depth is 2, was 3).
:ok ->
:ok = activate(sock)
{:next_state, :authentication, %{data | sock: sock}}
HandlerHelpers.activate(data.sock)
{_, stats} =
if not data.proxy,

Check warning on line 302 in lib/supavisor/db_handler.ex

GitHub Actions / Code style 

Avoid negated conditions in if-else blocks.
do: Telem.network_usage(:db, data.sock, data.id, data.stats),
else: {nil, data.stats}
HandlerHelpers.sock_send(data.client_sock, bin)
{_, client_stats} =
if not data.proxy,

Check warning on line 316 in lib/supavisor/db_handler.ex

GitHub Actions / Code style 

Avoid negated conditions in if-else blocks.

Check warning on line 316 in lib/supavisor/db_handler.ex

GitHub Actions / Code style 

Function body is nested too deep (max depth is 2, was 3).
do: Telem.network_usage(:client, data.client_sock, data.id, data.client_stats),
else: {nil, data.client_stats}