[Swan-dev] pluto: IKEv2: create functions for boilerplate for starting and ending SK/SKF payloads; Was: [Swan-commit] Changes to ref refs/heads/master

[Andrew Cagney]

On Fri, 28 Sep 2018 at 19:02, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> These previously existing functions are used four times in
> ikev2_send.c.  Why were they not used in ikev2_parent.c too?

Being conservative.  Come up with an interface, try it on new or
replaced code (here encrypted notifies), push it, and then wait for
the dust to settle. Shuffling the code to add *_send.[hc] was pretty
invasive it needed time to settle.  Since old code needs to be propped
up, the implementation is also often a little strange - just wrapping
an existing function for instance - I need to leave more bread crumbs
so what is going on is easier to understand.   Otherwise functions
exporting a new interface but implemented as wrappers to old code end
up being garbage collected (an example is send_recorded_v1_ike_msg() /
resend_recorded_v1_ike_message() - there was no bread crumb pointing
out how the wrapped function was unnecessarily complicated and can be
simplified with inlining).

Of course this just leaves two TODO items:
- tracking down test cases that, through logging, can be shown to
exercise a code path and then convert (sometimes I get to add logging,
see my wrapper to ikev2_crypto_continue(), sometimes I get to add test
cases, and that takes time).
- clean up the implementation

> The ikev2_send.c version looks a bit nicer.  They should replace the
> functions I wrote.
>
> It would be good if close_v2sk_payload could handle fragmenting (I
> said that about end_encrypted_payload in the commit).
>
> Current oddity: the payload size is padded before fragmentation and
> after.  I imagine that only after is correct.

Interesting.

> Another oddity: currently not all messages can be fragmented by our
> code.  If that were handled in close_v2sk_payload, we could fragment
> any encrypted packet.
>
> start_encrypted_payload and end_encrypted_payload support SK and SKF
> payloads.  It would be good if open_v2sk_payload and
> close_v2sk_payload could too.

I'm not sure.  Both of these suggest more research.

> If start_encrypted_payload and end_encrypted_payload are replaced,
> move_pbs_previous_np and ikev2_padup_pre_encrypt become unused and should
> be deleted.

Having to hack move_*() into the IKEv2 code really should have raised
a red flag - changing that interface was high risk as there was a good
chance that an edge case would be missed.

And I'm pretty sure that is what happened.  While testing move*()
et.al. you should have stumbled across the  ikev2_sehd.[hc] code.  I
suspect you didn't because it is currently only used to send a single
nested payload which doesn't trigger the problem (true?).

Anyway, this is all now academic - I've changes pending to remove the
need for move*().

Andrew