[PATCH] add new_keys argument to esp_setup_keys() and comment clarifying esp.secrets format

[Daniel Lenski]

On Thu, Dec 15, 2016 at 12:59 AM, David Woodhouse <dwmw2 at infradead.org> wrote:
> On Wed, 2016-10-19 at 08:39 -0700, Daniel Lenski wrote:
>> -       unsigned char secrets[0x40];
>> +       unsigned char secrets[0x40]; /* Encryption key bytes, then HMAC key bytes */
>
> You're allowed to object to that horridness and split it into two
> separate fields for the encryption and HMAC keys, instead of just
> documenting it.

Okay. I don't know that much about security primitives, and thought
there might be some reason to avoid padding between the as-stored
encryption and HMAC keys. I was certainly wondering how it had been
done in the first place.

> In fact, one might argue that would be the better approach...

Yes, I assume OpenConnect jams them together like this only because
that's the form in which Juniper provides the keys.

>
>> -       vpninfo->old_esp_maxseq = vpninfo->esp_in[vpninfo->current_esp_in].seq + 32;
>> -       vpninfo->current_esp_in ^= 1;
>> +       if (new_keys) {
>> +               vpninfo->old_esp_maxseq = vpninfo->esp_in[vpninfo->current_esp_in].seq + 32;
>> +               vpninfo->current_esp_in ^= 1;
>> +       }
>
> Hm, do you not want that part in setup_esp_keys()? That's where we flip
> to the other incoming ESP setup, while still allowing a few more
> packets to be received on the existing one. Is there really no
> rekeying?

Right. setup_esp_keys() also generates new, random keys, however. And
with GP, the keys always come from the gateway in the big, verbose
getconfig XML file.

There is, as far as I can, never any requirement or signal from the
server to do a rekey. However, the client can effectively rekey by
closing the ESP connection, repulling the getconfig XML, and
restarting the ESP connection (that's what kill -USR2 / pause_cmd does
in my current implementation).

> Perhaps that part wants moving *out* of setup_esp_keys() to it callers?
> And in fact perhaps you could do the new_keys bit that way too? Just
> make an oncp_setup_esp_keys() function which creates the new keys and
> *then* calls the generic setup_esp_keys()? Note that the flip to the
> new incoming ESP setup has to be done *first*, or at least taken into
> account.

Yes, I think this would be the cleanest separation. Will do.

Thanks,
Dan