[Swan-dev] COOKIE_SIZE is IKEv1!

[D. Hugh Redelmeier]

| From: Andrew Cagney <andrew.cagney at gmail.com>

| 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

There are two relevant RFCs.  The other one calls them cookies.

In v1, the IKE SPI is a bit confusing.  Look at RFC 2408 2.4.  But
these "cookies" are not, each, IKEv1 SPIs.

And 2408's use of the word cookie is questionable since it was
copied (badly) from Photurus.

But that is the term of the standard to which the code was written.

As Paul suggests, we could transition to the new terminology.
But I still wonder when v1 code gets deleted.  If it is soon, do the
renaming then.  If it isn't soon, why rename and disrupt the code.

I have proposed that all IKEv2_ names become v2_ names and IKEv1-only
names (currently with no prefix) become v1_names.  Nobody seemed
interested.

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

Of it's not about v2 cookies.  Currently names with no qualification
are V1 names (even if they apply only to v1).  So COOKIE_SIZE, by
convention, is in v1 terms.  But it is relevant to v2 (as previously
discussed).

| 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.

No, it was already there.  And it's pretty easy to fix if we invent a
better name since it is named.

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

Right, it was part of the commit we're talking about.  Called out in
the commit message.

| > 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!

But the code isn't written that way: we do support two standards.

COOKIE_SIZE and IKEv2_IKE_SPI_SIZE are not independent.  I did say
that I'd be OK with both names being defined as long as their
definitions are not independent.

| > 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).

What are you thinking here?  It seems like a pretty straight-forward
description.

|  The
| IKEv2 proposal code should have been clean.

What are you thinking of here?  Does the proposal code use the size of
an IKE SPI?  Yes, it did, with a naked numeral.  Not good.

| > | 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.

No, it's a style guideline.  Advocated by any number of wise people.
The first place I remember seeing it written down was in Kernighan &
Plauger's "Elements of Programming Style".  But it surely appeared
before that.

|  Code should either be using
| sizeof,

This works in many case and might be an improvement (not clear).  I
replaced "8" with COOKIE_SIZE in two places.  sizeof would work
awkwardly in one of those (it would require declaring a useless
variable) and not at all in the other.

| or more likely spi_size - since it can change from the POV of
| common code.

I don't know what you mean here.  Are you referring to
proto_spi_size()?  If so, that would work for neither of the places
that I changed 8 to COOKIE_SIZE.

|  Our cookie hashing code cheats and hashes zeros.

What are you referring to?  "cookie" hashing or IKEv2 cookie hashing?
zero bytes of size COOKIE_SIZE?  Hashing as specified by the RFC or
our own local hashing?

Why is it cheating?

| I'm talking about defining macros for the macros sake.

And you think COOKIE_SIZE is that?  There are 129 lines that use
COOKIE_SIZE (including comments) in our tree.

|  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?

It depends.  If AES_DEY_DEF_LEN appears many times, naming is better.
If it appears once, then this is itself naming.  The crossover point
is a matter of taste.