[Swan-dev] code smell

[Andrew Cagney]

The two functions have a basic but subtle difference - everything is
reversed.  For instance:
- on the way out its encryption then prf; on the way in it is prf then
encryption
- on the way out it's aead(true); on the way in it's aead(false) (and
at some point that was wrong)
- on the way out SA initiator selects *i* keys; but on the way in the
SA initiator selects *r* keys
So, looking at the last point, having the keys grouped into a sub
structure of struct state would be a nice tweak (going by the names,
can you tell which field I added :-), but I'd be wary of trying to
merge the functions as a whole.

The more interesting issue is that the intermediate exchange code
doesn't follow this model.  Both the incoming and outgoing code paths
put that code last.  This might help explain <<??? do these need to be
copies?>> (but answering this means means reading several RFCs ...).
And I'm curious as to what happens when, in encrypt_v2SK*(),
intermediate auth is combined with a non-aead algorithm?

On Tue, 13 Oct 2020 at 01:26, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> (This isn't a term that rolls off my tongue, but it is a term of art.)
>
> There is a big chunk of code that is almost identical in
> encrypt_v2SK_payload and ikev2_verify_and_decrypt_sk_payload.
>
> It would be good if this code be shared (likely in a function).
>
> This would mean that whenever a change is needed, it will only be needed
> one place.  For example, I've just fixed a bug that appeared in each.  If
> they used common code, this would have been just one bug.  This isn't the
> first pair of bugs that I fixed in this code.
>
> I don't have the time to find the extent of the duplication, but it
> includes at least the last IF in each function.
>
> The duplication itself isn't a bug but it is something to be avoided.
> That's what the term "smell" so crudely suggests.
>
> Where something "smells" in a piece of code, there is often more than one
> thing wrong.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev