From 5ba49e7801d6521721063a6e7636eb50f0b53c10 Mon Sep 17 00:00:00 2001 From: Nelson Kopliku Date: Tue, 7 May 2024 12:05:57 +0200 Subject: [PATCH] Use plain string certificates (#2596) * Use plain string certificates in suma http executor * Refactor suma auth to not read/write certificate from filesystem * Refactor Suma modules to use plain string certificates * Adjust default ca_cert arguments --- .../adapter/suma_http_executor.ex | 54 ++++++++++++------- .../software_updates/auth/state.ex | 7 +-- .../software_updates/auth/suma_auth.ex | 21 +------- .../infrastructure/software_updates/suma.ex | 12 ++--- .../software_updates/suma_api.ex | 38 ++++++------- .../software_updates/auth/suma_auth_test.exs | 33 ------------ .../software_updates/suma_test.exs | 1 - 7 files changed, 61 insertions(+), 105 deletions(-) diff --git a/lib/trento/infrastructure/software_updates/adapter/suma_http_executor.ex b/lib/trento/infrastructure/software_updates/adapter/suma_http_executor.ex index 4a14d0dc4f..1f9eb15b2a 100644 --- a/lib/trento/infrastructure/software_updates/adapter/suma_http_executor.ex +++ b/lib/trento/infrastructure/software_updates/adapter/suma_http_executor.ex @@ -3,13 +3,11 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do SUMA Http requests executor """ - alias Trento.Infrastructure.SoftwareUpdates.SumaApi - @callback login( base_url :: String.t(), username :: String.t(), password :: String.t(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, HTTPoison.Response.t()} | {:error, HTTPoison.Error.t()} @@ -17,7 +15,7 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do base_url :: String.t(), auth :: String.t(), fully_qualified_domain_name :: String.t(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, HTTPoison.Response.t()} | {:error, HTTPoison.Error.t()} @@ -25,7 +23,7 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do base_url :: String.t(), auth :: String.t(), system_id :: pos_integer(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, HTTPoison.Response.t()} | {:error, HTTPoison.Error.t()} @@ -33,14 +31,14 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do base_url :: String.t(), auth :: String.t(), system_id :: pos_integer(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, HTTPoison.Response.t()} | {:error, HTTPoison.Error.t()} @behaviour Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor @impl true - def login(base_url, username, password, use_ca_cert \\ false) do + def login(base_url, username, password, ca_cert) do payload = Jason.encode!(%{ "login" => username, @@ -51,44 +49,60 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do "#{base_url}/auth/login", payload, [{"Content-type", "application/json"}], - ssl_options(use_ca_cert) ++ timeout_options() + ssl_options(ca_cert) ++ timeout_options() ) end @impl true - def get_system_id(base_url, auth, fully_qualified_domain_name, use_ca_cert \\ false) do + def get_system_id(base_url, auth, fully_qualified_domain_name, ca_cert) do HTTPoison.get( "#{base_url}/system/getId?name=#{fully_qualified_domain_name}", [{"Content-type", "application/json"}], - request_options(auth, use_ca_cert) + request_options(auth, ca_cert) ) end @impl true - def get_relevant_patches(base_url, auth, system_id, use_ca_cert \\ false) do + def get_relevant_patches(base_url, auth, system_id, ca_cert) do HTTPoison.get( "#{base_url}/system/getRelevantErrata?sid=#{system_id}", [{"Content-type", "application/json"}], - request_options(auth, use_ca_cert) + request_options(auth, ca_cert) ) end @impl true - def get_upgradable_packages(base_url, auth, system_id, use_ca_cert \\ false) do + def get_upgradable_packages(base_url, auth, system_id, ca_cert) do HTTPoison.get( "#{base_url}/system/listLatestUpgradablePackages?sid=#{system_id}", [{"Content-type", "application/json"}], - request_options(auth, use_ca_cert) + request_options(auth, ca_cert) ) end - defp request_options(auth, use_ca_cert), - do: [hackney: [cookie: [auth]]] ++ ssl_options(use_ca_cert) ++ timeout_options() + defp request_options(auth, ca_cert), + do: [hackney: [cookie: [auth]]] ++ ssl_options(ca_cert) ++ timeout_options() + + defp timeout_options, do: [timeout: 1_000, recv_timeout: 1_500] + + defp ssl_options(nil), do: [] - defp timeout_options, do: [timeout: 500, recv_timeout: 1_500] + defp ssl_options(ca_cert), + do: [ssl: [verify: :verify_peer, cacerts: [get_cert_der(ca_cert)]]] - defp ssl_options(true), - do: [ssl: [verify: :verify_peer, cacertfile: SumaApi.ca_cert_path()]] + def get_cert_der(ca_cert) do + {cert_type, cert_entry} = + ca_cert + |> :public_key.pem_decode() + |> hd() + |> :public_key.pem_entry_decode() + |> split_type_and_entry() - defp ssl_options(_), do: [] + :public_key.der_encode(cert_type, cert_entry) + end + + def split_type_and_entry(ans1_entry) do + ans1_type = elem(ans1_entry, 0) + {ans1_type, ans1_entry} + end end diff --git a/lib/trento/infrastructure/software_updates/auth/state.ex b/lib/trento/infrastructure/software_updates/auth/state.ex index 9b06dda5cd..d2fb6b65a2 100644 --- a/lib/trento/infrastructure/software_updates/auth/state.ex +++ b/lib/trento/infrastructure/software_updates/auth/state.ex @@ -9,8 +9,7 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.State do :username, :password, :ca_cert, - :auth, - use_ca_cert: false + :auth ] @type t :: %{ @@ -18,17 +17,15 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.State do username: String.t() | nil, password: String.t() | nil, ca_cert: String.t() | nil, - use_ca_cert: boolean(), auth: String.t() | nil } defimpl Inspect, for: State do - def inspect(%State{url: url, username: username, use_ca_cert: use_ca_cert}, opts) do + def inspect(%State{url: url, username: username}, opts) do Inspect.Map.inspect( %{ url: url, username: username, - use_ca_cert: use_ca_cert, password: "", auth: "", ca_cert: "" diff --git a/lib/trento/infrastructure/software_updates/auth/suma_auth.ex b/lib/trento/infrastructure/software_updates/auth/suma_auth.ex index 06ae88e5a8..32676edb27 100644 --- a/lib/trento/infrastructure/software_updates/auth/suma_auth.ex +++ b/lib/trento/infrastructure/software_updates/auth/suma_auth.ex @@ -78,8 +78,7 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuth do defp setup_auth(%State{auth: nil} = state) do with {:ok, %{url: url, username: username, password: password, ca_cert: ca_cert}} <- SoftwareUpdates.get_settings(), - :ok <- write_ca_cert_file(ca_cert), - {:ok, auth_cookie} <- SumaApi.login(url, username, password, ca_cert != nil) do + {:ok, auth_cookie} <- SumaApi.login(url, username, password, ca_cert) do {:ok, %State{ state @@ -87,26 +86,10 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuth do username: username, password: password, ca_cert: ca_cert, - auth: auth_cookie, - use_ca_cert: ca_cert != nil + auth: auth_cookie }} end end defp setup_auth(%State{} = state), do: {:ok, state} - - defp write_ca_cert_file(nil) do - case File.rm_rf(SumaApi.ca_cert_path()) do - {:ok, _} -> :ok - _ -> :error - end - end - - defp write_ca_cert_file(ca_cert) do - SumaApi.ca_cert_path() - |> Path.dirname() - |> File.mkdir_p!() - - File.write(SumaApi.ca_cert_path(), ca_cert) - end end diff --git a/lib/trento/infrastructure/software_updates/suma.ex b/lib/trento/infrastructure/software_updates/suma.ex index 7c780ea1e3..c2a2d369a7 100644 --- a/lib/trento/infrastructure/software_updates/suma.ex +++ b/lib/trento/infrastructure/software_updates/suma.ex @@ -56,23 +56,23 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma do defp do_handle({:get_system_id, fully_qualified_domain_name}, %State{ url: url, auth: auth_cookie, - use_ca_cert: use_ca_cert + ca_cert: ca_cert }), - do: SumaApi.get_system_id(url, auth_cookie, fully_qualified_domain_name, use_ca_cert) + do: SumaApi.get_system_id(url, auth_cookie, fully_qualified_domain_name, ca_cert) defp do_handle({:get_relevant_patches, system_id}, %State{ url: url, auth: auth_cookie, - use_ca_cert: use_ca_cert + ca_cert: ca_cert }), - do: SumaApi.get_relevant_patches(url, auth_cookie, system_id, use_ca_cert) + do: SumaApi.get_relevant_patches(url, auth_cookie, system_id, ca_cert) defp do_handle({:get_upgradable_packages, system_id}, %State{ url: url, auth: auth_cookie, - use_ca_cert: use_ca_cert + ca_cert: ca_cert }), - do: SumaApi.get_upgradable_packages(url, auth_cookie, system_id, use_ca_cert) + do: SumaApi.get_upgradable_packages(url, auth_cookie, system_id, ca_cert) defp auth, do: Application.fetch_env!(:trento, __MODULE__)[:auth] end diff --git a/lib/trento/infrastructure/software_updates/suma_api.ex b/lib/trento/infrastructure/software_updates/suma_api.ex index 0a641ea067..b098215922 100644 --- a/lib/trento/infrastructure/software_updates/suma_api.ex +++ b/lib/trento/infrastructure/software_updates/suma_api.ex @@ -8,35 +8,31 @@ defmodule Trento.Infrastructure.SoftwareUpdates.SumaApi do @login_retries 5 - @ca_cert_path "/tmp/suma_ca_cert.crt" - - def ca_cert_path, do: @ca_cert_path - @spec login( url :: String.t(), username :: String.t(), password :: String.t(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, any()} | {:error, :max_login_retries_reached | any()} - def login(url, username, password, use_ca_cert), + def login(url, username, password, ca_cert), do: url |> get_suma_api_url() - |> try_login(username, password, use_ca_cert, @login_retries) + |> try_login(username, password, ca_cert, @login_retries) @spec get_system_id( url :: String.t(), auth :: any(), fully_qualified_domain_name :: String.t(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, pos_integer()} | {:error, :system_id_not_found | :authentication_error} - def get_system_id(url, auth, fully_qualified_domain_name, use_ca_cert) do + def get_system_id(url, auth, fully_qualified_domain_name, ca_cert) do response = url |> get_suma_api_url() - |> http_executor().get_system_id(auth, fully_qualified_domain_name, use_ca_cert) + |> http_executor().get_system_id(auth, fully_qualified_domain_name, ca_cert) with {:ok, %HTTPoison.Response{status_code: 200, body: body}} <- response, {:ok, %{success: true, result: result}} <- Jason.decode(body, keys: :atoms), @@ -59,15 +55,15 @@ defmodule Trento.Infrastructure.SoftwareUpdates.SumaApi do url :: String.t(), auth :: any(), system_id :: pos_integer(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, [map()]} | {:error, :error_getting_patches | :authentication_error} - def get_relevant_patches(url, auth, system_id, use_ca_cert) do + def get_relevant_patches(url, auth, system_id, ca_cert) do response = url |> get_suma_api_url() - |> http_executor().get_relevant_patches(auth, system_id, use_ca_cert) + |> http_executor().get_relevant_patches(auth, system_id, ca_cert) with {:ok, %HTTPoison.Response{status_code: 200, body: body}} <- response, {:ok, %{success: true, result: result}} <- Jason.decode(body, keys: :atoms) do @@ -90,14 +86,14 @@ defmodule Trento.Infrastructure.SoftwareUpdates.SumaApi do url :: String.t(), auth :: any(), system_id :: pos_integer(), - use_ca_cert :: boolean() + ca_cert :: String.t() | nil ) :: {:ok, [map()]} | {:error, :error_getting_packages | :authentication_error} - def get_upgradable_packages(url, auth, system_id, use_ca_cert) do + def get_upgradable_packages(url, auth, system_id, ca_cert) do url |> get_suma_api_url() - |> http_executor().get_upgradable_packages(auth, system_id, use_ca_cert) + |> http_executor().get_upgradable_packages(auth, system_id, ca_cert) |> handle_auth_error() |> decode_response( error_atom: :error_getting_packages, @@ -136,19 +132,19 @@ defmodule Trento.Infrastructure.SoftwareUpdates.SumaApi do {:error, :max_login_retries_reached} end - defp try_login(url, username, password, use_ca_cert, retry) do - case do_login(url, username, password, use_ca_cert) do + defp try_login(url, username, password, ca_cert, retry) do + case do_login(url, username, password, ca_cert) do {:ok, _} = successful_login -> successful_login {:error, reason} -> Logger.error("Failed to Log into SUSE Manager, retrying...", error: inspect(reason)) - try_login(url, username, password, use_ca_cert, retry - 1) + try_login(url, username, password, ca_cert, retry - 1) end end - defp do_login(url, username, password, use_ca_cert) do - case http_executor().login(url, username, password, use_ca_cert) do + defp do_login(url, username, password, ca_cert) do + case http_executor().login(url, username, password, ca_cert) do {:ok, %HTTPoison.Response{headers: headers, status_code: 200} = response} -> Logger.debug("Successfully logged into suma #{inspect(response)}") {:ok, get_session_cookies(headers)} diff --git a/test/trento/infrastructure/software_updates/auth/suma_auth_test.exs b/test/trento/infrastructure/software_updates/auth/suma_auth_test.exs index 55be744b5e..94ef1f6ad6 100644 --- a/test/trento/infrastructure/software_updates/auth/suma_auth_test.exs +++ b/test/trento/infrastructure/software_updates/auth/suma_auth_test.exs @@ -41,7 +41,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do username: nil, password: nil, ca_cert: nil, - use_ca_cert: false, auth: nil } @@ -55,34 +54,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do setup_initial_settings() end - test "should save existing CA certificate to local file", %{ - settings: %Settings{ca_cert: ca_cert} - } do - assert {:ok, _} = start_supervised({SumaAuth, @test_integration_name}) - - expect(SumaApiMock, :login, fn _, _, _, true -> successful_login_response() end) - - assert {:ok, %State{ca_cert: ^ca_cert}} = SumaAuth.authenticate(@test_integration_name) - - cert_file_path = "/tmp/suma_ca_cert.crt" - - assert File.exists?(cert_file_path) - ^ca_cert = File.read!(cert_file_path) - end - - test "should not save CA certificate file if no cert is provided" do - insert_software_updates_settings(ca_cert: nil, ca_uploaded_at: nil) - - assert {:ok, _} = start_supervised({SumaAuth, @test_integration_name}) - - expect(SumaApiMock, :login, fn _, _, _, false -> successful_login_response() end) - - assert {:ok, %State{ca_cert: nil}} = - SumaAuth.authenticate(@test_integration_name) - - refute File.exists?("/tmp/suma_ca_cert.crt") - end - test "should redact sensitive data in SUMA state", %{ settings: %Settings{url: url, username: username, password: password} } do @@ -101,7 +72,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do username: username, password: "", ca_cert: "", - use_ca_cert: true, auth: "" } @@ -153,7 +123,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do username: nil, password: nil, ca_cert: nil, - use_ca_cert: false, auth: nil } @@ -214,7 +183,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do username: username, password: password, ca_cert: ca_cert, - use_ca_cert: true, auth: "pxt-session-cookie=4321" } @@ -235,7 +203,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do username: nil, password: nil, ca_cert: nil, - use_ca_cert: false, auth: nil } diff --git a/test/trento/infrastructure/software_updates/suma_test.exs b/test/trento/infrastructure/software_updates/suma_test.exs index 61900041e9..3bc32a3709 100644 --- a/test/trento/infrastructure/software_updates/suma_test.exs +++ b/test/trento/infrastructure/software_updates/suma_test.exs @@ -233,7 +233,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.SumaTest do username: "user", password: "password", ca_cert: "cert", - use_ca_cert: true, auth: "cookie" } end