-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gnrc_ipv6_nib/SLAAC: rfc7217 stable privacy addresses #20370
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the time to do a full review at the moment, but from a quick high-level reading this looks like it's well done, and checks a lot of boxes (including careful treatment of Kconfig handling).
A few notes around here and there are really just "things that struck me" with no particular severity. Given there are a lot of fixup commits from the onset, please consider squashing (possibly after handling any of my comments), as that'll make the sequence of commits easier to understand.
# Set another macro that is needed for this option. | ||
|
||
##prepare value | ||
stable_privacy_secret_key != python3 -c "import secrets; print(','.join([f'0x{byte:02x}' for byte in secrets.token_bytes(16)]))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to think a bit about device individualization in RIOT as a whole (esp. for questions like "is this maybe better created in a HWRNG assisted way at first start?"), but until then, passing it into the build seems fine.
As someone who repeatedly flashes devices and then starts network interactions, it may be convenient to store this in the build directory and recreate only if absent, as that will make addresses stable across rebuilds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree, letting the board generate the secret_key by itself (and permanently store it - as per the RFC) would be more convenient, as a future improvement!
Requires Python >=3.6, which is already EOL, so not noteworthy RIOT-OS#20370 (comment)
RIOT-OS#20370 (comment) and CODING_CONVENTIONS.md
Squashed |
Requires Python >=3.6, which is already EOL, so not noteworthy RIOT-OS#20370 (comment)
RIOT-OS#20370 (comment) and CODING_CONVENTIONS.md
Squashed a typo |
Need to check for matching address rather than prefix because for a 6LN, the SLAAC prefix is never stored in the prefix list, since everything is considered off-link.
Requires Python >=3.6, which is already EOL, so not noteworthy RIOT-OS#20370 (comment)
Move pre-condition check to the corresponding method requiring this condition
Wrap in `if(is_rfc7217)`, use normal IDGEN if false
Remove fallback to netif.pid because presumably doesn't fulfill the RFC stability requirements of "constant across system bootstrap sequences and other network events (e.g., bringing another interface up or down)"
RIOT-OS#20370 (comment) and CODING_CONVENTIONS.md
Rebased. |
_auto_configure_addr -> auto_configure_addr _auto_configure_addr_default -> _auto_configure_addr
Not only for the caller `_auto_configure_addr`. Other caller is NETOPT_IPV6_IID. Move check to callee. Probably low severity because address conflicts are unlikely anyway.
`-Werror=maybe-uninitialized`
Example for a caller assuming idempotency: uhcp (-> `gnrc_netif_ipv6_add_prefix` -> `NETOPT_IPV6_IID`)
Contribution description
Implementation of RFC7217 ("stable privacy") means that for addresses generated by SLAAC, their IID (interface identifier) is
RIOT-specific: The implementation in this PR compiles the secret_key into the elffile, therefore the same elffile shouldn't be flashed onto multiple devices - in order to fulfill the RFC requirements.
Notes about implementation:
RFC compatibility: Requirement levels: (implemented: MUST, SHOULD. not implemented: MAY, OPTIONAL.)
--
Adaptability
Supports any link layer that has a hardware address:
https://github.com/xnumad/RIOT/blob/88363f3cde7175691fe27399d73a54e08934e6c7/sys/net/gnrc/network_layer/ipv6/nib/_nib-slaac.c#L168-L169
The usage of IIDs which do not match the link layer address causes LOWPAN_IPHC to not be able to statelessly compress the IP address anymore. An optimization to enable compression again could be to add compression contexts (feature branch for opportunistic compression contexts: https://github.com/xnumad/RIOT/tree/feature%2Fopportunistic-compression-contexts).
6LoWPAN specifics
They do not affect this PRs current functionality but are worth noting.
For link-local addresses on a 6LN iface, this PR does not use the IDGEN (interface identifier generation) mechanism described by rfc7217.
Testing procedure
Add
CFLAGS += -DCONFIG_GNRC_IPV6_STABLE_PRIVACY=1
at the appropriate position in the Makefile ofexamples/gnrc_networking
. Tested on BOARD=nrf52840dkOutput of
ifconfig
command shows that SLAAC addresses have random differing IIDs (except for link-local address on a 6LoWPAN iface, see above). The IID is stable within a subnet, i.e. it also persists across reboots.Issues/PRs references
Commits marked with 🍒 (cherry-picked) are largely derived from the RFC8981 implementation at #20369