From 83581f0be026c6c9c95c413fefffb4ab46102b6f Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:25:58 +0200 Subject: [PATCH] Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071) (#5177) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071) * Refs #20181: Add BB test Signed-off-by: Mario Dominguez * Refs #20181: Add Fix Signed-off-by: Mario Dominguez * Refs #20181: linter Signed-off-by: Mario Dominguez * Refs #20181. Pass in secure_endpoints as lambda capture. Signed-off-by: Miguel Company * 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 * Refs #20181. Unmatch non-secure before matching secure. Signed-off-by: Miguel Company --------- Signed-off-by: Mario Dominguez Signed-off-by: Miguel Company Co-authored-by: Miguel Company (cherry picked from commit 3ca60e061deb1ab6e5b85ad49a5337ccecd124d4) * Fix conflicts Signed-off-by: Miguel Company --------- Signed-off-by: Miguel Company Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Miguel Company --- .../builtin/discovery/participant/PDPSimple.h | 11 ++- .../discovery/participant/PDPSimple.cpp | 27 ++++--- src/cpp/rtps/security/SecurityManager.cpp | 1 + .../blackbox/common/BlackboxTestsSecurity.cpp | 72 +++++++++++++++++++ 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h b/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h index 88405d2c337..5bcc0816ec4 100644 --- a/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h +++ b/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h @@ -136,7 +136,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, diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp index 4fa156335d4..1253152a2c1 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp @@ -303,7 +303,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 @@ -509,7 +513,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); } } @@ -519,8 +523,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(builtin_endpoints_.get()); @@ -552,7 +561,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 { @@ -565,7 +575,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); } } @@ -575,7 +585,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(notify_secure_endpoints); @@ -612,7 +623,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(); @@ -670,7 +681,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(); } diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index d5c6269196d..af825ff0ae6 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -105,6 +105,7 @@ SecurityManager::SecurityManager( participant->getRTPSParticipantAttributes().allocation.locators.max_multicast_locators, participant->getRTPSParticipantAttributes().allocation.data_limits}) { + std::cout << __func__ << std::endl; assert(participant != nullptr); static OpenSSLInit openssl_init; } diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 034fffc2092..2357490ea80 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include @@ -5014,6 +5015,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 tcp_client("HelloWorldTopic_TCP"); + PubSubReader 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(); + 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(); + 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() {