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
