Skip to content

Commit

Permalink
Use plain string certificates (#2596)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
nelsonkopliku authored May 7, 2024
1 parent 88cb7a7 commit 5ba49e7
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 105 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,42 @@ 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()}

@callback get_system_id(
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()}

@callback get_relevant_patches(
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()}

@callback get_upgradable_packages(
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,
Expand All @@ -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
7 changes: 2 additions & 5 deletions lib/trento/infrastructure/software_updates/auth/state.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,23 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.State do
:username,
:password,
:ca_cert,
:auth,
use_ca_cert: false
:auth
]

@type t :: %{
url: String.t() | nil,
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: "<REDACTED>",
auth: "<REDACTED>",
ca_cert: "<REDACTED>"
Expand Down
21 changes: 2 additions & 19 deletions lib/trento/infrastructure/software_updates/auth/suma_auth.ex
Original file line number Diff line number Diff line change
Expand Up @@ -78,35 +78,18 @@ 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
| url: url,
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
12 changes: 6 additions & 6 deletions lib/trento/infrastructure/software_updates/suma.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
38 changes: 17 additions & 21 deletions lib/trento/infrastructure/software_updates/suma_api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do
username: nil,
password: nil,
ca_cert: nil,
use_ca_cert: false,
auth: nil
}

Expand All @@ -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
Expand All @@ -101,7 +72,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do
username: username,
password: "<REDACTED>",
ca_cert: "<REDACTED>",
use_ca_cert: true,
auth: "<REDACTED>"
}

Expand Down Expand Up @@ -153,7 +123,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do
username: nil,
password: nil,
ca_cert: nil,
use_ca_cert: false,
auth: nil
}

Expand Down Expand Up @@ -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"
}

Expand All @@ -235,7 +203,6 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Auth.SumaAuthTest do
username: nil,
password: nil,
ca_cert: nil,
use_ca_cert: false,
auth: nil
}

Expand Down
Loading

0 comments on commit 5ba49e7

Please sign in to comment.