Bug 10733

Summary: Allow setting IPSec parameter "Auto"
Product: IPFire Reporter: Tom Rymes <tomvend>
Component: ---Assignee: Michael Tremer <michael.tremer>
Status: CLOSED FIXED QA Contact:
Severity: Balancing    
Priority: - Unknown - CC: peter.mueller
Version: 2   
Hardware: all   
OS: Linux   
Bug Depends on:    
Bug Blocks: 11618    
Attachments: Modified vpnmain.cgi (allows selection of auto parameter)
Proposed patch
Fixed Proposed Patch
Second Fixed Proposed Patch

Description Tom Rymes 2015-01-28 15:13:57 UTC
We are having issues with IPSec tunnels that drop needing to be reset periodically. According to this link, It may be more appropriate to set 'auto=route' instead of the current 'auto=start': 

https://wiki.strongswan.org/issues/825

However, I cannot change this setting without manually editing the configuration, and that change will be overwritten if I make changes in the GUI. Would it be possible to add this setting to the "advanced settings" page for each IPSec connection? It may even make sense to change the default setting for all new tunnels.
Comment 1 Tom Rymes 2015-01-28 18:20:05 UTC
A quick note to mention that initial testing indicates that, when auto=route, tunnels that have dropped will be properly reconnected when traffic bound for that tunnel is sent out. This is a major improvement for us, manifested as a perceived SIGNIFICANT improvement in IPSec reliability.
Comment 2 Tom Rymes 2015-08-03 02:28:36 UTC
To add more fuel to this fire, I have the following recommendation from Noel Kuntze on the StrongSwan mailing list regarding the use of auto= and dpdaction:

"Use auto=route and dpdaction=clear between sites with static IPs.
For connection between sites with mixed static and dynamic IPs,
use auto=add and dpdaction=clear on the side with the static IP
and auto=route and dpdaction=restart, or auto=route and dpdaction=clear
on the side with the dynamic IP."

https://lists.strongswan.org/pipermail/users/2015-July/008552.html

This brings two things to mind:

1.) The current WUI does not allow me to do this.
2.) The current default setting is not recommended. 

Tom
Comment 3 Tom Rymes 2015-10-01 01:09:10 UTC
Michael: You and I spoke about this briefly today. Just a reminder that this would be handy. Perhaps a descriptive name for each setting would help avoid scaring people off with additional IPSec settings:

Tunnel boot options:
- Start tunnel immediately (auto=start).
- Wait for remote system to initiate tunnel(auto=add).
- Wait for traffic to start tunnel (auto=route).

I would suggest that the "auto=" part of the description be left in so more advanced users aren't required to guess what the WUI is doing behind the scenes.
Comment 4 Michael Tremer 2015-12-10 18:31:26 UTC
Tom, do we actually need this any more?
Comment 5 Tom Rymes 2015-12-10 19:29:33 UTC
I think so. Based on the recommendations I received from Noel, using route or add is often preferred, and I think it helps avoid multiple Child SAs when both sides try to bring the tunnel back up at the same time.

Another link to the discussion on the Strongswan Users list: http://comments.gmane.org/gmane.network.vpn.strongswan.user/10315
Comment 6 Tom Rymes 2015-12-10 19:43:42 UTC
Also, as I mentioned above, the reasons I think that this should be implemented (for net to net, at least) are:

1.) The current WUI does not allow me to use recommended settings.
2.) The current default setting is not recommended.

Neither of these are a Good Thing™.

The main downside I can envision is that an added connection will not show as connected until you route some traffic over it and you change the setting from something other than "start". FWIW, I think that auto=route is generally how Cisco devices handle tunnels, if memory serves.
Comment 7 Michael Tremer 2015-12-12 14:15:06 UTC
You are right about Cisco.

I am willing to implement this then. But probably not this year any more. Please remind me next year if you don't hear anything from me.
Comment 8 Tom Rymes 2015-12-30 21:16:56 UTC
I poked about a bit in the vpnmain.cgi file, and it seems to me that the following is needed to move this forward:

