From 3eefbb7a21e656db76326d5dff1e133406f0b05c Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Fri, 25 Jun 2021 15:43:27 -0700 Subject: [PATCH] address review comments --- src/controller/CHIPDeviceController.cpp | 11 ++++++----- .../ExampleOperationalCredentialsIssuer.cpp | 10 +++++----- .../ExampleOperationalCredentialsIssuer.h | 4 ++-- src/controller/OperationalCredentialsDelegate.h | 16 ++++++---------- .../CHIP/CHIPOperationalCredentialsDelegate.h | 4 ++-- .../CHIP/CHIPOperationalCredentialsDelegate.mm | 10 +++++----- 6 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 2709013b9acece..15cc44cf0f5c1c 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -311,7 +311,7 @@ CHIP_ERROR DeviceController::LoadLocalCredentials(Transport::AdminPairingInfo * // Make sure it chains back to the trusted root. ChipLogProgress(Controller, "Generating operational certificate for the controller"); ReturnErrorOnFailure(mOperationalCredentialsDelegate->GenerateNodeOperationalCertificate( - Optional(mLocalDeviceId), 0, ByteSpan(CSR.Get(), csrLength), 1, &mLocalNOCCallback)); + Optional(mLocalDeviceId), 0, ByteSpan(CSR.Get(), csrLength), ByteSpan(), &mLocalNOCCallback)); } ReturnErrorOnFailure(mAdmins.Store(admin->GetAdminId())); @@ -1226,6 +1226,8 @@ void DeviceCommissioner::OnDeviceNOCGenerated(void * context, const ByteSpan & n DeviceCommissioner * commissioner = reinterpret_cast(context); + // The operational certificate array can contain upto 2 certificates (NOC, and ICAC) + // The memory is allocated to account for both these certificates. uint32_t chipCertAllocatedLen = kMaxCHIPCertLength * 2; chip::Platform::ScopedMemoryBuffer chipCert; @@ -1264,9 +1266,6 @@ CHIP_ERROR DeviceCommissioner::ProcessOpCSR(const ByteSpan & CSR, const ByteSpan Device * device = &mActiveDevices[mDeviceBeingPaired]; - // TODO: Verify the OpCSR signature using pubkey from DAC - // This will be done when device attestation is implemented. - // Verify that Nonce matches with what we sent const ByteSpan nonce = device->GetCSRNonce(); VerifyOrReturnError(CSRNonce.size() == nonce.size(), CHIP_ERROR_INVALID_ARGUMENT); @@ -1275,8 +1274,10 @@ CHIP_ERROR DeviceCommissioner::ProcessOpCSR(const ByteSpan & CSR, const ByteSpan chip::Platform::ScopedMemoryBuffer chipOpCert; ReturnErrorCodeIf(!chipOpCert.Alloc(kMaxCHIPCertLength * 2), CHIP_ERROR_NO_MEMORY); + // TODO: Send DAC as input parameter to GenerateNodeOperationalCertificate() + // This will be done when device attestation is implemented. ReturnErrorOnFailure(mOperationalCredentialsDelegate->GenerateNodeOperationalCertificate( - Optional(device->GetDeviceId()), 0, CSR, 1, &mDeviceNOCCallback)); + Optional(device->GetDeviceId()), 0, CSR, ByteSpan(), &mDeviceNOCCallback)); return CHIP_NO_ERROR; } diff --git a/src/controller/ExampleOperationalCredentialsIssuer.cpp b/src/controller/ExampleOperationalCredentialsIssuer.cpp index e1536f8eab7def..204d1e08474b49 100644 --- a/src/controller/ExampleOperationalCredentialsIssuer.cpp +++ b/src/controller/ExampleOperationalCredentialsIssuer.cpp @@ -64,22 +64,22 @@ CHIP_ERROR ExampleOperationalCredentialsIssuer::Initialize(PersistentStorageDele } CHIP_ERROR -ExampleOperationalCredentialsIssuer::GenerateNodeOperationalCertificate(const Optional & deviceId, FabricId fabricId, - const ByteSpan & csr, int64_t serialNumber, +ExampleOperationalCredentialsIssuer::GenerateNodeOperationalCertificate(const Optional & nodeId, FabricId fabricId, + const ByteSpan & csr, const ByteSpan & DAC, Callback::Callback * onNOCGenerated) { VerifyOrReturnError(mInitialized, CHIP_ERROR_INCORRECT_STATE); NodeId assignedId; - if (deviceId.HasValue()) + if (nodeId.HasValue()) { - assignedId = deviceId.Value(); + assignedId = nodeId.Value(); } else { assignedId = mNextAvailableNodeId++; } - X509CertRequestParams request = { serialNumber, mIssuerId, mNow, mNow + mValidity, true, fabricId, true, assignedId }; + X509CertRequestParams request = { 1, mIssuerId, mNow, mNow + mValidity, true, fabricId, true, assignedId }; P256PublicKey pubkey; ReturnErrorOnFailure(VerifyCertificateSigningRequest(csr.data(), csr.size(), pubkey)); diff --git a/src/controller/ExampleOperationalCredentialsIssuer.h b/src/controller/ExampleOperationalCredentialsIssuer.h index e0a9b78a8b4079..412c8124da105f 100644 --- a/src/controller/ExampleOperationalCredentialsIssuer.h +++ b/src/controller/ExampleOperationalCredentialsIssuer.h @@ -43,8 +43,8 @@ class DLL_EXPORT ExampleOperationalCredentialsIssuer : public OperationalCredent public: virtual ~ExampleOperationalCredentialsIssuer() {} - CHIP_ERROR GenerateNodeOperationalCertificate(const Optional & deviceId, FabricId fabricId, const ByteSpan & csr, - int64_t serialNumber, Callback::Callback * onNOCGenerated) override; + CHIP_ERROR GenerateNodeOperationalCertificate(const Optional & nodeId, FabricId fabricId, const ByteSpan & csr, + const ByteSpan & DAC, Callback::Callback * onNOCGenerated) override; CHIP_ERROR GetRootCACertificate(FabricId fabricId, MutableByteSpan & outCert) override; diff --git a/src/controller/OperationalCredentialsDelegate.h b/src/controller/OperationalCredentialsDelegate.h index 58ab15ceaabf48..386b911205aac3 100644 --- a/src/controller/OperationalCredentialsDelegate.h +++ b/src/controller/OperationalCredentialsDelegate.h @@ -29,13 +29,9 @@ namespace chip { namespace Controller { -typedef void (*NOCGenerated)(void * context, const ByteSpan & noc, const PeerId & deviceId); +typedef void (*NOCGenerated)(void * context, const ByteSpan & noc, const PeerId & nodeId); -// TODO - Reduce memory requirement for generating x509 certificates -// As per specifications (section 6.3.7. Trusted Root CA Certificates), DER certs -// should require 600 bytes. Currently, due to ASN writer overheads, a larger buffer -// is needed, even though the generated certificate fits in 600 bytes limit. -constexpr uint32_t kMaxCHIPDERCertLength = 1024; +constexpr uint32_t kMaxCHIPDERCertLength = 600; /// Callbacks for CHIP operational credentials generation class DLL_EXPORT OperationalCredentialsDelegate @@ -54,17 +50,17 @@ class DLL_EXPORT OperationalCredentialsDelegate * * The delegate will call `onNOCGenerated` when the NOC is ready. * - * @param[in] deviceId Optional device ID. If provided, the generated NOC must use the provided ID. + * @param[in] nodeId Optional node ID. If provided, the generated NOC must use the provided ID. * If ID is not provided, the delegate must generate one. * @param[in] fabricId Fabric ID for which the certificate is being requested. * @param[in] csr Certificate Signing Request from the node in DER format. - * @param[in] serialNumber Serial number to assign to the new certificate. + * @param[in] DAC Device attestation certificate received from the device being commissioned * @param[in] onNOCGenerated Callback handler to provide generated NOC to the caller of GenerateNodeOperationalCertificate() * * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code. */ - virtual CHIP_ERROR GenerateNodeOperationalCertificate(const Optional & deviceId, FabricId fabricId, - const ByteSpan & csr, int64_t serialNumber, + virtual CHIP_ERROR GenerateNodeOperationalCertificate(const Optional & nodeId, FabricId fabricId, const ByteSpan & csr, + const ByteSpan & DAC, Callback::Callback * onNOCGenerated) = 0; /** diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h index 71e3dedd418ef6..43700b7263fd01 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h @@ -33,8 +33,8 @@ class CHIPOperationalCredentialsDelegate : public chip::Controller::OperationalC CHIP_ERROR init(CHIPPersistentStorageDelegateBridge * storage); - CHIP_ERROR GenerateNodeOperationalCertificate(const chip::Optional & deviceId, chip::FabricId fabricId, - const chip::ByteSpan & csr, int64_t serialNumber, + CHIP_ERROR GenerateNodeOperationalCertificate(const chip::Optional & nodeId, chip::FabricId fabricId, + const chip::ByteSpan & csr, const chip::ByteSpan & DAC, chip::Callback::Callback * onNOCGenerated) override; CHIP_ERROR GetRootCACertificate(chip::FabricId fabricId, chip::MutableByteSpan & outCert) override; diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm index 244289e5a09002..01b0de111c6e49 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm @@ -186,8 +186,8 @@ static BOOL isRunningTests(void) return CHIP_NO_ERROR; } -CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateNodeOperationalCertificate(const chip::Optional & deviceId, - chip::FabricId fabricId, const chip::ByteSpan & csr, int64_t serialNumber, +CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateNodeOperationalCertificate(const chip::Optional & nodeId, + chip::FabricId fabricId, const chip::ByteSpan & csr, const chip::ByteSpan & DAC, chip::Callback::Callback * onNOCGenerated) { uint32_t validityStart, validityEnd; @@ -203,8 +203,8 @@ static BOOL isRunningTests(void) } chip::NodeId assignedId; - if (deviceId.HasValue()) { - assignedId = deviceId.Value(); + if (nodeId.HasValue()) { + assignedId = nodeId.Value(); } else { if (mDeviceBeingPaired == chip::kUndefinedNodeId) { return CHIP_ERROR_INCORRECT_STATE; @@ -213,7 +213,7 @@ static BOOL isRunningTests(void) } chip::Credentials::X509CertRequestParams request - = { serialNumber, mIssuerId, validityStart, validityEnd, true, fabricId, true, assignedId }; + = { 1, mIssuerId, validityStart, validityEnd, true, fabricId, true, assignedId }; chip::Crypto::P256PublicKey pubkey; CHIP_ERROR err = chip::Crypto::VerifyCertificateSigningRequest(csr.data(), csr.size(), pubkey);