Skip to content

Commit

Permalink
Hotfix: Secure simple participants with initialpeers over TCP mat…
Browse files Browse the repository at this point in the history 
…ch (#5071)

* Refs #20181: Add BB test

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

* Refs #20181: Add Fix

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

* Refs #20181: linter

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

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

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* 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 <miguelcompany@eprosima.com>

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

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

---------

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
  • Loading branch information
@Mario-DL @MiguelCompany
Mario-DL and MiguelCompany authored Aug 27, 2024
1 parent 0ef4841 commit 3ca60e0
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 9 deletions.
27 changes: 19 additions & 8 deletions src/cpp/rtps/builtin/discovery/participant/PDPSimple.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,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
Expand Down Expand Up @@ -554,7 +558,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);
}
}
Expand All @@ -564,8 +568,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());
Expand Down Expand Up @@ -597,7 +606,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
{
Expand All @@ -610,7 +620,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);
}
}
Expand All @@ -620,7 +630,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);
Expand Down Expand Up @@ -657,7 +668,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();

Expand Down Expand Up @@ -715,7 +726,7 @@ void PDPSimple::match_pdp_remote_endpoints(
writer->matched_reader_add_edp(*temp_reader_data);
}

if (dds::BEST_EFFORT_RELIABILITY_QOS == reliability_kind)
if (!writer_only && (dds::BEST_EFFORT_RELIABILITY_QOS == reliability_kind))
{
endpoints->writer.writer_->unsent_changes_reset();
}
Expand Down
11 changes: 10 additions & 1 deletion src/cpp/rtps/builtin/discovery/participant/PDPSimple.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
72 changes: 72 additions & 0 deletions test/blackbox/common/BlackboxTestsSecurity.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <fastdds/rtps/common/EntityId_t.hpp>
#include <fastdds/rtps/transport/shared_mem/SharedMemTransportDescriptor.hpp>
#include <fastdds/rtps/transport/test_UDPv4TransportDescriptor.hpp>
#include <fastdds/utils/IPFinder.hpp>
#include <gtest/gtest.h>

#include "../utils/filter_helpers.hpp"
Expand Down Expand Up @@ -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()
{
Expand Down

0 comments on commit 3ca60e0

Please sign in to comment.