On 11/30, Paul Wouters ? wrote:
> On Fri, 28 Nov 2014, Matt Rogers wrote:
>
> >>Matt wrote the problem below. I am still confused what exactly is
> >>happening and why we would need his patch for this. I would think
> >>that if we --down tunnelA we should notice the phase1 is still used
> >>by tunnelB and leave/move it around instead?
> >>
> >
> >The use of preferred_ike is really just to manually work around this cisco
> >quirk,
> >and it's kind of a corner case. What you described above may be a better
> >solution (it doesn't happen that way now) but in practice I don't know if
> >it would help avoid the cisco behavior like preferred_ike does.
>
> I don't think it is a corner case. It is a bug on our end. We have one
> parent that has two children and we delete one child. We shouldn't shoot
> the parent.
>
I've been hacking on this some and I believe I'm on the right path, but I'd like
to post what I have so far and get some input. Unfortunately ipsec states don't
keep track of their IKE state beyond their cloned-from state (which can go
away)
and newest_isakmp_sa (which won't get updated for the other host pair instances
and tends to be #0, see the grepped status below).
I have a "connswitch" test that I have not committed yet, that sets up the test
as:
...
004 "TUNNEL-A" #1: STATE_MAIN_I4: ISAKMP SA established {auth=RSA_SIG
cipher=aes_256 integ=sha group=MODP2048}
002 "TUNNEL-A" #2: initiating Quick Mode
RSASIG+ENCRYPT+TUNNEL+PFS+DONT_REKEY+UP+IKEV2_ALLOW+SAREF_TRACK+IKE_FRAG_ALLOW
117 "TUNNEL-A" #2: STATE_QUICK_I1: initiate
004 "TUNNEL-A" #2: STATE_QUICK_I2: sent QI2, IPsec SA established tunnel mode
{ESP=>0xESPESP <0xESPESP xfrm=AES_128-HMAC_SHA1 NATOA=none NATD=none
DPD=passive}
west #
ipsec auto --up TUNNEL-B
002 "TUNNEL-B" #3: initiating Quick Mode
RSASIG+ENCRYPT+TUNNEL+PFS+DONT_REKEY+UP+IKEV2_ALLOW+SAREF_TRACK+IKE_FRAG_ALLOW
117 "TUNNEL-B" #3: STATE_QUICK_I1: initiate
004 "TUNNEL-B" #3: STATE_QUICK_I2: sent QI2, IPsec SA established tunnel mode
{ESP=>0xESPESP <0xESPESP xfrm=AES_128-HMAC_SHA1 NATOA=none NATD=none
DPD=passive}
west #
ipsec auto --up TUNNEL-C
002 "TUNNEL-C" #4: initiating Quick Mode
RSASIG+ENCRYPT+TUNNEL+PFS+DONT_REKEY+UP+IKEV2_ALLOW+SAREF_TRACK+IKE_FRAG_ALLOW
117 "TUNNEL-C" #4: STATE_QUICK_I1: initiate
004 "TUNNEL-C" #4: STATE_QUICK_I2: sent QI2, IPsec SA established tunnel mode
{ESP=>0xESPESP <0xESPESP xfrm=AES_128-HMAC_SHA1 NATOA=none NATD=none
DPD=passive}
west #
west #
ipsec auto --status | grep ISAKMP
000 "TUNNEL-A": newest ISAKMP SA: #1; newest IPsec SA: #2;
000 "TUNNEL-B": newest ISAKMP SA: #0; newest IPsec SA: #3;
000 "TUNNEL-C": newest ISAKMP SA: #0; newest IPsec SA: #4;
000 #1: "TUNNEL-A":500 STATE_MAIN_I4 (ISAKMP SA established); EVENT_SA_EXPIRE
in XXs; newest ISAKMP; lastdpd=-1s(seq in:0 out:0); idle; import:admin initiate
west #
sleep 60
(east rekeys)
west #
ipsec auto --status | grep ISAKMP
000 "TUNNEL-A": newest ISAKMP SA: #0; newest IPsec SA: #2;
000 "TUNNEL-B": newest ISAKMP SA: #5; newest IPsec SA: #3;
000 "TUNNEL-C": newest ISAKMP SA: #0; newest IPsec SA: #4;
000 #5: "TUNNEL-B":500 STATE_MAIN_R3 (sent MR3, ISAKMP SA established);
EVENT_SA_EXPIRE in XXs; newest ISAKMP; lastdpd=-1s(seq in:0 out:0); idle;
import:not set
west #
ipsec auto --down TUNNEL-B
002 "TUNNEL-B": terminating SAs using this connection
002 "TUNNEL-B": matt same_state_and_unused_ike
002 "TUNNEL-B": matt same_state_and_unused_ike
002 "TUNNEL-B" #3: deleting state #3 (STATE_QUICK_I2)
005 "TUNNEL-B" #3: ESP traffic information: in=0B out=0B
002 "TUNNEL-B": matt same_state_and_unused_ike
002 "TUNNEL-B": matt same_state_and_unused_ike
002 "TUNNEL-B": matt ike_used_by_other_conns:#5 compare with
other->newest_isakmp_sa 0 for TUNNEL-A
002 "TUNNEL-B": matt ike_used_by_other_conns: found a newest isakmp match
002 "TUNNEL-B": matt same_state_and_unused_ike
002 "TUNNEL-B": matt same_state_and_unused_ike
Afterwards, TUNNEL-B IKE sa remains:
000 #2: "TUNNEL-A":500 STATE_QUICK_I2 (sent QI2, IPsec SA established);
EVENT_SA_REPLACE_IF_USED in 27977s; newest IPSEC; eroute owner; isakmp#1; idle;
import:admin initiate
000 #2: "TUNNEL-A" esp.ESPSPIi@192.1.2.23 esp.ESPSPIi@192.1.2.45
tun.1000@192.1.2.23 tun.1001@192.1.2.45 ref=3 refhim=1 Traffic:!
ESPmax=4194303B
000 #5: "TUNNEL-B":500 STATE_MAIN_R3 (sent MR3, ISAKMP SA established);
EVENT_SA_EXPIRE in XXs; newest ISAKMP; lastdpd=-1s(seq in:0 out:0); idle;
import:not set
000 #4: "TUNNEL-C":500 STATE_QUICK_I2 (sent QI2, IPsec SA established);
EVENT_SA_REPLACE_IF_USED in 27758s; newest IPSEC; eroute owner; isakmp#1; idle;
import:admin initiate
000 #4: "TUNNEL-C" esp.ESPSPIi@192.1.2.23 esp.ESPSPIi@192.1.2.45
tun.1004@192.1.2.23 tun.1005@192.1.2.45 ref=11 refhim=9 Traffic:!
ESPmax=4194303B
west #
Although there's a crash after this in connection_compare() if I delete
TUNNEL-B
and view the status. I guess the connection has been freed but we still have
this
lingering TUNNEL-B state?
Also some of the function naming in state.c was really confusing me. There were
ones
that mentioned phase1 states but were not phase-specific, so I opted for more
realistic descriptions.
There was also a small bug of foreach_states_by_connection_func_delete() that
said
/* on pass 2, ignore phase2 states */
But what was really happening is that on pass 2, the IKE states were being
ignored. The intent from the function comment is that IKE states are ignored
until
the second pass so notifications can be sent out.
diff --git a/programs/pluto/ikev1_dpd.c b/programs/pluto/ikev1_dpd.c
index f6b5927..22eaf05 100644
--- a/programs/pluto/ikev1_dpd.c
+++ b/programs/pluto/ikev1_dpd.c
@@ -355,7 +355,7 @@ static void p2_dpd_outI1(struct state *p2st)
deltatime_t timeout = p2st->st_connection->dpd_timeout;
/* find the related Phase 1 state */
- st = find_phase1_state(p2st->st_connection,
+ st = find_latest_state_for_conn(p2st->st_connection,
ISAKMP_SA_ESTABLISHED_STATES);
if (st == NULL) {
diff --git a/programs/pluto/ikev1_main.c b/programs/pluto/ikev1_main.c
index 04ae331..a8274a9 100644
--- a/programs/pluto/ikev1_main.c
+++ b/programs/pluto/ikev1_main.c
@@ -2615,7 +2615,7 @@ void send_notification_from_state(struct state *st, enum
state_kind from_state,
from_state = st->st_state;
if (IS_QUICK(from_state)) {
- p1st = find_phase1_state(st->st_connection,
+ p1st = find_latest_state_for_conn(st->st_connection,
ISAKMP_SA_ESTABLISHED_STATES);
if ((p1st == NULL) ||
(!IS_ISAKMP_SA_ESTABLISHED(p1st->st_state))) {
@@ -2698,7 +2698,7 @@ bool ikev1_delete_out(struct state *st)
/* If there are IPsec SA's related to this state struct... */
if (IS_IPSEC_SA_ESTABLISHED(st->st_state)) {
/* Find their phase1 state object */
- p1st = find_phase1_state(st->st_connection,
+ p1st = find_latest_state_for_conn(st->st_connection,
ISAKMP_SA_ESTABLISHED_STATES);
if (p1st == NULL) {
DBG(DBG_CONTROL,
diff --git a/programs/pluto/initiate.c b/programs/pluto/initiate.c
index 8c99513..342d79b 100644
--- a/programs/pluto/initiate.c
+++ b/programs/pluto/initiate.c
@@ -1498,7 +1498,7 @@ void connection_check_phase2(void)
ipstr(&c->spd.that.host_addr, &b),
c->name);
- p1st = find_phase1_state(c,
+ p1st = find_latest_state_for_conn(c,
ISAKMP_SA_ESTABLISHED_STATES |
PHASE1_INITIATOR_STATES);
diff --git a/programs/pluto/ipsec_doi.c b/programs/pluto/ipsec_doi.c
index 19e74e4..45d0665 100644
--- a/programs/pluto/ipsec_doi.c
+++ b/programs/pluto/ipsec_doi.c
@@ -239,7 +239,7 @@ void ipsecdoi_initiate(int whack_sock,
* other issues around intent might matter).
* Note: there is no way to initiate with a Road Warrior.
*/
- struct state *st = find_phase1_state(c,
+ struct state *st = find_latest_state_for_conn(c,
ISAKMP_SA_ESTABLISHED_STATES |
PHASE1_INITIATOR_STATES);
diff --git a/programs/pluto/state.c b/programs/pluto/state.c
index 9fd7ab6..3740f2e 100644
--- a/programs/pluto/state.c
+++ b/programs/pluto/state.c
@@ -612,8 +612,8 @@ static void foreach_states_by_connection_func_delete(struct
connection *c,
st = st->st_hashchain_next; /* before this is deleted */
- /* on pass 2, ignore phase2 states */
- if (pass == 1 &&
+ /* on pass 1, ignore IKE states */
+ if (pass == 0 &&
IS_ISAKMP_SA_ESTABLISHED(this->st_state))
continue;
@@ -649,12 +649,11 @@ static void
foreach_states_by_connection_func_delete(struct connection *c,
}
/*
- * delete all states that were created for a given connection.
- * if relations == TRUE, then also delete states that share
- * the same phase 1 SA.
- */
static bool same_phase1_sa_relations(struct state *this,
struct connection *c)
+*/
+static bool same_state_connection_or_parent(struct state *this,
+ struct connection *c)
{
so_serial_t parent_sa = c->newest_isakmp_sa;
@@ -691,16 +690,65 @@ void delete_states_dead_interfaces(void)
}
/*
- * delete all states that were created for a given connection.
- * if relations == TRUE, then also delete states that share
- * the same phase 1 SA.
- */
+ * Was called the confusing:
static bool same_phase1_sa(struct state *this,
struct connection *c)
+*/
+static bool same_state_connection(struct state *this,
+ struct connection *c)
{
return this->st_connection == c;
}
+static bool ike_used_by_other_conns(struct state *this)
+{
+ struct connection *other = this->st_connection->hp_next;
+
+ if (!IS_ISAKMP_SA_ESTABLISHED(this->st_state))
+ return FALSE;
+
+ while (other != NULL) {
+ libreswan_log("matt ike_used_by_other_conns:#%d compare with
other->newest_isakmp_sa %d for %s",
+ this->st_serialno, other->newest_isakmp_sa, other->name);
+
+ if (other->newest_isakmp_sa == 0) {
+ struct state *st = find_latest_state_for_conn(other,
+ ISAKMP_SA_ESTABLISHED_STATES);
+ if (st != NULL &&
+ st->st_serialno == this->st_serialno) {
+ libreswan_log("matt ike_used_by_other_conns: found a newest
isakmp match");
+ return TRUE;
+ }
+
+ } else if (other->newest_isakmp_sa == this->st_serialno)
+ return TRUE;
+
+ other = other->hp_next;
+ }
+
+ return FALSE;
+}
+
+static bool same_state_and_unused_ike(struct state *this,
+ struct connection *c)
+{
+ libreswan_log("matt same_state_and_unused_ike");
+ if (ike_used_by_other_conns(this))
+ return FALSE;
+
+ return same_state_connection(this, c);
+}
+
+static bool same_state_parent_and_unused_ike(struct state *this,
+ struct connection *c)
+{
+ libreswan_log("matt same_state_parent_and_unused_ike");
+ if (ike_used_by_other_conns(this))
+ return FALSE;
+
+ return same_state_connection_or_parent(this, c);
+}
+
void delete_states_by_connection(struct connection *c, bool relations)
{
enum connection_kind ck = c->kind;
@@ -713,7 +761,8 @@ void delete_states_by_connection(struct connection *c, bool
relations)
c->kind = CK_GOING_AWAY;
foreach_states_by_connection_func_delete(c,
- relations ? same_phase1_sa_relations : same_phase1_sa);
+ relations ? same_state_parent_and_unused_ike :
+ same_state_and_unused_ike);
/*
* Seems to dump here because 1 of the states is NULL. Removing the Assert
@@ -737,6 +786,15 @@ void delete_states_by_connection(struct connection *c,
bool relations)
}
}
+static bool same_state_parent_no_ike(struct state *this,
+ struct connection *c)
+{
+ if (IS_ISAKMP_SA_ESTABLISHED(this->st_state))
+ return FALSE;
+ else
+ return same_state_connection_or_parent(this, c);
+}
+
/*
* delete_p2states_by_connection - deletes only the phase 2 of conn
*
@@ -745,15 +803,6 @@ void delete_states_by_connection(struct connection *c,
bool relations)
* This is like delete_states_by_connection with relations=TRUE,
* but it only deletes phase 2 states.
*/
-static bool same_phase1_no_phase2(struct state *this,
- struct connection *c)
-{
- if (IS_ISAKMP_SA_ESTABLISHED(this->st_state))
- return FALSE;
- else
- return same_phase1_sa_relations(this, c);
-}
-
void delete_p2states_by_connection(struct connection *c)
{
enum connection_kind ck = c->kind;
@@ -764,7 +813,7 @@ void delete_p2states_by_connection(struct connection *c)
if (ck == CK_INSTANCE)
c->kind = CK_GOING_AWAY;
- foreach_states_by_connection_func_delete(c, same_phase1_no_phase2);
+ foreach_states_by_connection_func_delete(c, same_state_parent_no_ike);
if (ck == CK_INSTANCE) {
c->kind = ck;
@@ -1242,9 +1291,13 @@ struct state *find_phase2_state_to_delete(const struct
state *p1st,
return NULL;
}
-/* Find newest Phase 1 negotiation state object for suitable for connection c
+/* For c, find the most recent state object who's state is any
+ * one of the ok_states set, or NULL if none are found.
+ *
+ * May not necessarily be c == st->st_connection
*/
-struct state *find_phase1_state(const struct connection *c, lset_t ok_states)
+struct state *find_latest_state_for_conn(const struct connection *c,
+ lset_t ok_states)
{
struct state
*st,
diff --git a/programs/pluto/state.h b/programs/pluto/state.h
index 3d01a88..57553d1 100644
--- a/programs/pluto/state.h
+++ b/programs/pluto/state.h
@@ -451,7 +451,8 @@ extern struct state
*state_with_serialno(so_serial_t sn),
*find_phase2_state_to_delete(const struct state *p1st, u_int8_t protoid,
ipsec_spi_t spi, bool *bogus),
- *find_phase1_state(const struct connection *c, lset_t ok_states),
+ *find_latest_state_for_conn(const struct connection *c,
+ lset_t ok_states),
*find_sender(size_t packet_len, u_char * packet);
#ifdef HAVE_LABELED_IPSEC
Thanks,
Matt
_______________________________________________
Swan-dev mailing list
Swan-dev@lists.libreswan.org
https://lists.libreswan.org/mailman/listinfo/swan-dev
