From 31d2bd38aee6637e9776b6181d8f217cf3f7f7ab Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Thu, 22 Feb 2024 08:19:41 +0100 Subject: [PATCH 1/9] Refs #20439: Blackbox tests Signed-off-by: Mario Dominguez --- test/blackbox/CMakeLists.txt | 2 + .../api/fastrtps_deprecated/PubSubReader.hpp | 2 + .../api/fastrtps_deprecated/PubSubWriter.hpp | 2 + .../blackbox/auth_handshake_props_profile.xml | 57 +++++++++ .../blackbox/common/BlackboxTestsSecurity.cpp | 110 ++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 test/blackbox/auth_handshake_props_profile.xml diff --git a/test/blackbox/CMakeLists.txt b/test/blackbox/CMakeLists.txt index 3ae980a2ac6..103c942e800 100644 --- a/test/blackbox/CMakeLists.txt +++ b/test/blackbox/CMakeLists.txt @@ -180,6 +180,8 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/partitions_profile.xml ${CMAKE_CURRENT_BINARY_DIR}/partitions_profile.xml) configure_file(${CMAKE_CURRENT_SOURCE_DIR}/builtin_transports_profile.xml ${CMAKE_CURRENT_BINARY_DIR}/builtin_transports_profile.xml) +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/auth_handshake_props_profile.xml + ${CMAKE_CURRENT_BINARY_DIR}/auth_handshake_props_profile.xml COPYONLY) file(COPY "${CMAKE_CURRENT_SOURCE_DIR}/datagrams" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") if(FASTRTPS_API_TESTS) diff --git a/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp b/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp index 3ec3f834d4e..9eafaab8da2 100644 --- a/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp +++ b/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp @@ -353,6 +353,8 @@ class PubSubReader eprosima::fastrtps::Domain::removeParticipant(participant_); participant_ = nullptr; } + + initialized_ = false; } std::list data_not_received() diff --git a/test/blackbox/api/fastrtps_deprecated/PubSubWriter.hpp b/test/blackbox/api/fastrtps_deprecated/PubSubWriter.hpp index 33f6e857175..cf7fbe4f3b6 100644 --- a/test/blackbox/api/fastrtps_deprecated/PubSubWriter.hpp +++ b/test/blackbox/api/fastrtps_deprecated/PubSubWriter.hpp @@ -381,6 +381,8 @@ class PubSubWriter eprosima::fastrtps::Domain::removeParticipant(participant_); participant_ = nullptr; } + + initialized_ = false; } void send( diff --git a/test/blackbox/auth_handshake_props_profile.xml b/test/blackbox/auth_handshake_props_profile.xml new file mode 100644 index 00000000000..7cb8d4b1421 --- /dev/null +++ b/test/blackbox/auth_handshake_props_profile.xml @@ -0,0 +1,57 @@ + + + + + 0 + + + + + + dds.sec.auth.plugin + builtin.PKI-DH + + + + dds.sec.auth.builtin.PKI-DH.identity_ca + file://${CERTS_PATH}/maincacert.pem + + + dds.sec.auth.builtin.PKI-DH.identity_certificate + file://${CERTS_PATH}/mainpubcert.pem + + + dds.sec.auth.builtin.PKI-DH.private_key + file://${CERTS_PATH}/mainpubkey.pem + + + + dds.sec.crypto.plugin + builtin.AES-GCM-GMAC + + + dds.sec.access.builtin.Access-Permissions.governance + file://${CERTS_PATH}/governance_helloworld_all_enable.smime + + + dds.sec.access.builtin.Access-Permissions.permissions + file://${CERTS_PATH}/permissions_helloworld.smime + + + dds.sec.auth.builtin.PKI-DH.max_handshake_requests + 10 + + + dds.sec.auth.builtin.PKI-DH.initial_handshake_resend_period + 100 + + + dds.sec.auth.builtin.PKI-DH.handshake_resend_period_gain + 1.1 + + + + + + + diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 3d2378b5a81..8d10c5281d0 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -4817,6 +4817,116 @@ TEST_P(Security, MaliciousParticipantRemovalIgnore) reader.block_for_all(); } +TEST(Security, ValidateAuthenticationHandshakePropertiesParsing) +{ + PubSubWriter writer(TEST_TOPIC_NAME); + + PropertyPolicy property_policy; + + property_policy.properties().emplace_back(Property("dds.sec.auth.plugin", + "builtin.PKI-DH")); + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.identity_ca", + "file://" + std::string(certs_path) + "/maincacert.pem")); + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.identity_certificate", + "file://" + std::string(certs_path) + "/mainsubcert.pem")); + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.private_key", + "file://" + std::string(certs_path) + "/mainsubkey.pem")); + property_policy.properties().emplace_back(Property("dds.sec.crypto.plugin", + "builtin.AES-GCM-GMAC")); + + // max_handshake_requests out of bounds + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.max_handshake_requests", + "0")); + + writer.property_policy(property_policy).init(); + + // Writer creation should fail + ASSERT_FALSE(writer.isInitialized()); + + writer.destroy(); + + property_policy.properties().pop_back(); + + // initial_handshake_resend_period out of bounds + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.initial_handshake_resend_period", + "-200")); + + writer.property_policy(property_policy).init(); + + // Writer creation should fail + ASSERT_FALSE(writer.isInitialized()); + + writer.destroy(); + + property_policy.properties().pop_back(); + + // handshake_resend_period_gain out of bounds + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.handshake_resend_period_gain", + "0.5")); + + writer.property_policy(property_policy).init(); + + // Writer creation should fail + ASSERT_FALSE(writer.isInitialized()); + + writer.destroy(); + + property_policy.properties().pop_back(); + + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.max_handshake_requests", + "5")); + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.initial_handshake_resend_period", + "200")); + property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.handshake_resend_period_gain", + "1.75")); + + writer.property_policy(property_policy).init(); + + // Writer should correctly initialize + ASSERT_TRUE(writer.isInitialized()); +} + +TEST(Security, ValidateAuthenticationHandshakeProperties) +{ + // Create + PubSubReader reader(TEST_TOPIC_NAME); + PubSubWriter writer(TEST_TOPIC_NAME); + + PropertyPolicy property_policy; + std::string xml_file = "auth_handshake_props_profile.xml"; + std::string profile_name = "auth_handshake_props"; + + // Set a configuration that makes participant authentication + // to be performed quickly so that we receive handshake + // in 0.15 secs approx + writer.set_xml_filename(xml_file); + writer.set_participant_profile(profile_name); + + reader.set_xml_filename(xml_file); + reader.set_participant_profile(profile_name); + + reader.init(); + ASSERT_TRUE(reader.isInitialized()); + + writer.init(); + ASSERT_TRUE(writer.isInitialized()); + + // If the settings were correctly applied + // we expect to be authorized in less than 0.5 seconds + // In reality, this time could be 0.2 perfectly, + // but some padding is left because of the ci + // or slower platforms + auto t0 = std::chrono::steady_clock::now(); + reader.waitAuthorized(); + double auth_elapsed_time = std::chrono::duration_cast( + std::chrono::steady_clock::now() - t0).count(); + + // Both should be authorized + writer.waitAuthorized(); + + ASSERT_TRUE(auth_elapsed_time < 0.5); +} + void blackbox_security_init() { From 312a9a7582b6cbd8e2a84285aebfc9c2a856a281 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Thu, 22 Feb 2024 08:21:50 +0100 Subject: [PATCH 2/9] Refs #20439: Add new get_property() api method in PropertyPolicy Signed-off-by: Mario Dominguez --- .../fastdds/rtps/attributes/PropertyPolicy.h | 191 ++++++++++-------- src/cpp/rtps/attributes/PropertyPolicy.cpp | 50 ++++- 2 files changed, 147 insertions(+), 94 deletions(-) diff --git a/include/fastdds/rtps/attributes/PropertyPolicy.h b/include/fastdds/rtps/attributes/PropertyPolicy.h index 5150a46ebdf..d24da32dc76 100644 --- a/include/fastdds/rtps/attributes/PropertyPolicy.h +++ b/include/fastdds/rtps/attributes/PropertyPolicy.h @@ -28,93 +28,118 @@ namespace rtps { class PropertyPolicy { - public: - RTPS_DllAPI PropertyPolicy() {} - - RTPS_DllAPI PropertyPolicy(const PropertyPolicy& property_policy) : - properties_(property_policy.properties_), - binary_properties_(property_policy.binary_properties_) {} - - RTPS_DllAPI PropertyPolicy(PropertyPolicy&& property_policy) : - properties_(std::move(property_policy.properties_)), - binary_properties_(std::move(property_policy.binary_properties_)) {} - - RTPS_DllAPI PropertyPolicy& operator=(const PropertyPolicy& property_policy) - { - properties_ = property_policy.properties_; - binary_properties_ = property_policy.binary_properties_; - return *this; - } - - RTPS_DllAPI PropertyPolicy& operator=(PropertyPolicy&& property_policy) - { - properties_ = std::move(property_policy.properties_); - binary_properties_= std::move(property_policy.binary_properties_); - return *this; - } - - RTPS_DllAPI bool operator==(const PropertyPolicy& b) const - { - return (this->properties_ == b.properties_) && - (this->binary_properties_ == b.binary_properties_); - } - - //!Get properties - RTPS_DllAPI const PropertySeq& properties() const - { - return properties_; - } - - //!Set properties - RTPS_DllAPI PropertySeq& properties() - { - return properties_; - } - - //!Get binary_properties - RTPS_DllAPI const BinaryPropertySeq& binary_properties() const - { - return binary_properties_; - } - - //!Set binary_properties - RTPS_DllAPI BinaryPropertySeq& binary_properties() - { - return binary_properties_; - } - - private: - PropertySeq properties_; - - BinaryPropertySeq binary_properties_; +public: + + RTPS_DllAPI PropertyPolicy() + { + } + + RTPS_DllAPI PropertyPolicy( + const PropertyPolicy& property_policy) + : properties_(property_policy.properties_) + , binary_properties_(property_policy.binary_properties_) + { + } + + RTPS_DllAPI PropertyPolicy( + PropertyPolicy&& property_policy) + : properties_(std::move(property_policy.properties_)) + , binary_properties_(std::move(property_policy.binary_properties_)) + { + } + + RTPS_DllAPI PropertyPolicy& operator =( + const PropertyPolicy& property_policy) + { + properties_ = property_policy.properties_; + binary_properties_ = property_policy.binary_properties_; + return *this; + } + + RTPS_DllAPI PropertyPolicy& operator =( + PropertyPolicy&& property_policy) + { + properties_ = std::move(property_policy.properties_); + binary_properties_ = std::move(property_policy.binary_properties_); + return *this; + } + + RTPS_DllAPI bool operator ==( + const PropertyPolicy& b) const + { + return (this->properties_ == b.properties_) && + (this->binary_properties_ == b.binary_properties_); + } + + //!Get properties + RTPS_DllAPI const PropertySeq& properties() const + { + return properties_; + } + + //!Set properties + RTPS_DllAPI PropertySeq& properties() + { + return properties_; + } + + //!Get binary_properties + RTPS_DllAPI const BinaryPropertySeq& binary_properties() const + { + return binary_properties_; + } + + //!Set binary_properties + RTPS_DllAPI BinaryPropertySeq& binary_properties() + { + return binary_properties_; + } + +private: + + PropertySeq properties_; + + BinaryPropertySeq binary_properties_; }; class PropertyPolicyHelper { - public: - /*! - * @brief Returns only the properties whose name starts with the prefix. - * Prefix is removed in returned properties. - * @param property_policy PropertyPolicy where properties will be searched. - * @param prefix Prefix used to search properties. - * @return A copy of properties whose name starts with the prefix. - */ - RTPS_DllAPI static PropertyPolicy get_properties_with_prefix( - const PropertyPolicy& property_policy, - const std::string& prefix); - - //!Get the length of the property_policy - RTPS_DllAPI static size_t length(const PropertyPolicy& property_policy); - - //!Look for a property_policy by name - RTPS_DllAPI static std::string* find_property( - PropertyPolicy& property_policy, - const std::string& name); - - //!Retrieves a property_policy by name - RTPS_DllAPI static const std::string* find_property( - const PropertyPolicy& property_policy, - const std::string& name); +public: + + /*! + * @brief Returns only the properties whose name starts with the prefix. + * Prefix is removed in returned properties. + * @param property_policy PropertyPolicy where properties will be searched. + * @param prefix Prefix used to search properties. + * @return A copy of properties whose name starts with the prefix. + */ + RTPS_DllAPI static PropertyPolicy get_properties_with_prefix( + const PropertyPolicy& property_policy, + const std::string& prefix); + + //!Get the length of the property_policy + RTPS_DllAPI static size_t length( + const PropertyPolicy& property_policy); + + //!Look for a property_policy by name + RTPS_DllAPI static std::string* find_property( + PropertyPolicy& property_policy, + const std::string& name); + + //!Retrieves a property_policy by name + RTPS_DllAPI static const std::string* find_property( + const PropertyPolicy& property_policy, + const std::string& name); + + /** + * @brief Retrieves a property by name + * @param property_policy PropertyPolicy where the property will be searched. + * @param name Name of the property to be searched. + * @return A pointer to the property if found, nullptr otherwise. + */ + RTPS_DllAPI static const Property* get_property( + const PropertyPolicy& property_policy, + const std::string& name); }; } //namespace rtps diff --git a/src/cpp/rtps/attributes/PropertyPolicy.cpp b/src/cpp/rtps/attributes/PropertyPolicy.cpp index f3d98367baf..4f4a8e9819e 100644 --- a/src/cpp/rtps/attributes/PropertyPolicy.cpp +++ b/src/cpp/rtps/attributes/PropertyPolicy.cpp @@ -22,7 +22,9 @@ using namespace eprosima::fastrtps::rtps; -PropertyPolicy PropertyPolicyHelper::get_properties_with_prefix(const PropertyPolicy& property_policy, const std::string& prefix) +PropertyPolicy PropertyPolicyHelper::get_properties_with_prefix( + const PropertyPolicy& property_policy, + const std::string& prefix) { PropertyPolicy returned_property_policy; @@ -30,7 +32,7 @@ PropertyPolicy PropertyPolicyHelper::get_properties_with_prefix(const PropertyPo std::for_each(property_policy.properties().begin(), property_policy.properties().end(), [&returned_property_policy, &prefix](const Property& property) { - if(property.name().compare(0, prefix.size(), prefix) == 0) + if (property.name().compare(0, prefix.size(), prefix) == 0) { Property new_property(property); new_property.name().erase(0, prefix.size()); @@ -42,7 +44,7 @@ PropertyPolicy PropertyPolicyHelper::get_properties_with_prefix(const PropertyPo std::for_each(property_policy.binary_properties().begin(), property_policy.binary_properties().end(), [&returned_property_policy, &prefix](const BinaryProperty& property) { - if(property.name().compare(0, prefix.size(), prefix) == 0) + if (property.name().compare(0, prefix.size(), prefix) == 0) { BinaryProperty new_property(property); new_property.name().erase(0, prefix.size()); @@ -53,19 +55,23 @@ PropertyPolicy PropertyPolicyHelper::get_properties_with_prefix(const PropertyPo return returned_property_policy; } -size_t PropertyPolicyHelper::length(const PropertyPolicy& property_policy) +size_t PropertyPolicyHelper::length( + const PropertyPolicy& property_policy) { return property_policy.properties().size() + - property_policy.binary_properties().size(); + property_policy.binary_properties().size(); } -std::string* PropertyPolicyHelper::find_property(PropertyPolicy& property_policy, const std::string& name) +std::string* PropertyPolicyHelper::find_property( + PropertyPolicy& property_policy, + const std::string& name) { std::string* returnedValue = nullptr; - for(auto property = property_policy.properties().begin(); property != property_policy.properties().end(); ++property) + for (auto property = property_policy.properties().begin(); property != property_policy.properties().end(); + ++property) { - if(property->name().compare(name) == 0) + if (property->name().compare(name) == 0) { returnedValue = &property->value(); break; @@ -75,13 +81,16 @@ std::string* PropertyPolicyHelper::find_property(PropertyPolicy& property_policy return returnedValue; } -const std::string* PropertyPolicyHelper::find_property(const PropertyPolicy& property_policy, const std::string& name) +const std::string* PropertyPolicyHelper::find_property( + const PropertyPolicy& property_policy, + const std::string& name) { const std::string* returnedValue = nullptr; - for(auto property = property_policy.properties().begin(); property != property_policy.properties().end(); ++property) + for (auto property = property_policy.properties().begin(); property != property_policy.properties().end(); + ++property) { - if(property->name().compare(name) == 0) + if (property->name().compare(name) == 0) { returnedValue = &property->value(); break; @@ -90,3 +99,22 @@ const std::string* PropertyPolicyHelper::find_property(const PropertyPolicy& pro return returnedValue; } + +const Property* PropertyPolicyHelper::get_property( + const PropertyPolicy& property_policy, + const std::string& name) +{ + const Property* returnedValue = nullptr; + + for (auto property = property_policy.properties().begin(); property != property_policy.properties().end(); + ++property) + { + if (property->name().compare(name) == 0) + { + returnedValue = &*property; + break; + } + } + + return returnedValue; +} From 6c66e604472cedfe0c70a9024cc0c417ed3befb2 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Thu, 22 Feb 2024 08:22:16 +0100 Subject: [PATCH 3/9] Refs #20439: Add new PropertyParser module in Property.h Signed-off-by: Mario Dominguez --- include/fastdds/rtps/common/Property.h | 124 +++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/include/fastdds/rtps/common/Property.h b/include/fastdds/rtps/common/Property.h index fa10981e2b9..0b1e9d6581c 100644 --- a/include/fastdds/rtps/common/Property.h +++ b/include/fastdds/rtps/common/Property.h @@ -18,9 +18,13 @@ #ifndef _FASTDDS_RTPS_COMMON_PROPERTYQOS_H_ #define _FASTDDS_RTPS_COMMON_PROPERTYQOS_H_ +#include +#include #include #include +#include + namespace eprosima { namespace fastrtps { namespace rtps { @@ -214,6 +218,126 @@ class PropertyHelper }; +struct PropertyParser +{ + + /** + * @brief Parse a property value as an integer + * @param property Property to parse + * @param check_upper_bound If true, check that the value is lower than upper_bound + * @param upper_bound Upper bound to check + * @param check_lower_bound If true, check that the value is greater than lower_bound + * @param lower_bound Lower bound to check + * @param exception Exception to throw if the value is not a valid integer or if it is out of bounds + * @return The parsed integer value + * + * @warning May throw an exception_t if the value is not a valid integer + * or if it is out of bounds. + */ + template + inline static int as_int( + const Property& property, + const bool& check_upper_bound, + const int& upper_bound, + const bool& check_lower_bound, + const int& lower_bound, + const exception_t& exception) + { + return parse_value( + std::function( + [](const Property& property) + { + return std::stoi(property.value()); + } + ), + property, + check_upper_bound, + upper_bound, + check_lower_bound, + lower_bound, + exception); + } + + /** + * @brief Parse a property value as a double + * @param property Property to parse + * @param check_upper_bound If true, check that the value is lower than upper_bound + * @param upper_bound Upper bound to check + * @param check_lower_bound If true, check that the value is greater than lower_bound + * @param lower_bound Lower bound to check + * @param exception Exception to throw if the value is not a valid integer or if it is out of bounds + * @return The parsed integer value + * + * @warning May throw an exception_t if the value is not a valid double + * or if it is out of bounds. + */ + template + inline static double as_double( + const Property& property, + const bool& check_upper_bound, + const double& upper_bound, + const bool& check_lower_bound, + const double& lower_bound, + const exception_t& exception) + { + return parse_value( + std::function( + [](const Property& property) + { + return std::stod(property.value()); + } + ), + property, + check_upper_bound, + upper_bound, + check_lower_bound, + lower_bound, + exception); + } + +private: + + template + inline static value_t parse_value( + const std::function& conversor, + const Property& property, + const bool& check_upper_bound, + const value_t& upper_bound, + const bool& check_lower_bound, + const value_t& lower_bound, + const exception_t& exception) + { + try + { + value_t converted_value = conversor(property); + + if (check_lower_bound && converted_value <= lower_bound) + { + throw exception_t("Value '" + property.value() + + "' for " + property.name() + " must be greater than " + std::to_string(lower_bound)); + } + + if (check_upper_bound && converted_value >= upper_bound) + { + throw exception_t("Value '" + property.value() + + "' for " + property.name() + " must be lower than " + std::to_string(upper_bound)); + } + + return converted_value; + } + catch (const std::invalid_argument&) + { + throw exception; + } + catch (const std::out_of_range&) + { + throw exception; + } + } + +}; + } //namespace eprosima } //namespace fastrtps } //namespace rtps From decab8d896d6f4dd15ffbcf83f05d681b39d3fde Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Thu, 22 Feb 2024 08:24:20 +0100 Subject: [PATCH 4/9] Refs #20439: Integrate AuthenticationHandshakeProperties in Security Manager Signed-off-by: Mario Dominguez --- src/cpp/rtps/security/SecurityManager.cpp | 72 +++++++++++++++++++++-- src/cpp/rtps/security/SecurityManager.h | 28 ++++++++- 2 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index fe59a811437..f7d9890c92d 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -198,6 +198,17 @@ bool SecurityManager::init( if (authentication_plugin_ != nullptr) { + // retrieve authentication properties, if any + const PropertyPolicy auth_handshake_properties = PropertyPolicyHelper::get_properties_with_prefix( + participant_->getRTPSParticipantAttributes().properties, + "dds.sec.auth.builtin.PKI-DH."); + + // if auth_handshake_properties is empty, the default values are used + if (PropertyPolicyHelper::length(auth_handshake_properties) > 0) + { + auth_handshake_props_.parse_from_property_policy(auth_handshake_properties); + } + authentication_plugin_->set_logger(logging_plugin_, exception); // Validate local participant @@ -618,7 +629,7 @@ bool SecurityManager::discovered_participant( resend_handshake_message_token(guid); return true; }, - DiscoveredParticipantInfo::INITIAL_RESEND_HANDSHAKE_MILLISECS)); // TODO (Ricardo) Configurable + static_cast(auth_handshake_props_.initial_handshake_resend_period_ms_))); IdentityHandle* remote_identity_handle = nullptr; @@ -920,6 +931,7 @@ bool SecurityManager::on_process_handshake( handshake_message_send = true; expected_sequence_number = message.message_identity().sequence_number(); remote_participant_info->change_sequence_number_ = change->sequenceNumber; + remote_participant_info->handshake_requests_sent_++; } else { @@ -984,7 +996,8 @@ bool SecurityManager::on_process_handshake( remote_participant_info->expected_sequence_number_ = expected_sequence_number; // Avoid DoS attack by exponentially increasing event interval auto time_ms = remote_participant_info->event_->getIntervalMilliSec(); - remote_participant_info->event_->update_interval_millisec(time_ms * 2); + remote_participant_info->event_->update_interval_millisec( + time_ms * auth_handshake_props_.handshake_resend_period_gain_); remote_participant_info->event_->restart_timer(); } @@ -1526,6 +1539,7 @@ void SecurityManager::process_participant_stateless_message( if (participant_stateless_message_writer_history_->add_change(p_change)) { remote_participant_info->change_sequence_number_ = p_change->sequenceNumber; + remote_participant_info->handshake_requests_sent_++; } //TODO (Ricardo) What to do if not added? } @@ -1598,6 +1612,7 @@ void SecurityManager::process_participant_stateless_message( if (participant_stateless_message_writer_history_->add_change(p_change)) { remote_participant_info->change_sequence_number_ = p_change->sequenceNumber; + remote_participant_info->handshake_requests_sent_++; } //TODO (Ricardo) What to do if not added? } @@ -4187,7 +4202,7 @@ void SecurityManager::resend_handshake_message_token( if (remote_participant_info) { - if (remote_participant_info->handshake_requests_sent_ >= DiscoveredParticipantInfo::MAX_HANDSHAKE_REQUESTS) + if (remote_participant_info->handshake_requests_sent_ >= auth_handshake_props_.max_handshake_requests_) { if (remote_participant_info->auth_status_ != AUTHENTICATION_FAILED) { @@ -4222,7 +4237,8 @@ void SecurityManager::resend_handshake_message_token( { // Avoid DoS attack by exponentially increasing event interval auto time_ms = remote_participant_info->event_->getIntervalMilliSec(); - remote_participant_info->event_->update_interval_millisec(time_ms * 2); + remote_participant_info->event_->update_interval_millisec( + time_ms * auth_handshake_props_.handshake_resend_period_gain_); remote_participant_info->event_->restart_timer(); } } @@ -4260,6 +4276,54 @@ void SecurityManager::on_validation_failed( } } +SecurityManager::AuthenticationHandshakeProperties::AuthenticationHandshakeProperties() + : max_handshake_requests_(10) + , initial_handshake_resend_period_ms_(125) + , handshake_resend_period_gain_(1.5) +{ + +} + +void SecurityManager::AuthenticationHandshakeProperties::parse_from_property_policy( + const PropertyPolicy& auth_handshake_properties) +{ + const Property* const max_handshake_requests = + PropertyPolicyHelper::get_property(auth_handshake_properties, "max_handshake_requests"); + + if (max_handshake_requests != nullptr) + { + max_handshake_requests_ = PropertyParser::as_int( + *max_handshake_requests, + false, 0, + true, 0, + SecurityException("Error parsing max_handshake_requests property value.")); + } + + const Property* const initial_handshake_resend_period = + PropertyPolicyHelper::get_property(auth_handshake_properties, "initial_handshake_resend_period"); + + if (initial_handshake_resend_period != nullptr) + { + initial_handshake_resend_period_ms_ = PropertyParser::as_int( + *initial_handshake_resend_period, + false, 0, + true, 0, + SecurityException("Error parsing initial_handshake_resend_period property value.")); + } + + const Property* const handshake_resend_period_gain = + PropertyPolicyHelper::get_property(auth_handshake_properties, "handshake_resend_period_gain"); + + if (handshake_resend_period_gain != nullptr) + { + handshake_resend_period_gain_ = PropertyParser::as_double( + *handshake_resend_period_gain, + false, 0.0, + true, 1.0, + SecurityException("Error parsing handshake_resend_period_gain property value.")); + } +} + bool SecurityManager::DiscoveredParticipantInfo::check_guid_comes_from( Authentication* const auth_plugin, const GUID_t& adjusted, diff --git a/src/cpp/rtps/security/SecurityManager.h b/src/cpp/rtps/security/SecurityManager.h index 6f237a61684..9b68f6152c2 100644 --- a/src/cpp/rtps/security/SecurityManager.h +++ b/src/cpp/rtps/security/SecurityManager.h @@ -356,6 +356,30 @@ class SecurityManager AUTHENTICATION_NOT_AVAILABLE }; + struct AuthenticationHandshakeProperties + { + AuthenticationHandshakeProperties(); + ~AuthenticationHandshakeProperties() = default; + + /** + * @brief Parses the properties from a propertypolicy rule + * @param properties PropertyPolicy reference to the properties to parse + */ + void parse_from_property_policy( + const PropertyPolicy& properties); + + // Maximum number of handshake requests to be sent + // Must be greater than 0 + int max_handshake_requests_; + // Initial wait time (in milliseconds) for the first handshake request resend + // Must be greater than 0 + int initial_handshake_resend_period_ms_; + // Gain for the period between handshake request resends + // The initial period is multiplied by this value each time a resend is performed + // Must be greater than 1 + double handshake_resend_period_gain_; + }; + class DiscoveredParticipantInfo { struct AuthenticationInfo @@ -398,7 +422,7 @@ class SecurityManager EventUniquePtr event_; - uint32_t handshake_requests_sent_; + int handshake_requests_sent_; private: @@ -789,6 +813,8 @@ class SecurityManager uint32_t domain_id_ = 0; + AuthenticationHandshakeProperties auth_handshake_props_; + IdentityHandle* local_identity_handle_ = nullptr; PermissionsHandle* local_permissions_handle_ = nullptr; From 17d506a59d2ba3faf3aef6e514f812d5b217b9f2 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Thu, 22 Feb 2024 18:35:47 +0100 Subject: [PATCH 5/9] Refs #20439: Linter Signed-off-by: Mario Dominguez --- include/fastdds/rtps/attributes/PropertyPolicy.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/fastdds/rtps/attributes/PropertyPolicy.h b/include/fastdds/rtps/attributes/PropertyPolicy.h index d24da32dc76..92f78e9c769 100644 --- a/include/fastdds/rtps/attributes/PropertyPolicy.h +++ b/include/fastdds/rtps/attributes/PropertyPolicy.h @@ -132,11 +132,11 @@ class PropertyPolicyHelper const std::string& name); /** - * @brief Retrieves a property by name - * @param property_policy PropertyPolicy where the property will be searched. - * @param name Name of the property to be searched. - * @return A pointer to the property if found, nullptr otherwise. - */ + * @brief Retrieves a property by name + * @param property_policy PropertyPolicy where the property will be searched. + * @param name Name of the property to be searched. + * @return A pointer to the property if found, nullptr otherwise. + */ RTPS_DllAPI static const Property* get_property( const PropertyPolicy& property_policy, const std::string& name); From f856919d2348852280820d0c06d2f3f7129bc2c8 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Fri, 23 Feb 2024 08:32:45 +0100 Subject: [PATCH 6/9] Refs #20439: Fix elapsed time test comparison Signed-off-by: Mario Dominguez --- test/blackbox/common/BlackboxTestsSecurity.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 8d10c5281d0..2d7b812bfec 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -4916,15 +4916,16 @@ TEST(Security, ValidateAuthenticationHandshakeProperties) // In reality, this time could be 0.2 perfectly, // but some padding is left because of the ci // or slower platforms + std::chrono::duration max_time(500); auto t0 = std::chrono::steady_clock::now(); reader.waitAuthorized(); - double auth_elapsed_time = std::chrono::duration_cast( - std::chrono::steady_clock::now() - t0).count(); + auto auth_elapsed_time = std::chrono::duration( + std::chrono::steady_clock::now() - t0); // Both should be authorized writer.waitAuthorized(); - ASSERT_TRUE(auth_elapsed_time < 0.5); + ASSERT_TRUE(auth_elapsed_time < max_time); } From d07bd96f6093d96ea3f67bd8c29c75f8e0f088ff Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Fri, 23 Feb 2024 08:34:07 +0100 Subject: [PATCH 7/9] Refs #20439: versions.md Signed-off-by: Mario Dominguez --- versions.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/versions.md b/versions.md index 64ab0ffe017..3b2166d4272 100644 --- a/versions.md +++ b/versions.md @@ -1,6 +1,8 @@ Forthcoming ----------- +* Added authentication handshake properties. + Version 2.13.0 -------------- From 26edae58f81c399df2df5a5f57160b91082291a9 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Mon, 26 Feb 2024 17:42:50 +0100 Subject: [PATCH 8/9] Refs #20439: Review suggestions Signed-off-by: Mario Dominguez --- include/fastdds/rtps/common/Property.h | 14 ++++++++------ src/cpp/rtps/security/SecurityManager.cpp | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/include/fastdds/rtps/common/Property.h b/include/fastdds/rtps/common/Property.h index 0b1e9d6581c..ed1c0673069 100644 --- a/include/fastdds/rtps/common/Property.h +++ b/include/fastdds/rtps/common/Property.h @@ -265,8 +265,8 @@ struct PropertyParser * @param upper_bound Upper bound to check * @param check_lower_bound If true, check that the value is greater than lower_bound * @param lower_bound Lower bound to check - * @param exception Exception to throw if the value is not a valid integer or if it is out of bounds - * @return The parsed integer value + * @param exception Exception to throw if the value is not a valid double or if it is out of bounds + * @return The parsed double value * * @warning May throw an exception_t if the value is not a valid double * or if it is out of bounds. @@ -312,16 +312,18 @@ struct PropertyParser { value_t converted_value = conversor(property); - if (check_lower_bound && converted_value <= lower_bound) + if (check_lower_bound && converted_value < lower_bound) { throw exception_t("Value '" + property.value() + - "' for " + property.name() + " must be greater than " + std::to_string(lower_bound)); + "' for " + property.name() + " must be greater or equal to " + + std::to_string(lower_bound)); } - if (check_upper_bound && converted_value >= upper_bound) + if (check_upper_bound && converted_value > upper_bound) { throw exception_t("Value '" + property.value() + - "' for " + property.name() + " must be lower than " + std::to_string(upper_bound)); + "' for " + property.name() + " must be lower or equal to " + + std::to_string(upper_bound)); } return converted_value; diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index f7d9890c92d..a4eb1efa07d 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -4295,7 +4295,7 @@ void SecurityManager::AuthenticationHandshakeProperties::parse_from_property_pol max_handshake_requests_ = PropertyParser::as_int( *max_handshake_requests, false, 0, - true, 0, + true, 1, SecurityException("Error parsing max_handshake_requests property value.")); } @@ -4307,7 +4307,7 @@ void SecurityManager::AuthenticationHandshakeProperties::parse_from_property_pol initial_handshake_resend_period_ms_ = PropertyParser::as_int( *initial_handshake_resend_period, false, 0, - true, 0, + true, 1, SecurityException("Error parsing initial_handshake_resend_period property value.")); } From 05299b87db56f8d87423a62aa8f90d3f77357057 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Tue, 27 Feb 2024 19:12:07 +0100 Subject: [PATCH 9/9] Refs #20439: Make ints size explicit. Leave it as 32 to cover all the range of stoi() Signed-off-by: Mario Dominguez --- src/cpp/rtps/security/SecurityManager.cpp | 4 ++-- src/cpp/rtps/security/SecurityManager.h | 16 +++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index a4eb1efa07d..5ffdbdcedde 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -4292,7 +4292,7 @@ void SecurityManager::AuthenticationHandshakeProperties::parse_from_property_pol if (max_handshake_requests != nullptr) { - max_handshake_requests_ = PropertyParser::as_int( + max_handshake_requests_ = (int32_t)PropertyParser::as_int( *max_handshake_requests, false, 0, true, 1, @@ -4304,7 +4304,7 @@ void SecurityManager::AuthenticationHandshakeProperties::parse_from_property_pol if (initial_handshake_resend_period != nullptr) { - initial_handshake_resend_period_ms_ = PropertyParser::as_int( + initial_handshake_resend_period_ms_ = (int32_t)PropertyParser::as_int( *initial_handshake_resend_period, false, 0, true, 1, diff --git a/src/cpp/rtps/security/SecurityManager.h b/src/cpp/rtps/security/SecurityManager.h index 9b68f6152c2..9fec2491387 100644 --- a/src/cpp/rtps/security/SecurityManager.h +++ b/src/cpp/rtps/security/SecurityManager.h @@ -370,10 +370,10 @@ class SecurityManager // Maximum number of handshake requests to be sent // Must be greater than 0 - int max_handshake_requests_; + int32_t max_handshake_requests_; // Initial wait time (in milliseconds) for the first handshake request resend // Must be greater than 0 - int initial_handshake_resend_period_ms_; + int32_t initial_handshake_resend_period_ms_; // Gain for the period between handshake request resends // The initial period is multiplied by this value each time a resend is performed // Must be greater than 1 @@ -390,12 +390,13 @@ class SecurityManager AuthenticationInfo( AuthenticationStatus auth_status) - : identity_handle_(nullptr) + : handshake_requests_sent_(0) + , identity_handle_(nullptr) , handshake_handle_(nullptr) , auth_status_(auth_status) , expected_sequence_number_(0) , change_sequence_number_(SequenceNumber_t::unknown()) - , handshake_requests_sent_(0) + { } @@ -410,6 +411,8 @@ class SecurityManager { } + int32_t handshake_requests_sent_; + IdentityHandle* identity_handle_; HandshakeHandle* handshake_handle_; @@ -422,8 +425,6 @@ class SecurityManager EventUniquePtr event_; - int handshake_requests_sent_; - private: AuthenticationInfo( @@ -434,9 +435,6 @@ class SecurityManager typedef std::unique_ptr AuthUniquePtr; - static constexpr uint32_t INITIAL_RESEND_HANDSHAKE_MILLISECS = 125; - static constexpr uint32_t MAX_HANDSHAKE_REQUESTS = 5; - DiscoveredParticipantInfo( AuthenticationStatus auth_status, const ParticipantProxyData& participant_data)