From 0c610e923f195dca6da5a296a78d359677c9a451 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 1/2] 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 | 29 +++++-- .../blackbox/common/BlackboxTestsSecurity.cpp | 81 +++++++++++++++++++ 3 files changed, 113 insertions(+), 8 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..a6354ca016c 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,11 @@ void PDPSimple::match_pdp_remote_endpoints( writer->matched_reader_add(*temp_reader_data); } +<<<<<<< HEAD if (BEST_EFFORT_RELIABILITY_QOS == reliability_kind) +======= + if (!writer_only && (dds::BEST_EFFORT_RELIABILITY_QOS == reliability_kind)) +>>>>>>> 3ca60e061 (Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071)) { endpoints->writer.writer_->unsent_changes_reset(); } diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 034fffc2092..9b126b742d8 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -21,6 +21,16 @@ #include #include +<<<<<<< HEAD +======= +#include +#include +#include +#include +#include +#include +#include +>>>>>>> 3ca60e061 (Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071)) #include #include "PubSubReader.hpp" @@ -5014,6 +5024,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() { From 388945f7b0925a9a68f9394f3e211dbdd2f49a45 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Tue, 3 Sep 2024 12:59:44 +0200 Subject: [PATCH 2/2] Fix conflicts Signed-off-by: Miguel Company --- .../rtps/builtin/discovery/participant/PDPSimple.cpp | 6 +----- test/blackbox/common/BlackboxTestsSecurity.cpp | 11 +---------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp index a6354ca016c..1253152a2c1 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp @@ -681,11 +681,7 @@ void PDPSimple::match_pdp_remote_endpoints( writer->matched_reader_add(*temp_reader_data); } -<<<<<<< HEAD - if (BEST_EFFORT_RELIABILITY_QOS == reliability_kind) -======= - if (!writer_only && (dds::BEST_EFFORT_RELIABILITY_QOS == reliability_kind)) ->>>>>>> 3ca60e061 (Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071)) + 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 9b126b742d8..2357490ea80 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -21,16 +21,6 @@ #include #include -<<<<<<< HEAD -======= -#include -#include -#include -#include -#include -#include -#include ->>>>>>> 3ca60e061 (Hotfix: Secure simple participants with `initialpeers` over `TCP` match (#5071)) #include #include "PubSubReader.hpp" @@ -43,6 +33,7 @@ #include #include #include +#include #include