fix(pairing): fix ownership_voucher_controller errors (#1904)

mizzet1 · AliouneGaye21 · commit e146b5972682 · 2026-04-03T18:51:28.000+02:00
Signed-off-by: Riccardo Nalgi &lt;riccardo.nalgi@secomind.com&gt;
Signed-off-by: Alioune Gaye &lt;alioune.gaye@secomind.com&gt;
diff --git a/apps/astarte_pairing/lib/astarte_pairing/queries.ex b/apps/astarte_pairing/lib/astarte_pairing/queries.ex
@@ -251,7 +251,8 @@ defmodule Astarte.Pairing.Queries do
     consistency = Consistency.device_info(:write)
     opts = [prefix: keyspace, consistency: consistency]
 
-    %OwnershipVoucher{guid: guid, output_voucher: new_voucher}
+    %OwnershipVoucher{guid: guid}
+    |> Ecto.Changeset.change(output_voucher: new_voucher)
     |> Repo.update(opts)
   end
 
diff --git a/apps/astarte_pairing/lib/astarte_pairing_web/controllers/owner_key_controller.ex b/apps/astarte_pairing/lib/astarte_pairing_web/controllers/owner_key_controller.ex
@@ -55,7 +55,7 @@ defmodule Astarte.PairingWeb.OwnerKeyController do
         }
       ) do
     with {:ok, keys} <-
-           Secrets.Core.get_keys_from_algorithm(realm_name, @supported_key_algorithms) do
+           Secrets.Core.get_keys(realm_name, @supported_key_algorithms) do
       send_resp(conn, 200, Jason.encode!(keys))
     end
   end
@@ -64,7 +64,8 @@ defmodule Astarte.PairingWeb.OwnerKeyController do
         "realm_name" => realm_name,
         "key_algorithm" => key_algorithm
       }) do
-    with {:ok, keys} <- Secrets.Core.get_keys_from_algorithm(realm_name, key_algorithm) do
+    with {:ok, algorithm} <- Secrets.Core.string_to_key_type(key_algorithm),
+         {:ok, keys} <- Secrets.Core.get_keys(realm_name, [algorithm]) do
       json(conn, %{data: keys})
     end
   end
diff --git a/apps/astarte_pairing/lib/astarte_pairing_web/controllers/ownership_voucher_controller.ex b/apps/astarte_pairing/lib/astarte_pairing_web/controllers/ownership_voucher_controller.ex
@@ -19,45 +19,13 @@
 defmodule Astarte.PairingWeb.OwnershipVoucherController do
   use Astarte.PairingWeb, :controller
 
-  alias Astarte.FDO.Core.OwnershipVoucher.CreateRequest
   alias Astarte.FDO.OwnershipVoucher
   alias Astarte.FDO.OwnershipVoucher.LoadRequest
   alias Astarte.FDO.TO0
   alias Astarte.Secrets.Core, as: SecretsCore
 
   action_fallback Astarte.PairingWeb.FallbackController
 
-  def create(conn, %{
-        "data" => data,
-        "realm_name" => realm_name
-      }) do
-    create = CreateRequest.changeset(%CreateRequest{}, data)
-
-    with {:ok, create} <- Ecto.Changeset.apply_action(create, :insert),
-         %CreateRequest{
-           decoded_ownership_voucher: decoded_ownership_voucher,
-           cbor_ownership_voucher: cbor_ownership_voucher,
-           private_key: private_key,
-           extracted_private_key: extracted_private_key,
-           device_guid: device_guid
-         } = create,
-         :ok <-
-           OwnershipVoucher.save_voucher(
-             realm_name,
-             cbor_ownership_voucher,
-             device_guid,
-             private_key
-           ),
-         :ok <-
-           TO0.claim_ownership_voucher(
-             realm_name,
-             decoded_ownership_voucher,
-             extracted_private_key
-           ) do
-      send_resp(conn, 200, "")
-    end
-  end
-
   @doc """
   Validates an FDO Ownership Voucher load request and register the OV in the database.
 
@@ -72,8 +40,17 @@ defmodule Astarte.PairingWeb.OwnershipVoucherController do
              voucher_data: req.cbor_ownership_voucher,
              guid: req.device_guid,
              key_name: req.key_name,
-             key_algorithm: req.key_algorithm
-           }) do
+             key_algorithm: req.key_algorithm,
+             replacement_guid: req.replacement_guid,
+             replacement_rendezvous_info: req.decoded_replacement_rendezvous_info,
+             replacement_public_key: req.decoded_replacement_public_key
+           }),
+         :ok <-
+           TO0.claim_ownership_voucher(
+             realm_name,
+             req.decoded_ownership_voucher,
+             req.extracted_owner_key
+           ) do
       json(conn, %{
         data: %{
           public_key: req.extracted_owner_key.public_pem,
@@ -93,7 +70,7 @@ defmodule Astarte.PairingWeb.OwnershipVoucherController do
     with {:ok, pem} <- ensure_ownership_voucher_parameter(data),
          {:ok, voucher} <- OwnershipVoucher.decode_binary_voucher(pem),
          key_algorithm = OwnershipVoucher.key_algorithm(voucher),
-         {:ok, keys_map} <- SecretsCore.get_keys_from_algorithm(realm_name, key_algorithm) do
+         {:ok, keys_map} <- SecretsCore.get_keys(realm_name, key_algorithm) do
       json(conn, %{data: keys_map})
     end
   end
diff --git a/apps/astarte_pairing/test/astarte_pairing_web/controllers/owner_key_controller_test.exs b/apps/astarte_pairing/test/astarte_pairing_web/controllers/owner_key_controller_test.exs
@@ -206,19 +206,17 @@ defmodule Astarte.PairingWeb.Controllers.OwnerKeyControllerTest do
 
       keys = Jason.decode!(keys)
 
-      assert keys == [
-               %{
-                 "es256" => [
-                   "key_to_create",
-                   "key_to_create1",
-                   "key_to_create2",
-                   "key_to_create3"
-                 ]
-               },
-               %{"es384" => []},
-               %{"rs256" => []},
-               %{"rs384" => []}
-             ]
+      assert keys == %{
+               "es256" => [
+                 "key_to_create",
+                 "key_to_create1",
+                 "key_to_create2",
+                 "key_to_create3"
+               ],
+               "es384" => [],
+               "rs256" => [],
+               "rs384" => []
+             }
     end
   end
 
diff --git a/apps/astarte_pairing/test/astarte_pairing_web/controllers/ownership_voucher_controller_test.exs b/apps/astarte_pairing/test/astarte_pairing_web/controllers/ownership_voucher_controller_test.exs
@@ -174,7 +174,7 @@ defmodule Astarte.PairingWeb.Controllers.OwnershipVoucherControllerTest do
       assert %{"es256" => [@sample_key_name, "another_key"]} = get_in(body, ["data"])
     end
 
-    test "returns 200 with an empty list when no keys are registered", context do
+    test "returns 200 with an empty key list when no keys are registered", context do
       %{auth_conn: conn, path: path, realm_name: realm_name} = context
 
       stub(Secrets, :create_namespace, fn ^realm_name, :es256 ->
diff --git a/apps/astarte_pairing/test/support/helpers/fdo.ex b/apps/astarte_pairing/test/support/helpers/fdo.ex
@@ -24,11 +24,11 @@ defmodule Astarte.Helpers.FDO do
   import StreamData
 
   alias Astarte.DataAccess.FDO.OwnershipVoucher, as: DBOwnershipVoucher
-  alias Astarte.FDO.Core.OwnershipVoucher.CreateRequest
   alias Astarte.DataAccess.Realms.Realm
   alias Astarte.DataAccess.Repo
   alias Astarte.FDO.Core.Hash
   alias Astarte.FDO.Core.OwnershipVoucher
+  alias Astarte.FDO.Core.OwnershipVoucher.CreateRequest
   alias Astarte.FDO.Core.OwnershipVoucher.RendezvousInfo
   alias Astarte.FDO.Core.OwnershipVoucher.RendezvousInfo.RendezvousDirective
   alias Astarte.FDO.Core.OwnershipVoucher.RendezvousInfo.RendezvousInstr
diff --git a/apps/astarte_pairing/test/test_helper.exs b/apps/astarte_pairing/test/test_helper.exs
@@ -28,6 +28,7 @@ Mimic.copy(Astarte.FDO.OwnerOnboarding.DeviceAttestation)
 Mimic.copy(Astarte.FDO.OwnershipVoucher)
 Mimic.copy(Astarte.FDO.Rendezvous)
 Mimic.copy(Astarte.FDO.Rendezvous.Client)
+Mimic.copy(Astarte.FDO.TO0)
 Mimic.copy(Astarte.FDO.ServiceInfo)
 Mimic.copy(Astarte.Pairing.Config)
 Mimic.copy(Astarte.Pairing.Queries)
diff --git a/libs/astarte_data_access/lib/astarte_data_access/fdo/ownership_voucher.ex b/libs/astarte_data_access/lib/astarte_data_access/fdo/ownership_voucher.ex
@@ -38,8 +38,8 @@ defmodule Astarte.DataAccess.FDO.OwnershipVoucher do
     field :key_name, :string
     field :key_algorithm, Ecto.Enum, values: [es256: 0, es384: 1, rs256: 10, rs384: 11]
     field :replacement_guid, :binary
-    field :replacement_rv_info, CBOREncoded, using: RendezvousInfo
-    field :replacement_pub_key, CBOREncoded, using: PublicKey
+    field :replacement_rendezvous_info, CBOREncoded, using: RendezvousInfo
+    field :replacement_public_key, CBOREncoded, using: PublicKey
   end
 
   @doc false
@@ -51,8 +51,8 @@ defmodule Astarte.DataAccess.FDO.OwnershipVoucher do
       :guid,
       :key_algorithm,
       :replacement_guid,
-      :replacement_rv_info,
-      :replacement_pub_key
+      :replacement_rendezvous_info,
+      :replacement_public_key
     ])
     |> validate_required([:key_name, :key_algorithm, :voucher_data, :guid])
   end
diff --git a/libs/astarte_fdo/lib/owner_onboarding.ex b/libs/astarte_fdo/lib/owner_onboarding.ex
@@ -134,20 +134,21 @@ defmodule Astarte.FDO.OwnerOnboarding do
   def prove_device(realm_name, body, session) do
     guid = session.guid
 
-    # TODO: credential reuse requires also Owner2Key and/or rv info to be changed
-    # for credential reuse; so far, there is no API to do so, so it-s limited to the guid
-
     with {:ok, ownership_voucher} <- OwnershipVoucher.fetch(realm_name, guid),
+         {:ok, ov_entry} <- fetch_ov_entry(realm_name, guid),
          {:ok, owner_key} <- Secrets.get_key_for_guid(realm_name, guid),
          {:ok, owner_public_key} <- OwnershipVoucher.owner_public_key(ownership_voucher) do
-      rendezvous_info = ownership_voucher.header.rendezvous_info
+      next_guid = ov_entry.replacement_guid || guid
+
+      next_rv_info =
+        ov_entry.replacement_rendezvous_info || ownership_voucher.header.rendezvous_info
 
-      # {:ok, private_key} = COSE.Keys.from_pem(private_key)
+      next_owner_pub_key = ov_entry.replacement_public_key || owner_public_key
 
       connection_credentials = %{
-        guid: guid,
-        rendezvous_info: rendezvous_info,
-        owner_pub_key: owner_public_key,
+        guid: next_guid,
+        rendezvous_info: next_rv_info,
+        owner_pub_key: next_owner_pub_key,
         owner_private_key: owner_key,
         device_info: "owned by astarte - realm #{realm_name}.#{Config.base_url_domain!()}"
       }
@@ -252,11 +253,13 @@ defmodule Astarte.FDO.OwnerOnboarding do
   end
 
   defp maybe_replace_voucher(realm_name, to2_session) do
-    if not OwnershipVoucher.credential_reuse?(to2_session) do
-      with {:ok, old_voucher} <-
-             OwnershipVoucher.fetch(realm_name, to2_session.guid),
+    {:ok, ov_entry} = fetch_ov_entry(realm_name, to2_session.guid)
+
+    if not OwnershipVoucher.credential_reuse?(ov_entry) do
+      with {:ok, old_voucher} <- OwnershipVoucher.fetch(realm_name, to2_session.guid),
+           # Passiamo sia la ov_entry (per le chiavi) che to2_session (per l'HMAC)
            {:ok, new_voucher} <-
-             OwnershipVoucher.generate_replacement_voucher(old_voucher, to2_session) do
+             OwnershipVoucher.generate_replacement_voucher(old_voucher, ov_entry, to2_session) do
         cbor_voucher = CoreOwnershipVoucher.cbor_encode(new_voucher)
 
         Queries.add_output_voucher(
@@ -292,4 +295,15 @@ defmodule Astarte.FDO.OwnerOnboarding do
   defp build_done2_message(setup_dv_nonce) do
     %Done2Payload{:nonce_to2_setup_dv => setup_dv_nonce} |> Done2Payload.encode()
   end
+
+  defp fetch_ov_entry(realm_name, guid) do
+    keyspace = Astarte.DataAccess.Realms.Realm.keyspace_name(realm_name)
+
+    case Astarte.DataAccess.Repo.get(Astarte.DataAccess.FDO.OwnershipVoucher, guid,
+           prefix: keyspace
+         ) do
+      nil -> {:error, :not_found}
+      entry -> {:ok, entry}
+    end
+  end
 end
diff --git a/libs/astarte_fdo/lib/owner_onboarding/session.ex b/libs/astarte_fdo/lib/owner_onboarding/session.ex
@@ -34,8 +34,6 @@ defmodule Astarte.FDO.Core.OwnerOnboarding.Session do
   alias Astarte.FDO.Core.OwnerOnboarding.Session
   alias Astarte.FDO.Core.OwnerOnboarding.SessionKey
   alias Astarte.FDO.Core.OwnerOnboarding.SignatureInfo
-  alias Astarte.FDO.Core.OwnershipVoucher.RendezvousInfo
-  alias Astarte.FDO.Core.PublicKey
   alias Astarte.FDO.OwnerOnboarding.SessionToken
   alias COSE.Messages.Encrypt0
 
@@ -59,9 +57,6 @@ defmodule Astarte.FDO.Core.OwnerOnboarding.Session do
     field :device_service_info, map() | nil
     field :owner_service_info, [binary()] | nil
     field :last_chunk_sent, non_neg_integer() | nil
-    field :replacement_guid, binary() | nil
-    field :replacement_rv_info, RendezvousInfo.t() | nil
-    field :replacement_pub_key, PublicKey.t() | nil
     field :replacement_hmac, Hash.t() | nil
   end
 
@@ -294,9 +289,6 @@ defmodule Astarte.FDO.Core.OwnerOnboarding.Session do
         device_service_info: device_service_info,
         owner_service_info: owner_service_info,
         last_chunk_sent: last_chunk_sent,
-        # replacement_guid: replacement_guid,
-        # replacement_rv_info: replacement_rv_info,
-        # replacement_pub_key: replacement_pub_key,
         replacement_hmac: replacement_hmac
       } = database_session
 
@@ -319,9 +311,6 @@ defmodule Astarte.FDO.Core.OwnerOnboarding.Session do
         device_service_info: device_service_info,
         owner_service_info: owner_service_info,
         last_chunk_sent: last_chunk_sent,
-        # replacement_guid: replacement_guid,
-        # replacement_rv_info: replacement_rv_info,
-        # replacement_pub_key: replacement_pub_key,
         replacement_hmac: replacement_hmac
       }
 
diff --git a/libs/astarte_fdo/lib/ownership_voucher/load_request.ex b/libs/astarte_fdo/lib/ownership_voucher/load_request.ex
@@ -87,6 +87,7 @@ defmodule Astarte.FDO.OwnershipVoucher.LoadRequest do
         :error -> [replacement_guid: "is not valid base64"]
       end
     end)
+    |> decode_replacement_fields()
   end
 
   defp validate_replacement_rendezvous_info(changeset) do
@@ -101,6 +102,61 @@ defmodule Astarte.FDO.OwnershipVoucher.LoadRequest do
     end)
   end
 
+  defp decode_replacement_fields(%Ecto.Changeset{valid?: false} = changeset), do: changeset
+
+  defp decode_replacement_fields(changeset) do
+    changeset
+    |> decode_replacement_guid()
+    |> decode_replacement_rendezvous_info()
+    |> decode_replacement_public_key()
+  end
+
+  defp decode_replacement_guid(changeset) do
+    case fetch_change(changeset, :replacement_guid) do
+      {:ok, b64} ->
+        case Base.decode64(b64) do
+          {:ok, bin} -> put_change(changeset, :replacement_guid, bin)
+          :error -> add_error(changeset, :replacement_guid, "is not valid base64")
+        end
+
+      :error ->
+        changeset
+    end
+  end
+
+  defp decode_replacement_rendezvous_info(changeset) do
+    case fetch_change(changeset, :replacement_rendezvous_info) do
+      {:ok, b64} ->
+        with {:ok, cbor_bin} <- Base.decode64(b64),
+             {:ok, rv_info} <- RendezvousInfo.decode_cbor(cbor_bin) do
+          put_change(changeset, :decoded_replacement_rendezvous_info, rv_info)
+        else
+          _ ->
+            add_error(
+              changeset,
+              :replacement_rendezvous_info,
+              "is not valid base64-encoded CBOR rendezvous info"
+            )
+        end
+
+      :error ->
+        changeset
+    end
+  end
+
+  defp decode_replacement_public_key(changeset) do
+    case fetch_change(changeset, :replacement_public_key) do
+      {:ok, pem_string} ->
+        case public_key_from_pem(pem_string) do
+          {:ok, public_key} -> put_change(changeset, :decoded_replacement_public_key, public_key)
+          :error -> add_error(changeset, :replacement_public_key, "is not a valid PEM public key")
+        end
+
+      :error ->
+        changeset
+    end
+  end
+
   defp put_device_guid(%Ecto.Changeset{valid?: false} = changeset), do: changeset
 
   defp put_device_guid(changeset) do
@@ -139,7 +195,11 @@ defmodule Astarte.FDO.OwnershipVoucher.LoadRequest do
         if key_algorithm in valid_algorithms do
           changeset
         else
-          add_error(changeset, :key_algorithm, "is not compatible with the ownership voucher's key type")
+          add_error(
+            changeset,
+            :key_algorithm,
+            "is not compatible with the ownership voucher's key type"
+          )
         end
 
       {:error, _} ->
@@ -229,14 +289,12 @@ defmodule Astarte.FDO.OwnershipVoucher.LoadRequest do
 
   # Extract the SubjectPublicKeyInfo DER bytes embedded in an X.509 certificate.
   defp spki_der_from_cert(cert_der) do
-    try do
-      {:Certificate, {:TBSCertificate, _, _, _, _, _, _, spki, _, _, _}, _, _} =
-        :public_key.pkix_decode_cert(cert_der, :plain)
+    {:Certificate, {:TBSCertificate, _, _, _, _, _, _, spki, _, _, _}, _, _} =
+      :public_key.pkix_decode_cert(cert_der, :plain)
 
-      {:ok, :public_key.der_encode(:SubjectPublicKeyInfo, spki)}
-    rescue
-      _ -> :error
-    end
+    {:ok, :public_key.der_encode(:SubjectPublicKeyInfo, spki)}
+  rescue
+    _ -> :error
   end
 
   # EC public key: pem_entry_decode yields {{:ECPoint, <<4,x,y>>}, {:namedCurve, oid}}
diff --git a/libs/astarte_fdo/lib/ownership_voucher/ownership_voucher.ex b/libs/astarte_fdo/lib/ownership_voucher/ownership_voucher.ex
diff --git a/libs/astarte_fdo/test/astarte_fdo/onboarding/done_test.exs b/libs/astarte_fdo/test/astarte_fdo/onboarding/done_test.exs
diff --git a/libs/astarte_fdo/test/astarte_fdo/owner_onboarding/owner_onboarding_test.exs b/libs/astarte_fdo/test/astarte_fdo/owner_onboarding/owner_onboarding_test.exs
diff --git a/libs/astarte_secrets/lib/astarte_secrets/core.ex b/libs/astarte_secrets/lib/astarte_secrets/core.ex
diff --git a/libs/astarte_secrets/test/astarte_secrets/core_test.exs b/libs/astarte_secrets/test/astarte_secrets/core_test.exs
