openvpn icon indicating copy to clipboard operation
openvpn copied to clipboard

Old/New IP - Logic Redo

Open stoops opened this issue 1 month ago • 2 comments

There was an update pushed earlier this year which undid some important logic in terms of learning/unlearning the ifconfig IP address. The older original code would check that the new/old IPs don't match first before performing that logic. I believe this was important and correct because it could cause connection interruptions during the learn/unlearn process if the IP address has stayed the same!

commit 5e4c9a69eaf9d32e85613bd71ed219fdbb062d34
Author: Marco Baffo <[email protected]>
Date:   Fri Oct 17 22:19:12 2025 +0200
...
-update_vhash(struct multi_context *m, struct multi_instance *mi, const char *old_ip, const char *old_ipv6)
+update_vhash(struct multi_context *m, struct multi_instance *mi, const char *new_ip, const char *new_ipv6)
 {
-    struct in_addr addr;
-    struct in6_addr new_ipv6;
-
-    if ((mi->context.options.ifconfig_local && (!old_ip || strcmp(old_ip, mi->context.options.ifconfig_local)))
-        && inet_pton(AF_INET, mi->context.options.ifconfig_local, &addr) == 1)
+    if (new_ip)
     {

stoops avatar Nov 19 '25 14:11 stoops

As it has been discussed on the mailing list, this is currently not seen as a desirable patch, in the pre-2.7 runup.

This said, to consider this in the port-2.7 phase, it would need to be reworked

  • the repetition of if (addr_stat == 1 && new_addr_t != old_addr_t does not make anyhting more readable
  • it might actually not work correctly, if there is no new_ip at all (PUSH_REPLY -ifconfig) and addr_stat ends up being 0, so not removing the old ip - this is a valid use case
  • IPv6 is not covered at all

cron2 avatar Nov 19 '25 15:11 cron2

As it has been discussed on the mailing list, this is currently not seen as a desirable patch, in the pre-2.7 runup.

This said, to consider this in the port-2.7 phase, it would need to be reworked

  • the repetition of if (addr_stat == 1 && new_addr_t != old_addr_t does not make anyhting more readable
  • it might actually not work correctly, if there is no new_ip at all (PUSH_REPLY -ifconfig) and addr_stat ends up being 0, so not removing the old ip - this is a valid use case
  • IPv6 is not covered at all

Thanks, I did miss those cases you mentioned - I admit I am not an IPv6 expert also but anyway, its worth a try for me to re-work the logic to see if I can make it better (or if the devs there who know more than me could redo it in a more proper style than what I could do). It might take me some time. :)

stoops avatar Nov 19 '25 17:11 stoops