From: Marco Baffo <[email protected]> Use the current virtual addresses as parser defaults so omitted options remain unchanged and NULL consistently means address removal. Move removal into update_vaddr(), skip unchanged addresses, and set the new address before learning its route. If learning either new address fails, restart the client since the PUSH_UPDATE is already queued. Report the scheduled exit as a failed update so management does not count the client as successfully updated.
Change-Id: I75ba77fcbb11790c7cc86cc436fc75ff99bd1c9e Signed-off-by: Marco Baffo <[email protected]> Acked-by: Razvan Cojocaru <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/2060 --- 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/+/2060 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Razvan Cojocaru <[email protected]> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index af8b1c7..0027550 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -4477,7 +4477,7 @@ } /* Function to unlearn previous ifconfig of a client in the server multi_context after a PUSH_UPDATE */ -void +static void unlearn_ifconfig(struct multi_context *m, struct multi_instance *mi) { in_addr_t old_addr = 0; @@ -4489,7 +4489,7 @@ } /* Function to unlearn previous ifconfig-ipv6 of a client in the server multi_context after a PUSH_UPDATE */ -void +static void unlearn_ifconfig_ipv6(struct multi_context *m, struct multi_instance *mi) { struct in6_addr old_addr6; @@ -4502,59 +4502,102 @@ } /** - * Update the vhash with new IP/IPv6 addresses in the multi_context when a - * push-update message containing ifconfig/ifconfig-ipv6 options is sent - * from the server. + * Update the vhash when a push-update message containing + * ifconfig/ifconfig-ipv6 options is sent from the server. * * @param m The multi_context * @param mi The multi_instance of the client we are updating - * @param new_ip The new IPv4 address or NULL if no change - * @param new_ipv6 The new IPv6 address or NULL if no change + * @param new_ip The new IPv4 address, NULL to remove, current address to keep + * @param new_ipv6 The new IPv6 address, NULL to remove, current address to keep + * @return false if the update failed, true otherwise. On failure the + * client is restarted */ -void -update_vhash(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6) +bool +update_vaddr(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6) { + /* -ifconfig */ + if (!new_ip && mi->context.c2.push_ifconfig_defined) + { + unlearn_ifconfig(m, mi); + } + /* -ifconfig-ipv6 */ + if (!new_ipv6 && mi->context.c2.push_ifconfig_ipv6_defined) + { + unlearn_ifconfig_ipv6(m, mi); + } + if (new_ip) { - /* Remove old IP */ - if (mi->context.c2.push_ifconfig_defined) - { - unlearn_ifconfig(m, mi); - } - - /* Add new IP */ struct in_addr new_addr; - CLEAR(new_addr); if (inet_pton(AF_INET, new_ip, &new_addr) == 1 - && multi_learn_in_addr_t(m, mi, ntohl(new_addr.s_addr), -1, true)) + && (!mi->context.c2.push_ifconfig_defined + || ntohl(new_addr.s_addr) != mi->context.c2.push_ifconfig_local)) { + /* Remove old IP */ + if (mi->context.c2.push_ifconfig_defined) + { + unlearn_ifconfig(m, mi); + } + + /* DCO needs the new address while installing its on-link route. */ mi->context.c2.push_ifconfig_defined = true; mi->context.c2.push_ifconfig_local = ntohl(new_addr.s_addr); - /* set our client's VPN endpoint for status reporting purposes */ - mi->reporting_addr = mi->context.c2.push_ifconfig_local; + if (multi_learn_in_addr_t(m, mi, mi->context.c2.push_ifconfig_local, -1, true) == mi) + { + /* set our client's VPN endpoint for status reporting purposes */ + mi->reporting_addr = mi->context.c2.push_ifconfig_local; + } + else + { + mi->context.c2.push_ifconfig_defined = false; + mi->context.c2.push_ifconfig_local = 0; + + msg(M_WARN, "MULTI: failed to learn PUSH_UPDATE IPv4 address %s, restarting client", + new_ip); + /* PUSH_UPDATE is already queued, so reconnect to restore consistent state. */ + send_restart(&mi->context, NULL); + multi_schedule_context_wakeup(m, mi); + return false; + } } } if (new_ipv6) { - /* Remove old IPv6 */ - if (mi->context.c2.push_ifconfig_ipv6_defined) - { - unlearn_ifconfig_ipv6(m, mi); - } - - /* Add new IPv6 */ struct in6_addr new_addr6; - CLEAR(new_addr6); if (inet_pton(AF_INET6, new_ipv6, &new_addr6) == 1 - && multi_learn_in6_addr(m, mi, new_addr6, -1, true)) + && (!mi->context.c2.push_ifconfig_ipv6_defined + || memcmp(&new_addr6, &mi->context.c2.push_ifconfig_ipv6_local, sizeof(new_addr6)))) { + /* Remove old IPv6 */ + if (mi->context.c2.push_ifconfig_ipv6_defined) + { + unlearn_ifconfig_ipv6(m, mi); + } + + /* DCO needs the new address while installing its on-link route. */ mi->context.c2.push_ifconfig_ipv6_defined = true; mi->context.c2.push_ifconfig_ipv6_local = new_addr6; - /* set our client's VPN endpoint for status reporting purposes */ - mi->reporting_addr_ipv6 = mi->context.c2.push_ifconfig_ipv6_local; + if (multi_learn_in6_addr(m, mi, new_addr6, -1, true) == mi) + { + /* set our client's VPN endpoint for status reporting purposes */ + mi->reporting_addr_ipv6 = mi->context.c2.push_ifconfig_ipv6_local; + } + else + { + mi->context.c2.push_ifconfig_ipv6_defined = false; + CLEAR(mi->context.c2.push_ifconfig_ipv6_local); + + msg(M_WARN, "MULTI: failed to learn PUSH_UPDATE IPv6 address %s, restarting client", + new_ipv6); + /* PUSH_UPDATE is already queued, so reconnect to restore consistent state. */ + send_restart(&mi->context, NULL); + multi_schedule_context_wakeup(m, mi); + return false; + } } } + return true; } bool diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h index 3d2e932..f5a1a6f 100644 --- a/src/openvpn/multi.h +++ b/src/openvpn/multi.h @@ -661,9 +661,7 @@ lookup_by_cid(struct multi_context *m, const unsigned long cid); #endif -void -update_vhash(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6); -void unlearn_ifconfig(struct multi_context *m, struct multi_instance *mi); -void unlearn_ifconfig_ipv6(struct multi_context *m, struct multi_instance *mi); +bool +update_vaddr(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6); #endif /* MULTI_H */ diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c index 34cb26c..cd940fb 100644 --- a/src/openvpn/push_util.c +++ b/src/openvpn/push_util.c @@ -149,14 +149,35 @@ struct options o; CLEAR(o); - /* Set canary values to detect ifconfig options in push-update messages. - * These placeholder strings will be overwritten to NULL by the option - * parser if -ifconfig or -ifconfig-ipv6 options are present in the - * push-update. + /* Set canary values to detect -ifconfig and -ifconfig-ipv6 options in + * push-update messages. These placeholder strings will be overwritten + * to NULL by the option parser if -ifconfig or -ifconfig-ipv6 options + * are present in the push-update, to the new value if present, or + * they will remain equal to the current values otherwise. + * If the current values are not defined they will remain NULL. */ - const char *canary = "canary"; - o.ifconfig_local = canary; - o.ifconfig_ipv6_local = canary; + + char ifconfig_local[INET_ADDRSTRLEN]; + if (mi->context.c2.push_ifconfig_defined) + { + struct in_addr addr4 = { .s_addr = htonl(mi->context.c2.push_ifconfig_local) }; + if (!inet_ntop(AF_INET, &addr4, ifconfig_local, sizeof(ifconfig_local))) + { + return false; + } + o.ifconfig_local = ifconfig_local; + } + + char ifconfig_ipv6_local[INET6_ADDRSTRLEN]; + if (mi->context.c2.push_ifconfig_ipv6_defined) + { + if (!inet_ntop(AF_INET6, &mi->context.c2.push_ifconfig_ipv6_local, + ifconfig_ipv6_local, sizeof(ifconfig_ipv6_local))) + { + return false; + } + o.ifconfig_ipv6_local = ifconfig_ipv6_local; + } struct buffer_entry *e = msgs->head; while (e) @@ -192,34 +213,15 @@ e = e->next; } + bool success = true; if (option_types_found & OPT_P_UP) { - /* -ifconfig */ - if (!o.ifconfig_local && mi->context.c2.push_ifconfig_defined) - { - unlearn_ifconfig(m, mi); - } - /* -ifconfig-ipv6 */ - if (!o.ifconfig_ipv6_local && mi->context.c2.push_ifconfig_ipv6_defined) - { - unlearn_ifconfig_ipv6(m, mi); - } - - if (o.ifconfig_local && !strcmp(o.ifconfig_local, canary)) - { - o.ifconfig_local = NULL; - } - if (o.ifconfig_ipv6_local && !strcmp(o.ifconfig_ipv6_local, canary)) - { - o.ifconfig_ipv6_local = NULL; - } - - /* new ifconfig or new ifconfig-ipv6 */ - update_vhash(m, mi, o.ifconfig_local, o.ifconfig_ipv6_local); + /* ifconfig or ifconfig-ipv6 */ + success = update_vaddr(m, mi, o.ifconfig_local, o.ifconfig_ipv6_local); } uninit_options(&o); - return true; + return success; } /* Return true if the client supports push-update */ diff --git a/tests/unit_tests/openvpn/mock_push_dependencies.c b/tests/unit_tests/openvpn/mock_push_dependencies.c index f083f89..041a5e6 100644 --- a/tests/unit_tests/openvpn/mock_push_dependencies.c +++ b/tests/unit_tests/openvpn/mock_push_dependencies.c @@ -48,19 +48,10 @@ return flags; } -void -unlearn_ifconfig(struct multi_context *m, struct multi_instance *mi) +bool +update_vaddr(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6) { -} - -void -unlearn_ifconfig_ipv6(struct multi_context *m, struct multi_instance *mi) -{ -} - -void -update_vhash(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6) -{ + return true; } void _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
