Skip to content
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

Integrate secure pairing with example app and device controller #2230

Merged
merged 9 commits into from
Aug 20, 2020

Conversation

pan-apple
Copy link
Contributor

Problem

Missing secure pairing integration

Summary of Changes

Integrated secure pairing with accessory and device controller.

fixes #611
fixes #612

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2020

This pull request introduces 1 alert when merging f508a95 into 4af7537 - view on LGTM.com

new alerts:

  • 1 for Non-virtual destructor in base class

@Damian-Nordic
Copy link
Contributor

Do I understand correctly that this commit takes away the ability to test examples other than the echo-server as they don't implement rendezvous over BLE yet and the manual key exchange has been dropped?

I wonder if it would be possible to keep the manual key exchange path in the controller, possibly guarded by a compile-time switch, to support testing examples which are not ready yet. Perhaps even if all examples switch to a proper rendezvous method the possibility to test a connection with fixed encryption keys could be useful (?).

@pan-apple
Copy link
Contributor Author

Do I understand correctly that this commit takes away the ability to test examples other than the echo-server as they don't implement rendezvous over BLE yet and the manual key exchange has been dropped?

I wonder if it would be possible to keep the manual key exchange path in the controller, possibly guarded by a compile-time switch, to support testing examples which are not ready yet. Perhaps even if all examples switch to a proper rendezvous method the possibility to test a connection with fixed encryption keys could be useful (?).

@Damian-Nordic we can add provisions to have a test secret in the devices that do not yet have Rendezvous integrated. Here's how I am doing it in a test in the current PR (https://github.com/project-chip/connectedhomeip/pull/2230/files?file-filters%5B%5D=#diff-6827ec482c0a42a089c6e57eb3e087acR109). I prefer this over ManualKeyExchange, as the latter is not an actual API that we want to support in CHIP. The longer we keep ManualKeyExchange in the tree, more stuff will get built on top of it.

Do we have any test controller applications that are checked in CHIP repo that can interact with test device examples besides echo-server? I can help migrate the test and example application over to deriving security keys from test secret.

@github-actions
Copy link

Size increase report for "gn_nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "gn_linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "nrf-example-build"

