Message ID | 20230414094227.9153-1-kprovost@netgate.com |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel] DCO: support key rotation notifications | expand |
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
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 */