Remove include dependency on underlying crypto libraries #2310

pan-apple · 2020-08-24T21:33:09Z

Problem

CHIPCryptoPal.h includes headers from underlying implementations (OpenSSL, mbedTLS). This requires the code at upper layers to be aware of which crypto implementation is being used, and requires it to specify include paths (e.g. iOS framework project file has to point to mbedTLS header file location).

Summary of Changes

Move implementation specific code to individual implementation source code. The interface has opaque types for the context, and the size of context is checked via static_assert in each implementation file.

fixes #2284

src/crypto/CHIPCryptoPALOpenSSL.cpp

src/crypto/CHIPCryptoPALmbedTLS.cpp

gerickson

Generally, I'd recommend:

Using C++ rather than C-style casts.
Seeing if static_asserts can be reduced by including them only in an initialization context. In addition, they are compile- rather than run-time, so I don't think they provide much value scattered about as they are.
Would recommend using the more portable static assertions from nlassert.h if there is nothing else about this module that has explicit C++11 dependencies.

pan-apple · 2020-08-24T22:15:34Z

Generally, I'd recommend:

Using C++ rather than C-style casts.

Seeing if static_asserts can be reduced by including them only in an initialization context. In addition, they are compile- rather than run-time, so I don't think they provide much value scattered about as they are.

Would recommend using the more portable static assertions from nlassert.h if there is nothing else about this module that has explicit C++11 dependencies.

Thanks @gerickson , all these should be addressed now.

src/crypto/CHIPCryptoPALOpenSSL.cpp

src/crypto/CHIPCryptoPALmbedTLS.cpp

rwalker-apple · 2020-08-24T22:57:22Z

src/darwin/Framework/CHIP.xcodeproj/project.pbxproj

@@ -454,7 +454,6 @@
 					"CHIP_DEVICE_LAYER_TARGET_LINUX=0",
 					"CHIP_DEVICE_LAYER_TARGET_NRF5=0",
 					"CHIP_DEVICE_LAYER_TARGET_EFR32=0",
-					CHIP_CRYPTO_MBEDTLS,


sooo pretty!

rwalker-apple

lgt 6DAF m, little nits

github-actions · 2020-08-24T23:58:21Z

Size increase report for "nrfconnect-example-build"

File	Section	File	VM
chip-nrf52840-lock-example.elf	bss	0	744
chip-nrf52840-lock-example.elf	text	134	134
chip-nrf52840-lock-example.elf	[LOAD #1 [RWX]]	-6	-6
chip-nrf52840-lock-example.elf	[LOAD #3 [RW]]	0	-8

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_info,0,804
bss,744,0
.debug_abbrev,0,169
.debug_str,0,169
.strtab,0,151
text,134,134
.debug_line,0,88
.debug_frame,0,80
.symtab,0,64
.debug_aranges,0,16
.debug_ranges,0,16
[Unmapped],0,-5
[LOAD #1 [RWX]],-6,-6
[LOAD #3 [RW]],-8,0

github-actions · 2020-08-24T23:58:28Z

Size increase report for "esp32-example-build"

File	Section	File	VM
chip-wifi-echo.elf	.dram0.bss	0	1472
chip-wifi-echo.elf	.flash.text	188	188

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:

sections,vmsize,filesize
.dram0.bss,1472,0
.debug_info,0,961
.debug_abbrev,0,246
.flash.text,188,188
.debug_str,0,173
.debug_line,0,170
.strtab,0,151
.debug_frame,0,48
.symtab,0,32
.debug_aranges,0,16
.debug_ranges,0,16
[Unmapped],0,-1

github-actions · 2020-08-24T23:58:40Z

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 · 2020-08-24T23:58:45Z

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

rwalker-apple

nice

rwalker-apple · 2020-08-26T06:04:00Z

@woody-apple needs a button pusher

pan-apple · 2020-08-26T17:45:20Z

@saurabhst, @jelderton, @BroderickCarlin, any comments?

…p#2310) * Remove include dependency on underlying crypto libraries * Restyled by clang-format * Use nlassert, cpp style cast * Restyled by clang-format * use inline methods to cleanup the code * Restyled by clang-format Co-authored-by: Restyled.io <commits@restyled.io>

* WIP: Demonstration of action()-based binary flashing. Strawman for one method of building flashable binaries and a consistent method of flashing them, for parity with some existing `make flash` commands and ultimately for automated testing. This is temporarily wholly inside examples/lock-app/nrf5 in order to experiment with minimal disruption but is intended to become generic. issue #1520 GN build parity - flashing * Remove include dependency on underlying crypto libraries (#2310) * Remove include dependency on underlying crypto libraries * Restyled by clang-format * Use nlassert, cpp style cast * Restyled by clang-format * use inline methods to cleanup the code * Restyled by clang-format Co-authored-by: Restyled.io <commits@restyled.io> * [shell] add missing inttypes.h header (#2357) * Address feedback and generalize. * WIP: Demonstration of action()-based binary flashing. Strawman for one method of building flashable binaries and a consistent method of flashing them, for parity with some existing `make flash` commands and ultimately for automated testing. This is temporarily wholly inside examples/lock-app/nrf5 in order to experiment with minimal disruption but is intended to become generic. issue #1520 GN build parity - flashing * Address feedback and generalize. * Add missing (ignored) symbolic link for `build`. * Remove unused import from gn_run_binary.py * Fix paths in flashing rules * Remove the build symlinks On second thought, we need to wait for automake removal for this. * Revised flashing script generation and execution. * Streamlined scripts and BUILD.gn files. * Script/generator cleanup and description. * Restyled by gn Co-authored-by: Pankaj Garg <pgarg2@apple.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Jiacheng Guo <gjc@google.com> Co-authored-by: Michael Spang <spang@google.com>

Remove include dependency on underlying crypto libraries

57227a6

pan-apple requested review from andy31415, anush-apple, BroderickCarlin, chrisdecenzo, hawk248, jelderton, robszewczyk, saurabhst and woody-apple as code owners August 24, 2020 21:33

boring-cyborg bot added crypto darwin labels Aug 24, 2020

restyled-io bot mentioned this pull request Aug 24, 2020

Restyle Remove include dependency on underlying crypto libraries #2311

Closed

Restyled by clang-format

6db9c7e

pan-apple mentioned this pull request Aug 24, 2020

Fix chip-tool build on Mac not finding openssl headers #2303

Merged

pan-apple requested a review from rwalker-apple August 24, 2020 21:36