address review comments

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<NodeId>(mLocalDeviceId), 0, ByteSpan(CSR.Get(), csrLength), 1, &mLocalNOCCallback));
+                Optional<NodeId>(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<DeviceCommissioner *>(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<uint8_t> 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<uint8_t> 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<NodeId>(device->GetDeviceId()), 0, CSR, 1, &mDeviceNOCCallback));
+        Optional<NodeId>(device->GetDeviceId()), 0, CSR, ByteSpan(), &mDeviceNOCCallback));
 
     return CHIP_NO_ERROR;
 }

src/controller/ExampleOperationalCredentialsIssuer.cpp

@@ -64,22 +64,22 @@ CHIP_ERROR ExampleOperationalCredentialsIssuer::Initialize(PersistentStorageDele
 }
 
 CHIP_ERROR
-ExampleOperationalCredentialsIssuer::GenerateNodeOperationalCertificate(const Optional<NodeId> & deviceId, FabricId fabricId,
-                                                                        const ByteSpan & csr, int64_t serialNumber,
+ExampleOperationalCredentialsIssuer::GenerateNodeOperationalCertificate(const Optional<NodeId> & nodeId, FabricId fabricId,
+                                                                        const ByteSpan & csr, const ByteSpan & DAC,
                                                                         Callback::Callback<NOCGenerated> * 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));

src/controller/ExampleOperationalCredentialsIssuer.h

@@ -43,8 +43,8 @@ class DLL_EXPORT ExampleOperationalCredentialsIssuer : public OperationalCredent
 public:
     virtual ~ExampleOperationalCredentialsIssuer() {}
 
-    CHIP_ERROR GenerateNodeOperationalCertificate(const Optional<NodeId> & deviceId, FabricId fabricId, const ByteSpan & csr,
-                                                  int64_t serialNumber, Callback::Callback<NOCGenerated> * onNOCGenerated) override;
+    CHIP_ERROR GenerateNodeOperationalCertificate(const Optional<NodeId> & nodeId, FabricId fabricId, const ByteSpan & csr,
+                                                  const ByteSpan & DAC, Callback::Callback<NOCGenerated> * onNOCGenerated) override;
 
     CHIP_ERROR GetRootCACertificate(FabricId fabricId, MutableByteSpan & outCert) override;
 

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<NodeId> & deviceId, FabricId fabricId,
-                                                          const ByteSpan & csr, int64_t serialNumber,
+    virtual CHIP_ERROR GenerateNodeOperationalCertificate(const Optional<NodeId> & nodeId, FabricId fabricId, const ByteSpan & csr,
+                                                          const ByteSpan & DAC,
                                                           Callback::Callback<NOCGenerated> * onNOCGenerated) = 0;
 
     /**

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<chip::NodeId> & deviceId, chip::FabricId fabricId,
-        const chip::ByteSpan & csr, int64_t serialNumber,
+    CHIP_ERROR GenerateNodeOperationalCertificate(const chip::Optional<chip::NodeId> & nodeId, chip::FabricId fabricId,
+        const chip::ByteSpan & csr, const chip::ByteSpan & DAC,
         chip::Callback::Callback<chip::Controller::NOCGenerated> * onNOCGenerated) override;
 
     CHIP_ERROR GetRootCACertificate(chip::FabricId fabricId, chip::MutableByteSpan & outCert) override;

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<chip::NodeId> & deviceId,
-    chip::FabricId fabricId, const chip::ByteSpan & csr, int64_t serialNumber,
+CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateNodeOperationalCertificate(const chip::Optional<chip::NodeId> & nodeId,
+    chip::FabricId fabricId, const chip::ByteSpan & csr, const chip::ByteSpan & DAC,
     chip::Callback::Callback<chip::Controller::NOCGenerated> * 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);