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

strongswan: swanctl init script doesn't load connections #15446

Closed
stintel opened this issue Apr 18, 2021 · 31 comments
Closed

strongswan: swanctl init script doesn't load connections #15446

stintel opened this issue Apr 18, 2021 · 31 comments
Assignees

Comments

@stintel
Copy link
Member

stintel commented Apr 18, 2021

Maintainer: @pprindeville @Thermi
Environment: OpenWrt master r16565-37958f0d115 on octeon (Ubiquiti EdgeRouter Lite)

Description:
The swanctl init script doesn't load connections defined in /etc/swanctl/conf.d/. The file /etc/swanctl/conf.d/ contains include conf.d/*.conf, and running swanctl --load-all manually works fine. The deprecated ipsec script is disabled.

# /etc/init.d/swanctl restart
# swanctl -L
#
# swanctl --load-all
loaded certificate from '/etc/swanctl/x509/ar1.crt'
loaded ECDSA key from '/etc/swanctl/private/ar1.key'
no authorities found, 0 unloaded
no pools found, 0 unloaded
loaded connection 'ar0'
successfully loaded 1 connections, 0 unloaded
# swanctl -L
vr0: IKEv2, no reauthentication, rekeying every 14400s
  local:  %any
  remote: [HIDDEN]
  local public key authentication:
    id: ar0
    certs:
  remote public key authentication:
    id: vr0
  vr0-1: TUNNEL, rekeying every 3600s
    local:  0.0.0.0/0 ::/0
    remote: 0.0.0.0/0 ::/0
@stintel
Copy link
Member Author

stintel commented Apr 18, 2021

A while ago when I was trying to implement swanctl support myself, I also had this issue. The solution I came up with back then was stintel@b489d87, but it's quite hackish.

@Thermi
Copy link
Contributor

Thermi commented Apr 19, 2021

Did you have a preexisting /etc/strongswan.conf file?

@stintel
Copy link
Member Author

stintel commented Apr 19, 2021

Did you have a preexisting /etc/strongswan.conf file?

# cat /etc/strongswan.conf
# strongswan.conf - strongSwan configuration file
#
# Refer to the strongswan.conf(5) manpage for details
#
# Configuration changes should be made in the included files

