Hotfix: Secure simple participants with initialpeers over TCP match (#5071) (#5176)

* Refs #20181: Add BB test

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20181: Add Fix

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20181: linter

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20181. Pass in secure_endpoints as lambda capture.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20181. New approach.

Automatically sending DATA(p) when receiving a DATA(p) could lead to an infinite ping-pong between the two participants.
This resulted in some cases in the transport threads eating all CPU resources.

The new approach matches the discovered participant to the builtin non-secure PDP writer, so it will receive the DATA(p) of the local participant in the next periodic announcement.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20181. Unmatch non-secure before matching secure.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Mario Dominguez <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Co-authored-by: Miguel Company <[email protected]>
(cherry picked from commit 3ca60e0)

# Conflicts:
#	src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp
#	test/blackbox/common/BlackboxTestsSecurity.cpp

Co-authored-by: Mario Domínguez López <[email protected]>

Author: mergify[bot]

include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h

@@ -143,7 +143,16 @@ class PDPSimple : public PDP
 
     void match_pdp_remote_endpoints(
             const ParticipantProxyData& pdata,
-            bool notify_secure_endpoints);
+            bool notify_secure_endpoints,
+            bool writer_only);
+
+    /**
+     * @brief Unmatch PDP endpoints with a remote participant.
+     *
+     * @param participant_guid GUID of the remote participant.
+     */
+    void unmatch_pdp_remote_endpoints(
+            const GUID_t& participant_guid);
 
     void assign_low_level_remote_endpoints(
             const ParticipantProxyData& pdata,

src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp

@@ -304,7 +304,11 @@ bool PDPSimple::createPDPEndpoints()
         secure_endpoints->secure_reader.listener_.reset(new PDPListener(this));
 
         endpoints = secure_endpoints;
-        endpoints->reader.listener_.reset(new PDPSecurityInitiatorListener(this));
+        endpoints->reader.listener_.reset(new PDPSecurityInitiatorListener(this,
+                [this](const ParticipantProxyData& participant_data)
+                {
+                    match_pdp_remote_endpoints(participant_data, false, true);
+                }));
     }
     else
 #endif  // HAVE_SECURITY
@@ -550,7 +554,7 @@ void PDPSimple::assignRemoteEndpoints(
         {
             // This participant is not secure.
             // Match PDP and other builtin endpoints.
-            match_pdp_remote_endpoints(*pdata, false);
+            match_pdp_remote_endpoints(*pdata, false, false);
             assign_low_level_remote_endpoints(*pdata, false);
         }
     }
@@ -560,8 +564,13 @@ void PDPSimple::removeRemoteEndpoints(
         ParticipantProxyData* pdata)
 {
     EPROSIMA_LOG_INFO(RTPS_PDP, "For RTPSParticipant: " << pdata->m_guid);
+    unmatch_pdp_remote_endpoints(pdata->m_guid);
+}
 
