Skip to content

Commit

Permalink
Allow external ownership of CHIPDeviceController operational key (#18482
Browse files Browse the repository at this point in the history 
)

* 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]>
  • Loading branch information
@tcarmelveilleux @restyled-commits
@bzbarsky-apple @pull
3 people authored and pull[bot] committed Jul 19, 2022
1 parent 30ee538 commit 1291541
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 39 deletions.
14 changes: 11 additions & 3 deletions src/controller/CHIPDeviceController.cpp
View file
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
22 changes: 20 additions & 2 deletions src/controller/CHIPDeviceController.h
View file
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,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
{
Expand Down Expand Up @@ -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
Expand Down
87 changes: 57 additions & 30 deletions src/credentials/FabricTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
36 changes: 32 additions & 4 deletions src/credentials/FabricTable.h
View file
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 @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -229,6 +256,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 1291541

Please sign in to comment.