Feedback from Android commissioning fixes (project-chip#21927)

* Feedback from Android commissioning fixes * Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Update src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Restyle Feedback from Android commissioning fixes (project-chip#21928) * Restyled by whitespace * Restyled by google-java-format * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]>
isiu-apple · Sep 16, 2022 · f2ce40d · f2ce40d
1 parent 95f2300
commit f2ce40d
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 8 deletions.
diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -561,6 +561,12 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      */
     CHIP_ERROR NetworkCredentialsReady();
 
+    /**
+     * @brief
+     *  This function returns the current CommissioningStage for this commissioner.
+     */
+    CommissioningStage GetCommissioningStage() { return mCommissioningStage; }
+
 #if CONFIG_NETWORK_LAYER_BLE
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     /**

diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp
@@ -205,7 +205,6 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B
 
     P256PublicKey pubkey;
     ReturnErrorOnFailure(VerifyCertificateSigningRequest(csr.data(), csr.size(), pubkey));
-    // TODO: verify signed by DAC creds?
     ChipLogProgress(chipTool, "VerifyCertificateSigningRequest");
 
     jobject csrInfo;

diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp
@@ -524,7 +524,9 @@ JNI_METHOD(void, setUseJavaCallbackForNOCRequest)
 
     if (useCallback)
     {
-        // if we are assigning a callback, then make the device commissioner delegate verification to the cloud
+        // if we are assigning a callback, then make the device commissioner delegate verification to the
+        // PartialDACVerifier so that DAC chain and CD validation can be performed by custom code
+        // triggered by ChipDeviceController.NOCChainIssuer.onNOCChainGenerationNeeded().
         wrapper->Controller()->SetDeviceAttestationVerifier(wrapper->GetPartialDACVerifier());
     }
     else
@@ -554,6 +556,17 @@ JNI_METHOD(void, updateCommissioningNetworkCredentials)
         ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format());
         JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err);
     }
+
+    // Only invoke NetworkCredentialsReady when called in response to NetworkScan result
+    if (wrapper->Controller()->GetCommissioningStage() == CommissioningStage::kNeedsNetworkCreds)
+    {
+        err = wrapper->Controller()->NetworkCredentialsReady();
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Controller, "NetworkCredentialsReady failed. Err = %" CHIP_ERROR_FORMAT, err.Format());
+            JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err);
+        }
+    }
 }
 
 JNI_METHOD(jbyteArray, convertX509CertToMatterCert)

diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
@@ -63,9 +63,17 @@ public void setScanNetworksListener(ScanNetworksListener listener) {
   }
 
   /**
-   * Sets this DeviceController to use the given issuer for issuing operational certs. By default,
-   * the DeviceController uses an internal, OperationalCredentialsDelegate (see
-   * AndroidOperationalCredentialsIssuer)
+   * Sets this DeviceController to use the given issuer for issuing operational certs and verifying
+   * the DAC. By default, the DeviceController uses an internal, OperationalCredentialsDelegate (see
+   * AndroidOperationalCredentialsIssuer).
+   *
+   * <p>When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be
+   * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials
+   * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform
+   * DAC verification and the CSR signing.
+   *
+   * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used rather
+   * than the DefaultDACVerifier.
    *
    * @param issuer
    */
@@ -692,8 +700,12 @@ protected void finalize() throws Throwable {
   public interface NOCChainIssuer {
     /**
      * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be
-     * called when the NOC CSR needs to be signed. This allows for custom credentials issuer
-     * implementations, for example, when a proprietary cloud API will perform the CSR signing.
+     * called when the DAC chain must be verified and NOC chain needs to be issued from a CSR. This
+     * allows for custom credentials issuer and DAC verifier implementations, for example, when a
+     * proprietary cloud API will perform DAC verification and the NOC chain issuance from CSR.
+     *
+     * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used
+     * rather than the DefaultDACVerifier.
      *
      * <p>The commissioning workflow will stop upon the onNOCChainGenerationNeeded callback and
      * resume once onNOCChainGeneration is called.
@@ -716,6 +728,11 @@ public interface NOCChainIssuer {
    * <p>Set the AttemptNetworkScanWiFi or AttemptNetworkScanThread to configure the enable/disable
    * WiFi or Thread network scan during commissioning in the the default CommissioningDelegate used
    * by the ChipDeviceCommissioner.
+   *
+   * <p>When the callbacks onScanNetworksFailure or onScanNetworksSuccess are invoked, the
+   * commissioning flow has reached the kNeedsNetworkCreds and will wait to advance until this
+   * device controller's updateCommissioningNetworkCredentials method is called with the desired
+   * network credentials set.
    */
   public interface ScanNetworksListener {
     /** Notifies when scan networks call fails. */

diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp
@@ -36,6 +36,10 @@ namespace Credentials {
 // As per specifications section 11.22.5.1. Constant RESP_MAX
 constexpr size_t kMaxResponseLength = 900;
 
+/**
+ * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks
+ * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h.
+ */
 void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
                                                       Callback::Callback<OnAttestationInformationVerification> * onCompletion)
 {
@@ -144,7 +148,7 @@ void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
     }
 
 exit:
-    onCompletion->mCall(onCompletion->mContext, attestationError); // TODO: is this check getting done?
+    onCompletion->mCall(onCompletion->mContext, attestationError);
 }
 
 } // namespace Credentials

diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h
@@ -21,11 +21,37 @@
 namespace chip {
 namespace Credentials {
 
+/**
+ * @brief
+ *   This class is based upon the DefaultDACVerifier but has all checks removed which require
+ * local availability of trust anchors that are not available from the commissionee, such as the
+ * PAA root certificates and the CSA keys used to sign the Certification Declaration (CD).
+ *
+ *   This class should only be used in conjunction with an OperationalCredentialsDelegate
+ * which performs the removed checks. For example, an OperationalCredentialsDelegate implementation
+ * might send the DAC chain and signed CD to custom code which obtains these keys from the DCL.
+ *
+ * Specifically, the following list of checks have been removed:
+ * (1) Make sure the PAA is valid and approved by CSA.
+ * (2) vid-scoped PAA check: if the PAA is vid scoped, then its vid must match the DAC vid.
+ * (3) cert chain check: verify PAI is signed by PAA, and DAC is signed by PAI.
+ * (4) PAA subject key id extraction: the PAA subject key must match the PAA key referenced in the PAI.
+ * (5) CD signature check: make sure a valid CSA CD key is used to sign the CD.
+ *
+ * Any other checks performed by the DefaultDACVerifier should be performed here too. Changes
+ * made to DefaultDACVerifier::VerifyAttestationInformation should be made to
+ * PartialDACVerifier::VerifyAttestationInformation.
+ */
 class PartialDACVerifier : public DefaultDACVerifier
 {
 public:
     PartialDACVerifier() {}
 
+    /**
+     * @brief
+     * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks
+     * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h.
+     */
     void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
                                       Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;