Merge pull request #1782 from sehraf/pr_improve_udpStunner

Reenable UDP stunner and add documentation

libretroshare/src/libretroshare.pro

@@ -12,10 +12,6 @@ DESTDIR = lib
 
 !include("use_libretroshare.pri"):error("Including")
 
-# the dht stunner is used to obtain RS external ip addr. when it is natted
-# this system is unreliable and rs supports a newer and better one (asking connected peers)
-# CONFIG += useDhtStunner
-
 # treat warnings as error for better removing
 #QMAKE_CFLAGS += -Werror
 #QMAKE_CXXFLAGS += -Werror

libretroshare/src/pqi/p3netmgr.cc

@@ -711,47 +711,6 @@ void p3NetMgrIMPL::netExtCheck()
 			}
 		}
 
-#ifdef ALLOW_DHT_STUNNER
-        // (cyril) I disabled this because it's pretty dangerous. The DHT can report a wrong address quite easily
-        // if the other DHT peers are not collaborating.
-        
-		/* Next ask the DhtStunner */
-		{
-#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
-			std::cerr << "p3NetMgrIMPL::netExtCheck() Ext Not Ok, Checking DhtStunner" << std::endl;
-#endif
-			uint8_t isstable = 0;
-			struct sockaddr_storage tmpaddr;
-			sockaddr_storage_clear(tmpaddr);
-
-			if (mDhtStunner)
-			{
-				/* input network bits */
-				if (mDhtStunner->getExternalAddr(tmpaddr, isstable))
-				{
-					if((rsBanList == NULL) || rsBanList->isAddressAccepted(tmpaddr,RSBANLIST_CHECKING_FLAGS_BLACKLIST))
-					{
-						// must be stable???
-						isStable = (isstable == 1);
-						//mNetFlags.mExtAddr = tmpaddr;
-						mNetFlags.mExtAddrOk = true;
-						mNetFlags.mExtAddrStableOk = isStable;
-
-						address_votes[tmpaddr].n++ ;
-#ifdef	NETMGR_DEBUG_STATEBOX
-						std::cerr << "p3NetMgrIMPL::netExtCheck() From DhtStunner: ";
-						std::cerr << sockaddr_storage_tostring(tmpaddr);
-						std::cerr << " Stable: " << (uint32_t) isstable;
-						std::cerr << std::endl;
-#endif
-					}
-					else
-						std::cerr << "(SS) DHTStunner returned wrong own IP " << sockaddr_storage_iptostring(tmpaddr) << " (banned). Rejecting." << std::endl;
-				}
-			}
-		}
-#endif
-
 		/* ask ExtAddrFinder */
 		{
 			/* ExtAddrFinder */
@@ -829,6 +788,49 @@ void p3NetMgrIMPL::netExtCheck()
 #endif
 		}
 
+#ifdef ALLOW_DHT_STUNNER
+		// (cyril) I disabled this because it's pretty dangerous. The DHT can report a wrong address quite easily
+		// if the other DHT peers are not collaborating.
+		// (sehraf) For the record: The udp stunner uses multiple (as for now: two) peers to ensure that the IP recieved is the correct one, see UdpStunner::locked_checkExternalAddress()
+		// Nevertheless this stays a more risky method to determine the external ip address.
+
+		/* lastly ask the DhtStunner as fallback */
+		if (address_votes.empty()) {
+#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
+			std::cerr << "p3NetMgrIMPL::netExtCheck() Ext Not Ok, Checking DhtStunner" << std::endl;
+#endif
+			uint8_t isstable = 0;
+			struct sockaddr_storage tmpaddr;
+			sockaddr_storage_clear(tmpaddr);
+
+			if (mDhtStunner)
+			{
+				/* input network bits */
+				if (mDhtStunner->getExternalAddr(tmpaddr, isstable))
+				{
+					if((rsBanList == NULL) || rsBanList->isAddressAccepted(tmpaddr,RSBANLIST_CHECKING_FLAGS_BLACKLIST))
+					{
+						// must be stable???
+						isStable = (isstable == 1);
+						//mNetFlags.mExtAddr = tmpaddr;
+						mNetFlags.mExtAddrOk = true;
+						mNetFlags.mExtAddrStableOk = isStable;
+
+						address_votes[tmpaddr].n++ ;
+#ifdef	NETMGR_DEBUG_STATEBOX
+						std::cerr << "p3NetMgrIMPL::netExtCheck() From DhtStunner: ";
+						std::cerr << sockaddr_storage_tostring(tmpaddr);
+						std::cerr << " Stable: " << (uint32_t) isstable;
+						std::cerr << std::endl;
+#endif
+					}
+					else
+						std::cerr << "(SS) DHTStunner returned wrong own IP " << sockaddr_storage_iptostring(tmpaddr) << " (banned). Rejecting." << std::endl;
+				}
+			}
+		}
+#endif
+
 		/* any other sources ??? */
 
 		/* finalise address */

