Removed the changes to add revocation set support to chip-tool and it will be submitted as a followup PR

vijs · vijs · commit 11838c6616a0 · 2024-03-15T12:44:42.000-04:00
diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn
@@ -109,7 +109,6 @@ static_library("chip-tool-utils") {
     "${chip_root}/src/app/tests/suites/commands/interaction_model",
     "${chip_root}/src/controller/data_model",
     "${chip_root}/src/credentials:file_attestation_trust_store",
-    "${chip_root}/src/credentials:json_file_revocation_set",
     "${chip_root}/src/lib",
     "${chip_root}/src/lib/core:types",
     "${chip_root}/src/lib/support/jsontlv",
diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h
@@ -69,9 +69,6 @@ class CHIPCommand : public Command
     CHIPCommand(const char * commandName, CredentialIssuerCommands * credIssuerCmds, const char * helpText = nullptr) :
         Command(commandName, helpText), mCredIssuerCmds(credIssuerCmds)
     {
-        AddArgument(
-            "revocation-set-path", &mRevocationSetPath,
-            "Path to file holding a list of Revoked Certificates. Can be absolute or relative to the current working directory.");
         AddArgument("paa-trust-store-path", &mPaaTrustStorePath,
                     "Path to directory holding PAA certificate information.  Can be absolute or relative to the current working "
                     "directory.");
@@ -221,7 +218,6 @@ class CHIPCommand : public Command
     chip::Optional<chip::NodeId> mCommissionerNodeId;
     chip::Optional<chip::VendorId> mCommissionerVendorId;
     chip::Optional<uint16_t> mBleAdapterId;
-    chip::Optional<char *> mRevocationSetPath;
     chip::Optional<char *> mPaaTrustStorePath;
     chip::Optional<char *> mCDTrustStorePath;
     chip::Optional<bool> mUseMaxSizedCerts;
@@ -230,7 +226,6 @@ class CHIPCommand : public Command
     // Cached trust store so commands other than the original startup command
     // can spin up commissioners as needed.
     static const chip::Credentials::AttestationTrustStore * sTrustStore;
-    static const chip::Credentials::RevocationSet * sRevocationSet;
 
     static void RunQueuedCommand(intptr_t commandArg);
     typedef decltype(RunQueuedCommand) MatterWorkCallback;
diff --git a/examples/chip-tool/commands/common/CredentialIssuerCommands.h b/examples/chip-tool/commands/common/CredentialIssuerCommands.h
@@ -59,8 +59,7 @@ class CredentialIssuerCommands
      * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code.
      */
     virtual CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams,
-                                              const chip::Credentials::AttestationTrustStore * trustStore,
-                                              const chip::Credentials::RevocationSet * revocationSet) = 0;
+                                              const chip::Credentials::AttestationTrustStore * trustStore) = 0;
 
     /**
      * @brief Add a list of additional non-default CD verifying keys (by certificate)
diff --git a/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h b/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h
@@ -34,12 +34,11 @@ class ExampleCredentialIssuerCommands : public CredentialIssuerCommands
         return mOpCredsIssuer.Initialize(storage);
     }
     CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams,
-                                      const chip::Credentials::AttestationTrustStore * trustStore,
-                                      const chip::Credentials::RevocationSet * revocationSet) override
+                                      const chip::Credentials::AttestationTrustStore * trustStore) override
     {
         chip::Credentials::SetDeviceAttestationCredentialsProvider(chip::Credentials::Examples::GetExampleDACProvider());
 
-        mDacVerifier                          = chip::Credentials::GetDefaultDACVerifier(trustStore, revocationSet);
+        mDacVerifier                          = chip::Credentials::GetDefaultDACVerifier(trustStore);
         setupParams.deviceAttestationVerifier = mDacVerifier;
         mDacVerifier->EnableCdTestKeySupport(mAllowTestCdSigningKey);
 
diff --git a/scripts/tools/check_includes_config.py b/scripts/tools/check_includes_config.py
@@ -134,7 +134,6 @@
 
     'src/credentials/attestation_verifier/FileAttestationTrustStore.h': {'vector'},
     'src/credentials/attestation_verifier/FileAttestationTrustStore.cpp': {'string'},
-    'src/credentials/attestation_verifier/JSONFileRevocationSet.cpp': {'fstream'},
 
     'src/setup_payload/AdditionalDataPayload.h': {'string'},
     'src/setup_payload/AdditionalDataPayloadParser.cpp': {'vector'},
diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -388,6 +388,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
     case CommissioningStage::kSendAttestationRequest:
         return CommissioningStage::kAttestationVerification;
     case CommissioningStage::kAttestationVerification:
+        return CommissioningStage::kAttestationRevocationCheck;
+    case CommissioningStage::kAttestationRevocationCheck:
         return CommissioningStage::kSendOpCertSigningRequest;
     case CommissioningStage::kSendOpCertSigningRequest:
         return CommissioningStage::kValidateCSR;
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -392,6 +392,7 @@ DeviceCommissioner::DeviceCommissioner() :
     mOnDeviceConnectionRetryCallback(OnDeviceConnectionRetryFn, this),
 #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
     mDeviceAttestationInformationVerificationCallback(OnDeviceAttestationInformationVerification, this),
+    mDACChainRevocationStatusVerificationCallback(OnDACChainRevocationStatusVerification, this),
     mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this)
 {}
 
@@ -877,7 +878,8 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de
         return CHIP_ERROR_INCORRECT_STATE;
     }
 
-    if (mCommissioningStage != CommissioningStage::kAttestationVerification)
+    if (mCommissioningStage != CommissioningStage::kAttestationVerification &&
+        mCommissioningStage != CommissioningStage::kAttestationRevocationCheck)
     {
         ChipLogError(Controller, "Commissioning is not attestation verification phase");
         return CHIP_ERROR_INCORRECT_STATE;
@@ -1146,14 +1148,69 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
         }
     }
     else
+    {
+        {
+            ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device.");
+            commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+        }
+    }
+}
+
+void DeviceCommissioner::OnDACChainRevocationStatusVerification(
+    void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result)
+{
+    MATTER_TRACE_SCOPE("OnDACChainRevocationStatusVerification", "DeviceCommissioner");
+    DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
+
+    if (!commissioner->mDeviceBeingCommissioned)
+    {
+        ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device");
+        return;
+    }
+
+    auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
+    Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
+
+    if (result != AttestationVerificationResult::kSuccess)
+    {
+        CommissioningDelegate::CommissioningReport report;
+        report.Set<AttestationErrorInfo>(result);
+        if (result == AttestationVerificationResult::kNotImplemented)
+        {
+            ChipLogError(Controller,
+                         "Failed in verifying 'DAC Chain Revocation Status' command received from the device due to default "
+                         "DeviceAttestationVerifier Class not being overridden by a real implementation.");
+            commissioner->CommissioningStageComplete(CHIP_ERROR_NOT_IMPLEMENTED, report);
+            return;
+        }
+
+        ChipLogError(Controller,
+                     "Failed in verifying 'DAC Chain Revocation Status' command received from the device: err %hu. Look at "
+                     "AttestationVerificationResult enum to understand the errors",
+                     static_cast<uint16_t>(result));
+        // Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to
+        // understand the errors.
+
+        // If a device attestation status delegate is installed, delegate handling of failure to the client and let them decide on
+        // whether to proceed further or not.
+        if (deviceAttestationDelegate)
+        {
+            commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
+        }
+        else
+        {
+            commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
+        }
+    }
+    else
     {
         if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation())
         {
             commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
         }
         else
         {
-            ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device.");
+            ChipLogProgress(Controller, "Successfully validated 'DAC Chain Revocation Status' command received from the device.");
             commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
         }
     }
@@ -1305,6 +1362,18 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR
+DeviceCommissioner::ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info)
+{
+    MATTER_TRACE_SCOPE("ValidateDACChainRevocationStatus", "DeviceCommissioner");
+    VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+    mDeviceAttestationVerifier->ValidateDACChainRevocationStatus(info, &mDACChainRevocationStatusVerificationCallback);
+
+    return CHIP_NO_ERROR;
+}
+
 CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements,
                                            const ByteSpan & AttestationSignature, const ByteSpan & dac, const ByteSpan & csrNonce)
 {
@@ -2925,6 +2994,31 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         }
     }
     break;
+    case CommissioningStage::kAttestationRevocationCheck: {
+        ChipLogProgress(Controller, "Verifying device's DAC chain revocation status");
+        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 certificates");
+            CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
+            return;
+        }
+
+        DeviceAttestationVerifier::AttestationInfo info(
+            params.GetAttestationElements().Value(),
+            proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(),
+            params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(),
+            params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value());
+
+        if (ValidateDACChainRevocationStatus(info) != CHIP_NO_ERROR)
+        {
+            ChipLogError(Controller, "Error validating device's DAC chain revocation status");
+            CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
+            return;
+        }
+    }
+    break;
     case CommissioningStage::kSendOpCertSigningRequest: {
         if (!params.GetCSRNonce().HasValue())
         {
diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -604,6 +604,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      */
     CHIP_ERROR ValidateAttestationInfo(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
 
+    /**
+     * @brief
+     *   This function validates the revocation status of the DAC Chain sent by the device.
+     *
+     * @param[in] info Structure contatining all the required information for validating the device attestation.
+     */
+    CHIP_ERROR ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
+
     /**
      * @brief
      * Sends CommissioningStepComplete report to the commissioning delegate. Function will fill in current step.
@@ -884,6 +892,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     static void OnDeviceAttestationInformationVerification(void * context,
                                                            const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
                                                            Credentials::AttestationVerificationResult result);
+    static void OnDACChainRevocationStatusVerification(void * context,
+                                                       const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
+                                                       Credentials::AttestationVerificationResult result);
 
     static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac,
                                            const ByteSpan & rcac, Optional<IdentityProtectionKeySpan> ipk,
@@ -1017,6 +1028,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
 
     chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
         mDeviceAttestationInformationVerificationCallback;
+    chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
+        mDACChainRevocationStatusVerificationCallback;
 
     chip::Callback::Callback<OnNOCChainGeneration> mDeviceNOCChainCallback;
     SetUpCodePairer mSetUpCodePairer;
diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h
@@ -46,6 +46,7 @@ enum CommissioningStage : uint8_t
     kSendDACCertificateRequest,  ///< Send DAC CertificateChainRequest (0x3E:2) command to the device
     kSendAttestationRequest,     ///< Send AttestationRequest (0x3E:0) command to the device
     kAttestationVerification,    ///< Verify AttestationResponse (0x3E:1) validity
+    kAttestationRevocationCheck, ///< Verify Revocation Status of device's DAC chain
     kSendOpCertSigningRequest,   ///< Send CSRRequest (0x3E:4) command to the device
     kValidateCSR,                ///< Verify CSRResponse (0x3E:5) validity
     kGenerateNOCChain,           ///< TLV encode Node Operational Credentials (NOC) chain certs
diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn
@@ -136,7 +136,6 @@ shared_library("ChipDeviceCtrl") {
     public_deps += [
       "${chip_root}/src/controller/data_model",
       "${chip_root}/src/credentials:file_attestation_trust_store",
-      "${chip_root}/src/credentials:json_file_revocation_set",
       "${chip_root}/src/lib/support:testing",
       "${chip_root}/src/tracing/json",
       "${chip_root}/src/tracing/perfetto",
diff --git a/src/credentials/BUILD.gn b/src/credentials/BUILD.gn
@@ -185,18 +185,3 @@ static_library("file_attestation_trust_store") {
     "${nlassert_root}:nlassert",
   ]
 }
-
-static_library("json_file_revocation_set") {
-  output_name = "libJSONFileRevocationSet"
-
-  sources = [
-    "attestation_verifier/JSONFileRevocationSet.cpp",
-    "attestation_verifier/JSONFileRevocationSet.h",
-  ]
-
-  public_deps = [
-    ":credentials",
-    "${chip_root}/third_party/jsoncpp",
-    "${nlassert_root}:nlassert",
-  ]
-}
diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp
@@ -458,20 +458,6 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
         VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = attestationError);
     }
 
-    {
-        attestationError = IsCertificateRevoked(true, paaVidPid, paaDerBuffer);
-        VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
-                     attestationError = AttestationVerificationResult::kPaaRevoked);
-
-        attestationError = IsCertificateRevoked(false, paiVidPid, info.paiDerBuffer);
-        VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
-                     attestationError = AttestationVerificationResult::kPaiRevoked);
-
-        attestationError = IsCertificateRevoked(false, dacVidPid, info.dacDerBuffer);
-        VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
-                     attestationError = AttestationVerificationResult::kDacRevoked);
-    }
-
 exit:
     onCompletion->mCall(onCompletion->mContext, info, attestationError);
 }
@@ -621,33 +607,14 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa
     return CHIP_NO_ERROR;
 }
 
-AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest,
-                                                                       ByteSpan certificate)
+void DefaultDACVerifier::ValidateDACChainRevocationStatus(const AttestationInfo & info,
+                                                          Callback::Callback<OnAttestationInformationVerification> * onCompletion)
 {
-    uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 };
-    MutableByteSpan issuer(issuerBuf);
-    uint8_t akidBuf[kAuthorityKeyIdentifierLength];
-    MutableByteSpan akid(akidBuf);
-    uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength];
-    MutableByteSpan serialNumber(serialNumberBuf);
-
-    VerifyOrReturnError(ExtractIssuerFromX509Cert(certificate, issuer) == CHIP_NO_ERROR,
-                        AttestationVerificationResult::kPaaFormatInvalid);
-    VerifyOrReturnError(ExtractAKIDFromX509Cert(certificate, akid) == CHIP_NO_ERROR,
-                        AttestationVerificationResult::kPaaFormatInvalid);
-    VerifyOrReturnError(ExtractSerialNumberFromX509Cert(certificate, serialNumber) == CHIP_NO_ERROR,
-                        AttestationVerificationResult::kPaaFormatInvalid);
-
-    return IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, akid, serialNumber);
-}
+    AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess;
 
-AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, AttestationCertVidPid vidPidUnderTest,
-                                                                       ByteSpan issuer, ByteSpan authorityKeyId,
-                                                                       ByteSpan serialNumber)
-{
-    VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kSuccess);
+    // TODO
 
-    return mRevocationSet->IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, authorityKeyId, serialNumber);
+    onCompletion->mCall(onCompletion->mContext, info, attestationError);
 }
 
 bool CsaCdKeysTrustStore::IsCdTestKey(const ByteSpan & kid) const
@@ -726,9 +693,9 @@ const AttestationTrustStore * GetTestAttestationTrustStore()
     return &gTestAttestationTrustStore.get();
 }
 
-DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet)
+DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore)
 {
-    static DefaultDACVerifier defaultDACVerifier{ paaRootStore, revocationSet };
+    static DefaultDACVerifier defaultDACVerifier{ paaRootStore };
 
     return &defaultDACVerifier;
 }
diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h
diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h
diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp
diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.h b/src/credentials/attestation_verifier/JSONFileRevocationSet.h

Original file line number	Diff line number	Diff line change
`@@ -34,12 +34,11 @@ class ExampleCredentialIssuerCommands : public CredentialIssuerCommands`
`34`	`34`	`return mOpCredsIssuer.Initialize(storage);`
`35`	`35`	`}`
`36`	`36`	`CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams,`
`37`		`- const chip::Credentials::AttestationTrustStore * trustStore,`
`38`		`- const chip::Credentials::RevocationSet * revocationSet) override`
	`37`	`+ const chip::Credentials::AttestationTrustStore * trustStore) override`
`39`	`38`	`{`
`40`	`39`	`chip::Credentials::SetDeviceAttestationCredentialsProvider(chip::Credentials::Examples::GetExampleDACProvider());`
`41`	`40`
`42`		`- mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore, revocationSet);`
	`41`	`+ mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore);`
`43`	`42`	`setupParams.deviceAttestationVerifier = mDacVerifier;`
`44`	`43`	`mDacVerifier->EnableCdTestKeySupport(mAllowTestCdSigningKey);`
`45`	`44`