1.) Adding a drop-down menu to the "advanced" page. I think this will work, but could use proper translation/i18n formatting. This is near line 2581:

<tr>
  <td width="15%">Auto:</td>
  <td>
    <select name='AUTO_EQUALS'>
     <option value='ignore' $selected{'AUTO_EQUALS'}{'ignore'}>ignore</option>
     <option value='add' $selected{'AUTO_EQUALS'}{'add'}>add</option>
     <option value='route' $selected{'AUTO_EQUALS'}{'route'}>route</option>
     <option value='start' $selected{'AUTO_EQUALS'}{'start'}>start</option>
    </select>
  </td>
</tr>

2.) Adding a line similar to this in the block near line 1949:

<input type='hidden' name='AUTO_EQUALS' value='$cgiparams{'AUTO_EQUALS'}' />

3.) Adding confighash and cgiparams lines similar to these ones used for DPD at lines 1271, 1806, 2255, and 2282 (this is well beyond my skills):

  $cgiparams{'DPD_ACTION'}                = $confighash{$cgiparams{'KEY'}}[27];
  $confighash{$key}[27] = $cgiparams{'DPD_ACTION'};

4.) Adding a block similar to this in the vicinity of line 2384:

$selected{'AUTO_EQUALS'}{'ignore'} = '';
    $selected{'AUTO_EQUALS'}{'add'} = '';
    $selected{'AUTO_EQUALS'}{'route'} = '';
    $selected{'AUTO_EQUALS'}{'start'} = '';
    $selected{'AUTO_EQUALS'}{$cgiparams{'AUTO_EQUALS'}} = "selected='selected'";

I have no idea if this is helpful to anyone, but I thought I would try to pitch in in whatever small way I can. Please let me know if there is a better way to present this. I will endeavor to implement what I can and test over the holiday, but any pointers to help me out would be appreciated.
Comment 9 Tom Rymes 2015-12-30 21:30:43 UTC
I forgot one part: 

This section:

# Automatically start only if a net-to-net connection
      if ($lconfighash{$key}[3] eq 'host') {
           print CONF "\tauto=add\n";
           print CONF "\trightsourceip=$lvpnsettings{'RW_NET'}\n";
      } else {
           print CONF "\tauto=route\n";
      }

Needs to be changed to this:

# Automatically start only if a net-to-net connection
      if ($lconfighash{$key}[3] eq 'host') {
           print CONF "\tauto=add\n";
           print CONF "\trightsourceip=$lvpnsettings{'RW_NET'}\n";
      } else {
           my $autoequals = $lconfighash{$key}[33];
           print CONF "\tauto=$autoequals\n";
      }

Also, I am guessing that these lines would work for #3, above (MOBIKE is the most recently added feature, and it is #32, so I used #33)?

  $cgiparams{'AUTO_EQUALS'}                = $confighash{$cgiparams{'KEY'}}[33];
  $confighash{$key}[33] = $cgiparams{'AUTO_EQUALS'};
Comment 10 Tom Rymes 2015-12-30 23:15:52 UTC
OK, so my apologies for "thinking out loud" on the comments here, but I think it was helpful. I have mocked up a version of vpnmain.cgi that included the needed changes, as far as I can see. The one thing I noticed that was a problem was that existing tunnels have their configuration written out with no value for "auto=". If you edit the existing tunnel, its configuration will be properly updated. I thought that the entry on line 111 assigning a default value of 'start' to AUTO_EQUALS would avoid this problem, but apparently not.

I am also utterly unaware of how these changes will affect a RoadWarrior tunnel, as I have never used one. I'm pretty sure that the config file will be unchanged, but it may be that the new control is visible on the Advanced page for a host-to-net tunnel. If that is the case, no matter what you select using the drop-down, the config will be written out with "auto-add".

