diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index be7f03bdd8d30f..423c77a005a110 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -249,56 +249,62 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio if (err != CHIP_NO_ERROR) { ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast(report.stageCompleted)); - return err; } - switch (report.stageCompleted) + else { - case CommissioningStage::kSendPAICertificateRequest: - SetPAI(report.Get().certificate); - break; - case CommissioningStage::kSendDACCertificateRequest: - SetDAC(report.Get().certificate); - break; - case CommissioningStage::kSendAttestationRequest: - // These don't need to be deep copied to local memory because they are used in this one step then never again. - mParams.SetAttestationElements(report.Get().attestationElements) - .SetAttestationSignature(report.Get().signature); - // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we woouldn't need to hold a - // reference to the operational credential delegate here - if (mOperationalCredentialsDelegate != nullptr) + switch (report.stageCompleted) { - MutableByteSpan nonce(mCSRNonce); - ReturnErrorOnFailure(mOperationalCredentialsDelegate->ObtainCsrNonce(nonce)); - mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce))); + case CommissioningStage::kSendPAICertificateRequest: + SetPAI(report.Get().certificate); + break; + case CommissioningStage::kSendDACCertificateRequest: + SetDAC(report.Get().certificate); + break; + case CommissioningStage::kSendAttestationRequest: + // These don't need to be deep copied to local memory because they are used in this one step then never again. + mParams.SetAttestationElements(report.Get().attestationElements) + .SetAttestationSignature(report.Get().signature); + // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a + // reference to the operational credential delegate here + if (mOperationalCredentialsDelegate != nullptr) + { + MutableByteSpan nonce(mCSRNonce); + ReturnErrorOnFailure(mOperationalCredentialsDelegate->ObtainCsrNonce(nonce)); + mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce))); + } + break; + case CommissioningStage::kSendOpCertSigningRequest: { + NOCChainGenerationParameters nocParams; + nocParams.nocsrElements = report.Get().attestationElements; + nocParams.signature = report.Get().signature; + mParams.SetNOCChainGenerationParameters(nocParams); } break; - case CommissioningStage::kSendOpCertSigningRequest: { - NOCChainGenerationParameters nocParams; - nocParams.nocsrElements = report.Get().attestationElements; - nocParams.signature = report.Get().signature; - mParams.SetNOCChainGenerationParameters(nocParams); - } - break; - case CommissioningStage::kGenerateNOCChain: - // For NOC chain generation, we re-use the buffers. NOCChainGenerated triggers the next stage before - // storing the returned certs, so just return here without triggering the next stage. - return NOCChainGenerated(report.Get().noc, report.Get().icac, report.Get().rcac, - report.Get().ipk, report.Get().adminSubject); - case CommissioningStage::kFindOperational: - mOperationalDeviceProxy = report.Get().operationalProxy; - break; - case CommissioningStage::kCleanup: - ReleasePAI(); - ReleaseDAC(); - mCommissioneeDeviceProxy = nullptr; - mOperationalDeviceProxy = nullptr; - mParams = CommissioningParameters(); - return CHIP_NO_ERROR; - default: - break; + case CommissioningStage::kGenerateNOCChain: + // For NOC chain generation, we re-use the buffers. NOCChainGenerated triggers the next stage before + // storing the returned certs, so just return here without triggering the next stage. + return NOCChainGenerated(report.Get().noc, report.Get().icac, report.Get().rcac, + report.Get().ipk, report.Get().adminSubject); + case CommissioningStage::kFindOperational: + mOperationalDeviceProxy = report.Get().operationalProxy; + break; + case CommissioningStage::kCleanup: + ReleasePAI(); + ReleaseDAC(); + mCommissioneeDeviceProxy = nullptr; + mOperationalDeviceProxy = nullptr; + mParams = CommissioningParameters(); + return CHIP_NO_ERROR; + default: + break; + } } CommissioningStage nextStage = GetNextCommissioningStage(report.stageCompleted, err); + if (nextStage == CommissioningStage::kError) + { + return CHIP_ERROR_INCORRECT_STATE; + } DeviceProxy * proxy = mCommissioneeDeviceProxy; if (nextStage == CommissioningStage::kSendComplete || diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 3a430defe757bd..c2c0f04837252d 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -58,7 +58,6 @@ class AutoCommissioner : public CommissioningDelegate OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; CommissioningParameters mParams = CommissioningParameters(); // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory - // TODO(cecille): Include memory from CommissioneeDeviceProxy once BLE is moved over uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen]; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 05449f7733291c..daf962bb24d286 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -882,7 +882,7 @@ CHIP_ERROR DeviceCommissioner::Commission(NodeId remoteDeviceId, CommissioningPa if (device == nullptr || device->GetDeviceId() != remoteDeviceId || (!device->IsSecureConnected() && !device->IsSessionSetupInProgress())) { - ChipLogError(Controller, "Invalid device for commissioning" ChipLogFormatX64, ChipLogValueX64(remoteDeviceId)); + ChipLogError(Controller, "Invalid device for commissioning " ChipLogFormatX64, ChipLogValueX64(remoteDeviceId)); return CHIP_ERROR_INCORRECT_STATE; } if (mCommissioningStage != CommissioningStage::kSecurePairing) @@ -958,6 +958,7 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status) void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err) { + // PASE session establishment failure. mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this); if (mPairingDelegate != nullptr) @@ -970,6 +971,7 @@ void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err) void DeviceCommissioner::OnSessionEstablished() { + // PASE session established. VerifyOrReturn(mDeviceBeingCommissioned != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR)); PASESession * pairing = &mDeviceBeingCommissioned->GetPairing(); @@ -1023,7 +1025,7 @@ void DeviceCommissioner::OnCertificateChainFailureResponse(void * context, uint8 commissioner->mCertificateChainResponseCallback.Cancel(); commissioner->mOnCertificateChainFailureCallback.Cancel(); // TODO: Map error status to correct error code - commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL); + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL); } void DeviceCommissioner::OnCertificateChainResponse(void * context, ByteSpan certificate) @@ -1034,10 +1036,10 @@ void DeviceCommissioner::OnCertificateChainResponse(void * context, ByteSpan cer commissioner->mCertificateChainResponseCallback.Cancel(); commissioner->mOnCertificateChainFailureCallback.Cancel(); - CommissioningDelegate::CommissioningReport report(commissioner->mCommissioningStage); + CommissioningDelegate::CommissioningReport report; report.Set(RequestedCertificate(certificate)); - commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); } CHIP_ERROR DeviceCommissioner::SendAttestationRequestCommand(DeviceProxy * device, const ByteSpan & attestationNonce) @@ -1062,7 +1064,7 @@ void DeviceCommissioner::OnAttestationFailureResponse(void * context, uint8_t st commissioner->mAttestationResponseCallback.Cancel(); commissioner->mOnAttestationFailureCallback.Cancel(); // TODO: Map error status to correct error code - commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL); + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL); } void DeviceCommissioner::OnAttestationResponse(void * context, chip::ByteSpan attestationElements, chip::ByteSpan signature) @@ -1073,9 +1075,9 @@ void DeviceCommissioner::OnAttestationResponse(void * context, chip::ByteSpan at commissioner->mAttestationResponseCallback.Cancel(); commissioner->mOnAttestationFailureCallback.Cancel(); - CommissioningDelegate::CommissioningReport report(CommissioningStage::kSendAttestationRequest); + CommissioningDelegate::CommissioningReport report; report.Set(AttestationResponse(attestationElements, signature)); - commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); } void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * context, AttestationVerificationResult result) @@ -1152,7 +1154,7 @@ void DeviceCommissioner::OnCSRFailureResponse(void * context, uint8_t status) commissioner->mOpCSRResponseCallback.Cancel(); commissioner->mOnCSRFailureCallback.Cancel(); // TODO: Map error status to correct error code - commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL); + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL); } void DeviceCommissioner::OnOperationalCertificateSigningRequest(void * context, ByteSpan NOCSRElements, @@ -1164,19 +1166,16 @@ void DeviceCommissioner::OnOperationalCertificateSigningRequest(void * context, commissioner->mOpCSRResponseCallback.Cancel(); commissioner->mOnCSRFailureCallback.Cancel(); - CommissioningDelegate::CommissioningReport report(CommissioningStage::kSendOpCertSigningRequest); + CommissioningDelegate::CommissioningReport report; report.Set(AttestationResponse(NOCSRElements, AttestationSignature)); - commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); } void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac, Optional ipk, Optional adminSubject) { - CHIP_ERROR err = CHIP_NO_ERROR; - DeviceCommissioner * commissioner = static_cast(context); - CommissioningDelegate::CommissioningReport report(CommissioningStage::kGenerateNOCChain); // TODO(#13825): If not passed by the signer, the commissioner should // provide its current IPK to the commissionee in the AddNOC command. @@ -1184,19 +1183,20 @@ void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR s 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; ChipLogProgress(Controller, "Received callback from the CA for NOC Chain generation. Status %s", ErrorStr(status)); - VerifyOrExit(commissioner->mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE); + if (commissioner->mState != State::Initialized) + { + status = CHIP_ERROR_INCORRECT_STATE; + } + if (status != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed in generating device's operational credentials. Error %s", ErrorStr(status)); + } // TODO - Verify that the generated root cert matches with commissioner's root cert - + CommissioningDelegate::CommissioningReport report; report.Set(NocChain(noc, icac, rcac, ipk.HasValue() ? ipk.Value() : AesCcm128KeySpan(placeHolderIpk), adminSubject.HasValue() ? adminSubject.Value() : commissioner->GetNodeId())); - err = commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); -exit: - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Failed in generating device's operational credentials. Error %s", ErrorStr(err)); - commissioner->OnSessionEstablishmentError(err); - } + commissioner->CommissioningStageComplete(status, report); } CHIP_ERROR DeviceCommissioner::ProcessOpCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, @@ -1280,7 +1280,7 @@ void DeviceCommissioner::OnAddNOCFailureResponse(void * context, uint8_t status) commissioner->mOpCSRResponseCallback.Cancel(); commissioner->mOnCertFailureCallback.Cancel(); // TODO: Map error status to correct error code - commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL); + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL); } void DeviceCommissioner::OnOperationalCertificateAddResponse(void * context, uint8_t StatusCode, uint8_t FabricIndex, @@ -1310,7 +1310,7 @@ void DeviceCommissioner::OnOperationalCertificateAddResponse(void * context, uin if (err != CHIP_NO_ERROR) { ChipLogProgress(Controller, "Add NOC failed with error %s", ErrorStr(err)); - commissioner->OnSessionEstablishmentError(err); + commissioner->CommissioningStageComplete(err); } } @@ -1341,8 +1341,7 @@ void DeviceCommissioner::OnRootCertSuccessResponse(void * context) commissioner->mRootCertResponseCallback.Cancel(); commissioner->mOnRootCertFailureCallback.Cancel(); - CommissioningDelegate::CommissioningReport report(CommissioningStage::kSendTrustedRootCert); - commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR); } void DeviceCommissioner::OnRootCertFailureResponse(void * context, uint8_t status) @@ -1352,7 +1351,7 @@ void DeviceCommissioner::OnRootCertFailureResponse(void * context, uint8_t statu commissioner->mRootCertResponseCallback.Cancel(); commissioner->mOnRootCertFailureCallback.Cancel(); // TODO: Map error status to correct error code - commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL); + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL); } CHIP_ERROR DeviceCommissioner::OnOperationalCredentialsProvisioningCompletion(CommissioneeDeviceProxy * device) @@ -1475,20 +1474,25 @@ void BasicFailure(void * context, uint8_t status) { ChipLogProgress(Controller, "Received failure response %d\n", (int) status); DeviceCommissioner * commissioner = static_cast(context); - commissioner->OnSessionEstablishmentError(static_cast(status)); + commissioner->CommissioningStageComplete(static_cast(status)); } -void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err) +void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) { if (mCommissioningDelegate == nullptr) { return; } - CommissioningDelegate::CommissioningReport report(mCommissioningStage); - CHIP_ERROR status = mCommissioningDelegate->CommissioningStepFinished(err, report); + report.stageCompleted = mCommissioningStage; + CHIP_ERROR status = mCommissioningDelegate->CommissioningStepFinished(err, report); if (status != CHIP_NO_ERROR) { - OnSessionEstablishmentError(status); + // Commissioning delegate will only return error if it failed to perform the appropriate commissioning step. + // In this case, we should call back the commissioning complete and call session error + if (mPairingDelegate != nullptr) + { + mPairingDelegate->OnCommissioningComplete(mDeviceBeingCommissioned->GetDeviceId(), status); + } } } @@ -1499,14 +1503,6 @@ void DeviceCommissioner::OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & ChipLogValueX64(nodeData.mPeerId.GetNodeId())); VerifyOrReturn(mState == State::Initialized); - if (mDeviceBeingCommissioned != nullptr && mDeviceBeingCommissioned->GetDeviceId() == nodeData.mPeerId.GetNodeId()) - { - // Let's release the device that's being paired, if pairing was successful, - // and the device is available on the operational network. - ReleaseCommissioneeDevice(mDeviceBeingCommissioned); - mDeviceBeingCommissioned = nullptr; - } - mDNSCache.Insert(nodeData); mCASESessionManager->FindOrEstablishSession(nodeData.mPeerId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback); @@ -1520,7 +1516,7 @@ void DeviceCommissioner::OnNodeIdResolutionFailed(const chip::PeerId & peer, CHI CommissioneeDeviceProxy * device = mDeviceBeingCommissioned; if (device->GetDeviceId() == peer.GetNodeId() && mCommissioningStage == CommissioningStage::kFindOperational) { - OnSessionEstablishmentError(error); + CommissioningStageComplete(error); } } DeviceController::OnNodeIdResolutionFailed(peer, error); @@ -1530,16 +1526,25 @@ void DeviceCommissioner::OnNodeIdResolutionFailed(const chip::PeerId & peer, CHI void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) { + // CASE session established. DeviceCommissioner * commissioner = static_cast(context); VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connected callback with null context. Ignoring")); if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational) { - if (commissioner->mCommissioningDelegate != nullptr) + if (commissioner->mDeviceBeingCommissioned != nullptr && + commissioner->mDeviceBeingCommissioned->GetDeviceId() == device->GetDeviceId()) { - CommissioningDelegate::CommissioningReport report(CommissioningStage::kFindOperational); - report.Set(OperationalNodeFoundData(device)); - commissioner->mCommissioningDelegate->CommissioningStepFinished(CHIP_NO_ERROR, report); + // Let's release the device that's being paired, if pairing was successful, + // and the device is available on the operational network. + commissioner->ReleaseCommissioneeDevice(commissioner->mDeviceBeingCommissioned); + commissioner->mDeviceBeingCommissioned = nullptr; + if (commissioner->mCommissioningDelegate != nullptr) + { + CommissioningDelegate::CommissioningReport report; + report.Set(OperationalNodeFoundData(device)); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); + } } } else @@ -1552,13 +1557,24 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peerId, CHIP_ERROR error) { + // CASE session establishment failed. DeviceCommissioner * commissioner = static_cast(context); ChipLogProgress(Controller, "Device connection failed. Error %s", ErrorStr(error)); VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connection failure callback with null context. Ignoring")); VerifyOrReturn(commissioner->mPairingDelegate != nullptr, ChipLogProgress(Controller, "Device connection failure callback with null pairing delegate. Ignoring")); - commissioner->mPairingDelegate->OnCommissioningComplete(peerId.GetNodeId(), error); + + commissioner->mCASESessionManager->ReleaseSession(peerId); + if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational && + commissioner->mCommissioningDelegate != nullptr) + { + commissioner->CommissioningStageComplete(error); + } + else + { + commissioner->mPairingDelegate->OnPairingComplete(error); + } } void DeviceCommissioner::SetupCluster(ClusterBase & base, DeviceProxy * proxy, EndpointId endpoint, @@ -1650,8 +1666,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio if (!params.GetAttestationNonce().HasValue()) { ChipLogError(Controller, "No attestation nonce found"); - CommissioningDelegate::CommissioningReport report(step); - mCommissioningDelegate->CommissioningStepFinished(CHIP_ERROR_INVALID_ARGUMENT, report); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } SendAttestationRequestCommand(proxy, params.GetAttestationNonce().Value()); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 01f2bb13c38388..041fb1b3208638 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -584,7 +584,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, const ByteSpan & attestationNonce, const ByteSpan & pai, const ByteSpan & dac, DeviceProxy * proxy); - void CommissioningStageComplete(CHIP_ERROR err); + void + CommissioningStageComplete(CHIP_ERROR err, + CommissioningDelegate::CommissioningReport report = CommissioningDelegate::CommissioningReport()); #if CONFIG_NETWORK_LAYER_BLE /** diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 4e91ab072bb260..12bda4f6fb6ab9 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -239,7 +239,7 @@ class CommissioningDelegate struct CommissioningReport : Variant { - CommissioningReport(CommissioningStage stage) : stageCompleted(stage) {} + CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted; // TODO: Add other things the delegate needs to know. };