ikev1: Remove outbound policy of rekeyed CHILD_SA

[leonshaw]

Remove outbound policy of rekeyed CHILD_SA since only one policy is
valid. Otherwise, during update-SA job (when NAT mapping changed),
CHILD_SA are updated and installed one by one, leaving a window where
old SAs are being used. There are also circumstances where the new SA is
not processed last.

[tobiasbrunner]

Thanks. Probably makes sense to remove the old outbound SA. We really only keep a rekeyed CHILD_SA around to process inbound packets in case the other peer decides to continue to use it until it expires.

There are also circumstances where the new SA is not processed last.

What does that mean exactly? And what are these circumstances?

Also, why are you still using IKEv1?

[leonshaw]

What does that mean exactly? And what are these circumstances?

We've experienced one-way down session after reauth/rekey and remote IP change. After piping ip xfrm monitor policy to logs, I see the policy of the new CHILD_SA overwritten by the old one. Looks like the order of CHILD_SA went wrong. Haven't figured out the root cause yet, but removing the old outbound policy seems to solve my issue.
The logs:

May  7 10:00:38 SHAR1 charon: 05[KNL] NAT mappings of CHILD_SA ESP/0xc512d4a4/x.x.x.x changed to y.y.y.y[22235], queuing update job
May  7 10:00:38 SHAR1 charon: 05[IKE] remote endpoint changed from z.z.z.z[15781] to y.y.y.y[22235]
May  7 10:00:38 SHAR1 root: Updated src 0.0.0.0/0 dst 100.64.0.104/32
May  7 10:00:38 SHAR1 root:     dir out action block priority 383615
May  7 10:00:38 SHAR1 root: Updated src 100.64.0.104/32 dst 0.0.0.0/0
May  7 10:00:38 SHAR1 root:     dir in priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src y.y.y.y dst x.x.x.x
May  7 10:00:38 SHAR1 root:         proto esp reqid 4 mode tunnel
May  7 10:00:38 SHAR1 root: Updated src 100.64.0.104/32 dst 0.0.0.0/0
May  7 10:00:38 SHAR1 root:     dir fwd priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src y.y.y.y dst x.x.x.x
May  7 10:00:38 SHAR1 root:         proto esp reqid 4 mode tunnel
May  7 10:00:38 SHAR1 root: Updated src 0.0.0.0/0 dst 100.64.0.104/32
May  7 10:00:38 SHAR1 root:     dir out priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src x.x.x.x dst y.y.y.y
May  7 10:00:38 SHAR1 root:         proto esp spi 0x02b582de reqid 4 mode tunnel        <=== the new SPI
May  7 10:00:38 SHAR1 root: Updated src 0.0.0.0/0 dst 100.64.0.104/32
May  7 10:00:38 SHAR1 root:     dir out action block priority 383615
May  7 10:00:38 SHAR1 root: Updated src 100.64.0.104/32 dst 0.0.0.0/0
May  7 10:00:38 SHAR1 root:     dir in priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src y.y.y.y dst x.x.x.x
May  7 10:00:38 SHAR1 root:         proto esp reqid 4 mode tunnel
May  7 10:00:38 SHAR1 root: Updated src 100.64.0.104/32 dst 0.0.0.0/0
May  7 10:00:38 SHAR1 root:     dir fwd priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src y.y.y.y dst x.x.x.x
May  7 10:00:38 SHAR1 root:         proto esp reqid 4 mode tunnel
May  7 10:00:38 SHAR1 root: Updated src 0.0.0.0/0 dst 100.64.0.104/32
May  7 10:00:38 SHAR1 root:     dir out priority 383615
May  7 10:00:38 SHAR1 root:     tmpl src x.x.x.x dst y.y.y.y
May  7 10:00:38 SHAR1 root:         proto esp spi 0x0444d365 reqid 4 mode tunnel        <=== the old one

Also, why are you still using IKEv1?

We want to support connecting by username/password from OS built-in VPN clients, without configuring certificates.

[tobiasbrunner]

I see the policy of the new CHILD_SA overwritten by the old one. Looks like the order of CHILD_SA went wrong.

Interesting. Looks like that's caused by this:

strongswan/src/libcharon/processing/jobs/adopt_children_job.c

Lines 215 to 219 in 7022fdc

	while (children->remove_last(children,
	(void**)&child_sa) == SUCCESS)
	{
	ike_sa->add_child_sa(ike_sa, child_sa);
	}

That should be remove_first() instead, so the older SAs are added first to the new IKE_SA. I've pushed a fix to the 1041-ikev1-remove-rekeyed branch, which also contains your change.

We want to support connecting by username/password from OS built-in VPN clients, without configuring certificates.

You mean with PSK? Please don't do that for roadwarrior connections. If the server certificate is issued by a trusted third-party CA (e.g. Let's Encrypt), you won't need to configure/install any certificates on the clients. And even if not, this shouldn't be a reason to continue to use a severely inferior protocol (especially for mobile clients, see e.g. MOBIKE).

[leonshaw]

That should be remove_first() instead, so the older SAs are added first to the new IKE_SA. I've pushed a fix to the 1041-ikev1-remove-rekeyed branch, which also contains your change.

Great.

You mean with PSK? Please [don't do that for roadwarrior connections]

Yes, PSK. We will consider getting a cert from trusted CA. Thanks!