| Message ID | 20220719080715.1445-1-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi, On Tue, Jul 19, 2022 at 10:07:15AM +0200, Antonio Quartulli wrote: > A server may push options that are not compatible with DCO. > In this case we should log a message and bail out. > > Signed-off-by: Antonio Quartulli <a@unstable.cc> This patch is fine as it is, but during discussion we found a missing case - a server pushing "compress <something>". There's basically (at least) 3 ways to handle this - so bringing this up for discussion, and a future patch - add "compress" to check_dco_pull_options() - extend dco_check_option_conflict() to set "allow-compression no" (this will make openvpn refuse all incoming "compress" configs on its own) - or extend dco_check_option_conflict() to make "allow-compression asym/yes" disable DCO I tend to option 3 - default for allow-compression is "no" anyway with 2.6, so if you have this in your config, compression could show up in a ccd/ file or pushed -> no DCO for you. gert
Hi, On Tue, Jul 19, 2022 at 10:07:15AM +0200, Antonio Quartulli wrote: > A server may push options that are not compatible with DCO. > In this case we should log a message and bail out. > > Signed-off-by: Antonio Quartulli <a@unstable.cc> I tried to test this (by forcing the error with --pull-filter ignore peer-id) and do not like the result - it does the pull, it does the OPTIONS IMPORT 2022-08-03 20:06:50 Pushed option removed by filter: 'peer-id 1' 2022-08-03 20:06:50 OPTIONS IMPORT: timers and/or timeouts modified 2022-08-03 20:06:50 OPTIONS IMPORT: compression parms modified 2022-08-03 20:06:50 OPTIONS IMPORT: --ifconfig/up options modified 2022-08-03 20:06:50 OPTIONS IMPORT: route options modified 2022-08-03 20:06:50 OPTIONS IMPORT: data channel crypto options modified ... then it does all the routing and ifconfig and stuff... 2022-08-03 20:06:50 net_route_v4_best_gw query: dst 0.0.0.0 2022-08-03 20:06:50 net_route_v4_best_gw result: via 195.30.8.94 dev ens160 2022-08-03 20:06:50 ROUTE_GATEWAY 195.30.8.94/255.255.255.224 IFACE=ens160 HWADDR=00:50:56:9d:04:1e 2022-08-03 20:06:50 GDG6: remote_host_ipv6=n/a 2022-08-03 20:06:50 net_route_v6_best_gw query: dst :: 2022-08-03 20:06:50 net_route_v6_best_gw result: via 2001:608:1:995a::ffff dev ens160 2022-08-03 20:06:50 ROUTE6_GATEWAY 2001:608:1:995a::ffff IFACE=ens160 2022-08-03 20:06:50 net_iface_new: add tun0 type ovpn-dco 2022-08-03 20:06:50 sitnl_send: rtnl: generic error (-17): File exists 2022-08-03 20:06:50 net_iface_new: add tun1 type ovpn-dco ... 2022-08-03 20:06:50 net_iface_up: set tun7 up 2022-08-03 20:06:50 net_addr_ptp_v4_add: 10.194.2.186 peer 10.194.2.185 dev tun7 2022-08-03 20:06:50 net_iface_mtu_set: mtu 1500 for tun7 2022-08-03 20:06:50 net_iface_up: set tun7 up 2022-08-03 20:06:50 net_addr_v6_add: fd00:abcd:194:2::102d/64 dev tun7 2022-08-03 20:06:50 net_route_v4_add: 10.194.0.0/16 via 10.194.2.185 dev [NULL] table 0 metric 200 2022-08-03 20:06:50 net_route_v4_add: 10.194.2.1/32 via 10.194.2.185 dev [NULL] table 0 metric 200 ... and *then* it hits the new check 2022-08-03 20:06:52 OPTIONS IMPORT: Server did not request DATA_V2 packet format required for data channel offload 2022-08-03 20:06:52 OPTIONS ERROR: pushed options are incompatible with data channel offload. Use --disable-dco to connectto this server 2022-08-03 20:06:52 Failed to open tun/tap interface ... leading to a bogus error message (the tun/tap interface opened just fine). So I'd say the check is working fine, but it is happening at the wrong place - if I move it to the other OPTIONS IMPORT stuff at the end of "part1" (do_deferred_options()) I get a much nicer report... 2022-08-03 20:13:29 Pushed option removed by filter: 'peer-id 1' 2022-08-03 20:13:29 OPTIONS IMPORT: timers and/or timeouts modified 2022-08-03 20:13:29 OPTIONS IMPORT: compression parms modified 2022-08-03 20:13:29 OPTIONS IMPORT: --ifconfig/up options modified 2022-08-03 20:13:29 OPTIONS IMPORT: route options modified 2022-08-03 20:13:29 OPTIONS IMPORT: data channel crypto options modified 2022-08-03 20:13:29 OPTIONS IMPORT: Server did not request DATA_V2 packet format required for data channel offload 2022-08-03 20:13:29 OPTIONS ERROR: pushed options are incompatible with data channel offload. Use --disable-dco to connectto this server 2022-08-03 20:13:46 ERROR: Failed to apply push options (it still says "failed to open tun/tap interface", but at least it says "ERROR: failed to apply push options" before) So - can I have a v4 please? PS: that missing blank "to connectto this server" is part of the v3 patch and could be fixed in v4 as well :-)
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index b39759e1..fa4a8487 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -339,4 +339,16 @@ dco_check_option_conflict(int msglevel, const struct options *o) return true; } +bool +dco_check_pull_options(int msglevel, const struct options *o) +{ + if (!o->use_peer_id) + { + msg(msglevel, "OPTIONS IMPORT: Server did not request DATA_V2 packet " + "format required for data channel offload"); + return false; + } + return true; +} + #endif /* defined(ENABLE_DCO) */ diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index cb7f7e4f..a4bf6e37 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -65,6 +65,17 @@ bool dco_available(int msglevel); */ bool dco_check_option_conflict(int msglevel, const struct options *o); +/** + * Check whether any of the options pushed by the server is not supported by + * our current dco implementation. If so print a warning at warning level + * for the first conflicting option found and return false. + * + * @param msglevel the msg level to use to print the warnings + * @param o the options struct that hold the options + * @return true if no conflict was detected, false otherwise + */ +bool dco_check_pull_options(int msglevel, const struct options *o); + /** * Initialize the DCO context * @@ -154,6 +165,12 @@ dco_check_option_conflict(int msglevel, const struct options *o) return false; } +static inline bool +dco_check_pull_options(int msglevel, const struct options *o) +{ + return false; +} + static inline bool ovpn_dco_init(int mode, dco_context_t *dco) { diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 8e1221dd..81fd49fa 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2366,6 +2366,17 @@ finish_options(struct context *c) return false; } + /* Check if the pushed options are compatible with DCO if we have + * DCO enabled */ + if (dco_enabled(&c->options) + && !dco_check_pull_options(D_TLS_ERRORS, &c->options)) + { + msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are incompatible with " + "data channel offload. Use --disable-dco to connect" + "to this server"); + return false; + } + return true; }
A server may push options that are not compatible with DCO. In this case we should log a message and bail out. Signed-off-by: Antonio Quartulli <a@unstable.cc> --- Changes from v2: * split if condition on two lines Changes from v1: * move check_dco_pull_options() to dco.c (renamed to dco_check_pull_options()) * make options argument const * add msglevel as first argument src/openvpn/dco.c | 12 ++++++++++++ src/openvpn/dco.h | 17 +++++++++++++++++ src/openvpn/init.c | 11 +++++++++++ 3 files changed, 40 insertions(+)