github-actions[bot] commented on code in PR #68816:
URL: https://github.com/apache/doris/pull/68816#discussion_r4249017936
##########
be/test/service/http/http_client_test.cpp:
##########
@@ -46,10 +56,290 @@
#include "service/http/utils.h"
#include "service/http_service.h"
#include "testutil/http_utils.h"
+#include "util/defer_op.h"
#include "util/md5.h"
namespace doris {
+class HttpClientTlsTest : public testing::Test {
+protected:
+ void SetUp() override {
+ {
+ std::scoped_lock lock(*config::get_mutable_string_config_lock());
+ _original_ca_paths = config::ca_cert_file_paths;
+ }
+ EVP_PKEY* raw_key = nullptr;
+ generate_key(&raw_key);
+ ASSERT_FALSE(HasFatalFailure());
+ std::unique_ptr<EVP_PKEY, decltype(&EVP_PKEY_free)> key(raw_key,
EVP_PKEY_free);
+ std::unique_ptr<X509, decltype(&X509_free)> certificate(X509_new(),
X509_free);
+ ASSERT_NE(certificate, nullptr);
+ generate_certificate(certificate.get(), key.get());
+ ASSERT_FALSE(HasFatalFailure());
+ start_server_and_write_ca(certificate.get(), key.get());
+ ASSERT_FALSE(HasFatalFailure());
+ }
+
+ static void generate_key(EVP_PKEY** key) {
+ std::unique_ptr<EVP_PKEY_CTX, decltype(&EVP_PKEY_CTX_free)>
key_context(
+ EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, nullptr), EVP_PKEY_CTX_free);
+ ASSERT_NE(key_context, nullptr);
+ ASSERT_EQ(EVP_PKEY_keygen_init(key_context.get()), 1);
+ ASSERT_EQ(EVP_PKEY_CTX_set_rsa_keygen_bits(key_context.get(), 2048),
1);
+ ASSERT_EQ(EVP_PKEY_keygen(key_context.get(), key), 1);
+ }
+
+ static void generate_certificate(X509* certificate, EVP_PKEY* key) {
+ ASSERT_EQ(X509_set_version(certificate, 2), 1);
+ ASSERT_EQ(ASN1_INTEGER_set(X509_get_serialNumber(certificate), 1), 1);
+ ASSERT_NE(X509_gmtime_adj(X509_getm_notBefore(certificate), -60),
nullptr);
+ ASSERT_NE(X509_gmtime_adj(X509_getm_notAfter(certificate), 3600),
nullptr);
+ ASSERT_EQ(X509_set_pubkey(certificate, key), 1);
+ auto* subject = X509_get_subject_name(certificate);
+ ASSERT_EQ(X509_NAME_add_entry_by_txt(subject, "CN", MBSTRING_ASC,
+ reinterpret_cast<const unsigned
char*>("127.0.0.1"),
+ -1, -1, 0),
+ 1);
+ ASSERT_EQ(X509_set_issuer_name(certificate, subject), 1);
+ add_extension(certificate, NID_basic_constraints, "critical,CA:TRUE");
+ add_extension(certificate, NID_subject_alt_name, "IP:127.0.0.1");
+ ASSERT_FALSE(HasFatalFailure());
+ ASSERT_GT(X509_sign(certificate, key, EVP_sha256()), 0);
+ }
+
+ void start_server_and_write_ca(X509* certificate, EVP_PKEY* key) {
+ _ssl_context.reset(SSL_CTX_new(TLS_server_method()));
+ ASSERT_NE(_ssl_context, nullptr);
+ ASSERT_EQ(SSL_CTX_use_certificate(_ssl_context.get(), certificate), 1);
+ ASSERT_EQ(SSL_CTX_use_PrivateKey(_ssl_context.get(), key), 1);
+ std::unique_ptr<BIO, decltype(&BIO_free)>
cert_bio(BIO_new(BIO_s_mem()), BIO_free);
+ ASSERT_NE(cert_bio, nullptr);
+ ASSERT_EQ(PEM_write_bio_X509(cert_bio.get(), certificate), 1);
+ BUF_MEM* cert_pem = nullptr;
+ BIO_get_mem_ptr(cert_bio.get(), &cert_pem);
+ char ca_path[] = "/tmp/doris-http-client-ca-XXXXXX";
+ const int fd = mkstemp(ca_path);
+ ASSERT_GE(fd, 0);
+ _ca_path = ca_path;
+ Defer close_file {[fd] { close(fd); }};
+ ASSERT_EQ(write(fd, cert_pem->data, cert_pem->length),
+ static_cast<ssize_t>(cert_pem->length));
+ start_listener();
+ ASSERT_FALSE(HasFatalFailure());
+ }
+
+ void start_listener() {
+ _listen_fd = socket(AF_INET, SOCK_STREAM, 0);
+ ASSERT_GE(_listen_fd, 0);
+ sockaddr_in address {};
+ address.sin_family = AF_INET;
+ address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ ASSERT_EQ(bind(_listen_fd, reinterpret_cast<sockaddr*>(&address),
sizeof(address)), 0);
+ ASSERT_EQ(listen(_listen_fd, 8), 0);
+ socklen_t address_length = sizeof(address);
+ ASSERT_EQ(getsockname(_listen_fd,
reinterpret_cast<sockaddr*>(&address), &address_length),
+ 0);
+ _port = ntohs(address.sin_port);
+ _server_thread = std::thread([this] { serve_https(); });
+ }
+
+ void serve_https() {
+ sigset_t blocked_signals;
+ sigemptyset(&blocked_signals);
+ sigaddset(&blocked_signals, SIGPIPE);
+ ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_signals, nullptr), 0);
+ Defer drain_sigpipe {[&blocked_signals] {
+ const timespec no_wait {};
+ while (sigtimedwait(&blocked_signals, nullptr, &no_wait) ==
SIGPIPE) {
+ }
+ }};
+ while (!_stopped) {
+ const int fd = accept(_listen_fd, nullptr, nullptr);
+ if (fd < 0) {
+ return;
+ }
+ {
+ std::scoped_lock lock(_connection_mutex);
+ _connection_fd = fd;
+ if (_stopped) {
+ shutdown(fd, SHUT_RDWR);
+ }
+ }
+ Defer close_connection {[this, fd] {
+ std::scoped_lock lock(_connection_mutex);
+ _connection_fd = -1;
+ close(fd);
+ }};
+ respond_https(fd);
+ }
+ }
+
+ void respond_https(int fd) {
+ const timeval timeout {.tv_sec = 5, .tv_usec = 0};
+ ASSERT_EQ(setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout,
sizeof(timeout)), 0);
+ ASSERT_EQ(setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout,
sizeof(timeout)), 0);
+ std::unique_ptr<SSL, decltype(&SSL_free)>
connection(SSL_new(_ssl_context.get()), SSL_free);
+ ASSERT_NE(connection, nullptr);
+ ASSERT_EQ(SSL_set_fd(connection.get(), fd), 1);
+ if (SSL_accept(connection.get()) != 1) {
+ return;
+ }
+ char request[4096];
+ if (SSL_read(connection.get(), request, sizeof(request)) > 0) {
+ constexpr std::string_view response =
+ "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection:
close\r\n\r\nOK";
+ SSL_write(connection.get(), response.data(), response.size());
+ }
+ SSL_shutdown(connection.get());
+ }
+
+ void TearDown() override {
+ set_ca_paths(_original_ca_paths);
+ if (!_ca_path.empty()) {
+ EXPECT_EQ(unlink(_ca_path.c_str()), 0);
+ }
+ _stopped = true;
+ {
+ std::scoped_lock lock(_connection_mutex);
+ if (_connection_fd >= 0) {
+ shutdown(_connection_fd, SHUT_RDWR);
+ }
+ }
+ if (_listen_fd >= 0) {
+ shutdown(_listen_fd, SHUT_RDWR);
Review Comment:
[P2] Wake the listener explicitly before joining the TLS server. On macOS,
`shutdown()` on this unconnected listening socket returns `ENOTCONN` without
waking the worker's blocking `accept()` ([Darwin socket
implementation](https://raw.githubusercontent.com/apple-oss-distributions/xnu/main/bsd/kern/uipc_socket.c)).
For example, `tls_ca_path_probe_is_cached_and_configuration_invalidates_it`
starts the server but never connects to it; if the worker has reached
`accept()`, setting `_stopped` cannot interrupt that syscall,
`_server_thread.join()` waits indefinitely, and the later `close()` is never
reached. This remains after fixing the separate `sigtimedwait` compile issue.
Use a portable wakeup or a bounded/nonblocking accept loop before joining.
##########
be/test/service/http/http_client_test.cpp:
##########
@@ -46,10 +56,290 @@
#include "service/http/utils.h"
#include "service/http_service.h"
#include "testutil/http_utils.h"
+#include "util/defer_op.h"
#include "util/md5.h"
namespace doris {
+class HttpClientTlsTest : public testing::Test {
+protected:
+ void SetUp() override {
+ {
+ std::scoped_lock lock(*config::get_mutable_string_config_lock());
+ _original_ca_paths = config::ca_cert_file_paths;
+ }
+ EVP_PKEY* raw_key = nullptr;
+ generate_key(&raw_key);
+ ASSERT_FALSE(HasFatalFailure());
+ std::unique_ptr<EVP_PKEY, decltype(&EVP_PKEY_free)> key(raw_key,
EVP_PKEY_free);
+ std::unique_ptr<X509, decltype(&X509_free)> certificate(X509_new(),
X509_free);
+ ASSERT_NE(certificate, nullptr);
+ generate_certificate(certificate.get(), key.get());
+ ASSERT_FALSE(HasFatalFailure());
+ start_server_and_write_ca(certificate.get(), key.get());
+ ASSERT_FALSE(HasFatalFailure());
+ }
+
+ static void generate_key(EVP_PKEY** key) {
+ std::unique_ptr<EVP_PKEY_CTX, decltype(&EVP_PKEY_CTX_free)>
key_context(
+ EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, nullptr), EVP_PKEY_CTX_free);
+ ASSERT_NE(key_context, nullptr);
+ ASSERT_EQ(EVP_PKEY_keygen_init(key_context.get()), 1);
+ ASSERT_EQ(EVP_PKEY_CTX_set_rsa_keygen_bits(key_context.get(), 2048),
1);
+ ASSERT_EQ(EVP_PKEY_keygen(key_context.get(), key), 1);
+ }
+
+ static void generate_certificate(X509* certificate, EVP_PKEY* key) {
+ ASSERT_EQ(X509_set_version(certificate, 2), 1);
+ ASSERT_EQ(ASN1_INTEGER_set(X509_get_serialNumber(certificate), 1), 1);
+ ASSERT_NE(X509_gmtime_adj(X509_getm_notBefore(certificate), -60),
nullptr);
+ ASSERT_NE(X509_gmtime_adj(X509_getm_notAfter(certificate), 3600),
nullptr);
+ ASSERT_EQ(X509_set_pubkey(certificate, key), 1);
+ auto* subject = X509_get_subject_name(certificate);
+ ASSERT_EQ(X509_NAME_add_entry_by_txt(subject, "CN", MBSTRING_ASC,
+ reinterpret_cast<const unsigned
char*>("127.0.0.1"),
+ -1, -1, 0),
+ 1);
+ ASSERT_EQ(X509_set_issuer_name(certificate, subject), 1);
+ add_extension(certificate, NID_basic_constraints, "critical,CA:TRUE");
+ add_extension(certificate, NID_subject_alt_name, "IP:127.0.0.1");
+ ASSERT_FALSE(HasFatalFailure());
+ ASSERT_GT(X509_sign(certificate, key, EVP_sha256()), 0);
+ }
+
+ void start_server_and_write_ca(X509* certificate, EVP_PKEY* key) {
+ _ssl_context.reset(SSL_CTX_new(TLS_server_method()));
+ ASSERT_NE(_ssl_context, nullptr);
+ ASSERT_EQ(SSL_CTX_use_certificate(_ssl_context.get(), certificate), 1);
+ ASSERT_EQ(SSL_CTX_use_PrivateKey(_ssl_context.get(), key), 1);
+ std::unique_ptr<BIO, decltype(&BIO_free)>
cert_bio(BIO_new(BIO_s_mem()), BIO_free);
+ ASSERT_NE(cert_bio, nullptr);
+ ASSERT_EQ(PEM_write_bio_X509(cert_bio.get(), certificate), 1);
+ BUF_MEM* cert_pem = nullptr;
+ BIO_get_mem_ptr(cert_bio.get(), &cert_pem);
+ char ca_path[] = "/tmp/doris-http-client-ca-XXXXXX";
+ const int fd = mkstemp(ca_path);
+ ASSERT_GE(fd, 0);
+ _ca_path = ca_path;
+ Defer close_file {[fd] { close(fd); }};
+ ASSERT_EQ(write(fd, cert_pem->data, cert_pem->length),
+ static_cast<ssize_t>(cert_pem->length));
+ start_listener();
+ ASSERT_FALSE(HasFatalFailure());
+ }
+
+ void start_listener() {
+ _listen_fd = socket(AF_INET, SOCK_STREAM, 0);
+ ASSERT_GE(_listen_fd, 0);
+ sockaddr_in address {};
+ address.sin_family = AF_INET;
+ address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ ASSERT_EQ(bind(_listen_fd, reinterpret_cast<sockaddr*>(&address),
sizeof(address)), 0);
+ ASSERT_EQ(listen(_listen_fd, 8), 0);
+ socklen_t address_length = sizeof(address);
+ ASSERT_EQ(getsockname(_listen_fd,
reinterpret_cast<sockaddr*>(&address), &address_length),
+ 0);
+ _port = ntohs(address.sin_port);
+ _server_thread = std::thread([this] { serve_https(); });
+ }
+
+ void serve_https() {
+ sigset_t blocked_signals;
+ sigemptyset(&blocked_signals);
+ sigaddset(&blocked_signals, SIGPIPE);
+ ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_signals, nullptr), 0);
+ Defer drain_sigpipe {[&blocked_signals] {
+ const timespec no_wait {};
+ while (sigtimedwait(&blocked_signals, nullptr, &no_wait) ==
SIGPIPE) {
Review Comment:
[P2] Keep the TLS fixture's signal cleanup portable. On the macOS BE
unit-test build, this unconditional `sigtimedwait` call fails to compile
because Darwin does not provide it ([Apple signal
declarations](https://raw.githubusercontent.com/apple-oss-distributions/Libc/main/include/signal.h)).
`be/test/CMakeLists.txt` includes `service/*.cpp` on macOS, so this breaks the
whole `doris_be_test` target, even when the TLS module is disabled. The
repository already handles this exact platform limitation in
`be/CMakeLists.txt` and `boost-1.81.0-mac-sigtimedwait.patch`. Guard this
cleanup for supported platforms or provide a macOS-compatible SIGPIPE drain.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
