Attention is currently required from: Arne Schwabe.
Hello Arne Schwabe,
I'd like you to do a code review.
Please visit
https://gerrit.openvpn.net/c/openvpn/+/1983?usp=email
to review the following change.
Change subject: dco_linux: enable asymmetric peer IDs when supported
......................................................................
dco_linux: enable asymmetric peer IDs when supported
When notifying ovpn about a new peer, also send the TX peer ID so the
module can forward packets with the correct peer ID value. This
attribute is optional: if tx_peer_id is missing, the module falls back
to the non-multipeer configuration, where tx_peer_id equals rx_peer_id.
The attribute is sent only to a kernel that knows it, since older ones
reject OVPN_CMD_PEER_NEW with EINVAL on an attribute they cannot parse.
Define the first DCO capability, DCO_CAP_ASYM_PEER_ID, as a row in the
requirement table: the doit policy of OVPN_CMD_PEER_NEW must expose
OVPN_A_PEER_TX_ID as a u32 inside OVPN_A_PEER.
The bit is then consumed. It travels in tls_options, so a peer announces
its own ID only when it can use asymmetric peer-ids, and a server
replies with its own only when the client announced one and the local
kernel can do it too. A peer that announces nothing leaves the other
side on symmetric peer-ids, so there is nothing to exchange over the
wire. The local ID is generated under the same bit, which also lifts the
blanket skip tls_multi_init_finalize() applied whenever DCO was enabled.
Change-Id: Ic5c421ec2e9627340924a05509bb8aff0bbf17da
Signed-off-by: Ralf Lici <[email protected]>
Signed-off-by: Gianmarco De Gregori <[email protected]>
---
M src/openvpn/dco.c
M src/openvpn/dco.h
M src/openvpn/dco_freebsd.c
M src/openvpn/dco_internal.h
M src/openvpn/dco_linux.c
M src/openvpn/dco_win.c
M src/openvpn/init.c
M src/openvpn/ovpn_dco_linux.h
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
M src/openvpn/ssl_ncp.c
11 files changed, 119 insertions(+), 49 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/83/1983/1
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 15f32e1..6aa29b8 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -57,8 +57,10 @@
{
bool epoch = ks->crypto_options.flags & CO_EPOCH_DATA_KEY_FORMAT;
- msg(D_DCO_DEBUG, "%s: rx_peer_id=%d keyid=%d epoch=%d, currently %d keys
installed",
- __func__, multi->dco_rx_peer_id, ks->key_id, epoch,
multi->dco_keys_installed);
+ msg(D_DCO_DEBUG,
+ "%s: rx_peer_id=%d tx_peer_id=%u keyid=%d epoch=%d, currently %d keys
installed",
+ __func__, multi->dco_rx_peer_id, multi->tx_peer_id, ks->key_id, epoch,
+ multi->dco_keys_installed);
/* Install a key in the PRIMARY slot only when no other key exist.
* From that moment on, any new key will be installed in the SECONDARY
@@ -510,9 +512,11 @@
}
bool
-dco_check_pull_options(msglvl_t msglevel, const struct options *o)
+dco_check_pull_options(msglvl_t msglevel, const struct tls_multi *multi)
{
- if (!o->use_peer_id)
+ /* asymmetric peer-ids are negotiated, not pushed, so options.use_peer_id
+ * is not yet in sync when this runs */
+ if (!multi->use_peer_id)
{
msg(msglevel, "OPTIONS IMPORT: Server did not request DATA_V2 packet "
"format required for data channel offload");
@@ -544,8 +548,8 @@
c->c2.tls_multi->dco_rx_peer_id = -1;
}
#endif
- int ret = dco_new_peer(&c->c1.tuntap->dco, multi->rx_peer_id, sock->sd,
NULL,
- proto_is_dgram(sock->info.proto) ? remoteaddr :
NULL,
+ int ret = dco_new_peer(&c->c1.tuntap->dco, multi->rx_peer_id,
multi->tx_peer_id,
+ sock->sd, NULL, proto_is_dgram(sock->info.proto) ?
remoteaddr : NULL,
NULL, NULL);
if (ret < 0)
{
@@ -628,6 +632,7 @@
const struct context *c = &mi->context;
uint32_t rx_peer_id = c->c2.tls_multi->rx_peer_id;
+ uint32_t tx_peer_id = c->c2.tls_multi->tx_peer_id;
struct sockaddr *remoteaddr, *localaddr = NULL;
struct sockaddr_storage local = { 0 };
const socket_descriptor_t sd = c->c2.link_sockets[0]->sd;
@@ -665,7 +670,7 @@
}
int ret =
- dco_new_peer(&c->c1.tuntap->dco, rx_peer_id, sd, localaddr,
remoteaddr, vpn_addr4, vpn_addr6);
+ dco_new_peer(&c->c1.tuntap->dco, rx_peer_id, tx_peer_id, sd,
localaddr, remoteaddr, vpn_addr4, vpn_addr6);
if (ret < 0)
{
return ret;
diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
index 0a8c9da..6f6b983 100644
--- a/src/openvpn/dco.h
+++ b/src/openvpn/dco.h
@@ -46,6 +46,9 @@
#define DCO_IROUTE_METRIC 100
#define DCO_DEFAULT_METRIC 200
+/** Local DCO capability bits, derived from kernel introspection */
+#define DCO_CAP_ASYM_PEER_ID (1u << 0) /**< kernel supports OVPN_A_PEER_TX_ID
*/
+
#if defined(ENABLE_DCO)
/**
@@ -95,10 +98,10 @@
* for the first conflicting option found and return false.
*
* @param msglevel the msg level to use to print the warnings
- * @param o the options struct that hold the options
+ * @param multi the TLS state holding the negotiated parameters
* @return true if no conflict was detected, false otherwise
*/
-bool dco_check_pull_options(msglvl_t msglevel, const struct options *o);
+bool dco_check_pull_options(msglvl_t msglevel, const struct tls_multi *multi);
/**
* Initialize the DCO context
@@ -265,6 +268,7 @@
* of them and when it could not be asked
*/
unsigned int dco_probe_capabilities(void);
+
#else /* if defined(ENABLE_DCO) */
typedef void *dco_context_t;
@@ -294,7 +298,7 @@
}
static inline bool
-dco_check_pull_options(msglvl_t msglevel, const struct options *o)
+dco_check_pull_options(msglvl_t msglevel, const struct tls_multi *multi)
{
return false;
}
@@ -405,5 +409,6 @@
{
return 0;
}
+
#endif /* defined(ENABLE_DCO) */
#endif /* ifndef DCO_H */
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 178b754..dd8cd6f 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -133,9 +133,9 @@
}
int
-dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, int sd, struct
sockaddr *localaddr,
- struct sockaddr *remoteaddr, const struct in_addr *vpn_ipv4,
- const struct in6_addr *vpn_ipv6)
+dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, unsigned int
tx_peer_id, int sd,
+ struct sockaddr *localaddr, struct sockaddr *remoteaddr,
+ const struct in_addr *vpn_ipv4, const struct in6_addr *vpn_ipv6)
{
struct ifdrv drv;
nvlist_t *nvl, *local_nvl, *remote_nvl;
@@ -143,7 +143,8 @@
nvl = nvlist_create(0);
- msg(D_DCO_DEBUG, "%s: rx-peer-id %u, fd %d", __func__, rx_peer_id, sd);
+ msg(D_DCO_DEBUG, "%s: rx-peer-id %u, tx-peer-id %u, fd %d",
+ __func__, rx_peer_id, tx_peer_id, sd);
if (localaddr)
{
diff --git a/src/openvpn/dco_internal.h b/src/openvpn/dco_internal.h
index c507184..b4821d3 100644
--- a/src/openvpn/dco_internal.h
+++ b/src/openvpn/dco_internal.h
@@ -59,8 +59,9 @@
* They are implemented by dco_linux.c
*/
-int dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id,
socket_descriptor_t sd, struct sockaddr *localaddr,
- struct sockaddr *remoteaddr, const struct in_addr *vpn_ipv4,
const struct in6_addr *vpn_ipv6);
+int dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, unsigned int
tx_peer_id,
+ socket_descriptor_t sd, struct sockaddr *localaddr, struct
sockaddr *remoteaddr,
+ const struct in_addr *vpn_ipv4, const struct in6_addr
*vpn_ipv6);
int dco_del_peer(dco_context_t *dco, unsigned int rx_peer_id);
diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 96b7521..ff50405 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -120,6 +120,10 @@
int expected_type;
};
+static const struct dco_capability_req dco_capability_reqs[] = {
+ { DCO_CAP_ASYM_PEER_ID, OVPN_CMD_PEER_NEW, OVPN_A_PEER, OVPN_A_PEER_TX_ID,
NL_ATTR_TYPE_U32 },
+};
+
/** the outcome of resolving one capability requirement */
enum ovpn_cap_status
{
@@ -141,7 +145,8 @@
};
/* What the local kernel supports. It cannot change while we run, so the first
- * caller resolves it and the rest just read it. */
+ * caller resolves it and the rest just read it. That caller is
+ * do_init_crypto_tls(), which runs before any peer is added. */
static unsigned int dco_caps = 0;
static bool dco_caps_resolved = false;
@@ -390,14 +395,30 @@
return dco_caps;
}
- /* no capability is defined yet: resolve one whose answer we already know
*/
- static const struct dco_capability_req self_test = { 0, OVPN_CMD_PEER_NEW,
OVPN_A_PEER,
- OVPN_A_PEER_ID,
NL_ATTR_TYPE_U32 };
- int err = 0;
- enum ovpn_cap_status status = ovpn_cap_probe_one(&self_test, &err);
+ for (size_t i = 0; i < SIZE(dco_capability_reqs); i++)
+ {
+ const struct dco_capability_req *req = &dco_capability_reqs[i];
+ int err = 0;
- msg(D_DCO_DEBUG, "%s: policy introspection self-test: %s (status %d, err
%d)", __func__,
- status == OVPN_CAP_SUPPORTED ? "ok" : "unexpected", (int)status, err);
+ switch (ovpn_cap_probe_one(req, &err))
+ {
+ case OVPN_CAP_SUPPORTED:
+ dco_caps |= req->cap;
+ break;
+
+ case OVPN_CAP_UNSUPPORTED:
+ msg(D_DCO_DEBUG, "%s: kernel does not support DCO capability
0x%x", __func__,
+ req->cap);
+ break;
+
+ case OVPN_CAP_UNKNOWN:
+ msg(M_WARN,
+ "Note: cannot determine DCO capability 0x%x from the
kernel (%s), "
+ "assuming it is unsupported",
+ req->cap, err ? nl_geterror(err) : "incomplete policy
dump");
+ break;
+ }
+ }
dco_caps_resolved = true;
msg(D_DCO_DEBUG, "%s: DCO local capabilities: 0x%x", __func__, dco_caps);
@@ -523,9 +544,9 @@
}
int
-dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, int sd, struct
sockaddr *localaddr,
- struct sockaddr *remoteaddr, const struct in_addr *vpn_ipv4,
- const struct in6_addr *vpn_ipv6)
+dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, unsigned int
tx_peer_id, int sd,
+ struct sockaddr *localaddr, struct sockaddr *remoteaddr,
+ const struct in_addr *vpn_ipv4, const struct in6_addr *vpn_ipv6)
{
struct gc_arena gc = gc_new();
const char *remotestr = "[undefined]";
@@ -533,13 +554,21 @@
{
remotestr = print_sockaddr(remoteaddr, &gc);
}
- msg(D_DCO_DEBUG, "%s: rx-peer-id %u, fd %d, remote addr: %s", __func__,
rx_peer_id, sd, remotestr);
+ msg(D_DCO_DEBUG, "%s: rx-peer-id %u, tx-peer-id %u, fd %d, remote addr:
%s",
+ __func__, rx_peer_id, tx_peer_id, sd, remotestr);
struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_NEW);
struct nlattr *attr = nla_nest_start(nl_msg, OVPN_A_PEER);
int ret = -EMSGSIZE;
NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, rx_peer_id);
+ /* old kernels reject the whole command if they see an attribute they do
not
+ * know. This is the same answer the TLS layer used to decide whether to
+ * negotiate an asymmetric ID, so the two cannot disagree */
+ if (dco_caps & DCO_CAP_ASYM_PEER_ID)
+ {
+ NLA_PUT_U32(nl_msg, OVPN_A_PEER_TX_ID, tx_peer_id);
+ }
NLA_PUT_U32(nl_msg, OVPN_A_PEER_SOCKET, sd);
/* Set the remote endpoint if defined (for UDP) */
@@ -719,12 +748,6 @@
{
dco->ovpn_dco_id = resolve_ovpn_netlink_id(M_FATAL);
- /* nothing consumes the bitmap yet; resolve it here to exercise the path */
- if (check_debug_level(D_DCO_DEBUG))
- {
- dco_probe_capabilities();
- }
-
dco->nl_sock = nl_socket_alloc();
if (!dco->nl_sock)
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index f1e0418..3fce358 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -405,11 +405,12 @@
}
int
-dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, socket_descriptor_t
sd,
- struct sockaddr *localaddr, struct sockaddr *remoteaddr,
+dco_new_peer(dco_context_t *dco, unsigned int rx_peer_id, unsigned int
tx_peer_id,
+ socket_descriptor_t sd, struct sockaddr *localaddr, struct
sockaddr *remoteaddr,
const struct in_addr *vpn_ipv4, const struct in6_addr *vpn_ipv6)
{
- msg(D_DCO_DEBUG, "%s: rx-peer-id %u, fd " SOCKET_PRINTF, __func__,
rx_peer_id, sd);
+ msg(D_DCO_DEBUG, "%s: rx-peer-id %u, tx-peer-id %u, fd " SOCKET_PRINTF,
+ __func__, rx_peer_id, tx_peer_id, sd);
if (dco->ifmode == DCO_MODE_P2P)
{
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 9306a96..7083d1a 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2677,7 +2677,7 @@
}
/* Check if pushed options are compatible with DCO, if enabled */
- if (dco_enabled(&c->options) && !dco_check_pull_options(D_PUSH_ERRORS,
&c->options))
+ if (dco_enabled(&c->options) && !dco_check_pull_options(D_PUSH_ERRORS,
c->c2.tls_multi))
{
msg(D_PUSH_ERRORS, "OPTIONS ERROR: pushed options are incompatible
"
"with data channel offload. Use --disable-dco
to connect to "
@@ -3446,6 +3446,10 @@
/* let the TLS engine know if keys have to be installed in DCO or not */
to.dco_enabled = dco_enabled(options);
+ if (to.dco_enabled)
+ {
+ to.dco_capabilities = dco_probe_capabilities();
+ }
/*
* Initialize OpenVPN's master TLS-mode object.
diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h
index b3c9ff0..28cf97a 100644
--- a/src/openvpn/ovpn_dco_linux.h
+++ b/src/openvpn/ovpn_dco_linux.h
@@ -54,6 +54,7 @@
OVPN_A_PEER_LINK_TX_BYTES,
OVPN_A_PEER_LINK_RX_PACKETS,
OVPN_A_PEER_LINK_TX_PACKETS,
+ OVPN_A_PEER_TX_ID,
__OVPN_A_PEER_MAX,
OVPN_A_PEER_MAX = (__OVPN_A_PEER_MAX - 1)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 137e340..889a9ae 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1197,9 +1197,10 @@
tls_session_init(multi, &multi->session[TM_ACTIVE]);
tls_session_init(multi, &multi->session[TM_INITIAL]);
- if (!multi->opt.dco_enabled)
+ /* Calculate the asymmetric peer-id. Leaving it unset is what keeps us from
+ * announcing one to the peer, so DCO needs the kernel to support it */
+ if (dco_can_asym_peer_id(&multi->opt))
{
- /* Calculate the asymmetric peer-id */
if (multi->rx_peer_id == MAX_PEER_ID &&
multi->session[TM_INITIAL].opt->mode != MODE_SERVER)
{
multi->rx_peer_id = (uint32_t)(get_random() % (MAX_PEER_ID - 1));
@@ -1922,9 +1923,16 @@
static bool
push_peer_info_peerid(struct buffer *out, struct tls_multi *multi, struct
tls_session *session)
{
- if (multi->rx_peer_id == MAX_PEER_ID || session->opt->dco_enabled)
+ if (multi->rx_peer_id == MAX_PEER_ID)
{
- /* No valid peer id or DCO is enabled. Cannot use this feature */
+ /* No valid peer id */
+ return true;
+ }
+
+ if (!dco_can_asym_peer_id(session->opt))
+ {
+ /* DCO is in use but the local kernel cannot do asymmetric peer-ids:
+ * do not announce our ID so that the peer falls back as well */
return true;
}
@@ -2361,16 +2369,18 @@
{
output_peer_info_env(session->opt->es, multi->peer_info);
uint32_t peer_id = extract_asymmetric_peer_id(multi->peer_info);
- if (peer_id != MAX_PEER_ID && !session->opt->dco_enabled)
+ if (peer_id != MAX_PEER_ID && dco_can_asym_peer_id(session->opt))
{
+ /* set before key_method_2_write() runs, so that a server knows
+ * whether to announce its own ID back */
multi->tx_peer_id = peer_id;
multi->use_asymmetric_peer_id = true;
multi->use_peer_id = true;
}
else
{
- /* Peer has no support for asymmetric peer-id, and DCO currently
- * can only handle symmetric peer IDs */
+ /* the peer announced nothing, or we cannot use asymmetric peer-ids
+ * locally: stay symmetric */
multi->tx_peer_id = multi->rx_peer_id;
}
}
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index f32d188..2c9ad53 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -451,9 +451,21 @@
size_t ekm_label_size;
size_t ekm_size;
- bool dco_enabled; /**< Whether keys have to be installed in DCO or not */
+ bool dco_enabled; /**< Whether keys have to be installed in
DCO or not */
+ unsigned int dco_capabilities; /**< local kernel DCO capability bitmap
(DCO_CAP_*) */
};
+/**
+ * Whether asymmetric peer-ids can be used: either DCO is not in the way, or
the
+ * local kernel supports them. When false we neither generate nor announce an
+ * ID, so the remote peer falls back as well.
+ */
+static inline bool
+dco_can_asym_peer_id(const struct tls_options *o)
+{
+ return !o->dco_enabled || (o->dco_capabilities & DCO_CAP_ASYM_PEER_ID);
+}
+
/** @addtogroup control_processor
* @{ */
/** @name Index of key_state objects within a tls_session structure
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index f13f507..6cfe408 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -398,7 +398,7 @@
{
/* will return 0 if peer_info is null */
const unsigned int iv_proto_peer = extract_iv_proto(multi->peer_info);
- const unsigned int tx_peer_id =
extract_asymmetric_peer_id(multi->peer_info);
+ uint32_t tx_peer_id = extract_asymmetric_peer_id(multi->peer_info);
/* The other peer does not support P2P NCP */
if (!(iv_proto_peer & IV_PROTO_NCP_P2P))
@@ -406,6 +406,14 @@
return;
}
+ if (!dco_can_asym_peer_id(session->opt))
+ {
+ /* We cannot use asymmetric peer-ids locally: ignore the announced
+ * ID and use the legacy shared peer-id selection below */
+ tx_peer_id = MAX_PEER_ID;
+ multi->use_asymmetric_peer_id = false;
+ }
+
if (iv_proto_peer & IV_PROTO_DATA_V2)
{
multi->use_peer_id = true;
@@ -436,8 +444,7 @@
if (multi->use_peer_id)
{
/* The asymmetric peer-id trumps on the EKM generated ones */
- if ((iv_proto_peer & IV_PROTO_TLS_KEY_EXPORT)
- && (tx_peer_id != MAX_PEER_ID) && !multi->opt.dco_enabled)
+ if ((iv_proto_peer & IV_PROTO_TLS_KEY_EXPORT) && (tx_peer_id !=
MAX_PEER_ID))
{
multi->tx_peer_id = tx_peer_id;
multi->use_asymmetric_peer_id = true;
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/1983?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ic5c421ec2e9627340924a05509bb8aff0bbf17da
Gerrit-Change-Number: 1983
Gerrit-PatchSet: 1
Gerrit-Owner: Gianmarco De Gregori <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-CC: Ralf Lici <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