File Section File VM
chip-nrf52840-lock-example.out [LOAD #2 [RW]] 0 4
chip-nrf52840-lock-example.out .bss 0 -992
chip-nrf52840-lock-example.out .text -1644 -1644
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.out and ./pull_artifact/chip-nrf52840-lock-example.out:

sections,vmsize,filesize
.debug_line,0,3054
[Unmapped],0,1645
.debug_macro,0,1198
.debug_str,0,825
.debug_info,0,791
.debug_ranges,0,64
[LOAD #2 [RW]],4,0
.debug_aranges,0,-64
.debug_abbrev,0,-98
.debug_frame,0,-244
.symtab,0,-720
.strtab,0,-740
.bss,-992,0
.debug_loc,0,-1263
.text,-1644,-1644


@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
chip-nrf52840-lock-example.elf [LOAD #2 [RW]] 4 4
chip-nrf52840-lock-example.elf [LOAD #1 [RWX]] -4 -4
chip-nrf52840-lock-example.elf datas -4 -4
chip-nrf52840-lock-example.elf rodata -172 -172
chip-nrf52840-lock-example.elf bss 0 -960
chip-nrf52840-lock-example.elf text -2332 -2332
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.elf and ./pull_artifact/chip-nrf52840-lock-example.elf:

sections,vmsize,filesize
.debug_str,0,672
.debug_frame,0,192
.debug_aranges,0,16
[LOAD #2 [RW]],4,4
[LOAD #1 [RWX]],-4,-4
datas,-4,-4
[Unmapped],0,-24
.debug_ranges,0,-112
rodata,-172,-172
.debug_abbrev,0,-584
.debug_line,0,-605
.symtab,0,-640
.strtab,0,-801
bss,-960,0
.debug_loc,0,-1617
.debug_info,0,-2077
text,-2332,-2332


@github-actions
Copy link

Size increase report for "linux-example-build"

File Section File VM
chip-standalone-demo.out .text 3712 3712
chip-standalone-demo.out .eh_frame 944 944
chip-standalone-demo.out .rela.dyn 336 336
chip-standalone-demo.out .rodata 232 232
chip-standalone-demo.out .eh_frame_hdr 176 176
chip-standalone-demo.out .data.rel.ro 128 128
chip-standalone-demo.out .data 32 32
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-standalone-demo.out and ./pull_artifact/chip-standalone-demo.out:

sections,vmsize,filesize
.debug_info,0,29301
.debug_loc,0,9883
.debug_line,0,9085
.debug_macro,0,4755
.debug_str,0,4586
.text,3712,3712
.debug_abbrev,0,3320
.strtab,0,1852
.debug_ranges,0,1472
.eh_frame,944,944
.symtab,0,672
.debug_aranges,0,400
.rela.dyn,336,336
.rodata,232,232
.eh_frame_hdr,176,176
.data.rel.ro,128,128
.data,32,32
[Unmapped],0,-1438


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow


@andy31415
Copy link
Contributor

LGTM seems to also complain about non virtual destructors.

@pan-apple
Copy link
Contributor Author

LGTM seems to also complain about non virtual destructors.

That should be fixed in the current set of commits. It complained about it earlier yesterday.

@Damian-Nordic
Copy link
Contributor

@Damian-Nordic we can add provisions to have a test secret in the devices that do not yet have Rendezvous integrated. Here's how I am doing it in a test in the current PR (https://github.com/project-chip/connectedhomeip/pull/2230/files?file-filters%5B%5D=#diff-6827ec482c0a42a089c6e57eb3e087acR109). I prefer this over ManualKeyExchange, as the latter is not an actual API that we want to support in CHIP. The longer we keep ManualKeyExchange in the tree, more stuff will get built on top of it.

Do we have any test controller applications that are checked in CHIP repo that can interact with test device examples besides echo-server? I can help migrate the test and example application over to deriving security keys from test secret.

@pan-apple we were using specifically on/off commands of chip-tool to control the door-lock example for nrfconnect platform and I realized that after merging this PR ChipDeviceController::ConnectDevice method used by commands other than echo-ble will always fail due to uninitialized secure session. I actually tested that.

I'm fine with the shared secret approach and I can implement it in our example, but I wonder if we can have the possibility to bypass rendezvous in chip-tool or we should write another test controller.

@pan-apple
Copy link
Contributor Author

@Damian-Nordic we can add provisions to have a test secret in the devices that do not yet have Rendezvous integrated. Here's how I am doing it in a test in the current PR (https://github.com/project-chip/connectedhomeip/pull/2230/files?file-filters%5B%5D=#diff-6827ec482c0a42a089c6e57eb3e087acR109). I prefer this over ManualKeyExchange, as the latter is not an actual API that we want to support in CHIP. The longer we keep ManualKeyExchange in the tree, more stuff will get built on top of it.
Do we have any test controller applications that are checked in CHIP repo that can interact with test device examples besides echo-server? I can help migrate the test and example application over to deriving security keys from test secret.

@pan-apple we were using specifically on/off commands of chip-tool to control the door-lock example for nrfconnect platform and I realized that after merging this PR ChipDeviceController::ConnectDevice method used by commands other than echo-ble will always fail due to uninitialized secure session. I actually tested that.

I'm fine with the shared secret approach and I can implement it in our example, but I wonder if we can have the possibility to bypass rendezvous in chip-tool or we should write another test controller.

@Damian-Nordic , yes, as an immediate fix, the rendezvous would be bypassed in the chip-tool. We can add a temporary API to CHIPDeviceController that can initialize the security session from a test secret. Once we have support for Rendezvous in chip-tool, this API can be removed.

@pan-apple
Copy link
Contributor Author

@saurabhst, @jelderton, @BroderickCarlin, do you have any review feedback?

@andy31415
Copy link
Contributor

Not sure if I should press the big green button here or not. I believe 32f9903 uses the deleted function, but we do not have a CI test for it.

Are we ok to have a temporary breakage?

@pan-apple
Copy link
Contributor Author

Not sure if I should press the big green button here or not. I believe 32f9903 uses the deleted function, but we do not have a CI test for it.

Are we ok to have a temporary breakage?

I am adding support for using test secret for key derivation here (pan-apple#1). I can amend that PR to take care of the EFR32 scenario as well. The other PR is in my fork as of right now. I'll submit it for review here once this merges.

@andy31415 andy31415 merged commit ade4ad6 into project-chip:master Aug 20, 2020
@pan-apple pan-apple deleted the rendezvous branch August 20, 2020 21:35
mspang added a commit to mspang/connectedhomeip that referenced this pull request Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants