feat(pairing): allow key uploading (#1874)

Allow using Astarte as proxy to upload keys in OpenBao.
Endpoint: POST /v1/:realm_name/fdo/owner_key
Payload params: action="upload" / key_name / key_data

Controller tests added

Signed-off-by: frossq <francesco.asquini@secomind.com>

Author: frossq

apps/astarte_pairing/lib/astarte_pairing_web/controllers/fallback_controller.ex

@@ -78,6 +78,22 @@ defmodule Astarte.PairingWeb.FallbackController do
     |> render(:invalid_auth_path)
   end
 
+  # The format of the uploaded (FDO) key is not the expected one
+  def call(conn, {:error, :unprocessable_key}) do
+    conn
+    |> put_status(:unprocessable_entity)
+    |> put_view(Astarte.PairingWeb.ErrorView)
+    |> render(:unprocessable_key)
+  end
+
+  # A key with the same name has already been imported in OpenBao
+  def call(conn, {:error, :key_already_imported}) do
+    conn
+    |> put_status(:conflict)
+    |> put_view(Astarte.PairingWeb.ErrorView)
+    |> render(:key_already_imported)
+  end
+
   # This is called when no JWT token is present
   def auth_error(conn, {:unauthenticated, :unauthenticated}, _opts) do
     conn

apps/astarte_pairing/lib/astarte_pairing_web/controllers/owner_key_controller.ex

@@ -40,9 +40,10 @@ defmodule Astarte.PairingWeb.OwnerKeyController do
 
     with {:ok, create_or_upload_changeset} <-
            Ecto.Changeset.apply_action(create_or_upload_changeset, :insert),
-         {:ok, public_key} <-
+         {:ok, resp} <-
            OwnerKeyInitialization.create_or_upload(create_or_upload_changeset, realm_name) do
-      send_resp(conn, 200, public_key)
+      # the successful resp will be a public key (when creating) or an empty string (when uploading)
+      send_resp(conn, 200, resp)
     end
   end
 

apps/astarte_pairing/lib/astarte_pairing_web/views/error_view.ex

@@ -71,6 +71,22 @@ defmodule Astarte.PairingWeb.ErrorView do
     }
   end
 
+  def render("unprocessable_key.json", _assigns) do
+    %{
+      errors: %{
+        detail: "The provided key format is invalid or cannot be processed."
+      }
+    }
+  end
+
+  def render("key_already_imported.json", _assigns) do
+    %{
+      errors: %{
+        detail: "A key with this name has already been imported"
+      }
+    }
+  end
+
   # In case no render clause matches or no
   # template is found, let's render it as 500
   def template_not_found(_template, assigns) do

apps/astarte_pairing/test/astarte_pairing_web/controllers/owner_key_controller_test.exs

@@ -26,16 +26,17 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
   alias Astarte.Secrets.Key
   alias Astarte.Secrets.OwnerKeyInitialization
   alias Astarte.Secrets.OwnerKeyInitializationOptions
+  alias COSE.Keys
 
   setup :verify_on_exit!
 
-  describe "/fdo/owner_key" do
+  describe "POST /fdo/owner_key" do
     setup :owner_key_setup
 
     test "rejects the request if no correct key action is specified", context do
       %{
         auth_conn: conn,
-        create_path: path,
+        owner_key_path: path,
         create_key_payload: payload
       } = context
 
@@ -50,7 +51,7 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
     test "rejects the request if no algorithm is specified while creating key", context do
       %{
         auth_conn: conn,
-        create_path: path,
+        owner_key_path: path,
         create_key_payload: payload
       } = context
 
@@ -65,7 +66,7 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
          context do
       %{
         auth_conn: conn,
-        create_path: path,
+        owner_key_path: path,
         create_key_payload: payload
       } = context
 
@@ -80,11 +81,13 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
     test "creates in OpenBao a key of the chosen type (EC256)", context do
       %{
         auth_conn: conn,
-        create_path: path,
+        owner_key_path: path,
         create_key_payload: payload,
         openbao_namespace: namespace
       } = context
 
+      on_exit(fn -> cleanup_keys(namespace) end)
+
       public_key_created =
         conn
         |> post(path, payload)
@@ -96,8 +99,85 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
       assert public_key_created == public_key_retrieved
     end
 