-    GUID_t guid = pdata->m_guid;
+void PDPSimple::unmatch_pdp_remote_endpoints(
+        const GUID_t& participant_guid)
+{
+    GUID_t guid = participant_guid;
 
     {
         auto endpoints = dynamic_cast<fastdds::rtps::SimplePDPEndpoints*>(builtin_endpoints_.get());
@@ -593,7 +602,8 @@ void PDPSimple::notifyAboveRemoteEndpoints(
 {
     if (notify_secure_endpoints)
     {
-        match_pdp_remote_endpoints(pdata, true);
+        unmatch_pdp_remote_endpoints(pdata.m_guid);
+        match_pdp_remote_endpoints(pdata, true, false);
     }
     else
     {
@@ -606,7 +616,7 @@ void PDPSimple::notifyAboveRemoteEndpoints(
             notify_and_maybe_ignore_new_participant(part_data, ignored);
             if (!ignored)
             {
-                match_pdp_remote_endpoints(*part_data, false);
+                match_pdp_remote_endpoints(*part_data, false, false);
                 assign_low_level_remote_endpoints(*part_data, false);
             }
         }
@@ -616,7 +626,8 @@ void PDPSimple::notifyAboveRemoteEndpoints(
 
 void PDPSimple::match_pdp_remote_endpoints(
         const ParticipantProxyData& pdata,
-        bool notify_secure_endpoints)
+        bool notify_secure_endpoints,
+        bool writer_only)
 {
 #if !HAVE_SECURITY
     static_cast<void>(notify_secure_endpoints);
@@ -653,7 +664,7 @@ void PDPSimple::match_pdp_remote_endpoints(
     }
 #endif // HAVE_SECURITY
 
-    if (0 != (endp & pdp_writer_mask))
+    if (!writer_only && (0 != (endp & pdp_writer_mask)))
     {
         auto temp_writer_data = get_temporary_writer_proxies_pool().get();
 
@@ -711,7 +722,7 @@ void PDPSimple::match_pdp_remote_endpoints(
             writer->matched_reader_add(*temp_reader_data);
         }
 
-        if (BEST_EFFORT_RELIABILITY_QOS == reliability_kind)
+        if (!writer_only && (BEST_EFFORT_RELIABILITY_QOS == reliability_kind))
         {
             endpoints->writer.writer_->unsent_changes_reset();
         }

test/blackbox/common/BlackboxTestsSecurity.cpp

@@ -21,6 +21,7 @@
 #include <fstream>
 #include <map>
 
+#include <fastrtps/utils/IPFinder.h>
 #include <gtest/gtest.h>
 
 #include "PubSubReader.hpp"
@@ -5016,6 +5017,77 @@ TEST(Security, ValidateAuthenticationHandshakeProperties)
     ASSERT_TRUE(auth_elapsed_time < max_time);
 }
 
+// Regression test for Redmine issue #20181
+// Two simple secure participants with tcp transport and initial peers must match.
+// It basically tests that the PDPSecurityInitiatorListener
+// in PDPSimple answers back with the proxy data.
+TEST(Security, security_with_initial_peers_over_tcpv4_correctly_behaves)
+{
+    // Create
+    PubSubWriter<HelloWorldPubSubType> tcp_client("HelloWorldTopic_TCP");
+    PubSubReader<HelloWorldPubSubType> tcp_server("HelloWorldTopic_TCP");
+
+    // Search for a valid WAN address
+    LocatorList_t all_locators;
+    Locator_t wan_locator;
+    IPFinder::getIP4Address(&all_locators);
+
+    for (auto& locator : all_locators)
+    {
+        if (!IPLocator::isLocal(locator))
+        {
+            wan_locator = locator;
+            break;
+        }
+    }
+
+    uint16_t server_listening_port = 11810;
+    wan_locator.port = server_listening_port;
+    wan_locator.kind = LOCATOR_KIND_TCPv4;
+
+    auto tcp_client_transport_descriptor = std::make_shared<eprosima::fastdds::rtps::TCPv4TransportDescriptor>();
+    LocatorList_t initial_peers;
+    initial_peers.push_back(wan_locator);
+    tcp_client.disable_builtin_transport()
+            .add_user_transport_to_pparams(tcp_client_transport_descriptor)
+            .initial_peers(initial_peers);
+
+    auto tcp_server_transport_descriptor = std::make_shared<eprosima::fastdds::rtps::TCPv4TransportDescriptor>();
+    tcp_server_transport_descriptor->listening_ports.push_back(server_listening_port);
+    IPLocator::copyIPv4(wan_locator, tcp_server_transport_descriptor->wan_addr);
+
+    std::cout << "SETTING WAN address to " <<  wan_locator << std::endl;
+
+    tcp_server.disable_builtin_transport()
+            .add_user_transport_to_pparams(tcp_server_transport_descriptor);
+
+    // Configure security
+    const std::string governance_file("governance_helloworld_all_enable.smime");
+    const std::string permissions_file("permissions_helloworld.smime");
+    CommonPermissionsConfigure(tcp_server, tcp_client, governance_file, permissions_file);
+
+    tcp_server.init();
+    tcp_client.init();
+
+    ASSERT_TRUE(tcp_server.isInitialized());
+    ASSERT_TRUE(tcp_client.isInitialized());
+
+    tcp_server.waitAuthorized();
+    tcp_client.waitAuthorized();
+
+    tcp_server.wait_discovery();
+    tcp_client.wait_discovery();
+
+    ASSERT_TRUE(tcp_server.is_matched());
+    ASSERT_TRUE(tcp_client.is_matched());
+
+    auto data = default_helloworld_data_generator();
+    tcp_server.startReception(data);
+    tcp_client.send(data);
+    ASSERT_TRUE(data.empty());
+    tcp_server.block_for_all(std::chrono::seconds(10));
+}
+
 
 void blackbox_security_init()
 {