Allow external ownership of CHIPDeviceController operational key (#18482

) * Allow external ownership of CHIPDeviceController operational key - For controllers that make use of their own key management, such as OS-aided key management, it is currently impossible to pass-in the Operational Key pair to use for a given controller. This PR adds APIs to avoid FabricTable from trying to manage the lifecycle of the operational key, and allow the caller/initializer of CHIPDeviceControllerFactory to properly inject its own operational key that it manages, without attempts of storage or dynamic memory management. Issue #18444 Issue #7695 Testing done: - Unit tests still pass, cert tests as well - This was tested internally by us using the APIs to manage a key for Android. That change will be a follow-up * Restyled by clang-format * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Address review comments * Restyled by clang-format * Address review comments Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jan 3, 2024 · 1407004 · 1407004
1 parent e71ddad
commit 1407004
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 39 deletions.
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -127,7 +127,7 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     mVendorId = params.controllerVendorId;
     if (params.operationalKeypair != nullptr || !params.controllerNOC.empty() || !params.controllerRCAC.empty())
     {
-        ReturnErrorOnFailure(ProcessControllerNOCChain(params));
+        ReturnErrorOnFailure(InitControllerNOCChain(params));
 
         if (params.enableServerInteractions)
         {
@@ -144,15 +144,23 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParams & params)
+CHIP_ERROR DeviceController::InitControllerNOCChain(const ControllerInitParams & params)
 {
     FabricInfo newFabric;
     constexpr uint32_t chipCertAllocatedLen = kMaxCHIPCertLength;
     chip::Platform::ScopedMemoryBuffer<uint8_t> chipCert;
     Credentials::P256PublicKeySpan rootPublicKey;
     FabricId fabricId;
 
-    ReturnErrorOnFailure(newFabric.SetOperationalKeypair(params.operationalKeypair));
+    if (params.hasExternallyOwnedOperationalKeypair)
+    {
+        ReturnErrorOnFailure(newFabric.SetExternallyOwnedOperationalKeypair(params.operationalKeypair));
+    }
+    else
+    {
+        ReturnErrorOnFailure(newFabric.SetOperationalKeypair(params.operationalKeypair));
+    }
+
     newFabric.SetVendorId(params.controllerVendorId);
 
     ReturnErrorCodeIf(!chipCert.Alloc(chipCertAllocatedLen), CHIP_ERROR_NO_MEMORY);

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -95,6 +95,13 @@ struct ControllerInitParams
        controllerNOC. It's used by controller to establish CASE sessions with devices */
     Crypto::P256Keypair * operationalKeypair = nullptr;
 
+    /**
+     * Controls whether or not the operationalKeypair should be owned by the caller.
+     * By default, this is false, but if the keypair cannot be serialized, then
+     * setting this to true will allow you to manage this keypair's lifecycle.
+     */
+    bool hasExternallyOwnedOperationalKeypair = false;
+
     /* The following certificates must be in x509 DER format */
     ByteSpan controllerNOC;
     ByteSpan controllerICAC;
@@ -240,6 +247,19 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
      */
     CHIP_ERROR DisconnectDevice(NodeId nodeId);
 
+    /**
+     * @brief
+     *   Reconfigures a new set of operational credentials to be used with this
+     *   controller given ControllerInitParams state.
+     *
+     * WARNING: This is a low-level method that should only be called directly
+     *          if you know exactly how this will interact with controller state,
+     *          since there are several integrations that do this call for you.
+     *          It can be used for fine-grained dependency injection of a controller's
+     *          NOC and operational keypair.
+     */
+    CHIP_ERROR InitControllerNOCChain(const ControllerInitParams & params);
+
 protected:
     enum class State
     {
@@ -275,8 +295,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
 
 private:
     void ReleaseOperationalDevice(OperationalDeviceProxy * device);
-
-    CHIP_ERROR ProcessControllerNOCChain(const ControllerInitParams & params);
 };
 
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY

diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp
@@ -94,20 +94,6 @@ CHIP_ERROR FabricInfo::CommitToStorage(PersistentStorageDelegate * storage)
         storage->SyncSetKeyValue(keyAlloc.FabricNOC(mFabricIndex), mNOCCert.data(), static_cast<uint16_t>(mNOCCert.size())));
 
     {
-        Crypto::P256SerializedKeypair serializedOpKey;
-        if (mOperationalKey != nullptr)
-        {
-            ReturnErrorOnFailure(mOperationalKey->Serialize(serializedOpKey));
-        }
-        else
-        {
-            // Could we just not store it instead?  What would deserialize need
-            // to do then?
-            P256Keypair keypair;
-            ReturnErrorOnFailure(keypair.Initialize());
-            ReturnErrorOnFailure(keypair.Serialize(serializedOpKey));
-        }
-
         uint8_t buf[OpKeyTLVMaxSize()];
         TLV::TLVWriter writer;
         writer.Init(buf);
@@ -117,7 +103,14 @@ CHIP_ERROR FabricInfo::CommitToStorage(PersistentStorageDelegate * storage)
 
         ReturnErrorOnFailure(writer.Put(kOpKeyVersionTag, kOpKeyVersion));
 
-        ReturnErrorOnFailure(writer.Put(kOpKeyDataTag, ByteSpan(serializedOpKey.Bytes(), serializedOpKey.Length())));
+        // If key storage is externally managed, key is not stored here,
+        // and when loading is done later, it will be ignored.
+        if (!mHasExternallyOwnedOperationalKey && (mOperationalKey != nullptr))
+        {
+            Crypto::P256SerializedKeypair serializedOpKey;
+            ReturnErrorOnFailure(mOperationalKey->Serialize(serializedOpKey));
+            ReturnErrorOnFailure(writer.Put(kOpKeyDataTag, ByteSpan(serializedOpKey.Bytes(), serializedOpKey.Length())));
+        }
 
         ReturnErrorOnFailure(writer.EndContainer(outerType));
 
@@ -213,27 +206,36 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
         ReturnErrorOnFailure(reader.Get(opKeyVersion));
         VerifyOrReturnError(opKeyVersion == kOpKeyVersion, CHIP_ERROR_VERSION_MISMATCH);
 
-        ReturnErrorOnFailure(reader.Next(kOpKeyDataTag));
-        ByteSpan keyData;
-        ReturnErrorOnFailure(reader.GetByteView(keyData));
+        CHIP_ERROR err = reader.Next(kOpKeyDataTag);
+        if (err == CHIP_NO_ERROR)
+        {
+            ByteSpan keyData;
+            ReturnErrorOnFailure(reader.GetByteView(keyData));
 
-        // Unfortunately, we have to copy the data into a P256SerializedKeypair.
-        Crypto::P256SerializedKeypair serializedOpKey;
-        VerifyOrReturnError(keyData.size() <= serializedOpKey.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
+            // Unfortunately, we have to copy the data into a P256SerializedKeypair.
+            Crypto::P256SerializedKeypair serializedOpKey;
+            VerifyOrReturnError(keyData.size() <= serializedOpKey.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-        memcpy(serializedOpKey.Bytes(), keyData.data(), keyData.size());
-        serializedOpKey.SetLength(keyData.size());
+            memcpy(serializedOpKey.Bytes(), keyData.data(), keyData.size());
+            serializedOpKey.SetLength(keyData.size());
 
-        if (mOperationalKey == nullptr)
-        {
+            if (mOperationalKey == nullptr)
+            {
 #ifdef ENABLE_HSM_CASE_OPS_KEY
-            mOperationalKey = chip::Platform::New<P256KeypairHSM>();
+                mOperationalKey = chip::Platform::New<P256KeypairHSM>();
 #else
-            mOperationalKey = chip::Platform::New<P256Keypair>();
+                mOperationalKey = chip::Platform::New<P256Keypair>();
 #endif
+            }
+            VerifyOrReturnError(mOperationalKey != nullptr, CHIP_ERROR_NO_MEMORY);
+            ReturnErrorOnFailure(mOperationalKey->Deserialize(serializedOpKey));
+        }
+        else
+        {
+            // Key was absent: set mOperationalKey to null, for another caller to set
+            // it. This may happen if externally owned.
+            mOperationalKey = nullptr;
         }
-        VerifyOrReturnError(mOperationalKey != nullptr, CHIP_ERROR_NO_MEMORY);
-        ReturnErrorOnFailure(mOperationalKey->Deserialize(serializedOpKey));
 
         ReturnErrorOnFailure(reader.ExitContainer(containerType));
         ReturnErrorOnFailure(reader.VerifyEndOfContainer());
@@ -321,6 +323,9 @@ CHIP_ERROR FabricInfo::DeleteFromStorage(PersistentStorageDelegate * storage, Fa
 CHIP_ERROR FabricInfo::SetOperationalKeypair(const P256Keypair * keyPair)
 {
     VerifyOrReturnError(keyPair != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+
+    mHasExternallyOwnedOperationalKey = false;
+
     P256SerializedKeypair serialized;
     ReturnErrorOnFailure(keyPair->Serialize(serialized));
     if (mOperationalKey == nullptr)
@@ -335,6 +340,20 @@ CHIP_ERROR FabricInfo::SetOperationalKeypair(const P256Keypair * keyPair)
     return mOperationalKey->Deserialize(serialized);
 }
 
+CHIP_ERROR FabricInfo::SetExternallyOwnedOperationalKeypair(P256Keypair * keyPair)
+{
+    VerifyOrReturnError(keyPair != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    if (!mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
+    {
+        chip::Platform::Delete(mOperationalKey);
+        mOperationalKey = nullptr;
+    }
+
+    mHasExternallyOwnedOperationalKey = true;
+    mOperationalKey                   = keyPair;
+    return CHIP_NO_ERROR;
+}
+
 void FabricInfo::ReleaseCert(MutableByteSpan & cert)
 {
     if (cert.data() != nullptr)
@@ -565,7 +584,15 @@ CHIP_ERROR FabricInfo::SetFabricInfo(FabricInfo & newFabric)
     ReturnErrorOnFailure(VerifyCredentials(newFabric.mNOCCert, newFabric.mICACert, newFabric.mRootCert, validContext, operationalId,
                                            fabricId, pubkey));
 
-    SetOperationalKeypair(newFabric.GetOperationalKey());
+    if (newFabric.mHasExternallyOwnedOperationalKey)
+    {
+        ReturnErrorOnFailure(SetExternallyOwnedOperationalKeypair(newFabric.GetOperationalKey()));
+    }
+    else
+    {
+        ReturnErrorOnFailure(SetOperationalKeypair(newFabric.GetOperationalKey()));
+    }
+
     SetRootCert(newFabric.mRootCert);
     mOperationalId = operationalId;
     mFabricId      = fabricId;

diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h
@@ -74,7 +74,7 @@ class DLL_EXPORT FabricInfo
 
     ~FabricInfo()
     {
-        if (mOperationalKey != nullptr)
+        if (!mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
         {
             chip::Platform::Delete(mOperationalKey);
         }
@@ -111,8 +111,9 @@ class DLL_EXPORT FabricInfo
 
     Crypto::P256Keypair * GetOperationalKey() const
     {
-        if (mOperationalKey == nullptr)
+        if (!mHasExternallyOwnedOperationalKey && (mOperationalKey == nullptr))
         {
+            // TODO: Refactor the following two cases to go through SetOperationalKey()
 #ifdef ENABLE_HSM_CASE_OPS_KEY
             mOperationalKey = chip::Platform::New<Crypto::P256KeypairHSM>();
             mOperationalKey->CreateOperationalKey(mFabricIndex);
@@ -123,8 +124,33 @@ class DLL_EXPORT FabricInfo
         }
         return mOperationalKey;
     }
+
+    /**
+     * Sets the P256Keypair used for this fabric.  This will make a copy of the keypair
+     * via the P256Keypair::Serialize and P256Keypair::Deserialize methods.
+     *
+     * The keyPair argument is safe to deallocate once this method returns.
+     *
+     * If your P256Keypair does not support serialization, use the
+     * `SetExternallyOwnedOperationalKeypair` method instead.
+     */
     CHIP_ERROR SetOperationalKeypair(const Crypto::P256Keypair * keyPair);
 
+    /**
+     * Sets the P256Keypair used for this fabric, delegating ownership of the
+     * key to the caller. The P256Keypair provided here must be freed later by
+     * the caller of this method if it was allocated dynamically.
+     *
+     * This should be used if your P256Keypair does not support serialization
+     * and deserialization (e.g. your private key is held in a secure element
+     * and cannot be accessed directly), or if you back your operational
+     * private keys by external implementation of the cryptographic interfaces.
+     *
+     * To have the ownership of the key managed for you, use
+     * SetOperationalKeypair instead.
+     */
+    CHIP_ERROR SetExternallyOwnedOperationalKeypair(Crypto::P256Keypair * keyPair);
+
     // TODO - Update these APIs to take ownership of the buffer, instead of copying
     //        internally.
     // TODO - Optimize persistent storage of NOC and Root Cert in FabricInfo.
@@ -186,11 +212,12 @@ class DLL_EXPORT FabricInfo
         mVendorId       = VendorId::NotSpecified;
         mFabricLabel[0] = '\0';
 
-        if (mOperationalKey != nullptr)
+        if (mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
         {
             chip::Platform::Delete(mOperationalKey);
-            mOperationalKey = nullptr;
         }
+        mOperationalKey = nullptr;
+
         ReleaseOperationalCerts();
         mFabricIndex = kUndefinedFabricIndex;
     }
@@ -229,6 +256,7 @@ class DLL_EXPORT FabricInfo
 #else
     mutable Crypto::P256Keypair * mOperationalKey = nullptr;
 #endif
+    bool mHasExternallyOwnedOperationalKey = false;
 
     MutableByteSpan mRootCert;
     MutableByteSpan mICACert;