Copilot commented on code in PR #3600:
URL: https://github.com/apache/thrift/pull/3600#discussion_r3429170417
##########
lib/cpp/test/SecurityTest.cpp:
##########
@@ -226,6 +228,77 @@ struct SecurityFixture
BOOST_FIXTURE_TEST_SUITE(BOOST_TEST_MODULE, SecurityFixture)
+BOOST_AUTO_TEST_CASE(default_ssl_context_options)
+{
+ apache::thrift::transport::SSLContext context;
+ const auto options = SSL_CTX_get_options(context.get());
+
+ if (SSL_OP_NO_SSLv2 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_SSLv2) != 0);
+ }
+ if (SSL_OP_NO_SSLv3 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_SSLv3) != 0);
+ }
+ if (SSL_OP_NO_TLSv1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
+ }
+ if (SSL_OP_NO_TLSv1_1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) != 0);
+ }
+}
+
+BOOST_AUTO_TEST_CASE(custom_ssl_context_options)
+{
+ class CustomSSLContext : public apache::thrift::transport::SSLContext
+ {
+ public:
+ CustomSSLContext() : SSLContext()
+ {
+ SSL_CTX_clear_options(get(), SSL_OP_NO_TLSv1_1);
+ }
+ };
+
+ std::shared_ptr<apache::thrift::transport::SSLContext> context;
+ TSSLSocketFactory factory([&context]() {
+ context = std::make_shared<CustomSSLContext>();
+ return context;
+ });
+ const auto options = SSL_CTX_get_options(context->get());
+
+ if (SSL_OP_NO_TLSv1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
+ }
+ if (SSL_OP_NO_TLSv1_1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) == 0);
+ }
+}
+
+BOOST_AUTO_TEST_CASE(custom_ssl_context_factory_validation)
+{
+ try
+ {
+ SSLContextFactory contextFactory;
+ TSSLSocketFactory factory(contextFactory);
+ BOOST_FAIL("Expected empty SSLContextFactory to throw");
+ }
+ catch (const TSSLException& ex)
+ {
+ BOOST_CHECK_EQUAL("SSLContextFactory must not be empty", ex.what());
+ }
+
+ try
+ {
+ TSSLSocketFactory factory([]() {
+ return std::shared_ptr<apache::thrift::transport::SSLContext>();
+ });
+ BOOST_FAIL("Expected null SSLContextFactory result to throw");
+ }
+ catch (const TSSLException& ex)
+ {
+ BOOST_CHECK_EQUAL("SSLContextFactory must not return null", ex.what());
+ }
Review Comment:
These checks compare a string literal to `ex.what()` (a `const char*`).
Depending on Boost.Test overload resolution, this can devolve into pointer
comparison rather than content comparison. Prefer comparing
`std::string(ex.what())` to the expected message, or use a Boost.Test assertion
that clearly performs string/content comparison.
##########
lib/cpp/test/SecurityTest.cpp:
##########
@@ -226,6 +228,77 @@ struct SecurityFixture
BOOST_FIXTURE_TEST_SUITE(BOOST_TEST_MODULE, SecurityFixture)
+BOOST_AUTO_TEST_CASE(default_ssl_context_options)
+{
+ apache::thrift::transport::SSLContext context;
+ const auto options = SSL_CTX_get_options(context.get());
+
+ if (SSL_OP_NO_SSLv2 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_SSLv2) != 0);
+ }
+ if (SSL_OP_NO_SSLv3 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_SSLv3) != 0);
+ }
+ if (SSL_OP_NO_TLSv1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
+ }
+ if (SSL_OP_NO_TLSv1_1 != 0) {
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) != 0);
+ }
+}
+
+BOOST_AUTO_TEST_CASE(custom_ssl_context_options)
+{
+ class CustomSSLContext : public apache::thrift::transport::SSLContext
+ {
+ public:
+ CustomSSLContext() : SSLContext()
+ {
+ SSL_CTX_clear_options(get(), SSL_OP_NO_TLSv1_1);
+ }
+ };
+
+ std::shared_ptr<apache::thrift::transport::SSLContext> context;
+ TSSLSocketFactory factory([&context]() {
+ context = std::make_shared<CustomSSLContext>();
+ return context;
+ });
+ const auto options = SSL_CTX_get_options(context->get());
Review Comment:
`context` is declared before `factory`, so it will be destroyed after
`factory`. Since `TSSLSocketFactory`’s destructor can trigger global OpenSSL
cleanup when the factory count reaches 0, this creates a teardown-order hazard:
the `SSLContext` may be freed after OpenSSL global cleanup has already run. To
avoid potential use-after-cleanup / flaky crashes, explicitly destroy/reset
`context` before `factory` goes out of scope (or otherwise ensure the factory
outlives any references to the context).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
