From 24b3325792dfce2b4a5ed1158aaf930cf19b5e2b Mon Sep 17 00:00:00 2001 From: csoler Date: Wed, 23 Sep 2015 21:45:15 -0400 Subject: [PATCH] Improved reliability of the determination of external address: removed DHT stunner from the pool, added mPeerMgr which vote is based on ext address most often reported by peer discovery --- libretroshare/src/pqi/p3netmgr.cc | 259 +++++++++++++++++------------ libretroshare/src/pqi/p3peermgr.cc | 93 ++++++++++- libretroshare/src/pqi/p3peermgr.h | 10 +- 3 files changed, 247 insertions(+), 115 deletions(-) diff --git a/libretroshare/src/pqi/p3netmgr.cc b/libretroshare/src/pqi/p3netmgr.cc index ccca8eb37..8776ad182 100644 --- a/libretroshare/src/pqi/p3netmgr.cc +++ b/libretroshare/src/pqi/p3netmgr.cc @@ -696,9 +696,9 @@ void p3NetMgrIMPL::netExtCheck() bool isStable = false; struct sockaddr_storage tmpip ; - std::map address_votes ; + std::map address_votes ; - /* check for External Address */ + /* check for External Address */ /* in order of importance */ /* (1) UPnP -> which handles itself */ { @@ -708,35 +708,41 @@ void p3NetMgrIMPL::netExtCheck() /* net Assist */ if (netAssistExtAddress(tmpip)) - { + { #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) - std::cerr << "p3NetMgrIMPL::netExtCheck() Ext supplied from netAssistExternalAddress()" << std::endl; + std::cerr << "p3NetMgrIMPL::netExtCheck() Ext supplied from netAssistExternalAddress()" << std::endl; #endif - if(sockaddr_storage_isValidNet(tmpip)) - { - if(rsBanList->isAddressAccepted(tmpip,RSBANLIST_CHECKING_FLAGS_BLACKLIST)) - { - // must be stable??? - isStable = true; - mNetFlags.mExtAddr = tmpip; - mNetFlags.mExtAddrOk = true; - mNetFlags.mExtAddrStableOk = isStable; + if(sockaddr_storage_isValidNet(tmpip)) + { + if(rsBanList->isAddressAccepted(tmpip,RSBANLIST_CHECKING_FLAGS_BLACKLIST)) + { + // must be stable??? + isStable = true; + //mNetFlags.mExtAddr = tmpip; + mNetFlags.mExtAddrOk = true; + mNetFlags.mExtAddrStableOk = isStable; - address_votes[tmpip].n++ ; - } - else - std::cerr << "(SS) netAssisExternalAddress returned wrong own IP " << sockaddr_storage_iptostring(tmpip) << " (banned). Rejecting." << std::endl; - } + address_votes[tmpip].n++ ; + + std::cerr << "NetAssistAddress reported external address " << sockaddr_storage_iptostring(tmpip) << std::endl; + } + else + std::cerr << "(SS) netAssisExternalAddress returned banned own IP " << sockaddr_storage_iptostring(tmpip) << " (banned). Rejecting." << std::endl; + } #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) - else - { - std::cerr << "p3NetMgrIMPL::netExtCheck() Bad Address supplied from netAssistExternalAddress()" << std::endl; - } + else + { + std::cerr << "p3NetMgrIMPL::netExtCheck() Bad Address supplied from netAssistExternalAddress()" << std::endl; + } #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. + /* Next ask the DhtStunner */ { #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) @@ -748,30 +754,31 @@ void p3NetMgrIMPL::netExtCheck() if (mDhtStunner) { - /* input network bits */ - if (mDhtStunner->getExternalAddr(tmpaddr, isstable)) - { - if(rsBanList->isAddressAccepted(tmpaddr,RSBANLIST_CHECKING_FLAGS_BLACKLIST)) - { - // must be stable??? - isStable = (isstable == 1); - mNetFlags.mExtAddr = tmpaddr; - mNetFlags.mExtAddrOk = true; - mNetFlags.mExtAddrStableOk = isStable; + /* input network bits */ + if (mDhtStunner->getExternalAddr(tmpaddr, isstable)) + { + if(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++ ; + 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; + 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; - } + } + else + std::cerr << "(SS) DHTStunner returned wrong own IP " << sockaddr_storage_iptostring(tmpaddr) << " (banned). Rejecting." << std::endl; + } } } +#endif /* otherwise ask ExtAddrFinder */ { @@ -782,119 +789,159 @@ void p3NetMgrIMPL::netExtCheck() std::cerr << "p3NetMgrIMPL::netExtCheck() checking ExtAddrFinder" << std::endl; #endif bool extFinderOk = mExtAddrFinder->hasValidIP(tmpip); - if (extFinderOk) + if (extFinderOk) { #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) std::cerr << "p3NetMgrIMPL::netExtCheck() Ext supplied by ExtAddrFinder" << std::endl; #endif /* best guess at port */ sockaddr_storage_setport(tmpip, sockaddr_storage_port(mLocalAddr)); - + #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) std::cerr << "p3NetMgrIMPL::netExtCheck() "; std::cerr << "ExtAddr: " << sockaddr_storage_tostring(tmpip); std::cerr << std::endl; #endif - mNetFlags.mExtAddr = tmpip; + //mNetFlags.mExtAddr = tmpip; mNetFlags.mExtAddrOk = true; - address_votes[tmpip].n++ ; + address_votes[tmpip].n++ ; /* XXX HACK TO FIX */ #warning "ALLOWING ExtAddrFinder -> ExtAddrStableOk = true (which it is not normally)" mNetFlags.mExtAddrStableOk = true; + + std::cerr << "ExtAddrFinder reported external address " << sockaddr_storage_iptostring(tmpip) << std::endl; } } } - - /* any other sources ??? */ + + /* also ask peer mgr. */ + if (mPeerMgr) + { +#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) + std::cerr << "p3NetMgrIMPL::netExtCheck() checking mPeerMgr" << std::endl; +#endif + uint8_t isstable ; // unused + sockaddr_storage tmpaddr ; + + if (mPeerMgr->getExtAddressReportedByFriends(tmpaddr, isstable)) + { +#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) + std::cerr << "p3NetMgrIMPL::netExtCheck() Ext supplied by ExtAddrFinder" << std::endl; +#endif + /* best guess at port */ + sockaddr_storage_setport(tmpaddr, sockaddr_storage_port(mLocalAddr)); + +#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) + std::cerr << "p3NetMgrIMPL::netExtCheck() "; + std::cerr << "ExtAddr: " << sockaddr_storage_tostring(tmpip); + std::cerr << std::endl; +#endif + + //mNetFlags.mExtAddr = tmpaddr; + mNetFlags.mExtAddrOk = true; + mNetFlags.mExtAddrStableOk = isstable; + + address_votes[tmpaddr].n++ ; + + std::cerr << "PeerMgr reported external address " << sockaddr_storage_iptostring(tmpaddr) << std::endl; + } +#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) + else + std::cerr << " No reliable address returned." << std::endl; +#endif + } + + /* any other sources ??? */ + /* finalise address */ if (mNetFlags.mExtAddrOk) - { - // look at votes. + { + // look at votes. - std::cerr << "Figuring out ext addr from voting:" << std::endl; - uint32_t admax = 0 ; + std::cerr << "Figuring out ext addr from voting:" << std::endl; + uint32_t admax = 0 ; - for(std::map::const_iterator it(address_votes.begin());it!=address_votes.end();++it) - { - std::cerr << " Vote: " << sockaddr_storage_iptostring(it->first) << " : " << it->second.n << " votes." ; + for(std::map::const_iterator it(address_votes.begin());it!=address_votes.end();++it) + { + std::cerr << " Vote: " << sockaddr_storage_iptostring(it->first) << " : " << it->second.n << " votes." ; - if(it->second.n > admax) - { - mNetFlags.mExtAddr = it->first ; - admax = it->second.n ; + if(it->second.n > admax) + { + mNetFlags.mExtAddr = it->first ; + admax = it->second.n ; - std::cerr << " Kept!" << std::endl; - } - else - std::cerr << " Discarded." << std::endl; - } + std::cerr << " Kept!" << std::endl; + } + else + std::cerr << " Discarded." << std::endl; + } #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) - std::cerr << "p3NetMgrIMPL::netExtCheck() "; - std::cerr << "ExtAddr: " << sockaddr_storage_tostring(mNetFlags.mExtAddr); - std::cerr << std::endl; + std::cerr << "p3NetMgrIMPL::netExtCheck() "; + std::cerr << "ExtAddr: " << sockaddr_storage_tostring(mNetFlags.mExtAddr); + std::cerr << std::endl; #endif - //update ip address list - mExtAddr = mNetFlags.mExtAddr; + //update ip address list + mExtAddr = mNetFlags.mExtAddr; - mNetStatus = RS_NET_DONE; - netSetupDone = true; + mNetStatus = RS_NET_DONE; + netSetupDone = true; #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) - std::cerr << "p3NetMgrIMPL::netExtCheck() Ext Ok: RS_NET_DONE" << std::endl; + std::cerr << "p3NetMgrIMPL::netExtCheck() Ext Ok: RS_NET_DONE" << std::endl; #endif - if (!mNetFlags.mExtAddrStableOk) - { + if (!mNetFlags.mExtAddrStableOk) + { #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET) - std::cerr << "p3NetMgrIMPL::netUdpCheck() UDP Unstable :( "; - std::cerr << std::endl; - std::cerr << "p3NetMgrIMPL::netUdpCheck() We are unreachable"; - std::cerr << std::endl; - std::cerr << "netMode => RS_NET_MODE_UNREACHABLE"; - std::cerr << std::endl; + std::cerr << "p3NetMgrIMPL::netUdpCheck() UDP Unstable :( "; + std::cerr << std::endl; + std::cerr << "p3NetMgrIMPL::netUdpCheck() We are unreachable"; + std::cerr << std::endl; + std::cerr << "netMode => RS_NET_MODE_UNREACHABLE"; + std::cerr << std::endl; #endif - // Due to the new UDP connections - we can still connect some of the time! - // So limit warning! + // Due to the new UDP connections - we can still connect some of the time! + // So limit warning! - //mNetMode &= ~(RS_NET_MODE_ACTUAL); - //mNetMode |= RS_NET_MODE_UNREACHABLE; + //mNetMode &= ~(RS_NET_MODE_ACTUAL); + //mNetMode |= RS_NET_MODE_UNREACHABLE; - /* send a system warning message */ - //pqiNotify *notify = getPqiNotify(); - //if (notify) - { - std::string title = - "Warning: Bad Firewall Configuration"; + /* send a system warning message */ + //pqiNotify *notify = getPqiNotify(); + //if (notify) + { + std::string title = + "Warning: Bad Firewall Configuration"; - std::string msg; - msg += " **** WARNING **** \n"; - msg += "Retroshare has detected that you are behind"; - msg += " a restrictive Firewall\n"; - msg += "\n"; - msg += "You will have limited connectivity to other firewalled peers\n"; - msg += "\n"; - msg += "You can fix this by:\n"; - msg += " (1) opening an External Port\n"; - msg += " (2) enabling UPnP, or\n"; - msg += " (3) get a new (approved) Firewall/Router\n"; + std::string msg; + msg += " **** WARNING **** \n"; + msg += "Retroshare has detected that you are behind"; + msg += " a restrictive Firewall\n"; + msg += "\n"; + msg += "You will have limited connectivity to other firewalled peers\n"; + msg += "\n"; + msg += "You can fix this by:\n"; + msg += " (1) opening an External Port\n"; + msg += " (2) enabling UPnP, or\n"; + msg += " (3) get a new (approved) Firewall/Router\n"; - //notify->AddSysMessage(0, RS_SYS_WARNING, title, msg); + //notify->AddSysMessage(0, RS_SYS_WARNING, title, msg); - std::cerr << msg << std::endl; - } + std::cerr << msg << std::endl; + } - } + } - } + } if (mNetFlags.mExtAddrOk) { diff --git a/libretroshare/src/pqi/p3peermgr.cc b/libretroshare/src/pqi/p3peermgr.cc index dde9d3569..4f9606e45 100644 --- a/libretroshare/src/pqi/p3peermgr.cc +++ b/libretroshare/src/pqi/p3peermgr.cc @@ -1246,20 +1246,51 @@ bool p3PeerMgrIMPL::addCandidateForOwnExternalAddress(const RsPeerId &from, cons // * emit a warnign when the address is unknown // * if multiple peers report the same address => notify the LinkMgr that the external address had changed. - sockaddr_storage addr_filtered ; - sockaddr_storage_copyip(addr_filtered,addr) ; + sockaddr_storage addr_filtered ; + sockaddr_storage_clear(addr_filtered) ; + sockaddr_storage_copyip(addr_filtered,addr) ; #ifdef PEER_DEBUG - std::cerr << "Own external address is " << sockaddr_storage_iptostring(addr_filtered) << ", as reported by friend " << from << std::endl; + std::cerr << "Own external address is " << sockaddr_storage_iptostring(addr_filtered) << ", as reported by friend " << from << std::endl; #endif - if(!sockaddr_storage_isExternalNet(addr_filtered)) + if(!sockaddr_storage_isExternalNet(addr_filtered)) + { +#ifdef PEER_DEBUG + std::cerr << " address is not an external address. Returning false" << std::endl ; +#endif + return false ; + } + + // Update a list of own IPs: + // - remove old values for that same peer + // - remove values for non connected peers + { -#ifdef PEER_DEBUG - std::cerr << " address is not an external address. Returning false" << std::endl ; -#endif - return false ; + RsStackMutex stack(mPeerMtx); /****** STACK LOCK MUTEX *******/ + + mReportedOwnAddresses[from] = addr_filtered ; + + for(std::map::iterator it(mReportedOwnAddresses.begin());it!=mReportedOwnAddresses.end();) + if(!mLinkMgr->isOnline(it->first)) + { + std::map::iterator tmp(it) ; + ++tmp ; + mReportedOwnAddresses.erase(it) ; + it=tmp ; + } + else + ++it ; + + sockaddr_storage current_best_ext_address_guess ; + uint32_t count ; + + locked_computeCurrentBestOwnExtAddressCandidate(current_best_ext_address_guess,count) ; + + std::cerr << "p3PeerMgr:: Current external address is calculated to be: " << sockaddr_storage_iptostring(current_best_ext_address_guess) << " (simultaneously reported by " << count << " peers)." << std::endl; } + + // now current sockaddr_storage own_addr ; @@ -1282,10 +1313,56 @@ bool p3PeerMgrIMPL::addCandidateForOwnExternalAddress(const RsPeerId &from, cons RsServer::notify()->AddFeedItem(RS_FEED_ITEM_SEC_IP_WRONG_EXTERNAL_IP_REPORTED, from.toStdString(), sockaddr_storage_iptostring(own_addr), sockaddr_storage_iptostring(addr)); } + + // we could also sweep over all connected friends and see if some report a different address. return true ; } +bool p3PeerMgrIMPL::locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage& addr, uint32_t& count) +{ + std::map addr_counts ; + + for(std::map::iterator it(mReportedOwnAddresses.begin());it!=mReportedOwnAddresses.end();++it) + ++addr_counts[it->second].n ; + +#ifdef PEER_DEBUG + std::cerr << "Current ext addr statistics:" << std::endl; +#endif + + count = 0 ; + + for(std::map::const_iterator it(addr_counts.begin());it!=addr_counts.end();++it) + { + if(it->second.n > count) + { + addr = it->first ; + count = it->second.n ; + } + +#ifdef PEER_DEBUG + std::cerr << sockaddr_storage_iptostring(it->first) << " : " << it->second.n << std::endl; +#endif + } + + return true ; +} + +bool p3PeerMgrIMPL::getExtAddressReportedByFriends(sockaddr_storage &addr, uint8_t& isstable) +{ + RsStackMutex stack(mPeerMtx); /****** STACK LOCK MUTEX *******/ + + uint32_t count ; + + locked_computeCurrentBestOwnExtAddressCandidate(addr,count) ; + +#ifdef PEER_DEBUG + std::cerr << "Estimation count = " << count << ". Trusted? = " << (count>=2) << std::endl; +#endif + + return count >= 2 ;// 2 is not conservative enough. 3 should be probably better. +} + static bool cleanIpList(std::list& lst,const RsPeerId& pid,p3LinkMgr *link_mgr) { bool changed = false ; diff --git a/libretroshare/src/pqi/p3peermgr.h b/libretroshare/src/pqi/p3peermgr.h index 2af81855a..83963ccd0 100644 --- a/libretroshare/src/pqi/p3peermgr.h +++ b/libretroshare/src/pqi/p3peermgr.h @@ -153,6 +153,7 @@ virtual bool setLocalAddress(const RsPeerId &id, const struct sockaddr_storage virtual bool setExtAddress(const RsPeerId &id, const struct sockaddr_storage &addr) = 0; virtual bool setDynDNS(const RsPeerId &id, const std::string &dyndns) = 0; virtual bool addCandidateForOwnExternalAddress(const RsPeerId& from, const struct sockaddr_storage &addr) = 0; +virtual bool getExtAddressReportedByFriends(struct sockaddr_storage& addr,uint8_t& isstable) = 0; virtual bool setNetworkMode(const RsPeerId &id, uint32_t netMode) = 0; virtual bool setVisState(const RsPeerId &id, uint16_t vs_disc, uint16_t vs_dht) = 0; @@ -200,6 +201,7 @@ virtual int getFriendCount(bool ssl, bool online) = 0; // Single Use Function... shouldn't be here. used by p3serverconfig.cc virtual bool haveOnceConnected() = 0; +virtual bool locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage &addr, uint32_t &count)=0; /*************************************************************************************************/ /*************************************************************************************************/ @@ -256,6 +258,7 @@ virtual bool setLocalAddress(const RsPeerId &id, const struct sockaddr_storage virtual bool setExtAddress(const RsPeerId &id, const struct sockaddr_storage &addr); virtual bool setDynDNS(const RsPeerId &id, const std::string &dyndns); virtual bool addCandidateForOwnExternalAddress(const RsPeerId& from, const struct sockaddr_storage &addr) ; +virtual bool getExtAddressReportedByFriends(struct sockaddr_storage& addr, uint8_t &isstable) ; virtual bool setNetworkMode(const RsPeerId &id, uint32_t netMode); virtual bool setVisState(const RsPeerId &id, uint16_t vs_disc, uint16_t vs_dht); @@ -327,6 +330,7 @@ int getConnectAddresses(const RsPeerId &id, struct sockaddr_storage &lAddr, struct sockaddr_storage &eAddr, pqiIpAddrSet &histAddrs, std::string &dyndns); + protected: /* Internal Functions */ @@ -335,6 +339,8 @@ bool removeBannedIps(); void printPeerLists(std::ostream &out); +virtual bool locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage &addr, uint32_t &count); + protected: /*****************************************************************/ /*********************** p3config ******************************/ @@ -349,7 +355,7 @@ void printPeerLists(std::ostream &out); p3LinkMgrIMPL *mLinkMgr; p3NetMgrIMPL *mNetMgr; - + private: RsMutex mPeerMtx; /* protects below */ @@ -362,6 +368,8 @@ private: std::map mFriendList; // std::map mOthersList; + std::map mReportedOwnAddresses ; + std::list groupList; uint32_t lastGroupId;