Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipsec: connections VTI support for dynamic local/remote IP #6781

Closed
2 tasks done
pfoo opened this issue Aug 25, 2023 · 29 comments
Closed
2 tasks done

ipsec: connections VTI support for dynamic local/remote IP #6781

pfoo opened this issue Aug 25, 2023 · 29 comments
Assignees
Labels
feature Adding new functionality roadmap Major roadmap item
Milestone

Comments

@pfoo
Copy link

pfoo commented Aug 25, 2023

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug

With legacy IPsec configuration it is possible to :

  • input FQDN in ipsec remote outter IP configuration.
  • use interface name for local outter IP configuration.

VTI interface was then created/modified according to FQDN resolution and/or interface IP change, this was usefull when having a dynamic local IP (assigned to the interface) or a dynamic remote IP.

Main issue : With the new IPsec Connection UI it is impossible to add FQDN or interface name in the Virtual Tunnel Interfaces tab, which breaks dynamic IP VTI configurations.

IPSec configuration itself should work as we can input FQDN in local_addrs/remote_addrs fields, however the help text state that "If FQDNs are assigned, they are resolved every time a configuration lookup is done."
Secondary issue : An option to regularly check if the FQDN changed it's IP (or maybe allow to use alias) and reload ipsec and associated automatic-pass firewall rules might be useful as it will prevent unnecessary use of %any.

For now new IPsec connection UI seems broken for dynamic local/remote IP in routed/VTI setup.

To Reproduce

Steps to reproduce the behavior:

  1. Go to VPN -> IPsec -> Virtual Tunnel Interfaces
  2. Click on 'Add'
  3. Try to enter anything else than an IP address in 'Local address' or 'Remote address'
  4. See error : 'Please specify a valid address.'

Expected behavior

Allow IPsec routed/VTI configurations to works with local and remote dynamic IPs :

  • VTI tab -> For remote addresses : Add ability to enter FQDN and/or aliases and resolve them regularly and update VTI interface accordingly.
  • VTI tab -> For local addresses : Add ability to track an interface to update the VTI local IP accordingly. This would also means we need to specify if we want to track ipv4 or ipv6 of the interface.
  • IPSec Connection tab -> make sure FQDN are regularly resolved or allow to use aliases and reload ipsec when the IP changes.

Describe alternatives you considered

The only workaround would be to manually update the VTI interface configuration every time the local/remote IP changes.

Additional context

Some reports on the forum :
https://forum.opnsense.org/index.php?topic=35140.0
https://forum.opnsense.org/index.php?topic=33117.0

Environment

OPNsense 23.7.2-amd64

@AdSchellevis AdSchellevis added the support Community support label Aug 25, 2023
@AdSchellevis
Copy link
Member

can't work reliable, see note in https://docs.opnsense.org/manual/vpnet.html#route-based-vti

@pfoo
Copy link
Author

pfoo commented Aug 25, 2023

No it won't be as reliable as with a static IP. But I have a working VTI configuration with dynamic IP for months now on opnsense, years even on pfsense.

As soon as you have dynamic IP somewhere, it does not work reliably anymore, VTI or not, as you will have to tolerate some downtime.

The tunnel endpoints can be changed live with a single ifconfig command. The main issue is detecting when it needs to be changed.

Unfortunately a lot of us are still stuck to dynamic IP especially on non-professional home ISP

@AdSchellevis
Copy link
Member

if/when if_ipsec(4) supports the endpoints to be changed without recreating the interface, we might reconsider putting time in this, but until that time, this isn't a priority as it will break somewhere and lead to hard to explain support cases. While developing the new module we tried different options (binding to non existing endpoints, changing on updates), but it's just not reliable due to upstream reasons.

@8191
Copy link
Member

8191 commented Sep 1, 2023

No it won't be as reliable as with a static IP. But I have a working VTI configuration with dynamic IP for months now on opnsense, years even on pfsense.

I double that. I use IPsec VTI since a few years now (I believe starting with 21.1) and did not have any major problems so far.
In my case the IPs do not change frequently, only once each few months, I'd say. Anyway, restarting the IPsec tunnel on the centre firewall is still easier than modifying the configuration (causing potential config errors) after each IP change.

@AdSchellevis
Copy link
Member

You can keep using the legacy tunnels, maybe eventually we think of something that is sustainable or an upstream change will offer the possibility to change if_ipsec(4) addressing on the fly without breaking traffic at some point.

@AdSchellevis AdSchellevis added feature Adding new functionality and removed support Community support labels Oct 18, 2023
@AdSchellevis AdSchellevis self-assigned this Oct 18, 2023
@cektom
Copy link

cektom commented Oct 29, 2023

+1 for this. I'm also using a Site-to-Site Tunnel with dynamic IPs on both sides for over a year now. With some monit rules to check the tunnel health and dead peer detection enabled, the downtime was ~5-10 minutes every few weeks.

@bipbip365
Copy link

@cektom
Hello, I might have rules to check tunnel health and dead peer detection enabled. I need to put the same configuration in place

AdSchellevis added a commit that referenced this issue Feb 12, 2024
…nfiguration to connection up event in order to support dynamic dns scenarios. a bit of an experiment for #6781

Simplify ipsec_configure_vti() to make sure we only drop interfaces when not required anymore (tunnel address cleanup is unconditional) and only set local/remote address when configured.
AdSchellevis added a commit that referenced this issue Feb 12, 2024
…nfiguration to connection up event in order to support dynamic dns scenarios. a bit of an experiment for #6781

Simplify ipsec_configure_vti() to make sure we only drop interfaces when not required anymore (tunnel address cleanup is unconditional) and only set local/remote address when configured.
@AdSchellevis
Copy link
Member

This 816ecae should do the trick, but now I'm looking for people to try it out.

@joni1993
Copy link

I could successfully set this up with one dynamic IP and one static at least. (Leaving both addresses empty).

@AdSchellevis
Copy link
Member

@joni1993 thanks for testing. I'll leave this here for a little while, maybe someone else want to try it as well. Merging should be relatively safe as setups with local/remote addresses set are not provisioned using the new hook.

@fichtner fichtner added the roadmap Major roadmap item label Feb 15, 2024
@fichtner fichtner added this to the 24.7 milestone Feb 15, 2024
@fichtner fichtner changed the title New IPsec Connections UI: VTI is unusable for dynamic local/remote IP ipsec: connections VTI support for dynamic local/remote IP Feb 15, 2024
@AdSchellevis
Copy link
Member

nobody else interested in this? that's surprising....

@stefankubis
Copy link

still in use as a "legacy config" - but ever wondered why this did not work using the new way - how is this testable?

@AdSchellevis
Copy link
Member

opnsense-patch 816ecae ?

fichtner pushed a commit that referenced this issue Mar 12, 2024
…nfiguration to connection up event in order to support dynamic dns scenarios. closes #6781

Simplify ipsec_configure_vti() to make sure we only drop interfaces when not required anymore (tunnel address cleanup is unconditional) and only set local/remote address when configured.
fichtner pushed a commit that referenced this issue Mar 15, 2024
…nfiguration to connection up event in order to support dynamic dns scenarios. closes #6781

(cherry picked from commit 7182c04)
(cherry picked from commit e26112a)
@pfoo
Copy link
Author

pfoo commented Mar 29, 2024

Seems to be working !
I however have to figure a way to force my ISP to change my ipv4 to fully test this

@joni1993
Copy link

joni1993 commented Apr 22, 2024

@AdSchellevis: I am not sure what exactly changed, but for me it is now not working anymore. Not sure if it was a coincidence it worked back then or something else changed (did not find any related code changes).

Enabling the old setup works - i noticed following changes comparing the old and new UI method:

ifconfig does not show the tunnel attribute when using the new UI - also it is not RUNNING.
Manually executing ifconfig ipsec100 tunnel LOCAL_WAN_IP REMOTE_WAN_IP (the ips substituted ofcourse), brings the tunnel into RUNNING and makes the remote inner tunnel ip respond. Manually reapplying routes also brings connectivity over the tunnel to remote networks.

If i can help any further, just let me know.

EDIT: Above method worked once, now i can't get it working again.
EDIT2: Needed to manually reconnect the tunnel.

@AdSchellevis
Copy link
Member

@joni1993 when the old type works, it's unlikely related to this change as both are being setup quite similar, on my end a statically configure vti looks normal after an update.

$configured_intf = array_merge(ipsec_get_configured_vtis(), (new \OPNsense\IPsec\Swanctl())->getVtiDevices());
$current_interfaces = [];
foreach (legacy_interfaces_details() as $intf => $intf_details) {
if (strpos($intf, 'ipsec') === 0) {
$current_interfaces[$intf] = $intf_details;
}
}
service_log(sprintf('Creating IPsec VTI instance%s...', empty($device) ? 's' : " {$device}"), $verbose);
// drop changed or not existing interfaces and tunnel endpoints
foreach ($current_interfaces as $intf => $intf_details) {
if ($device !== null && $device != $intf) {
continue;
}
if (empty($configured_intf[$intf])) {
log_msg(sprintf("destroy interface %s", $intf), LOG_DEBUG);
legacy_interface_destroy($intf);
unset($current_interfaces[$intf]);
} else {
foreach (['ipv4', 'ipv6'] as $proto) {
foreach ($intf_details[$proto] as $addr) {
if (!empty($addr['endpoint'])) {
$isfound = false;
foreach ($configured_intf[$intf]['networks'] as $network) {
if (
$network['tunnel_local'] == $addr['ipaddr']
&& $network['tunnel_remote'] == $addr['endpoint']
) {
$isfound = true;
break;
}
}
if (!$isfound) {
log_msg(sprintf(
"remove tunnel %s %s from interface %s",
$addr['ipaddr'],
$addr['endpoint'],
$intf
), LOG_DEBUG);
mwexecf('/sbin/ifconfig %s %s %s delete', [
$intf, $proto == 'ipv6' ? 'inet6' : 'inet', $addr['ipaddr'], $addr['endpoint']
]);
}
}
}
}
}
}
// configure new interfaces and tunnels
foreach ($configured_intf as $intf => $intf_details) {
if ($device !== null && $device != $intf) {
continue;
}
// create required interfaces
if (empty($current_interfaces[$intf])) {
// prevent ipsec vti interface to hit 32768 limit (create numbered, rename and attach afterwards)
if (legacy_interface_create('ipsec', $intf) === null) {
break;
}
}
// [re]initialise if_ipsec
mwexecf('/sbin/ifconfig %s reqid %s', [$intf, $intf_details['reqid']]);
if (!empty($intf_details['local']) && !empty($intf_details['remote'])) {
mwexecf('/sbin/ifconfig %s %s tunnel %s %s up', [
$intf,
is_ipaddrv6($intf_details['local']) ? 'inet6' : 'inet',
$intf_details['local'],
$intf_details['remote']
]);
}
// create new tunnel endpoints
foreach ($intf_details['networks'] as $endpoint) {
if (!empty($current_interfaces[$intf])) {
$already_configured = $current_interfaces[$intf][$endpoint['inet'] == 'inet6' ? 'ipv6' : 'ipv4'];
} else {
$already_configured = [];
}
$isfound = false;
foreach ($already_configured as $addr) {
if (!empty($addr['endpoint'])) {
if (
$endpoint['tunnel_local'] == $addr['ipaddr']
&& $endpoint['tunnel_remote'] == $addr['endpoint']
) {
$isfound = true;
}
}
}
if (!$isfound) {
if ($endpoint['inet'] == 'inet') {
mwexecf('/sbin/ifconfig %s %s %s %s', [
$intf, $endpoint['inet'], sprintf("%s/%s", $endpoint['tunnel_local'], $endpoint['mask']),
$endpoint['tunnel_remote']
]);
} else {
// XXX: don't specify a tunnel endpoint for ipv6, although this looks like an illogical
// construction, a netmask seems to be a requirement for some ipv6 consumers (frr)
mwexecf('/sbin/ifconfig %s %s %s', [
$intf, $endpoint['inet'], sprintf("%s/%s", $endpoint['tunnel_local'], $endpoint['mask'])
]);
}
}
}
}

