From bddba1ee7c5d8655479bdcc4da873162bc1f2227 Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Mon, 1 Feb 2021 16:46:04 -0500
Subject: [PATCH] upnp: Always use published address as reflexive candidates
The external address of the IGD might not be always routable if
the router is not directly connected to the internet.
So we need to always add the published address returned by the DHT even if UPNP
is enabled and actif. Failing to do so, and if the peer use only relay candidates,
the TURN will reject all incoming connections from our public address because
it were not in the candidates list and thus not authorized by TURN.
The automatic reallocation of failing mappings was also improved.
Change-Id: I5fb1424d5dc61b990595c229be647099b017eba1
Gitlab: #335
---
src/account.cpp | 16 ++--
src/account.h | 8 +-
src/ice_transport.cpp | 186 +++++++++++++++++++++---------------
src/jamidht/jamiaccount.cpp | 61 ++++++++----
src/jamidht/jamiaccount.h | 5 +-
src/sip/sipaccount.cpp | 4 +-
src/sip/sipaccountbase.cpp | 12 ++-
src/sip/sipaccountbase.h | 2 +-
src/upnp/upnp_context.cpp | 60 ++++++++----
src/upnp/upnp_context.h | 7 ++
src/upnp/upnp_control.cpp | 10 ++
src/upnp/upnp_control.h | 1 +
12 files changed, 241 insertions(+), 131 deletions(-)
diff --git a/src/account.cpp b/src/account.cpp
index ee28a9b383..8eac6fece9 100644
--- a/src/account.cpp
+++ b/src/account.cpp
@@ -155,10 +155,10 @@ Account::enableUpnp(bool state)
{
std::lock_guard<std::mutex> lk {upnp_mtx};
- if (state and !upnp_)
- upnp_.reset(new upnp::Controller());
- else if (!state and upnp_)
- upnp_.reset();
+ if (state and !upnpCtrl_)
+ upnpCtrl_.reset(new upnp::Controller());
+ else if (!state and upnpCtrl_)
+ upnpCtrl_.reset();
}
void
@@ -529,8 +529,8 @@ IpAddr
Account::getUPnPIpAddress() const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
- if (upnp_)
- return upnp_->getExternalIP();
+ if (upnpCtrl_)
+ return upnpCtrl_->getExternalIP();
return {};
}
@@ -542,8 +542,8 @@ bool
Account::getUPnPActive() const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
- if (upnp_)
- return upnp_->hasValidIGD();
+ if (upnpCtrl_)
+ return upnpCtrl_->hasValidIGD();
return false;
}
diff --git a/src/account.h b/src/account.h
index 0fca8e383c..f11a500761 100644
--- a/src/account.h
+++ b/src/account.h
@@ -324,7 +324,7 @@ public:
void addDefaultModerator(const std::string& peerURI);
void removeDefaultModerator(const std::string& peerURI);
- bool isLocalModeratorsEnabled() const { return localModeratorsEnabled_; }
+ bool isLocalModeratorsEnabled() const { return localModeratorsEnabled_; }
void enableLocalModerators(bool isModEnabled) { localModeratorsEnabled_ = isModEnabled; }
public: // virtual methods that has to be implemented by concrete classes
@@ -343,9 +343,9 @@ private:
std::mutex callIDSetMtx_;
std::set<std::string> callIDSet_;
- void enableUpnp(bool state);
-
protected:
+ virtual void enableUpnp(bool state);
+
static void parseString(const std::map<std::string, std::string>& details,
const char* key,
std::string& s);
@@ -515,7 +515,7 @@ protected:
*/
bool upnpEnabled_;
mutable std::mutex upnp_mtx {};
- std::shared_ptr<jami::upnp::Controller> upnp_;
+ std::shared_ptr<jami::upnp::Controller> upnpCtrl_;
std::set<std::string> defaultModerators_ {};
bool localModeratorsEnabled_;
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 914b756806..146eaceae8 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -120,10 +120,16 @@ public:
static const char* getCandidateType(const pj_ice_sess_cand* cand);
bool isTcpEnabled() const { return config_.protocol == PJ_ICE_TP_TCP; }
bool addStunConfig(int af);
- void setupDefaultCandidates();
+ void addDefaultCandidates();
void requestUpnpMappings();
bool hasUpnp() const;
- void setupServerReflexiveCandidates();
+ // Take a list of address pairs (local/public) and add them as
+ // reflexive candidates using STUN config.
+ void addServerReflexiveCandidates(const std::vector<std::pair<IpAddr, IpAddr>>& addrList);
+ // Generate server reflexive candidates using the published (DHT/Account) address
+ const std::vector<std::pair<IpAddr, IpAddr>> setupGenericReflexiveCandidates();
+ // Generate server reflexive candidates using UPNP mappings.
+ const std::vector<std::pair<IpAddr, IpAddr>> setupUpnpReflexiveCandidates();
void setDefaultRemoteAddress(unsigned comp_id, const IpAddr& addr);
const IpAddr& getDefaultRemoteAddress(unsigned comp_id) const;
bool handleEvents(unsigned max_msec);
@@ -337,12 +343,14 @@ IceTransport::Impl::Impl(const char* name,
peerChannels_.resize(component_count_ + 1);
- setupDefaultCandidates();
+ addDefaultCandidates();
- if (upnp_)
- requestUpnpMappings();
+ addServerReflexiveCandidates(setupGenericReflexiveCandidates());
- setupServerReflexiveCandidates();
+ if (upnp_) {
+ requestUpnpMappings();
+ addServerReflexiveCandidates(setupUpnpReflexiveCandidates());
+ }
pool_.reset(
pj_pool_create(iceTransportFactory.getPoolFactory(), "IceTransport.pool", 512, 512, NULL));
@@ -544,9 +552,9 @@ IceTransport::Impl::handleEvents(unsigned max_msec)
void
IceTransport::Impl::onComplete(pj_ice_strans* ice_st, pj_ice_strans_op op, pj_status_t status)
{
- const char* opname = op == PJ_ICE_STRANS_OP_INIT
- ? "initialization"
- : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation" : "unknown_op";
+ const char* opname = op == PJ_ICE_STRANS_OP_INIT ? "initialization"
+ : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation"
+ : "unknown_op";
const bool done = status == PJ_SUCCESS;
if (done) {
@@ -742,7 +750,7 @@ IceTransport::Impl::addStunConfig(int af)
stun.af = af;
stun.conn_type = config_.stun.conn_type;
- JAMI_DBG("[ice:%p)] added host stun config for %s transport",
+ JAMI_DBG("[ice:%p] added host stun config for %s transport",
this,
config_.protocol == PJ_ICE_TP_TCP ? "TCP" : "UDP");
@@ -750,7 +758,7 @@ IceTransport::Impl::addStunConfig(int af)
}
void
-IceTransport::Impl::setupDefaultCandidates()
+IceTransport::Impl::addDefaultCandidates()
{
JAMI_DBG("[ice:%p]: Setup default candidates", this);
@@ -790,11 +798,11 @@ IceTransport::Impl::requestUpnpMappings()
std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
auto ret = upnpMappings_.emplace(mapPtr->getMapKey(), *mapPtr);
if (ret.second) {
- JAMI_DBG("[ice:%p]: UPNP mapping %s successfully allocated",
+ JAMI_DBG("[ice:%p] UPNP mapping %s successfully allocated",
this,
mapPtr->toString(true).c_str());
} else {
- JAMI_WARN("[ice:%p]: UPNP mapping %s already in the list!",
+ JAMI_WARN("[ice:%p] UPNP mapping %s already in the list!",
this,
mapPtr->toString().c_str());
}
@@ -812,70 +820,14 @@ IceTransport::Impl::hasUpnp() const
}
void
-IceTransport::Impl::setupServerReflexiveCandidates()
+IceTransport::Impl::addServerReflexiveCandidates(
+ const std::vector<std::pair<IpAddr, IpAddr>>& addrList)
{
- JAMI_DBG("[ice:%p]: Setup server reflexive candidates", this);
-
- std::vector<std::pair<IpAddr, IpAddr>> addrList;
- auto hasUpnpCand = hasUpnp();
- auto isTcp = isTcpEnabled();
-
- // Server reflexive candidates are built using UPNP mappings if enabled and
- // available. Otherwise, the published address will be used instead. This
- // address is given with the other ICE options.
- // For TCP transport, the connection type is set to passive for UPNP
- // candidates and set to active otherwise.
- if (hasUpnpCand) {
- std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
- unsigned compId = 1;
- for (auto const& [_, map] : upnpMappings_) {
- assert(map.getMapKey());
- IpAddr localAddr {map.getInternalAddress()};
- localAddr.setPort(map.getInternalPort());
- IpAddr publicAddr {map.getExternalAddress()};
- publicAddr.setPort(map.getExternalPort());
- addrList.emplace_back(localAddr, publicAddr);
-
- JAMI_DBG("[ice:%p]: Using upnp mapping %s (%s) -> %s (%s) for comp %u",
- this,
- localAddr.toString(true).c_str(),
- localAddr.getFamily() == pj_AF_INET()
- ? "IPv4"
- : localAddr.getFamily() == pj_AF_INET6() ? "IPv6" : "unknown",
- publicAddr.toString(true).c_str(),
- publicAddr.getFamily() == pj_AF_INET()
- ? "IPv4"
- : localAddr.getFamily() == pj_AF_INET6() ? "IPv6" : "unknown",
- compId);
- compId++;
- }
- } else {
- auto localAddr = accountLocalAddr_;
- auto publicAddr = accountPublicAddr_;
- if (localAddr and publicAddr) {
- for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
- uint16_t port = 0;
- if (isTcp and not hasUpnpCand)
- port = 9;
- else
- port = upnp::Controller::generateRandomPort(isTcp ? PortType::TCP
- : PortType::UDP);
-
- localAddr.setPort(port);
- publicAddr.setPort(port);
- addrList.emplace_back(localAddr, publicAddr);
-
- JAMI_DBG("[ice:%p]: Using Account address [%s : %s] for comp %u",
- this,
- localAddr.toString(true).c_str(),
- publicAddr.toString(true).c_str(),
- compIdx + 1);
- }
- }
- }
-
if (addrList.size() != component_count_) {
- JAMI_WARN("[ice:%p]: No DHT or UPNP server reflexive candidates added", this);
+ JAMI_WARN("[ice:%p]: Provided addr list size %lu does not match component count %u",
+ this,
+ addrList.size(),
+ component_count_);
return;
}
@@ -889,14 +841,15 @@ IceTransport::Impl::setupServerReflexiveCandidates()
for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
auto localAddr = addrList[compIdx].first;
auto publicAddr = addrList[compIdx].second;
+
pj_sockaddr_cp(&stun.cfg.user_mapping[compIdx].local_addr, localAddr.pjPtr());
pj_sockaddr_cp(&stun.cfg.user_mapping[compIdx].mapped_addr, publicAddr.pjPtr());
- if (isTcp) {
- if (hasUpnpCand) {
- stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_PASSIVE;
- } else {
+ if (isTcpEnabled()) {
+ if (publicAddr.getPort() == 9) {
stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_ACTIVE;
+ } else {
+ stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_PASSIVE;
}
} else {
stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_UDP;
@@ -907,6 +860,81 @@ IceTransport::Impl::setupServerReflexiveCandidates()
assert(stun.cfg.user_mapping_cnt < PJ_ICE_MAX_COMP);
}
+const std::vector<std::pair<IpAddr, IpAddr>>
+IceTransport::Impl::setupGenericReflexiveCandidates()
+{
+ std::vector<std::pair<IpAddr, IpAddr>> addrList;
+ auto isTcp = isTcpEnabled();
+
+ // First, set default server reflexive candidates using current
+ // account public address.
+ // Then, check for UPNP mappings and and them if available.
+ // For TCP transport, the connection type is set to passive for UPNP
+ // candidates and set to active otherwise.
+
+ if (accountLocalAddr_ and accountPublicAddr_) {
+ for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
+ // For TCP, the type is set to active, because most likely the incoming
+ // connection will be blocked by the NAT.
+ // For UDP use random port number.
+ uint16_t port = isTcp ? 9
+ : upnp::Controller::generateRandomPort(isTcp ? PortType::TCP
+ : PortType::UDP);
+
+ accountLocalAddr_.setPort(port);
+ accountPublicAddr_.setPort(port);
+ addrList.emplace_back(accountLocalAddr_, accountPublicAddr_);
+
+ JAMI_DBG("[ice:%p]: Add generic local reflexive candidates [%s : %s] for comp %u",
+ this,
+ accountLocalAddr_.toString(true).c_str(),
+ accountPublicAddr_.toString(true).c_str(),
+ compIdx + 1);
+ }
+ }
+
+ return addrList;
+}
+
+const std::vector<std::pair<IpAddr, IpAddr>>
+IceTransport::Impl::setupUpnpReflexiveCandidates()
+{
+ // Add UPNP server reflexive candidates if available.
+ if (not hasUpnp())
+ return {};
+
+ std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
+
+ if (static_cast<unsigned>(upnpMappings_.size()) < component_count_) {
+ JAMI_WARN("[ice:%p]: Not enough mappings %lu. Expected %u",
+ this,
+ upnpMappings_.size(),
+ component_count_);
+ return {};
+ }
+
+ std::vector<std::pair<IpAddr, IpAddr>> addrList;
+
+ unsigned compId = 1;
+ for (auto const& [_, map] : upnpMappings_) {
+ assert(map.getMapKey());
+ IpAddr localAddr {map.getInternalAddress()};
+ localAddr.setPort(map.getInternalPort());
+ IpAddr publicAddr {map.getExternalAddress()};
+ publicAddr.setPort(map.getExternalPort());
+ addrList.emplace_back(localAddr, publicAddr);
+
+ JAMI_DBG("[ice:%p]: Add UPNP local reflexive candidates [%s : %s] for comp %u",
+ this,
+ localAddr.toString(true).c_str(),
+ publicAddr.toString(true).c_str(),
+ compId);
+ compId++;
+ }
+
+ return addrList;
+}
+
void
IceTransport::Impl::setDefaultRemoteAddress(unsigned comp_id, const IpAddr& addr)
{
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index a1357123d3..a111671c3f 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -680,16 +680,9 @@ JamiAccount::onConnectedOutgoingCall(const std::shared_ptr<SIPCall>& call,
const auto localAddress = ip_utils::getInterfaceAddr(shared->getLocalInterface(),
target.getFamily());
- IpAddr addrSdp;
- if (shared->getUPnPActive()) {
- // use UPnP addr, or published addr if its set
- addrSdp = shared->getPublishedSameasLocal() ? shared->getUPnPIpAddress()
- : shared->getPublishedIpAddress();
- } else {
- addrSdp = shared->isStunEnabled() or (not shared->getPublishedSameasLocal())
- ? shared->getPublishedIpAddress()
- : localAddress;
- }
+ IpAddr addrSdp = shared->getPublishedSameasLocal()
+ ? localAddress
+ : shared->getPublishedIpAddress(target.getFamily());
// fallback on local address
if (not addrSdp)
@@ -1804,12 +1797,12 @@ JamiAccount::registerAsyncOps()
loadCachedProxyServer([onLoad](const std::string&) { onLoad(); });
- if (upnp_) {
+ if (upnpCtrl_) {
JAMI_DBG("UPnP: Attempting to map ports for Jami account");
// Release current mapping if any.
if (dhtUpnpMapping_.isValid()) {
- upnp_->releaseMapping(dhtUpnpMapping_);
+ upnpCtrl_->releaseMapping(dhtUpnpMapping_);
}
dhtUpnpMapping_ = upnp::Mapping {0, 0, upnp::PortType::UDP};
@@ -1875,7 +1868,7 @@ JamiAccount::registerAsyncOps()
});
// Request the mapping.
- auto map = upnp_->reserveMapping(dhtUpnpMapping_);
+ auto map = upnpCtrl_->reserveMapping(dhtUpnpMapping_);
// The returned mapping is invalid. Load the account now since
// we may never receive the callback.
if (not map or not map->isValid()) {
@@ -1913,7 +1906,7 @@ JamiAccount::doRegister()
setRegistrationState(RegistrationState::TRYING);
/* if UPnP is enabled, then wait for IGD to complete registration */
- if (upnp_ or proxyServerCached_.empty()) {
+ if (upnpCtrl_ or proxyServerCached_.empty()) {
registerAsyncOps();
} else {
doRegister_();
@@ -1922,6 +1915,35 @@ JamiAccount::doRegister()
cacheTurnServers(); // reset cache for TURN servers
}
+void
+JamiAccount::enableUpnp(bool enable)
+{
+ std::lock_guard<std::mutex> lk {upnp_mtx};
+
+ if (enable) {
+ auto const& addr = getPublishedIpAddress();
+
+ if (upnpCtrl_ and upnpCtrl_->getExternalIP() == addr) {
+ JAMI_DBG(
+ "[Account %s] Already have a valid controller instance with external address [%s]",
+ getAccountID().c_str(),
+ upnpCtrl_->getExternalIP().toString().c_str());
+ return;
+ }
+
+ JAMI_DBG("[Account %s] Creating a new upnp controller instance", getAccountID().c_str());
+ // Need a known IPv4 address to validate the external address
+ // returned by the IGD.
+ if (addr and addr.getFamily() == AF_INET) {
+ upnpCtrl_.reset(new upnp::Controller(addr));
+ } else {
+ upnpCtrl_.reset(new upnp::Controller());
+ }
+ } else {
+ upnpCtrl_.reset();
+ }
+}
+
std::vector<std::string>
JamiAccount::loadBootstrap() const
{
@@ -2564,8 +2586,8 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb)
dht_->join();
// Release current upnp mapping if any.
- if (upnp_ and dhtUpnpMapping_.isValid()) {
- upnp_->releaseMapping(dhtUpnpMapping_);
+ if (upnpCtrl_ and dhtUpnpMapping_.isValid()) {
+ upnpCtrl_->releaseMapping(dhtUpnpMapping_);
dhtUpnpMapping_ = upnp::Mapping {};
}
@@ -2590,7 +2612,7 @@ JamiAccount::connectivityChanged()
return;
}
// Let UPNP handle connectivity change if enabled.
- if (not upnp_)
+ if (not upnpCtrl_)
dht_->connectivityChanged();
// reset cache
setPublishedAddress({});
@@ -3352,6 +3374,11 @@ JamiAccount::storeActiveIpAddress()
JAMI_DBG("[Account %s] Store DHT public IPv4 address : %s",
getAccountID().c_str(),
publicAddr.toString().c_str());
+
+ // Check if we need to create a new UPNP controller instance.
+ if (upnpEnabled_ and (not upnpCtrl_ or not upnpCtrl_->hasValidIGD())) {
+ enableUpnp(true);
+ }
}
// IPv6
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index dfaadb497f..843e1c9dce 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -186,6 +186,7 @@ public:
*/
void doUnregister(std::function<void(bool)> cb = {}) override;
+ void enableUpnp(bool enable) override;
/**
* @return pj_str_t "From" uri based on account information.
* From RFC3261: "The To header field first and foremost specifies the desired
@@ -648,8 +649,8 @@ private:
*/
in_port_t dhtPortUsed()
{
- return (upnp_ and dhtUpnpMapping_.isValid()) ? dhtUpnpMapping_.getExternalPort()
- : dhtDefaultPort_;
+ return (upnpCtrl_ and dhtUpnpMapping_.isValid()) ? dhtUpnpMapping_.getExternalPort()
+ : dhtDefaultPort_;
}
/* Current UPNP mapping */
diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp
index 53c9473b80..b5b4beec1d 100644
--- a/src/sip/sipaccount.cpp
+++ b/src/sip/sipaccount.cpp
@@ -770,7 +770,7 @@ SIPAccount::mapPortUPnP()
}
});
- auto mapRes = upnp_->reserveMapping(map);
+ auto mapRes = upnpCtrl_->reserveMapping(map);
if (mapRes and mapRes->getState() == upnp::MappingState::OPEN) {
return true;
}
@@ -789,7 +789,7 @@ SIPAccount::doRegister()
JAMI_DBG("doRegister %s", hostname_.c_str());
/* if UPnP is enabled, then wait for IGD to complete registration */
- if (upnp_) {
+ if (upnpCtrl_) {
JAMI_DBG("UPnP: waiting for IGD to register SIP account");
setRegistrationState(RegistrationState::TRYING);
if (not mapPortUPnP()) {
diff --git a/src/sip/sipaccountbase.cpp b/src/sip/sipaccountbase.cpp
index aac4c79fb1..08f08e27c1 100644
--- a/src/sip/sipaccountbase.cpp
+++ b/src/sip/sipaccountbase.cpp
@@ -602,9 +602,17 @@ SIPAccountBase::setMessageDisplayed(const std::string& contactId,
}
IpAddr
-SIPAccountBase::getPublishedIpAddress() const
+SIPAccountBase::getPublishedIpAddress(uint16_t family) const
{
- // Prefere IPv4 if available. More likely to succeed behind NAT.
+ if (family == AF_INET)
+ return publishedIp_[0];
+ if (family == AF_INET6)
+ return publishedIp_[1];
+
+ assert(family == AF_UNSPEC);
+
+ // If family is not set, prefere IPv4 if available. It's more
+ // likely to succeed behind NAT.
if (publishedIp_[0])
return publishedIp_[0];
if (publishedIp_[1])
diff --git a/src/sip/sipaccountbase.h b/src/sip/sipaccountbase.h
index 20c3d9e73d..8472a65aca 100644
--- a/src/sip/sipaccountbase.h
+++ b/src/sip/sipaccountbase.h
@@ -184,7 +184,7 @@ public:
*/
std::string getPublishedAddress() const { return publishedIpAddress_; }
- IpAddr getPublishedIpAddress() const;
+ IpAddr getPublishedIpAddress(uint16_t family = PF_UNSPEC) const;
void setPublishedAddress(const IpAddr& ip_addr);
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index 7c88161624..09ade02c30 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -164,6 +164,20 @@ UPnPContext::connectivityChanged()
StopUpnp();
StartUpnp();
+
+ processMappingWithAutoUpdate();
+}
+
+void
+UPnPContext::setPublicAddress(const IpAddr& addr)
+{
+ if (not addr)
+ return;
+
+ JAMI_DBG("Setting the known public address to %s", addr.toString().c_str());
+
+ std::lock_guard<std::mutex> lock(mappingMutex_);
+ knownPublicAddress_ = std::move(addr);
}
bool
@@ -250,7 +264,8 @@ UPnPContext::releaseMapping(const Mapping& map)
auto mapPtr = getMappingWithKey(map.getMapKey());
if (not mapPtr) {
- JAMI_WARN("Trying to release an unknown mapping %s", map.toString().c_str());
+ // Might happen if the mapping failed
+ JAMI_DBG("Mapping %s does not exist or was already removed", map.toString().c_str());
return false;
}
@@ -285,7 +300,6 @@ UPnPContext::registerController(void* controller)
}
JAMI_DBG("Successfully registered controller %p", this);
-
if (not started_)
StartUpnp();
}
@@ -453,7 +467,6 @@ UPnPContext::deleteUnneededMappings(PortType type, int portCount)
// mappings than required. So, all mappings in a state other
// than "OPEN" state (typically in in-progress state) will
// be deleted as well.
- deleteMapping(map);
it = unregisterMapping(it);
} else {
it++;
@@ -773,6 +786,8 @@ UPnPContext::processMappingWithAutoUpdate()
registerMapping(newMapping);
// Release the old one.
map->setAvailable(true);
+ map->enableAutoUpdate(false);
+ map->setNotifyCallback(nullptr);
unregisterMapping(map);
}
}
@@ -789,25 +804,34 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
CHECK_VALID_THREAD();
- auto const& igdAddr = igd->getLocalIp();
+ auto const& igdLocalAddr = igd->getLocalIp();
auto protocolName = igd->getProtocolName();
// Check if IGD has a valid addresses.
- if (not igdAddr) {
+ if (not igdLocalAddr) {
JAMI_ERR("[%s] IGD has an invalid local address", protocolName);
return;
}
+
if (not igd->getPublicIp()) {
- JAMI_ERR("[%s] IGD IGD has an invalid public address", protocolName);
+ JAMI_ERR("[%s] IGD has an invalid public address", protocolName);
return;
}
- char const* IgdState = event == UpnpIgdEvent::ADDED
- ? "ADDED"
- : event == UpnpIgdEvent::REMOVED ? "REMOVED" : "INVALID";
+ if (knownPublicAddress_ and igd->getPublicIp() != knownPublicAddress_) {
+ JAMI_WARN("[%s] IGD external address [%s] does not match known public address [%s]."
+ " The mapped addresses might not be reachable",
+ protocolName,
+ igd->getPublicIp().toString().c_str(),
+ knownPublicAddress_.toString().c_str());
+ }
+
+ char const* IgdState = event == UpnpIgdEvent::ADDED ? "ADDED"
+ : event == UpnpIgdEvent::REMOVED ? "REMOVED"
+ : "INVALID";
JAMI_WARN("State of IGD %s [%s] changed to [%s]",
- igdAddr.toString(true, true).c_str(),
+ igdLocalAddr.toString(true, true).c_str(),
protocolName,
IgdState);
@@ -827,12 +851,12 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
if (ret.second) {
JAMI_DBG("IGD [%s] on address %s was added. Will process any pending requests",
protocolName,
- igdAddr.toString(true, true).c_str());
+ igdLocalAddr.toString(true, true).c_str());
} else {
// Already in the list.
JAMI_ERR("IGD [%s] on address %s already in the list",
protocolName,
- igdAddr.toString(true, true).c_str());
+ igdLocalAddr.toString(true, true).c_str());
return;
}
}
@@ -876,10 +900,10 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
if (it == mappingList.end()) {
// We may receive a response for a canceled request. Just
// ignore it.
- JAMI_WARN("Mapping request %s from IGD %s [%s] does not match local request",
- mapRes.toString().c_str(),
- igd->getLocalIp().toString().c_str(),
- mapRes.getProtocolName());
+ JAMI_DBG("Mapping request %s from IGD %s [%s] does not match local request",
+ mapRes.toString().c_str(),
+ igd->getLocalIp().toString().c_str(),
+ mapRes.getProtocolName());
return;
}
key = it->second->getMapKey();
@@ -1078,6 +1102,10 @@ UPnPContext::unregisterMapping(std::map<Mapping::key_t, Mapping::sharedPtr_t>::i
void
UPnPContext::unregisterMapping(const Mapping::sharedPtr_t& map)
{
+ if (map->getAutoUpdate()) {
+ // Dont unregister mappings with auto-update enabled.
+ return;
+ }
auto& mappingList = getMappingList(map->getType());
if (mappingList.erase(map->getMapKey()) == 1) {
diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h
index 8d98d6cef4..566e87b61d 100644
--- a/src/upnp/upnp_context.h
+++ b/src/upnp/upnp_context.h
@@ -105,6 +105,9 @@ public:
// Retrieve the UPnPContext singleton
static std::shared_ptr<UPnPContext> getUPnPContext();
+ // Set the known public address
+ void setPublicAddress(const IpAddr& addr);
+
// Check if there is a valid IGD in the IGD list.
bool hasValidIGD() const;
@@ -245,6 +248,10 @@ private:
bool started_ {false};
+ // The known public address. The external addresses returned by
+ // the IGDs will be checked against this address.
+ IpAddr knownPublicAddress_ {};
+
// Set of registered controllers
std::set<void*> controllerList_;
diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp
index 69829e0b80..72048aa23d 100644
--- a/src/upnp/upnp_control.cpp
+++ b/src/upnp/upnp_control.cpp
@@ -38,6 +38,16 @@ Controller::Controller()
JAMI_DBG("Controller@%p: Created UPnP Controller session", this);
}
+Controller::Controller(const IpAddr& addr)
+ : Controller()
+{
+ assert(upnpContext_);
+ assert(addr);
+ assert(addr.getFamily() == AF_INET);
+
+ upnpContext_->setPublicAddress(addr);
+}
+
Controller::~Controller()
{
JAMI_DBG("Controller@%p: Destroying UPnP Controller session", this);
diff --git a/src/upnp/upnp_control.h b/src/upnp/upnp_control.h
index 920feb4ac4..87714f12e8 100644
--- a/src/upnp/upnp_control.h
+++ b/src/upnp/upnp_control.h
@@ -47,6 +47,7 @@ class Controller
{
public:
Controller();
+ Controller(const IpAddr& addr);
~Controller();
// Checks if a valid IGD is available.
--
GitLab