| Message ID | 20241029-b4-ovpn-v11-20-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-10-29, 11:47:33 +0100, Antonio Quartulli wrote: > +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) > +{ [...] > + > + nla_nest_end(msg, k_attr); > + genlmsg_end(msg, hdr); > + > + genlmsg_multicast_netns(&ovpn_nl_family, dev_net(peer->ovpn->dev), msg, > + 0, OVPN_NLGRP_PEERS, GFP_ATOMIC); > + Is openvpn meant to support moving the device to a different netns? In that case I'm not sure the netns the ovpn netdevice is in is the right one, the userspace client will be in the encap socket's netns instead of the netdevice's? (same thing in the next patch)
On 05/11/2024 11:33, Sabrina Dubroca wrote: > 2024-10-29, 11:47:33 +0100, Antonio Quartulli wrote: >> +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) >> +{ > [...] >> + >> + nla_nest_end(msg, k_attr); >> + genlmsg_end(msg, hdr); >> + >> + genlmsg_multicast_netns(&ovpn_nl_family, dev_net(peer->ovpn->dev), msg, >> + 0, OVPN_NLGRP_PEERS, GFP_ATOMIC); >> + > > Is openvpn meant to support moving the device to a different netns? In > that case I'm not sure the netns the ovpn netdevice is in is the right > one, the userspace client will be in the encap socket's netns instead > of the netdevice's? > > (same thing in the next patch) Well, moving between netns's may not be among the most common use cases, but I can see people doing all kind of weird things, if not forbidden. Hence, I would not assume the netdevice to always stay in the same netns all time long. This said, what you say assumes that the userspace process won't change netns after having added the peer. I think we can live with that. I will change this call to use the sock's netns then. Thanks a lot! Regards, >
2024-11-12, 16:44:09 +0100, Antonio Quartulli wrote: > On 05/11/2024 11:33, Sabrina Dubroca wrote: > > 2024-10-29, 11:47:33 +0100, Antonio Quartulli wrote: > > > +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) > > > +{ > > [...] > > > + > > > + nla_nest_end(msg, k_attr); > > > + genlmsg_end(msg, hdr); > > > + > > > + genlmsg_multicast_netns(&ovpn_nl_family, dev_net(peer->ovpn->dev), msg, > > > + 0, OVPN_NLGRP_PEERS, GFP_ATOMIC); > > > + > > > > Is openvpn meant to support moving the device to a different netns? In > > that case I'm not sure the netns the ovpn netdevice is in is the right > > one, the userspace client will be in the encap socket's netns instead > > of the netdevice's? > > > > (same thing in the next patch) > > Well, moving between netns's may not be among the most common use cases, but > I can see people doing all kind of weird things, if not forbidden. The idea would be that only the ovpn device is in one particular netns, so that no packets can make it out of that netns unencrypted. I don't know if anybody actually does that. > Hence, I would not assume the netdevice to always stay in the same netns all > time long. > > This said, what you say assumes that the userspace process won't change > netns after having added the peer. That shouldn't matter as long as it's still listening to multicast messages in the original netns. Around that same "which netns" question, ovpn_udp{4,6}_output uses the socket's, but ovpn_nexthop_from_rt{4,6} uses the netdev's.
On 13/11/2024 15:28, Sabrina Dubroca wrote: > 2024-11-12, 16:44:09 +0100, Antonio Quartulli wrote: >> On 05/11/2024 11:33, Sabrina Dubroca wrote: >>> 2024-10-29, 11:47:33 +0100, Antonio Quartulli wrote: >>>> +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) >>>> +{ >>> [...] >>>> + >>>> + nla_nest_end(msg, k_attr); >>>> + genlmsg_end(msg, hdr); >>>> + >>>> + genlmsg_multicast_netns(&ovpn_nl_family, dev_net(peer->ovpn->dev), msg, >>>> + 0, OVPN_NLGRP_PEERS, GFP_ATOMIC); >>>> + >>> >>> Is openvpn meant to support moving the device to a different netns? In >>> that case I'm not sure the netns the ovpn netdevice is in is the right >>> one, the userspace client will be in the encap socket's netns instead >>> of the netdevice's? >>> >>> (same thing in the next patch) >> >> Well, moving between netns's may not be among the most common use cases, but >> I can see people doing all kind of weird things, if not forbidden. > > The idea would be that only the ovpn device is in one particular > netns, so that no packets can make it out of that netns unencrypted. I > don't know if anybody actually does that. Well I can imagine starting openvpn in the main netns and moving the device afterwards to something more restrictive (i.e. even a docker specific netns). > >> Hence, I would not assume the netdevice to always stay in the same netns all >> time long. >> >> This said, what you say assumes that the userspace process won't change >> netns after having added the peer. > > That shouldn't matter as long as it's still listening to multicast > messages in the original netns. Oky. > > Around that same "which netns" question, ovpn_udp{4,6}_output uses the > socket's, but ovpn_nexthop_from_rt{4,6} uses the netdev's. I think this is ok, because routing related decision should be taken in the netns where the device is, but the transport layer should make decisions based on where the socket lives. Regards, >
2024-11-14, 11:38:51 +0100, Antonio Quartulli wrote: > On 13/11/2024 15:28, Sabrina Dubroca wrote: > > Around that same "which netns" question, ovpn_udp{4,6}_output uses the > > socket's, but ovpn_nexthop_from_rt{4,6} uses the netdev's. > > I think this is ok, because routing related decision should be taken in the > netns where the device is, but the transport layer should make decisions > based on where the socket lives. Right, thanks for checking.
diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c index cfb014c947b968752ba3dab84ec42dc8ec086379..a2346bc630be9b60604282d20a33321c277bc56f 100644 --- a/drivers/net/ovpn/crypto.c +++ b/drivers/net/ovpn/crypto.c @@ -55,6 +55,25 @@ void ovpn_crypto_state_release(struct ovpn_crypto_state *cs) } } +/* removes the key matching the specified id from the crypto context */ +void ovpn_crypto_kill_key(struct ovpn_crypto_state *cs, u8 key_id) +{ + struct ovpn_crypto_key_slot *ks = NULL; + + spin_lock_bh(&cs->lock); + if (rcu_access_pointer(cs->slots[0])->key_id == key_id) { + ks = rcu_replace_pointer(cs->slots[0], NULL, + lockdep_is_held(&cs->lock)); + } else if (rcu_access_pointer(cs->slots[1])->key_id == key_id) { + ks = rcu_replace_pointer(cs->slots[1], NULL, + lockdep_is_held(&cs->lock)); + } + spin_unlock_bh(&cs->lock); + + if (ks) + ovpn_crypto_key_slot_put(ks); +} + /* Reset the ovpn_crypto_state object in a way that is atomic * to RCU readers. */ diff --git a/drivers/net/ovpn/crypto.h b/drivers/net/ovpn/crypto.h index 96fd41f4b81b74f8a3ecfe33ee24ba0122d222fe..b7a7be752d54f1f8bcd548e0a714511efcaf68a8 100644 --- a/drivers/net/ovpn/crypto.h +++ b/drivers/net/ovpn/crypto.h @@ -140,4 +140,6 @@ int ovpn_crypto_config_get(struct ovpn_crypto_state *cs, enum ovpn_key_slot slot, struct ovpn_key_config *keyconf); +void ovpn_crypto_kill_key(struct ovpn_crypto_state *cs, u8 key_id); + #endif /* _NET_OVPN_OVPNCRYPTO_H_ */ diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index 0e8a6f2c76bc7b2ccc287ad1187cf50f033bf261..c04791a508e5c0ae292b7b5d8098096c676b2f99 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -248,6 +248,19 @@ void ovpn_encrypt_post(void *data, int ret) if (likely(ovpn_skb_cb(skb)->req)) aead_request_free(ovpn_skb_cb(skb)->req); + if (unlikely(ret == -ERANGE)) { + /* we ran out of IVs and we must kill the key as it can't be + * use anymore + */ + netdev_warn(peer->ovpn->dev, + "killing key %u for peer %u\n", ks->key_id, + peer->id); + ovpn_crypto_kill_key(&peer->crypto, ks->key_id); + /* let userspace know so that a new key must be negotiated */ + ovpn_nl_key_swap_notify(peer, ks->key_id); + goto err; + } + if (unlikely(ret < 0)) goto err; diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index fe9377b9b8145784917460cd5f222bc7fae4d8db..2b2ba1a810a0e87fb9ffb43b988fa52725a9589b 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c @@ -999,6 +999,61 @@ int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info) return 0; } +/** + * ovpn_nl_key_swap_notify - notify userspace peer's key must be renewed + * @peer: the peer whose key needs to be renewed + * @key_id: the ID of the key that needs to be renewed + * + * Return: 0 on success or a negative error code otherwise + */ +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) +{ + struct nlattr *k_attr; + 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_KEY_SWAP_NTF); + if (!hdr) { + ret = -ENOBUFS; + goto err_free_msg; + } + + if (nla_put_u32(msg, OVPN_A_IFINDEX, peer->ovpn->dev->ifindex)) + goto err_cancel_msg; + + k_attr = nla_nest_start(msg, OVPN_A_KEYCONF); + if (!k_attr) + goto err_cancel_msg; + + if (nla_put_u32(msg, OVPN_A_KEYCONF_PEER_ID, peer->id)) + goto err_cancel_msg; + + if (nla_put_u16(msg, OVPN_A_KEYCONF_KEY_ID, key_id)) + goto err_cancel_msg; + + nla_nest_end(msg, k_attr); + 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 9e87cf11d1e9813b7a75ddf3705ab7d5fabe899f..33390b13c8904d40b629662005a9eb92ff617c3b 100644 --- a/drivers/net/ovpn/netlink.h +++ b/drivers/net/ovpn/netlink.h @@ -12,4 +12,6 @@ int ovpn_nl_register(void); void ovpn_nl_unregister(void); +int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id); + #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. Userspace has two ways of deciding when the key has to be renewed before exhausting the IV space: 1) time based approach: after X seconds/minutes userspace generates a new key and sends it to the kernel. This is based on guestimate and normally default timer value works well. 2) packet count based approach: after X packets/bytes userspace generates a new key and sends it to the kernel. Userspace keeps track of the amount of traffic by periodically polling GET_PEER and fetching the VPN/LINK stats. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/crypto.c | 19 ++++++++++++++++ drivers/net/ovpn/crypto.h | 2 ++ drivers/net/ovpn/io.c | 13 +++++++++++ drivers/net/ovpn/netlink.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/netlink.h | 2 ++ 5 files changed, 91 insertions(+)