+    test "rejects the request if no key data is specified while uploading key", context do
+      %{
+        auth_conn: conn,
+        owner_key_path: path,
+        upload_key_payload: payload
+      } = context
+
+      {_, payload_no_key_data} = pop_in(payload, [:data, :key_data])
+
+      conn
+      |> post(path, payload_no_key_data)
+      |> response(422)
+    end
+
+    test "rejects the request if an invalid key body is passed while uploading key",
+         context do
+      %{
+        auth_conn: conn,
+        owner_key_path: path,
+        upload_key_payload: payload
+      } = context
+
+      payload_wrong_key_data =
+        update_in(payload, [:data, :key_data], fn _ -> "invalid_key_body" end)
+
+      conn
+      |> post(path, payload_wrong_key_data)
+      |> response(422)
+    end
+
+    test "uploads in OpenBao a key of the chosen type (EC256)", context do
+      %{
+        auth_conn: conn,
+        owner_key_path: path,
+        upload_key_payload: payload,
+        openbao_namespace: namespace
+      } = context
+
+      on_exit(fn -> cleanup_keys(namespace) end)
+
+      conn
+      |> post(path, payload)
+      |> response(200)
+
+      assert {:ok, %Key{public_pem: public_key_retrieved}} =
+               Secrets.get_key(payload[:data][:key_name], namespace: namespace)
+
+      # we load a private key PEM, OpenBao returns a public key PEM. Need to compare them
+      assert private_pem_public_pem_match?(payload[:data][:key_data], public_key_retrieved)
+    end
+
+    test "returns a warning if a key with the same name has been already imported in OpenBao",
+         context do
+      %{
+        auth_conn: conn,
+        owner_key_path: path,
+        upload_key_payload: payload,
+        openbao_namespace: namespace
+      } = context
+
+      on_exit(fn -> cleanup_keys(namespace) end)
+
+      payload_duplicated_key =
+        update_in(payload, [:data, :key_name], fn _ -> "duplicated_key" end)
+
+      conn
+      |> post(path, payload_duplicated_key)
+      |> response(200)
+
+      resp_message =
+        conn
+        |> post(path, payload_duplicated_key)
+        |> response(409)
+
+      assert resp_message =~ "has already been imported"
+    end
+
     test "returns a 404 error if FDO feature is disabled", context do
-      %{auth_conn: conn, create_path: path, create_key_payload: payload} = context
+      %{auth_conn: conn, owner_key_path: path, create_key_payload: payload} = context
 
       stub(Config, :enable_fdo!, fn -> false end)
 
@@ -117,6 +197,8 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
         list_path: path
       } = context
 
+      on_exit(fn -> cleanup_keys(namespace) end)
+
       keys =
         conn
         |> get(path)
@@ -142,7 +224,7 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
 
   defp owner_key_setup(context) do
     %{auth_conn: conn, realm_name: realm_name} = context
-    create_path = owner_key_path(conn, :create_or_upload_key, realm_name)
+    owner_key_path = owner_key_path(conn, :create_or_upload_key, realm_name)
 
     create_key_payload = %{
       data: %{
@@ -152,11 +234,20 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
       }
     }
 
+    upload_key_payload = %{
+      data: %{
+        action: "upload",
+        key_name: "key_to_upload",
+        key_data: Keys.ECC.generate(:es256) |> Keys.to_pem()
+      }
+    }
+
     {:ok, namespace_es256} = Secrets.create_namespace(realm_name, :es256)
 
     %{
-      create_path: create_path,
+      owner_key_path: owner_key_path,
       create_key_payload: create_key_payload,
+      upload_key_payload: upload_key_payload,
       openbao_namespace: namespace_es256
     }
   end
@@ -202,4 +293,31 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
 
     %{list_path: list_path, openbao_namespace: namespace_es256}
   end
+
+  # simple version, works only for EC keys
+  # TODO implement using COSE functions
+  defp private_pem_public_pem_match?(private_pem, public_pem) do
+    private_pem_decoded = decode_pem(private_pem) |> extract_public()
+    public_pem_decoded = decode_pem(public_pem)
+
+    private_pem_decoded == public_pem_decoded
+  end
+
+  defp decode_pem(pem_string) do
+    [entry] = :public_key.pem_decode(pem_string)
+    :public_key.pem_entry_decode(entry)
+  end
+
+  defp extract_public({:ECPrivateKey, _version, _priv, params, pub_bytes, _attributes}) do
+    {{:ECPoint, pub_bytes}, params}
+  end
+
+  defp cleanup_keys(namespace) do
+    {:ok, keys_to_delete} = Secrets.list_keys_names(namespace: namespace)
+
+    Enum.each(keys_to_delete, fn key ->
+      Secrets.enable_key_deletion(key, namespace: namespace)
+      Secrets.delete_key(key, namespace: namespace)
+    end)
+  end
 end

