Skip to content

Commit

Permalink
Store the updated fabric info when bringing up controller on existing…
Browse files Browse the repository at this point in the history 
… fabric. (#18260)

Right now bringing up a controller on an existing fabric updates the
in-memory fabric table but does not store the updated data.

This is not very consistent with our desire to not even keep the
fabric table in memory, and leads to inconsistent behavior if someone
later reloads the fabric table from storage in some way.

Specific changes:

1. Change FabricInfo::SetFabricInfo to not modify any member state until
   validation of the incoming info is complete, so we won't corrupt an existing
   fabric due to mistaken input and leave it in a bad state.

2. If updating the existing fabric in ProcessControllerNOCChain succeeds, store
   the updated fabric info.  This is done outside the fabric table for now,
   pending changes to the fabric table to store things itself instead of keeping
   them in RAM.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Jul 11, 2023
1 parent 7cdb27e commit 2306904
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 14 deletions.
3 changes: 3 additions & 0 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParam
if (mFabricInfo != nullptr)
{
ReturnErrorOnFailure(mFabricInfo->SetFabricInfo(newFabric));
// Store the new fabric info, since we might now have new certificates
// and whatnot.
ReturnErrorOnFailure(params.systemState->Fabrics()->Store(mFabricInfo->GetFabricIndex()));
}
else
{
Expand Down
31 changes: 21 additions & 10 deletions src/credentials/FabricTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
// The compressed fabric ID doesn't change for a fabric over time.
// Computing it here will save computational overhead when it's accessed by other
// parts of the code.
ReturnErrorOnFailure(GeneratePeerId(mFabricId, nodeId, &mOperationalId));
ReturnErrorOnFailure(GeneratePeerId(mRootCert, mFabricId, nodeId, &mOperationalId));
ReturnErrorOnFailure(SetNOCCert(nocCert));
}

Expand Down Expand Up @@ -267,7 +267,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
return CHIP_NO_ERROR;
}

