HPCC-34165 HTTPLIB add timeout to SSL_connect/accept #19895

mckellyln · 2025-05-21T12:57:30Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2025-05-21T12:58:31Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-34165

Jirabot Action Result:
Workflow Transition To: Merge Pending
Additional PR: #19895

Copilot

Pull Request Overview

Adds timeout support to SSL handshakes by making the socket non-blocking and polling until SSL_connect/SSL_accept complete or time out.

Introduces SSL_handleWait and SSL_timedAcceptorConnect for timed SSL handshakes.
Updates ssl_new to toggle non-blocking mode around the handshake.
Integrates the new timed handshake in both SSLServer and SSLClient.

Comments suppressed due to low confidence (2)

system/httplib/httplib.h:5491

[nitpick] Function name SSL_timedAcceptorConnect implies only accept but is also used for connect. Consider renaming to SSL_timedHandshake or similar to reflect its dual role.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

system/httplib/httplib.h:5491

No tests were added to verify the new handshake timeout behavior. Consider adding unit or integration tests that trigger both success and timeout paths.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

system/httplib/httplib.h

Copilot · 2025-05-21T13:00:41Z

system/httplib/httplib.h


+enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };


The enum SSLMethod is declared in the global scope and could collide; consider moving it into the detail namespace to avoid polluting the public API.

Suggested change

enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };

namespace detail {

enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };

} // namespace detail

Copilot · 2025-05-21T13:00:42Z

system/httplib/httplib.h


+enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
+


Add comments or docstrings for SSL_handleWait to clarify its behavior, parameters (especially errCode vs. sslErr), and return semantics.

Suggested change

/**

* Handles SSL/TLS wait conditions for read or write operations.

*

* This function checks the SSL error code and waits for the socket to become

* ready for reading or writing, based on the error condition. It uses a

* timeout mechanism to avoid indefinite blocking.

*

* @param ssl A pointer to the SSL object representing the connection.

* @param sock The socket file descriptor associated with the SSL connection.

* @param errCode The error code returned by a previous SSL operation.

* This is used to determine the type of wait condition.

* @param sslErr A pointer to an integer where the SSL error code will be stored.

* This is an output parameter that provides additional error details.

* @param remainingMs The maximum time to wait, in milliseconds, for the socket to

* become ready for the required operation.

* @return Returns 0 if the socket is ready, -1 if an error occurs, or

* a positive value if the timeout expires.

*/

Signed-off-by: M Kelly <mark.kelly+copilot@lexisnexisrisk.com>

Copilot

Pull Request Overview

Adds timeout support for SSL handshakes in HTTPLIB by switching to non-blocking I/O and polling.

Introduces SSL_handleWait and SSL_timedAcceptorConnect for timed SSL_accept/SSL_connect
Updates detail::ssl_new to perform non-blocking handshake and restore blocking mode
Modifies server and client code to use the timed handshake routine

Comments suppressed due to low confidence (2)

system/httplib/httplib.h:5491

[nitpick] The function name SSL_timedAcceptorConnect is ambiguous since it handles both accept and connect. Consider renaming it to something more general like SSL_timedHandshake.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

system/httplib/httplib.h:5463

New handshake timeout helpers were added but there are no tests covering these new code paths. Consider adding unit or integration tests for SSL handshake timeouts.

inline int SSL_handleWait(SSL *ssl, socket_t sock, int errCode, int *sslErr, unsigned remainingMs)

Copilot · 2025-05-21T13:22:57Z

system/httplib/httplib.h

+enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
+


The SSLMethod enum is declared in the global namespace, which can pollute the project namespace. Consider moving SSLMethod and its helper functions into the detail namespace to encapsulate implementation details.

Suggested change

enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };

namespace detail {

enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };

} // namespace detail

Copilot · 2025-05-21T13:22:57Z

system/httplib/httplib.h

