[strongSwan-dev] Removed SA/policies flush from starter

[Emeric POUPON]

Hello,

I noticed that you do no longer flush the policies in starter from 5.4.0 , see [1] and [2].

Actually, using the pfkey backend we can control which type of SA we can flush (see [3])
I was thinking about restoring this flush during libcharon initialization/deinitialization.
Is that the correct place to do this now ?

Regards,

Emeric


[1] https://github.com/strongswan/strongswan/commit/d8fdd1018e1654b04b614354a493026a9dad30e5
[2] https://github.com/strongswan/strongswan/commit/bd24f87d35f505a94814fd93b86816d69761527e
[3] https://github.com/strongswan/strongswan/commit/603e3b489bb8a448f0dbcad9406fbfb64523abe1

[Tobias Brunner]

Hi Emeric,

Post by Emeric POUPON
Actually, using the pfkey backend we can control which type of SA we can flush (see [3])

Yes, but not which policies [2]. If there was an external tool that
also managed policies starter flushed these policies (even if
installpolicies=no was configured). And charon should properly clean up
when terminating anyway so there should be no need to flush the kernel
state (if charon restarts after a crash it will now update and adopt
existing policies in the kernel, so that should not result in an error
anymore either). And in the rare cases where one does want to flush the
SAs and policies in the kernel setkey (or ip xfrm on Linux) may be used.

Post by Emeric POUPON
I was thinking about restoring this flush during libcharon initialization/deinitialization.

Why?

Regards,
Tobias

Post by Emeric POUPON
[1] https://github.com/strongswan/strongswan/commit/d8fdd1018e1654b04b614354a493026a9dad30e5
[2] https://github.com/strongswan/strongswan/commit/bd24f87d35f505a94814fd93b86816d69761527e
[3] https://github.com/strongswan/strongswan/commit/603e3b489bb8a448f0dbcad9406fbfb64523abe1

[Emeric POUPON]

Hello,

Thanks for your response

Post by Emeric POUPON
I was thinking about restoring this flush during libcharon
initialization/deinitialization.

Why?

Well, if the daemon crashes it restarts in a desynchronized state with the kernel, which is difficult to debug/monitor.
Examples:
- there is some IPsec traffic for quite a long time but charon has no IKE SA negotiated (no stat, no monitoring available using charon),
- if you perform a config reload just after a crash and the connection is no longer in the configuration, you will end up with the previous SA not flushed (we have a patch to remove the IKE SA/CHILD SA on deleted connections),
- if you perform an "ipsec stop" after a crash, nothing is done.

We could imagine an option (disabled by default) to enable this flush during startup?

[Tobias Brunner]

Post by Emeric POUPON
Well, if the daemon crashes it restarts in a desynchronized state with the kernel, which is difficult to debug/monitor.
- there is some IPsec traffic for quite a long time but charon has no IKE SA negotiated (no stat, no monitoring available using charon),
- if you perform a config reload just after a crash and the connection is no longer in the configuration, you will end up with the previous SA not flushed (we have a patch to remove the IKE SA/CHILD SA on deleted connections),
- if you perform an "ipsec stop" after a crash, nothing is done.

So you expect charon to crash regularly? My stance is that it should
basically never crash. And starter previously did actually not flush
the kernel state when it restarted the daemon after a crash (it only did
so when terminating). I don't really see a problem with 1, actually I
think it's better everything stays in place until an SA might get
replaced with a new one (avoids interruptions). 2 seems a bit
contrived. And 3 can just as easily be handled by calling a separate
command to flush the kernel state.

Post by Emeric POUPON
We could imagine an option (disabled by default) to enable this flush during startup?

Sure, if you think it's worthwhile. On FreeBSD you could actually use
charon.start-scripts to implement this:

charon {
start-scripts {
flush-policies = setkey -FP
flush-sas = setkey -F
}
}

