Fixed issues in revocation check and code cleanup

Author: vijs

src/controller/AutoCommissioner.cpp

@@ -389,8 +389,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
     case CommissioningStage::kSendAttestationRequest:
         return CommissioningStage::kAttestationVerification;
     case CommissioningStage::kAttestationVerification:
-        return mBypassDeviceAttestation ? CommissioningStage::kSendOpCertSigningRequest
-                                        : CommissioningStage::kAttestationRevocationCheck;
+        return CommissioningStage::kAttestationRevocationCheck;
     case CommissioningStage::kAttestationRevocationCheck:
         return CommissioningStage::kSendOpCertSigningRequest;
     case CommissioningStage::kSendOpCertSigningRequest:
@@ -582,7 +581,6 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
     mStopCommissioning       = false;
-    mBypassDeviceAttestation = false;
     mCommissioner            = commissioner;
     mCommissioneeDeviceProxy = proxy;
     mNeedsNetworkSetup =
@@ -697,7 +695,8 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
                              "Failed device attestation. Device vendor and/or product ID do not match the IDs expected. "
                              "Verify DAC certificate chain and certification declaration to ensure spec rules followed.");
             }
-            else if (report.stageCompleted == CommissioningStage::kAttestationVerification)
+
+            if (report.stageCompleted == CommissioningStage::kAttestationVerification)
             {
                 ChipLogError(Controller, "Failed verifying attestation information. Now checking DAC chain revoked status.");
                 // don't error out until we check for DAC chain revocation status

src/controller/AutoCommissioner.h

@@ -38,7 +38,6 @@ class AutoCommissioner : public CommissioningDelegate
 
     CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override;
     void StopCommissioning() { mStopCommissioning = true; };
-    void BypassDeviceAttestation() override { mBypassDeviceAttestation = true; }
 
     CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override;
 
@@ -95,8 +94,7 @@ class AutoCommissioner : public CommissioningDelegate
                  mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId));
     };
 
-    bool mStopCommissioning       = false;
-    bool mBypassDeviceAttestation = false;
+    bool mStopCommissioning = false;
 
     DeviceCommissioner * mCommissioner                               = nullptr;
     CommissioneeDeviceProxy * mCommissioneeDeviceProxy               = nullptr;

src/controller/CHIPDeviceController.cpp

@@ -947,8 +947,7 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de
         return CHIP_ERROR_INCORRECT_STATE;
     }
 
