[Swan-dev] COOKIE_SIZE is IKEv1!

[Andrew Cagney]

On Mon, 16 Jul 2018 at 10:37, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> | - COOKIE_SIZE is IKEv1 so should not appear in IKEv2 code at all!
> | IKEv2 has cookies but they are completely different, having nothing to
> | do with this value.
>
> COOKIE_SIZE is the size of the fields in the header that hold v2 IKE
> SPIs.  This is by protocol design, not an accident.  So this usage is
> correct.

But why use 'cookie' for something that isn't even a cookie!  Here's
the relevant section of the RFC:

https://tools.ietf.org/html/rfc7296#section-2.6

2.6.  IKE SA SPIs and Cookies

   The initial two eight-octet fields in the header, called the "IKE
   SPIs", are used as a connection identifier at the beginning of IKE
   packets.  Each endpoint chooses one of the two SPIs and MUST choose
   them so as to be unique identifiers of an IKE SA.  An SPI value of
   zero is special: it indicates that the remote SPI value is not yet
   known by the sender.

  [...]

   When a responder detects a large number of half-open IKE SAs, it
   SHOULD reply to IKE_SA_INIT requests with a response containing the
   COOKIE notification.  The data associated with this notification MUST
   be between 1 and 64 octets in length (inclusive), and its generation
   is described later in this section.  If the IKE_SA_INIT response
   includes the COOKIE notification, the initiator MUST then retry the
   IKE_SA_INIT request, and include the COOKIE notification containing
   the received data as the first payload, and all other payloads
   unchanged.  The initial exchange will then be as follows:

So what we're sizing here isn't an IKEv2 cookie.

Yes the common code still uses [ir]cookie.  We all get to grit our
teeth and wait until IKEv1 gets killed and it can be renamed (but I
see Paul's suggesting renaming it sooner).  Pushing the term cookie
into the IKEv2 code is going backwards.


> We call the fields isa_rcookie and isa_icookie.  They end up in
> st_icookie and st_rcookie.
>
> If we want to give the size a v2 name, that's fine.  It should be
> defined as COOKIE_SIZE, not 8, to make the relationship manifest.
> I'd prefer one name that shows both meanings but
> SIZE_IKEv1_COOKIE_IKEv2_IKE_SPI is unwieldy.
>
> I'm would be much less happy about about giving the fields two names.
> Aliasing a mutable thing is a really horrible trap.
>
> The current definition looks like this:
>
> /* COOKIE_SIZE is also IKEv2 IKE SPI size */
> #define COOKIE_SIZE 8

Full disclosure?

5618b2c31d      (D. Hugh Redelmeier     2018-07-14 08:59:54 -0400
 300)/* COOKIE_SIZE is also IKEv2 IKE SPI size */

> So if someone is puzzled about a reference to COOKIE_SIZE in V2 code,
> they can look at the definition and discover this explanation.

IMNSHO, our goal is to have the protocol and core code readable by
someone that has only read the IKEv2 RFC.  Using the term COOKIE to
mean something it doesn't has the exact opposite effect!

> COOKIE_SIZE was already widely used in v2 code before my change.  Many
> of those uses could be replaced by sizeofs.

I'm sure there are (I'm sure there's a name for this argument).  The
IKEv2 proposal code should have been clean.

> | - I suspect IPSEC_DOI_SPI_SIZE is equally dubious
>
> No, it is the size that is used in the kernel for ESP and AH SPIs.
> Nothing to do with the version of IKE.  There might be another
> existing name for this, I haven't looked.  If there is, it might be
> better.  Good luck grepping for 4.
>
> | and by using magic macros we've just burried what should be simple numbers.
>
> This I completely disagree with.  8 means many things.  COOKIE_SIZE
> shows what the heck the number is.  And the name helps you find
> related uses.  Good luck grepping for 8.

I'll take that as a rhetorical comment.  Code should either be using
sizeof, or more likely spi_size - since it can change from the POV of
common code.  Our cookie hashing code cheats and hashes zeros.

> We do not drop magic numbers into our code.  I'm surprised you think
> that we should.

I'm talking about defining macros for the macros sake.  Consider:

const struct encrypt_desc ike_alg_encrypt_aes_cbc = {
...
        .keydeflen =  128,
        .key_bit_lengths = { 256, 192, 128, },
...

vs:

const struct encrypt_desc ike_alg_encrypt_aes_cbc = {
...
        .keydeflen =    AES_KEY_DEF_LEN,
        .key_bit_lengths = { AES_KEY_MAX_LEN, 192, AES_KEY_MIN_LEN, },
...

(neither case appears in the code).  Which is clearer?