The patch 659ed11 was released in 24.1.4 if I'm not mistaken.

@joni1993
Copy link

joni1993 commented Apr 23, 2024

Are they really set up similar?
I do not know where the generated files are coming from but looking at /usr/local/etc/swanctl/swanctl.conf, the old method generates local_addrs and remote_addrs configuration for the connection, while the new method does not generate such configuration entries.

On the other hand the new method generates the updown event handler in the children section (which the old one does not) - and manually executing the handler like /usr/local/opnsense/scripts/ipsec/updown_event.py --connection_child 782d9cb1-8af0-4890-a8bf-007eb1934394 --action up --reqid 100 --local MYIP --remote REMOTEIP throws:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/configparser.py", line 789, in get
    value = d[option]
  File "/usr/local/lib/python3.9/collections/__init__.py", line 941, in __getitem__
    return self.__missing__(key)            # support subclasses that define __missing__
  File "/usr/local/lib/python3.9/collections/__init__.py", line 933, in __missing__
    raise KeyError(key)
KeyError: 'connection_child'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/opnsense/scripts/ipsec/updown_event.py", line 64, in <module>
    or cnf.get(section, 'connection_child') == cmd_args.connection_child:
  File "/usr/local/lib/python3.9/configparser.py", line 792, in get
    raise NoOptionError(option, section)
configparser.NoOptionError: No option 'connection_child' in section: 'vti_72691b462a734a82963ade379b5fb4b5'

@joni1993
Copy link

joni1993 commented Apr 23, 2024

Changing the updown event script as follows:


diff --git a/src/opnsense/scripts/ipsec/updown_event.py b/src/opnsense/scripts/ipsec/updown_event.py
index dd3030460..6facc4b13 100755
--- a/src/opnsense/scripts/ipsec/updown_event.py
+++ b/src/opnsense/scripts/ipsec/updown_event.py
@@ -59,9 +59,8 @@ if __name__ == '__main__':
             spds = []
             vtis = []
             for section in cnf.sections():
