[Swan-dev] iphone ios8 gets xauth request before isakmp is established

[Paul Wouters]

On Tue, 16 Dec 2014, Wolfgang Nothdurft wrote:

ExecSum: try attached patch?

>>> So I think the problem can also be solved, if the retransmit for the
>>> xauth password request were reduced to 10 or 20 seconds.
>>> 
>>> snippet from ikev1_xauth.c (xauth_send_request):
>>> 
>>> 795         event_schedule(EVENT_v1_RETRANSMIT,
>>> EVENT_RETRANSMIT_DELAY_0 * 3,
>>> 796                    st);
>> 
>> That looks much better compared to adding a sleep() :)
>> 
>> I've reduced this specific timeout to 1* EVENT_RETRANSMIT_DELAY_0 to
>> work around the issue now. In general, the retransmit/timeout behaviour
>> of libreswan will see some modernization in the next two months which
>> will also reduce all timeouts to modern day timings.
>
> The IPhone works with this change, but the customer criticized that it now 
> take ages (15 seconds) to connect. ;)

So I think the problem might relate to:

In complete_v1_state_transition():

[...]
                /* if requested, send the new reply packet */
                 if (smc->flags & SMF_REPLY) {
 		[...]
 		send_ike_msg(st, enum_name(&state_names, from_state));
 		}
[...]
                /* Special case for XAUTH server */
                 if (st->st_connection->spd.this.xauth_server) {
                         if (st->st_oakley.doing_xauth &&
                             IS_ISAKMP_SA_ESTABLISHED(st->st_state)) {
                                 libreswan_log(
                                         "XAUTH: Sending XAUTH Login/Password Request");
                                 xauth_send_request(st);
                                 break;
                         }
                 }

 		{
 		// note in email: similar checks for mode_cfg
 		modecfg_send_request(st);
 		}


I guess this does not really follow our state machine model very well.

The xauth_send_request() and modecfg_send_request() should really
cause a different state change and get scheduled properly. Note that
these functions already build a packet from scratch.

It is a bit odd that one incoming packet results in us sending two (or
three?) packets back. And we surely rapid fire these two packets in the
iphone case right after each other.

Possibly, xauth_send_request() should really be a new event scheduled,
eg:	event_schedule(EVENT_v1_SEND_XAUTH, 1 /* second delay */. st);

Attached a completely untested patch, let me know how that works out? :)
(it also does not really follow our state model properly)

Paul


> Testing an Android shows the same behaviour, unfortunately doesn't work with 
> this fix. (see attached log).
>
> I think a small delay for the first xauth request is needed anyway.
>
> Wolfgang
>
-------------- next part --------------
diff --git a/include/pluto_constants.h b/include/pluto_constants.h
index e700dbf..a902157 100644
--- a/include/pluto_constants.h
+++ b/include/pluto_constants.h
@@ -109,6 +109,7 @@ enum event_type {
 
 	EVENT_SO_DISCARD,		/* v1/v2 discard unfinished state object */
 	EVENT_v1_RETRANSMIT,		/* v1 Retransmit IKE packet */
+	EVENT_v1_SEND_XAUTH,		/* v1 send xauth request */
 	EVENT_SA_REPLACE,		/* v1/v2 SA replacement event */
 	EVENT_SA_REPLACE_IF_USED,	/* v1 SA replacement event */
 	EVENT_SA_EXPIRE,		/* v1/v2 SA expiration event */
diff --git a/programs/pluto/ikev1.c b/programs/pluto/ikev1.c
index 32768b2..8e31520 100644
--- a/programs/pluto/ikev1.c
+++ b/programs/pluto/ikev1.c
@@ -2485,8 +2485,9 @@ void complete_v1_state_transition(struct msg_digest **mdp, stf_status result)
 			if (st->st_oakley.doing_xauth &&
 			    IS_ISAKMP_SA_ESTABLISHED(st->st_state)) {
 				libreswan_log(
-					"XAUTH: Sending XAUTH Login/Password Request");
-				xauth_send_request(st);
+					"XAUTH: Scheduling XAUTH Login/Password Request");
+				// xauth_send_request(st);
+				event_schedule(EVENT_v1_SEND_XAUTH, 1, st);
 				break;
 			}
 		}
diff --git a/programs/pluto/pluto_constants.c b/programs/pluto/pluto_constants.c
index 50ea2b1..bfa3269 100644
--- a/programs/pluto/pluto_constants.c
+++ b/programs/pluto/pluto_constants.c
@@ -84,6 +84,7 @@ static const char *const timer_event_name[] = {
 
 	"EVENT_SO_DISCARD",
 	"EVENT_v1_RETRANSMIT",
+	"EVENT_v1_SEND_XAUTH",
 	"EVENT_SA_REPLACE",
 	"EVENT_SA_REPLACE_IF_USED",
 	"EVENT_SA_EXPIRE",
diff --git a/programs/pluto/timer.c b/programs/pluto/timer.c
index c54ef04..7f43eae 100644
--- a/programs/pluto/timer.c
+++ b/programs/pluto/timer.c
@@ -598,6 +598,7 @@ void handle_next_timer_event(void)
 
 	case EVENT_v1_RETRANSMIT:
 	case EVENT_v2_RETRANSMIT:
+	case EVENT_v1_SEND_XAUTH:
 	case EVENT_SA_REPLACE:
 	case EVENT_SA_REPLACE_IF_USED:
 	case EVENT_v2_RESPONDER_TIMEOUT:
@@ -671,6 +672,10 @@ void handle_next_timer_event(void)
 		retransmit_v1_msg(st);
 		break;
 
+	case EVENT_v1_SEND_XAUTH:
+		xauth_send_request(st);
+		break;
+
 	case EVENT_v2_RETRANSMIT:
 		retransmit_v2_msg(st);
 		break;