-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 22318 - commissioner attestation delegate should be able to override success #22321
Issue 22318 - commissioner attestation delegate should be able to override success #22321
Conversation
PR #22321: Size comparison from 4e01cee to ea82080 Increases (5 builds for bl602, cc13x2_26x2, linux, telink)
Decreases (1 build for cc13x2_26x2)
Full report (20 builds for bl602, cc13x2_26x2, k32w, linux, mbed, nrfconnect, qpg, telink)
|
5f321a6
to
5a8aa81
Compare
PR #22321: Size comparison from 8873a20 to 5a8aa81 Increases (6 builds for esp32, linux, telink)
Decreases (3 builds for psoc6)
Full report (45 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
src/credentials/attestation_verifier/DeviceAttestationVerifier.h
Outdated
Show resolved
Hide resolved
src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate_Internal.h
Outdated
Show resolved
Hide resolved
3996724
to
aa32f4b
Compare
PR #22321: Size comparison from 8873a20 to aa32f4b Increases (9 builds for bl602, cc13x2_26x2, linux, psoc6, telink)
Decreases (4 builds for cc13x2_26x2, esp32, nrfconnect, psoc6)
Full report (45 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
src/credentials/attestation_verifier/DeviceAttestationDelegate.h
Outdated
Show resolved
Hide resolved
src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm
Outdated
Show resolved
Hide resolved
@jtung-apple - this is a core code change. Please help me triage it for 1.0 acceptance by providing 1.0 justification:
|
@jtung-apple - regarding |
src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
src/credentials/attestation_verifier/DeviceAttestationDelegate.h
Outdated
Show resolved
Hide resolved
src/credentials/attestation_verifier/DeviceAttestationVerifier.h
Outdated
Show resolved
Hide resolved
src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm
Outdated
Show resolved
Hide resolved
aa32f4b
to
711527f
Compare
PR #22321: Size comparison from 3522317 to 18cfcc0 Increases (10 builds for bl602, cc13x2_26x2, linux, psoc6)
Decreases (1 build for cc13x2_26x2)
Full report (45 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
@andy31415 I've also verified that chip-tool continues to commission correctly. |
PR #22321: Size comparison from 3522317 to 27ee7d2 Increases (6 builds for cc13x2_26x2)
Decreases (25 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, nrfconnect, psoc6)
Full report (26 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, psoc6)
|
|
|
@andy31415 For bookkeeping - I've filed new Issue #22423 with template and copied justification over and linked to this PR. |
src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
@jtung-apple thanks for the new issue template, approving. |
…rride success (project-chip#22321) * Issue 22318 - commissioner attestation delegate should be able to override success * restyled * hook up darwin delegate when commissioning * restyled * header doc and nullability fix for darwin MTRDeviceAttestationDelegate * NULL check before memcpy Co-authored-by: Boris Zbarsky <[email protected]> * Declare const member as const in MTRDeviceAttestationDelegateBridge Co-authored-by: Boris Zbarsky <[email protected]> * Address review comments Co-authored-by: Boris Zbarsky <[email protected]>
Problem
Copying problem description from issue #22318:
The SDK currently allows an attestation delegate to override an attestation failure in response to user preference. The attestation delegate should also be able to override attestation success, in case application logic decides to stop commissioning a device.
For example, if the application keeps a more up-to-date revocation list, it can use that information to reject commissioning particular devices.
For this to work, the completion callback for the delegate should include DAC/PAI/CD for the device.
Edit for justification:
why is this important
This is required for third party developers on iOS to verify device attestation.
what is the sideffect if we decide to not accept this change into 1.0 branch or if we fail to finish its development?
Once commissioning starts, third party developers would not be able to reject attestation results, even if they have more verification information
/Edit
Change overview
DeviceAttestationDelegate
add an optional override to use the new flowAttestationDeviceInfo
object to hold a copy of the device informationDeviceCommissioner
check if the delegate uses the new flow, and always pass the device informationTesting