[Swan-dev] xauth delay

[Paul Wouters]

On Thu, 5 Feb 2015, D. Hugh Redelmeier wrote:

> commit 928b0e03bd33d4e5f878a70ab21475d8f107970d
>
> This adds a delay for an xauth password request.
>
> I don't actually understand the logic.  What are the specifications?
> I heard that it was something to do with working around an iOS bug.

the xauth exchange is kinda weird. both ends can initiate it. It is
followed by a modecfg exchange. both ends can initiate it. In practise,
I guess everyone follows cisco implementations (there is no RFC) so the
server asks for xauth data and the server asks for modecfg to start.

This causes the server to need to send two packets. One is a
confirmation of XAUTH and the next is the start of modecfg. Before this
patch, these packets were send out immediately. This causes these two
packets to sometimes get re-ordered, especially on 3G networks. The
phone (racoon) client has a bug. If it receives a modecfg start request
before a xauth confirmation request, it aborts the whole exchange and
fails the tunnel.

So the idea of the match is to have a small delay after sending the
xauth confirmation before sending the modecfg packet to have a much
better chance at these packets not getting reordered.

> It doesn't seem as if delete_state deletes any pending event in
> st->send_xauth_event.  This is likely a bug.

Yes. the original patch I wrote used our "standard" event, but that
clobbered over our existing event for regular "timeout" of the entire
exchange. So Antony used a new (parallel) event time to solve that. I
guess this was an oversight that should be fixed. Either by going back
to the standard event and chaining it with a standard event afterwards
or by adding support to delete_state().

> There is a convention that all names of fields in struct state start with
> st_. send_xauth_event does not.

I didn't know that. I thought only very "generic" names got the prefix.
Looking through the struct I see you are right and can point the finger
to be for a few of those violations. I will fix those up once we have
less branches in flight.

> The EVENT_v1_SEND_XAUTH handler doesn't seem to care about the result
> returned by xauth_send_request.

That could probably use some improvement. We might not properly try to
retransmit that packet. We have all the information to build the packet,
so any failure there should be a passert/STF_INTERNAL_ERROR

> I don't know what the response is supposed to be or how it is supposed to
> be handled (I haven't dived into xauth and hope to avoid that).

There will be an incoming packet we process through the state machine.

> How this mechanism fits into the retry logic isn't clear to me.  The retry
> time is less than this delay so that suggests that a retry might come out
> before the first xauth_send_request.  Is this valid?

That might be a new artifact of the libevent branch.

> Do we know that the original state is prepared to send the xauth after the
> delay?  How do we know that some other transition hasn't happened?

It should be fast enough that we couldn't possibly receive a response
packet to our last packet send. If they ask us for modecfg we would
ignore/fail it anyway - i dont think we even properly support that case
as no one in the wild does it that way)

Any other transition I can think of would be deleting, which we already
discussed.

Paul