Skip to content

Commit f638fe2

Browse files
vijsvijs
committed
Implemented PR #31222 review comments
1 parent 11838c6 commit f638fe2

13 files changed

+102
-91
lines changed

‎docs/ERROR_CODES.md

+1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ This file was **AUTOMATICALLY** generated by
4747
| 29 | 0x1D | `CHIP_ERROR_INVALID_IPK` |
4848
| 30 | 0x1E | `CHIP_ERROR_INVALID_STRING_LENGTH` |
4949
| 31 | 0x1F | `CHIP_ERROR_INVALID_LIST_LENGTH` |
50+
| 32 | 0x20 | `CHIP_ERROR_FAILED_DEVICE_ATTESTATION` |
5051
| 33 | 0x21 | `CHIP_ERROR_END_OF_TLV` |
5152
| 34 | 0x22 | `CHIP_ERROR_TLV_UNDERRUN` |
5253
| 35 | 0x23 | `CHIP_ERROR_INVALID_TLV_ELEMENT` |

‎src/controller/AutoCommissioner.cpp

+3-1
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
388388
case CommissioningStage::kSendAttestationRequest:
389389
return CommissioningStage::kAttestationVerification;
390390
case CommissioningStage::kAttestationVerification:
391-
return CommissioningStage::kAttestationRevocationCheck;
391+
return mBypassDeviceAttestation ? CommissioningStage::kSendOpCertSigningRequest
392+
: CommissioningStage::kAttestationRevocationCheck;
392393
case CommissioningStage::kAttestationRevocationCheck:
393394
return CommissioningStage::kSendOpCertSigningRequest;
394395
case CommissioningStage::kSendOpCertSigningRequest:
@@ -577,6 +578,7 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione
577578
return CHIP_ERROR_INVALID_ARGUMENT;
578579
}
579580
mStopCommissioning = false;
581+
mBypassDeviceAttestation = false;
580582
mCommissioner = commissioner;
581583
mCommissioneeDeviceProxy = proxy;
582584
mNeedsNetworkSetup =

‎src/controller/AutoCommissioner.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ class AutoCommissioner : public CommissioningDelegate
3737

3838
CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override;
3939
void StopCommissioning() { mStopCommissioning = true; };
40+
void BypassDeviceAttestation() override { mBypassDeviceAttestation = true; }
4041

4142
CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override;
4243

@@ -92,7 +93,8 @@ class AutoCommissioner : public CommissioningDelegate
9293
mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId));
9394
};
9495

95-
bool mStopCommissioning = false;
96+
bool mStopCommissioning = false;
97+
bool mBypassDeviceAttestation = false;
9698

9799
DeviceCommissioner * mCommissioner = nullptr;
98100
CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr;

‎src/controller/CHIPDeviceController.cpp

+53-74
Original file line numberDiff line numberDiff line change
@@ -392,8 +392,8 @@ DeviceCommissioner::DeviceCommissioner() :
392392
mOnDeviceConnectionRetryCallback(OnDeviceConnectionRetryFn, this),
393393
#endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
394394
mDeviceAttestationInformationVerificationCallback(OnDeviceAttestationInformationVerification, this),
395-
mDACChainRevocationStatusVerificationCallback(OnDACChainRevocationStatusVerification, this),
396-
mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this)
395+
mDACChainRevocationStatusCallback(OnDACChainRevocationStatus, this), mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this),
396+
mSetUpCodePairer(this)
397397
{}
398398

399399
CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
@@ -888,6 +888,8 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de
888888
ChipLogProgress(Controller, "Continuing commissioning after attestation failure for device ID 0x" ChipLogFormatX64,
889889
ChipLogValueX64(commissioneeDevice->GetDeviceId()));
890890

