Re: [Openvpn-devel] [PATCH v3] Route: add support for user defined routing table
Hi,
-Original Message-
From: Frank Lichtenheld
Sent: giovedì 13 aprile 2023 16:19
To: Gert Doering
Cc: Gianmarco De Gregori ;
openvpn-devel@lists.sourceforge.net
Subject: Re: [Openvpn-devel] [PATCH v3] Route: add support for user defined
routing table
On Thu, Apr 13, 2023 at 04:12:32PM +0200, Gert Doering wrote:
> Hi,
>
> On Tue, Apr 04, 2023 at 10:32:26AM +0200, Gianmarco De Gregori wrote:
> > diff --git a/src/openvpn/route.c b/src/openvpn/route.c index
> > 3798bc65..00419dce 100644
> > --- a/src/openvpn/route.c
> > +++ b/src/openvpn/route.c
> > @@ -325,7 +325,6 @@ init_route(struct route_ipv4 *r,
> >
> > CLEAR(*r);
> > r->option = ro;
> > -
> > /* network */
> >
> > if (!is_route_parm_defined(ro->network))
> > @@ -437,6 +436,27 @@ init_route(struct route_ipv4 *r,
> >
> > r->flags |= RT_DEFINED;
> >
> > +/* routing table id */
> > +
> > +r->table_id = 0;
> > +if (ro->table_id)
> > +{
> > +r->table_id = atoi(ro->table_id);
> > +if (r->table_id < 0)
> > +{
> > +msg(M_WARN, PACKAGE_NAME "ROUTE: routing table id for
> > + network %s (%s) must be >= 0",
>
> Frank's comments alerted me to this, and this certainly is not the way
> to approach it. Syntax checking of the routing table ID must happen
> during option parsing (options.c), not in init_route() - so, this
> function should be able to rely on ro->table_id being an *int*, and
> properly sanitized - "if set, the content is valid".
>
> Same for IPv6, of course.
To be fair, he implemented it in the same way all the other parameters are
implemented. That is why I did not complain about that (e.g. compare
ro->metric, which is treated exactly the same way).
However, I agree with your general sentiment.
I completely agree with switching from uint32_t to *int* as I notice that
those muliple checks doesn't make any sense.
As Frank said, I chose this approach of following how
options->route_default_metric and ro->metric are implemented to be
consistent with the overall data flow, anyway, in a follow-up patch I could
perform a clean-up of all those parameters to be sanitized and managed
through options.c .
Regards,
Gianmarco De Gregori
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Route: add support for user defined routing table
On Thu, Apr 13, 2023 at 04:12:32PM +0200, Gert Doering wrote:
> Hi,
>
> On Tue, Apr 04, 2023 at 10:32:26AM +0200, Gianmarco De Gregori wrote:
> > diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> > index 3798bc65..00419dce 100644
> > --- a/src/openvpn/route.c
> > +++ b/src/openvpn/route.c
> > @@ -325,7 +325,6 @@ init_route(struct route_ipv4 *r,
> >
> > CLEAR(*r);
> > r->option = ro;
> > -
> > /* network */
> >
> > if (!is_route_parm_defined(ro->network))
> > @@ -437,6 +436,27 @@ init_route(struct route_ipv4 *r,
> >
> > r->flags |= RT_DEFINED;
> >
> > +/* routing table id */
> > +
> > +r->table_id = 0;
> > +if (ro->table_id)
> > +{
> > +r->table_id = atoi(ro->table_id);
> > +if (r->table_id < 0)
> > +{
> > +msg(M_WARN, PACKAGE_NAME "ROUTE: routing table id for network
> > %s (%s) must be >= 0",
>
> Frank's comments alerted me to this, and this certainly is not the way
> to approach it. Syntax checking of the routing table ID must happen during
> option parsing (options.c), not in init_route() - so, this function
> should be able to rely on ro->table_id being an *int*, and properly
> sanitized - "if set, the content is valid".
>
> Same for IPv6, of course.
To be fair, he implemented it in the same way all the other parameters are
implemented. That is why
I did not complain about that (e.g. compare ro->metric, which is treated
exactly the same way).
However, I agree with your general sentiment.
Regards,
--
Frank Lichtenheld
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Route: add support for user defined routing table
Hi,
On Tue, Apr 04, 2023 at 10:32:26AM +0200, Gianmarco De Gregori wrote:
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 3798bc65..00419dce 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -325,7 +325,6 @@ init_route(struct route_ipv4 *r,
>
> CLEAR(*r);
> r->option = ro;
> -
> /* network */
>
> if (!is_route_parm_defined(ro->network))
> @@ -437,6 +436,27 @@ init_route(struct route_ipv4 *r,
>
> r->flags |= RT_DEFINED;
>
> +/* routing table id */
> +
> +r->table_id = 0;
> +if (ro->table_id)
> +{
> +r->table_id = atoi(ro->table_id);
> +if (r->table_id < 0)
> +{
> +msg(M_WARN, PACKAGE_NAME "ROUTE: routing table id for network %s
> (%s) must be >= 0",
Frank's comments alerted me to this, and this certainly is not the way
to approach it. Syntax checking of the routing table ID must happen during
option parsing (options.c), not in init_route() - so, this function
should be able to rely on ro->table_id being an *int*, and properly
sanitized - "if set, the content is valid".
Same for IPv6, of course.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Route: add support for user defined routing table
On Tue, Apr 04, 2023 at 10:32:26AM +0200, Gianmarco De Gregori wrote:
> Add the ability for users to specify a custom
> routing table where routes should be installed in.
> As of now routes are always installed in the main
> routing table of the operating system, however,
> with the new --route-table option it is possibile
> to specify the ID of the default routing table
> to be used by --route(-ipv6).
>
> The --route(-ipv6) directives have been extended
> with an additional argument (5th for --route)
> (4th for --route-ipv6) so that each of them
> can possibly use an independent routing table.
>
> Please note: this feature is currently supported
> only by Linux/SITNL.
> Support for other platforms should be added in related backends.
>
> Signed-off-by: Gianmarco De Gregori
> ---
[...]
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2680f268..3914ab23 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
[...]
> @@ -6998,7 +7020,22 @@ add_option(struct options *options,
> }
> /* p[3] is metric, if present */
> }
> -add_route_ipv6_to_option_list(options->routes_ipv6, p[1], p[2],
> p[3]);
> +
> +/* at the moment the routing table id is supported only by
> Linux/SITNL */
> +#ifndef ENABLE_SITNL
> +if (p[5])
p[4]
> +{
> +static bool route6_table_warned = false;
> +
> +if (!route6_table_warned)
> +{
> +msg(M_WARN, "NOTE: table specified for --route-ipv6, but not
> supported on this platform");
> +route6_table_warned = true;
> +}
> +}
> +#endif
> +
> +add_route_ipv6_to_option_list(options->routes_ipv6, p[1], p[2],
> p[3], p[4]);
> }
> else if (streq(p[0], "max-routes") && !p[2])
> {
[...]
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 3798bc65..00419dce 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
[...]
> @@ -437,6 +436,27 @@ init_route(struct route_ipv4 *r,
>
> r->flags |= RT_DEFINED;
>
> +/* routing table id */
> +
> +r->table_id = 0;
> +if (ro->table_id)
> +{
> +r->table_id = atoi(ro->table_id);
> +if (r->table_id < 0)
Isn't r->table_id an uint32 ?
> +{
> +msg(M_WARN, PACKAGE_NAME "ROUTE: routing table id for network %s
> (%s) must be >= 0",
> +ro->network,
> +ro->table_id);
> +goto fail;
> +}
> +r->flags |= RT_TABLE_DEFINED;
> +}
> +else if (rl->spec.flags & RTSA_DEFAULT_TABLE_ID)
> +{
> +r->table_id = rl->spec.table_id;
> +r->flags |= RT_TABLE_DEFINED;
> +}
> +
> return true;
>
> fail:
> @@ -493,6 +513,27 @@ init_route_ipv6(struct route_ipv6 *r6,
>
> r6->flags |= RT_DEFINED;
>
> +/* routing table id */
> +
> +r6->table_id = 0;
> +if (r6o->table_id)
> +{
> +r6->table_id = atoi(r6o->table_id);
> +if (r6->table_id < 0)
Isn't r6->table_id an uint32 ?
> +{
> +msg(M_WARN, PACKAGE_NAME "ROUTE: routing table id for network %s
> (%s) must be >= 0",
> +r6o->prefix,
> +r6o->table_id);
> +goto fail;
> +}
> +r6->flags |= RT_TABLE_DEFINED;
> +}
> +else if (rl6->spec_flags & RTSA_DEFAULT_TABLE_ID)
> +{
> +r6->table_id = rl6->default_route_table_id;
> +r6->flags |= RT_TABLE_DEFINED;
> +}
> +
> return true;
>
> fail:
[...]
> @@ -1978,10 +2043,16 @@ add_route_ipv6(struct route_ipv6 *r6, const struct
> tuntap *tt,
> metric = r6->metric;
> }
>
> +uint32_t table_id = 0;
> +if ((r6->flags & RT_TABLE_DEFINED) && (r6->table_id > 0))
Isn't r6->table_id an uint32 ?
> +{
> +table_id = r6->table_id;
> +}
> +
> status = RTA_SUCCESS;
> int ret = net_route_v6_add(ctx, >network, r6->netbits,
> gateway_needed ? >gateway : NULL,
> - device, 0, metric);
> + device, table_id, metric);
> if (ret == -EEXIST)
> {
> msg(D_ROUTE, "NOTE: Linux route add command failed because route
> exists");
[...]
Regards,
--
Frank Lichtenheld
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3] Route: add support for user defined routing table
Add the ability for users to specify a custom
routing table where routes should be installed in.
As of now routes are always installed in the main
routing table of the operating system, however,
with the new --route-table option it is possibile
to specify the ID of the default routing table
to be used by --route(-ipv6).
The --route(-ipv6) directives have been extended
with an additional argument (5th for --route)
(4th for --route-ipv6) so that each of them
can possibly use an independent routing table.
Please note: this feature is currently supported
only by Linux/SITNL.
Support for other platforms should be added in related backends.
Signed-off-by: Gianmarco De Gregori
---
Changes from v1:
* Fixed parameters (metric and table_id) order in init_route_list() call in
init.c : 1535.
Changes from v2:
* Add route_default_table_id to show_settings() in options.c : 1800.
doc/man-sections/vpn-network-options.rst | 16 +++-
src/openvpn/helper.c | 1 +
src/openvpn/init.c | 15 +++-
src/openvpn/options.c| 45 +-
src/openvpn/options.h| 1 +
src/openvpn/route.c | 101 +--
src/openvpn/route.h | 17 +++-
7 files changed, 180 insertions(+), 16 deletions(-)
diff --git a/doc/man-sections/vpn-network-options.rst
b/doc/man-sections/vpn-network-options.rst
index 8e3c92ee..c25bbf31 100644
--- a/doc/man-sections/vpn-network-options.rst
+++ b/doc/man-sections/vpn-network-options.rst
@@ -367,6 +367,14 @@ routing.
Like ``--redirect-gateway``, but omit actually changing the default gateway.
Useful when pushing private subnets.
+--route-table id
+ Specify a default table id for use with --route.
+ By default, OpenVPN installs routes in the main routing
+ table of the operating system, but with this option,
+ a user defined routing table can be used instead.
+
+ (Supported on Linux only, on other platforms this is a no-op).
+
--route args
Add route to routing table after connection is established. Multiple
routes can be specified. Routes will be automatically torn down in
@@ -379,6 +387,7 @@ routing.
route network/IP netmask
route network/IP netmask gateway
route network/IP netmask gateway metric
+ route network/IP netmask gateway metric table-id
This option is intended as a convenience proxy for the ``route``\(8)
shell command, while at the same time providing portable semantics
@@ -394,6 +403,9 @@ routing.
``metric``
default taken from ``--route-metric`` if set, otherwise :code:`0`.
+ ``table-id`` (Supported on Linux only, on other platforms this is a no-op).
+ default taken from ``--route-table`` if set, otherwise :code:`0`.
+
The default can be specified by leaving an option blank or setting it to
:code:`default`.
@@ -444,12 +456,14 @@ routing.
Valid syntax:
::
- route-ipv6 ipv6addr/bits [gateway] [metric]
+ route-ipv6 ipv6addr/bits [gateway] [metric] [table-id]
The gateway parameter is only used for IPv6 routes across *tap* devices,
and if missing, the ``ipv6remote`` field from ``--ifconfig-ipv6`` or
``--route-ipv6-gateway`` is used.
+ (table-id supported on Linux only, on other platforms this is a no-op).
+
--route-gateway arg
Specify a default *gateway* for use with ``--route``.
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 7c219fdf..4a0e0d85 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -120,6 +120,7 @@ helper_add_route(const in_addr_t network, const in_addr_t
netmask, struct option
print_in_addr_t(network, 0, >gc),
print_in_addr_t(netmask, 0, >gc),
NULL,
+ NULL,
NULL);
}
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index d358ad00..00caa283 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1504,6 +1504,7 @@ do_init_route_list(const struct options *options,
const char *gw = NULL;
int dev = dev_type_enum(options->dev, options->dev_type);
int metric = 0;
+uint32_t table_id = 0; /* unspec table */
/* if DCO is enabled we have both regular routes and iroutes in the system
* routing table, and normal routes must have a higher metric for that to
@@ -1522,6 +1523,10 @@ do_init_route_list(const struct options *options,
{
gw = options->route_default_gateway;
}
+if (options->route_default_table_id)
+{
+table_id = options->route_default_table_id;
+}
if (options->route_default_metric)
{
metric = options->route_default_metric;
@@ -1531,6 +1536,7 @@ do_init_route_list(const struct options *options,
options->routes,
gw,
metric,
+table_id,
