Skip to content

Commit

Permalink
Allow remote signing infrastructure to assign operational IDs (#13801)
Browse files Browse the repository at this point in the history 
* Allow remote signing infrastructure to assign operational IDs

The operational credentials delegate interface includes the
SetFabricIdForNextNOCRequest and SetNodeIdForNextNOCRequest methods,
which are supposed to provide non-binding operational ID hints to
signers.

However, these actually are not optional because no matter what
identifiers the signer encloses in the certificate chain, the
commissioner assumes the signer has honored its suggestions and will
not be able to locate or communicate with the commissioned node unless
it carries the identifiers the commissioner has proposed.

This commit fixes the issue by using the signer-provided certificate
chain as source of truth, extracting compressed fabric ID and node ID
from this.

This will not disrupt signers that honor the commissioner's hints, but
also works for signers that assign their own IDs.  In the case of a
combined commissioner / admin, the signer is free to assign node IDs of
its choice.  In the case of a standalone commissioner implementation
that does not establish CASE or invoke CommissioningComplete, the signer
is free to select all of root cert, fabric ID and node ID.  In all
cases, the resulting PeerId struct in the CommissioneeDeviceProxy will
carry identifiers that match the certificate chain from the signer.

Fixes #13500

* per tcarmelveilleux, do not add PeerId dependency to credentials code
  • Loading branch information
@msandstedt @pull
msandstedt authored and pull[bot] committed Sep 25, 2023
1 parent 25ac36c commit 3739957
Show file tree
Hide file tree
Showing 9 changed files with 163 additions and 15 deletions.
11 changes: 8 additions & 3 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re

FabricInfo * fabric = mSystemState->Fabrics()->FindFabricWithIndex(mFabricIndex);

VerifyOrExit(IsOperationalNodeId(remoteDeviceId), err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(mDeviceBeingCommissioned == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -1277,6 +1276,7 @@ void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR s
const ByteSpan & rcac)
{
CHIP_ERROR err = CHIP_NO_ERROR;
Crypto::P256PublicKey rootPubKey;

DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);

Expand All @@ -1293,14 +1293,16 @@ void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR s
device = commissioner->mDeviceBeingCommissioned;

{
Credentials::P256PublicKeySpan rootPubKeySpan;
// Reuse NOC Cert buffer for temporary store Root Cert.
MutableByteSpan rootCert = device->GetMutableNOCCert();

err = ConvertX509CertToChipCert(rcac, rootCert);
SuccessOrExit(err);

err = commissioner->SendTrustedRootCertificate(device, rootCert);
SuccessOrExit(err);
SuccessOrExit(err = commissioner->SendTrustedRootCertificate(device, rootCert));
SuccessOrExit(err = Credentials::ExtractPublicKeyFromChipCert(rootCert, rootPubKeySpan));
rootPubKey = Crypto::P256PublicKey(rootPubKeySpan); // deep copy
}

if (!icac.empty())
Expand All @@ -1324,6 +1326,9 @@ void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR s
SuccessOrExit(err);
}

SuccessOrExit(err = device->SetPeerId(rootPubKey, device->GetNOCCert()));
VerifyOrExit(IsOperationalNodeId(device->GetDeviceId()), err = CHIP_ERROR_INVALID_ARGUMENT);

exit:
if (err != CHIP_NO_ERROR)
{
Expand Down
14 changes: 12 additions & 2 deletions src/controller/CommissioneeDeviceProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@ CHIP_ERROR CommissioneeDeviceProxy::LoadSecureSessionParameters()
ExitNow(err = CHIP_NO_ERROR);
}

SuccessOrExit(mSessionManager->NewPairing(mSecureSession, Optional<Transport::PeerAddress>::Value(mDeviceAddress), mDeviceId,
&mPairing, CryptoContext::SessionRole::kInitiator, mFabricIndex));
SuccessOrExit(mSessionManager->NewPairing(mSecureSession, Optional<Transport::PeerAddress>::Value(mDeviceAddress),
GetDeviceId(), &mPairing, CryptoContext::SessionRole::kInitiator, mFabricIndex));
mState = ConnectionState::SecureConnected;

exit:
Expand All @@ -202,6 +202,16 @@ bool CommissioneeDeviceProxy::GetAddress(Inet::IPAddress & addr, uint16_t & port
return true;
}

CHIP_ERROR CommissioneeDeviceProxy::SetPeerId(const Crypto::P256PublicKey & rootPublicKey, ByteSpan noc)
{
CompressedFabricId compressedFabricId;
NodeId nodeId;
ReturnErrorOnFailure(
Credentials::ExtractNodeIdCompressedFabricIdFromRootPubKeyOpCert(rootPublicKey, noc, compressedFabricId, nodeId));
mPeerId = PeerId().SetCompressedFabricId(compressedFabricId).SetNodeId(nodeId);
return CHIP_NO_ERROR;
}

void CommissioneeDeviceProxy::ReleaseDAC()
{
if (mDAC != nullptr)
Expand Down
13 changes: 8 additions & 5 deletions src/controller/CommissioneeDeviceProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
void Init(ControllerDeviceInitParams params, NodeId deviceId, const Transport::PeerAddress & peerAddress, FabricIndex fabric)
{
Init(params, fabric);
mDeviceId = deviceId;
mState = ConnectionState::Connecting;
mPeerId = PeerId().SetNodeId(deviceId);
mState = ConnectionState::Connecting;

mDeviceAddress = peerAddress;
}
Expand Down Expand Up @@ -210,7 +210,9 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat

void Reset();

NodeId GetDeviceId() const override { return mDeviceId; }
NodeId GetDeviceId() const override { return mPeerId.GetNodeId(); }
PeerId GetPeerId() const { return mPeerId; }
CHIP_ERROR SetPeerId(const Crypto::P256PublicKey & rootPublicKey, ByteSpan noc);

bool MatchesSession(const SessionHandle & session) const { return mSecureSession.Contains(session); }

Expand Down Expand Up @@ -284,8 +286,9 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
kYes,
kNo,
};
/* Node ID assigned to the CHIP device */
NodeId mDeviceId;

/* Compressed fabric ID and node ID assigned to the device. */
PeerId mPeerId;

/** Address used to communicate with the device.
*/
Expand Down
18 changes: 18 additions & 0 deletions src/credentials/CHIPCert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,24 @@ CHIP_ERROR ExtractNodeIdFabricIdFromOpCert(const ChipCertificateData & opcert, N
return CHIP_NO_ERROR;
}

CHIP_ERROR ExtractNodeIdFabricIdCompressedFabricIdFromRootPubKeyOpCert(const Crypto::P256PublicKey & rootPubKey, ByteSpan noc,
CompressedFabricId & compressedFabricId, FabricId & fabricId,
NodeId & nodeId)
{
ReturnErrorOnFailure(Credentials::ExtractNodeIdFabricIdFromOpCert(noc, &nodeId, &fabricId));
ReturnErrorOnFailure(GenerateCompressedFabricId(rootPubKey, fabricId, compressedFabricId));
return CHIP_NO_ERROR;
}

CHIP_ERROR ExtractNodeIdCompressedFabricIdFromRootPubKeyOpCert(const Crypto::P256PublicKey & rootPubKey, ByteSpan noc,
CompressedFabricId & compressedFabricId, NodeId & nodeId)
{
FabricId fabricId;
ReturnErrorOnFailure(
ExtractNodeIdFabricIdCompressedFabricIdFromRootPubKeyOpCert(rootPubKey, noc, compressedFabricId, fabricId, nodeId));
return CHIP_NO_ERROR;
}

CHIP_ERROR ExtractFabricIdFromCert(const ChipCertificateData & cert, FabricId * fabricId)
{
const ChipDN & subjectDN = cert.mSubjectDN;
Expand Down
19 changes: 19 additions & 0 deletions src/credentials/CHIPCert.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,25 @@ CHIP_ERROR ExtractFabricIdFromCert(const ChipCertificateData & cert, FabricId *
*/
CHIP_ERROR ExtractNodeIdFabricIdFromOpCert(const ChipCertificateData & opcert, NodeId * nodeId, FabricId * fabricId);

/**
* Extract Node ID, Fabric ID and Compressed Fabric ID from an operational
* certificate and root public key.
*
* @return CHIP_ERROR on failure or CHIP_NO_ERROR otherwise.
*/
CHIP_ERROR ExtractNodeIdFabricIdCompressedFabricIdFromRootPubKeyOpCert(const Crypto::P256PublicKey & rootPubKey, ByteSpan noc,
CompressedFabricId & compressedFabricId, FabricId & fabricId,
NodeId & nodeId);

/**
* Extract Node ID and Compressed Fabric ID from an operational certificate
* and root public key.
*
* @return CHIP_ERROR on failure or CHIP_NO_ERROR otherwise.
*/
CHIP_ERROR ExtractNodeIdCompressedFabricIdFromRootPubKeyOpCert(const Crypto::P256PublicKey & rootPubKey, ByteSpan noc,
CompressedFabricId & compressedFabricId, NodeId & nodeId);

/**
* Extract CASE Authenticated Tags from an operational certificate in ByteSpan TLV-encoded form.
*
Expand Down
72 changes: 67 additions & 5 deletions src/credentials/tests/TestChipCert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ static void TestChipCert_VerifyGeneratedCertsNoICA(nlTestSuite * inSuite, void *
NL_TEST_ASSERT(inSuite, certSet.FindValidCert(subjectDN, subjectKeyId, validContext, &resultCert) == CHIP_NO_ERROR);
}

static void TestChipCert_ExtractPeerId(nlTestSuite * inSuite, void * inContext)
static void TestChipCert_ExtractNodeIdFabricId(nlTestSuite * inSuite, void * inContext)
{
struct TestCase
{
Expand All @@ -1043,7 +1043,7 @@ static void TestChipCert_ExtractPeerId(nlTestSuite * inSuite, void * inContext)
};
// clang-format on

// Test extraction from the raw ByteSpan form.
// Test node ID and fabric ID extraction from the raw ByteSpan form.
for (auto & testCase : sTestCases)
{
ByteSpan cert;
Expand All @@ -1058,7 +1058,7 @@ static void TestChipCert_ExtractPeerId(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, fabricId == testCase.ExpectedFabricId);
}

// Test extraction from the parsed form.
// Test node ID and fabric ID extraction from the parsed form.
ChipCertificateSet certSet;
for (auto & testCase : sTestCases)
{
Expand All @@ -1077,7 +1077,7 @@ static void TestChipCert_ExtractPeerId(nlTestSuite * inSuite, void * inContext)
certSet.Release();
}

// Test extraction from the parsed form.
// Test fabric ID extraction from the parsed form.
for (auto & testCase : sTestCases)
{
CHIP_ERROR err = certSet.Init(1);
Expand Down Expand Up @@ -1108,6 +1108,67 @@ static void TestChipCert_ExtractPeerId(nlTestSuite * inSuite, void * inContext)
}
}

static void TestChipCert_ExtractOperationalDiscoveryId(nlTestSuite * inSuite, void * inContext)
{
struct TestCase
{
uint8_t Noc;
uint8_t Rcac;
uint64_t ExpectedNodeId;
uint64_t ExpectedFabricId;
uint64_t ExpectedCompressedFabricId;
};

// clang-format off
static constexpr TestCase sTestCases[] = {
// Cert ICA ExpectedNodeId ExpectedFabricId ExpectedCompressedFabricId
// ===========================================================================================================
{ TestCert::kNode01_01, TestCert::kRoot01, 0xDEDEDEDE00010001, 0xFAB000000000001D, 0x3893C4324526C775 },
{ TestCert::kNode01_02, TestCert::kRoot01, 0xDEDEDEDE00010002, 0xFAB000000000001D, 0x3893C4324526C775 },
{ TestCert::kNode02_01, TestCert::kRoot02, 0xDEDEDEDE00020001, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_02, TestCert::kRoot02, 0xDEDEDEDE00020002, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_03, TestCert::kRoot02, 0xDEDEDEDE00020003, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_04, TestCert::kRoot02, 0xDEDEDEDE00020004, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_05, TestCert::kRoot02, 0xDEDEDEDE00020005, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_06, TestCert::kRoot02, 0xDEDEDEDE00020006, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_07, TestCert::kRoot02, 0xDEDEDEDE00020007, 0xFAB000000000001D, 0x89E8911178DAC089 },
{ TestCert::kNode02_08, TestCert::kRoot02, 0xDEDEDEDE00020008, 0xFAB000000000001D, 0x89E8911178DAC089 },
};
// clang-format on

for (auto & testCase : sTestCases)
{
ByteSpan noc;
ByteSpan rcac;
CHIP_ERROR err = GetTestCert(testCase.Noc, sNullLoadFlag, noc);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
err = GetTestCert(testCase.Rcac, sNullLoadFlag, rcac);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// Extract Node ID and Fabric ID from the leaf node certificate.
NodeId nodeId;
FabricId fabricId;
err = ExtractNodeIdFabricIdFromOpCert(noc, &nodeId, &fabricId);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, nodeId == testCase.ExpectedNodeId);
NL_TEST_ASSERT(inSuite, fabricId == testCase.ExpectedFabricId);

// Extract the Public key from the root certificate.
Credentials::P256PublicKeySpan rootPubKey;
err = Credentials::ExtractPublicKeyFromChipCert(rcac, rootPubKey);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// Extract Node ID and Fabric ID from the NOC, and generate the
// compressed fabric ID from the root CA public Key and fabric ID.
CompressedFabricId compressedFabricId;
err = ExtractNodeIdFabricIdCompressedFabricIdFromRootPubKeyOpCert(rootPubKey, noc, compressedFabricId, fabricId, nodeId);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, compressedFabricId == testCase.ExpectedCompressedFabricId);
NL_TEST_ASSERT(inSuite, fabricId == testCase.ExpectedFabricId);
NL_TEST_ASSERT(inSuite, nodeId == testCase.ExpectedNodeId);
}
}

static void TestChipCert_ExtractCATsFromOpCert(nlTestSuite * inSuite, void * inContext)
{
struct TestCase
Expand Down Expand Up @@ -1273,7 +1334,8 @@ static const nlTest sTests[] = {
NL_TEST_DEF("Test CHIP Generate NOC using ICA", TestChipCert_GenerateNOCICA),
NL_TEST_DEF("Test CHIP Verify Generated Cert Chain", TestChipCert_VerifyGeneratedCerts),
NL_TEST_DEF("Test CHIP Verify Generated Cert Chain No ICA", TestChipCert_VerifyGeneratedCertsNoICA),
NL_TEST_DEF("Test extracting PeerId from node certificate", TestChipCert_ExtractPeerId),
NL_TEST_DEF("Test extracting Node ID and Fabric ID from node certificate", TestChipCert_ExtractNodeIdFabricId),
NL_TEST_DEF("Test extracting Operational Discovery ID from node and root certificate", TestChipCert_ExtractOperationalDiscoveryId),
NL_TEST_DEF("Test extracting CASE Authenticated Tags from node certificate", TestChipCert_ExtractCATsFromOpCert),
NL_TEST_DEF("Test extracting PublicKey and SKID from chip certificate", TestChipCert_ExtractPublicKeyAndSKID),
NL_TEST_SENTINEL()
Expand Down
11 changes: 11 additions & 0 deletions src/crypto/CHIPCryptoPAL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -657,5 +657,16 @@ CHIP_ERROR GenerateCompressedFabricId(const Crypto::P256PublicKey & root_public_
return status;
}

CHIP_ERROR GenerateCompressedFabricId(const Crypto::P256PublicKey & rootPublicKey, uint64_t fabricId, uint64_t & compressedFabricId)
{
uint8_t allocated[sizeof(fabricId)];
MutableByteSpan span(allocated);
ReturnErrorOnFailure(GenerateCompressedFabricId(rootPublicKey, fabricId, span));
// Decode compressed fabric ID accounting for endianness, as GenerateCompressedFabricId()
// returns a binary buffer and is agnostic of usage of the output as an integer type.
compressedFabricId = Encoding::BigEndian::Get64(allocated);
return CHIP_NO_ERROR;
}

} // namespace Crypto
} // namespace chip
13 changes: 13 additions & 0 deletions src/crypto/CHIPCryptoPAL.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,19 @@ class Spake2p_P256_SHA256_HKDF_HMAC : public Spake2p
CHIP_ERROR GenerateCompressedFabricId(const Crypto::P256PublicKey & root_public_key, uint64_t fabric_id,
MutableByteSpan & out_compressed_fabric_id);

/**
* @brief Compute the compressed fabric identifier used for operational discovery service
* records from a Node's root public key and Fabric ID. This is a conveniance
* overload that writes to a uint64_t (CompressedFabricId) type.
*
* @param[in] root_public_key The root public key associated with the node's fabric
* @param[in] fabric_id The fabric ID associated with the node's fabric
* @param[out] compressedFabricId output location for compressed fabric ID
* @returns a CHIP_ERROR on failure or CHIP_NO_ERROR otherwise.
*/
CHIP_ERROR GenerateCompressedFabricId(const Crypto::P256PublicKey & rootPublicKey, uint64_t fabricId,
uint64_t & compressedFabricId);

typedef CapacityBoundBuffer<kMax_x509_Certificate_Length> X509DerCertificate;

CHIP_ERROR LoadCertsFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t * max_certs);
Expand Down
7 changes: 7 additions & 0 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1742,10 +1742,12 @@ static void TestCompressedFabricIdentifier(nlTestSuite * inSuite, void * inConte
};
static_assert(sizeof(kExpectedCompressedFabricIdentifier) == kCompressedFabricIdentifierSize,
"Expected compressed fabric identifier must the correct size");
const uint64_t kExpectedCompressedFabricIdentifierInt = 0x87e1b004e235a130;

uint8_t compressed_fabric_id[kCompressedFabricIdentifierSize];
MutableByteSpan compressed_fabric_id_span(compressed_fabric_id);
ClearSecretData(compressed_fabric_id, sizeof(compressed_fabric_id));
uint64_t compressed_fabric_id_int;

CHIP_ERROR error = GenerateCompressedFabricId(root_public_key, kFabricId, compressed_fabric_id_span);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);
Expand Down Expand Up @@ -1773,6 +1775,11 @@ static void TestCompressedFabricIdentifier(nlTestSuite * inSuite, void * inConte
error = GenerateCompressedFabricId(root_public_key, kFabricId, compressed_fabric_id_small_span);
NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_BUFFER_TOO_SMALL);

// Test overload that writes to an integer output type.
error = GenerateCompressedFabricId(root_public_key, kFabricId, compressed_fabric_id_int);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, compressed_fabric_id_int == kExpectedCompressedFabricIdentifierInt);

// Test invalid public key
const uint8_t kInvalidRootPublicKey[65] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
Expand Down

0 comments on commit 3739957

Please sign in to comment.