diff --git a/lib/trento/software_updates.ex b/lib/trento/software_updates.ex index 62f0dd6f68..3c01e181cc 100644 --- a/lib/trento/software_updates.ex +++ b/lib/trento/software_updates.ex @@ -26,12 +26,12 @@ defmodule Trento.SoftwareUpdates do @spec get_settings :: {:ok, Settings.t()} | {:error, :settings_not_configured} def get_settings do - case has_settings?(settings = Repo.one!(Settings)) do - true -> - {:ok, settings} + settings = Repo.one(Settings.base_query()) - false -> - {:error, :settings_not_configured} + if settings do + {:ok, settings} + else + {:error, :settings_not_configured} end end @@ -61,17 +61,7 @@ defmodule Trento.SoftwareUpdates do @spec clear_settings :: :ok def clear_settings do - {1, _} = - Repo.update_all(Settings, - set: [ - url: nil, - username: nil, - password: nil, - ca_cert: nil, - ca_uploaded_at: nil, - updated_at: DateTime.utc_now() - ] - ) + Repo.delete_all(Settings.base_query()) Discovery.clear_software_updates_discoveries() @@ -123,25 +113,30 @@ defmodule Trento.SoftwareUpdates do end end - defp has_settings?(%Settings{url: url, username: username, password: password}), - do: url != nil and username != nil and password != nil - defp ensure_no_settings_configured do - case has_settings?(settings = Repo.one!(Settings)) do - false -> - {:ok, :settings_not_configured, settings} + case Repo.one(Settings.base_query()) do + nil -> + {:ok, :settings_not_configured, nil} - true -> + %Settings{} -> Logger.error("Error: software updates settings already configured") {:error, :settings_already_configured} end end - defp save_or_update_settings(%Settings{} = settings, settings_submission, date_service) do + defp save_or_update_settings(settings, settings_submission, date_service) do result = - settings - |> Settings.changeset(settings_submission, date_service) - |> Repo.update() + case settings do + nil -> + %Settings{} + |> Settings.changeset(settings_submission, date_service) + |> Repo.insert() + + %Settings{} -> + settings + |> Settings.changeset(settings_submission, date_service) + |> Repo.update() + end case result do {:ok, _} = success -> diff --git a/lib/trento/software_updates/settings.ex b/lib/trento/software_updates/settings.ex index 02e21dda55..e4ee1ef2d9 100644 --- a/lib/trento/software_updates/settings.ex +++ b/lib/trento/software_updates/settings.ex @@ -4,6 +4,7 @@ defmodule Trento.SoftwareUpdates.Settings do """ use Ecto.Schema + use Trento.Support.Ecto.STI, sti_identifier: :suse_manager_settings import Ecto.Changeset @@ -11,15 +12,17 @@ defmodule Trento.SoftwareUpdates.Settings do @type t :: %__MODULE__{} - @primary_key {:id, :binary_id, autogenerate: false} - schema "software_update_settings" do - field :url, :string - field :username, :string - field :password, Trento.Support.Ecto.EncryptedBinary - field :ca_cert, Trento.Support.Ecto.EncryptedBinary - field :ca_uploaded_at, :utc_datetime_usec + @derive {Jason.Encoder, except: [:__meta__, :__struct__]} + @primary_key {:id, :binary_id, autogenerate: true} + schema "settings" do + field :url, :string, source: :suse_manager_settings_url + field :username, :string, source: :suse_manager_settings_username + field :password, Trento.Support.Ecto.EncryptedBinary, source: :suse_manager_settings_password + field :ca_cert, Trento.Support.Ecto.EncryptedBinary, source: :suse_manager_settings_ca_cert + field :ca_uploaded_at, :utc_datetime_usec, source: :suse_manager_settings_ca_uploaded_at timestamps(type: :utc_datetime_usec) + sti_fields() end @spec changeset(t() | Ecto.Changeset.t(), map) :: Ecto.Changeset.t() @@ -30,7 +33,8 @@ defmodule Trento.SoftwareUpdates.Settings do |> validate_change(:url, &validate_url/2) |> maybe_validate_ca_cert(attrs) |> maybe_change_cert_upload_date(attrs, date_service) - |> unique_constraint(:id, name: :software_update_settings_pkey) + |> sti_changes() + |> unique_constraint(:type) end defp validate_url(_url_atom, url) do @@ -43,11 +47,50 @@ defmodule Trento.SoftwareUpdates.Settings do end end - defp maybe_validate_ca_cert(changeset, settings_submission) do - if nil != Map.get(settings_submission, :ca_cert) do - validate_required(changeset, :ca_cert) + defp maybe_validate_ca_cert(changeset, %{ca_cert: nil}), do: changeset + + defp maybe_validate_ca_cert(changeset, %{ca_cert: _ca_cert}) do + changeset + |> validate_required(:ca_cert) + |> validate_change(:ca_cert, &validate_ca_cert/2) + end + + defp maybe_validate_ca_cert(changeset, _), do: changeset + + defp validate_ca_cert(_ca_cert_atom, ca_cert) do + with {:ok, certificate} <- parse_ca_cert(ca_cert), + :ok <- ca_cert_valid?(certificate) do + [] else - changeset + {:error, errors} -> errors + end + end + + defp parse_ca_cert(ca_cert) do + case X509.Certificate.from_pem(ca_cert) do + {:ok, _} = result -> + result + + _ -> + {:error, [ca_cert: {"unable to parse X.509 certificate", validation: :ca_cert_parsing}]} + end + rescue + _ -> + # We discovered that an exception is thrown when attempting to parse invalid strings + # wrapped in valid headers like "foobar" + # https://github.com/trento-project/web/pull/2581#pullrequestreview-2040240059 + {:error, [ca_cert: {"unable to parse X.509 certificate", validation: :ca_cert_parsing}]} + end + + defp ca_cert_valid?(certificate) do + now = DateTime.utc_now() + {:Validity, never_before, never_after} = X509.Certificate.validity(certificate) + + if never_before |> X509.DateTime.to_datetime() |> DateTime.before?(now) and + never_after |> X509.DateTime.to_datetime() |> DateTime.after?(now) do + :ok + else + {:error, [ca_cert: {"the X.509 certificate is not valid", validation: :ca_cert_validity}]} end end diff --git a/mix.exs b/mix.exs index b4e9beddc6..f850f667c1 100644 --- a/mix.exs +++ b/mix.exs @@ -109,7 +109,8 @@ defmodule Trento.MixProject do {:ecto, "~> 3.10", override: true}, # https://github.com/deadtrickster/ssl_verify_fun.erl/pull/27 {:ssl_verify_fun, "~> 1.1", manager: :rebar3, override: true}, - {:parallel_stream, "~> 1.1.0"} + {:parallel_stream, "~> 1.1.0"}, + {:x509, "~> 0.8.8"} ] end diff --git a/mix.lock b/mix.lock index d20647db49..402e1ed65c 100644 --- a/mix.lock +++ b/mix.lock @@ -94,4 +94,5 @@ "tzdata": {:hex, :tzdata, "1.1.1", "20c8043476dfda8504952d00adac41c6eda23912278add38edc140ae0c5bcc46", [:mix], [{:hackney, "~> 1.17", [hex: :hackney, repo: "hexpm", optional: false]}], "hexpm", "a69cec8352eafcd2e198dea28a34113b60fdc6cb57eb5ad65c10292a6ba89787"}, "unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"}, "unplug": {:hex, :unplug, "1.0.0", "8ec2479de0baa9a6283c04a1cc616c5ca6c5b80b8ff1d857481bb2943368dbbc", [:mix], [{:plug, "~> 1.8", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "d171a85758aa412d4e85b809c203e1b1c4c76a4d6ab58e68dc9a8a8acd9b7c3a"}, + "x509": {:hex, :x509, "0.8.8", "aaf5e58b19a36a8e2c5c5cff0ad30f64eef5d9225f0fd98fb07912ee23f7aba3", [:mix], [], "hexpm", "ccc3bff61406e5bb6a63f06d549f3dba3a1bbb456d84517efaaa210d8a33750f"}, } diff --git a/priv/repo/migrations/20240502105156_migrate_suma_settings_sti.exs b/priv/repo/migrations/20240502105156_migrate_suma_settings_sti.exs new file mode 100644 index 0000000000..cb08cb835d --- /dev/null +++ b/priv/repo/migrations/20240502105156_migrate_suma_settings_sti.exs @@ -0,0 +1,15 @@ +defmodule Trento.Repo.Migrations.MigrateSumaSettingsSti do + use Ecto.Migration + + def change do + alter table(:settings) do + add :suse_manager_settings_url, :string + add :suse_manager_settings_username, :string + add :suse_manager_settings_password, :binary + add :suse_manager_settings_ca_cert, :binary + add :suse_manager_settings_ca_uploaded_at, :utc_datetime_usec + end + + drop table(:software_update_settings) + end +end diff --git a/test/support/factory.ex b/test/support/factory.ex index ca6be74628..9acf14ae63 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -822,14 +822,17 @@ defmodule Trento.Factory do end def software_updates_settings_factory(attrs) do + self_signed_cert = build(:self_signed_certificate) + url = Map.get(attrs, :url, Faker.Internet.url()) username = Map.get(attrs, :username, Faker.Internet.user_name()) password = Map.get(attrs, :password, Faker.Lorem.word()) - ca_cert = Map.get(attrs, :ca_cert, Faker.Lorem.sentence()) + ca_cert = Map.get(attrs, :ca_cert, self_signed_cert) ca_uploaded_at = Map.get(attrs, :ca_uploaded_at, DateTime.utc_now()) %Settings{} |> Settings.changeset(%{ + type: :suse_manager_settings, url: url, username: username, password: password, @@ -860,7 +863,7 @@ defmodule Trento.Factory do insert( :software_updates_settings, attrs, - conflict_target: :id, + conflict_target: :type, on_conflict: :replace_all ) end @@ -918,4 +921,15 @@ defmodule Trento.Factory do ]) } end + + def self_signed_certificate_factory(attrs) do + validity = Map.get(attrs, :validity, 500) + + 2048 + |> X509.PrivateKey.new_rsa() + |> X509.Certificate.self_signed("/C=US/ST=NT/L=Springfield/O=ACME Inc./CN=Intermediate CA", + validity: validity + ) + |> X509.Certificate.to_pem() + end end diff --git a/test/trento/software_updates_test.exs b/test/trento/software_updates_test.exs index b054da7ca5..1fe6987089 100644 --- a/test/trento/software_updates_test.exs +++ b/test/trento/software_updates_test.exs @@ -50,7 +50,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do ca_uploaded_at: ca_uploaded_at } = insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() ) @@ -172,7 +172,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do url: url = "https://valid.com", username: username = Faker.Internet.user_name(), password: password = Faker.Lorem.word(), - ca_cert: ca_cert = Faker.Lorem.sentence() + ca_cert: ca_cert = build(:self_signed_certificate) } assert {:ok, @@ -221,7 +221,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do url: "https://valid.com", username: Faker.Internet.user_name(), password: Faker.Lorem.word(), - ca_cert: Faker.Lorem.sentence() + ca_cert: build(:self_signed_certificate) } assert {:ok, _} = operation.(settings) @@ -358,7 +358,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do change_submission = %{ url: new_url = "https://new.com", - ca_cert: new_ca_cert = "new_ca_cert" + ca_cert: new_ca_cert = build(:self_signed_certificate) } assert {:ok, @@ -381,7 +381,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do ca_uploaded_at: _initial_ca_uploaded_at } = insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() ) @@ -395,7 +395,7 @@ defmodule Trento.SoftwareUpdates.SettingsTest do change_submission = %{ url: new_url = "https://new.com", - ca_cert: new_ca_cert = "new_ca_cert" + ca_cert: new_ca_cert = build(:self_signed_certificate) } Enum.each(1..3, fn run_iteration -> @@ -445,6 +445,55 @@ defmodule Trento.SoftwareUpdates.SettingsTest do ca_uploaded_at: nil }} = SoftwareUpdates.change_settings(change_submission) end + + test "should reject an invalid SSL certificate" do + insert_software_updates_settings(ca_cert: nil) + + change_submission = %{ + ca_cert: Faker.Lorem.word() + } + + assert {:error, + %{ + errors: [ + ca_cert: {"unable to parse X.509 certificate", [validation: :ca_cert_parsing]} + ] + }} = SoftwareUpdates.change_settings(change_submission) + end + + test "should reject a 'foobar' SSL certificate" do + insert_software_updates_settings(ca_cert: nil) + + change_submission = %{ + ca_cert: """ + -----BEGIN CERTIFICATE----- + foobar + -----END CERTIFICATE----- + """ + } + + assert {:error, + %{ + errors: [ + ca_cert: {"unable to parse X.509 certificate", [validation: :ca_cert_parsing]} + ] + }} = SoftwareUpdates.change_settings(change_submission) + end + + test "should reject an expired SSL certificate" do + insert_software_updates_settings(ca_cert: nil) + + change_submission = %{ + ca_cert: build(:self_signed_certificate, validity: 0) + } + + assert {:error, + %{ + errors: [ + ca_cert: {"the X.509 certificate is not valid", [validation: :ca_cert_validity]} + ] + }} = SoftwareUpdates.change_settings(change_submission) + end end describe "clearing software update settings" do diff --git a/test/trento_web/controllers/v1/suma_credentials_controller_test.exs b/test/trento_web/controllers/v1/suma_credentials_controller_test.exs index f6e3a04313..59b1fb075f 100644 --- a/test/trento_web/controllers/v1/suma_credentials_controller_test.exs +++ b/test/trento_web/controllers/v1/suma_credentials_controller_test.exs @@ -16,7 +16,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do describe "retrieve user settings" do test "should return user settings", %{conn: conn} do insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() ) @@ -45,7 +45,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do url: Faker.Internet.image_url(), username: Faker.Internet.user_name(), password: Faker.Lorem.word(), - ca_cert: Faker.Lorem.sentence() + ca_cert: build(:self_signed_certificate) } %{"ca_uploaded_at" => ca_uploaded_at} = @@ -64,7 +64,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do url: "http://insecureurl.com", username: Faker.Internet.user_name(), password: Faker.Lorem.word(), - ca_cert: Faker.Lorem.sentence() + ca_cert: build(:self_signed_certificate) } resp = @@ -119,7 +119,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do url: Faker.Internet.image_url(), username: Faker.Internet.user_name(), password: Faker.Lorem.word(), - ca_cert: Faker.Lorem.sentence() + ca_cert: build(:self_signed_certificate) } resp = @@ -341,7 +341,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do ca_uploaded_at: initial_ca_uploaded_at } = insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() ) @@ -374,11 +374,14 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do ca_uploaded_at: initial_ca_uploaded_at } = insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() ) - change_submission = %{url: new_url = "https://new.com", ca_cert: "new_ca_cert"} + change_submission = %{ + url: new_url = "https://new.com", + ca_cert: build(:self_signed_certificate) + } resp = conn @@ -402,7 +405,7 @@ defmodule TrentoWeb.V1.SUMACredentialsControllerTest do ca_uploaded_at: _initial_ca_uploaded_at } = insert_software_updates_settings( - ca_cert: Faker.Lorem.sentence(), + ca_cert: build(:self_signed_certificate), ca_uploaded_at: DateTime.utc_now() )