891+
mCommissioningDelegate->BypassDeviceAttestation();
892+
891893
if (attestationResult != AttestationVerificationResult::kSuccess)
892894
{
893895
ChipLogError(Controller, "Client selected error: %u for failed 'Attestation Information' for device",
@@ -1107,6 +1109,22 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
11071109
MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
11081110
DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
11091111

1112+
if (commissioner->attestationInformationVerificationResult == AttestationVerificationResult::kNotImplemented)
1113+
{
1114+
commissioner->attestationInformationVerificationResult = result;
1115+
}
1116+
1117+
if (commissioner->attestationInformationVerificationResult == AttestationVerificationResult::kSuccess &&
1118+
commissioner->dacChainRevocationStatusResult == AttestationVerificationResult::kNotImplemented)
1119+
{
1120+
// Check for revoked DAC Chain before calling delegate. Enter next stage.
1121+
return commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
1122+
}
1123+
1124+
result = commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kSuccess
1125+
? commissioner->attestationInformationVerificationResult
1126+
: commissioner->dacChainRevocationStatusResult;
1127+
11101128
if (!commissioner->mDeviceBeingCommissioned)
11111129
{
11121130
ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device");
@@ -1156,64 +1174,21 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
11561174
}
11571175
}
11581176

1159-
void DeviceCommissioner::OnDACChainRevocationStatusVerification(
1160-
void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result)
1177+
void DeviceCommissioner::OnDACChainRevocationStatus(void * context,
1178+
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
1179+
AttestationVerificationResult result)
11611180
{
1162-
MATTER_TRACE_SCOPE("OnDACChainRevocationStatusVerification", "DeviceCommissioner");
1181+
MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
11631182
DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
11641183

1165-
if (!commissioner->mDeviceBeingCommissioned)
1184+
if (commissioner->dacChainRevocationStatusResult == AttestationVerificationResult::kNotImplemented)
11661185
{
1167-
ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device");
1168-
return;
1186+
commissioner->dacChainRevocationStatusResult = result;
11691187
}
11701188

1171-
auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
1172-
Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
1173-
1174-
if (result != AttestationVerificationResult::kSuccess)
1175-
{
1176-
CommissioningDelegate::CommissioningReport report;
1177-
report.Set<AttestationErrorInfo>(result);
1178-
if (result == AttestationVerificationResult::kNotImplemented)
1179-
{
1180-
ChipLogError(Controller,
1181-
"Failed in verifying 'DAC Chain Revocation Status' command received from the device due to default "
1182-
"DeviceAttestationVerifier Class not being overridden by a real implementation.");
1183-
commissioner->CommissioningStageComplete(CHIP_ERROR_NOT_IMPLEMENTED, report);
1184-
return;
1185-
}
1186-
1187-
ChipLogError(Controller,
1188-
"Failed in verifying 'DAC Chain Revocation Status' command received from the device: err %hu. Look at "
1189-
"AttestationVerificationResult enum to understand the errors",
1190-
static_cast<uint16_t>(result));
1191-
// Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to
1192-
// understand the errors.
1189+
VerifyOrReturn(commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kNotImplemented);
11931190

1194-
// If a device attestation status delegate is installed, delegate handling of failure to the client and let them decide on
1195-
// whether to proceed further or not.
1196-
if (deviceAttestationDelegate)
1197-
{
1198-
commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
1199-
}
1200-
else
1201-
{
1202-
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
1203-
}
1204-
}
1205-
else
1206-
{
1207-
if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation())
1208-
{
1209-
commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
1210-
}
1211-
else
1212-
{
1213-
ChipLogProgress(Controller, "Successfully validated 'DAC Chain Revocation Status' command received from the device.");
1214-
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
1215-
}
1216-
}
1191+
OnDeviceAttestationInformationVerification(context, info, commissioner->dacChainRevocationStatusResult);
12171192
}
12181193

12191194
void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation(
@@ -1363,13 +1338,13 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device
13631338
}
13641339

