From b71ee5a32e05c2afc393b0c31b17d3690de14ea2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Dom=C3=ADnguez=20L=C3=B3pez?= <116071334+Mario-DL@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:49:01 +0200 Subject: [PATCH] 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) # Conflicts: # src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp # test/blackbox/common/BlackboxTestsSecurity.cpp --- .../builtin/discovery/participant/PDPSimple.h | 11 ++- .../discovery/participant/PDPSimple.cpp | 27 ++++--- .../blackbox/common/BlackboxTestsSecurity.cpp | 72 +++++++++++++++++++ 3 files changed, 101 insertions(+), 9 deletions(-) diff --git a/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h b/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h index dadfcc8c7b0..a770803c81a 100644 --- a/include/fastdds/rtps/builtin/discovery/participant/PDPSimple.h +++ b/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, diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp index 42cb70ba1e3..79753461b6a 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp +++ b/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(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(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(); } diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index a4f9afc5a65..944a9d166cf 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #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 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() {