Hopefully this saves someone some work.
Comment 11 Tom Rymes 2015-12-30 23:16:43 UTC
Created attachment 391 [details]
Modified vpnmain.cgi (allows selection of auto parameter)
Comment 12 Tom Rymes 2015-12-30 23:21:25 UTC
Created attachment 392 [details]
Proposed patch

In case this is preferred, I created a patch using the command:

" diff -Naur /srv/web/ipfire/cgi-bin/vpnmain.cgi.orig /srv/web/ipfire/cgi-bin/vpnmain.cgi > autoequalspatch.txt"

If this is not the correct way to do this, please let me know.
Comment 13 Tom Rymes 2015-12-30 23:29:27 UTC
Created attachment 393 [details]
Fixed Proposed Patch

This is a fixed patch. I had accidentally created the original patch as compared to a Core 94 version of vpnmain.cgi by accident. Due to the many changes to vpnmain.cgi in Core 95, this was far from ideal.

I'm going to go home and stop polluting the bug tracker now.
Comment 14 Tom Rymes 2015-12-30 23:39:18 UTC
Created attachment 394 [details]
Second Fixed Proposed Patch

Last Update, I promise. First Patch was run against the Core 94 file, the second one was run against a partially modified file. This version is properly run against the stock Core 95 file. I do not know if any changes were made to vpnmain.cgi for Core 96.
Comment 15 Tom Rymes 2016-05-17 16:17:41 UTC
Are there any updates on this? My proposed patch needs a lot of polish (it defaults to "ignore" when you edit a tunnel originally set to some other setting, etc), but I think it gets most of the way there. I'd bring it all of the way, but I'm over my head when it comes to PHP. If anyone can lend a hand, I'd appreciate it.
Comment 16 Tom Rymes 2016-09-30 04:37:38 UTC
Still looking for an update on this. Using auto=route really does seem to improve stability for us, and a recent move to a new router meant we were running with the normal "auto=start", which meant that we were seeing severely reduced reliability and also multiple SAs for each connection only minutes after establishing a connection. I presume that this is due to both endpoints trying to establish a tunnel at the same time, but I cannot be certain. 

Either way, the current default is not recommended by the Strongswan team, and it really should be changed, especially as it results in a major reduction in stability for net-to-net tunnels. Allowing the user to choose his/her preferred setting on the advanced page would also be welcome. 

I know I made a mess of this comments thread above, and that my attempted patch is rank amateurism at its best, but anything that could be done to move this forward would be GREATLY appreciated, and I think a major improvement to the project!!!!
Comment 17 Tom Rymes 2017-02-14 18:53:41 UTC
Looking into this again, as I noticed today that the StrongSWAN team now considers auto=route to be the preferred setting for site-to-site tunnels for security reasons:

"It is strongly advised to use auto=route in site-to-site setups to make sure that the kernel tells strongSwan to establish a CHILD_SA when there's no SA for a security policy. It is advisable to take a look at the strongswan.conf setting charon.ignore_acquire_ts when doing this."

(from https://wiki.strongswan.org/projects/strongswan/wiki/SecurityRecommendations)
Comment 18 Michael Tremer 2017-02-15 11:19:09 UTC
Hey Tom,

thank you for keeping the ball rolling - although I was so slow :)

I took your patch and integrated it with a few modifications: http://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=dcb406cc675c42f9add4a41c8a1e07eea7c3ab08

I tried to give the options better names for those people who don't know about the auto= parameter. I think it would have been nice to default to auto=route but I decided to be consistent and set the default to "Always On", i.e. auto=start.

Regarding the security concerns: We have firewall rules in place which don't allow for those packets to be sent out the RED interface when they are not going through the VPN. We will keep those just to be sure.
Comment 19 Tom Rymes 2017-12-17 00:46:00 UTC
This is, indeed, still working well, and I'd say that this bug can be closed?

Having said that, I still think that auto=route ought to be the default for new tunnels (let's not go changing existing tunnels!), as it is preferred by the StrongSwan team and results in much better reliability.
Comment 20 Peter Müller 2018-04-30 20:20:00 UTC
AFAIK this has been fixed by now.