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

cpu/esp_common: change esp_now key param #17419

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Dec 18, 2021

Contribution description

This PR changes the way the 128-bit key for encrypted ESP-NOW communication is defined.

The 128-bit key for encrypted ESP-NOW communication is defined by a 16-byte array of type uint8_t [16] specified by the ESP_NOW_KEY configuration parameter in RIOT. By default, this configuration parameter is specified as NULL (no encryption). While it easy to define the key as static initializer in C

#define ESP_NOW_KEY { 0x0f, 0x1e, ... }

it is not possible to define such static initializer in Kconfig. Therefore, in regard to the migration to Kconfig, the key is now defined as a string with hex values separated by spaces

#define ESP_NOW_KEY "0f 1e ..."

which is easy to implement in Kconfig.

The documentation is updated accordingly as well as fixed and improved a bit.

The PR includes the fix in PR #17420 (commit 87a72e4) which is skipped once PR #17420 is merged and this PR is rebased.

Testing procedure

  1. Set ENABLE_DEBUG in cpu/esp_common/esp_now_netdev.c.
  2. Compile and flash example/gnrc_networking for two nodes as follows:
    CFLAGS='-DESP_NOW_KEY=\"0f\ 1e\ 2d\ 3c\ 4b\ 5a\ 69\ 78\ 87\ 96\ a5\ b4\ c3\ d2\ e1\ f0\"' \
    make BOARD=esp32-wroom-32 -C examples/gnrc_networking PORT=/dev/ttyUSB<0|1> flash term
    
  3. Once the nodes have found each other and are connected, the output should look something like this.
    _esp_system_event_handler WiFi scan done
    wifi_scan_get_ap_num ret=0 num=2
    wifi_scan_get_aps ret=0 num=2
    esp_now_key: 0f 1e 2d 3c 4b 5a 69 78 87 96 a5 b4 c3 d2 e1 f0
    esp_now_add_peer node 30:ae:a4:41:60:f9 added with return value 0
    associated peers total=1, encrypted=1
    
  4. Ping the node from one node (optional), e.g.
    ping6 fe80::32ae:a4ff:fe41:60f9
    
    In this example the LLUA is used as generated from the MAC address that was given in the output in step 3:
    esp_now_add_peer node 30:ae:a4:41:60:f9 added with return value 0
    

Issues/PRs references

Depends on PR #17420

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Dec 18, 2021
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 18, 2021
@gschorcht gschorcht force-pushed the cpu/esp_common/change_esp_now_key_param branch 3 times, most recently from 7eebff0 to b163768 Compare December 19, 2021 08:28
The 128-bit key for encrypted ESP-NOW communication is defined by a 16-byte array of type `uint8_t [16]` specified by the `ESP_NOW_KEY' configuration parameter in RIOT. By default, this configuration parameter is specified as NULL (no encryption). While in C it is possible to simply define this configuration parameter by something like `#define ESP_NOW_KEY { 0x0f, 0x1e, ... }`, it is not possible in Kconfig to define such a static initializer. Therefore, in regard to the migration to Kconfig, the key is now defined as a string with hex values separated by spaces `#define ESP_NOW_KEY "0f 1e ..."`, which is easy to implement in Kconfig.
@gschorcht gschorcht force-pushed the cpu/esp_common/change_esp_now_key_param branch from b163768 to 8ed75af Compare December 21, 2021 13:29
@benpicco
Copy link
Contributor

benpicco commented Dec 21, 2021

I guess we could also handle this like IEEE802154_SEC_DEFAULT_KEY and just use a string in KConfig, but allow for a byte array to be supplied via the Makefile.

(I also assume this has the same issues wrt. key exchange as IEEE 802.15.4 security - #16844 They could probably use a common upper layer solution)

@gschorcht
Copy link
Contributor Author

I guess we could also handle this like IEEE802154_SEC_DEFAULT_KEY and just use a string in KConfig, but allow for a byte array to be supplied via the Makefile.

But defining it as a string reduces the possible entropy a lot. The solution in this PR would allow to use the value range. OK, it makes the definition a bit more difficult, but not too much.

@benpicco
Copy link
Contributor

Ok, but why the spaces? A hex byte is unambiguous with two characters, no need to separate them with spaces.
We could then also use that hex string to byte array method for the 802.15.4 security module

@benpicco benpicco requested a review from chrysn December 22, 2021 15:09
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jul 10, 2022
@stale stale bot closed this Aug 13, 2022
@gschorcht
Copy link
Contributor Author

I will continue to work on this in next few week.

@gschorcht gschorcht reopened this Aug 14, 2022
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants