[Openvpn-devel] DCO: support key rotation notifications

From: Kristof Provost <kp@FreeBSD.org>

Allow the kernel driver to notify us that it's time to renegotiate keys.
The intent is to avoid IV re-use after 2^32 packets.

This is a first draft intended for discussion. The accompanying kernel
change for FreeBSD can be found in https://reviews.freebsd.org/D39570

Signed-off-by: Kristof Provost <kprovost@netgate.com>
---
 src/openvpn/dco_freebsd.c      |  4 ++++
 src/openvpn/dco_freebsd.h      |  1 +
 src/openvpn/forward.c          | 32 +++++++++++++++++++++-----------
 src/openvpn/multi.c            |  4 ++++
 src/openvpn/ovpn_dco_freebsd.h |  1 +
 src/openvpn/ssl.c              |  6 ++++++
 src/openvpn/ssl.h              |  3 +++
 7 files changed, 40 insertions(+), 11 deletions(-)

Hi,

On 14/04/2023 11:42, Kristof Provost via Openvpn-devel wrote:
> From: Kristof Provost <kp@FreeBSD.org>
> 
> Allow the kernel driver to notify us that it's time to renegotiate keys.
> The intent is to avoid IV re-use after 2^32 packets.
> 
> This is a first draft intended for discussion. The accompanying kernel
> change for FreeBSD can be found in https://reviews.freebsd.org/D39570
> 
> Signed-off-by: Kristof Provost <kprovost@netgate.com>

This looks good to me and I think it's reasonable to use the 
CMD_SWAP_KEYS as notification for userspace to actually trigger a key 
rotation.

Acked-by: Antonio Quartulli <a@unstable.cc>

Linux and Windows part is now missing.

Cheers,

> ---
>   src/openvpn/dco_freebsd.c      |  4 ++++
>   src/openvpn/dco_freebsd.h      |  1 +
>   src/openvpn/forward.c          | 32 +++++++++++++++++++++-----------
>   src/openvpn/multi.c            |  4 ++++
>   src/openvpn/ovpn_dco_freebsd.h |  1 +
>   src/openvpn/ssl.c              |  6 ++++++
>   src/openvpn/ssl.h              |  3 +++
>   7 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index a334d5d2..1111abeb 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -550,6 +550,10 @@ dco_do_read(dco_context_t *dco)
>               dco->dco_message_type = OVPN_CMD_DEL_PEER;
>               break;
>   
> +        case OVPN_NOTIF_ROTATE_KEY:
> +            dco->dco_message_type = OVPN_CMD_SWAP_KEYS;
> +            break;
> +
>           default:
>               msg(M_WARN, "Unknown kernel notification %d", type);
>               break;
> diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h
> index a07f9b69..e1a054e0 100644
> --- a/src/openvpn/dco_freebsd.h
> +++ b/src/openvpn/dco_freebsd.h
> @@ -35,6 +35,7 @@ typedef enum ovpn_key_cipher dco_cipher_t;
>   enum ovpn_message_type_t {
>       OVPN_CMD_DEL_PEER,
>       OVPN_CMD_PACKET,
> +    OVPN_CMD_SWAP_KEYS,
>   };
>   
>   enum ovpn_del_reason_t {
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index b3e0ba5d..d50eb457 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1232,20 +1232,30 @@ process_incoming_dco(struct context *c)
>           return;
>       }
>   
> -    if (dco->dco_message_type != OVPN_CMD_DEL_PEER)
> +    switch (dco->dco_message_type)
>       {
> -        msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,
> -            dco->dco_message_type);
> -        return;
> -    }
> +        case OVPN_CMD_DEL_PEER:
> +            if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED)
> +            {
> +                    msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
> +                                    "%d", __func__, dco->dco_message_peer_id);
> +                    trigger_ping_timeout_signal(c);
> +                    return;
> +            }
> +            break;
>   
> -    if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED)
> -    {
> -        msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
> -            "%d", __func__, dco->dco_message_peer_id);
> -        trigger_ping_timeout_signal(c);
> -        return;
> +        case OVPN_CMD_SWAP_KEYS:
> +            msg(D_DCO_DEBUG, "%s: received key rotation notification for peer-id %d",
> +                            __func__, dco->dco_message_peer_id);
> +            tls_session_soft_reset(c->c2.tls_multi);
> +            break;
> +
> +        default:
> +            msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,
> +                dco->dco_message_type);
> +            return;
>       }
> +
>   #endif /* if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD)) */
>   }
>   
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 5444e752..6fb9cff2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -3284,6 +3284,10 @@ multi_process_incoming_dco(struct multi_context *m)
>           {
>               process_incoming_del_peer(m, mi, dco);
>           }
> +        else if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS)
> +        {
> +            tls_session_soft_reset(mi->context.c2.tls_multi);
> +        }
>       }
>       else
>       {
> diff --git a/src/openvpn/ovpn_dco_freebsd.h b/src/openvpn/ovpn_dco_freebsd.h
> index fec33835..53f94dfd 100644
> --- a/src/openvpn/ovpn_dco_freebsd.h
> +++ b/src/openvpn/ovpn_dco_freebsd.h
> @@ -36,6 +36,7 @@
>   
>   enum ovpn_notif_type {
>       OVPN_NOTIF_DEL_PEER,
> +    OVPN_NOTIF_ROTATE_KEY,
>   };
>   
>   enum ovpn_del_reason {
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 60aaee8d..26e86c8d 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1918,6 +1918,12 @@ key_state_soft_reset(struct tls_session *session)
>       ks->remote_addr = ks_lame->remote_addr;
>   }
>   
> +void
> +tls_session_soft_reset(struct tls_multi *tls_multi)
> +{
> +	key_state_soft_reset(&tls_multi->session[TM_ACTIVE]);
> +}
> +
>   /*
>    * Read/write strings from/to a struct buffer with a u16 length prefix.
>    */
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 4ed4cfaa..3c40fbed 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -573,6 +573,9 @@ bool
>   tls_session_generate_data_channel_keys(struct tls_multi *multi,
>                                          struct tls_session *session);
>   
> +void
> +tls_session_soft_reset(struct tls_multi *multi);
> +
>   /**
>    * Load ovpn.xkey provider used for external key signing
>    */

I have stared a bit at the code, and it all looks sane (though kp@ and
Antonio are the experts here, anyway :-) ).  I have not tested things
beyond "passes client/server tests, with and without DCO" - my kernel
does not have the new code bits yet (and for a real tests, one would
have to modify it to send OVPN_NOTIF_ROTATE_KEY after like 1000 packets
or so...).

There were a few indentations that uncrustify did not like, so fixed
those one the fly (only whitespace).

Your patch has been applied to the master and release/2.6 branch
(this is somewhere between "bugfix" and "long-term compatibility",
and less of a "new feature").

commit ec71489bfc7c1d798f5f6de8e9fc187b9127072c (master)
commit c468af2cd90c9f682519eff38a21fac8a3feb148 (release/2.6)
Author: Kristof Provost
Date:   Fri Apr 14 11:42:27 2023 +0200

     DCO: support key rotation notifications

     Signed-off-by: Kristof Provost <kprovost@netgate.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20230414094227.9153-1-kprovost@netgate.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26590.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

On 8 May 2023, at 13:55, Gert Doering wrote:
> I have stared a bit at the code, and it all looks sane (though kp@ and
> Antonio are the experts here, anyway :-) ).  I have not tested things
> beyond "passes client/server tests, with and without DCO" - my kernel
> does not have the new code bits yet (and for a real tests, one would
> have to modify it to send OVPN_NOTIF_ROTATE_KEY after like 1000 packets
> or so...).
>
> There were a few indentations that uncrustify did not like, so fixed
> those one the fly (only whitespace).
>
> Your patch has been applied to the master and release/2.6 branch
> (this is somewhere between "bugfix" and "long-term compatibility",
> and less of a "new feature").
>
> commit ec71489bfc7c1d798f5f6de8e9fc187b9127072c (master)
> commit c468af2cd90c9f682519eff38a21fac8a3feb148 (release/2.6)
> Author: Kristof Provost
> Date:   Fri Apr 14 11:42:27 2023 +0200
>
>      DCO: support key rotation notifications
>
>      Signed-off-by: Kristof Provost <kprovost@netgate.com>
>      Acked-by: Antonio Quartulli <antonio@openvpn.net>
>      Message-Id: <20230414094227.9153-1-kprovost@netgate.com>
>      URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26590.html
>      Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
Thanks!

I’ve also landed the (FreeBSD) kernel side of that: https://cgit.freebsd.org/src/commit/?id=f7ee28e755820375d5f441e19c1f1376a200e834


Best regards,
Kristof

Hi,

On Mon, May 08, 2023 at 06:15:52PM +0200, Kristof Provost wrote:
> I???ve also landed the (FreeBSD) kernel side of that: https://cgit.freebsd.org/src/commit/?id=f7ee28e755820375d5f441e19c1f1376a200e834

I now had (finally) time to test this.

 - Upgraded FreeBSD current to "as of today"
 - modified the kernel to set OVPN_SEQ_ROTATE to "10000" and added
   a printf() call (to make this easier to observe)
 - pinged!

-> and it works nicely, every 10.000 packets, I see my kernel printf()

Jun  8 21:47:24 fbsd14 kernel: ovpn: seq64=10000, need key rotation

and then 

Jun  8 21:47:36 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 UDPv6 WRITE [14] to [AF_INET6]2001:608:0:814::f000:3:61067: P_CONTROL_SOFT_RESET_V1 kid=1 [ ] pid=0 DATA len=0
...
Jun  8 21:47:36 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 dco_install_key: peer_id=0 keyid=1, currently 1 keys installed
Jun  8 21:47:36 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 dco_new_key: slot 1, key-id 1, peer-id 0, cipher AES-256-GCM
Jun  8 21:48:37 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 Swapping primary and secondary keys to primary-id=1 secondary-id=0
Jun  8 21:48:37 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 dco_swap_keys: peer-id 0
...
Jun  8 21:50:38 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 dco_install_key: peer_id=0 keyid=2, currently 1 keys installed
Jun  8 21:50:38 fbsd14 tun-udp-p2mp[917]: myclient peer-id=0 dco_new_key: slot 1, key-id 2, peer-id 0, cipher AES-256-GCM


-> so kernel triggers the userland re-keying, userland swaps key in
kernel, ping does not notice...

--- fd00:abcd:114:2::1002 ping6 statistics ---
20000 packets transmitted, 20000 packets received, 0.0% packet loss
round-trip min/avg/max/std-dev = 0.249/0.377/11.693/0.150 ms


Good work, folks :-)

gert

PS: Antonio, so where's the Linux side...?

On 8 Jun 2023, at 21:52, Gert Doering wrote:
> Hi,
>
> On Mon, May 08, 2023 at 06:15:52PM +0200, Kristof Provost wrote:
>> I???ve also landed the (FreeBSD) kernel side of that: https://cgit.freebsd.org/src/commit/?id=f7ee28e755820375d5f441e19c1f1376a200e834
>
> I now had (finally) time to test this.
>
>  - Upgraded FreeBSD current to "as of today"
>  - modified the kernel to set OVPN_SEQ_ROTATE to "10000" and added
>    a printf() call (to make this easier to observe)
>  - pinged!
>
> -> and it works nicely, every 10.000 packets, I see my kernel printf()
>
…
>
> Good work, folks :-)
>
Thanks.

I’d tell you off for doubting me, but the first thought in my head when I saw this e-mail was “Oh no, what did I break?”, so I’m going to let it go this time :)

Best regards,
Kristof