13651340
CHIP_ERROR
1366-
DeviceCommissioner::ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info)
1341+
DeviceCommissioner::CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info)
13671342
{
1368-
MATTER_TRACE_SCOPE("ValidateDACChainRevocationStatus", "DeviceCommissioner");
1343+
MATTER_TRACE_SCOPE("CheckForRevokedDACChain", "DeviceCommissioner");
13691344
VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
13701345
VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE);
13711346

1372-
mDeviceAttestationVerifier->ValidateDACChainRevocationStatus(info, &mDACChainRevocationStatusVerificationCallback);
1347+
mDeviceAttestationVerifier->CheckForRevokedDACChain(info, &mDACChainRevocationStatusCallback);
13731348

13741349
return CHIP_NO_ERROR;
13751350
}
@@ -2971,14 +2946,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
29712946
}
29722947
case CommissioningStage::kAttestationVerification: {
29732948
ChipLogProgress(Controller, "Verifying attestation");
2974-
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
2975-
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
2976-
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
2977-
{
2978-
ChipLogError(Controller, "Missing attestation information");
2979-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
2980-
return;
2981-
}
2949+
VerifyOrReturn(IsAttestationInformationMissing(params) == false);
2950+
2951+
// Reset results before verifying
2952+
attestationInformationVerificationResult = AttestationVerificationResult::kNotImplemented;
2953+
dacChainRevocationStatusResult = AttestationVerificationResult::kNotImplemented;
29822954

29832955
DeviceAttestationVerifier::AttestationInfo info(
29842956
params.GetAttestationElements().Value(),
@@ -2996,25 +2968,18 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
29962968
break;
29972969
case CommissioningStage::kAttestationRevocationCheck: {
29982970
ChipLogProgress(Controller, "Verifying device's DAC chain revocation status");
2999-
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
3000-
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
3001-
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
3002-
{
3003-
ChipLogError(Controller, "Missing attestation certificates");
3004-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
3005-
return;
3006-
}
2971+
VerifyOrReturn(IsAttestationInformationMissing(params) == false);
30072972

30082973
DeviceAttestationVerifier::AttestationInfo info(
30092974
params.GetAttestationElements().Value(),
30102975
proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(),
30112976
params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(),
30122977
params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value());
30132978

3014-
if (ValidateDACChainRevocationStatus(info) != CHIP_NO_ERROR)
2979+
if (CheckForRevokedDACChain(info) != CHIP_NO_ERROR)
30152980
{
30162981
ChipLogError(Controller, "Error validating device's DAC chain revocation status");
3017-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
2982+
CommissioningStageComplete(CHIP_ERROR_FAILED_DEVICE_ATTESTATION);
30182983
return;
30192984
}
30202985
}
@@ -3359,6 +3324,20 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device,
33593324
}
33603325
}
33613326

