| Message ID | 20200911115916.16117-1-gert@greenie.muc.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] If IPv6 pool specification sets pool start to ::0 address, increment. | expand |
Hi, On 11/09/2020 13:59, Gert Doering wrote: > index 1f74ac57..2814ff46 100644 > --- a/src/openvpn/pool.c > +++ b/src/openvpn/pool.c > @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, > } > > pool->ipv6.base = ipv6_base; > + > + /* if a pool starts at ::0, that first IPv6 address is not usable > + * first clients (subnet anycast address). Start with 1, then. > + * NOTE: this will also fire for something like > + * ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 > + * as we only look at the rightmost 32 bits. So be it... > + */ > + if (base == 0) why not memcmp'ing ipv6.base with in6addr_any (defined in netinet/in.h)? This way you get rid of the first NOTE (unless this header is linux specific..). Regards,
Hi, On Mon, Sep 14, 2020 at 10:56:59AM +0200, Antonio Quartulli wrote: > > pool->ipv6.base = ipv6_base; > > + > > + /* if a pool starts at ::0, that first IPv6 address is not usable > > + * first clients (subnet anycast address). Start with 1, then. > > + * NOTE: this will also fire for something like > > + * ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 > > + * as we only look at the rightmost 32 bits. So be it... > > + */ > > + if (base == 0) > > why not memcmp'ing ipv6.base with in6addr_any (defined in netinet/in.h)? > This way you get rid of the first NOTE (unless this header is linux > specific..). because ipv6.base is not "all zero"... it's "the IPv6 address that the pool starts on", host + network part... If you have pools like this: --ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 --ifconfig-ipv6-pool 2001:db8:0:1:1234::0/112 what would "comparing with in6addr_any" achieve? "ipv6.base" is 2001:db8:0:1:1234::0 in both cases. Now, "base" is "an integer that contains the lower 32 bit of the host part" - which is 0, for both these cases. To fix this "totally perfect" we'd need to move "base" to 64 bit integer, and then figure out how to do that portably across supported 32 bit platforms (including MSVC) (or work with 2x 32 bit 'base_low', 'base_hi', or do "full 128 bit on 16 uint8_t" stuff...). Yes, this can be done, but I was way too lazy and went for a less intrusive fix instead... gert
Hi, On 11/09/2020 13:59, Gert Doering wrote: > The first IPv6 address in a subnet is not usable (IPv6 anycast address), > but our pool code ignored this. > > Instead of assigning an unusable address or erroring out, just log the > fact, and increment the pool start to <pool_base>::1 > > NOTE: this is a bit simplistic. A pool that is larger than /96 and > has non-0 bits in the "uppermost bits" will still get the increment > as we only look at the lowermost 32 bits. > > NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this > is a non-issue, as the address for the pool start will be incremented > anyway. > > Reported-by: NicolaF_ in Trac > Trac: #1282 > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > doc/man-sections/server-options.rst | 3 ++- > src/openvpn/pool.c | 15 +++++++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst > index 2009953c..56ffff9a 100644 > --- a/doc/man-sections/server-options.rst > +++ b/doc/man-sections/server-options.rst > @@ -204,7 +204,8 @@ fast hardware. SSL/TLS authentication must be used in this mode. > ifconfig-ipv6-pool ipv6addr/bits > > The pool starts at ``ipv6addr`` and matches the offset determined from > - the start of the IPv4 pool. > + the start of the IPv4 pool. If the host part of the given IPv6 > + address is ``0``, the pool starts at ``ipv6addr`` +1. > > --ifconfig-pool-persist args > Persist/unpersist ifconfig-pool data to ``file``, at ``seconds`` > diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c > index 1f74ac57..2814ff46 100644 > --- a/src/openvpn/pool.c > +++ b/src/openvpn/pool.c > @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, > } > > pool->ipv6.base = ipv6_base; > + > + /* if a pool starts at ::0, that first IPv6 address is not usable can we reword a bit this comment? I.e.: "if the starting address of a pool has the host part all zero, that first ...." The "starts at ::0" confused me as if we were targeting pools starting at [::]. > + * first clients (subnet anycast address). Start with 1, then. > + * NOTE: this will also fire for something like > + * ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 > + * as we only look at the rightmost 32 bits. So be it... wouldn't this test miserably fail when the host part is smaller than 32? like for a 2001:db8:0:1:1234::0/124? Regards, > + */ > + if (base == 0) > + { > + msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: incrementing pool start " > + "to avoid ::0 assignment"); > + base++; > + pool->ipv6.base.s6_addr[15]++; > + } > + > pool_ipv6_size = ipv6_netbits >= 112 > ? (1 << (128 - ipv6_netbits)) - base > : IFCONFIG_POOL_MAX; >
Hi, On Thu, Sep 17, 2020 at 08:55:07AM +0200, Antonio Quartulli wrote: > > } > > > > pool->ipv6.base = ipv6_base; > > + > > + /* if a pool starts at ::0, that first IPv6 address is not usable > > can we reword a bit this comment? I.e.: "if the starting address of a > pool has the host part all zero, that first ...." > > The "starts at ::0" confused me as if we were targeting pools starting > at [::]. Okay. I did not want to make this comment too long, but it seems more verbosity is needed. I'll send a v2 > > + * first clients (subnet anycast address). Start with 1, then. > > + * NOTE: this will also fire for something like > > + * ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 > > + * as we only look at the rightmost 32 bits. So be it... > > wouldn't this test miserably fail when the host part is smaller than 32? > like for a 2001:db8:0:1:1234::0/124? We look at "base", which is only the host part, but "at most 32 bits of the host part". (This is *your* code...!) gert
Hi, On 17/09/2020 09:01, Gert Doering wrote: > We look at "base", which is only the host part, but "at most 32 bits of > the host part". > > (This is *your* code...!) (self-shaming dance mode=ON) Riiight, then drop this comment. The patch looks good, except for the comment that needs more verbosity. > > gert >
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst index 2009953c..56ffff9a 100644 --- a/doc/man-sections/server-options.rst +++ b/doc/man-sections/server-options.rst @@ -204,7 +204,8 @@ fast hardware. SSL/TLS authentication must be used in this mode. ifconfig-ipv6-pool ipv6addr/bits The pool starts at ``ipv6addr`` and matches the offset determined from - the start of the IPv4 pool. + the start of the IPv4 pool. If the host part of the given IPv6 + address is ``0``, the pool starts at ``ipv6addr`` +1. --ifconfig-pool-persist args Persist/unpersist ifconfig-pool data to ``file``, at ``seconds`` diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c index 1f74ac57..2814ff46 100644 --- a/src/openvpn/pool.c +++ b/src/openvpn/pool.c @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, } pool->ipv6.base = ipv6_base; + + /* if a pool starts at ::0, that first IPv6 address is not usable + * first clients (subnet anycast address). Start with 1, then. + * NOTE: this will also fire for something like + * ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64 + * as we only look at the rightmost 32 bits. So be it... + */ + if (base == 0) + { + msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: incrementing pool start " + "to avoid ::0 assignment"); + base++; + pool->ipv6.base.s6_addr[15]++; + } + pool_ipv6_size = ipv6_netbits >= 112 ? (1 << (128 - ipv6_netbits)) - base : IFCONFIG_POOL_MAX;
The first IPv6 address in a subnet is not usable (IPv6 anycast address), but our pool code ignored this. Instead of assigning an unusable address or erroring out, just log the fact, and increment the pool start to <pool_base>::1 NOTE: this is a bit simplistic. A pool that is larger than /96 and has non-0 bits in the "uppermost bits" will still get the increment as we only look at the lowermost 32 bits. NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this is a non-issue, as the address for the pool start will be incremented anyway. Reported-by: NicolaF_ in Trac Trac: #1282 Signed-off-by: Gert Doering <gert@greenie.muc.de> --- doc/man-sections/server-options.rst | 3 ++- src/openvpn/pool.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-)