Skip to content

Commit

Permalink
Handle response errors during commissioning (#17778)
Browse files Browse the repository at this point in the history 
* Handle ArmFailSafe response error propgating to AutoCommissioner

* format fixing

* Fixes based on pull request comments

* Fix issue inside AutoCommissioner
* Remove map, as the errors were redundant

* Finish off plumbing error codes and networkstatus to AutoCommissioner

* OnSetRegulatoryConfigResponse
* OnNetworkConfigResponse
* OnConnectNetworkResponse
* OnCommissioningCompleteResponse

* Remove TODO about checking if static_cast is correct

I saw an example were static_cast was used on another
`enum class Asdf : uint8_t`, so this does seems like the
appraoch used elsewhere in this codebase

* Last little bit of cleanup to turn this into real pull request

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Fix formatting issues

Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@tehampson @bzbarsky-apple @pull
2 people authored and pull[bot] committed Jan 2, 2024
1 parent 57d9848 commit 2282872
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 43 deletions.
30 changes: 18 additions & 12 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,22 +363,28 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
{
completionStatus.failedStage = MakeOptional(report.stageCompleted);
ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast<int>(report.stageCompleted));
if (report.stageCompleted == CommissioningStage::kAttestationVerification)
if (report.Is<AttestationErrorInfo>())
{
if (report.Is<AdditionalErrorInfo>())
completionStatus.attestationResult = MakeOptional(report.Get<AttestationErrorInfo>().attestationResult);
if ((report.Get<AttestationErrorInfo>().attestationResult ==
Credentials::AttestationVerificationResult::kDacProductIdMismatch) ||
(report.Get<AttestationErrorInfo>().attestationResult ==
Credentials::AttestationVerificationResult::kDacVendorIdMismatch))
{
completionStatus.attestationResult = MakeOptional(report.Get<AdditionalErrorInfo>().attestationResult);
if ((report.Get<AdditionalErrorInfo>().attestationResult ==
Credentials::AttestationVerificationResult::kDacProductIdMismatch) ||
(report.Get<AdditionalErrorInfo>().attestationResult ==
Credentials::AttestationVerificationResult::kDacVendorIdMismatch))
{
ChipLogError(Controller,
"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.");
}
ChipLogError(Controller,
"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.Is<CommissioningErrorInfo>())
{
completionStatus.commissioningError = MakeOptional(report.Get<CommissioningErrorInfo>().commissioningError);
}
else if (report.Is<NetworkCommissioningStatusInfo>())
{
completionStatus.networkCommissioningStatus =
MakeOptional(report.Get<NetworkCommissioningStatusInfo>().networkCommissioningStatus);
}
}
else
{
Expand Down
76 changes: 57 additions & 19 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestationFailure(DevicePro
to_underlying(attestationResult));

CommissioningDelegate::CommissioningReport report;
report.Set<AdditionalErrorInfo>(attestationResult);
report.Set<AttestationErrorInfo>(attestationResult);
CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
}
else
Expand Down Expand Up @@ -914,7 +914,7 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte
if (result != AttestationVerificationResult::kSuccess)
{
CommissioningDelegate::CommissioningReport report;
report.Set<AdditionalErrorInfo>(result);
report.Set<AttestationErrorInfo>(result);
if (result == AttestationVerificationResult::kNotImplemented)
{
ChipLogError(Controller,
Expand Down Expand Up @@ -975,7 +975,7 @@ void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation(
{
ChipLogProgress(Controller, "Device attestation failed and no delegate set, failing commissioning");
CommissioningDelegate::CommissioningReport report;
report.Set<AdditionalErrorInfo>(commissioner->mAttestationResult);
report.Set<AttestationErrorInfo>(commissioner->mAttestationResult);
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
}
}
Expand All @@ -986,7 +986,7 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeFailedDeviceAttestation(vo
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);

CommissioningDelegate::CommissioningReport report;
report.Set<AdditionalErrorInfo>(commissioner->mAttestationResult);
report.Set<AttestationErrorInfo>(commissioner->mAttestationResult);
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
}

Expand Down Expand Up @@ -1455,6 +1455,8 @@ void DeviceCommissioner::SendCommissioningCompleteCallbacks(NodeId nodeId, const
}
else
{
// TODO: We should propogate detailed error information (commissioningError, networkCommissioningStatus) from
// completionStatus.
mPairingDelegate->OnCommissioningFailure(peerId, completionStatus.err, completionStatus.failedStage.ValueOr(kError),
completionStatus.attestationResult);
}
Expand Down Expand Up @@ -1710,46 +1712,82 @@ void DeviceCommissioner::OnDone()
void DeviceCommissioner::OnArmFailSafe(void * context,
const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data)
{
// TODO: Use errorCode
ChipLogProgress(Controller, "Received ArmFailSafe response");
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received ArmFailSafe response errorCode=%u", to_underlying(data.errorCode));
if (data.errorCode != GeneralCommissioning::CommissioningError::kOk)
{
err = CHIP_ERROR_INTERNAL;
report.Set<CommissioningErrorInfo>(data.errorCode);
}

DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnSetRegulatoryConfigResponse(
void * context, const GeneralCommissioning::Commands::SetRegulatoryConfigResponse::DecodableType & data)
{
// TODO: Use errorCode
ChipLogProgress(Controller, "Received SetRegulatoryConfig response");
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received SetRegulatoryConfig response errorCode=%u", to_underlying(data.errorCode));
if (data.errorCode != GeneralCommissioning::CommissioningError::kOk)
{
err = CHIP_ERROR_INTERNAL;
report.Set<CommissioningErrorInfo>(data.errorCode);
}
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnNetworkConfigResponse(void * context,
const NetworkCommissioning::Commands::NetworkConfigResponse::DecodableType & data)
{
// TODO: Use networkingStatus
ChipLogProgress(Controller, "Received NetworkConfig response");
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received NetworkConfig response, networkingStatus=%u", to_underlying(data.networkingStatus));
if (data.networkingStatus != NetworkCommissioning::NetworkCommissioningStatus::kSuccess)
{
err = CHIP_ERROR_INTERNAL;
report.Set<NetworkCommissioningStatusInfo>(data.networkingStatus);
}
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnConnectNetworkResponse(
void * context, const NetworkCommissioning::Commands::ConnectNetworkResponse::DecodableType & data)
{
// TODO: Use networkingStatus
ChipLogProgress(Controller, "Received ConnectNetwork response");
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received ConnectNetwork response, networkingStatus=%u", to_underlying(data.networkingStatus));
if (data.networkingStatus != NetworkCommissioning::NetworkCommissioningStatus::kSuccess)
{
err = CHIP_ERROR_INTERNAL;
report.Set<NetworkCommissioningStatusInfo>(data.networkingStatus);
}
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnCommissioningCompleteResponse(
void * context, const GeneralCommissioning::Commands::CommissioningCompleteResponse::DecodableType & data)
{
// TODO: Use errorCode
ChipLogProgress(Controller, "Received CommissioningComplete response");
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received CommissioningComplete response, errorCode=%u", to_underlying(data.errorCode));
if (data.errorCode != GeneralCommissioning::CommissioningError::kOk)
{
err = CHIP_ERROR_INTERNAL;
report.Set<CommissioningErrorInfo>(data.errorCode);
}
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, CommissioningStage step, CommissioningParameters & params,
Expand Down
41 changes: 29 additions & 12 deletions src/controller/CommissioningDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ struct CompletionStatus
CHIP_ERROR err;
Optional<CommissioningStage> failedStage;
Optional<Credentials::AttestationVerificationResult> attestationResult;
Optional<app::Clusters::GeneralCommissioning::CommissioningError> commissioningError;
Optional<app::Clusters::NetworkCommissioning::NetworkCommissioningStatus> networkCommissioningStatus;
};

constexpr uint16_t kDefaultFailsafeTimeout = 60;
Expand Down Expand Up @@ -447,38 +449,53 @@ struct ReadCommissioningInfo
GeneralCommissioningInfo general;
};

struct AdditionalErrorInfo
struct AttestationErrorInfo
{
AdditionalErrorInfo(Credentials::AttestationVerificationResult result) : attestationResult(result) {}
AttestationErrorInfo(Credentials::AttestationVerificationResult result) : attestationResult(result) {}
Credentials::AttestationVerificationResult attestationResult;
};

struct CommissioningErrorInfo
{
CommissioningErrorInfo(app::Clusters::GeneralCommissioning::CommissioningError result) : commissioningError(result) {}
app::Clusters::GeneralCommissioning::CommissioningError commissioningError;
};

struct NetworkCommissioningStatusInfo
{
NetworkCommissioningStatusInfo(app::Clusters::NetworkCommissioning::NetworkCommissioningStatus result) :
networkCommissioningStatus(result)
{}
app::Clusters::NetworkCommissioning::NetworkCommissioningStatus networkCommissioningStatus;
};

class CommissioningDelegate
{
public:
virtual ~CommissioningDelegate(){};
/* CommissioningReport is returned after each commissioning step is completed. The reports for each step are:
* kReadCommissioningInfo - ReadCommissioningInfo
* kArmFailsafe: none
* kConfigRegulatory: none
* kArmFailsafe: CommissioningErrorInfo if there is an error
* kConfigRegulatory: CommissioningErrorInfo if there is an error
* kSendPAICertificateRequest: RequestedCertificate
* kSendDACCertificateRequest: RequestedCertificate
* kSendAttestationRequest: AttestationResponse
* kAttestationVerification: AdditionalErrorInfo if there is an error
* kAttestationVerification: AttestationErrorInfo if there is an error
* kSendOpCertSigningRequest: CSRResponse
* kGenerateNOCChain: NocChain
* kSendTrustedRootCert: None
* kSendNOC: none
* kWiFiNetworkSetup: none
* kThreadNetworkSetup: none
* kWiFiNetworkEnable: none
* kThreadNetworkEnable: none
* kWiFiNetworkSetup: NetworkCommissioningStatusInfo if there is an error
* kThreadNetworkSetup: NetworkCommissioningStatusInfo if there is an error
* kWiFiNetworkEnable: NetworkCommissioningStatusInfo if there is an error
* kThreadNetworkEnable: NetworkCommissioningStatusInfo if there is an error
* kFindOperational: OperationalNodeFoundData
* kSendComplete: none
* kSendComplete: CommissioningErrorInfo if there is an error
* kCleanup: none
*/
struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData,
ReadCommissioningInfo, AdditionalErrorInfo>
struct CommissioningReport
: Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData, ReadCommissioningInfo,
AttestationErrorInfo, CommissioningErrorInfo, NetworkCommissioningStatusInfo>
{
CommissioningReport() : stageCompleted(CommissioningStage::kError) {}
CommissioningStage stageCompleted;
Expand Down

0 comments on commit 2282872

Please sign in to comment.