3327+
bool DeviceCommissioner::IsAttestationInformationMissing(CommissioningParameters & params)
3328+
{
3329+
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
3330+
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
3331+
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
3332+
{
3333+
ChipLogError(Controller, "Missing attestation information");
3334+
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
3335+
return true;
3336+
}
3337+
3338+
return false;
3339+
}
3340+
33623341
CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const
33633342
{
33643343
const auto * fabricInfo = GetFabricInfo();

‎src/controller/CHIPDeviceController.h

+11-5
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
610610
*
611611
* @param[in] info Structure contatining all the required information for validating the device attestation.
612612
*/
613-
CHIP_ERROR ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
613+
CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
614614

615615
/**
616616
* @brief
@@ -792,6 +792,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
792792

793793
ObjectPool<CommissioneeDeviceProxy, kNumMaxActiveDevices> mCommissioneeDevicePool;
794794

795+
Credentials::AttestationVerificationResult attestationInformationVerificationResult =
796+
Credentials::AttestationVerificationResult::kNotImplemented;
797+
Credentials::AttestationVerificationResult dacChainRevocationStatusResult =
798+
Credentials::AttestationVerificationResult::kNotImplemented;
799+
795800
#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
796801
UserDirectedCommissioningServer * mUdcServer = nullptr;
797802
// mUdcTransportMgr is for insecure communication (ex. user directed commissioning)
@@ -892,9 +897,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
892897
static void OnDeviceAttestationInformationVerification(void * context,
893898
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
894899
Credentials::AttestationVerificationResult result);
895-
static void OnDACChainRevocationStatusVerification(void * context,
896-
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
897-
Credentials::AttestationVerificationResult result);
900+
static void OnDACChainRevocationStatus(void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
901+
Credentials::AttestationVerificationResult result);
898902

899903
static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac,
900904
const ByteSpan & rcac, Optional<IdentityProtectionKeySpan> ipk,
@@ -1020,6 +1024,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
10201024
// extend it).
10211025
void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step);
10221026

1027+
bool IsAttestationInformationMissing(CommissioningParameters & params);
1028+
10231029
chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
10241030
chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
10251031
#if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
@@ -1029,7 +1035,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
10291035
chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
10301036
mDeviceAttestationInformationVerificationCallback;
10311037
chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
1032-
mDACChainRevocationStatusVerificationCallback;
1038+
mDACChainRevocationStatusCallback;
10331039

10341040
chip::Callback::Callback<OnNOCChainGeneration> mDeviceNOCChainCallback;
10351041
SetUpCodePairer mSetUpCodePairer;

‎src/controller/CommissioningDelegate.h

+2
Original file line numberDiff line numberDiff line change
@@ -761,6 +761,7 @@ class CommissioningDelegate
761761
* kSendDACCertificateRequest: RequestedCertificate
762762
* kSendAttestationRequest: AttestationResponse
763763
* kAttestationVerification: AttestationErrorInfo if there is an error
764+
* kAttestationRevocationCheck: AttestationErrorInfo if there is an error
764765
* kSendOpCertSigningRequest: CSRResponse
765766
* kGenerateNOCChain: NocChain
766767
* kSendTrustedRootCert: None
@@ -786,6 +787,7 @@ class CommissioningDelegate
786787
virtual void SetOperationalCredentialsDelegate(OperationalCredentialsDelegate * operationalCredentialsDelegate) = 0;
787788
virtual CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) = 0;
788789
virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningReport report) = 0;
790+
virtual void BypassDeviceAttestation() = 0;
789791
};
790792

791793
} // namespace Controller

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -607,12 +607,12 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa
607607
return CHIP_NO_ERROR;
608608
}
609609

610-
void DefaultDACVerifier::ValidateDACChainRevocationStatus(const AttestationInfo & info,
611-
Callback::Callback<OnAttestationInformationVerification> * onCompletion)
610+
void DefaultDACVerifier::CheckForRevokedDACChain(const AttestationInfo & info,
611+
Callback::Callback<OnAttestationInformationVerification> * onCompletion)
612612
{
613613
AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess;
614614

615-
// TODO
615+
// TODO(#xyz): Implement default version of CheckForRevokedDACChain
616616

617617
onCompletion->mCall(onCompletion->mContext, info, attestationError);
618618
}

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ class DefaultDACVerifier : public DeviceAttestationVerifier
7474
const ByteSpan & attestationSignatureBuffer,
7575
const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override;
7676

77-
void ValidateDACChainRevocationStatus(const AttestationInfo & info,
78-
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;
77+
void CheckForRevokedDACChain(const AttestationInfo & info,
78+
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;
7979

8080
CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; }
8181

‎src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,12 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier
7070
return CHIP_ERROR_NOT_IMPLEMENTED;
7171
}
7272

73-
void ValidateDACChainRevocationStatus(const AttestationInfo & info,
74-
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override
73+
void CheckForRevokedDACChain(const AttestationInfo & info,
74+
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override
7575
{
7676
(void) info;
7777
(void) onCompletion;
78+
VerifyOrDie(false);
7879
}
7980
};
8081

0 commit comments

Comments
 (0)