Skip to content

[23042] Bugfix: Properly delete secure endpoints if registration fails #5814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/cpp/rtps/participant/RTPSParticipantImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,7 @@ bool RTPSParticipantImpl::create_writer(
if (!m_security_manager.register_local_writer(SWriter->getGuid(),
param.endpoint.properties, SWriter->getAttributes().security_attributes()))
{
SWriter->local_actions_on_writer_removed();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a call to local_actions_on_writer_removed() on the StatefulWriter Destructor to ensure that events are always destroyed, instead of manually calling it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local_actions_on_writer_removed() comes from the former deinit() that was explicitly moved out from the StatefulPersistentWriter destructor, in this commit

However I dont see any reason why we should not call it from the StatafulWriter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling virtual functions inside destructors is dangerous. We could have a proxy method that receives a pointer to a writer and performs the call to local_actions_on_writer_removed() and then deletes the writer

delete(SWriter);
return false;
}
Expand All @@ -984,6 +985,7 @@ bool RTPSParticipantImpl::create_writer(
if (!m_security_manager.register_local_builtin_writer(SWriter->getGuid(),
SWriter->getAttributes().security_attributes()))
{
SWriter->local_actions_on_writer_removed();
delete(SWriter);
return false;
}
Expand Down Expand Up @@ -1111,6 +1113,7 @@ bool RTPSParticipantImpl::create_reader(
if (!m_security_manager.register_local_reader(SReader->getGuid(),
param.endpoint.properties, SReader->getAttributes().security_attributes()))
{
SReader->local_actions_on_reader_removed();
delete(SReader);
return false;
}
Expand All @@ -1120,6 +1123,7 @@ bool RTPSParticipantImpl::create_reader(
if (!m_security_manager.register_local_builtin_reader(SReader->getGuid(),
SReader->getAttributes().security_attributes()))
{
SReader->local_actions_on_reader_removed();
delete(SReader);
return false;
}
Expand Down
2 changes: 2 additions & 0 deletions test/blackbox/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ target_include_directories(BlackboxTests_RTPS PRIVATE
${Asio_INCLUDE_DIR})
target_link_libraries(BlackboxTests_RTPS fastdds fastcdr foonathan_memory GTest::gtest)
gtest_discover_tests(BlackboxTests_RTPS
PROPERTIES
ENVIRONMENT "CERTS_PATH=${PROJECT_SOURCE_DIR}/test/certs"
TEST_PREFIX "BlackboxTests_RTPS."
TEST_FILTER ${BLACKBOX_HIGH_LEVEL_IGNORED_TESTS}
NO_PRETTY_VALUES
Expand Down
7 changes: 7 additions & 0 deletions test/blackbox/common/RTPSBlackboxTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ using namespace eprosima::fastdds::rtps;

//#define cout "Use Log instead!"

const char* certs_path = nullptr;
uint16_t global_port = 0;
//bool enable_datasharing;

Expand Down Expand Up @@ -119,5 +120,11 @@ int main(
testing::InitGoogleTest(&argc, argv);
testing::AddGlobalTestEnvironment(new BlackboxEnvironment);

if (!::testing::GTEST_FLAG(list_tests))
{
#if HAVE_SECURITY
blackbox_security_init();
#endif // if HAVE_SECURITY
}
return RUN_ALL_TESTS();
}
100 changes: 100 additions & 0 deletions test/blackbox/common/RTPSBlackboxTestsSecurity.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Copyright 2025 Proyectos y Sistemas de Mantenimiento SL (eProsima).
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "BlackboxTests.hpp"

#if HAVE_SECURITY

#include <chrono>
#include <cstdint>
#include <memory>
#include <thread>

#include <gtest/gtest.h>

#include <fastdds/dds/log/Log.hpp>
#include <fastdds/rtps/attributes/RTPSParticipantAttributes.hpp>
#include <fastdds/rtps/participant/RTPSParticipant.hpp>
#include <fastdds/rtps/RTPSDomain.hpp>

#include "RTPSWithRegistrationReader.hpp"
#include "RTPSWithRegistrationWriter.hpp"

using namespace eprosima::fastdds;
using namespace eprosima::fastdds::rtps;


void set_authentication_config(
rtps::PropertySeq& properties)
{
properties.emplace_back("dds.sec.auth.plugin", "builtin.PKI-DH");
properties.emplace_back("dds.sec.auth.builtin.PKI-DH.identity_ca",
"file://" + std::string(certs_path) + "/maincacert.pem");
properties.emplace_back("dds.sec.auth.builtin.PKI-DH.identity_certificate",
"file://" + std::string(certs_path) + "/mainpubcert.pem");
properties.emplace_back("dds.sec.auth.builtin.PKI-DH.private_key",
"file://" + std::string(certs_path) + "/mainpubkey.pem");
}

void set_participant_crypto_config(
rtps::PropertySeq& props,
const std::string& governance_file = "governance_helloworld_all_enable.smime",
const std::string& permissions_file = "permissions_helloworld.smime")
{
props.emplace_back("dds.sec.crypto.plugin", "builtin.AES-GCM-GMAC");
props.emplace_back(Property("dds.sec.access.plugin",
"builtin.Access-Permissions"));
props.emplace_back(Property(
"dds.sec.access.builtin.Access-Permissions.permissions_ca",
"file://" + std::string(certs_path) + "/maincacert.pem"));
props.emplace_back(Property("dds.sec.access.builtin.Access-Permissions.governance",
"file://" + std::string(certs_path) + "/" + governance_file));
props.emplace_back(Property("dds.sec.access.builtin.Access-Permissions.permissions",
"file://" + std::string(certs_path) + "/" + permissions_file));
}

/**
* This test checks that a StatefulWriter created with security,
* but with a wrong permissions on topic, is properly cleaned up
*/
TEST(RTPSSecurityTests, statefulwriter_wrong_permissions_properly_cleaned_up)
{
RTPSWithRegistrationWriter<HelloWorldPubSubType> writer("CustomTopicName");
RTPSWithRegistrationReader<HelloWorldPubSubType> reader("CustomTopicName");

PropertyPolicy security_properties;
set_authentication_config(security_properties.properties());
set_participant_crypto_config(security_properties.properties());

writer.add_participant_properties(security_properties);
reader.add_participant_properties(security_properties);

// This should fail but properly exit
ASSERT_NO_THROW(writer.init());
ASSERT_NO_THROW(reader.init());
}

void blackbox_security_init()
{
certs_path = std::getenv("CERTS_PATH");

if (certs_path == nullptr)
{
std::cout << "Cannot get enviroment variable CERTS_PATH" << std::endl;
exit(-1);
}
}

#endif // if HAVE_SECURITY

7 changes: 7 additions & 0 deletions test/blackbox/common/RTPSWithRegistrationReader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,13 @@ class RTPSWithRegistrationReader
return *this;
}

RTPSWithRegistrationReader& add_participant_properties(
const eprosima::fastdds::rtps::PropertyPolicy& props)
{
participant_attr_.properties = props;
return *this;
}

RTPSWithRegistrationReader& persistence_guid_att(
const eprosima::fastdds::rtps::GuidPrefix_t& guidPrefix,
const eprosima::fastdds::rtps::EntityId_t& entityId)
Expand Down
7 changes: 7 additions & 0 deletions test/blackbox/common/RTPSWithRegistrationWriter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,13 @@ class RTPSWithRegistrationWriter
return *this;
}

RTPSWithRegistrationWriter& add_participant_properties(
const eprosima::fastdds::rtps::PropertyPolicy& props)
{
participant_attr_.properties = props;
return *this;
}

RTPSWithRegistrationWriter& persistence_guid_att(
const eprosima::fastdds::rtps::GuidPrefix_t& guidPrefix,
const eprosima::fastdds::rtps::EntityId_t& entityId)
Expand Down