-                if cnf.get(section, 'reqid') == cmd_args.reqid \
-                        or cnf.get(section, 'connection_child') == cmd_args.connection_child:
-                    if section.startswith('spd_'):
+                if cnf.get(section, 'reqid') == cmd_args.reqid:
+                    if cnf.has_option(section,"connection_child") and cnf.get(section, 'connection_child') == cmd_args.connection_child and section.startswith('spd_'):
                         spds.append({
                             'reqid': cmd_args.reqid,
                             'local' : cmd_args.local,

Adds the tunnel property to the interface correctly and bringing up the tunnel. Only one manual disconnect/connect was needed after changing the script - disabling and enabling the vti was not enough. (why? - when creating a vti one want's to fire the event to bring up the tunnel?)

I don't know if the patch is correct for the SPD part though.. but the vti part of src/opnsense/service/templates/OPNsense/IPsec/reqid_events.conf definetely never have the connection_child section

@AdSchellevis
Copy link
Member

@joni1993 what does the reqid_events.conf looks like on your end? it's unlikely there are VTI sections in the configuration when properly configured with static addresses

{% if vti.enabled == '1' and vti.local|default('') == '' and vti.remote|default('') == '' %}

Without sections starting with vti_ the ifconfig commands are not being executed.

@joni1993
Copy link

joni1993 commented Apr 23, 2024

I am not using static addresses - thats the whole point of this issue right? ;)

"Phase 1" (Connections):
Local:
Remote:

"Local Authentication" (Connections):
Round: 0
Authentication: PSK
id: [A static CNAME pointing to a DynDns domain]

"Remote Authentication" (Connections):
Round: 0
Authentication: PSK
id:

"Children" (Connections):
Mode: Tunnel
Start action: Trap+Start
DPD action: Start
Reqid: 100
Local: 0.0.0.0/0
Remote: 0.0.0.0/0

Pre-Shared-Keys:
Local Identifier:
Remote Identifier:
PSK:
Type: PSK

Virtual Tunnel Interface:
Reqid: 100
Local address: (because it uses dynamic ips)
Remote address: (because both need to be empty if one is)
Tunnel local address: [x.y.z.1]
Tunnel remote address: [x.y.z.2]

reqid_events:

...
[vti_6066f6a169734e1089435d68c390ff17]
reqid = 100
description = VPN
uuid = 6066f6a1-6973-4e10-8943-5d68c390ff17
...

@AdSchellevis
Copy link
Member

@joni1993 you mentioned your setup worked before this change, dynamic addresses weren't supported before 21.1.4 for the new connections.

@joni1993
Copy link

It worked when i was on 21.1.1 + manually applying the patch.

But i suspect it maybe worked because back then i did just disable the old config and enabled the new config at the same time and when applying some "old state"/IKE/Phase2 got reused for the new config - which made it seem working.

But yesterday after updating to 24.1.6, it did not work anymore, so i suspect it initially only worked because of some quirks/coincidence/leftovers-states.

@AdSchellevis
Copy link
Member

if the log contains the event being processed in :

syslog.syslog(syslog.LOG_NOTICE, '[UPDOWN] received %s event for reqid %s' % (cmd_args.action, cmd_args.reqid))

we can add some additional logging to help local debugging, in that case just open a new ticket to request more logging in this event.

@joni1993
Copy link

Yes it does include the line - but the script probably fails as written in my above comment - because the tunnel property is never applied.

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/configparser.py", line 789, in get
    value = d[option]
  File "/usr/local/lib/python3.9/collections/__init__.py", line 941, in __getitem__
    return self.__missing__(key)            # support subclasses that define __missing__
  File "/usr/local/lib/python3.9/collections/__init__.py", line 933, in __missing__
    raise KeyError(key)
KeyError: 'connection_child'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/opnsense/scripts/ipsec/updown_event.py", line 64, in <module>
    or cnf.get(section, 'connection_child') == cmd_args.connection_child:
  File "/usr/local/lib/python3.9/configparser.py", line 792, in get
    raise NoOptionError(option, section)
configparser.NoOptionError: No option 'connection_child' in section: 'vti_72691b462a734a82963ade379b5fb4b5'

@AdSchellevis
Copy link
Member

@joni1993 ah, got it, missed that part in your initial comment, I'll take a look

AdSchellevis added a commit that referenced this issue Apr 24, 2024
@AdSchellevis
Copy link
Member

@joni1993 can you try b0bf317 ?

@joni1993
Copy link

joni1993 commented Apr 24, 2024

Yes - this works and brings up the tunnel. Interestingly after disconnecting the tunnel and connecting again after tearing down and bringing up the VTI, the static routes for the gateway did not get applied.

fichtner pushed a commit that referenced this issue Apr 24, 2024
…down_event.py as get() doesn't have a default. (#6781 (comment))

(cherry picked from commit b0bf317)
@AdSchellevis
Copy link
Member

the event shouldn't try to enforce routing, but if we only change the outer addressing, the inner shouldn't change anyway

@joni1993
Copy link

I agree. Somehow the Gateway Interface magically changed to some arbitrary interface...

Anyway I'd call this case closed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality roadmap Major roadmap item
Development

No branches or pull requests

8 participants