| Message ID | 20240627130843.21042-23-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-06-27, 15:08:40 +0200, Antonio Quartulli wrote: > IV wrap-around is cryptographically dangerous for a number of ciphers, > therefore kill the key and inform userspace (via netlink) should the > IV space go exhausted. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > drivers/net/ovpn/netlink.c | 39 ++++++++++++++++++++++++++++++++++++++ > drivers/net/ovpn/netlink.h | 8 ++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c > index 31c58cda6a3d..e43bbc9ad5d2 100644 > --- a/drivers/net/ovpn/netlink.c > +++ b/drivers/net/ovpn/netlink.c > @@ -846,6 +846,45 @@ int ovpn_nl_del_key_doit(struct sk_buff *skb, struct genl_info *info) > return 0; > } > > +int ovpn_nl_notify_swap_keys(struct ovpn_peer *peer) This is not getting called anywhere in this version. v3 had a change to ovpn_encrypt_one to handle the -ERANGE coming from ovpn_pktid_xmit_next. Assuming this was getting called just as the TX key expires (like it was in v3), I'm a bit unclear on how the client can deal well with this event. I don't see any way for userspace to know the current IV state (no notification for when the packetid gets past some threshold, and pid_xmit isn't getting dumped via netlink), so no chance for userspace to swap keys early and avoid running out of IVs. And then, since we don't have a usable primary key anymore, we will have to drop packets until userspace tells the kernel to swap the keys (or possibly install a secondary). Am I missing something in the kernel/userspace interaction?
Hi, On 17/07/2024 12:42, Sabrina Dubroca wrote: > 2024-06-27, 15:08:40 +0200, Antonio Quartulli wrote: >> IV wrap-around is cryptographically dangerous for a number of ciphers, >> therefore kill the key and inform userspace (via netlink) should the >> IV space go exhausted. >> >> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >> --- >> drivers/net/ovpn/netlink.c | 39 ++++++++++++++++++++++++++++++++++++++ >> drivers/net/ovpn/netlink.h | 8 ++++++++ >> 2 files changed, 47 insertions(+) >> >> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c >> index 31c58cda6a3d..e43bbc9ad5d2 100644 >> --- a/drivers/net/ovpn/netlink.c >> +++ b/drivers/net/ovpn/netlink.c >> @@ -846,6 +846,45 @@ int ovpn_nl_del_key_doit(struct sk_buff *skb, struct genl_info *info) >> return 0; >> } >> >> +int ovpn_nl_notify_swap_keys(struct ovpn_peer *peer) > > This is not getting called anywhere in this version. v3 had a change > to ovpn_encrypt_one to handle the -ERANGE coming from ovpn_pktid_xmit_next. > Darn! I must have missed this. This must have been removed by accident during the n-th rebase/reshuffle. Thanks for pointing it out. > Assuming this was getting called just as the TX key expires (like it > was in v3), I'm a bit unclear on how the client can deal well with > this event. Correct assumption. > > I don't see any way for userspace to know the current IV state (no > notification for when the packetid gets past some threshold, and > pid_xmit isn't getting dumped via netlink), so no chance for userspace > to swap keys early and avoid running out of IVs. And then, since we > don't have a usable primary key anymore, we will have to drop packets > until userspace tells the kernel to swap the keys (or possibly install > a secondary). > > Am I missing something in the kernel/userspace interaction? There are two events triggering userspace to generate a new key: 1) time based 2) packet count based 1) is easy: after X seconds/minutes generate a new key and send it to the kernel. It's obviously based on guestimate and normally our default works well. 2) after X packets/bytes generate a new key. Here userspace keeps track of the amount of traffic by periodically polling GET_PEER and fetching the VPN/LINK stats. A future improvement could be to have ovpn proactively notifying userspace after reaching a certain threshold, but for now this mechanism does not exist. I hope it helps. Cheers,
2024-07-17, 13:03:11 +0200, Antonio Quartulli wrote: > On 17/07/2024 12:42, Sabrina Dubroca wrote: > > I don't see any way for userspace to know the current IV state (no > > notification for when the packetid gets past some threshold, and > > pid_xmit isn't getting dumped via netlink), so no chance for userspace > > to swap keys early and avoid running out of IVs. And then, since we > > don't have a usable primary key anymore, we will have to drop packets > > until userspace tells the kernel to swap the keys (or possibly install > > a secondary). > > > > Am I missing something in the kernel/userspace interaction? > > There are two events triggering userspace to generate a new key: > 1) time based > 2) packet count based > > 1) is easy: after X seconds/minutes generate a new key and send it to the > kernel. It's obviously based on guestimate and normally our default works > well. > > 2) after X packets/bytes generate a new key. Here userspace keeps track of > the amount of traffic by periodically polling GET_PEER and fetching the > VPN/LINK stats. Oh right, that's what I was missing. TX packet count should be equivalent to packetid. Thanks. > A future improvement could be to have ovpn proactively notifying userspace > after reaching a certain threshold, but for now this mechanism does not > exist. If it's not there from the start, you won't be able to rely on it (because the userspace client may run on a kernel that does not provide the notification), so you would still have to fetch the stats, unless you have a way to poll for the threshold notification feature being present. > I hope it helps. Yup, thanks. Can you add this explanation to the commit message for this patch in the next version? Documenting a bit the expectations of the kernel/userspace interactions would be helpful, also for the sequencing of key installation/key swap operations. I'm guessing it goes something like this: 1. client sets up a primary key (key#1) and uses it 2. at some point, it sets up a secondary key (key#2) 3. later, keys are swapped (key#2 is now primary) 4. after some more time, the secondary (key#1) is removed and a new secondary (key#3) is installed [steps 3 and 4 keep repeating] And from reading patch 21, both the TX and RX key seem to be changed together (swap and delete operate on the whole keyslot, and set requires both the ENCRYPT_DIR and DECRYPT_DIR attributes). A rough description of the overall life of a client (opening sockets and setting up the ovpn device/peers) could also be useful alongside the code.
On 17/07/2024 15:26, Sabrina Dubroca wrote: > 2024-07-17, 13:03:11 +0200, Antonio Quartulli wrote: >> On 17/07/2024 12:42, Sabrina Dubroca wrote: >>> I don't see any way for userspace to know the current IV state (no >>> notification for when the packetid gets past some threshold, and >>> pid_xmit isn't getting dumped via netlink), so no chance for userspace >>> to swap keys early and avoid running out of IVs. And then, since we >>> don't have a usable primary key anymore, we will have to drop packets >>> until userspace tells the kernel to swap the keys (or possibly install >>> a secondary). >>> >>> Am I missing something in the kernel/userspace interaction? >> >> There are two events triggering userspace to generate a new key: >> 1) time based >> 2) packet count based >> >> 1) is easy: after X seconds/minutes generate a new key and send it to the >> kernel. It's obviously based on guestimate and normally our default works >> well. >> >> 2) after X packets/bytes generate a new key. Here userspace keeps track of >> the amount of traffic by periodically polling GET_PEER and fetching the >> VPN/LINK stats. > > Oh right, that's what I was missing. TX packet count should be > equivalent to packetid. Thanks. np! > > >> A future improvement could be to have ovpn proactively notifying userspace >> after reaching a certain threshold, but for now this mechanism does not >> exist. > > If it's not there from the start, you won't be able to rely on it > (because the userspace client may run on a kernel that does not > provide the notification), so you would still have to fetch the stats, > unless you have a way to poll for the threshold notification feature > being present. You're right. Then scratch that because userspace is not ready for this. > > >> I hope it helps. > > Yup, thanks. Can you add this explanation to the commit message for > this patch in the next version? Documenting a bit the expectations of > the kernel/userspace interactions would be helpful, also for the > sequencing of key installation/key swap operations. I'm guessing it > goes something like this: Sure, will document how this works. > > 1. client sets up a primary key (key#1) and uses it > 2. at some point, it sets up a secondary key (key#2) > 3. later, keys are swapped (key#2 is now primary) > 4. after some more time, the secondary (key#1) is removed and a new > secondary (key#3) is installed > [steps 3 and 4 keep repeating] > > And from reading patch 21, both the TX and RX key seem to be changed > together (swap and delete operate on the whole keyslot, and set > requires both the ENCRYPT_DIR and DECRYPT_DIR attributes). You are correct. Encryption and decryption keys are derived from the same key material that is exchanged/generated upon each "renegotiation". > > A rough description of the overall life of a client (opening sockets > and setting up the ovpn device/peers) could also be useful alongside > the code. > ok, will add it too! Thanks!
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index 31c58cda6a3d..e43bbc9ad5d2 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c @@ -846,6 +846,45 @@ int ovpn_nl_del_key_doit(struct sk_buff *skb, struct genl_info *info) return 0; } +int ovpn_nl_notify_swap_keys(struct ovpn_peer *peer) +{ + struct sk_buff *msg; + int ret = -EMSGSIZE; + void *hdr; + + netdev_info(peer->ovpn->dev, "peer with id %u must rekey - primary key unusable.\n", + peer->id); + + msg = nlmsg_new(100, GFP_ATOMIC); + if (!msg) + return -ENOMEM; + + hdr = genlmsg_put(msg, 0, 0, &ovpn_nl_family, 0, OVPN_CMD_SWAP_KEYS); + if (!hdr) { + ret = -ENOBUFS; + goto err_free_msg; + } + + if (nla_put_u32(msg, OVPN_A_IFINDEX, peer->ovpn->dev->ifindex)) + goto err_cancel_msg; + + if (nla_put_u32(msg, OVPN_A_PEER_ID, peer->id)) + goto err_cancel_msg; + + genlmsg_end(msg, hdr); + + genlmsg_multicast_netns(&ovpn_nl_family, dev_net(peer->ovpn->dev), msg, + 0, OVPN_NLGRP_PEERS, GFP_ATOMIC); + + return 0; + +err_cancel_msg: + genlmsg_cancel(msg, hdr); +err_free_msg: + nlmsg_free(msg); + return ret; +} + /** * ovpn_nl_register - perform any needed registration in the NL subsustem * diff --git a/drivers/net/ovpn/netlink.h b/drivers/net/ovpn/netlink.h index 9e87cf11d1e9..c86cd102eeef 100644 --- a/drivers/net/ovpn/netlink.h +++ b/drivers/net/ovpn/netlink.h @@ -12,4 +12,12 @@ int ovpn_nl_register(void); void ovpn_nl_unregister(void); +/** + * ovpn_nl_notify_swap_keys - notify userspace peer's key must be renewed + * @peer: the peer whose key needs to be renewed + * + * Return: 0 on success or a negative error code otherwise + */ +int ovpn_nl_notify_swap_keys(struct ovpn_peer *peer); + #endif /* _NET_OVPN_NETLINK_H_ */
IV wrap-around is cryptographically dangerous for a number of ciphers, therefore kill the key and inform userspace (via netlink) should the IV space go exhausted. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/netlink.c | 39 ++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/netlink.h | 8 ++++++++ 2 files changed, 47 insertions(+)