On Fri, 28 Sep 2018 at 19:02, D. Hugh Redelmeier <[email protected]> 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 _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