This doesn't work on Linux as the scripts run after the plugins have
been initialized so `ip xfrm` would flush the bypass policies set on the
UDP sockets used by the daemon. On FreeBSD PF_KEY apparently can't
manage socket specific policies. One problem could be, though, that
starter might send the connections before the scripts are executed
(starter waits for the PID file to appear, which happens even before the
plugins are loaded), so there is a potential race if you use trap
policies. But you could switch to swanctl and run the command to load
the connections after the ones above.

Regards,
Tobias

[Emeric POUPON]

Post by Tobias Brunner

Post by Emeric POUPON
We could imagine an option (disabled by default) to enable this flush during startup?

Sure, if you think it's worthwhile. On FreeBSD you could actually use
charon {
start-scripts {
flush-policies = setkey -FP
flush-sas = setkey -F
}
}
This doesn't work on Linux as the scripts run after the plugins have
been initialized so `ip xfrm` would flush the bypass policies set on the
UDP sockets used by the daemon. On FreeBSD PF_KEY apparently can't
manage socket specific policies. One problem could be, though, that
starter might send the connections before the scripts are executed
(starter waits for the PID file to appear, which happens even before the
plugins are loaded), so there is a potential race if you use trap
policies. But you could switch to swanctl and run the command to load
the connections after the ones above.

Thanks for the suggestion: that does the job!
To make sure the race does not occur, I just switched the run_scripts before the engine start.
I created a 'sleep 10' job to test, charon does receive everything just after.

Wouldn't it be safer to commit this by the way?

Thanks again for your support,


Emeric


diff --git a/src/libcharon/daemon.c b/src/libcharon/daemon.c
index cef8b89..56d4d13 100644
--- a/src/libcharon/daemon.c
+++ b/src/libcharon/daemon.c
@@ -746,12 +746,12 @@ static void run_scripts(private_daemon_t *this, char *verb)
METHOD(daemon_t, start, void,
private_daemon_t *this)
{
+ run_scripts(this, "start");
+
/* start the engine, go multithreaded */
lib->processor->set_threads(lib->processor,
lib->settings->get_int(lib->settings, "%s.threads",
DEFAULT_THREADS, lib->ns));
-
- run_scripts(this, "start");
}

[Tobias Brunner]

Post by Emeric POUPON
Wouldn't it be safer to commit this by the way?

No, then we couldn't e.g. run swanctl commands to load connections, as
the vici plugin wouldn't be able process the requests.

Regards,
Tobias

[Emeric POUPON]

Post by Tobias Brunner

Post by Emeric POUPON
Wouldn't it be safer to commit this by the way?

No, then we couldn't e.g. run swanctl commands to load connections, as
the vici plugin wouldn't be able process the requests.

Then for the startup scripts I guess there is the same race using swanctl or starter? How can one be sure the scripts are completed before loading the connections?

By the way, why does starter manage to send the connections with my patch?
I thought the socket was created but no thread was actually accepting on it. Isn't it the same with the vici plugin?

Regards,
Emeric

[Tobias Brunner]

Post by Tobias Brunner
This doesn't work on Linux as the scripts run after the plugins have
been initialized so `ip xfrm` would flush the bypass policies set on the
UDP sockets used by the daemon.

I have to correct myself here. While socket-specific policies are
listed in `ip xfrm policy` they are not actually removed when flushing
all policies via XFRM. So as long as
charon.plugins.kernel-netlink.port_bypass is disabled (the default) this
works fine on Linux too (except for the same race with starter).

Post by Tobias Brunner

Post by Tobias Brunner

Post by Emeric POUPON
Wouldn't it be safer to commit this by the way?

No, then we couldn't e.g. run swanctl commands to load connections, as
the vici plugin wouldn't be able process the requests.

Then for the startup scripts I guess there is the same race using swanctl or starter?
How can one be sure the scripts are completed before loading the connections?

By loading the connections with a start script (e.g. `swanctl --load-all
--noprompt`) that runs after the two others (scripts are executed in the
order defined in the start-scripts section).