libs/astarte_secrets/lib/astarte_secrets/core.ex

@@ -181,6 +181,14 @@ defmodule Astarte.Secrets.Core do
       {:ok, %HTTPoison.Response{status_code: 204}} ->
         :ok
 
+      {:ok, %Response{status_code: 500, body: body}} = resp ->
+        if body =~ "import path cannot be used with an existing key" do
+          {:error, :key_already_imported}
+        else
+          Logger.error("Encountered HTTP error while importing key #{key_name}: #{inspect(resp)}")
+          :error
+        end
+
       error_resp ->
         Logger.error(
           "Encountered HTTP error while importing key #{key_name}: #{inspect(error_resp)}"
@@ -275,7 +283,7 @@ defmodule Astarte.Secrets.Core do
       {:ok, %HTTPoison.Response{status_code: 200, body: resp_body}} ->
         case parse_data_key(resp_body, "keys") do
           {:ok, keys} ->
-            {:ok, keys}
+            filter_real_keys(keys)
 
           {:error, reason} ->
             Logger.error("Encountered error while getting keys list: #{inspect(reason)}")
@@ -517,4 +525,11 @@ defmodule Astarte.Secrets.Core do
       %{key_algorithm => keys}
     end
   end
+
+  defp filter_real_keys(keys) do
+    # drop the "import/" element that (if present) is listed as a key
+    # but is actually an operational endpoint
+    keys = Enum.reject(keys, fn key -> key == "import/" end)
+    {:ok, keys}
+  end
 end

libs/astarte_secrets/lib/astarte_secrets/owner_key_initialization.ex

@@ -25,6 +25,7 @@ defmodule Astarte.Secrets.OwnerKeyInitialization do
   alias Astarte.Secrets
   alias Astarte.Secrets.Core
   alias Astarte.Secrets.OwnerKeyInitializationOptions
+  alias COSE.Keys
 
   def create_or_upload(
         %OwnerKeyInitializationOptions{
@@ -36,10 +37,28 @@ defmodule Astarte.Secrets.OwnerKeyInitialization do
       ) do
     {:ok, key_algorithm} = Core.string_to_key_type(key_algorithm)
     {:ok, namespace} = Secrets.create_namespace(realm_name, key_algorithm)
-
     do_create_key(key_name, key_algorithm, namespace)
   end
 
+  def create_or_upload(
+        %OwnerKeyInitializationOptions{
+          action: "upload",
+          key_name: key_name,
+          key_data: key_data
+        },
+        realm_name
+      ) do
+    case Keys.from_pem(key_data) do
+      {:ok, decoded_key_data} ->
+        key_algorithm = decoded_key_data.alg
+        {:ok, namespace} = Secrets.create_namespace(realm_name, key_algorithm)
+        do_upload_key(key_name, key_algorithm, decoded_key_data, namespace)
+
+      :error ->
+        {:error, :unprocessable_key}
+    end
+  end
+
   defp do_create_key(key_name, key_algorithm, namespace) do
     with {:ok, key_data} <-
            Secrets.create_keypair(key_name, key_algorithm, namespace: namespace) do
@@ -48,4 +67,10 @@ defmodule Astarte.Secrets.OwnerKeyInitialization do
       {:ok, public_key}
     end
   end
+
+  defp do_upload_key(key_name, key_algorithm, key_body, namespace) do
+    with :ok <- Secrets.import_key(key_name, key_algorithm, key_body, namespace: namespace) do
+      {:ok, ""}
+    end
+  end
 end

libs/astarte_secrets/test/astarte_secrets/core_test.exs

@@ -269,6 +269,14 @@ defmodule Astarte.Secrets.CoreTest do
       stored_pem = key_data.public_pem |> String.trim_trailing()
       assert expected_pub_pem == stored_pem
     end
+
+    test "returns error if the key has already been imported", %{unique_id: uid, opts: opts} do
+      key_name = "imported_ec256_#{uid}"
+      ec_key = ECC.generate(:es256)
+
+      assert :ok = Secrets.import_key(key_name, :es256, ec_key, opts)
+      assert {:error, :key_already_imported} = Secrets.import_key(key_name, :es256, ec_key, opts)
+    end
   end
 
   defp http_stubs_setup(_context) do