+
+inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)
+{
+    unsigned timeoutMs = 60*1000;


[nitpick] Using a hardcoded magic number for the handshake timeout makes it inflexible. Extract this into a named constant or configuration parameter so it can be adjusted without code changes.

Suggested change

unsigned timeoutMs = 60*1000;

unsigned timeoutMs = CPPHTTPLIB_HANDSHAKE_TIMEOUT_MS;

mckellyln · 2025-05-21T18:06:49Z

@jakesmith does this look correct ?
We could probably also add the same non-blocking timeout support to SSL_read() and SSL_write().

jakesmith

I see that httplib.h explicitly warns that they do not support non-blocking:
https://github.com/yhirose/cpp-httplib#:~:text=Important,that%20you%20want.

Should we instead be switching to something more robust?
https://github.com/microsoft/cpprestsdk - not actively maintained though, but simple, fairly close to httplib in use I think
https://github.com/boostorg/beast - more widely used and maintained, but looks like requires quite a lot of boiler plate code

mckellyln · 2025-05-28T11:47:50Z

Should we add this ?
Or look at alternatives ?
@asselitx thoughts ?

asselitx · 2025-05-28T22:38:36Z

The change looks good, but if httplib is built expecting blocking sockets we may see other cases (like SSL_read and SSL_write that you pointed out) not handling timeouts. Also, I'm not sure- is there a danger of problems if the library doesn't support nonblocking anywhere else?

From what I see there have been few changes to lib over the last 5 years and its used only as a client for secrets and the elastic metrics sink. This is a good point where we either 'own it' and commit to customizing it or switch to another implementation.

Given that HTTP requires standards and security compliance would we want to keep our hands off the code and manage regular updates with perhaps vcpkg? If keeping current is important, then using microsoft/ccprestsdk might not be best.

boostorg/beast seems powerful, active and well supported but not terse - probably the best option if we really wanted to lean on the library and take upstream updates.

https://github.com/libcpr/cpr Could be a possibility if we only need clients and if we verify it does handle timeouts on accept/read/write

https://gitlab.com/eidheim/Simple-Web-Server Also has client support and seems pretty streamlined.

mckellyln requested a review from Copilot May 21, 2025 12:58

Copilot AI reviewed May 21, 2025

View reviewed changes

HPCC-34165 HTTPLIB add timeout to SSL_connect/accept

762fff2

Signed-off-by: M Kelly <mark.kelly+copilot@lexisnexisrisk.com>

mckellyln force-pushed the hpcc-34165 branch from d210683 to 762fff2 Compare May 21, 2025 13:10

mckellyln requested a review from Copilot May 21, 2025 13:21

Copilot AI reviewed May 21, 2025

View reviewed changes

mckellyln requested a review from jakesmith May 21, 2025 18:05

mckellyln requested a review from asselitx May 21, 2025 18:07

jakesmith reviewed May 22, 2025

View reviewed changes

mckellyln requested a review from ghalliday May 28, 2025 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HPCC-34165 HTTPLIB add timeout to SSL_connect/accept #19895

HPCC-34165 HTTPLIB add timeout to SSL_connect/accept #19895

Uh oh!

mckellyln commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI May 21, 2025

Uh oh!

Copilot AI May 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 21, 2025

Uh oh!

Copilot AI May 21, 2025

Uh oh!

mckellyln commented May 21, 2025

Uh oh!

jakesmith left a comment

Uh oh!

mckellyln commented May 28, 2025 •

edited

Loading

Uh oh!

asselitx commented May 28, 2025

Uh oh!

Uh oh!

+/**
+ * Handles SSL/TLS wait conditions for read or write operations.
+ *
+ * This function checks the SSL error code and waits for the socket to become
+ * ready for reading or writing, based on the error condition. It uses a
+ * timeout mechanism to avoid indefinite blocking.
+ *
+ * @param ssl        A pointer to the SSL object representing the connection.
+ * @param sock       The socket file descriptor associated with the SSL connection.
+ * @param errCode    The error code returned by a previous SSL operation.
+ *                   This is used to determine the type of wait condition.
+ * @param sslErr     A pointer to an integer where the SSL error code will be stored.
+ *                   This is an output parameter that provides additional error details.
+ * @param remainingMs The maximum time to wait, in milliseconds, for the socket to
+ *                   become ready for the required operation.
+ * @return           Returns 0 if the socket is ready, -1 if an error occurs, or
+ *                   a positive value if the timeout expires.
+ */

	unsigned timeoutMs = 60*1000;
	unsigned timeoutMs = CPPHTTPLIB_HANDSHAKE_TIMEOUT_MS;

HPCC-34165 HTTPLIB add timeout to SSL_connect/accept #19895

Are you sure you want to change the base?

HPCC-34165 HTTPLIB add timeout to SSL_connect/accept #19895

Uh oh!

Conversation

mckellyln commented May 21, 2025

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 21, 2025

Choose a reason for hiding this comment

Uh oh!

mckellyln commented May 21, 2025

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

mckellyln commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asselitx commented May 28, 2025

Uh oh!

Uh oh!

mckellyln commented May 28, 2025 •

edited

Loading