libretroshare/src/tcponudp/udpstunner.cc

@@ -164,12 +164,12 @@ int	UdpStunner::grabExclusiveMode(std::string holder)  /* returns seconds since
 		commsage = 1;
 	}
 #ifdef DEBUG_UDP_STUNNER_FILTER
-#endif
 	std::cerr << "UdpStunner::grabExclusiveMode() SUCCESS. last comms: " << commsage;
 	std::cerr << " ago";
 	std::cerr << std::endl;
 	std::cerr << "UdpStunner::grabExclusiveMode() Exclusive held by: " << mExclusiveHolder;
 	std::cerr << std::endl;
+#endif
 
 	return commsage;
 }
@@ -181,13 +181,12 @@ int	UdpStunner::releaseExclusiveMode(std::string holder, bool forceStun)
 	if (!mExclusiveMode)
 	{
 #ifdef DEBUG_UDP_STUNNER_FILTER
-#endif
 		std::cerr << "UdpStunner::cancelExclusiveMode() ERROR, not in exclusive Mode";
 		std::cerr << std::endl;
+#endif
 		return 0;
 	}
 
-	rstime_t now = time(NULL);
 	mExclusiveMode = false;
 	if (forceStun)
 	{
@@ -217,10 +216,11 @@ int	UdpStunner::releaseExclusiveMode(std::string holder, bool forceStun)
 	}
 
 #ifdef DEBUG_UDP_STUNNER_FILTER
-#endif
+	rstime_t now = time(nullptr);
 	std::cerr << "UdpStunner::cancelExclusiveMode() Canceled. Was in ExclusiveMode for: " << now - mExclusiveModeTS;
 	std::cerr << " secs";
 	std::cerr << std::endl;
+#endif
 
 	return 1;
 }
@@ -326,7 +326,7 @@ bool UdpStunner::locked_handleStunPkt(void *data, int size, struct sockaddr_in &
 #endif
 		/* generate a response */
 		int len;
-		void *pkt = UdpStun_generate_stun_reply(&from, &len);
+		void *pkt = UdpStun_generate_stun_reply(&from, &len, data);
 		if (!pkt)
 			return false;
 
@@ -485,7 +485,7 @@ bool    UdpStun_response(void *stun_pkt, int size, struct sockaddr_in &addr)
 		return false;
 	}
 
-	if (htons(((uint16_t *) stun_pkt)[0]) != 0x0101)
+	if (htons(((uint16_t *) stun_pkt)[0]) != UdpStunner::STUN_BINDING_RESPONSE)
 	{
 		/* not a response */
 		return false;
@@ -517,33 +517,38 @@ bool UdpStun_generate_stun_pkt(void *stun_pkt, int *len)
 	}
 
 	/* just the header */
-	((uint16_t *) stun_pkt)[0] = (uint16_t) htons(0x0001);
+	((uint16_t *) stun_pkt)[0] = (uint16_t) htons(UdpStunner::STUN_BINDING_REQUEST);
 	((uint16_t *) stun_pkt)[1] = (uint16_t) htons(20); /* only header */
-	/* transaction id - should be random */
+	/* RFC 3489
-	((uint32_t *) stun_pkt)[1] = (uint32_t) htonl(0x0020); 
+	 *	The transaction ID is used to correlate requests and responses.
-	((uint32_t *) stun_pkt)[2] = (uint32_t) htonl(0x0121); 
+	 *
-	((uint32_t *) stun_pkt)[3] = (uint32_t) htonl(0x0111); 
+	 * RFC 5389 introduces a mmgic cokie at the location where preciously the transaction ID was located:
-	((uint32_t *) stun_pkt)[4] = (uint32_t) htonl(0x1010); 
+	 *	In RFC 3489, this field was part of
+	 *	the transaction ID; placing the magic cookie in this location allows
+	 *	a server to detect if the client will understand certain attributes
+	 *	that were added in this revised specification.
+	 */
+	RsRandom::random_bytes(&((uint8_t *) stun_pkt)[4], 4 * sizeof (uint32_t));
 	*len = 20;
 	return true;
 }
 
 
