Skip to content

Commit

Permalink
Allow external ownership of CHIPDeviceController operational key
Browse files Browse the repository at this point in the history
- 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 project-chip#18444
Issue project-chip#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
  • Loading branch information
tcarmelveilleux committed May 16, 2022
1 parent 0ddaa8b commit ef8a02e
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 12 deletions.
14 changes: 11 additions & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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);
Expand Down
16 changes: 14 additions & 2 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -240,6 +247,13 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
*/
CHIP_ERROR DisconnectDevice(NodeId nodeId);

/**
* @brief
* Configures a new set of operational credentials to be used with this
* controller.
*/
CHIP_ERROR InitControllerNOCChain(const ControllerInitParams & params);

protected:
enum class State
{
Expand Down Expand Up @@ -275,8 +289,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
Expand Down
36 changes: 32 additions & 4 deletions src/credentials/FabricTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ CHIP_ERROR FabricInfo::CommitToStorage(PersistentStorageDelegate * storage)

{
Crypto::P256SerializedKeypair serializedOpKey;
if (mOperationalKey != nullptr)
if (!mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
{
ReturnErrorOnFailure(mOperationalKey->Serialize(serializedOpKey));
}
Expand All @@ -117,7 +117,10 @@ CHIP_ERROR FabricInfo::CommitToStorage(PersistentStorageDelegate * storage)

ReturnErrorOnFailure(writer.Put(kOpKeyVersionTag, kOpKeyVersion));

ReturnErrorOnFailure(writer.Put(kOpKeyDataTag, ByteSpan(serializedOpKey.Bytes(), serializedOpKey.Length())));
if (!mHasExternallyOwnedOperationalKey)
{
ReturnErrorOnFailure(writer.Put(kOpKeyDataTag, ByteSpan(serializedOpKey.Bytes(), serializedOpKey.Length())));
}

ReturnErrorOnFailure(writer.EndContainer(outerType));

Expand Down Expand Up @@ -318,9 +321,12 @@ CHIP_ERROR FabricInfo::DeleteFromStorage(PersistentStorageDelegate * storage, Fa
return prevDeleteErr;
}

CHIP_ERROR FabricInfo::SetOperationalKeypair(const P256Keypair * keyPair)
CHIP_ERROR FabricInfo::SetOperationalKeypair(P256Keypair * keyPair)
{
VerifyOrReturnError(keyPair != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

mHasExternallyOwnedOperationalKey = false;

P256SerializedKeypair serialized;
ReturnErrorOnFailure(keyPair->Serialize(serialized));
if (mOperationalKey == nullptr)
Expand All @@ -335,6 +341,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)
Expand Down Expand Up @@ -565,7 +585,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;
Expand Down
31 changes: 28 additions & 3 deletions src/credentials/FabricTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class DLL_EXPORT FabricInfo

~FabricInfo()
{
if (mOperationalKey != nullptr)
if (!mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
{
chip::Platform::Delete(mOperationalKey);
}
Expand Down Expand Up @@ -123,7 +123,31 @@ class DLL_EXPORT FabricInfo
}
return mOperationalKey;
}
CHIP_ERROR SetOperationalKeypair(const Crypto::P256Keypair * keyPair);


/**
* Sets the P256Keypair used for this fabric, transferring ownership of the
* key to this object by making a copy via the P256Keypair::Serialize and
* P256Keypair::Deserialize methods.
*
* If your P256Keypair does not support serialization, use the
* `SetExternallyOwnedOperationalKeypair` method instead.
*/
CHIP_ERROR SetOperationalKeypair(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.
*
* 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).
*
* 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.
Expand Down Expand Up @@ -186,7 +210,7 @@ class DLL_EXPORT FabricInfo
mVendorId = VendorId::NotSpecified;
mFabricLabel[0] = '\0';

if (mOperationalKey != nullptr)
if (mHasExternallyOwnedOperationalKey && mOperationalKey != nullptr)
{
chip::Platform::Delete(mOperationalKey);
mOperationalKey = nullptr;
Expand Down Expand Up @@ -229,6 +253,7 @@ class DLL_EXPORT FabricInfo
#else
mutable Crypto::P256Keypair * mOperationalKey = nullptr;
#endif
bool mHasExternallyOwnedOperationalKey = false;

MutableByteSpan mRootCert;
MutableByteSpan mICACert;
Expand Down

0 comments on commit ef8a02e

Please sign in to comment.