-    if (mCommissioningStage != CommissioningStage::kAttestationVerification &&
-        mCommissioningStage != CommissioningStage::kAttestationRevocationCheck)
+    if (mCommissioningStage != CommissioningStage::kAttestationRevocationCheck)
     {
         ChipLogError(Controller, "Commissioning is not attestation verification phase");
         return CHIP_ERROR_INCORRECT_STATE;
@@ -957,8 +956,6 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de
     ChipLogProgress(Controller, "Continuing commissioning after attestation failure for device ID 0x" ChipLogFormatX64,
                     ChipLogValueX64(commissioneeDevice->GetDeviceId()));
 
-    mCommissioningDelegate->BypassDeviceAttestation();
-
     if (attestationResult != AttestationVerificationResult::kSuccess)
     {
         ChipLogError(Controller, "Client selected error: %u for failed 'Attestation Information' for device",
@@ -1198,8 +1195,14 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
     auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
     Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
 
-    result =
-        params.GetCompletionStatus().attestationResult.HasValue() ? params.GetCompletionStatus().attestationResult.Value() : result;
+    if (params.GetCompletionStatus().attestationResult.HasValue())
+    {
+        auto previousResult = params.GetCompletionStatus().attestationResult.Value();
+        if (previousResult != AttestationVerificationResult::kSuccess)
+        {
+            result = previousResult;
+        }
+    }
 
     if (result != AttestationVerificationResult::kSuccess)
     {
@@ -3066,7 +3069,12 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     }
     case CommissioningStage::kAttestationVerification: {
         ChipLogProgress(Controller, "Verifying attestation");
-        VerifyOrReturn(IsAttestationInformationMissing(params) == false);
+        if (IsAttestationInformationMissing(params))
+        {
+            ChipLogError(Controller, "Missing attestation information");
+            CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
+            return;
+        }
 
         DeviceAttestationVerifier::AttestationInfo info(
             params.GetAttestationElements().Value(),
@@ -3084,7 +3092,12 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     break;
     case CommissioningStage::kAttestationRevocationCheck: {
         ChipLogProgress(Controller, "Verifying device's DAC chain revocation status");
-        VerifyOrReturn(IsAttestationInformationMissing(params) == false);
+        if (IsAttestationInformationMissing(params))
+        {
+            ChipLogError(Controller, "Missing attestation information");
+            CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
+            return;
+        }
 
         DeviceAttestationVerifier::AttestationInfo info(
             params.GetAttestationElements().Value(),
@@ -3464,14 +3477,12 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device,
     }
 }
 
-bool DeviceCommissioner::IsAttestationInformationMissing(CommissioningParameters & params)
+bool DeviceCommissioner::IsAttestationInformationMissing(const CommissioningParameters & params)
 {
     if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
         !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
         !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
     {
-        ChipLogError(Controller, "Missing attestation information");
-        CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
         return true;
     }
 

src/controller/CHIPDeviceController.h

@@ -1061,7 +1061,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     // extend it).
     void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step);
 
-    bool IsAttestationInformationMissing(CommissioningParameters & params);
+    bool IsAttestationInformationMissing(const CommissioningParameters & params);
 
     chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
     chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;

src/controller/CommissioningDelegate.h

@@ -812,7 +812,6 @@ class CommissioningDelegate
     virtual void SetOperationalCredentialsDelegate(OperationalCredentialsDelegate * operationalCredentialsDelegate) = 0;
     virtual CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy)       = 0;
     virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningReport report)                        = 0;
-    virtual void BypassDeviceAttestation()                                                                          = 0;
 };
 
 } // namespace Controller

src/controller/python/OpCredsBinding.cpp

@@ -331,6 +331,8 @@ class TestCommissioner : public chip::Controller::AutoCommissioner
             return mNeedsDST && mParams.GetDSTOffsets().HasValue();
         case chip::Controller::CommissioningStage::kError:
         case chip::Controller::CommissioningStage::kSecurePairing:
+        // "not valid" because attestation verification always fails after entering revocation check step
+        case chip::Controller::CommissioningStage::kAttestationVerification:
             return false;
         default:
             return true;

src/controller/python/test/test_scripts/commissioning_failure_test.py

@@ -96,7 +96,7 @@ def main():
 
     # TODO: Start at stage 2 once handling for arming failsafe on pase is done.
     if options.report:
-        for testFailureStage in range(3, 20):
+        for testFailureStage in range(3, 21):
             FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1,
                                         setuppin=20202021,
                                         nodeid=1),
@@ -105,7 +105,7 @@ def main():
                       "Commissioning failure tests failed for simulated report failure on stage {}".format(testFailureStage))
 
     else:
-        for testFailureStage in range(3, 20):
+        for testFailureStage in range(3, 21):
             FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1,
                                         setuppin=20202021,
                                         nodeid=1),

src/python_testing/TC_CGEN_2_4.py

@@ -34,9 +34,9 @@
 kSendPAICertificateRequest = 10
 kSendDACCertificateRequest = 11
 kSendAttestationRequest = 12
-kSendOpCertSigningRequest = 14
-kSendTrustedRootCert = 17
-kSendNOC = 18
+kSendOpCertSigningRequest = 15
+kSendTrustedRootCert = 18
+kSendNOC = 19
 
 
 class TC_CGEN_2_4(MatterBaseTest):