From: mrbff (C. Review) <ge...@op...> - 2024-11-20 17:43:00
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to review the following change. Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 6 files changed, 355 insertions(+), 62 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/1 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8d9d06..b95a61a 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2567,6 +2567,47 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..3d0206e 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,6 +86,8 @@ bool pulled_options, unsigned int option_types_found); +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index e772a54..5b4419d 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if ((!opt->routes + || opt->route_method != ROUTE_METHOD_SERVICE + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5452,67 +5494,6 @@ } } -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5614,6 +5595,170 @@ { return options->forward_compatible ? M_WARN : msglevel; } + +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + destroy_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + destroy_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5647,11 +5792,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 71a0372..69ea4bf 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No option found"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 2e584c7..0cd71d2 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1291,6 +1291,60 @@ } } +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv4 *r; + for (r = rl->routes; r; r = r->next) + { + delete_route(r, tt, flags, &rl->rgi, es, ctx); + } + rl->iflags &= ~RL_ROUTES_ADDED; + } + + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + + if (rl) + { + clear_route_list(rl); + } + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } +} + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv6 *r6; + for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) + { + delete_route_ipv6(r6, tt, flags, es, ctx); + } + rl6->iflags &= ~RL_ROUTES_ADDED; + } + + if (rl6) + { + clear_route_ipv6_list(rl6); + } + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } +} + #ifndef ENABLE_SMALL static const char * diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..b798c9d 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: mrbff (C. Review) <ge...@op...> - 2024-11-21 18:25:01
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#2). Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 6 files changed, 354 insertions(+), 32 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/2 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8d9d06..b95a61a 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2567,6 +2567,47 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..3d0206e 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,6 +86,8 @@ bool pulled_options, unsigned int option_types_found); +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 8dea578..5b4419d 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if ((!opt->routes + || opt->route_method != ROUTE_METHOD_SERVICE + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5452,37 +5494,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5585,6 +5596,169 @@ return options->forward_compatible ? M_WARN : msglevel; } +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + destroy_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + destroy_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5618,11 +5792,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 71a0372..69ea4bf 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No option found"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 2e584c7..0cd71d2 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1291,6 +1291,60 @@ } } +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv4 *r; + for (r = rl->routes; r; r = r->next) + { + delete_route(r, tt, flags, &rl->rgi, es, ctx); + } + rl->iflags &= ~RL_ROUTES_ADDED; + } + + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + + if (rl) + { + clear_route_list(rl); + } + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } +} + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv6 *r6; + for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) + { + delete_route_ipv6(r6, tt, flags, es, ctx); + } + rl6->iflags &= ~RL_ROUTES_ADDED; + } + + if (rl6) + { + clear_route_ipv6_list(rl6); + } + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } +} + #ifndef ENABLE_SMALL static const char * diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..b798c9d 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 2 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: stipa (C. Review) <ge...@op...> - 2024-12-03 14:42:56
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 7: Code-Review-2 (2 comments) File src/openvpn/init.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/94a55af1_56ebeb0e : PS4, Line 89: bool do_update(struct context *c, unsigned int option_types_found); Doxygen File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/3765b756_2ab72e4d : PS7, Line 3300: && (opt->routes->flags & RG_DEF1)) this gives me "access violation" on Windows, opt->routes is nullptr. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 03 Dec 2024 14:42:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 20:40:18
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#8). Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 6 files changed, 383 insertions(+), 32 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/8 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8d9d06..b95a61a 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2567,6 +2567,47 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..3323497 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,6 +86,18 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 6f97fcd..b4ee450 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5452,37 +5494,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5585,6 +5596,188 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + destroy_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + destroy_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5618,11 +5811,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 2afd539..9d9f055 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 2e584c7..0cd71d2 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1291,6 +1291,60 @@ } } +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv4 *r; + for (r = rl->routes; r; r = r->next) + { + delete_route(r, tt, flags, &rl->rgi, es, ctx); + } + rl->iflags &= ~RL_ROUTES_ADDED; + } + + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + + if (rl) + { + clear_route_list(rl); + } + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } +} + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv6 *r6; + for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) + { + delete_route_ipv6(r6, tt, flags, es, ctx); + } + rl6->iflags &= ~RL_ROUTES_ADDED; + } + + if (rl6) + { + clear_route_ipv6_list(rl6); + } + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } +} + #ifndef ENABLE_SMALL static const char * diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..b798c9d 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 8 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 20:40:53
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 7: (2 comments) File src/openvpn/init.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/c56bc11c_d397fa0c : PS4, Line 89: bool do_update(struct context *c, unsigned int option_types_found); > Doxygen Done File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/f9991389_446e70bf : PS7, Line 3300: && (opt->routes->flags & RG_DEF1)) > this gives me "access violation" on Windows, opt->routes is nullptr. Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Tue, 03 Dec 2024 20:40:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-12-04 10:41:29
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 8: Code-Review-2 (1 comment) Patchset: PS8: Now I got this one: 2024-12-04 11:39:16 us=531000 PUSH: Received control message: 'PUSH_UPDATE,ifconfig 10.8.0.4 255.255.255.0' 2024-12-04 11:39:18 us=328000 OPTIONS IMPORT: --ifconfig/up options modified 2024-12-04 11:39:18 us=328000 OPTIONS ERROR: failed to negotiate cipher with server. Configure --data-ciphers-fallback if you want to connect to this server. 2024-12-04 11:39:18 us=328000 ERROR: Failed to apply push options 2024-12-04 11:39:18 us=328000 Failed to update options 2024-12-04 11:39:18 us=328000 TCP/UDP: Closing socket -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 8 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 04 Dec 2024 10:41:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-12-06 09:08:27
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 8: (1 comment) Patchset: PS8: > Now I got this one: […] Not resolved. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 8 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Fri, 06 Dec 2024 09:08:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-12-10 10:44:59
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#9). Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 392 insertions(+), 40 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/9 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8d9d06..310e117 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2445,7 +2445,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2567,6 +2567,47 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2645,11 +2686,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2745,7 +2783,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..100f112 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 45b3cfa..a9040a6 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2409,7 +2409,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 6f97fcd..b4ee450 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5452,37 +5494,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5585,6 +5596,188 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + destroy_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + destroy_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5618,11 +5811,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 2afd539..9d9f055 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 2e584c7..0cd71d2 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1291,6 +1291,60 @@ } } +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv4 *r; + for (r = rl->routes; r; r = r->next) + { + delete_route(r, tt, flags, &rl->rgi, es, ctx); + } + rl->iflags &= ~RL_ROUTES_ADDED; + } + + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + + if (rl) + { + clear_route_list(rl); + } + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } +} + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv6 *r6; + for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) + { + delete_route_ipv6(r6, tt, flags, es, ctx); + } + rl6->iflags &= ~RL_ROUTES_ADDED; + } + + if (rl6) + { + clear_route_ipv6_list(rl6); + } + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } +} + #ifndef ENABLE_SMALL static const char * diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..b798c9d 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 9 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-12-10 10:46:20
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 9: (1 comment) Patchset: PS8: > Not resolved. Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 9 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Tue, 10 Dec 2024 10:46:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-12-27 13:11:46
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 9: Code-Review-2 (1 comment) Patchset: PS9: I managed to change client IP address and return it back, so ifconfig push works, no crashes etc. However pushing routes didn't work for me - push-update command was processed but routing table wasn't updated: https://gist.github.com/lstipakov/7fd402f43c042e103845912028a0971f -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 9 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Fri, 27 Dec 2024 13:11:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-01-08 11:51:28
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#10). Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 400 insertions(+), 40 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/10 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8d9d06..9738d3b 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2445,7 +2445,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2567,6 +2567,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2645,11 +2694,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2745,7 +2791,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..100f112 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 45b3cfa..a9040a6 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2409,7 +2409,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index e417a99..bd00f65 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5452,37 +5494,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5585,6 +5596,188 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + destroy_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + destroy_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx, options); + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5618,11 +5811,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 2afd539..9d9f055 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index d17b285..5358e3a 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1291,6 +1291,60 @@ } } +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv4 *r; + for (r = rl->routes; r; r = r->next) + { + delete_route(r, tt, flags, &rl->rgi, es, ctx); + } + rl->iflags &= ~RL_ROUTES_ADDED; + } + + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + + if (rl) + { + clear_route_list(rl); + } + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } +} + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) + { + struct route_ipv6 *r6; + for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) + { + delete_route_ipv6(r6, tt, flags, es, ctx); + } + rl6->iflags &= ~RL_ROUTES_ADDED; + } + + if (rl6) + { + clear_route_ipv6_list(rl6); + } + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } +} + #ifndef ENABLE_SMALL static const char * diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..b798c9d 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + +void +destroy_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx, struct options *options); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 10 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: stipa (C. Review) <ge...@op...> - 2025-01-21 12:59:16
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 11: -Code-Review (1 comment) File src/openvpn/route.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/f45f9775_7e0c3f33 : PS11, Line 325: destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, Apart from lacking comments, how does it differ from delete_routes()? Couldn't we reuse existing function (probably with some adjustments) ? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 11 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 21 Jan 2025 12:58:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-01-21 14:14:44
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 11: (1 comment) File src/openvpn/route.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/7dd177d3_7d6de585 : PS11, Line 325: destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, > Apart from lacking comments, how does it differ from delete_routes()? Couldn't we reuse existing fun […] The destroy_routes_vx are used to split the logic of delete_routes in v4 and v6 and also to delete the routes in the "options" struct. In most cases where delete_route functions are used it is correct not to delete the routes in the "options" struct so I simply created the functions I needed. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 11 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Tue, 21 Jan 2025 14:14:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2025-01-21 14:29:39
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 11: (1 comment) File src/openvpn/route.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/ea98c22c_ce50dc61 : PS11, Line 325: destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, > The destroy_routes_vx are used to split the logic of delete_routes in v4 and v6 and also to delete t […] Could we then remove existing delete_routes() which is called from the single place and instead call delete_routes_v4() and delete_routes_v6() (and rename destroy->delete) ? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 11 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 21 Jan 2025 14:29:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: mrbff <ma...@ma...> Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-01-21 15:55:30
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#12). Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 373 insertions(+), 42 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/12 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 4ada221..fdbc817 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2462,7 +2462,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2584,6 +2584,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2662,11 +2711,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2772,7 +2818,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 11c32ac..77490d8 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f426b46..5b693f9 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2411,7 +2411,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index b05135f..7e38d2c 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1055,6 +1055,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3260,8 +3294,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5419,37 +5461,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5552,6 +5563,198 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5585,11 +5788,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index db1393e..29c7f79 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index d17b285..35cf5e9 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1258,7 +1258,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1274,8 +1283,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..0cc9ed6 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -321,6 +321,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 12 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2025-01-21 15:59:02
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 12: (1 comment) File src/openvpn/route.h: http://gerrit.openvpn.net/c/openvpn/+/809/comment/03187090_139abae8 : PS11, Line 325: destroy_routes_v4(struct route_list *rl, const struct tuntap *tt, > Could we then remove existing delete_routes() which is called from the single place and instead call […] ok i extracted the logic from delete_routes() and i separated it into delete_routes_v4() and delete_routes_v6() -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 12 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Tue, 21 Jan 2025 15:58:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Comment-In-Reply-To: mrbff <ma...@ma...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2025-01-22 09:14:28
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 12 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 22 Jan 2025 09:14:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-03-21 04:57:59
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#13). The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now. Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 373 insertions(+), 42 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/13 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index cd94e6e..9a1abd5 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2463,7 +2463,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2585,6 +2585,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2663,11 +2712,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2775,7 +2821,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 50b38e3..31b7b4c 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index a673ec1..ec04369 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2425,7 +2425,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index bf29c56..b35f82b 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1084,6 +1084,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3229,8 +3263,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5416,37 +5458,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5549,6 +5560,198 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5582,11 +5785,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index a40a28e..bad59a4 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index dbdc01d..bcff674 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1257,7 +1257,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1273,8 +1282,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index dda210a..adec67c 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -328,6 +328,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 13 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2025-05-20 12:26:17
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#14). The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now. Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 373 insertions(+), 42 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/14 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9527429..d5bc630 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2470,7 +2470,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2594,6 +2594,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2672,11 +2721,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2784,7 +2830,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 50b38e3..31b7b4c 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 80dd0c0..be121c0 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2425,7 +2425,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index b0dd825..33e1a61 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1082,6 +1082,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3222,8 +3256,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5372,37 +5414,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5505,6 +5516,198 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5538,11 +5741,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index a40a28e..bad59a4 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index dbdc01d..bcff674 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1257,7 +1257,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1273,8 +1282,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index dda210a..adec67c 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -328,6 +328,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 14 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2025-07-07 17:16:14
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#15). The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now. Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 373 insertions(+), 42 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/15 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 78ebb17..9ab5378 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2470,7 +2470,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2594,6 +2594,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2672,11 +2721,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2784,7 +2830,7 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 5c6b9c1..25078a6 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index a760e07..7f0d890 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2423,7 +2423,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 67fa906..6597610 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1085,6 +1085,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3089,8 +3123,16 @@ msg(M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in use)"); opt->routes->flags |= RG_DEF1; } + else if (opt->routes + && ((opt->route_method != ROUTE_METHOD_SERVICE) + || !(opt->routes->flags & RG_REROUTE_GW)) + && (opt->routes->flags & RG_DEF1)) + { + msg(M_INFO, "Flag 'def1' removed from --redirect-gateway"); + opt->routes->flags &= ~RG_DEF1; + } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5490,37 +5532,6 @@ } } -void -options_server_import(struct options *o, - const char *filename, - int msglevel, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); - read_config_file(o, - filename, - 0, - filename, - 0, - msglevel, - permission_mask, - option_types_found, - es); -} - -void -options_string_import(struct options *options, - const char *config, - const int msglevel, - const unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); -} - #define VERIFY_PERMISSION(mask) { \ if (!verify_permission(p[0], file, line, (mask), permission_mask, \ option_types_found, msglevel, options, is_inline)) \ @@ -5623,6 +5634,198 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} bool apply_push_options(struct context *c, struct options *options, @@ -5656,11 +5859,47 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; } +void +options_server_import(struct options *o, + const char *filename, + int msglevel, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + msg(D_PUSH, "OPTIONS IMPORT: reading client specific options from: %s", filename); + read_config_file(o, + filename, + 0, + filename, + 0, + msglevel, + permission_mask, + option_types_found, + es); +} + +void +options_string_import(struct options *options, + const char *config, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + read_config_string("[CONFIG-STRING]", options, config, msglevel, permission_mask, option_types_found, es); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 0907226..a6c14ca 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 156262a..89ebaee 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1265,7 +1265,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1281,8 +1290,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 237375c..b89ec9f 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -335,6 +335,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 15 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2025-07-21 18:25:51
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 21: Code-Review-1 (4 comments) Patchset: PS21: As discussed on IRC, I do have some things I'd like to see changed before merging. I am taking note of the +2's already given, it's just polishing, comments, readability... File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/20d8d7ae_599aaf68 : PS21, Line 2833: if (!is_update && !check_pull_client_ncp(c, found)) does it make sense to suppress this check? It should never fail, and never modify anything (as we passed on the first run). As it is, this code is a bit confusing, so we'd need a comment ``` /* on PUSH_UPDATE, do not run the NCP checks, because they take 5 minutes * to complete and we are in a hurry */ ``` (put the real reasoning there, of course ;-) ) Mmmh, I think I see the reason, OPT_P_NCP is not set on PUSH_UPDATE, so the code assumes "we have no cipher" and does fallback things... so the comment would need to be something like ``` /* on PUSH_UPDATE, NCP related flags are never updated, and so the code * would assume "no cipher pushed = NCP failed" - so, don't call it on * updates */ ``` File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/758d81ff_703882a6 : PS21, Line 5502: options_server_import(struct options *o, please do not move them (as for 808). thanks :-) http://gerrit.openvpn.net/c/openvpn/+/809/comment/dbc0b2a7_f3d837dd : PS21, Line 3134: } with the extra code, the comment before the function is no longer appropriate. Also, it's unclear to me why this is needed? I'm not guessing the reasoning but claiming "this does not look reasonable" - if `redirect-gateway def1` was pushed or updated, we should not remove it again just because it's not METHOD_SERVICE? If we need it, we definitely need a comment to explain why. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 21 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Mon, 21 Jul 2025 18:25:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-07-23 17:10:32
|
Attention is currently required from: cron2, flichtenheld, mrbff, plaisthos, stipa. Hello cron2, flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email to look at the new patch set (#22). The following approvals got outdated and were removed: Code-Review+2 by stipa, Code-Review-1 by cron2 The change is no longer submittable: Code-Review and checks~ChecksSubmitRule are unsatisfied now. Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... PUSH_UPDATE: Added remove_option() and do_update(). * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/route.c M src/openvpn/route.h 7 files changed, 350 insertions(+), 11 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/809/22 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index ba1dda4..3254cc6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2470,7 +2470,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2594,6 +2594,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2672,11 +2721,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2784,7 +2830,10 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + /* On PUSH_UPDATE, NCP related flags are never updated, and so the code + * would assume "no cipher pushed = NCP failed" - so, don't call it on + * updates */ + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 5c6b9c1..25078a6 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index ec260a2..26f95ec 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2424,7 +2424,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index b214d91..683dae6 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1086,6 +1086,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3091,7 +3125,7 @@ opt->routes->flags |= RG_DEF1; } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5326,6 +5360,18 @@ struct env_set *es); static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es); + +static void read_config_file(struct options *options, const char *file, int level, @@ -5547,6 +5593,11 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; @@ -5685,6 +5736,199 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 858b821..22082a9 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 156262a..89ebaee 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1265,7 +1265,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1281,8 +1290,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 237375c..b89ec9f 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -335,6 +335,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 22 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2025-07-23 17:11:57
|
Attention is currently required from: cron2, flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 22: (3 comments) File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/1bf8ad22_b4ec599b : PS21, Line 2833: if (!is_update && !check_pull_client_ncp(c, found)) > does it make sense to suppress this check? It should never fail, and never modify anything (as we p […] Done File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/0b58e5d7_f3dccb6a : PS21, Line 5502: options_server_import(struct options *o, > please do not move them (as for 808). […] Done http://gerrit.openvpn.net/c/openvpn/+/809/comment/99a69d66_161a9554 : PS21, Line 3134: } > with the extra code, the comment before the function is no longer appropriate. […] actually i should remove it, in case of an update of the option `routes->flags = 0`, so there is no need for this code. I think i forgot there from the previous development where i updated the flags individually. Tnx for the catch. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 22 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Wed, 23 Jul 2025 17:11:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2025-07-27 12:21:24
|
Attention is currently required from: flichtenheld, mrbff, plaisthos, stipa. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 22: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 22 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Sun, 27 Jul 2025 12:21:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: Gert D. <ge...@gr...> - 2025-07-29 10:41:10
|
From: Marco Baffo <ma...@ma...> * Added remove_option() function and some utility functions to remove options at runtime following the push-update logic. * Added do_update() function to close and reopen the tun and apply option updates. Change-Id: I507180d7397b6959844a30908010132bc3411067 Signed-off-by: Marco Baffo <ma...@ma...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/809 This mail reflects revision 23 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/init.c b/src/openvpn/init.c index ba1dda4..3254cc6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2470,7 +2470,7 @@ if (pulled_options) { - if (!do_deferred_options(c, option_types_found)) + if (!do_deferred_options(c, option_types_found, false)) { msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; @@ -2594,6 +2594,55 @@ return true; } +bool +do_update(struct context *c, unsigned int option_types_found) +{ + /* Not necessary since to receive the update the openvpn + * instance must be up and running but just in case + */ + if (!c->c2.do_up_ran) + { + return false; + } + + bool tt_dco_win = tuntap_is_dco_win(c->c1.tuntap); + if (tt_dco_win) + { + msg(M_NONFATAL, "dco-win doesn't yet support reopening TUN device"); + return false; + } + + if (!do_deferred_options(c, option_types_found, true)) + { + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + return false; + } + + do_close_tun(c, true); + + management_sleep(1); + int error_flags = 0; + c->c2.did_open_tun = do_open_tun(c, &error_flags); + update_time(); + + if (c->c2.did_open_tun) + { + /* if --route-delay was specified, start timer */ + if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && c->options.route_delay_defined) + { + event_timeout_init(&c->c2.route_wakeup, c->options.route_delay, now); + event_timeout_init(&c->c2.route_wakeup_expire, c->options.route_delay + c->options.route_delay_window, now); + tun_standby_init(c->c1.tuntap); + } + + initialization_sequence_completed(c, error_flags); + } + + CLEAR(c->c1.pulled_options_digest_save); + + return true; +} + /* * These are the option categories which will be accepted by pull. */ @@ -2672,11 +2721,8 @@ return true; } -/* - * Handle non-tun-related pulled options. - */ bool -do_deferred_options(struct context *c, const unsigned int found) +do_deferred_options(struct context *c, const unsigned int found, const bool is_update) { if (found & OPT_P_MESSAGES) { @@ -2784,7 +2830,10 @@ /* process (potentially) pushed options */ if (c->options.pull) { - if (!check_pull_client_ncp(c, found)) + /* On PUSH_UPDATE, NCP related flags are never updated, and so the code + * would assume "no cipher pushed = NCP failed" - so, don't call it on + * updates */ + if (!is_update && !check_pull_client_ncp(c, found)) { return false; } diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 5c6b9c1..25078a6 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -86,13 +86,29 @@ bool pulled_options, unsigned int option_types_found); +/** + * @brief A simplified version of the do_up() function. This function is called + * after receiving a successful PUSH_UPDATE message. It closes and reopens + * the TUN device to apply the updated options. + * + * @param c The context structure. + * @param option_types_found The options found in the PUSH_UPDATE message. + * @return true on success. + * @return false on error. + */ +bool do_update(struct context *c, unsigned int option_types_found); + unsigned int pull_permission_mask(const struct context *c); const char *format_common_name(struct context *c, struct gc_arena *gc); void reset_coarse_timers(struct context *c); -bool do_deferred_options(struct context *c, const unsigned int found); +/* + * Handle non-tun-related pulled options. + * Set `is_update` param to true to skip NCP check. + */ +bool do_deferred_options(struct context *c, const unsigned int found, const bool is_update); void inherit_context_child(struct context *dest, const struct context *src, diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index b2d2b6c..68b4da6 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2424,7 +2424,7 @@ /* * Process sourced options. */ - do_deferred_options(&mi->context, option_types_found); + do_deferred_options(&mi->context, option_types_found, false); /* * make sure we got ifconfig settings from somewhere diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 2083eae..3a8ce86 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1086,6 +1086,40 @@ gc_free(&gc); } } + +static void +delete_all_dhcp_fo(struct options *o, struct env_item **list) +{ + struct env_item *current, *prev; + + ASSERT(list); + + for (current = *list, prev = NULL; current != NULL; current = current->next) + { + char *tmp_value = NULL; + if (!strncmp(current->string, "foreign_option_", sizeof("foreign_option_")-1)) + { + tmp_value = strchr(current->string, '='); + if (tmp_value && ++tmp_value) + { + if (!strncmp(tmp_value, "dhcp-option ", sizeof("dhcp-option ")-1)) + { + if (prev) + { + prev->next = current->next; + } + else + { + *list = current->next; + } + o->foreign_option_index--; + } + } + } + prev = current; + } +} + #endif /* ifndef _WIN32 */ static in_addr_t @@ -3091,7 +3125,7 @@ opt->routes->flags |= RG_DEF1; } } -#endif +#endif /* ifdef _WIN32 */ /* * Save/Restore certain option defaults before --pull is applied. @@ -5326,6 +5360,18 @@ struct env_set *es); static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es); + +static void read_config_file(struct options *options, const char *file, int level, @@ -5551,6 +5597,11 @@ add_option(options, p, false, file, line_num, 0, msglevel, permission_mask, option_types_found, es); } + else if (push_update_option_flags & PUSH_OPT_TO_REMOVE) + { + remove_option(c, options, p, false, file, line_num, msglevel, + permission_mask, option_types_found, es); + } } } return true; @@ -5689,6 +5740,199 @@ return options->forward_compatible ? M_WARN : msglevel; } +/** + * @brief Resets options found in the PUSH_UPDATE message that are preceded by the `-` flag. + * This function is used in push-updates to reset specified options. + * The number of parameters `p` must always be 1. If the permission is verified, + * all related options are erased or reset to their default values. + * Upon successful permission verification (by VERIFY_PERMISSION()), + * `option_types_found` is filled with the flag corresponding to the option. + * + * @param c The context structure. + * @param options A pointer to the options structure. + * @param p An array of strings containing the options and their parameters. + * @param is_inline A boolean indicating if the option is inline. + * @param file The file where the function is called. + * @param line The line number where the function is called. + * @param msglevel The message level. + * @param permission_mask The permission mask used by VERIFY_PERMISSION(). + * @param option_types_found A pointer to the variable where the flags corresponding to the options found are stored. + * @param es The environment set structure. + */ +static void +remove_option(struct context *c, + struct options *options, + char *p[], + bool is_inline, + const char *file, + int line, + const int msglevel, + const unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es) +{ + int msglevel_fc = msglevel_forward_compatible(options, msglevel); + + if (streq(p[0], "ifconfig") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_local = NULL; + options->ifconfig_remote_netmask = NULL; + } + else if (streq(p[0], "ifconfig-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->ifconfig_ipv6_local = NULL; + options->ifconfig_ipv6_netbits = 0; + options->ifconfig_ipv6_remote = NULL; + } + else if (streq(p[0], "route") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_list) + { + delete_routes_v4(c->c1.route_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes) + { + options->routes->routes = NULL; + options->routes->flags = 0; + } + } + } + else if (streq(p[0], "route-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (c->c1.route_ipv6_list) + { + delete_routes_v6(c->c1.route_ipv6_list, c->c1.tuntap, + ROUTE_OPTION_FLAGS(&c->options), + es, &c->net_ctx); + if (options->routes_ipv6) + { + options->routes_ipv6->routes_ipv6 = NULL; + options->routes_ipv6->flags = 0; + } + } + } + else if (streq(p[0], "route-gateway") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE_EXTRAS); + options->route_gateway_via_dhcp = false; + options->route_default_gateway = NULL; + } + else if (streq(p[0], "route-metric") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->route_default_metric = 0; + } + else if (streq(p[0], "push-continuation") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PULL_MODE); + options->push_continuation = 0; + } + else if ((streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + if (options->routes) + { + options->routes->flags = 0; + } + if (options->routes_ipv6) + { + options->routes_ipv6->flags = 0; + } + } + else if (streq(p[0], "dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + gc_free(&options->dns_options.gc); + CLEAR(options->dns_options); + } + else if (streq(p[0], "topology") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_UP); + options->topology = TOP_UNDEF; + helper_setdefault_topology(options); + } + else if (streq(p[0], "tun-mtu") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION); + options->ce.tun_mtu = TUN_MTU_DEFAULT; + options->ce.tun_mtu_defined = false; + options->ce.occ_mtu = 0; + } + else if (streq(p[0], "block-ipv6") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_ROUTE); + options->block_ipv6 = false; + } +#if defined(_WIN32) || defined(TARGET_ANDROID) + else if (streq(p[0], "dhcp-option") && !p[1]) + { + struct tuntap_options *o = &options->tuntap_options; + VERIFY_PERMISSION(OPT_P_DHCPDNS); + + o->domain = NULL; + o->netbios_scope = NULL; + o->netbios_node_type = 0; + o->dns6_len = 0; + memset(o->dns6, 0, sizeof(o->dns6)); + o->dns_len = 0; + memset(o->dns, 0, sizeof(o->dns)); + o->wins_len = 0; + memset(o->wins, 0, sizeof(o->wins)); + o->ntp_len = 0; + memset(o->ntp, 0, sizeof(o->ntp)); + o->nbdd_len = 0; + memset(o->nbdd, 0, sizeof(o->nbdd)); + while (o->domain_search_list_len-- > 0) + { + o->domain_search_list[o->domain_search_list_len] = NULL; + } + o->disable_nbt = 0; + o->dhcp_options = 0; +#if defined(TARGET_ANDROID) + o->http_proxy_port = 0; + o->http_proxy = NULL; +#endif + } +#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ +#ifdef _WIN32 + else if (streq(p[0], "block-outside-dns") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + options->block_outside_dns = false; + } +#else /* ifdef _WIN32 */ + else if (streq(p[0], "dhcp-option") && !p[1]) + { + VERIFY_PERMISSION(OPT_P_DHCPDNS); + delete_all_dhcp_fo(options, &es->list); + } +#endif + else + { + int i; + int msglevel_unknown = msglevel_fc; + /* Check if an option is in --ignore-unknown-option and + * set warning level to non fatal */ + for (i = 0; options->ignore_unknown_option && options->ignore_unknown_option[i]; i++) + { + if (streq(p[0], options->ignore_unknown_option[i])) + { + msglevel_unknown = M_WARN; + break; + } + } + msg(msglevel_unknown, "Unrecognized option or missing or extra parameter(s) in %s:%d: -%s (%s)", file, line, p[0], PACKAGE_VERSION); + } + return; +err: + msg(msglevel, "Error occurred trying to remove %s option", p[0]); +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 858b821..22082a9 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -542,6 +542,11 @@ { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } + else if (!do_update(c, option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to update options"); + goto error; + } } } event_timeout_clear(&c->c2.push_request_interval); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 156262a..89ebaee 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1265,7 +1265,16 @@ const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx) { - if (rl && rl->iflags & RL_ROUTES_ADDED) + delete_routes_v4(rl, tt, flags, es, ctx); + delete_routes_v6(rl6, tt, flags, es, ctx); +} + +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl && (rl->iflags & RL_ROUTES_ADDED)) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) @@ -1281,8 +1290,14 @@ { clear_route_list(rl); } +} - if (rl6 && (rl6->iflags & RL_ROUTES_ADDED) ) +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx) +{ + if (rl6 && (rl6->iflags & RL_ROUTES_ADDED)) { struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 237375c..b89ec9f 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -335,6 +335,16 @@ const struct env_set *es, openvpn_net_ctx_t *ctx); +void +delete_routes_v4(struct route_list *rl, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + +void +delete_routes_v6(struct route_ipv6_list *rl6, const struct tuntap *tt, + unsigned int flags, const struct env_set *es, + openvpn_net_ctx_t *ctx); + void setenv_routes(struct env_set *es, const struct route_list *rl); void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6); |