Attention is currently required from: plaisthos, ralf_lici, stipa.
Hello plaisthos, ralf_lici,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1751?usp=email
to look at the new patch set (#26).
The following approvals got outdated and were removed:
Code-Review-1 by ralf_lici
Change subject: oob: Probe every resolved address of a remote
......................................................................
oob: Probe every resolved address of a remote
A remote can resolve to several A/AAAA records; --server-probe
previously sent a SERVER_PROBE only to the first one, so a remote whose
first address was unreachable looked unresponsive even if another
address answered, and the connection could later try an address that was
never probed.
Probe every resolved address of each remote in parallel (per-remote
storage is sized to the resolved address count, no fixed cap), accept a
reply from any of them (first reply for a remote wins), and resend to
all of a remote's addresses while it is unanswered. The per-address send
moves into oob_probe_send_target(), which keeps the
one-probe-per-address rule across the addresses of all entries.
The addresses of one remote are taken to be the same service, advertising
the same priority, weight and margin; the first of them to answer speaks
for the remote, and its round-trip time is measured from the probe sent to
that address.
Change-Id: I00208610e0ce4f3ed9a87233e68ece1b3b8a768a
Signed-off-by: Lev Stipakov <[email protected]>
---
M doc/man-sections/client-options.rst
M src/openvpn/oob.c
M src/openvpn/oob.h
M src/openvpn/oob_client.c
M tests/unit_tests/openvpn/test_oob.c
5 files changed, 249 insertions(+), 89 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/51/1751/26
diff --git a/doc/man-sections/client-options.rst
b/doc/man-sections/client-options.rst
index a3102bb..4f24642 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -604,7 +604,7 @@
server-probe
server-probe max-latency-diff
- A small probe message is sent to the first resolved address of every UDP
+ A small probe message is sent to every resolved address of every UDP
remote, and each answering server replies with its advertised priority
and weight. Remotes are then reordered following DNS SRV (RFC 2782)
semantics: servers that answered are tried before those that did not,
@@ -620,6 +620,10 @@
priority group a candidate, since replies only arrive within the
one-second probe window; the group is then ordered by weight alone.
+ The addresses of one remote are expected to be the same service and to
+ advertise the same values; the first of them to answer speaks for the
+ remote.
+
The probe carries the same control-channel wrapping as a normal
connection (``--tls-auth`` or ``--tls-crypt``, when configured). With
``--tls-crypt-v2`` the remotes are left in their configured order,
diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index 537e2c0..2149dd6 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -153,9 +153,16 @@
{
for (int i = start; i < n; i++)
{
- if (targets[i].sent && !results[i].responded && addr_port_match(from,
&targets[i].dest))
+ if (!targets[i].sent || results[i].responded)
{
- return i;
+ continue;
+ }
+ for (int k = 0; k < targets[i].n_dests; k++)
+ {
+ if (addr_port_match(from, &targets[i].dests[k]))
+ {
+ return i;
+ }
}
}
return -1;
@@ -174,6 +181,20 @@
return -1;
}
+int
+oob_probe_rtt_ms(const struct oob_probe_target *t, const struct
openvpn_sockaddr *from,
+ const struct timeval *rcv)
+{
+ const int k = oob_addr_list_find(t->dests, t->n_dests, from);
+ if (k < 0 || oob_sent_at_unset(&t->sent_at[k]))
+ {
+ return -1; /* not this remote's address, or never probed */
+ }
+ const long ms = (long)(rcv->tv_sec - t->sent_at[k].tv_sec) * 1000
+ + (rcv->tv_usec - t->sent_at[k].tv_usec) / 1000;
+ return (ms > 0) ? (int)ms : 0;
+}
+
/* Base ordering: responders before non-responders, then by priority (lower
* first), then by RTT (lower first), then by original index for determinism.
* This groups responders into priority runs pre-sorted by RTT, which the
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index 639ee65..432ac70 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -195,15 +195,37 @@
struct oob_probe_reply reply; /* the values the server advertised */
};
-/* Where the client probed one connection entry. */
+/* Where the client probed one connection entry: its resolved addresses. */
struct oob_probe_target
{
- struct openvpn_sockaddr dest;
- socklen_t destlen;
- bool sent;
- struct timeval sent_at; /* when the first probe went out, for the RTT */
+ struct openvpn_sockaddr *dests;
+ socklen_t *destlens;
+ struct timeval *sent_at; /* per address: when its first probe went out
(zero: never) */
+ int n_dests;
+ bool sent; /* at least one address was probed */
};
+/* An address's first-send time; zero until a probe went out to it. */
+static inline bool
+oob_sent_at_unset(const struct timeval *tv)
+{
+ return tv->tv_sec == 0 && tv->tv_usec == 0;
+}
+
+/**
+ * Round-trip time of a reply, measured from the first probe sent to the
+ * address it came from -- not from the first probe sent for the remote, as
+ * the addresses of one remote go out one after another.
+ *
+ * @param t the probed entry
+ * @param from source address of the reply
+ * @param rcv when the reply arrived
+ * @return milliseconds (0 if the clock went backwards), or -1 if from is not
+ * an address of t or no probe was sent to it
+ */
+int oob_probe_rtt_ms(const struct oob_probe_target *t, const struct
openvpn_sockaddr *from,
+ const struct timeval *rcv);
+
/**
* Find the next entry, from index \p start on, that was probed at \p from and
* has not answered yet. Several entries can resolve to the same address, so a
diff --git a/src/openvpn/oob_client.c b/src/openvpn/oob_client.c
index 5a3042a..ab8bed4 100644
--- a/src/openvpn/oob_client.c
+++ b/src/openvpn/oob_client.c
@@ -322,6 +322,81 @@
}
}
+/* Does any address of t have a probe socket of its family? */
+static bool
+oob_probe_target_reachable(const struct probe_ctx *pc, const struct
oob_probe_target *t)
+{
+ for (int k = 0; k < t->n_dests; k++)
+ {
+ if (pc->sd[probe_af_index(t->dests[k].addr.sa.sa_family)] !=
SOCKET_UNDEFINED)
+ {
+ return true;
+ }
+ }
+ return false;
+}
+
+/* Send the probe to every resolved address of t, each on the socket matching
+ * its address family, recording when each address was first sent so a reply's
+ * RTT is measured from the probe to the address that answered. An address an
+ * earlier entry already probed in this round is not probed again; that probe's
+ * reply covers this entry too. Returns true if at least one address was sent
+ * to or is already covered. */
+static bool
+oob_probe_send_target(struct probe_ctx *pc, const struct buffer *probe, struct
oob_probe_target *t)
+{
+ bool any_sent = false;
+ /* The RTT runs from an address's first send. A reply carries nothing that
+ * ties it to a send round, so measuring from a resend would report a
+ * server slower than one round as fast. */
+ for (int k = 0; k < t->n_dests; k++)
+ {
+ const int af = probe_af_index(t->dests[k].addr.sa.sa_family);
+ socket_descriptor_t sd = pc->sd[af];
+ if (sd == SOCKET_UNDEFINED)
+ {
+ continue;
+ }
+#ifdef TARGET_ANDROID
+ /* Keep the probe out of an active tunnel, as the connection socket is.
+ * protect_fd_nonlocal() declines a local peer, so only a non-local one
+ * actually protects the socket and may latch the flag. */
+ if (!pc->fd_protected[af] && !addr_local(&t->dests[k].addr.sa))
+ {
+ protect_fd_nonlocal(sd, &t->dests[k].addr.sa);
+ pc->fd_protected[af] = true;
+ }
+#endif
+ const int seen = oob_addr_list_find(pc->probed, pc->n_probed,
&t->dests[k]);
+ if (seen >= 0)
+ {
+ /* Already probed this round by an earlier entry: share that send
+ * time, so both entries report the same RTT for the one reply. */
+ if (oob_sent_at_unset(&t->sent_at[k]))
+ {
+ t->sent_at[k] = pc->probed_at[seen];
+ }
+ any_sent = true;
+ continue;
+ }
+ struct timeval sent_at;
+ openvpn_gettimeofday(&sent_at, NULL);
+ if (sendto(sd, (const char *)CBPTR(probe), (int)BLEN(probe), 0,
+ (const struct sockaddr *)&t->dests[k], t->destlens[k])
+ >= 0)
+ {
+ if (oob_sent_at_unset(&t->sent_at[k]))
+ {
+ t->sent_at[k] = sent_at;
+ }
+ pc->probed_at[pc->n_probed] = sent_at;
+ pc->probed[pc->n_probed++] = t->dests[k];
+ any_sent = true;
+ }
+ }
+ return any_sent;
+}
+
/* Parse one received datagram as a PROBE_REPLY and, if valid and matching one
* of the probes we sent, record the reply in results. */
static void
@@ -364,18 +439,18 @@
return;
}
- /* Credit the reply to every still-unanswered remote probed at its source
- * address: several entries can resolve to the same address, and each takes
- * the first reply for it. */
+ /* Credit the reply to every still-unanswered remote that was probed at its
+ * source address: several entries can resolve to the same address, and
each
+ * takes the first reply for it. (A remote with several addresses may
answer
+ * from more than one; the first one wins, and its RTT is that address's.)
*/
struct timeval rcv;
openvpn_gettimeofday(&rcv, NULL);
int i = oob_probe_next_target_at(from, targets, results, n, 0);
while (i >= 0)
{
- /* From this round's send to now, i.e. including any time the reply
- * waited in the receive queue while we were still sending. */
- long ms = (long)(rcv.tv_sec - targets[i].sent_at.tv_sec) * 1000
- + (rcv.tv_usec - targets[i].sent_at.tv_usec) / 1000;
+ /* From the first send to this address to now, i.e. including any time
+ * the reply waited in the receive queue while we were still sending.
*/
+ const int ms = oob_probe_rtt_ms(&targets[i], from, &rcv);
results[i].responded = true;
results[i].rtt_ms = (ms > 0) ? (unsigned int)ms : 0;
@@ -475,27 +550,9 @@
pc->n_probed = 0;
for (int i = 0; i < n; i++)
{
- if (!targets[i].sent || results[i].responded)
+ if (targets[i].sent && !results[i].responded)
{
- continue;
- }
- if (oob_addr_list_find(pc->probed, pc->n_probed, &targets[i].dest) >=
0)
- {
- continue;
- }
- socket_descriptor_t sd =
pc->sd[probe_af_index(targets[i].dest.addr.sa.sa_family)];
- if (sd == SOCKET_UNDEFINED)
- {
- continue;
- }
- struct timeval sent_at;
- openvpn_gettimeofday(&sent_at, NULL);
- if (sendto(sd, (const char *)CBPTR(probe), (int)BLEN(probe), 0,
- (const struct sockaddr *)&targets[i].dest,
targets[i].destlen)
- >= 0)
- {
- pc->probed_at[pc->n_probed] = sent_at;
- pc->probed[pc->n_probed++] = targets[i].dest;
+ oob_probe_send_target(pc, probe, &targets[i]);
}
}
}
@@ -701,11 +758,24 @@
continue;
}
- /* Store the first resolved address natively (no IPv4-mapping); it is
- * later probed on its family's socket. */
- memcpy(&targets[i].dest, ai->ai_addr, ai->ai_addrlen);
- targets[i].destlen = (socklen_t)ai->ai_addrlen;
- need_af[probe_af_index(ai->ai_family)] = true;
+ /* Store every resolved address natively (no IPv4-mapping); each is
later
+ * probed on its family's socket. */
+ struct oob_probe_target *t = &targets[i];
+ int n_addr = 0;
+ for (const struct addrinfo *a = ai; a; a = a->ai_next)
+ {
+ n_addr++;
+ }
+ t->dests = gc_malloc(sizeof(*t->dests) * n_addr, true, &gc);
+ t->destlens = gc_malloc(sizeof(*t->destlens) * n_addr, true, &gc);
+ t->sent_at = gc_malloc(sizeof(*t->sent_at) * n_addr, true, &gc);
+ for (const struct addrinfo *a = ai; a; a = a->ai_next)
+ {
+ memcpy(&t->dests[t->n_dests], a->ai_addr, a->ai_addrlen);
+ t->destlens[t->n_dests] = (socklen_t)a->ai_addrlen;
+ t->n_dests++;
+ need_af[probe_af_index(a->ai_family)] = true;
+ }
freeaddrinfo(ai);
}
@@ -722,58 +792,32 @@
return;
}
- /* Send a probe to each resolved remote, on the socket of its address
family.
- * An address several entries resolve to is probed once; its reply is
- * credited to each of them. */
- pc.probed = gc_malloc(sizeof(*pc.probed) * l->len, false, &gc);
- pc.probed_at = gc_malloc(sizeof(*pc.probed_at) * l->len, false, &gc);
+ /* Send a probe to each resolved remote, on the socket of each address. */
+ int n_addrs = 0;
+ for (int i = 0; i < l->len; i++)
+ {
+ n_addrs += targets[i].n_dests;
+ }
+ pc.probed = gc_malloc(sizeof(*pc.probed) * n_addrs, false, &gc);
+ pc.probed_at = gc_malloc(sizeof(*pc.probed_at) * n_addrs, false, &gc);
pc.n_probed = 0;
int sent_count = 0;
for (int i = 0; i < l->len; i++)
{
const struct connection_entry *ce = l->array[i];
- if (!targets[i].destlen)
+ struct oob_probe_target *t = &targets[i];
+ if (t->n_dests == 0 || !oob_probe_target_reachable(&pc, t))
{
+ continue; /* nothing resolved, or its family is not probed */
+ }
+
+ if (!oob_probe_send_target(&pc, &probe, t))
+ {
+ msg(D_LOW, "server-probe: %s:%s: probe send failed", ce->remote,
ce->remote_port);
continue;
}
- const int af = probe_af_index(targets[i].dest.addr.sa.sa_family);
- socket_descriptor_t sd = pc.sd[af];
- if (sd == SOCKET_UNDEFINED)
- {
- continue; /* family not probed, see oob_probe_sockets_open() */
- }
-#ifdef TARGET_ANDROID
- /* Keep the probe out of an active tunnel, as the connection socket is.
- * protect_fd_nonlocal() declines a local peer, so only a non-local one
- * actually protects the socket and may latch the flag. */
- if (!pc.fd_protected[af] && !addr_local(&targets[i].dest.addr.sa))
- {
- protect_fd_nonlocal(sd, &targets[i].dest.addr.sa);
- pc.fd_protected[af] = true;
- }
-#endif
- const int seen = oob_addr_list_find(pc.probed, pc.n_probed,
&targets[i].dest);
- if (seen >= 0)
- {
- /* Already probed this round by an earlier entry: reuse that send
- * time, so both entries report the same RTT for the one reply. */
- targets[i].sent_at = pc.probed_at[seen];
- }
- else
- {
- openvpn_gettimeofday(&targets[i].sent_at, NULL);
- if (sendto(sd, (const char *)BPTR(&probe), (int)BLEN(&probe), 0,
- (struct sockaddr *)&targets[i].dest, targets[i].destlen)
- < 0)
- {
- msg(D_LOW, "server-probe: %s:%s: probe send failed",
ce->remote, ce->remote_port);
- continue;
- }
- pc.probed_at[pc.n_probed] = targets[i].sent_at;
- pc.probed[pc.n_probed++] = targets[i].dest;
- }
- targets[i].sent = true;
+ t->sent = true;
sent_count++;
}
diff --git a/tests/unit_tests/openvpn/test_oob.c
b/tests/unit_tests/openvpn/test_oob.c
index 8973f04..26aacaf 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -519,10 +519,12 @@
return sa;
}
+/* One probed address per entry. */
static struct oob_probe_target
-probe_target(struct openvpn_sockaddr dest, bool sent)
+probe_target(struct openvpn_sockaddr *dest, bool sent)
{
- return (struct oob_probe_target){ .dest = dest, .destlen =
sizeof(dest.addr.in4), .sent = sent };
+ static socklen_t len = sizeof(struct sockaddr_in);
+ return (struct oob_probe_target){ .dests = dest, .destlens = &len,
.n_dests = 1, .sent = sent };
}
/* Two entries resolve to the same address. The first reply from it is
@@ -531,7 +533,7 @@
test_probe_reply_credits_every_entry_at_address(void **state)
{
struct openvpn_sockaddr a = v4_addr(0x7f000001, 1194);
- struct oob_probe_target targets[2] = { probe_target(a, true),
probe_target(a, true) };
+ struct oob_probe_target targets[2] = { probe_target(&a, true),
probe_target(&a, true) };
struct oob_probe_result results[2] = { 0 };
int i = oob_probe_next_target_at(&a, targets, results, 2, 0);
@@ -555,9 +557,9 @@
struct openvpn_sockaddr b = v4_addr(0x7f000002, 1194);
struct openvpn_sockaddr a_other_port = v4_addr(0x7f000001, 1195);
struct oob_probe_target targets[3] = {
- probe_target(a, true),
- probe_target(b, true),
- probe_target(a, false), /* resolved but never sent */
+ probe_target(&a, true),
+ probe_target(&b, true),
+ probe_target(&a, false), /* resolved but never sent */
};
struct oob_probe_result results[3] = { 0 };
@@ -568,6 +570,70 @@
assert_int_equal(oob_probe_next_target_at(&a_other_port, targets, results,
3, 0), -1);
}
+/* A remote with several addresses is credited whichever of them answers. */
+static void
+test_probe_reply_matches_any_address_of_entry(void **state)
+{
+ struct openvpn_sockaddr addrs[2] = { v4_addr(0x7f000001, 1194),
v4_addr(0x7f000002, 1194) };
+ socklen_t lens[2] = { sizeof(struct sockaddr_in), sizeof(struct
sockaddr_in) };
+ struct oob_probe_target targets[1] = {
+ { .dests = addrs, .destlens = lens, .n_dests = 2, .sent = true },
+ };
+ struct oob_probe_result results[1] = { 0 };
+
+ assert_int_equal(oob_probe_next_target_at(&addrs[1], targets, results, 1,
0), 0);
+ assert_int_equal(oob_probe_next_target_at(&addrs[0], targets, results, 1,
0), 0);
+}
+
+/* The RTT of a reply is measured from the probe sent to the address that
+ * answered, not from the first probe sent for the remote. */
+static void
+test_probe_rtt_per_address(void **state)
+{
+ struct openvpn_sockaddr addrs[2] = { v4_addr(0x7f000001, 1194),
v4_addr(0x7f000002, 1194) };
+ socklen_t lens[2] = { sizeof(struct sockaddr_in), sizeof(struct
sockaddr_in) };
+ /* A went out at t=0 ms, B 20 ms later */
+ struct timeval sent_at[2] = { { .tv_sec = 1000, .tv_usec = 0 }, { .tv_sec
= 1000, .tv_usec = 20000 } };
+ struct oob_probe_target t = { .dests = addrs, .destlens = lens, .sent_at =
sent_at, .n_dests = 2, .sent = true };
+ struct timeval rcv = { .tv_sec = 1000, .tv_usec = 25000 };
+
+ assert_int_equal(oob_probe_rtt_ms(&t, &addrs[1], &rcv), 5); /* B: 25 - 20
*/
+ assert_int_equal(oob_probe_rtt_ms(&t, &addrs[0], &rcv), 25); /* A: 25 - 0
*/
+
+ /* an address of the remote that was never sent to, and a stranger */
+ sent_at[1] = (struct timeval){ 0 };
+ assert_int_equal(oob_probe_rtt_ms(&t, &addrs[1], &rcv), -1);
+ struct openvpn_sockaddr other = v4_addr(0x7f000003, 1194);
+ assert_int_equal(oob_probe_rtt_ms(&t, &other, &rcv), -1);
+
+ /* a clock that went backwards reads as 0, not as a huge value */
+ rcv.tv_usec = 0;
+ sent_at[0].tv_usec = 5000;
+ assert_int_equal(oob_probe_rtt_ms(&t, &addrs[0], &rcv), 0);
+}
+
+/* IPv6 addresses match on address and port too. */
+static void
+test_probe_addresses_ipv6(void **state)
+{
+ struct openvpn_sockaddr a = { 0 }, b = { 0 };
+ a.addr.in6.sin6_family = AF_INET6;
+ a.addr.in6.sin6_addr.s6_addr[15] = 1; /* ::1 */
+ a.addr.in6.sin6_port = htons(1194);
+ b = a;
+ b.addr.in6.sin6_port = htons(1195);
+ socklen_t len = sizeof(struct sockaddr_in6);
+ struct oob_probe_target targets[1] = {
+ { .dests = &a, .destlens = &len, .n_dests = 1, .sent = true },
+ };
+ struct oob_probe_result results[1] = { 0 };
+
+ assert_int_equal(oob_probe_next_target_at(&a, targets, results, 1, 0), 0);
+ assert_int_equal(oob_probe_next_target_at(&b, targets, results, 1, 0), -1);
+ assert_int_equal(oob_addr_list_find(&a, 1, &a), 0);
+ assert_int_equal(oob_addr_list_find(&a, 1, &b), -1);
+}
+
/* The per-round list of probed addresses compares address and port. */
static void
test_addr_list_find(void **state)
@@ -973,6 +1039,9 @@
cmocka_unit_test(test_server_probe_check_no_parameter),
cmocka_unit_test(test_probe_reply_credits_every_entry_at_address),
cmocka_unit_test(test_probe_reply_matches_only_its_address),
+ cmocka_unit_test(test_probe_reply_matches_any_address_of_entry),
+ cmocka_unit_test(test_probe_rtt_per_address),
+ cmocka_unit_test(test_probe_addresses_ipv6),
cmocka_unit_test(test_addr_list_find),
cmocka_unit_test(test_client_reply_read_finds_reply),
cmocka_unit_test(test_client_reply_read_skips_unknown),
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1751?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I00208610e0ce4f3ed9a87233e68ece1b3b8a768a
Gerrit-Change-Number: 1751
Gerrit-PatchSet: 26
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: ralf_lici <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: ralf_lici <[email protected]>
Gerrit-Attention: stipa <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
