Validation of CA certificate inside SUSE Manager settings (#2581)

* Add x509 to dependencies * Validate CA certificate * Fix tests * Migrate SUSE Manager integration settings to single table inheritance * Add tests for certificate validation * Move some case statement directly to pattern matching in signatures * Address review comments
trento-project · May 6, 2024 · 88cb7a7 · 88cb7a7
1 parent 46e17e2
commit 88cb7a7
Show file tree

Hide file tree

Showing 8 changed files with 177 additions and 56 deletions.
diff --git 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
@@ -4,22 +4,25 @@ defmodule Trento.SoftwareUpdates.Settings do
   """
 
   use Ecto.Schema
+  use Trento.Support.Ecto.STI, sti_identifier: :suse_manager_settings
 
   import Ecto.Changeset
 
   alias Trento.Support.DateService
 
   @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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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