[WIP] Initial attempt at rfc8106 #490
Conversation
|
Dear @jpds, I opened a PR with your commit to be able to discuss and review it. :) |
| [%%cstruct | ||
| type opt_rdnss = { | ||
| ty: uint8_t; | ||
| len: uint8_t; |
There was a problem hiding this comment.
hereafter we need a reserved : uint16_t (since there's some padding, and the lifetime starts at offset 4)
src/ipv6/ipv6_wire.ml
Outdated
| ty: uint8_t; | ||
| len: uint8_t; | ||
| rdnss_lifetime: uint32_t; | ||
| rdnss_addresses: uint8_t [@len 16]; |
There was a problem hiding this comment.
keep in mind that the addresses are any number of addresses (actually "len - 1" ones) -- but I can't see a way in ppx_cstruct to define "now there's a list of 16 byte values, in the number of len - 1". so it is fine to leave this as is (maybe add a comment), but the actualy decoding and encoding part needs to be aware of that.
There was a problem hiding this comment.
so maybe just skip the rdnss_addresses entirely here (and have the type opt_rdnss_header)?
src/ipv6/ndpv6.ml
Outdated
| @@ -270,6 +274,10 @@ type na = | |||
| na_target : Ipaddr.t; | |||
| na_tlla : Macaddr.t option } | |||
|
|
|||
| type rdnss = | |||
| { rdnss_lifetime: time option; | |||
| rdnss_addresses: Ipaddr.t } | |||
There was a problem hiding this comment.
| rdnss_addresses: Ipaddr.t } | |
| rdnss_addresses: Ipaddr.V6.t list } |
There was a problem hiding this comment.
Ipaddr is already aliased(?) to Ipaddr.V6 in this file.
src/ipv6/ndpv6.ml
Outdated
| | 0l -> None | ||
| | n -> Some (Int64.of_int32 n) | ||
| in | ||
| let rdnss_addresses = |
There was a problem hiding this comment.
here the decoding happens, and len - 1 needs to be looked at to figure out how many ip addresses to parse. all of them are IPv6 addresses, that's why I'd use Ipaddr.V6.t.
There was a problem hiding this comment.
If we remove from the type opt_rdnss the rdnss_addresses, we could:
let decode_ns off = Ipaddr_cstruct.V6.of_cstruct_exn (Cstruct.shift opt off) in
let rec collect_ns acc = function
| 0 -> acc
| n ->
let ns = decode_ns (Ipv6_wire.sizeof_opt_rdnss + n * 16) in
collect_ns (ns :: acc) (n - 1)
in
let rdnss_addresses = collect_ns [] (Ipv6_wire.get_opt_rdnss_len opt - 1) in
| let to_list rdnssl = | ||
| List.map fst rdnssl | ||
|
|
||
| let add rdnssl ~now ?(lifetime = Duration.of_year 1) ip = |
There was a problem hiding this comment.
From RFC 8106:
The value of Lifetime SHOULD by default be at least 3 * MaxRtrAdvInterval
Can we use that as default, and not 1 year :)
There was a problem hiding this comment.
I'm not sure how to access MaxRtrAdvInterval here as it's something that's set by the router.
There was a problem hiding this comment.
RFC 4861 says about MaxRtrAdvInterval:
MUST be no less than 4 seconds and no greater than 1800 seconds.
So perhaps 1½ hours is reasonable.
Reading the code and some parts of RFC 8106 it seems to me that the lifetime is only None when it's zero in the packet, and in the rfc it says
A value of zero means that the RDNSS addresses MUST no longer be used
So maybe we should either promote lifetime to a regular labeled argument and optionally assert lifetime is greater than zero, or we should not have a default value and do nothing if lifetime is None.
I wonder if something similar applies to RouterList.add where there's a FIXME comment about the default lifetime...
There was a problem hiding this comment.
... or perhaps instead of doing nothing we should actively remove that IP address from the list.
jpds
force-pushed
the
rfc8106
branch
5 times, most recently
from
86b94b3
to
384727c
Compare
Signed-off-by: Jonathan Davies <jpds@protonmail.com>
Signed-off-by: Jonathan Davies jpds@protonmail.com