From 5ceebe0130a47b963882f9d9815a72b12c3490ee 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: Thu, 31 Oct 2024 14:39:11 +0100 Subject: [PATCH] Fix Secure Discovery Server client disposals guid and handshake_handle assertion (#5257) * Refs #21696: Add regression test for SecurityManager assertion Signed-off-by: Mario Dominguez * Refs #21696: Fix SecurityManager assertion Signed-off-by: Mario Dominguez * Refs #21696: Regression test for guid disposal issue Signed-off-by: Mario Dominguez * Refs #21696: Fix remote disposal guid pdpclient issue Signed-off-by: Mario Dominguez * Refs #21696: Apply Jesus rev Signed-off-by: Mario Dominguez * Refs #21696: Apply NIT Signed-off-by: Mario Dominguez --------- Signed-off-by: Mario Dominguez --- .../discovery/participant/PDPClient.cpp | 11 +- src/cpp/rtps/security/SecurityManager.cpp | 7 + .../dds/communication/security/CMakeLists.txt | 46 ++++- .../communication/security/PublisherMain.cpp | 7 +- .../security/PublisherModule.cpp | 4 + .../security/PublisherModule.hpp | 3 + ...e_ds_pubsub_secure_crypto_communication.py | 181 +++++++++++++----- 7 files changed, 208 insertions(+), 51 deletions(-) diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp index 49faa1d11fa..7247622b785 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp @@ -769,9 +769,16 @@ void PDPClient::announceParticipantState( // if we are matched to a server report demise if (svr.is_connected) { - //locators.push_back(svr.metatrafficMulticastLocatorList); + GuidPrefix_t srv_guid_prefix = svr.guidPrefix; +#if HAVE_SECURITY + if (getRTPSParticipant()->is_secure()) + { + // Need the mangled guid prefix in this case + srv_guid_prefix = get_participant_proxy_data(svr.guidPrefix)->m_guid.guidPrefix; + } +#endif // HAVE_SECURITY locators.push_back(svr.metatrafficUnicastLocatorList); - remote_readers.emplace_back(svr.guidPrefix, + remote_readers.emplace_back(srv_guid_prefix, endpoints->reader.reader_->getGuid().entityId); } } diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index 020b12aa2fd..e24ad5780c8 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -935,6 +935,10 @@ bool SecurityManager::on_process_handshake( } else { + // Return the handshake handle + authentication_plugin_->return_handshake_handle(remote_participant_info->handshake_handle_, + exception); + remote_participant_info->handshake_handle_ = nullptr; EPROSIMA_LOG_ERROR(SECURITY, "WriterHistory cannot add the CacheChange_t"); } } @@ -946,6 +950,9 @@ bool SecurityManager::on_process_handshake( } else { + // Return the handshake handle + authentication_plugin_->return_handshake_handle(remote_participant_info->handshake_handle_, exception); + remote_participant_info->handshake_handle_ = nullptr; EPROSIMA_LOG_ERROR(SECURITY, "WriterHistory cannot retrieve a CacheChange_t"); } } diff --git a/test/dds/communication/security/CMakeLists.txt b/test/dds/communication/security/CMakeLists.txt index 699de565668..2b642d161aa 100644 --- a/test/dds/communication/security/CMakeLists.txt +++ b/test/dds/communication/security/CMakeLists.txt @@ -18,7 +18,7 @@ add_definitions( -DBOOST_ASIO_STANDALONE -DASIO_STANDALONE ) - + include_directories(${Asio_INCLUDE_DIR}) ############################################################################### @@ -393,6 +393,48 @@ if(SECURITY) "PATH=$\\;$\\;${WIN_PATH}") endif() + add_test(NAME SecureDiscoverServerMultipleClientsHandShakeAssertion + COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_BINARY_DIR}/multiple_secure_ds_pubsub_secure_crypto_communication.py + --pub $ + --xml-pub secure_ds_simple_secure_msg_crypto_pub_profile.xml + --sub $ + --xml-sub secure_ds_simple_secure_msg_crypto_sub_profile.xml + --samples 100 #not important to receive all samples + --servers $ + --xml-servers secure_simple_ds_server_profile.xml + --n-clients 30 + --relaunch-clients + --validation-method server) + + # Set test with label NoMemoryCheck + set_property(TEST SecureDiscoverServerMultipleClientsHandShakeAssertion PROPERTY LABELS "NoMemoryCheck") + + if(WIN32) + string(REPLACE ";" "\\;" WIN_PATH "$ENV{PATH}") + set_property(TEST SecureDiscoverServerMultipleClientsHandShakeAssertion APPEND PROPERTY ENVIRONMENT + "PATH=$\\;$\\;${WIN_PATH}") + endif() + + add_test(NAME SecureDiscoverServerMultipleClientsDisposalsReceived + COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_BINARY_DIR}/multiple_secure_ds_pubsub_secure_crypto_communication.py + --pub $ + --xml-pub secure_ds_simple_secure_msg_crypto_pub_profile.xml + --sub $ + --xml-sub secure_ds_simple_secure_msg_crypto_sub_profile.xml + --samples 5 + --servers $ + --xml-servers secure_simple_ds_server_profile.xml + --exit-on-disposal-received) + + # Set test with label NoMemoryCheck + set_property(TEST SecureDiscoverServerMultipleClientsDisposalsReceived PROPERTY LABELS "NoMemoryCheck") + + if(WIN32) + string(REPLACE ";" "\\;" WIN_PATH "$ENV{PATH}") + set_property(TEST SecureDiscoverServerMultipleClientsDisposalsReceived APPEND PROPERTY ENVIRONMENT + "PATH=$\\;$\\;${WIN_PATH}") + endif() + endif() -endif() \ No newline at end of file +endif() diff --git a/test/dds/communication/security/PublisherMain.cpp b/test/dds/communication/security/PublisherMain.cpp index 0d72b20ae5b..2b87543a7d6 100644 --- a/test/dds/communication/security/PublisherMain.cpp +++ b/test/dds/communication/security/PublisherMain.cpp @@ -40,6 +40,7 @@ int main( { int arg_count = 1; bool exit_on_lost_liveliness = false; + bool exit_on_disposal_received = false; bool fixed_type = false; bool zero_copy = false; uint32_t seed = 7800; @@ -55,6 +56,10 @@ int main( { exit_on_lost_liveliness = true; } + else if (strcmp(argv[arg_count], "--exit_on_disposal_received") == 0) + { + exit_on_disposal_received = true; + } else if (strcmp(argv[arg_count], "--fixed_type") == 0) { fixed_type = true; @@ -137,7 +142,7 @@ int main( DomainParticipantFactory::get_instance()->load_XML_profiles_file(xml_file); } - PublisherModule publisher(exit_on_lost_liveliness, fixed_type, zero_copy); + PublisherModule publisher(exit_on_lost_liveliness, exit_on_disposal_received, fixed_type, zero_copy); if (publisher.init(seed, magic)) { diff --git a/test/dds/communication/security/PublisherModule.cpp b/test/dds/communication/security/PublisherModule.cpp index 3d9477ede9b..ca7f60843b4 100644 --- a/test/dds/communication/security/PublisherModule.cpp +++ b/test/dds/communication/security/PublisherModule.cpp @@ -230,6 +230,10 @@ void PublisherModule::on_participant_discovery( { std::cout << "Publisher participant " << // participant->getGuid() << " removed participant " << info.info.m_guid << std::endl; + if (exit_on_disposal_received_) + { + run_ = false; + } } else if (info.status == ParticipantDiscoveryInfo::DROPPED_PARTICIPANT) { diff --git a/test/dds/communication/security/PublisherModule.hpp b/test/dds/communication/security/PublisherModule.hpp index fb144821a0a..e4e99d5743c 100644 --- a/test/dds/communication/security/PublisherModule.hpp +++ b/test/dds/communication/security/PublisherModule.hpp @@ -41,9 +41,11 @@ class PublisherModule PublisherModule( bool exit_on_lost_liveliness, + bool exit_on_disposal_received, bool fixed_type = false, bool zero_copy = false) : exit_on_lost_liveliness_(exit_on_lost_liveliness) + , exit_on_disposal_received_(exit_on_disposal_received) , fixed_type_(zero_copy || fixed_type) // If zero copy active, fixed type is required , zero_copy_(zero_copy) { @@ -91,6 +93,7 @@ class PublisherModule std::condition_variable cv_; unsigned int matched_ = 0; bool exit_on_lost_liveliness_ = false; + bool exit_on_disposal_received_ = false; bool fixed_type_ = false; bool zero_copy_ = false; bool run_ = true; diff --git a/test/dds/communication/security/multiple_secure_ds_pubsub_secure_crypto_communication.py b/test/dds/communication/security/multiple_secure_ds_pubsub_secure_crypto_communication.py index d519558eeac..989f6fb0fe3 100644 --- a/test/dds/communication/security/multiple_secure_ds_pubsub_secure_crypto_communication.py +++ b/test/dds/communication/security/multiple_secure_ds_pubsub_secure_crypto_communication.py @@ -18,6 +18,7 @@ import os import subprocess import sys +import time class ParseOptions(): """Parse arguments.""" @@ -93,9 +94,106 @@ def __parse_args(self): type=str, help='Path to the xml configuration file containing discovery server.' ) + parser.add_argument( + '-nc', + '--n-clients', + type=int, + help='Number of pubsub clients to launch (1 of each).' + ) + parser.add_argument( + '-rc', + '--relaunch-clients', + action='store_true', + help='Whether to kill clients and relaunch them.' + ) + parser.add_argument( + '-vm', + '--validation-method', + type=str, + help='Validation method to use [server, subscriber].' + ) + parser.add_argument( + '-edr', + '--exit-on-disposal-received', + action='store_true', + help='Let the publisher finish the process if receives a disposal.' + ) return parser.parse_args() +def launch_discovery_server_processes(servers, xml_servers): + + ds_procs = [] + + for i in range(0, len(servers)): + server_cmd = [] + + if not os.path.isfile(servers[i]): + print(f'Discovery server executable file does not exists: {servers[i]}') + sys.exit(1) + + if not os.access(servers[i], os.X_OK): + print( + 'Discovery server executable does not have execution permissions:' + f'{servers[i]}') + sys.exit(1) + + server_cmd.append(servers[i]) + server_cmd.extend(['--xml-file', xml_servers[i]]) + server_cmd.extend(['--server-id', str(i)]) + + ds_proc = subprocess.Popen(server_cmd) + print( + 'Running Discovery Server - commmand: ', + ' '.join(map(str, server_cmd))) + + ds_procs.append(ds_proc) + + return ds_procs + +def launch_client_processes(n_clients, pub_command, sub_command): + + pub_procs = [] + sub_procs = [] + + for i in range(0, n_clients): + sub_proc = subprocess.Popen(sub_command) + print( + f'Running Subscriber - commmand: ', + ' '.join(map(str, sub_command))) + + pub_proc = subprocess.Popen(pub_command) + print( + 'Running Publisher - commmand: ', + ' '.join(map(str, pub_command))) + + sub_procs.append(sub_proc) + pub_procs.append(pub_proc) + + return sub_procs, pub_procs + +def cleanup(pub_procs, sub_procs, ds_procs): + + [sub_proc.kill() for sub_proc in sub_procs] + [pub_proc.kill() for pub_proc in pub_procs] + [ds_proc.kill() for ds_proc in ds_procs] + +def cleanup_clients(pub_procs, sub_procs): + + [sub_proc.kill() for sub_proc in sub_procs] + [pub_proc.kill() for pub_proc in pub_procs] + +def terminate(ok = True): + if ok: + try: + sys.exit(os.EX_OK) + except AttributeError: + sys.exit(0) + else: + try: + sys.exit(os.EX_SOFTWARE) + except AttributeError: + sys.exit(1) def run(args): """ @@ -108,6 +206,8 @@ def run(args): """ pub_command = [] sub_command = [] + n_clients = 1 + relaunch_clients = False script_dir = os.path.dirname(os.path.realpath(__file__)) @@ -152,71 +252,60 @@ def run(args): if args.wait: pub_command.extend(['--wait', str(args.wait)]) + if args.exit_on_disposal_received: + pub_command.extend(['--exit_on_disposal_received']) + if args.samples: pub_command.extend(['--samples', str(args.samples)]) sub_command.extend(['--samples', str(args.samples)]) + if args.n_clients: + n_clients = int(args.n_clients) + + if args.relaunch_clients: + relaunch_clients = True + if len(args.servers) != len(args.xml_servers): print( 'Number of servers arguments should be equal to the number of xmls provided.') sys.exit(1) - ds_procs = [] - for i in range(0, len(args.servers)): - server_cmd = [] + ds_procs = launch_discovery_server_processes(args.servers, args.xml_servers) - if not os.path.isfile(args.servers[i]): - print(f'Discovery server executable file does not exists: {args.servers[i]}') - sys.exit(1) + sub_procs, pub_procs = launch_client_processes(n_clients, pub_command, sub_command) - if not os.access(args.servers[i], os.X_OK): - print( - 'Discovery server executable does not have execution permissions:' - f'{args.servers[i]}') - sys.exit(1) + terminate_ok = True - server_cmd.append(args.servers[i]) - server_cmd.extend(['--xml-file', args.xml_servers[i]]) - server_cmd.extend(['--server-id', str(i)]) + if relaunch_clients: + time.sleep(3) - ds_proc = subprocess.Popen(server_cmd) - print( - 'Running Discovery Server - commmand: ', - ' '.join(map(str, server_cmd))) + cleanup_clients(pub_procs, sub_procs) + sub_procs, pub_procs = launch_client_processes(n_clients, pub_command, sub_command) - ds_procs.append(ds_proc) + time.sleep(3) - sub_proc = subprocess.Popen(sub_command) - print( - f'Running Subscriber - commmand: ', - ' '.join(map(str, sub_command))) - - pub_proc = subprocess.Popen(pub_command) - print( - 'Running Publisher - commmand: ', - ' '.join(map(str, pub_command))) - - try: - outs, errs = sub_proc.communicate(timeout=15) - except subprocess.TimeoutExpired: - print('Subscriber process timed out, terminating...') - sub_proc.kill() - pub_proc.kill() - [ds_proc.kill() for ds_proc in ds_procs] + if args.validation_method == 'server': + # Check If discovery servers are still running + for ds_proc in ds_procs: + retcode = ds_proc.poll() + if retcode is not None and retcode is not 0: + print('Discovery Server process dead, terminating...') + terminate_ok = False + else: try: - sys.exit(os.EX_SOFTWARE) - except AttributeError: - sys.exit(1) + for sub_proc in sub_procs: + outs, errs = sub_proc.communicate(timeout=15) + if args.exit_on_disposal_received: + for pub_proc in pub_procs: + outs, errs = pub_proc.communicate(timeout=5) - pub_proc.kill() - ds_proc.kill() - [ds_proc.kill() for ds_proc in ds_procs] - try: - sys.exit(os.EX_OK) - except AttributeError: - sys.exit(0) + except subprocess.TimeoutExpired: + print('Target process timed out, terminating...') + terminate_ok = False + cleanup(pub_procs, sub_procs, ds_procs) + terminate(terminate_ok) if __name__ == '__main__':