[Swan-dev] ikev1_close_message()

[D. Hugh Redelmeier]

(This message is mostly archaeology, and incomplete at that.)

This function has a very useful rationale in comments:

/*
 * In IKEv1, some implementations (including freeswan/openswan/libreswan)
 * interpreted the RFC that the whole IKE message must padded to a multiple
 * of 4 octets, but other implementations (i.e. Checkpoint in Aggressive Mode)
 * drop padded IKE packets. Some of the text on this topic can be found in the
 * IKEv1 RFC 2408 section 3.6 Transform Payload.
 *
 * The ikepad= option can be set to yes or no on a per-connection basis,
 * and defaults to yes.
 *
 * In IKEv2, there is no padding specified in the RFC and some implementations
 * will reject IKEv2 messages that are padded. As there are no known IKEv2
 * clients that REQUIRE padding, padding is never done for IKEv2. If IKEv2
 * clients are discovered in the wild, we will revisit this - please contact
 * the libreswan developers if you find such an implementation.
 * Therefore the ikepad= option has no effect on IKEv2 connections.
 *
 * @param pbs PB Stream
 */

But the code seems to also call this for some attributes (all to do
with xauth).  This seems quite screwy.  But is it correct code?  Does
the xauth RFC prescribe it?

(I should dig in the RFC but I'm running out of time tonight.  And
heaven knows that interop might trump the standard.)

If it is correct, the function name is misleading and the comments are
too.  This would need to be fixed.

If it is incorrect, we should eliminate those spurious calls.

At the start of our repo, we had these comments.  I suspect that I
wrote them.  This function certainly did not apply to attributes, only
whole ISAKMP messages.  (At the time it was called close_message and
was in programs/pluto/ipsec_doi.c).

 /* The whole message must be a multiple of 4 octets.
  * I'm not sure where this is spelled out, but look at
  * rfc2408 3.6 Transform Payload.
  * Note: it talks about 4 BYTE boundaries!
  */

I feel that these comments should be preserved as an explanation of
our thinking.

In ab763468da8d8f33d5c7ac3d7281f704d60aed7d David McCullough
seems to have made the padding controllable for interop with Checkpoint.

The padding of attributes was in our repo from the start:
241fba64a3 (Michael Richardson 2005-11-02 12:31:00 -0500  586)  close_message(&strattr);

If xauth attributes need to be padded, does that need to be
suppressed?  Should that suppression be tied to the suppression of
ISAKMP message padding?  Some how I doubt this.

I'd guess that if xauth attributes need to be padded, they should use
a different mechanism.