From d1927d34b18354dd3026e75e9bc757c80b54b076 Mon Sep 17 00:00:00 2001 From: Rodolfo Silva Date: Wed, 3 Dec 2025 08:38:48 -0300 Subject: [PATCH 1/6] chore: replace FTP server implementation with ThousandIsland --- config/test.exs | 8 +- docker-compose.yml | 17 +++ lib/ex_ftp/application.ex | 3 +- lib/ex_ftp/ftp_common.ex | 2 +- lib/ex_ftp/passive_socket.ex | 2 + lib/ex_ftp/worker.ex | 282 +++++++++++++++++------------------ mix.exs | 3 +- mix.lock | 1 + test/ex_ftp/server_test.exs | 19 --- test/test_helper.exs | 6 +- 10 files changed, 173 insertions(+), 170 deletions(-) create mode 100644 docker-compose.yml delete mode 100644 test/ex_ftp/server_test.exs diff --git a/config/test.exs b/config/test.exs index b7fd5b7..193dd7b 100644 --- a/config/test.exs +++ b/config/test.exs @@ -7,9 +7,9 @@ config :ex_aws, s3: [ scheme: System.get_env("AWS_SCHEME", "http://"), host: System.get_env("AWS_HOST", "localhost"), - port: "AWS_PORT" |> System.get_env("4566") |> String.to_integer(), - access_key_id: "", - secret_access_key: "" + port: "AWS_PORT" |> System.get_env("9222") |> String.to_integer(), + access_key_id: "minio", + secret_access_key: "minio123456" ] config :ex_ftp, @@ -21,7 +21,7 @@ config :ex_ftp, authenticator_config: %{ authenticated_url: nil, authenticated_method: :get, - authenticated_ttl_ms: 24 * 60 * 60 * 60 * 1000, + authenticated_ttl_ms: to_timeout(day: 1), login_url: nil, login_method: :get }, diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 0000000..4fa7001 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,17 @@ +services: + minio: + image: minio/minio:RELEASE.2023-12-07T04-16-00Z + restart: unless-stopped + ports: + - 9222:9222 + - 9333:9333 + volumes: + - minio-data:/data + environment: + MINIO_ROOT_USER: minio + MINIO_ROOT_PASSWORD: minio123456 + entrypoint: sh + command: "-c 'mkdir -p /data/ex-ftp-test && minio server --address :9222 --console-address :9333 /data'" + +volumes: + minio-data: {} diff --git a/lib/ex_ftp/application.ex b/lib/ex_ftp/application.ex index 82b98ad..79f4366 100644 --- a/lib/ex_ftp/application.ex +++ b/lib/ex_ftp/application.ex @@ -10,8 +10,7 @@ defmodule ExFTP.Application do children = [ {Cachex, [:auth_cache]}, - {DynamicSupervisor, name: ExFTP.WorkerSupervisor, strategy: :one_for_one}, - {ExFTP.Server, port: port} + {ThousandIsland, port: port, handler_module: ExFTP.Worker, transport_options: [packet: :line]} ] opts = [strategy: :one_for_one, name: ExFTP.Supervisor] diff --git a/lib/ex_ftp/ftp_common.ex b/lib/ex_ftp/ftp_common.ex index 709b8f3..4b61f7e 100644 --- a/lib/ex_ftp/ftp_common.ex +++ b/lib/ex_ftp/ftp_common.ex @@ -12,6 +12,6 @@ defmodule ExFTP.Common do def send_resp(code, msg, socket) when is_integer(code) and is_bitstring(msg) do response = "#{code} #{msg}\r\n" Logger.info("Sending FTP response:\t#{inspect(response)}") - :ok = :gen_tcp.send(socket, response) + :ok = ThousandIsland.Socket.send(socket, response) end end diff --git a/lib/ex_ftp/passive_socket.ex b/lib/ex_ftp/passive_socket.ex index c591da3..b3ed479 100644 --- a/lib/ex_ftp/passive_socket.ex +++ b/lib/ex_ftp/passive_socket.ex @@ -23,6 +23,8 @@ defmodule ExFTP.PassiveSocket do GenServer.call(pid, {:write, data, opts}, :infinity) end + def close(nil), do: :ok + def close(pid) do if Process.alive?(pid) do GenServer.call(pid, {:close}, :infinity) diff --git a/lib/ex_ftp/worker.ex b/lib/ex_ftp/worker.ex index a065b3f..91f95bd 100644 --- a/lib/ex_ftp/worker.ex +++ b/lib/ex_ftp/worker.ex @@ -1,10 +1,9 @@ # SPDX-License-Identifier: Apache-2.0 defmodule ExFTP.Worker do @moduledoc """ - A module defining a `GenServer` which serves the FTP interface + A module defining a `Handler` which serves the FTP interface """ - - use GenServer + use ThousandIsland.Handler import Bitwise import ExFTP.Common @@ -16,8 +15,19 @@ defmodule ExFTP.Worker do require Logger - @impl GenServer - def init(socket) do + defstruct [ + :socket, + :storage_connector, + :authenticator, + :host, + pasv_socket: nil, + type: :ascii, + connector_state: %{current_working_directory: "/"}, + authenticator_state: %{} + ] + + @impl ThousandIsland.Handler + def handle_connection(socket, _options) do env = Application.get_all_env(:ex_ftp) ftp_addr = env[:ftp_addr] || "127.0.0.1" mix_env = env[:mix_env] @@ -31,24 +41,19 @@ defmodule ExFTP.Worker do |> :inet.parse_address() if mix_env != :test do - {:ok, {ip_address, _port}} = :inet.peername(socket) + {:ok, {ip_address, _port}} = ThousandIsland.Socket.peername(socket) ip_address_str = ip_address |> Tuple.to_list() |> Enum.join(".") Logger.info("Received FTP connection from #{ip_address_str}") end send_resp(220, "Hello from #{server_name}.", socket) - {:ok, - %{ - host: host, + {:continue, + %__MODULE__{ socket: socket, + host: host, pasv_socket: nil, type: :ascii, - username: nil, - current_user: nil, - user_prefix: nil, - prefix: "/", - virtual_directories: [], storage_connector: connector, connector_state: %{current_working_directory: "/"}, authenticator: authenticator, @@ -56,75 +61,76 @@ defmodule ExFTP.Worker do }} end - @impl GenServer - def handle_info({:tcp, _socket, data}, state) do - sanitized = - String.trim(data) - |> String.split(" ", parts: 2) - |> case do - ["PASS", _] -> "PASS *******" - _ -> String.trim(data) - end - - Logger.info("Received FTP message:\t#{inspect(sanitized)}") + @impl ThousandIsland.Handler + def handle_data(data, socket, state) do + data = String.trim(data) data - |> parse() - |> run(state) + |> String.split(" ", parts: 2) + |> log_message(data) + |> run(socket, state) end - def handle_info(:read_complete, %{socket: _socket, pasv_socket: pasv} = state) do - PassiveSocket.close(pasv) - {:noreply, %{state | pasv_socket: nil}} - end + @impl ThousandIsland.Handler + def handle_close(_socket, state), do: PassiveSocket.close(state.pasv_socket) - def handle_info({:tcp_closed, _}, state), do: {:stop, :normal, state} - def handle_info({:tcp_error, _}, state), do: {:stop, :normal, state} + @impl ThousandIsland.Handler + def handle_shutdown(_socket, state), do: PassiveSocket.close(state.pasv_socket) - def child_spec(arg) do - %{ - id: __MODULE__, - start: {__MODULE__, :start_link, [arg]}, - restart: :temporary - } + @impl ThousandIsland.Handler + def handle_timeout(_socket, state), do: PassiveSocket.close(state.pasv_socket) + + @impl GenServer + def handle_info({:send, msg}, {socket, state}) do + ThousandIsland.Socket.send(socket, msg) + {:noreply, {socket, state}, socket.read_timeout} end - def start_link(socket) do - GenServer.start_link(__MODULE__, socket, name: __MODULE__) + @impl GenServer + def handle_info(:read_complete, {socket, state}) do + Logger.info("Read complete") + PassiveSocket.close(state.pasv_socket) + {:noreply, {socket, %{state | pasv_socket: nil}}, socket.read_timeout} end - defp parse(data) do - data - |> String.trim() - |> String.split(" ", parts: 2) + @impl GenServer + def handle_info({:tcp_closed, _}, state), do: {:stop, :normal, state} + + @impl GenServer + def handle_info({:tcp_error, _}, state), do: {:stop, :normal, state} + + @impl GenServer + def handle_info({:EXIT, _pid, :normal}, {socket, state}) do + state = %{state | pasv_socket: nil} + {:noreply, {socket, state}, socket.read_timeout} end - defp run(["QUIT"], state) do - quit(state) + defp run(["QUIT"], socket, state) do + quit(socket, state) end - defp run(["SYST"], %{socket: socket} = state) do + defp run(["SYST"], socket, state) do send_resp(215, "UNIX Type: L8", socket) - {:noreply, state} + {:continue, state} end - defp run(["TYPE", type], %{socket: socket} = state) do + defp run(["TYPE", type], socket, state) do case type do "I" -> send_resp(200, "Switching to binary mode.", socket) - {:noreply, %{state | type: :image}} + {:continue, %{state | type: :image}} "A" -> send_resp(200, "Switching to ASCII mode.", socket) - {:noreply, %{state | type: :ascii}} + {:continue, %{state | type: :ascii}} _ -> send_resp(504, "Unsupported transfer type.", socket) - {:noreply, state} + {:continue, state} end end - defp run(["PASV"], %{socket: socket} = server_state) do + defp run(["PASV"], socket, server_state) do case check_auth(server_state) do :ok -> {:ok, pasv} = PassiveSocket.start_link() @@ -134,59 +140,53 @@ defmodule ExFTP.Worker do pasv_string = ip_port_to_pasv(host, port) send_resp(227, "Entering Passive Mode (#{pasv_string}).", socket) - {:noreply, %{server_state | pasv_socket: pasv}} + {:continue, %{server_state | pasv_socket: pasv}} _ -> - {:noreply, server_state} + {:continue, server_state} end end - defp run(["EPSV"], %{socket: socket} = server_state) do + defp run(["EPSV"], socket, server_state) do case check_auth(server_state) do :ok -> {:ok, pasv} = PassiveSocket.start_link() {:ok, port} = PassiveSocket.get_port(pasv) send_resp(229, "Entering Extended Passive Mode (|||#{port}|)", socket) - {:noreply, %{server_state | pasv_socket: pasv}} + {:continue, %{server_state | pasv_socket: pasv}} _ -> - {:noreply, server_state} + {:continue, server_state} end end - defp run(["EPRT", _eport_info], %{socket: socket} = server_state) do + defp run(["EPRT", _eport_info], socket, server_state) do with :ok <- check_auth(server_state) do send_resp(200, "EPRT command successful.", socket) end - {:noreply, server_state} + {:continue, server_state} end # Auth Commands - defp run(["USER", username], %{socket: socket, authenticator: authenticator} = server_state) do - valid? = authenticator.valid_user?(username) + defp run(["USER", username], socket, server_state) do + valid? = server_state.authenticator.valid_user?(username) server_state = - if valid?, - do: - server_state - |> Map.put(:authenticator_state, %{username: username}), - else: - server_state - |> Map.put(:authenticator_state, %{authenticated: false}) + %{ + server_state + | authenticator_state: if(valid?, do: %{username: username}, else: %{authenticated: false}) + } send_resp(331, "User name okay, need password.", socket) noreply(server_state) end - defp run( - ["PASS", password], - %{socket: socket, authenticator: authenticator, authenticator_state: auth_state} = server_state - ) do - authenticator.login(password, auth_state) + defp run(["PASS", password], socket, server_state) do + server_state.authenticator.login(password, server_state.authenticator_state) |> case do {:ok, auth_state} -> auth_state = Map.put(auth_state, :authenticated, true) @@ -194,9 +194,11 @@ defmodule ExFTP.Worker do send_resp(230, "Welcome.", socket) - server_state - |> Map.put(:authenticator_state, auth_state) - |> Map.put(:connector_state, connector_state) + %{ + server_state + | authenticator_state: auth_state, + connector_state: connector_state + } |> noreply() {_, %{} = auth_state} -> @@ -214,149 +216,144 @@ defmodule ExFTP.Worker do end # Storage Connector Commands - defp run(["PWD"], %{socket: _socket} = server_state) do + defp run(["PWD"], socket, server_state) do server_state |> check_auth() - |> with_ok(&pwd/1, server_state) + |> with_ok(&pwd/1, socket, server_state) |> update_connector_state(server_state) |> noreply() end - defp run(["CDUP"], state), do: run(["CWD", ".."], state) + defp run(["CDUP"], socket, state), do: run(["CWD", ".."], socket, state) - defp run(["CWD", path], %{socket: _socket} = server_state) do + defp run(["CWD", path], socket, server_state) do server_state |> check_auth() - |> with_ok(&cwd/1, server_state, path: path) + |> with_ok(&cwd/1, socket, server_state, path: path) |> update_connector_state(server_state) |> noreply() end - defp run(["MKD", path], %{socket: _socket} = server_state) do + defp run(["MKD", path], socket, server_state) do server_state |> check_auth() - |> with_ok(&mkd/1, server_state, path: path) + |> with_ok(&mkd/1, socket, server_state, path: path) |> update_connector_state(server_state) |> noreply() end - defp run(["RMD", path], %{socket: _socket} = server_state) do + defp run(["RMD", path], socket, server_state) do server_state |> check_auth() - |> with_ok(&rmd/1, server_state, path: path) + |> with_ok(&rmd/1, socket, server_state, path: path) |> update_connector_state(server_state) |> noreply() end - defp run(["DELE", path], %{socket: _socket} = server_state) do + defp run(["DELE", path], socket, server_state) do server_state |> check_auth() - |> with_ok(&dele/1, server_state, path: path) + |> with_ok(&dele/1, socket, server_state, path: path) |> update_connector_state(server_state) |> noreply() end - defp run(["LIST", "-a"], server_state), do: run(["LIST", "-a", "."], server_state) + defp run(["LIST", "-a"], socket, server_state), do: run(["LIST", "-a", "."], socket, server_state) - defp run(["LIST", "-a", path], %{socket: _socket} = server_state) do + defp run(["LIST", "-a", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&list/1, server_state, pasv: pasv, path: path, include_hidden: true) + |> with_ok(&list/1, socket, server_state, pasv: pasv, path: path, include_hidden: true) |> update_connector_state(server_state) |> noreply() end end - defp run(["LIST"], server_state), do: run(["LIST", "."], server_state) + defp run(["LIST"], socket, server_state), do: run(["LIST", "."], socket, server_state) - defp run(["LIST", path], %{socket: _socket} = server_state) do + defp run(["LIST", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&list/1, server_state, pasv: pasv, path: path, include_hidden: false) + |> with_ok(&list/1, socket, server_state, pasv: pasv, path: path, include_hidden: false) |> update_connector_state(server_state) |> noreply() end end - defp run(["NLST", "-a"], server_state), do: run(["NLST", "-a", "."], server_state) + defp run(["NLST", "-a"], socket, server_state), do: run(["NLST", "-a", "."], socket, server_state) - defp run(["NLST", "-a", path], %{socket: _socket} = server_state) do + defp run(["NLST", "-a", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&nlst/1, server_state, pasv: pasv, path: path, include_hidden: true) + |> with_ok(&nlst/1, socket, server_state, pasv: pasv, path: path, include_hidden: true) |> update_connector_state(server_state) |> noreply() end end - defp run(["NLST"], state), do: run(["NLST", "."], state) + defp run(["NLST"], socket, state), do: run(["NLST", "."], socket, state) - defp run(["NLST", path], %{socket: _socket} = server_state) do + defp run(["NLST", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&nlst/1, server_state, pasv: pasv, path: path, include_hidden: false) + |> with_ok(&nlst/1, socket, server_state, pasv: pasv, path: path, include_hidden: false) |> update_connector_state(server_state) |> noreply() end end - defp run(["RETR", path], %{socket: _socket} = server_state) do + defp run(["RETR", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&retr/1, server_state, pasv: pasv, path: path) + |> with_ok(&retr/1, socket, server_state, pasv: pasv, path: path) |> update_connector_state(server_state) |> noreply() end end - defp run(["SIZE", path], %{socket: _socket} = server_state) do + defp run(["SIZE", path], socket, server_state) do server_state |> check_auth() - |> with_ok(&size/1, server_state, path: path) + |> with_ok(&size/1, socket, server_state, path: path) |> update_connector_state(server_state) |> noreply() end - defp run(["STOR", path], %{socket: _socket} = server_state) do + defp run(["STOR", path], socket, server_state) do with {:ok, pasv} <- with_pasv_socket(server_state) do server_state |> check_auth() - |> with_ok(&stor/1, server_state, pasv: pasv, path: path) + |> with_ok(&stor/1, socket, server_state, pasv: pasv, path: path) |> update_connector_state(server_state) |> noreply() end end - defp run(_args, %{socket: socket} = state) do + defp run(_args, socket, state) do send_resp(502, "Command not implemented.", socket) - {:noreply, state} + {:continue, state} end - defp with_ok( - maybe_ok, - fnc, - %{socket: socket, storage_connector: connector, connector_state: connector_state}, - opts \\ [] - ) do - case maybe_ok do - :ok -> - fnc.(%{ - socket: socket, - storage_connector: connector, - connector_state: connector_state, - path: opts[:path], - pasv: opts[:pasv], - include_hidden: opts[:include_hidden] - }) + defp with_ok(result, fnc, socket, state, opts \\ []) - _ -> - connector_state - end + defp with_ok(:ok, fnc, socket, state, opts) do + fnc.(%{ + socket: socket, + storage_connector: state.storage_connector, + connector_state: state.connector_state, + path: opts[:path], + pasv: opts[:pasv], + include_hidden: opts[:include_hidden] + }) + end + + defp with_ok(_other, _fnc, _socket, state, _opts) do + state.connector_state end defp authenticate(auth, auth_state) do @@ -366,8 +363,7 @@ defmodule ExFTP.Worker do end defp get_auth_ttl do - Application.get_env(:ex_ftp, :authenticator_config, %{})[:authenticated_ttl_ms] || - 24 * 60 * 60 * 60 * 1000 + Application.get_env(:ex_ftp, :authenticator_config, %{})[:authenticated_ttl_ms] || to_timeout(day: 1) end defp check_auth(%{socket: socket, authenticator: auth, authenticator_state: %{username: username} = auth_state}) @@ -405,12 +401,10 @@ defmodule ExFTP.Worker do end defp update_connector_state(connector_state, server_state) do - Map.put(server_state, :connector_state, connector_state) + %{server_state | connector_state: connector_state} end - defp noreply(state) do - {:noreply, state} - end + defp noreply(state), do: {:continue, state} defp ip_port_to_pasv(ip, port) do upper_port = port >>> 8 @@ -425,23 +419,27 @@ defmodule ExFTP.Worker do {:ok, pasv} else send_resp(550, "CMD failed. PASV mode required.", Map.get(state, :socket)) - {:noreply, state} + noreply(state) end end - defp quit(%{socket: socket} = state) do + defp quit(socket, state) do Logger.info("Shutting down. Client closed connection.") send_resp(221, "Closing connection.", socket) - :gen_tcp.close(socket) + PassiveSocket.close(state.pasv_socket) - pasv = state[:pasv_socket] + {:close, %{state | pasv_socket: nil}} + end - if pasv && Process.alive?(pasv) do - PassiveSocket.close(pasv) - end + defp log_message(["PASS", _] = message, _data) do + Logger.info("Received FTP message:\t#{inspect("PASS *******")}") + message + end - {:stop, :normal, state} + defp log_message(message, data) do + Logger.info("Received FTP message:\t#{inspect(data)}") + message end end diff --git a/mix.exs b/mix.exs index ee90400..56972c4 100644 --- a/mix.exs +++ b/mix.exs @@ -134,7 +134,8 @@ defmodule ExFTP.MixProject do {:sweet_xml, "~> 0.7"}, {:configparser_ex, "~> 4.0"}, {:cachex, "~> 4.1"}, - {:proper_case, "~> 1.3"} + {:proper_case, "~> 1.3"}, + {:thousand_island, "~> 1.0"} ] end end diff --git a/mix.lock b/mix.lock index 66c69b8..3053627 100644 --- a/mix.lock +++ b/mix.lock @@ -43,6 +43,7 @@ "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.7", "354c321cf377240c7b8716899e182ce4890c5938111a1296add3ec74cf1715df", [:make, :mix, :rebar3], [], "hexpm", "fe4c190e8f37401d30167c8c405eda19469f34577987c76dde613e838bbc67f8"}, "sweet_xml": {:hex, :sweet_xml, "0.7.5", "803a563113981aaac202a1dbd39771562d0ad31004ddbfc9b5090bdcd5605277", [:mix], [], "hexpm", "193b28a9b12891cae351d81a0cead165ffe67df1b73fe5866d10629f4faefb12"}, "telemetry": {:hex, :telemetry, "1.3.0", "fedebbae410d715cf8e7062c96a1ef32ec22e764197f70cda73d82778d61e7a2", [:rebar3], [], "hexpm", "7015fc8919dbe63764f4b4b87a95b7c0996bd539e0d499be6ec9d7f3875b79e6"}, + "thousand_island": {:hex, :thousand_island, "1.4.2", "735fa783005d1703359bbd2d3a5a3a398075ba4456e5afe3c5b7cf4666303d36", [:mix], [{:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "1c7637f16558fc1c35746d5ee0e83b18b8e59e18d28affd1f2fa1645f8bc7473"}, "unicode_util_compat": {:hex, :unicode_util_compat, "0.7.1", "a48703a25c170eedadca83b11e88985af08d35f37c6f664d6dcfb106a97782fc", [:rebar3], [], "hexpm", "b3a917854ce3ae233619744ad1e0102e05673136776fb2fa76234f3e03b23642"}, "unsafe": {:hex, :unsafe, "1.0.2", "23c6be12f6c1605364801f4b47007c0c159497d0446ad378b5cf05f1855c0581", [:mix], [], "hexpm", "b485231683c3ab01a9cd44cb4a79f152c6f3bb87358439c6f68791b85c2df675"}, } diff --git a/test/ex_ftp/server_test.exs b/test/ex_ftp/server_test.exs deleted file mode 100644 index 837192e..0000000 --- a/test/ex_ftp/server_test.exs +++ /dev/null @@ -1,19 +0,0 @@ -defmodule ExFTP.ServerTest do - use ExUnit.Case - - doctest ExFTP.Server - @moduletag :capture_log - test "accepts tcp connections" do - %{active: active_children} = DynamicSupervisor.count_children(ExFTP.WorkerSupervisor) - port = Application.get_env(:ex_ftp, :ftp_port) - assert {:ok, socket} = :gen_tcp.connect({127, 0, 0, 1}, port, [:inet, :binary]) - - # give worker time to start up - Process.sleep(100) - - %{active: now_active_children} = DynamicSupervisor.count_children(ExFTP.WorkerSupervisor) - assert active_children + 1 == now_active_children - - :gen_tcp.close(socket) - end -end diff --git a/test/test_helper.exs b/test/test_helper.exs index fe56be0..aca43d7 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -22,7 +22,7 @@ defmodule ExFTP.TestHelper do def expect_recv(socket, code, msg_start \\ "") do match = "#{code} #{msg_start}" - {:ok, ^match <> _} = :gen_tcp.recv(socket, 0, 20_000) + assert {:ok, ^match <> _} = :gen_tcp.recv(socket, 0, 20_000) socket end @@ -100,6 +100,10 @@ defmodule ExFTP.StorageTester do |> send_and_expect("PWD", [], 257, "\"#{tmp_dir}\" is the current directory") |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") + |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") + |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") + |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") + |> send_and_expect("CDUP", [], 250, "Directory changed successfully.") |> send_and_expect("PWD", [], 257, "\"/\" is the current directory") socket From cbe4a873a8575e82cdac96dfe00025aaea8c7f8c Mon Sep 17 00:00:00 2001 From: Cam Cook Date: Sun, 7 Dec 2025 12:17:17 -0500 Subject: [PATCH 2/6] fix: revert to use LocalStack (which is what the ci uses) --- config/test.exs | 6 +++--- docker-compose.yml | 23 +++++++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/config/test.exs b/config/test.exs index 193dd7b..fa7435f 100644 --- a/config/test.exs +++ b/config/test.exs @@ -7,9 +7,9 @@ config :ex_aws, s3: [ scheme: System.get_env("AWS_SCHEME", "http://"), host: System.get_env("AWS_HOST", "localhost"), - port: "AWS_PORT" |> System.get_env("9222") |> String.to_integer(), - access_key_id: "minio", - secret_access_key: "minio123456" + port: "AWS_PORT" |> System.get_env("4566") |> String.to_integer(), + access_key_id: "", + secret_access_key: "" ] config :ex_ftp, diff --git a/docker-compose.yml b/docker-compose.yml index 4fa7001..97a3cc7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,17 +1,16 @@ services: - minio: - image: minio/minio:RELEASE.2023-12-07T04-16-00Z - restart: unless-stopped + localstack: + container_name: "${LOCALSTACK_DOCKER_NAME:-localstack-main}" + image: localstack/localstack ports: - - 9222:9222 - - 9333:9333 - volumes: - - minio-data:/data + - "4566:4566" # LocalStack Gateway + - "4510-4559:4510-4559" # external services port range environment: - MINIO_ROOT_USER: minio - MINIO_ROOT_PASSWORD: minio123456 - entrypoint: sh - command: "-c 'mkdir -p /data/ex-ftp-test && minio server --address :9222 --console-address :9333 /data'" + # LocalStack configuration: https://docs.localstack.cloud/references/configuration/ + - DEBUG=${DEBUG:-0} + volumes: + - localstack-data:/var/lib/localstack" + - "/var/run/docker.sock:/var/run/docker.sock" volumes: - minio-data: {} + localstack-data: {} From f21cfe4f78598bfdf4c80fd907ee338b1519c7c1 Mon Sep 17 00:00:00 2001 From: Cam Cook Date: Sun, 7 Dec 2025 13:51:12 -0500 Subject: [PATCH 3/6] style: minor fixes and highly opinioned style changes --- config/dev.exs | 17 +++++++- lib/ex_ftp/application.ex | 4 +- lib/ex_ftp/passive_socket.ex | 6 +-- lib/ex_ftp/worker.ex | 75 ++++++++++++++++++------------------ test/test_helper.exs | 5 +-- 5 files changed, 60 insertions(+), 47 deletions(-) diff --git a/config/dev.exs b/config/dev.exs index 3d93ac9..fa7435f 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -1,7 +1,7 @@ import Config alias ExFTP.Auth.PassthroughAuth -alias ExFTP.Storage.S3Connector +alias ExFTP.Storage.FileConnector config :ex_aws, s3: [ @@ -11,3 +11,18 @@ config :ex_aws, access_key_id: "", secret_access_key: "" ] + +config :ex_ftp, + ftp_port: "FTP_PORT" |> System.get_env("4041") |> String.to_integer(), + ftp_addr: System.get_env("FTP_ADDR", "127.0.0.1"), + min_passive_port: "MIN_PASSIVE_PORT" |> System.get_env("40002") |> String.to_integer(), + max_passive_port: "MAX_PASSIVE_PORT" |> System.get_env("40007") |> String.to_integer(), + authenticator: PassthroughAuth, + authenticator_config: %{ + authenticated_url: nil, + authenticated_method: :get, + authenticated_ttl_ms: to_timeout(day: 1), + login_url: nil, + login_method: :get + }, + storage_connector: FileConnector diff --git a/lib/ex_ftp/application.ex b/lib/ex_ftp/application.ex index 79f4366..4315a78 100644 --- a/lib/ex_ftp/application.ex +++ b/lib/ex_ftp/application.ex @@ -4,6 +4,8 @@ defmodule ExFTP.Application do use Application + require Logger + @impl true def start(_type, _args) do port = Application.get_env(:ex_ftp, :ftp_port, 4041) @@ -14,7 +16,7 @@ defmodule ExFTP.Application do ] opts = [strategy: :one_for_one, name: ExFTP.Supervisor] - + Logger.info("Accepting connections on port #{port}") Supervisor.start_link(children, opts) end end diff --git a/lib/ex_ftp/passive_socket.ex b/lib/ex_ftp/passive_socket.ex index b3ed479..a98de46 100644 --- a/lib/ex_ftp/passive_socket.ex +++ b/lib/ex_ftp/passive_socket.ex @@ -26,9 +26,9 @@ defmodule ExFTP.PassiveSocket do def close(nil), do: :ok def close(pid) do - if Process.alive?(pid) do - GenServer.call(pid, {:close}, :infinity) - end + if Process.alive?(pid), + do: GenServer.call(pid, {:close}, :infinity), + else: :ok end # Server API diff --git a/lib/ex_ftp/worker.ex b/lib/ex_ftp/worker.ex index 91f95bd..e466cdb 100644 --- a/lib/ex_ftp/worker.ex +++ b/lib/ex_ftp/worker.ex @@ -63,9 +63,8 @@ defmodule ExFTP.Worker do @impl ThousandIsland.Handler def handle_data(data, socket, state) do - data = String.trim(data) - data + |> String.trim() |> String.split(" ", parts: 2) |> log_message(data) |> run(socket, state) @@ -88,7 +87,7 @@ defmodule ExFTP.Worker do @impl GenServer def handle_info(:read_complete, {socket, state}) do - Logger.info("Read complete") + Logger.debug("Read complete") PassiveSocket.close(state.pasv_socket) {:noreply, {socket, %{state | pasv_socket: nil}}, socket.read_timeout} end @@ -105,9 +104,7 @@ defmodule ExFTP.Worker do {:noreply, {socket, state}, socket.read_timeout} end - defp run(["QUIT"], socket, state) do - quit(socket, state) - end + defp run(["QUIT"], socket, state), do: quit(socket, state) defp run(["SYST"], socket, state) do send_resp(215, "UNIX Type: L8", socket) @@ -171,47 +168,49 @@ defmodule ExFTP.Worker do # Auth Commands - defp run(["USER", username], socket, server_state) do - valid? = server_state.authenticator.valid_user?(username) + defp run(["USER", username], socket, %{authenticator: authenticator} = server_state) do + valid? = authenticator.valid_user?(username) server_state = - %{ - server_state - | authenticator_state: if(valid?, do: %{username: username}, else: %{authenticated: false}) - } + if valid?, + do: Map.put(server_state, :authenticator_state, %{username: username}), + else: Map.put(server_state, :authenticator_state, %{authenticated: false}) send_resp(331, "User name okay, need password.", socket) - noreply(server_state) + continue(server_state) end - defp run(["PASS", password], socket, server_state) do - server_state.authenticator.login(password, server_state.authenticator_state) + defp run( + ["PASS", password], + socket, + %{authenticator: authenticator, authenticator_state: auth_state, connector_state: connector_state} = + server_state + ) do + authenticator.login(password, auth_state) |> case do {:ok, auth_state} -> auth_state = Map.put(auth_state, :authenticated, true) - connector_state = Map.put(server_state.connector_state, :authenticator_state, auth_state) + connector_state = Map.put(connector_state, :authenticator_state, auth_state) send_resp(230, "Welcome.", socket) - %{ - server_state - | authenticator_state: auth_state, - connector_state: connector_state - } - |> noreply() + server_state + |> Map.put(:authenticator_state, auth_state) + |> Map.put(:connector_state, connector_state) + |> continue() {_, %{} = auth_state} -> send_resp(530, "Authentication failed.", socket) server_state |> Map.put(:authenticator_state, auth_state) - |> noreply() + |> continue() _ -> send_resp(530, "Authentication failed.", socket) - noreply(server_state) + continue(server_state) end end @@ -221,7 +220,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&pwd/1, socket, server_state) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["CDUP"], socket, state), do: run(["CWD", ".."], socket, state) @@ -231,7 +230,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&cwd/1, socket, server_state, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["MKD", path], socket, server_state) do @@ -239,7 +238,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&mkd/1, socket, server_state, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["RMD", path], socket, server_state) do @@ -247,7 +246,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&rmd/1, socket, server_state, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["DELE", path], socket, server_state) do @@ -255,7 +254,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&dele/1, socket, server_state, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["LIST", "-a"], socket, server_state), do: run(["LIST", "-a", "."], socket, server_state) @@ -266,7 +265,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&list/1, socket, server_state, pasv: pasv, path: path, include_hidden: true) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -278,7 +277,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&list/1, socket, server_state, pasv: pasv, path: path, include_hidden: false) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -290,7 +289,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&nlst/1, socket, server_state, pasv: pasv, path: path, include_hidden: true) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -302,7 +301,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&nlst/1, socket, server_state, pasv: pasv, path: path, include_hidden: false) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -312,7 +311,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&retr/1, socket, server_state, pasv: pasv, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -321,7 +320,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&size/1, socket, server_state, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end defp run(["STOR", path], socket, server_state) do @@ -330,7 +329,7 @@ defmodule ExFTP.Worker do |> check_auth() |> with_ok(&stor/1, socket, server_state, pasv: pasv, path: path) |> update_connector_state(server_state) - |> noreply() + |> continue() end end @@ -404,7 +403,7 @@ defmodule ExFTP.Worker do %{server_state | connector_state: connector_state} end - defp noreply(state), do: {:continue, state} + defp continue(state), do: {:continue, state} defp ip_port_to_pasv(ip, port) do upper_port = port >>> 8 @@ -419,7 +418,7 @@ defmodule ExFTP.Worker do {:ok, pasv} else send_resp(550, "CMD failed. PASV mode required.", Map.get(state, :socket)) - noreply(state) + continue(state) end end diff --git a/test/test_helper.exs b/test/test_helper.exs index aca43d7..690ad1d 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -277,12 +277,9 @@ defmodule ExFTP.StorageTester do |> File.read!() :ok = :gen_tcp.send(pasv_socket, data) - close_pasv(pasv_socket) - expect_recv(socket, 226, "Transfer Complete.") - :timer.sleep(400) - :timer.sleep(100) + :timer.sleep(600) send_and_expect(socket, "SIZE", [file], 213) end) end From db83ed2ef20e0864ea4ea5de20c6b1474d31248b Mon Sep 17 00:00:00 2001 From: Cam Cook Date: Sun, 7 Dec 2025 14:10:42 -0500 Subject: [PATCH 4/6] style: consistent continue --- lib/ex_ftp/worker.ex | 49 ++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/lib/ex_ftp/worker.ex b/lib/ex_ftp/worker.ex index e466cdb..615a82b 100644 --- a/lib/ex_ftp/worker.ex +++ b/lib/ex_ftp/worker.ex @@ -9,6 +9,7 @@ defmodule ExFTP.Worker do import ExFTP.Common import ExFTP.Storage.Common + alias __MODULE__, as: Worker alias ExFTP.Auth.PassthroughAuth alias ExFTP.PassiveSocket alias ExFTP.Storage.FileConnector @@ -48,17 +49,17 @@ defmodule ExFTP.Worker do send_resp(220, "Hello from #{server_name}.", socket) - {:continue, - %__MODULE__{ - socket: socket, - host: host, - pasv_socket: nil, - type: :ascii, - storage_connector: connector, - connector_state: %{current_working_directory: "/"}, - authenticator: authenticator, - authenticator_state: %{} - }} + %Worker{ + socket: socket, + host: host, + pasv_socket: nil, + type: :ascii, + storage_connector: connector, + connector_state: %{current_working_directory: "/"}, + authenticator: authenticator, + authenticator_state: %{} + } + |> continue() end @impl ThousandIsland.Handler @@ -108,23 +109,25 @@ defmodule ExFTP.Worker do defp run(["SYST"], socket, state) do send_resp(215, "UNIX Type: L8", socket) - {:continue, state} + continue(state) end defp run(["TYPE", type], socket, state) do - case type do + type + |> case do "I" -> send_resp(200, "Switching to binary mode.", socket) - {:continue, %{state | type: :image}} + %{state | type: :image} "A" -> send_resp(200, "Switching to ASCII mode.", socket) - {:continue, %{state | type: :ascii}} + %{state | type: :ascii} _ -> send_resp(504, "Unsupported transfer type.", socket) - {:continue, state} + state end + |> continue() end defp run(["PASV"], socket, server_state) do @@ -137,11 +140,12 @@ defmodule ExFTP.Worker do pasv_string = ip_port_to_pasv(host, port) send_resp(227, "Entering Passive Mode (#{pasv_string}).", socket) - {:continue, %{server_state | pasv_socket: pasv}} + %{server_state | pasv_socket: pasv} _ -> - {:continue, server_state} + server_state end + |> continue() end defp run(["EPSV"], socket, server_state) do @@ -151,11 +155,12 @@ defmodule ExFTP.Worker do {:ok, port} = PassiveSocket.get_port(pasv) send_resp(229, "Entering Extended Passive Mode (|||#{port}|)", socket) - {:continue, %{server_state | pasv_socket: pasv}} + %{server_state | pasv_socket: pasv} _ -> - {:continue, server_state} + server_state end + |> continue() end defp run(["EPRT", _eport_info], socket, server_state) do @@ -163,7 +168,7 @@ defmodule ExFTP.Worker do send_resp(200, "EPRT command successful.", socket) end - {:continue, server_state} + continue(server_state) end # Auth Commands @@ -335,7 +340,7 @@ defmodule ExFTP.Worker do defp run(_args, socket, state) do send_resp(502, "Command not implemented.", socket) - {:continue, state} + continue(state) end defp with_ok(result, fnc, socket, state, opts \\ []) From 1d757871940ea46b07c655c805b110ce29532d05 Mon Sep 17 00:00:00 2001 From: Cam Cook Date: Sun, 7 Dec 2025 14:26:39 -0500 Subject: [PATCH 5/6] style: remove server code, style --- lib/ex_ftp/server.ex | 60 -------------------------------------------- lib/ex_ftp/worker.ex | 50 ++++++++++++++++++------------------ 2 files changed, 25 insertions(+), 85 deletions(-) delete mode 100644 lib/ex_ftp/server.ex diff --git a/lib/ex_ftp/server.ex b/lib/ex_ftp/server.ex deleted file mode 100644 index 3906f4e..0000000 --- a/lib/ex_ftp/server.ex +++ /dev/null @@ -1,60 +0,0 @@ -# SPDX-License-Identifier: Apache-2.0 -defmodule ExFTP.Server do - @moduledoc false - - use GenServer - - require Logger - - def start_link(opts) do - GenServer.start_link(__MODULE__, opts) - end - - @impl GenServer - def init(opts) do - port = Keyword.get(opts, :port, 4040) - - {:ok, socket} = - :gen_tcp.listen( - port, - [:binary, packet: :line, active: true, reuseaddr: true] - ) - - Logger.info("Accepting connections on port #{port}") - - {:ok, %{socket: socket}, {:continue, :accept}} - end - - @impl GenServer - def handle_continue(:accept, %{socket: socket} = state) do - accept(socket) - - {:noreply, state} - end - - @impl GenServer - def handle_info(:accept, %{socket: socket} = state) do - accept(socket) - - {:noreply, state} - end - - defp accept(socket) do - {:ok, client} = :gen_tcp.accept(socket) - - :ok = - DynamicSupervisor.start_child(ExFTP.WorkerSupervisor, {ExFTP.Worker, client}) - |> case do - {:ok, pid} -> - :gen_tcp.controlling_process(client, pid) - - {:error, {:already_started, pid}} -> - :gen_tcp.controlling_process(client, pid) - - other -> - other - end - - send(self(), :accept) - end -end diff --git a/lib/ex_ftp/worker.ex b/lib/ex_ftp/worker.ex index 615a82b..ebcaf57 100644 --- a/lib/ex_ftp/worker.ex +++ b/lib/ex_ftp/worker.ex @@ -345,30 +345,27 @@ defmodule ExFTP.Worker do defp with_ok(result, fnc, socket, state, opts \\ []) - defp with_ok(:ok, fnc, socket, state, opts) do - fnc.(%{ - socket: socket, - storage_connector: state.storage_connector, - connector_state: state.connector_state, - path: opts[:path], - pasv: opts[:pasv], - include_hidden: opts[:include_hidden] - }) - end - - defp with_ok(_other, _fnc, _socket, state, _opts) do - state.connector_state - end + defp with_ok(:ok, fnc, socket, state, opts), + do: + fnc.(%{ + socket: socket, + storage_connector: state.storage_connector, + connector_state: state.connector_state, + path: opts[:path], + pasv: opts[:pasv], + include_hidden: opts[:include_hidden] + }) + + defp with_ok(_other, _fnc, _socket, state, _opts), do: state.connector_state defp authenticate(auth, auth_state) do - if auth.authenticated?(auth_state) do - :ok - end + if auth.authenticated?(auth_state), + do: :ok, + else: :not_authenticated end - defp get_auth_ttl do - Application.get_env(:ex_ftp, :authenticator_config, %{})[:authenticated_ttl_ms] || to_timeout(day: 1) - end + defp get_auth_ttl, + do: Application.get_env(:ex_ftp, :authenticator_config, %{})[:authenticated_ttl_ms] || to_timeout(day: 1) defp check_auth(%{socket: socket, authenticator: auth, authenticator_state: %{username: username} = auth_state}) when not is_nil(username) do @@ -396,11 +393,14 @@ defmodule ExFTP.Worker do end defp check_auth(%{socket: socket, authenticator: auth, authenticator_state: auth_state}) do - if :ok == authenticate(auth, auth_state) do - :ok - else - send_resp(530, "Not logged in.", socket) - :error + authenticate(auth, auth_state) + |> case do + :ok -> + :ok + + _ -> + send_resp(530, "Not logged in.", socket) + :error end end From ccb04228d6affe3b5b7708b5995d91acb7980413 Mon Sep 17 00:00:00 2001 From: Cam Cook Date: Sun, 7 Dec 2025 14:27:55 -0500 Subject: [PATCH 6/6] chore: dev config uses diff port from test --- config/dev.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/dev.exs b/config/dev.exs index fa7435f..fad1448 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -13,7 +13,7 @@ config :ex_aws, ] config :ex_ftp, - ftp_port: "FTP_PORT" |> System.get_env("4041") |> String.to_integer(), + ftp_port: "FTP_PORT" |> System.get_env("4040") |> String.to_integer(), ftp_addr: System.get_env("FTP_ADDR", "127.0.0.1"), min_passive_port: "MIN_PASSIVE_PORT" |> System.get_env("40002") |> String.to_integer(), max_passive_port: "MAX_PASSIVE_PORT" |> System.get_env("40007") |> String.to_integer(),