gnrc_ipv6_nib/SLAAC: rfc7217 stable privacy addresses #20370
Conversation
There was a problem hiding this comment.
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.
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.
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 |
Requires Python >=3.6, which is already EOL, so not noteworthy RIOT-OS#20370 (comment)
RIOT-OS#20370 (comment) and CODING_CONVENTIONS.md
|
Rebased. |
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
_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`)
|
Rebased to resolve merge conflicts |
| * @brief Use stable privacy addresses (rfc7217) | ||
| */ | ||
| #ifndef CONFIG_GNRC_IPV6_STABLE_PRIVACY | ||
| #define CONFIG_GNRC_IPV6_STABLE_PRIVACY 0 |
There was a problem hiding this comment.
| #define CONFIG_GNRC_IPV6_STABLE_PRIVACY 0 | |
| # define CONFIG_GNRC_IPV6_STABLE_PRIVACY (0) |
As in the other review, the preprocessor commands should have this indentation for new code.
| * @return index of the first matching address | ||
| * in gnrc_netif_t::ipv6_addrs of @p netif | ||
| * @return -1, if no matching address found for @p netif |
There was a problem hiding this comment.
| * @return index of the first matching address | |
| * in gnrc_netif_t::ipv6_addrs of @p netif | |
| * @return -1, if no matching address found for @p netif | |
| * @retval index of the first matching address | |
| * in gnrc_netif_t::ipv6_addrs of @p netif | |
| * @retval -1, if no matching address found for @p netif |
As in the other review, the @retval command is required to specify different return values.
| #define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES (0xC0U) | ||
| /** | ||
| * @brief Shift position of address generation retries | ||
| */ | ||
| #define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES_POS (6) |
There was a problem hiding this comment.
| #define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES (0xC0U) | |
| /** | |
| * @brief Shift position of address generation retries | |
| */ | |
| #define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES_POS (6) | |
| # define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES (0xC0U) | |
| /** | |
| * @brief Shift position of address generation retries | |
| */ | |
| # define GNRC_NETIF_IPV6_ADDRS_FLAGS_IDGEN_RETRIES_POS (6) |
| @@ -49,6 +49,7 @@ | |||
|
|
|||
| #define ENABLE_DEBUG 0 | |||
| #include "debug.h" | |||
| #include "../network_layer/ipv6/nib/_nib-slaac.h" | |||
There was a problem hiding this comment.
As in the other review, I think there is an include path missing in the Makefile.
| #include <hashes/sha256.h> | ||
| #include "ztimer.h" | ||
| #include "random.h" |
There was a problem hiding this comment.
| #include <hashes/sha256.h> | |
| #include "ztimer.h" | |
| #include "random.h" | |
| # include <hashes/sha256.h> | |
| # include "ztimer.h" | |
| # include "random.h" |
| * @brief Upper limit for random time to wait between IDGEN retries | ||
| */ | ||
| #ifndef STABLE_PRIVACY_IDGEN_DELAY_MS | ||
| #define STABLE_PRIVACY_IDGEN_DELAY_MS 1000 /*default value*/ |
There was a problem hiding this comment.
| #define STABLE_PRIVACY_IDGEN_DELAY_MS 1000 /*default value*/ | |
| # define STABLE_PRIVACY_IDGEN_DELAY_MS (1000) /* default value */ |
The (1000) should probably also be indented.
| void auto_configure_addr(gnrc_netif_t *netif, const ipv6_addr_t *pfx, | ||
| uint8_t pfx_len); | ||
|
|
||
| #if IS_ACTIVE(CONFIG_GNRC_IPV6_STABLE_PRIVACY) |
There was a problem hiding this comment.
These should be indented too.
| #if IS_ACTIVE(CONFIG_GNRC_IPV6_STABLE_PRIVACY) | ||
| /** | ||
| * @brief Overload of @ref _auto_configure_addr | ||
| * @param dad_ctr rfc7217 DAD_Counter |
There was a problem hiding this comment.
| * @param dad_ctr rfc7217 DAD_Counter | |
| * @param[in] dad_ctr rfc7217 DAD_Counter |
There was a problem hiding this comment.
Although it would probaly be good to document the other parameters too, as well as in the following function.
| * @return 0 on success | ||
| * @return -1 if failed, because retry limit reached | ||
| * @return `-ENOTSUP`, if interface has no link-layer address. |
There was a problem hiding this comment.
| * @return 0 on success | |
| * @return -1 if failed, because retry limit reached | |
| * @return `-ENOTSUP`, if interface has no link-layer address. | |
| * @retval 0 on success | |
| * @retval -1 if failed, because retry limit reached | |
| * @retval `-ENOTSUP`, if interface has no link-layer address. |
| /** | ||
| * @param[in,out] dad_ctr | ||
| * @param[in] reason | ||
| */ |
There was a problem hiding this comment.
This documentation is still incomplete.
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=1at the appropriate position in the Makefile ofexamples/gnrc_networking. Tested on BOARD=nrf52840dkOutput of
ifconfigcommand 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