-void *UdpStun_generate_stun_reply(struct sockaddr_in *stun_addr, int *len)
+void *UdpStun_generate_stun_reply(struct sockaddr_in *stun_addr, int *len, const void *transId)
 {
 	/* just the header */
 	void *stun_pkt = rs_malloc(28);
     
 	if(!stun_pkt)
-            return NULL ;
+		return nullptr ;
         
-	((uint16_t *) stun_pkt)[0] = (uint16_t) htons(0x0101);
+	((uint16_t *) stun_pkt)[0] = (uint16_t) htons(UdpStunner::STUN_BINDING_RESPONSE);
 	((uint16_t *) stun_pkt)[1] = (uint16_t) htons(28); /* only header + 8 byte addr */
-	/* transaction id - should be random */
+	/* RFC 3489
-	((uint32_t *) stun_pkt)[1] = (uint32_t) htonl(0x0f20); 
+	 *	The Binding Response MUST contain the same transaction ID contained in the Binding Request.
-	((uint32_t *) stun_pkt)[2] = (uint32_t) htonl(0x0f21); 
+	 */
-	((uint32_t *) stun_pkt)[3] = (uint32_t) htonl(0x0f11); 
+	memcpy(&((uint32_t *) stun_pkt)[1], &((uint32_t *) transId)[1], 4 * sizeof (uint32_t));
-	((uint32_t *) stun_pkt)[4] = (uint32_t) htonl(0x1010); 
+
 	/* now add address
 	 *  0  1    2  3
 	 * <INET>  <port>
@@ -586,20 +591,20 @@ bool UdpStun_isStunPacket(void *data, int size)
 		return false;
 	}
 
-	if ((size == 20) && (0x0001 == ntohs(((uint16_t *) data)[0])))
+	if ((size == 20) && (UdpStunner::STUN_BINDING_REQUEST == ntohs(((uint16_t *) data)[0])))
 	{
 #ifdef DEBUG_UDP_STUNNER_FILTER
-		std::cerr << "UdpStunner::isStunPacket() (size=20 & data[0]=0x0001) -> true";
+		std::cerr << "UdpStunner::isStunPacket() (size=20 & data[0]=STUN_BINDING_REQUEST) -> true";
 		std::cerr << std::endl;
 #endif
 		/* request */
 		return true;
 	}
 
-	if ((size == 28) && (0x0101 == ntohs(((uint16_t *) data)[0])))
+	if ((size == 28) && (UdpStunner::STUN_BINDING_RESPONSE == ntohs(((uint16_t *) data)[0])))
 	{
 #ifdef DEBUG_UDP_STUNNER_FILTER
-		std::cerr << "UdpStunner::isStunPacket() (size=28 & data[0]=0x0101) -> true";
+		std::cerr << "UdpStunner::isStunPacket() (size=28 & data[0]=STUN_BINDING_RESPONSE) -> true";
 		std::cerr << std::endl;
 #endif
 		/* response */
@@ -947,15 +952,15 @@ bool    UdpStunner::locked_recvdStun(const struct sockaddr_in &remote, const str
 	if (remote.sin_addr.s_addr == extaddr.sin_addr.s_addr)
 	{
 #ifdef DEBUG_UDP_STUNNER
-#endif
 		std::cerr << "UdpStunner::locked_recvdStun() WARNING, BAD PEER: ";
 		std::cerr << "Stun Peer Returned its own address: " << rs_inet_ntoa(remote.sin_addr);
 		std::cerr << std::endl;
+#endif
 		return false;
 	}
 #endif
 
-	bool found = true;
+	bool found = false;
 	std::list<TouStunPeer>::iterator it;
 	for(it = mStunList.begin(); it != mStunList.end(); ++it)
 	{

libretroshare/src/tcponudp/udpstunner.h

@@ -30,10 +30,10 @@
 #include "util/rsthreads.h"
 #include <string>
 
-/* UdpStun.
+/**
+ * @brief The TouStunPeer class
  * Stuns peers to determine external addresses.
  */
-
 class TouStunPeer
 {
 	public:
@@ -53,10 +53,17 @@ class TouStunPeer
 		return; 
 	}
 	
+	/// id for identification
 	std::string id;
-	struct sockaddr_in remote, eaddr;
+	/// Remote address of the peer.
+	struct sockaddr_in remote;
+	/// Our external IP address as reported by the peer.
+	struct sockaddr_in eaddr;
+	/// true when a response was received in the past
 	bool response;
+	/// used to rate limit STUN requests
 	rstime_t lastsend;
+	/// fail counter for dead/bad peer detection (0 = good)
 	uint32_t failCount;
 };
 
@@ -65,6 +72,13 @@ class TouStunPeer
  * #define UDPSTUN_ALLOW_LOCALNET	1	
  */
 
+/**
+ * @brief The UdpStunner class
+ * The UDP stunner implements the STUN protocol to determin the NAT type (behind that RS is usually running).
+ * It maintains a list of DHT peers that are regulary contacted.
+ *
+ * The actual NAT type determination logic is located in void pqiNetStateBox::determineNetworkState()
+ */
 class UdpStunner: public UdpSubReceiver
 {
 	public:
@@ -102,6 +116,12 @@ virtual int status(std::ostream &out);
 	/* monitoring / updates */
 	int tick();
 
+	/*
+	 * based on RFC 3489
+	 */
+	static constexpr uint16_t STUN_BINDING_REQUEST  = 0x0001;
+	static constexpr uint16_t STUN_BINDING_RESPONSE = 0x0101;
+
 	private:
 
 bool  	checkStunDesired();
@@ -143,13 +163,19 @@ bool    locked_checkExternalAddress();
 
 #endif
 
+	/// The UDP stunner will only (actively) contact it's peers when mPassiveStunMode is false. (has priority over mForceRestun
 	bool mPassiveStunMode;
+	/// Time between STUNs
 	uint32_t mTargetStunPeriod;
+	/// Rate that determines how often STUN attempts are successfull
 	double mSuccessRate;
 
+	/// Some variables used for tracking who and when exclusive mode is enabled
 	bool mExclusiveMode; /* when this is switched on, the stunner stays silent (and extAddr is maintained) */
 	rstime_t mExclusiveModeTS;
 	std::string mExclusiveHolder;
+
+	/// force a STUN immediately
 	bool mForceRestun;
 
 };
@@ -158,7 +184,14 @@ bool    locked_checkExternalAddress();
 
 bool	UdpStun_isStunPacket(void *data, int size);
 bool    UdpStun_response(void *stun_pkt, int size, struct sockaddr_in &addr);
-void   *UdpStun_generate_stun_reply(struct sockaddr_in *stun_addr, int *len);
+/**
+ * @brief UdpStun_generate_stun_reply Generates a STUN reply package.
+ * @param stun_addr The address to set in the response field.
+ * @param len Lenght of the generated package (always 28).
+ * @param transId The transaction ID of the request package.
+ * @return Pointer to the generated reply package.
+ */
+void   *UdpStun_generate_stun_reply(struct sockaddr_in *stun_addr, int *len, const void* transId);
 bool    UdpStun_generate_stun_pkt(void *stun_pkt, int *len);
 
 #endif

retroshare.pri

@@ -111,6 +111,19 @@ no_direct_chat:CONFIG -= direct_chat
 CONFIG *= bitdht
 no_bitdht:CONFIG -= bitdht
 
+# The DHT stunner is used to determine the NAT type using other RS DHT peers and the STUN (Session Traversal Utilities for NAT) protocol.
+# To disable DHT stunner append the following assignation to qmake command line
+# "CONFIG+=no_use_dht_stunner"
+CONFIG *= use_dht_stunner
+no_use_dht_stunner:CONFIG -= use_dht_stunner
+
+# The DHT stunner can be used to figure out our external IP. As this purely relying on random DHT peers that answer our request, it can easily be abused.
+# Therefore, it is turned off by default.
+# To enable external ip determination (additionally) based on the dht stunner append the following assignation to qmake
+# command line "CONFIG+=use_dht_stunner_ext_ip"
+CONFIG *= no_use_dht_stunner_ext_ip
+use_dht_stunner_ext_ip:CONFIG -= no_use_dht_stunner_ext_ip
+
 # To select your MacOsX version append the following assignation to qmake
 # command line "CONFIG+=rs_macos10.11" where 10.11 depends your version
 macx:CONFIG *= rs_macos10.11
@@ -492,6 +505,14 @@ rs_gxs_trans {
 
 bitdht {
     DEFINES *= RS_USE_BITDHT
+
+    use_dht_stunner {
+        CONFIG *= useDhtStunner
+
+        use_dht_stunner_ext_ip {
+            DEFINES *= ALLOW_DHT_STUNNER
+        }
+    }
 }
 
 direct_chat {