charon {
        load_modular = yes
        plugins {
                include strongswan.d/charon/*.conf
        }

        install_routes = no
}

include strongswan.d/*.conf

It is part of the strongswan package:

# opkg files strongswan
Package strongswan (5.9.2-5) is installed on root and has the following files:
/usr/lib/ipsec/libstrongswan.so.0
/usr/lib/ipsec/libstrongswan.so.0.0.0
/etc/strongswan.conf
/lib/upgrade/keep.d/strongswan

@pprindeville
Copy link
Member

Hmm...

root@OpenWrt2:~# grep '^include' /etc/swanctl/swanctl.conf 
include conf.d/*.conf
include /var/swanctl/swanctl.conf
root@OpenWrt2:~# 

I don't see anything in /etc/init.d/swanctl that requires UCI for it to launch.

@Thermi
Copy link
Contributor

Thermi commented Apr 19, 2021

Hmm...

root@OpenWrt2:~# grep '^include' /etc/swanctl/swanctl.conf 
include conf.d/*.conf
include /var/swanctl/swanctl.conf
root@OpenWrt2:~# 

I don't see anything in /etc/init.d/swanctl that requires UCI for it to launch.

The issue here is that /var/swanctl/strongswan.conf isn't included by default for some reason.

@stintel
Copy link
Member Author

stintel commented Apr 19, 2021

Why should /var/swanctl/strongswan.conf be included? My configs are in /etc/swanctl/conf.d/*.conf...

@Thermi
Copy link
Contributor

Thermi commented Apr 19, 2021

Because the generated config files are ephemeral and storing them in /var guarantees that they do not conflict with user defined files, packaged files, or are included in a tarball of the configuration (which would be redundant because they are a product of a script and the UCI config that is in in /etc).

The swanctl.init script generates a strongswan.conf file to set the options necessary for the correct functioning of the generated configs. It also generates a swanctl.conf file that contains the generated tunnel configurations.

On the test machine the inclusion worked fine. We now have to figure out why it doesn't work for you.

I propose just packaging a very short config file in swanctl/conf.d and strongswan.d that include the corresponding file in /var.

@stintel
Copy link
Member Author

stintel commented Apr 19, 2021

/var/swanctl/strongswan.conf doesn't exist on my system.
/var/swanctl/swanctl.conf does, but it's empty.

@Thermi
Copy link
Contributor

Thermi commented Apr 20, 2021

@stintel Please check for opkg-new files.

@stintel
Copy link
Member Author

stintel commented Apr 20, 2021

@stintel Please check for opkg-new files.

There aren't any related to strongSwan

# find / -type f -name '*-opkg*' 2> /dev/null
/etc/config/vallumd-opkg
/etc/keepalived/keepalived.conf-opkg
/overlay/upper/etc/config/vallumd-opkg
/overlay/upper/etc/keepalived/keepalived.conf-opkg
/rom/usr/lib/opkg/info/luci-app-opkg.control
/rom/usr/lib/opkg/info/luci-app-opkg.files-sha256sum
/rom/usr/lib/opkg/info/luci-app-opkg.list
/rom/usr/lib/opkg/info/luci-app-opkg.prerm
/rom/usr/share/luci/menu.d/luci-app-opkg.json
/rom/usr/share/rpcd/acl.d/luci-app-opkg.json
/usr/lib/opkg/info/luci-app-opkg.control
/usr/lib/opkg/info/luci-app-opkg.files-sha256sum
/usr/lib/opkg/info/luci-app-opkg.list
/usr/lib/opkg/info/luci-app-opkg.prerm
/usr/share/luci/menu.d/luci-app-opkg.json
/usr/share/rpcd/acl.d/luci-app-opkg.json

@Thermi
Copy link
Contributor

Thermi commented Apr 20, 2021

Weird.

With -5, the includes were just appended to the config files. See 643df01.

@pprindeville
Copy link
Member

What does the tail of your /etc/swanctl/swanctl.conf file look like? It should have:

...
# Include config snippets
include conf.d/*.conf

include /var/swanctl/swanctl.conf

@stintel
Copy link
Member Author

stintel commented Apr 20, 2021

# tail /etc/swanctl/swanctl.conf
        # cert_uri_base =

    # }

# }

# Include config snippets
include conf.d/*.conf

include /var/swanctl/swanctl.conf

Either way, not relevant as /var/swanctl/swanctl.conf is empty anyway.

@Thermi
Copy link
Contributor

Thermi commented Apr 20, 2021

It is relevant because that file will contain the config generated by the swanctl init script.

@stintel
Copy link
Member Author

stintel commented Apr 20, 2021

But it doesn't contain anything right now, so it's not relevant to this problem?

@Thermi
Copy link
Contributor

Thermi commented Apr 20, 2021

It is relevant as far as we can see that at the very least this file was installed correctly.

@stintel
Copy link
Member Author

stintel commented May 4, 2021

Any ideas? As you're deprecating the old ipsec script you're actively suggesting people to migrate from something that works to something that is broken...

@Thermi
Copy link
Contributor

Thermi commented May 5, 2021 via email

@stintel
Copy link
Member Author

stintel commented May 5, 2021

Forcing people to migrate to UCI is not acceptable. If the swanctl script does not load connections configured in /etc/swanctl/swanctl.conf (or files included from there), that is a bug. This combined with the fact that the ipsec init script now actively tells people it is deprecated will result in people losing access to devices in the field. This is never acceptable, and especially not in the current times with travel restrictions. I've stressed this multiple times when I was still maintaining the package.

The fact that strongSwan 6.0.0 will deprecate ipsec entirely doesn't change anything to this. While I agree that something needs to be done before strongSwan 6.0.0 arrives, the risk of people losing access to devices should be avoided at all cost.

@Thermi
Copy link
Contributor

Thermi commented May 5, 2021 via email

@stintel
Copy link
Member Author

stintel commented May 5, 2021

It does load these configs. That is why there is an include there.

No, it does not.

# tail -n4 /etc/swanctl/swanctl.conf
# Include config snippets
include conf.d/*.conf

include /var/swanctl/swanctl.conf

None of the connections in the files matched by the first include are loaded by the init script. I have to manually run swanctl --load-all and they appear. This confirms the configs on their own are fine, but the init script for some reason does not call swanctl --load-all.

@Thermi
Copy link
Contributor

Thermi commented May 5, 2021 via email

@stintel
Copy link
Member Author

stintel commented May 5, 2021

Just remove /etc/config/ipsec.

@Thermi
Copy link
Contributor

Thermi commented May 5, 2021

What do you mean with that?

@stintel
Copy link
Member Author

stintel commented May 5, 2021

I'm assuming loading the connections is handled by https://github.com/openwrt/packages/blob/master/net/strongswan/files/swanctl.init#L555-L557

This is part of the config_ipsec() function which is called here. When /etc/config/ipsec does not exist, the init script will not run config_ipsec at all so it will not append that start-script snippet to the included config file and swanctl --load-all will not be executed.

@stintel
Copy link
Member Author

stintel commented May 5, 2021

Additionally I was lacking include /var/ipsec/strongswan.conf in /etc/strongswan.conf. Why does it need to be appended to the file if the default already has include strongswan.d/*.conf.

Imo it would be better to just ship a file /etc/strongswan.d/start-scripts.conf or so in strongswan-swanctl that contains:

charon {
  start-scripts {
    load-all = /usr/sbin/swanctl --load-all --noprompt
  }
}

This would avoid appending in the Makefile, and reduce the appending in the init script, for a much cleaner solution imo.

@stintel
Copy link
Member Author

stintel commented May 5, 2021

I gave up maintainership of this package because I no longer have the energy to devote much time on this. I'm glad you took over that role, but please, do a better job. The swanctl init script was rushed, imo, and it shows.

@Thermi
Copy link
Contributor

Thermi commented May 5, 2021

Additionally I was lacking include /var/ipsec/strongswan.conf in /etc/strongswan.conf. Why does it need to be appended to the file if the default already has include strongswan.d/*.conf.

Imo it would be better to just ship a file /etc/strongswan.d/start-scripts.conf or so in strongswan-swanctl that contains:

charon {
  start-scripts {
    load-all = /usr/sbin/swanctl --load-all --noprompt
  }
}

This would avoid appending in the Makefile, and reduce the appending in the init script, for a much cleaner solution imo.

No, that'd change the behaviour of the daemon even if the service was not used and it would cause unnecessary opkg-new files to be created when the package is updated and the user edited them.
Also /etc is not for runtime generated files. That's what /var is for. That is why this is done the way it is done.
And it was not rushed. We took much time to deliberate on the details.

pprindeville added a commit to pprindeville/packages that referenced this issue May 5, 2021
Fixes issue openwrt#15446

Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>
pprindeville added a commit to pprindeville/packages that referenced this issue May 5, 2021
Fixes issue openwrt#15446

Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>
@pprindeville
Copy link
Member

I'm assuming loading the connections is handled by https://github.com/openwrt/packages/blob/master/net/strongswan/files/swanctl.init#L555-L557

This is part of the config_ipsec() function which is called here. When /etc/config/ipsec does not exist, the init script will not run config_ipsec at all so it will not append that start-script snippet to the included config file and swanctl --load-all will not be executed.

Please try out PR #15575.

pprindeville added a commit to pprindeville/packages that referenced this issue May 5, 2021
Fixes issue openwrt#15446

Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>
@stintel
Copy link
Member Author

stintel commented May 5, 2021

No, that'd change the behaviour of the daemon even if the service was not used and it would cause unnecessary opkg-new files to be created when the package is updated and the user edited them.
Also /etc is not for runtime generated files. That's what /var is for. That is why this is done the way it is done.
And it was not rushed. We took much time to deliberate on the details.

If you ship the file with the package it wouldn't be runtime generated. And you could either exclude the file from conffiles, or add a header that says the file is not to be edited. That would solve the -opkg file creation.

As for the changing behaviour of the daemon, I think an unconditional load-all is exactly what happens on systemd-based systems. I don't see why this would be a problem. If a user doesn't want the service he can disable it or not install it at all.

@pprindeville pprindeville self-assigned this May 5, 2021
@pprindeville
Copy link
Member

Resolved with PR #15575.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants