Fix an issue with client filtering tests (#62)

* Fix an issue with client filtering tests. Direct connected devices were skipping filtering test number 2

* Version 1.2.17

---------
Co-authored-by: EC2 Default User <ec2-user@ip-172-31-10-101.us-east-2.compute.internal>

HISTORY

@@ -46,3 +46,6 @@ Version 1.2.14 - Docker support and OpenSSL compile issue
 Version 1.2.15 - For for --ddp command line option.
 
 Version 1.2.16 - ASLR Support on Cygwin builds 
+
+Version 1.2.17 - Client fix for filtering test (does not impact server)
+

README

 STUNTMAN - An open source STUN server
-Version 1.2.16
+Version 1.2.17
 April 7, 2020
 ---------------------------------------------------------
 

client/clientmain.cpp

@@ -256,38 +256,6 @@ Cleanup:
 }
 
 
-
-
-void NatBehaviorToString(NatBehavior behavior, std::string* pStr)
-{
-    std::string& str = *pStr;
-
-    switch (behavior)
-    {
-        case UnknownBehavior:  str="Unknown Behavior"; break;
-        case DirectMapping:  str="Direct Mapping"; break;
-        case EndpointIndependentMapping: str = "Endpoint Independent Mapping"; break;
-        case AddressDependentMapping: str = "Address Dependent Mapping"; break;
-        case AddressAndPortDependentMapping: str = "Address and Port Dependent Mapping"; break;
-        default: ASSERT(false); str = ""; break;
-    }
-}
-
-void NatFilteringToString(NatFiltering filtering, std::string* pStr)
-{
-    std::string& str = *pStr;
-
-    switch (filtering)
-    {
-        case UnknownFiltering:  str="Unknown Behavior"; break;
-        case DirectConnectionFiltering:  str="Direct Mapping"; break;
-        case EndpointIndependentFiltering: str = "Endpoint Independent Filtering"; break;
-        case AddressDependentFiltering: str = "Address Dependent Filtering"; break;
-        case AddressAndPortDependentFiltering: str = "Address and Port Dependent Filtering"; break;
-        default: ASSERT(false); str = ""; break;
-    }
-}
-
 void DumpResults(StunClientLogicConfig& config, StunClientResults& results)
 {
     char szBuffer[100];
@@ -310,7 +278,7 @@ void DumpResults(StunClientLogicConfig& config, StunClientResults& results)
         Logging::LogMsg(LL_ALWAYS, "Behavior test: %s", results.fBehaviorTestSuccess?"success":"fail");
         if (results.fBehaviorTestSuccess)
         {
-            NatBehaviorToString(results.behavior, &strResult);
+            strResult = NatBehaviorToString(results.behavior);
             Logging::LogMsg(LL_ALWAYS, "Nat behavior: %s", strResult.c_str());
         }
     }
@@ -320,7 +288,7 @@ void DumpResults(StunClientLogicConfig& config, StunClientResults& results)
         Logging::LogMsg(LL_ALWAYS, "Filtering test: %s", results.fFilteringTestSuccess?"success":"fail");
         if (results.fFilteringTestSuccess)
         {
-            NatFilteringToString(results.filtering, &strResult);
+            strResult = NatFilteringToString(results.filtering);
             Logging::LogMsg(LL_ALWAYS, "Nat filtering: %s", strResult.c_str());
         }
     }

common.inc

-#BOOST_INCLUDE := -I/home/jselbie/boost_1_72_0
+#BOOST_INCLUDE := -I/home/jselbie/boost_1_73_0
 #OPENSSL_INCLUDE := -I/Users/jselbie/openssl/include
 
 DEFINES := -DNDEBUG

resources/README

 STUNTMAN - An open source STUN server
-Version 1.2.16
+Version 1.2.17
 April 7, 2020
 ---------------------------------------------------------
 

resources/readme.src

 STUNTMAN - An open source STUN server
-Version 1.2.16
+Version 1.2.17
 April 7, 2020
 ---------------------------------------------------------
 

stuncore/stunclientlogic.cpp

@@ -232,7 +232,7 @@ HRESULT CStunClientLogic::ProcessResponse(CRefCountedBuffer& spMsg, CSocketAddre
 
     hr = pCurrentTest->ProcessResponse(spMsg, addrRemote, addrLocal);
     // this likely puts the test into the completed state
-    // A subsequent call to GetNextMessage will invoke the next tset
+    // A subsequent call to GetNextMessage will invoke the next test
 
 Cleanup:
     return hr;

stuncore/stunclientlogic.h

@@ -18,10 +18,8 @@
 #ifndef STUNCLIENTLOGIC_H
 #define	STUNCLIENTLOGIC_H
 
-
 #include "stunclienttests.h"
-
-
+#include "stuntypes.h"
 
 struct StunClientLogicConfig
 {
@@ -36,24 +34,6 @@ struct StunClientLogicConfig
 };
 
 
-enum NatBehavior
-{
-    UnknownBehavior,
-    DirectMapping,                  // IP address and port are the same between client and server view (NO NAT)
-    EndpointIndependentMapping,     // same mapping regardless of IP:port original packet sent to (the kind of NAT we like)
-    AddressDependentMapping,        // mapping changes for local socket based on remote IP address only, but remote port can change (partially symmetric, not great)
-    AddressAndPortDependentMapping  // different port mapping if the ip address or port change (symmetric NAT, difficult to predict port mappings)
-};
-
-enum NatFiltering
-{
-    UnknownFiltering,
-    DirectConnectionFiltering,
-    EndpointIndependentFiltering,    // shouldn't be common unless connection is already direct (can receive on mapped address from anywhere regardless of where the original send went)
-    AddressDependentFiltering,       // IP-restricted NAT
-    AddressAndPortDependentFiltering // port-restricted NAT
-};
-
 struct StunClientResults
 {
     // basic binding test will set these results

stuncore/stunclienttests.cpp

@@ -341,21 +341,6 @@ CFilteringTest::CFilteringTest()
 }
 
 
-void CFilteringTest::PreRunCheck()
-{
-    // if the binding test detected "direct", there's nothing for us to do except declare this as "endpoint indedepent"
-    if (_fIsTest3 == false)
-    {
-        if (_pResults->fBindingTestSuccess && _pResults->fIsDirect)
-        {
-            _fCompleted = true;
-            _pResults->filtering = ::EndpointIndependentFiltering;
-            _pResults->fFilteringTestSuccess = true;
-        }
-    }
-}
-
-
 bool CFilteringTest::IsReadyToRun()
 {
     // we can run if the CBasicBindingTest succeeded and we have an "other" address

stuncore/stunclienttests.h

@@ -100,8 +100,7 @@ protected:
     
 public:
     CFilteringTest();
-    
-    void PreRunCheck();
+
     bool IsReadyToRun();
     HRESULT GetMessage(CRefCountedBuffer& spMsg, CSocketAddress* pAddrDest) ;
     HRESULT ProcessResponse(CRefCountedBuffer& spMsg, CSocketAddress& addrRemote, CSocketAddress& addrLocal) ;

stuncore/stuntypes.h

@@ -178,5 +178,23 @@ const uint32_t STUN_HEADER_SIZE = 20;
 const uint32_t MAX_STUN_MESSAGE_SIZE = 800; // some reasonable length
 const uint32_t MAX_STUN_ATTRIBUTE_SIZE = 780; // more than reasonable
 
+enum NatBehavior
+{
+    UnknownBehavior,
+    DirectMapping,                  // IP address and port are the same between client and server view (NO NAT)
+    EndpointIndependentMapping,     // same mapping regardless of IP:port original packet sent to (the kind of NAT we like)
+    AddressDependentMapping,        // mapping changes for local socket based on remote IP address only, but remote port can change (partially symmetric, not great)
+    AddressAndPortDependentMapping  // different port mapping if the ip address or port change (symmetric NAT, difficult to predict port mappings)
+};
+
+enum NatFiltering
+{
+    UnknownFiltering,
+    DirectConnectionFiltering,
+    EndpointIndependentFiltering,    // shouldn't be common unless connection is already direct (can receive on mapped address from anywhere regardless of where the original send went)
+    AddressDependentFiltering,       // IP-restricted NAT
+    AddressAndPortDependentFiltering // port-restricted NAT
+};
+
 
 #endif

stuncore/stunutils.cpp

@@ -85,3 +85,35 @@ Cleanup:
 
 }
 
+std::string NatBehaviorToString(NatBehavior behavior)
+{
+    std::string str;
+
+    switch (behavior)
+    {
+        case UnknownBehavior:  str="Unknown Behavior"; break;
+        case DirectMapping:  str="Direct Mapping"; break;
+        case EndpointIndependentMapping: str = "Endpoint Independent Mapping"; break;
+        case AddressDependentMapping: str = "Address Dependent Mapping"; break;
+        case AddressAndPortDependentMapping: str = "Address and Port Dependent Mapping"; break;
+        default: ASSERT(false); str = "Unknown NAT Behavior"; break;
+    }
+
+    return str;
+}
+
+std::string NatFilteringToString(NatFiltering filtering)
+{
+    std::string str;
+
+    switch (filtering)
+    {
+        case UnknownFiltering:  str="Unknown Filtering"; break;
+        case DirectConnectionFiltering:  str="Direct Mapping (Filtering)"; break;
+        case EndpointIndependentFiltering: str = "Endpoint Independent Filtering"; break;
+        case AddressDependentFiltering: str = "Address Dependent Filtering"; break;
+        case AddressAndPortDependentFiltering: str = "Address and Port Dependent Filtering"; break;
+        default: ASSERT(false); str = "Unknown NAT Filtering"; break;
+    }
+    return str;
+}
\ No newline at end of file

stuncore/stunutils.h

@@ -24,4 +24,7 @@ bool IsTransactionIdValid(StunTransactionId& transid);
 HRESULT GetXorMappedAddress(uint8_t* pData, size_t size, StunTransactionId &transid, CSocketAddress* pAddr);
 HRESULT GetMappedAddress(uint8_t* pData, size_t size, CSocketAddress* pAddr);
 
+std::string NatBehaviorToString(NatBehavior behavior);
+std::string NatFilteringToString(NatFiltering filtering);
+
 #endif

testcode/testclientlogic.cpp

@@ -23,36 +23,44 @@
 #include "testmessagehandler.h"
 
 #include "testclientlogic.h"
+#include "stuntypes.h"
 
 
 HRESULT CTestClientLogic::Run()
 {
     HRESULT hr = S_OK;
+    std::vector<NatBehavior> behaviorTests = {DirectMapping, EndpointIndependentMapping, AddressDependentMapping, AddressAndPortDependentMapping};
+    std::vector<NatFiltering> filteringTests = {EndpointIndependentFiltering, AddressDependentFiltering, AddressAndPortDependentFiltering};
+
     
     ChkA(Test1());
-    
-    printf("Testing detection for DirectMapping\n");
-    ChkA(TestBehaviorAndFiltering(true, DirectMapping, false, EndpointIndependentFiltering));
-    
-    printf("Testing detection for EndpointIndependent mapping\n");
-    ChkA(TestBehaviorAndFiltering(true, EndpointIndependentMapping, false, EndpointIndependentFiltering));
-    
-    printf("Testing detection for AddressDependentMapping\n");
-    ChkA(TestBehaviorAndFiltering(true, AddressDependentMapping, false, EndpointIndependentFiltering));
-    
-    printf("Testing detection for AddressAndPortDependentMapping\n");
-    ChkA(TestBehaviorAndFiltering(true, AddressAndPortDependentMapping, false, EndpointIndependentFiltering));
-    
-    printf("Testing detection for EndpointIndependentFiltering\n");
-    ChkA(TestBehaviorAndFiltering(true, EndpointIndependentMapping, true, EndpointIndependentFiltering));
 
-    printf("Testing detection for AddressDependentFiltering\n");
-    ChkA(TestBehaviorAndFiltering(true, EndpointIndependentMapping, true, AddressDependentFiltering));
-    
-    printf("Testing detection for AddressAndPortDependentFiltering\n");
-    ChkA(TestBehaviorAndFiltering(true, EndpointIndependentMapping, true, AddressAndPortDependentFiltering));
-    
-    
+
+    for (size_t i = 0; i < behaviorTests.size(); i++)
+    {
+        std::string testName = NatBehaviorToString(behaviorTests[i]);
+        printf("Testing Behavior detection for %s\n", testName.c_str());
+        ChkA(TestBehaviorAndFiltering(true, behaviorTests[i], false, AddressAndPortDependentFiltering));
+    }
+
+    for (size_t i = 0; i < filteringTests.size(); i++)
+    {
+        std::string testName = NatFilteringToString(filteringTests[i]);
+        printf("Testing Filtering detection for %s\n", testName.c_str());
+        ChkA(TestBehaviorAndFiltering(false, EndpointIndependentMapping, true, filteringTests[i]));
+    }
+
+    for (size_t b = 0; b < behaviorTests.size(); b++)
+    {
+        for (size_t f = 0; f < filteringTests.size(); f++)
+        {
+             std::string behaviorName = NatBehaviorToString(behaviorTests[b]);
+             std::string filteringName = NatFilteringToString(filteringTests[f]);
+             std::string testDescription = "Testing mix of [" + behaviorName + "] + [" + filteringName+"]";
+             printf("%s\n", testDescription.c_str());
+             ChkA(TestBehaviorAndFiltering(true, behaviorTests[b], true, filteringTests[f]));
+        }
+    }
     
 Cleanup:
     return hr;
@@ -384,7 +392,14 @@ HRESULT CTestClientLogic::TestBehaviorAndFiltering(bool fBehaviorTest, NatBehavi
     results.Init(); // zero it out
     _spClientLogic->GetResults(&results);
     
-    ChkIfA(results.behavior != behavior, E_UNEXPECTED);
+    if (fBehaviorTest)
+    {
+        ChkIfA(results.behavior != behavior, E_UNEXPECTED);
+    }
+    if (fFilteringTest)
+    {
+        ChkIfA(results.filtering != filtering, E_UNEXPECTED);
+    }
     
 Cleanup: