If an outgoing connection cannot be made because no address is known for
it, it should be removed from the outgoing_list, otherwise it will
prevent it from being re-added later when we do know addresses for it.
This fixes a hairy race condition that was introduced in
1e89a63f16, which changed
the underlying transport of handshake packets from REQ_KEY to ANS_KEY.
Unfortunately, what I missed in that commit is, on the receiving side,
there is a slight difference between req_key_h() and ans_key_h():
indeed, the latter resets validkey to false.
The reason why this is not a problem during typical operation is
because the normal SPTPS key regeneration procedure looks like this:
KEX ->
<- KEX
SIG ->
<- SIG
All these messages are sent over ANS_KEY, therefore the receiving side
will unset validkey. However, that's typically not a problem in practice
because upon reception of the last message (SIG), SPTPS will call
sptps_receive_record(), which will set validkey to true again, and
everything works out fine in the end.
However, that was the *typical* scenario. Now let's assume that the
SPTPS channel is in active use at the same time key regeneration
happens. Specifically, let's assume a normal VPN data packet sneaks in
during the key regeneration procedure:
KEX ->
<- KEX
<- (SPTPS packet, over TCP or UDP)
<- KEX (wtf?)
SIG -> (refused with Invalid packet seqno: XXX != 0)
At this point, both nodes are extremely confused and the SPTPS channel
becomes unusable with various errors being thrown on both sides. The
channel will stay down until automatic SPTPS channel restart kicks in
after 10 seconds.
(Note: the above is just an example - the race can occur on either side
whenever a packet is sent during the period of time between KEX and SIG
messages are received by the node sending the packet.)
I've seen this race occur in the wild - it is very likely to occur if
key regeneration occurs on a heavily loaded channel. It can be
reproduced fairly easily by setting KeyExpire to a short value (a few
seconds) and then running something like ping -f foobar -i 0.01.
The reason why this occurs is because tinc's TX code path triggers the
following:
- send_packet()
- try_tx()
- try_tx_sptps()
- validkey is false because we just received an ANS_KEY message
- waitingforkey is false because it's not used for key regeneration
- send_req_key()
- SPTPS channel restart (sptps_stop(), sptps_start()).
Obviously, it all goes downhill from there and the two nodes get very
confused quickly (for example the seqno gets reset, hence the error
messages).
This commit fixes the issue by keeping validkey set when SPTPS data is
received over ANS_KEY messages.
Unfortunately, libminiupnpc has a somewhat... "peculiar" approach to
backwards compatibility for their API, where they reserve the right to
make breaking changes when they feel like it, forcing users to resort
to #ifdefs to ensure they use the correct API. Sigh.
Previously, tinc would only build against API versions <= 13, because I
was doing my initial development using miniupnpc-1.9.20140610 which is
the version that ships with Debian. The changes in this commit are
required for tinc to build against more recent versions, from
1.9.20150730 to the latest one at the time of this commit, 1.9.20151026.
This commit makes tincd capable of discovering UPnP-IGD devices on the
local network, and add mappings (port redirects) for its TCP and/or UDP
port.
The goal is to improve reliability and performance of tinc with nodes
sitting behind home routers that support UPnP, by making it less reliant
on UDP Hole Punching, which is prone to failure when "hostile" NATs are
involved.
The way this is implemented is by leveraging the libminiupnpc library,
which we have just added a new dependency on. We use pthread to run the
UPnP client code in a dedicated thread; we can't use the tinc event loop
because libminiupnpc doesn't have a non-blocking API.
When tinc is used in router mode with a TAP device, Ethernet (MAC)
headers are not present in packets flowing over the VPN; it is the
node's responsibility to fill out this header before handing the
packet over to the TAP interface (which expects such headers).
Currently, tinc fills out the destination MAC address of the packet
(otherwise the host would not recognize the packets, and nothing would
work), but it does not fill out the source MAC address. In practice this
doesn't seem to cause any real issues (the host doesn't care about the
source address), but it does look weird when looking at the packets with
a sniffer, and it also result in the following valgrind warning:
==13651== Syscall param write(buf) points to uninitialised byte(s)
==13651== at 0x5C4B620: __write_nocancel (syscall-template.S:81)
==13651== by 0x1445AA: write_packet (device.c:183)
==13651== by 0x118C7C: send_packet (net_packet.c:1259)
==13651== by 0x12B70A: route_ipv4 (route.c:443)
==13651== by 0x12D5F8: route (route.c:971)
==13651== by 0x1152BC: receive_packet (net_packet.c:250)
==13651== by 0x117E1B: receive_sptps_record (net_packet.c:904)
==13651== by 0x1309A8: sptps_receive_data_datagram (sptps.c:488)
==13651== by 0x130A90: sptps_receive_data (sptps.c:508)
==13651== by 0x115569: receive_udppacket (net_packet.c:286)
==13651== by 0x119856: handle_incoming_vpn_data (net_packet.c:1499)
==13651== by 0x10F3DA: event_loop (event.c:287)
==13651== Address 0xffeffea3a is on thread 1's stack
==13651== in frame #6, created by receive_sptps_record (net_packet.c:821)
==13651==
This commit fixes the issue by filling out the source MAC address. It is
generated by negating the last byte of the device MAC address, which is
consistent with what route_arp() does.
In addition, this commit stops route_arp() from filling out the Ethernet
header of the packet - this is the responsibility of send_packet(), not
route().
This reverts commit c2319e90b1.
As a general principle, I do not believe it is worthwhile to cache
nodes. Sure, it brings lookup time down from O(log n) to O(1), but
considering that the scalability target of tinc is around 1000 nodes
and log2(1000) is 10, that looks like premature optimization; tree
lookups should already be very fast. Therefore, I believe it makes sense
to remove the cache as a code cleanup initiative.
This commit replaces the node UDP address hash table "cache" with a
full-blown splay tree, aligning it with node_tree (name-indexed) and
node_id_tree (ID-indexed).
I'm doing this for two reasons. The first reason is to make sure we
don't suddenly degrade to O(n) performance when two "hot" nodes end up
in the same hash table bucket (collision).
The second, and most important, reason, has to do with the fact that
the hash table that was being used overrides elements that collide.
Indeed, it turns out that there is one scenario in which the contents of
node_udp_cache has *correctness* implications, not just performance
implications. This has to do with the way handle_incoming_vpn_data() is
implemented.
Assume the following topology:
A <-> B <-> C
Now let's consider the perspective of tincd running on B, and let's
assume the following is true:
- All nodes are using the 1.1 protocol with node IDs and relaying
support.
- Nodes A and C have UDP addresses that hash to the same value.
- Node C "wins" in the node_udp_cache (i.e. it overwrites A in the
cache).
- Node A has a "dynamic" UDP address (i.e. an UDP address that has been
detected dynamically and cannot be deduced from edge addresses).
Then, before this commit, A would be unable to relay packets through B.
This is because handle_incoming_vpn_data() will fall back to
try_harder(), which won't be able to match any edge addresses, doesn't
check the dynamic UDP addresses, and won't be able to match any keys
because this is a relayed packet which is encrypted with C's key, not
B's. As a result, tinc will fail to match the source of the packet and
will drop the packet with a "Received UDP packet from unknown source"
message.
I have seen this happen in the wild; it is actually quite likely to
occur when there are more than a handful of nodes because node_udp_cache
only has 256 buckets, making collisions quite likely. This problem is
quite severe because it can completely prevent all packet communication
between nodes - indeed, if node A tries to initiate some communication
with C, it will use relaying at first, until C responds and helps A
establish direct communication with it (e.g. hole punching). If relaying
is broken, C will not help establish direct communication, and as a
result no packets can make it through at all.
The bug can be reproduced fairly easily by reproducing the topology
above while changing the (hardcoded) node_udp_cache size to 1 to force a
collision. One will quickly observe various issues when trying to make A
talk to C. Setting IndirectData on B will make the issue even more
severe and prevent all communication.
Arguably, another way to fix this problem is to make try_harder()
compare the packet's source address to each node's dynamic UDP
addresses. However, I do not like this solution because if two "hot"
nodes are contending on the same hash bucket, try_harder() will be
called very often and packet routing performance will degrade closer to
O(N) (where N is the total number of nodes in the graph). Using a more
appropriate data structure fixes the bug without introducing this
performance problem.
Left shifts of negative values is undefined in C. This happens a lot in
the Ed25519 code. Cast to unsigned first, then cast the result back to
signed where necessary.
Although not a problem for tinc internally, the size of the struct was 12
bytes instead of 4, causing some problems when interpreting the value
received from tincd by the CLI.
In some cases - mostly when e->to == myself the prevedge is set to NULL,
causing invalid memory access. In rare cases this may lead to malformed mst
or segfaults.
If ADD_EDGE came from tinc version 1.0.x local_address.sa.sa_family is set to 0.
If it came from tinc version 1.1.x forwarded for older verion it will be 255 - AF_UNKNOWN.
When tinc gets ADD_EDGE from older versions it will allocate
new edge in protocol_edge.c:189 due to missed case in lines 149-171 where
local_address is not defined.
The proper place to clean up resources of objects is in their
destructor. This makes sure proper cleanup when edge_del() is called as
well. At exit, free_edge() is called on all edges by free_edge_tree(),
which is called by exit_nodes().
Let configure include sys/if_tun.h when testing for netinet/if_ether.h
to detect the Kernel/libc header conflict on musl.
After this patch, configure will correctly detect netinet/if_ether.h as
unusable and the subsequent compilation will not attempt to use it.
Conflicts:
src/have.h
With AutoConnect = yes, tinc tries to establish connections to known hosts.
However, you could have set no Address for this host, which is perfectly fine
(as long as there is at least one bootstrap node with an address or a local
discovered node already part of the network)
So log this to LOG_DEBUG
In a "decentrally managed vpn" it is very likely that host config
files for some reachable nodes do not exist. Currently, tinc
fills the logs with "Cannot open config file" messages.
This commit changes the log level to LOG_DEBUG so
syslog doesn't get filled by default.
The definition of the splay_each() macro is somewhat complicated for
syntactic reasons. Here's what it does in a more readable way:
for (splay_node_t* node = tree->head; node;) {
type* item = node->data;
splay_node_t* next = node->next;
// RUN USER BLOCK with (item)
node = next;
}
list_each() works in the same way. Since node->next is saved before the
user block runs, this construct supports removing the current item from
within the user block. However, what it does *not* support is removing
*other items* from within the user block, especially the next item.
Indeed, that will invalide the next pointer in the above loop and
therefore result in an invalid pointer dereference.
Unfortunately, there is at least one code path where that unsupported
operation happens. It is located in ack_h(), where the authentication
protocol code detects a double connection (i.e. being connected to
another node twice). Running in the context of a socket read event, this
code will happily terminate the *other* metaconnection, resulting in its
socket being removed from the io tree. If, by misfortune, this other
metaconnection happened to have the next socket FD number (which is
quite possible due to FD reuse - albeit unlikely), and was part of the
io tree (which is quite likely because if that connection is stuck, it
will most likely have pending writes) then this will result in the next
pending io item being destroyed. Invalid pointer dereference ensues.
I did a quick audit of other uses of splay_each() and list_each() and
I believe this is the only scenario in which this "next pointer
invalidation" problem can occur in practice. While this bug has been
there since at least 6bc5d626a8 (November
2012), if not sooner, it happens quite rarely due to the very specific
set of conditions required to trigger it. Nevertheless, it does manage
to crash my central production nodes every other week or so.
Unfortunately, sptps_logger() cannot know if s->handle is pointing to a
connection_t or a node_t. But it needs to print name and hostname in
both cases. So make sure both types have name and hostname fields at the
start with the same offset.
The sptps_receive_data() was changed in commit d237efd to only process
one SPTPS record from a stream input. So now we have to put a loop
around it to ensure we process everything.
In some harmless places, checks for the return value of ECDSA and RSA
key generation and verification was omitted. Add them to keep the
compiler happy and to warn end users in case something is wrong.
It is not unusual for tinc to receive SPTPS packets to be relayed to
nodes that just became unreachable, due to state propagation delays in
the metagraph.
Unfortunately, the current code doesn't handle that situation correctly,
and still tries to relay the packet to the unreachable node. This
typically ends up segfaulting.
This commit fixes the issue by checking for reachability before relaying
the packet.
clang-3.7 warnings surfaced an actual bug:
invitation.c:185:5: error: address of array 'filename' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
if(filename) {
~~ ^~~~~~~~
The regression was introduced in 3ccdf50beb.
This issue was found through a clang-3.7 warning:
protocol_misc.c:167:46: error: format specifies type 'short' but the argument has type 'int'
[-Werror,-Wformat]
if(!send_request(c, "%d %hd", SPTPS_PACKET, len))
~~~ ^~~
%d
It is entirely possible that the configuration file could contain a
ConnectTo statement refering to its own name; that's a reasonable
scenario when one deploys semi-automatically generated tinc.conf files.
Amusingly, tinc does not like that at all, and actually sets up an
outgoing_t structure to myself (which obviously makes no sense). This is
mostly benign, though it does result in non-sensical "Already connected
to myself" messages every retry interval.
However, that also makes things blow up in close_network_connections(),
because there we delete the entire outgoing list and *then* the myself
node, which still has a reference to the freshly deleted outgoing
structure. Boom.
timeout_handler() calls try_tx(c->node) when c->edge exists.
Unfortunately, the existence of c->edge is not enough to conclude that
the node is reachable.
In fact, during connection establishment, there is a short period of
time where we create an edge for the node at the other end of the
metaconnection, but we don't have one from the other side yet.
Unfortunately, if timeout_handler() runs during that short time
window, it will call try_tx() on an unreachable node, which makes
things explode because that function is not prepared to handle that
case.
A typical symptom of this race condition is a hard SEGFAULT while trying
to send packets using metaconnections that don't exist, due to
n->nexthop containing garbage.
This patch fixes the issue by making try_tx() check for reachability,
and then making all code paths use try_tx() instead of the more
specialized methods so that they go through the check.
This regression was introduced in
eb7a0db18e.
We do this by creating an umbilical between the CLI and the daemon. The
daemon pipes log messages to the CLI until it starts the main loop. The
daemon then cuts the umbilical. The CLI copies all the received log
messages to stderr, and the last byte indicates whether the daemon
started succesfully or not, so the CLI can exit with a useful exit code.
This gets rid of xasprintf() in a number of places, and removes the need
to free() the temporary strings. A few potential memory leaks have been
fixed.
This dumps the name of the invitation file, as well as the name of the
node that is being invited. This can make it easier to find the
invitation file belonging to a given node.
It is possible that opening /dev/net/tun works but that interface
creation itself fails, for example if a non-root user tries to create a
new interface, or if the desired interface is already opened by another
process. In this case, the ioctl() fails, but we actually silently
ignored this condition.
The compile time local state directory is usually /var or
/usr/local/var. If this is not accessible for some reason, for example
because someone ./configured tinc without --localstatedir and
/usr/local/var does not exist, or if tinc is started by a non-root user,
then tinc will fall back to the directory where tinc.conf is stored.
A warning is logged when this happens.
This function is not used for normal traffic, only when a packet from an
unknown source is received and we need to check against candidates. No
failures should be logger in this case; if the packet is really not
valid this will be logged by handle_incoming_vpn_data().
try_tx_sptps() gives up on UDP communication if the recipient doesn't
support relaying. This is too restrictive - we only need the other node
to support relaying if we actually want to relay through them. If the
packet is sent directly, it's fine to send it to an old pre-node-IDs
tinc-1.1 node.
Currently, tinc tries to parse node IDs for all SPTPS packets, including
ones sent from older, pre-node-IDs tinc-1.1 nodes, and therefore doesn't
recognize packets from these nodes. This commit fixes that.
It also makes code slightly clearer by reducing the amount of fiddling
around packet offset/length.
A condition in try_harder() is always evaluating to false when talking
to a SPTPS node because n->status.validkey_in is always false in that
case. Fix the condition so that the SPTPS status is correctly checked.
This prevented recent tinc-1.1 nodes from talking to older, pre-node-ID
tinc-1.1 nodes.
The regression was introduced in
6056f1c13b.
Since commit 13f9bc1ff1, tinc passes the
-I. option to the preprocessor so that version_git.h can be found during
out-of-tree ("VPATH") builds.
The problem is, this option also affects the directory search for files
included *from* system headers. For example, on MinGW, unistd.h contains
the following line:
#include <process.h>
Which, due to -I. putting the tinc directory at the head of the search
order, results in tinc's process.h being included instead of the file
from MinGW. Hilarity ensues.
This commit fixes the issue by using -iquote, which doesn't affect
system headers.
KEY_CHANGED messages are only useful to invalidate keys for non-SPTPS nodes;
SPTPS nodes use a different internal mechanism (forced KEX) for that purpose.
Therefore, if we know we can't talk to legacy nodes, there's no point in
sending them these messages.
There are a number of ways a SPTPS tunnel can get into a corrupt state.
For example, during key regeneration, the KEX and SIG messages from
other nodes might arrive out of order, which confuses the hell out of
the SPTPS code. Another possible scenario is not noticing another node
crashed and restarted because there was no point in time where the node
was seen completely disconnected from *all* nodes; this could result in
using the wrong (old) key. There are probably other scenarios which have
not even been considered yet. Distributed systems are hard.
When SPTPS got confused by a packet, it used to crash the entire
process; fortunately that was fixed by commit
2e7f68ad2b. However, the error handling
(or lack thereof) leaves a lot to be desired. Currently, when SPTPS
encounters an error when receiving a packet, it just shrugs it off and
continues as if nothing happened. The problem is, sometimes getting
receive errors mean the tunnel is completely stuck and will not recover
on its own. In that case, the node will become unreachable - possibly
indefinitely.
The goal of this commit is to improve SPTPS error handling by taking
proactive action when an incoming packet triggers a failure, which is
often an indicator that the tunnel is stuck in some way. When that
happens, we simply restart SPTPS entirely, which should make the tunnel
recover quickly.
To prevent "storms" where two buggy nodes flood each other with invalid
packets and therefore spend all their time negotiating new tunnels, we
limit the frequency at which tunnel restarts happen to ten seconds.
It is likely this commit will solve the "Invalid KEX record length
during key regeneration" issue that has been seen in the wild. It is
difficult to be sure though because we do not have a full understanding
of all the possible conditions that can trigger this problem.
Commit 10c1f60c64 introduced a mechanism
by which a packet received by REQ_KEY could continue its journey over
UDP. This was based on the assumption that REQ_KEY messages would never
be used for handshake packets (which should never be sent over UDP,
because SPTPS currently doesn't handle lost handshake packets very
well).
Unfortunately, there is one case where handshake packets are sent using
REQ_KEY: when regenerating the SPTPS key for a pre-established channel.
With the current code, such packets risk getting relayed over UDP.
When processing a REQ_KEY message, it is impossible for the receiving
end to distinguish between a data SPTPS packet and a handshake packet,
because this information is stored in the type field which is encrypted
with the end-to-end key.
This commit fixes the issue by making tinc use ANS_KEY for all SPTPS
handshake messages. This works because ANS_KEY messages are never
forwarded using the SPTPS relay mechanisms, therefore they are
guaranteed to stick to TCP.
If the ADD_EDGE is for one of the edges we own, and if it is not the
same as we actually have, send a correcting ADD_EDGE back. Otherwise, if
the ADD_EDGE contains new information, update our idea of the local
address for that edge.
If the ADD_EDGE does not contain local address information, then we
never make a correction nor log a warning.
Currently, SPTPS packets are transported over TCP metaconnections using
extended REQ_KEY requests, in order for the packets to pass through
tinc-1.0 nodes unaltered. Unfortunately, this method presents two
significant downsides:
- An already encrypted SPTPS packet is decrypted and then encrypted
again every time it passes through a node, since it is transported
over the SPTPS channels of the metaconnections. This
double-encryption is unnecessary and wastes CPU cycles.
- More importantly, the only way to transport binary data over
standard metaconnection messages such as REQ_KEY is to encode it
in base64, which has a 33% encoding overhead. This wastes 25% of the
network bandwidth.
This commit introduces a new protocol message, SPTPS_PACKET, which can
be used to transport SPTPS packets over a TCP metaconnection in an
efficient way. The new message is appropriately protected through a
minor protocol version increment, and extended REQ_KEY messages are
still used with nodes that do not support the new message, as well as
for the intial handshake packets, for which efficiency is not a concern.
The way SPTPS_PACKET works is very similar to how the traditional PACKET
message works: after the SPTPS_PACKET message, the raw binary packet is
sent directly over the metaconnection. There is one important
difference, however: in the case of SPTPS_PACKET, the packet is sent
directly over the TCP stream completely bypassing the SPTPS channel of
the metaconnection itself for maximum efficiency. This is secure because
the SPTPS packet that is being sent is already encrypted with an
end-to-end key.
sptps_receive_data() always consumes the entire buffer passed to it,
which is somewhat inflexible. This commit improves the interface so that
sptps_receive_data() consumes at most one record. The goal is to allow
non-SPTPS stuff to be interleaved with SPTPS records in a single TCP
stream.
REQ_SPTPS implies the message has an ANS_ counterpart (like REQ_KEY,
ANS_KEY), but it doesn't. Therefore dropping the REQ_ seems more
appropriate, and we add a _PACKET suffix to reduce the likelihood of
naming conflicts.
Currently, when tinc receives a SPTPS packet over TCP via the REQ_KEY
encapsulation mechanism, it forwards it like any other TCP request. This
is inefficient, because even though we received the packet over TCP,
we might have an UDP link with the next hop, which means the packet
could be sent over UDP.
This commit removes that limitation by making sure SPTPS data packets
received through REQ_KEY requests are not forwarded as-is but passed
to send_sptps_data() instead, thereby using the same code path as if
the packet was received over UDP.
net_packet doesn't actually use send_sptps_data(); it only uses
send_sptps_data_priv(). In addition, the only user of send_sptps_data()
is protocol_key. Therefore it makes sense to expose
send_sptps_data_priv() directly, and move send_sptps_data() (which is
basically just boilerplate) as a local function in protocol_key.
Currently, when relaying SPTPS UDP packets, the code uses the direct
sender as the originator, instead of preserving the original source ID.
This wouldn't cause any issues in most cases because the originator and
the sender are the same in simple one-hop relay chains, but this will
break as soon as there is more than one relay.
This fixes some issues with the build system when building out of tree.
With this commit, it is now possible to do the following:
$ cd /tmp/build
$ /path/to/tinc/configure
$ make
Instead of using the hardcoded version number in configure.ac, this
makes tinc use the live version reported by "git describe",
queried on-the-fly during the build process and regenerated for every
build.
This makes tinc version output more useful, as tinc will now display the
number of commits since the last tag as well as the commit the binary is
built from, following the format described in git-describe(1).
Here's an example of tincd --version output:
tinc version release-1.1pre10-48-gc149315 (built Jun 29 2014 15:21:10, protocol 17.3)
When building directly from a release tag, this will look like the following:
tinc version release-1.1pre10 (built Jun 29 2014 15:21:10, protocol 17.3)
(Note that the format is slightly different - because of the way the
tags are named, it says "release-1.1pre10" instead of just "1.1pre10")
If git describe fails (for example when building from a release
tarball), the build automatically falls back to the autoconf-provided
VERSION macro (i.e. the old behavior).
read_rsa_public_key() was bailing out early if the given node already has an Ed25519 key, and
returned true even though c->rsa was NULL. The early bailout code isn't necessary anymore, so just
remove it.
This deals with the case where one node knows the Ed25519 key of another node, but not the other
way around. This was blocked by an overly paranoid check in id_h(). The upgrade_h() function already
handled this case, and the node that already knows the other's Ed25519 key checks that it has not
been changed, otherwise the connection will be aborted.
Unfortunately, glibc assumes that /etc/resolv.conf is a static file that
never changes. Even on servers, /etc/resolv.conf might be a dynamically
generated file, and we never know when it changes. So just call
res_init() every time, so glibc uses up-to-date nameserver information.
Conflicts:
src/have.h
src/net.c
src/net_setup.c
Testing has revealed that the newer series of Windows TAP drivers (i.e.
9.0.0.21 and later, also known as NDIS6, tap-windows6) suffer from
serious performance issues in the write path. Write operations seems to
take a very long time to complete, resulting in massive packet loss even
for throughputs as low as 10 Mbit/s.
I've made some attempts to alleviate the problem using parellelism. By
using custom code that allows up to 256 write operations at the same
time the results are much better, but it's still about 2 times worse
than the traditional 9.0.0.9 driver.
We need to investigate more and file a bug against tap-windows6, but in
the mean time, let's inform the user that he might not want to use the
latest drivers.
This is generally useful. We've seen issues that are specific to some
version of these drivers (especially the newer 9.0.0.21 version), so
it's relevant to log it, especially since that means it will be
copy-pasted by people posting their logs asking for help.
As a rule, it seems reasonable to make sure that tinc operates correctly
on at least 1G links, since these are pretty common. However, I have
observed replay window issues when operating at speeds of 600 Mbit/s and
above, especially when the receiving end is a Windows system (not sure
why). This commit increases the default so that this won't occur on
fresh setups.
It may not be obvious, but due to the way tinc operates (single-threaded
control loop with no intermediate packet buffer), UDP send and receive
buffers can have a massive impact on performance. It is therefore of
paramount importance that the buffers be large enough to prevent packet
drops that could occur while tinc is processing a packet.
Leaving that value to the OS default could be reasonable if we weren't
relying on it so much. Instead, this makes performance somewhat
unpredictable.
In practice, the worst case scenario occurs on Windows, where Microsoft
had the brillant idea of making the buffers 8K in size by default, no
matter what the link speed is. Considering that 8K flies past in a
matter of microseconds on >1G links, this is extremely inappropriate. On
these systems, changing the buffer size to 1M results in *obscene*
raw throughput improvements; I have observed a 10X jump from 40 Mbit/s
to 400 Mbit/s on my system.
In this commit, we stop trusting the OS to get this right and we use a
fixed 1M value instead, which should be enough for <=1G links.
Write operations to the Windows device do not necessarily complete
immediately; in fact, with the latest TAP-Win32 drivers, this never
seems to be the case.
write_packet() does not handle that case correctly, because the
OVERLAPPED structure and the packet data go out of scope before the
write operation completes, resulting in race conditions.
This commit fixes the issue by making sure these data structures are
kept in global scope, and by dropping any packets that may arrive while
the previous write operation is still pending.
On Windows, when disabling the device, tinc uses the CancelIo() to
cancel the pending read operation, and then proceeds to delete the event
handle immediately.
This assumes that CancelIo() blocks until the pending read request is
completely torn down and no references to it remain. While MSDN is not
completely clear on that subject, it does suggest that this is not the
case:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363791.aspx
If the function succeeds [...] the cancel operation for all pending
I/O operations issued by the calling thread for the specified file
handle was successfully requested.
This implies that cancellation was merely "requested", and that there
are no guarantees as to the state of the operation when CancelIo()
returns. Therefore, care must be taken not to close event handles
prematurely.
While I'm no aware of this potential race condition causing any problems
in practice, I don't want to take any chances.
Modern versions of GCC handle structure packing differently when
compiling for Windows, as reported in the following GCC bug report:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52991
In practice, this affects tinc because it uses packed structs as a
convenient way to populate packet headers. "struct ip" is especially
affected - on Linux, sizeof(struct ip) returns 20 as expected, while on
Windows, it returns 24 because of the broken alignment.
This in turn completely breaks code that has to populate an IP header.
Specifically, this breaks route_ipv4_unreachable() which is responsible,
among other things, for the generation of ICMP Fragmentation Needed
messages. On Windows, these messages are corrupted beyond hope because
of this alignment issue. For TCP connections that are established
before tinc obtains a fix on the MTU (and thus are not MSS clamped),
this can result in massive disruption.
This commit fixes the issue by forcing GCC to use standard alignment
for all packed structures in the tinc codebase instead of the MSVC
alignment.
HAVE_DECL_RES_INIT is generated using AC_CHECK_DECLS. tinc checks this
symbol using #ifdef, which is wrong because (according to autoconf docs)
the symbol is always defined, it's just set to zero if the check failed.
This broke the Windows build starting from
0b310bf406, because it introduced this
conditional in code that's not excluded from the Windows build.
Ironically, commit 0f8e2cc78c introduced
a regression on its own, since it accidently removed a return statement
that prevented try_tx_sptps() from sending UDP/MTU probes to nodes that
are past static relays.
This makes sure MTU_INFO messages are only sent at the maximum rate of
5 per second (by default). As usual with these "probe" mechanisms, the
rate of these messages cannot be higher than the rate of data packets
themselves, since they are sent from the RX path.
This makes sure UDP_INFO messages are only sent at the maximum rate of
5 per second (by default). As usual with these "probe" mechanisms, the
rate of these messages cannot be higher than the rate of data packets
themselves, since they are sent from the RX path.
In this commit, nodes use MTU_INFO messages to provide MTU information.
The issue this code is meant to address is the non-trivial problem of
finding the proper MTU when UDP SPTPS relays are involved. Currently,
tinc has no idea what the MTU looks like beyond the first relay, and
will arbitrarily use the first relay's MTU as the limit. This will fail
miserably if the MTU decreases after the first relay, forcing relays to
fall back to TCP. More generally, one should keep in mind that relay
paths can be arbitrarily complex, resulting in packets taking "epic
journeys" through the graph, switching back and forth between UDP (with
variable MTUs) and TCP multiple times along the path.
A solution that was considered consists in sending standard MTU probes
through the relays. This is inefficient (if there are 3 nodes on one
side of relay and 3 nodes on the other side, we end up with 3*3=9 MTU
discoveries taking place at the same time, while technically only
3+3=6 are needed) and would involve eyebrow-raising behaviors such as
probes being sent over TCP.
This commit implements an alternative solution, which consists in
the packet receiver sending MTU_INFO messages to the packet sender.
The message contains an MTU value which is set to maximum when the
message is originally sent. The message gets altered as it travels
through the metagraph, such that when the message arrives to the
destination, the MTU value contained in the message can be used to
send packets while making sure no relays will be forced to fall back to
TCP to deliver them.
The operating principles behind such a protocol message are similar to
how the UDP_INFO message works, but there is a key difference that
prevents us from simply reusing the same message: the UDP_INFO message
only cares about relay-to-relay links (i.e. it is sent between static
relays and the information it contains only makes sense between two
adjacent static relays), while the MTU_INFO cares about the end-to-end
MTU, including the entire relay path. Therefore, UDP_INFO messages stop
when they encounter static relays, while MTU_INFO messages don't stop
until they get to the original packet sender.
Note that, technically, the MTU that is obtained through this mechanism
can be slightly pessimistic, because it can be lowered by an
intermediate node that is not being used as a relay. Since nodes have no
way of knowing whether they'll be used as dynamic relays or not (and
have no say in the matter), this is not a trivial problem. That said,
this is highly unlikely to result in noticeable issues in realistic
scenarios.
In this commit, nodes use UDP_INFO messages to provide UDP address
information. The basic principle is that the node that receives packets
sends UDP_INFO messages to the node that's sending the packets. The
message originally contains no address information, and is (hopefully)
updated with relevant address information as it gets relayed through the
metagraph - specifically, each intermediate node will update the message
with its best guess as to what the address is while forwarding it.
When a node receives an UDP_INFO message, and it doesn't have a
confirmed UDP tunnel with the originator node, it will update its
records with the new address for that node, so that it always has the
best possible guess as to how to reach that node. This applies to the
destination node of course, but also to any intermediate nodes, because
there's no reason they should pass on the free intel, and because it
results in nice behavior in the presence of relay chains (multiple nodes
in a path all trying to reach the same destination).
If, on the other hand, the node does have a confirmed UDP tunnel, it
will ignore the address information contained in the message.
In all cases, if the node that receives the message is not the
destination node specified in the message, it will forward the message
but not before overriding the address information with the one from its
own records. If the node has a confirmed UDP tunnel, that means the
message is updated with the address of the confirmed tunnel; if not,
the message simply reflects the records of the intermediate node, which
just happen to be the contents of the UDP_INFO message it just got, so
it's simply forwarded with no modification.
This is similar to the way ANS_KEY messages are currently
overloaded to provide UDP address information, with two differences:
- UDP_INFO messages are sent way more often than ANS_KEY messages,
thereby keeping the address information fresh. Previously, if the UDP
situation were to change after the ANS_KEY message was sent, the
sender would virtually never get the updated information.
- Once a node puts address information in an ANS_KEY message, it is
never changed again as the message travels through the metagraph; in
contrast, UDP_INFO messages behave the opposite way, as they get
rewritten every time they travel through a node with a confirmed UDP
tunnel. The latter behavior seems more appropriate because UDP tunnel
information becomes more relevant as it moves closer to the
destination node. The ANS_KEY behavior is not satisfactory in some
cases such as multi-layered graphs where the first hop is located
before a NAT.
Ultimately, the rationale behind this whole process is to improve UDP
hole punching capabilities when port translation is in effect, and more
generally, to make tinc more reliable in (very) hostile network
conditions (such as multi-layered NAT).
This commit adds a new command line option for tincd which allows to
use tincd in non-detached mode with log messages still going to
syslog. The motivation for this change is to ease use of tincd
in Docker containers.
If receive_handshake() or the receive_record() user callback returns an
error, sptps_receive_data_datagram() crashes the entire process. This is
heavy-handed, makes tinc very brittle to certain failures (i.e.
unexpected packets), and is inconsistent with the rest of SPTPS code.
Refactoring commit 81578484dc seems to
have introduced a regression as it moved discovery code away from
send_sptps_data_priv() and within send_packet(). The issue is,
send_packet() is not called when the node is simply relaying an UDP
SPTPS packet: indeed, send_sptps_data_priv() is called directly from
handle_incoming_vpn_data() in that case.
As a result, try_tx_sptps() is not called in the relaying case, which in
practice means that a relay doesn't initiate UDP/MTU discovery with the
next relay (unless some other activity compels it to do so). This can
result in packets getting sent over TCP instead of UDP from the relay.
Refactoring commit 0e65326047 broke UDP
SPTPS relaying by accidently removing try_tx_sptps() logic related to
establishing connectivity to so-called "dynamic" relays (i.e. relays
that are not specified by IndirectData configuration statements, but
are used on-the-fly to circumvent loss of direct UDP connectivity).
Specifically, the TX path was not trying to establish a tunnel to
dynamic relays (nexthop) anymore. This meant that MTU was not being
discovered with dynamic relays, which basically meant that all packets
being sent to dynamic relays went over TCP, thereby defeating the whole
purpose of SPTPS UDP relaying.
Note that this bug could easily go unnoticed if a tunnel was established
with the dynamic tunnel for some other reason (i.e. exchanging actual
data packets with the relay node).
Unfortunately, glibc assumes that /etc/resolv.conf is a static file that
never changes. Even on servers, /etc/resolv.conf might be a dynamically
generated file, and we never know when it changes. So just call
res_init() every time, so glibc uses up-to-date nameserver information.
This will report possible problems in the configuration files, and in
some cases offers to fix them.
The code is far from perfect yet. It expects keys to be in their default
locations, it doesn't check for Public/PrivateKey[File] statemetns yet.
It also does not correctly handle Ed25519 public keys yet.
When no UDP communication has been done yet, tinc establishes a guess
for the UDP address+port of each node. However, when there are multiple nodes
behind a NAT, tinc will guess the exact same address+port combination
for them, because it doesn't know about the NAT mappings yet. So when
receiving a packet, don't trust that guess unless we have confirmed UDP
communication.
This ensures try_harder() is called in such cases. However, this
function was actually very inefficient, trying to verify packets
multiple times for nodes with multiple edges. Only call try_mac() at
most once per node.
If we receive any traffic from another node, we periodically send back a
gratuitous type 2 probe reply with the maximum received packet length.
On the other node, this causes the udp and perhaps mtu probe timers to
be reset, so it does not need to send a probe request. Gratuitous probe
replies from another node also count as received traffic for this
purpose, so for nodes that also have a meta-connection, UDP keepalive
packets in principle can now solely be type 2 replies. This reduces the
amount of probe traffic even more.
To work, gratuitous replies should be sent slightly more often than
udp_discovery_keepalive_interval, so probe requests won't be triggered.
This also means that the timer resolution must be smaller than the
difference between the two, and at the moment it's kind of a hack.
When we have fixed the PMTU, n->mtuprobes == -1. When we send MTU probes
when mtuprobes == -1, decrease mtuprobes, and reset it back to -1 in
mtu_probe_h(). If mtuprobes < -1, send MTU probes every second, until
mtuprobes <= -4, in which case we will restart MTU discovery.
This is not working at all anymore. Just remove it, and we'll do another
attempt at RTT, bandwidth and packet loss estimation after the new
probing code stabilizes.
We are trying to decouple UDP probing from MTU probing, so only send
very small packets during UDP probing. This significantly reduces the
amount of traffic sent (54 to 67 bytes per probe instead of 1500 bytes).
This means the MTU probing code takes over sending PMTU sized probes,
but this commit does not take care of detecting PMTU decreases.
In tinc 1.0.x, this was tracked in node->inkey, however in tinc 1.1 we have an abstraction layer for
the legacy cipher and digest, and we don't keep an explicit copy of the key around. We cannot use
cipher_active() or digest_active(), since it is possible to set both to the null algorithm. So add a bit to
node_status_t.
This introduces a new configuration option,
UDPDiscoveryKeepaliveInterval, which is used as the UDP discovery
interval once the UDP tunnel is established. The pre-existing option,
UDPDiscoveryInterval, is therefore only used before UDP connectivity
is established.
The defaults are set so that tinc sends UDP pings more aggressively
if the tunnel is not established yet. This is appropriate since the
size of probes in that scenario is very small (16 bytes).
