feat: use simple_connection for auth_query #342
Conversation
| @@ -88,7 +88,11 @@ defmodule Supavisor.Helpers do | |||
| @spec get_user_secret(pid(), String.t(), String.t()) :: {:ok, map()} | {:error, String.t()} | |||
| def get_user_secret(conn, auth_query, user) do | |||
| try do | |||
| Postgrex.query!(conn, auth_query, [user]) | |||
| # sanitize the user input by removing all characters that are not alphanumeric or underscores | |||
| user = String.replace(user, ~r/[^a-zA-Z0-9_]/, "") | |||
There was a problem hiding this comment.
I think that instead we should reject user if these characters are present instead of just pretending that these do not exists.
There was a problem hiding this comment.
I already have a check for invalid characters during connection, but it only checks for slashes. I added this sanitization as an additional guard. It might be worth applying the same rule (a-zA-Z0-9_) during connection as well
supavisor/lib/supavisor/client_handler.ex
Lines 157 to 164 in 61b629d
$ psql postgresql://p\"ostgres.tenant:password@localhost:5432/postgres
psql: error: connection to server at "localhost" (::1), port 5432 failed: Connection refused
Is the server running on that host and accepting TCP/IP connections?
connection to server at "localhost" (127.0.0.1), port 5432 failed: FATAL: Authentication error, reason: "Invalid characters in user or db_name"
| assert Enum.filter(Process.list(), fn pid -> | ||
| Process.info(pid)[:dictionary][:auth_host] == db_conf[:hostname] | ||
| end) == [] |
There was a problem hiding this comment.
| assert Enum.filter(Process.list(), fn pid -> | |
| Process.info(pid)[:dictionary][:auth_host] == db_conf[:hostname] | |
| end) == [] | |
| assert Enum.all?(Process.list(), fn pid -> | |
| Process.info(pid)[:dictionary][:auth_host] == db_conf[:hostname] | |
| end) |
There was a problem hiding this comment.
For what is worth, using filter for this test has a tiny benefit that it shows the left side with the failed PIDs in exception reports. :)
There was a problem hiding this comment.
True. That indeed may be useful.
| @@ -88,7 +88,11 @@ defmodule Supavisor.Helpers do | |||
| @spec get_user_secret(pid(), String.t(), String.t()) :: {:ok, map()} | {:error, String.t()} | |||
| def get_user_secret(conn, auth_query, user) do | |||
| try do | |||
| Postgrex.query!(conn, auth_query, [user]) | |||
| # sanitize the user input by removing all characters that are not alphanumeric or underscores | |||
| user = String.replace(user, ~r/[^a-zA-Z0-9_]/, "") | |||
There was a problem hiding this comment.
I think that instead we should reject user if these characters are present instead of just pretending that these do not exists.
The PID of the Postgrex connection can be lost when ClientHandler terminates in the middle of getting secrets via auth_query. As a result, we may get zombie connections, which can consume all allowed connections to the tenant's database. With this PR, auth_query will be done with SimpleConnection, where the process crashes automatically if the caller (ClientHandler) goes down.