CHIP_ERROR FabricInfo::GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId) const
CHIP_ERROR FabricInfo::GeneratePeerId(const ByteSpan & rcac, FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId)
{
ReturnErrorCodeIf(compressedPeerId == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
uint8_t compressedFabricIdBuf[sizeof(uint64_t)];
Expand All @@ -276,7 +276,7 @@ CHIP_ERROR FabricInfo::GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId *

{
P256PublicKeySpan rootPubkeySpan;
ReturnErrorOnFailure(GetRootPubkey(rootPubkeySpan));
ReturnErrorOnFailure(ExtractPublicKeyFromChipCert(rcac, rootPubkeySpan));
rootPubkey = rootPubkeySpan;
}

Expand Down Expand Up @@ -365,6 +365,13 @@ CHIP_ERROR FabricInfo::SetCert(MutableByteSpan & dstCert, const ByteSpan & srcCe

CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, ValidationContext & context,
PeerId & nocPeerId, FabricId & fabricId, Crypto::P256PublicKey & nocPubkey) const
{
return VerifyCredentials(noc, icac, mRootCert, context, nocPeerId, fabricId, nocPubkey);
}

CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac,
ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId,
Crypto::P256PublicKey & nocPubkey)
{
// TODO - Optimize credentials verification logic
// The certificate chain construction and verification is a compute and memory intensive operation.
Expand All @@ -375,7 +382,7 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan &
ChipCertificateSet certificates;
ReturnErrorOnFailure(certificates.Init(kMaxNumCertsInOpCreds));

ReturnErrorOnFailure(certificates.LoadCert(mRootCert, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
ReturnErrorOnFailure(certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));

if (!icac.empty())
{
Expand Down Expand Up @@ -423,7 +430,7 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan &
return err;
}

ReturnErrorOnFailure(GeneratePeerId(fabricId, nodeId, &nocPeerId));
ReturnErrorOnFailure(GeneratePeerId(rcac, fabricId, nodeId, &nocPeerId));
nocPubkey = P256PublicKey(certificates.GetLastCert()[0].mPublicKey);

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -551,13 +558,17 @@ CHIP_ERROR FabricInfo::SetFabricInfo(FabricInfo & newFabric)
validContext.mRequiredKeyUsages.Set(KeyUsageFlags::kDigitalSignature);
validContext.mRequiredKeyPurposes.Set(KeyPurposeFlags::kServerAuth);

SetOperationalKeypair(newFabric.GetOperationalKey());
SetRootCert(newFabric.mRootCert);

// Make sure to not modify any of our state until VerifyCredentials passes.
PeerId operationalId;
FabricId fabricId;
ChipLogProgress(Discovery, "Verifying the received credentials");
ReturnErrorOnFailure(
VerifyCredentials(newFabric.mNOCCert, newFabric.mICACert, validContext, mOperationalId, mFabricId, pubkey));
ReturnErrorOnFailure(VerifyCredentials(newFabric.mNOCCert, newFabric.mICACert, newFabric.mRootCert, validContext, operationalId,
fabricId, pubkey));

SetOperationalKeypair(newFabric.GetOperationalKey());
SetRootCert(newFabric.mRootCert);
mOperationalId = operationalId;
mFabricId = fabricId;
SetICACert(newFabric.mICACert);
SetNOCCert(newFabric.mNOCCert);
SetVendorId(newFabric.GetVendorId());
Expand Down
10 changes: 8 additions & 2 deletions src/credentials/FabricTable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,15 @@ class DLL_EXPORT FabricInfo
return Credentials::ExtractPublicKeyFromChipCert(mRootCert, publicKey);
}

// Verifies credentials, using this fabric info's root certificate.
CHIP_ERROR VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, Credentials::ValidationContext & context,
PeerId & nocPeerId, FabricId & fabricId, Crypto::P256PublicKey & nocPubkey) const;

// Verifies credentials, using the provided root certificate.
static CHIP_ERROR VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac,
Credentials::ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId,
Crypto::P256PublicKey & nocPubkey);

/**
* Reset the state to a completely uninitialized status.
*/
Expand All @@ -192,9 +198,9 @@ class DLL_EXPORT FabricInfo
CHIP_ERROR SetFabricInfo(FabricInfo & fabric);

/* Generate a compressed peer ID (containing compressed fabric ID) using provided fabric ID, node ID and
root public key of the fabric. The generated compressed ID is returned via compressedPeerId
root public key of the provided root certificate. The generated compressed ID is returned via compressedPeerId
output parameter */
CHIP_ERROR GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId) const;
static CHIP_ERROR GeneratePeerId(const ByteSpan & rcac, FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId);

friend class FabricTable;

Expand Down
4 changes: 2 additions & 2 deletions src/credentials/tests/TestFabricTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ void TestGetCompressedFabricID(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, fabricInfo.SetRootCert(ByteSpan(sTestRootCert)) == CHIP_NO_ERROR);

PeerId compressedId;
NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(1234, 4321, &compressedId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(ByteSpan(sTestRootCert), 1234, 4321, &compressedId) == CHIP_NO_ERROR);

// We are compairing with hard coded values here (which are generated manually when the test was written)
// This is to ensure that the same value is generated on big endian and little endian platforms.
// If in this test any input to GeneratePeerId() is changed, this value must be recomputed.
NL_TEST_ASSERT(inSuite, compressedId.GetCompressedFabricId() == 0x090F17C67be7b663);
NL_TEST_ASSERT(inSuite, compressedId.GetNodeId() == 4321);

NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(0xabcd, 0xdeed, &compressedId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(ByteSpan(sTestRootCert), 0xabcd, 0xdeed, &compressedId) == CHIP_NO_ERROR);

// We are compairing with hard coded values here (which are generated manually when the test was written)
// This is to ensure that the same value is generated on big endian and little endian platforms
Expand Down

0 comments on commit 2306904

Please sign in to comment.