Or when using charon-systemd (not on FreeBSD obviously as it does not
use systemd) load the connections via ExecStartPost (as the
strongswan-swanctl service unit does). This command runs after the
daemon signals its readiness to systemd, which happens after the scripts
ran. Actually, using ExecStartPost is not even necessary as starting
the daemon with systemd will block until it is ready (and start scripts
were executed) so any swanctl command that's executed directly after
e.g. `systemctl start` will run after the start scripts.

Post by Tobias Brunner
By the way, why does starter manage to send the connections with my patch?
I thought the socket was created but no thread was actually accepting on it. Isn't it the same with the vici plugin?

Yes, exactly the same. With the patch the scripts will run and starter
will concurrently write connection data to the socket. But here it
doesn't matter if nobody is reading from the socket yet as the scripts
won't block. After running them the threads are started and the data is
read from the socket.
However, the start-scripts facility was mainly added to run swanctl
commands on systems where starter is not used (Windows, systemd). And
swanctl commands are synchronous, i.e. they block until they get a
reply. So the patch would create a deadlock if swanctl commands are run
as start scripts: swanctl would wait for a response, the daemon's main
thread would wait for swanctl to terminate and therefore couldn't
continue to start the other threads that would allow the vici plugin to
respond to the swanctl request.

Regards,
Tobias

[Emeric POUPON]

hello,

Thanks for these valuable explanations.
For now I will go for the scripts+patch solution. When we migrate from starter to swanctl, we will remove the patch.

Regards,

Emeric

----- Original Message -----

Sent: Friday, 1 July, 2016 14:51:57
Subject: Re: [strongSwan-dev] Removed SA/policies flush from starter

Post by Tobias Brunner
This doesn't work on Linux as the scripts run after the plugins have
been initialized so `ip xfrm` would flush the bypass policies set on the
UDP sockets used by the daemon.

I have to correct myself here. While socket-specific policies are
listed in `ip xfrm policy` they are not actually removed when flushing
all policies via XFRM. So as long as
charon.plugins.kernel-netlink.port_bypass is disabled (the default) this
works fine on Linux too (except for the same race with starter).

Post by Tobias Brunner

Post by Tobias Brunner

Post by Emeric POUPON
Wouldn't it be safer to commit this by the way?

No, then we couldn't e.g. run swanctl commands to load connections, as
the vici plugin wouldn't be able process the requests.

Then for the startup scripts I guess there is the same race using swanctl or starter?
How can one be sure the scripts are completed before loading the connections?

By loading the connections with a start script (e.g. `swanctl --load-all
--noprompt`) that runs after the two others (scripts are executed in the
order defined in the start-scripts section).
Or when using charon-systemd (not on FreeBSD obviously as it does not
use systemd) load the connections via ExecStartPost (as the
strongswan-swanctl service unit does). This command runs after the
daemon signals its readiness to systemd, which happens after the scripts
ran. Actually, using ExecStartPost is not even necessary as starting
the daemon with systemd will block until it is ready (and start scripts
were executed) so any swanctl command that's executed directly after
e.g. `systemctl start` will run after the start scripts.

Post by Tobias Brunner
By the way, why does starter manage to send the connections with my patch?
I thought the socket was created but no thread was actually accepting on it.
Isn't it the same with the vici plugin?

Yes, exactly the same. With the patch the scripts will run and starter
will concurrently write connection data to the socket. But here it
doesn't matter if nobody is reading from the socket yet as the scripts
won't block. After running them the threads are started and the data is
read from the socket.
However, the start-scripts facility was mainly added to run swanctl
commands on systems where starter is not used (Windows, systemd). And
swanctl commands are synchronous, i.e. they block until they get a
reply. So the patch would create a deadlock if swanctl commands are run
as start scripts: swanctl would wait for a response, the daemon's main
thread would wait for swanctl to terminate and therefore couldn't
continue to start the other threads that would allow the vici plugin to
respond to the swanctl request.
Regards,
Tobias