Migrate node join client from httpclient to libcurl#8040
Open
achamayou wants to merge 23 commits into
Open
Conversation
added 11 commits
July 7, 2026 18:14
The CurlRequest HTTP_POST branch previously assumed the caller had pre-set CURLOPT_POSTFIELDS and did not enable CURLOPT_POST, so passing a RequestBody for a POST did not transmit the body. Enable CURLOPT_POST and declare the body size via CURLOPT_POSTFIELDSIZE_LARGE so the existing RequestBody read callback is used and a Content-Length is sent. Adds a curl_test case that posts a body to the echo server and asserts the method and body round-trip.
Replace the RPCSessions::create_client join path with an async curl request via CurlmLibuvContextSingleton, matching the JWT refresh and snapshot fetch clients. The service certificate remains the sole trust anchor (CURLOPT_CAINFO_BLOB + CURLOPT_CAPATH=nullptr), peer verification is enforced (VERIFYPEER=1), and hostname verification is now enabled (VERIFYHOST=2). The joining node presents its self-signed node certificate for mutual TLS. All existing response-handling branches (redirect, StartupSeqnoIsOld snapshot fetch, TRUSTED/PENDING, fatal errors) are preserved.
The legacy httpclient join path only invoked its fatal error callback for TLS handshake failures; transport-level failures (connection refused, unresolved host, timeouts) silently closed the connection and were retried by the periodic join timer. The initial curl migration treated every non-OK CURLcode as fatal, which caused nodes to shut down on a transient 'could not connect' during normal startup races. Categorise transient transport errors (COULDNT_RESOLVE_*, COULDNT_CONNECT, OPERATION_TIMEDOUT, GOT_NOTHING, RECV/SEND_ERROR, PARTIAL_FILE) as retryable, and keep TLS/protocol failures (including peer verification failures for an untrusted service certificate) fatal. Add connect and total timeouts so stalled attempts are abandoned for the timer to retry.
The curl-based join client reports an untrusted service certificate as "invalid service certificate" rather than the legacy TLS session's "invalid cert on handshake". Update the ServiceCertificateInvalid detection to match the new wording, retaining the old string for mixed-version test compatibility.
With the node join migrated to the curl client, the enclave-side HTTP client sessions are no longer used. Remove make_http_request (and its AbstractNodeState declaration), RPCSessions::create_client and create_unencrypted_client (and the now-dead client session id helper), and the HTTPClientSession, HTTP2ClientSession and UnencryptedHTTPClientSession classes along with the ClientSession base. The in-process NodeClient/HTTPNodeClient (used by retired node cleanup) and the tls::Client primitive (still exercised by the TLS unit tests and underpinning server-side TLS) are retained. Completes the httpclient removal tracked in #7262.
Add CHANGELOG entries for the join client migration to curl (including the new TLS hostname-verification requirement on the join connection) and the removal of the legacy httpclient infrastructure. Add an operator note to the start-network docs explaining that the target node's certificate SANs must cover join.target_rpc_address.
…t errors Move the join client's transient-vs-fatal CURLcode decision into a reusable, unit-tested ccf::curl::is_transient_transport_error() helper, and add CURLE_WEIRD_SERVER_REPLY, CURLE_HTTP2 and CURLE_HTTP2_STREAM to the retryable set. The legacy httpclient join silently retried these transfer/parse failures via the periodic timer, whereas the initial curl migration treated everything outside a small allow-list as fatal, so a transient garbled reply or HTTP/2 framing error during a startup race could shut a node down. CURLE_WRITE_ERROR (our own response size-cap rejection) remains fatal by design. Adds a table-driven curl_test that pins the full classification.
The curl response callback runs on the shared host libuv event loop thread. The previous code did all join processing there under NodeState::lock, including deserialising a potentially large snapshot and issuing KV/ledger writes. That stalls every other user of the shared curl loop (JWT refresh, quote endorsements, recovery decision) and risks a deadlock: the blocking host ring-buffer writer spin-waits for space while its only consumer runs on that same libuv thread. Restructure the callback to mirror the JWT refresh client: on the libuv thread it only captures the response (headers and body) and schedules a ccf::tasks task; all NodeState::lock-holding work then runs on a task thread, as it did before the curl migration (when the callback ran on the enclave RPC thread). This also removes the latent self-deadlock where a synchronous abort during shutdown could re-enter the callback while lock was held. Aborted requests are dropped on the libuv thread without scheduling a task.
Guard the TLS hostname-verification hardening the node join client relies on. The e2e_curl harness now also serves HTTPS with a self-signed certificate whose only SAN is a dNSName that does not cover the loopback IP it is served on. A new curl_test then asserts, with that certificate pinned as the CA, that CURLOPT_SSL_VERIFYHOST=2 rejects a request to the IP (CURLE_PEER_FAILED_VERIFICATION), accepts one to the SAN name resolved to the same address, and that VERIFYHOST=0 accepts the mismatch (control, proving the hostname check is the sole discriminator). The test skips cleanly when run outside the harness.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the migration of the node join client from the legacy in-enclave HTTP client (RPCSessions::create_client()) to the existing libcurl multi singleton, and removes the now-unused legacy HTTP client/session infrastructure. This aligns join with the already-migrated JWT/JWK refresh, endorsements, and snapshot fetch clients, while hardening join by enforcing TLS hostname verification.
Changes:
- Extend the curl wrapper to support POST-with-body and add curl unit/e2e coverage (including VERIFYHOST behavior).
- Rewrite
NodeState::initiate_join_unsafe()to perform/node/joinviaCurlmLibuvContextSingleton, preserving prior join control-flow branches and error semantics. - Remove legacy enclave-side HTTP client sessions/APIs, update docs and changelog, and adjust test infra log matching for invalid service certificates.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/infra/network.py | Updates join failure log matching to recognize the new curl-based “invalid service certificate” wording while retaining legacy compatibility. |
| tests/e2e_curl.py | Extends the curl e2e harness to stand up an HTTPS endpoint with a controlled SAN mismatch for VERIFYHOST coverage. |
| src/node/rpc/node_interface.h | Removes the legacy make_http_request API from the node interface. |
| src/node/node_state.h | Migrates join POST to async curl-multi with pinned service cert, mTLS client cert/key, and VERIFYHOST enforcement. |
| src/http/test/curl_test.cpp | Adds unit coverage for POST-with-body and VERIFYHOST SAN mismatch behavior, plus error classification tests. |
| src/http/http2_session.h | Removes the legacy HTTP2ClientSession implementation. |
| src/http/http_session.h | Removes the legacy HTTPClientSession and UnencryptedHTTPClientSession implementations. |
| src/http/curl.h | Adds retryability classification helper and first-class POST-with-body support in CurlRequest. |
| src/enclave/rpc_sessions.h | Removes create_client/create_unencrypted_client and associated client-session ID plumbing. |
| src/enclave/client_session.h | Deletes the legacy ClientSession base class. |
| doc/operations/start_network.rst | Documents the new join-time TLS hostname verification requirement and SAN/operator implications. |
| doc/dev/join_curl_migration_plan.md | Adds a detailed internal migration plan and rationale for the staged approach and validation gates. |
| CHANGELOG.md | Records the join migration + hostname verification hardening and the removal of the legacy enclave HTTP client infra. |
The periodic join timer fires every config.join.retry_timeout (default 1s), but a join attempt can remain in flight for up to CONNECTTIMEOUT/TIMEOUT. Because the join client shares the CurlmLibuvContextSingleton with the JWT refresh, endorsements and snapshot-fetch clients, unguarded retries against a slow or unresponsive target could accumulate in-flight requests and starve those other users. Track an atomic join_request_in_flight flag: initiate_join_unsafe() skips the retry if a request is already in flight, sets the flag before attaching the request, and the response callback clears it (on the libuv thread, so via an atomic store rather than NodeState::lock) for every completion path including synchronous abort during shutdown.
Follow-up to the single-in-flight join gate. If attach_request (or the singleton accessor) throws after the in-flight flag is set - realistically only under an OOM-class bad_alloc - the response callback never runs, so the flag would remain set and the periodic timer would gate out every future retry, stranding the node in pending. Wrap the handoff so the flag is reset on the exceptional path. Also add two clarifying comments: that join liveness depends on the curl singleton invoking each request callback exactly once, and that start_join_timer's initial initiate_join_unsafe call relies on the caller holding NodeState::lock.
added 2 commits
July 9, 2026 06:43
The join client flagged CURLE_PEER_FAILED_VERIFICATION (and CURLE_SSL_CACERT_BADFILE) as an "invalid service certificate", but with VERIFYHOST=2 that CURLcode is also returned for a hostname/SAN mismatch or any other peer certificate verification failure, so the wording could mislead operators. Replace the marker with the accurate "TLS certificate verification failed", rename the local bool, and broaden the code comment. Update the test infra matcher and comment in network.py to the new marker (still matching the legacy "invalid cert on handshake" wording for mixed-version tests). The marker was introduced by this unreleased PR, so there is no released node emitting the old text.
Address review nits on the join certificate-failure path. The fatal branch fires for both CURLE_PEER_FAILED_VERIFICATION and CURLE_SSL_CACERT_BADFILE, but the previous "TLS certificate verification failed" marker and comments only described peer verification, not the case where the configured service certificate blob cannot be loaded (CURLE_SSL_CACERT_BADFILE). Reword the marker to the more accurate "TLS certificate trust check failed" (updating the local bool and the test-infra matcher to match), expand both code comments to enumerate both underlying causes, and document the ServiceCertificateInvalid test exception, which now covers all join-time certificate trust failures rather than only a literally invalid certificate.
achamayou
commented
Jul 9, 2026
Member
Author
|
This migration plan was drafted during development. Keeping it as a PR comment for reviewer context rather than committing it to the repository (it previously lived at Migrating the node join client from the legacy HTTP client to libcurlTracking issue: #7262 - "Migrate Reference implementation: #8005, Goals
Non-goals
Current implementation (baseline)
auto network_ca = std::make_shared<::tls::CA>(std::string(
config.join.service_cert.begin(), config.join.service_cert.end()));
auto join_client_cert = std::make_unique<::tls::Cert>(
network_ca,
self_signed_node_cert,
node_sign_kp->private_key_pem(),
target_host);
auto join_client = rpcsessions->create_client(
std::move(join_client_cert),
rpcsessions->get_app_protocol_main_interface());
join_client->connect(target_host, target_port, /* response cb */, /* error cb */);
join_client->send_request(POST /node/join with JSON body);Relevant properties of the baseline:
Join response-handling branches to preserve
Target implementationModel on curl easy-handle options for the join request:
POST-with-body support in the curl wrapper (prerequisite)
Security analysis: which root CAs are accepted
Hostname verification hardening. Enabling Test strategy and validation gatesExisting coverage to lean on
Gaps to plug
Removal scope (after the join no longer uses the legacy client)Remove (confirmed to have no other users once the join and the dead
Keep:
Staged delivery (each stage is independently reviewable and gated)
|
cjen1-msft
reviewed
Jul 9, 2026
cjen1-msft
approved these changes
Jul 9, 2026
cjen1-msft
left a comment
cjen1-msft
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me.
One comment regarding an alternative way to sequence the retries which might simplify things (it did for fetch):
You can rely on the exactly once response from the curl backend to sequence the retries as you have perfect info of why the response failed/timed-out/succeeded.
- Remove doc/dev/join_curl_migration_plan.md; the plan is preserved as a comment on PR #8040 rather than shipped in the tree. - Add an explanatory comment for CURLOPT_POST=1L in the curl wrapper, linking to the libcurl documentation. - Split the CHANGELOG entry so the new join-time target RPC address SAN check (CURLOPT_SSL_VERIFYHOST) is highlighted as its own item.
7.0.7 has been released, so this PR's entries belong in a new dev version. Open [7.0.8] with the #8040 Changed and Removed entries and bump python/pyproject.toml to 7.0.8 to match.
- Set CURLOPT_INFILESIZE only for PUT uploads; POST already declares its body size via CURLOPT_POSTFIELDSIZE_LARGE, so the shared attach_to_curl no longer sets it redundantly. - Map CURLE_WRITE_ERROR on the join to a clear fatal message naming the response size cap, instead of libcurl's generic write error.
Since the join client was migrated to libcurl, a joining node verifies (CURLOPT_SSL_VERIFYHOST=2) that the target node's certificate covers the address it dialled. Add run_tls_san_join_mismatch, a dedicated-network test where the target's file-serving interface binds an address deliberately absent from the node certificate SANs, so a joining node dialling it is rejected with ServiceCertificateInvalid. This exercises the hostname/SAN check specifically, distinct from the untrusted-service-certificate path already covered by test_add_node_invalid_service_cert.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7262.
Migrates the node join sequence - the last remaining user of the legacy enclave
RPCSessions::create_client()HTTP client - to the curl multi singleton client already used for attestation endorsements, JWT/JWK refresh (#8005), and snapshot fetch. The now-unused httpclient infrastructure is then removed.Approach (staged)
ccf::curl::CurlRequest(+curl_testcoverage).NodeState::initiate_join_unsafeto build an async curl POST to/node/joinviaCurlmLibuvContextSingleton, preserving every existing response branch (redirect,StartupSeqnoIsOldsnapshot fetch,TRUSTED/PENDING, fatal errors).make_http_request,RPCSessions::create_client/create_unencrypted_client, and theHTTPClientSession/HTTP2ClientSession/UnencryptedHTTPClientSession/ClientSessionclasses.Security posture
The service certificate remains the sole trust anchor for the join connection:
CURLOPT_CAINFO_BLOBinstalls it andCURLOPT_CAPATH=nullptrprevents any fallback to the host CA store, so the set of accepted CAs is identical to the legacytls::CApath. Peer chain verification is preserved (VERIFYPEER=1), and the joining node still presents its self-signed node certificate for mutual TLS.Behavioural change (hardening): TLS certificate hostname verification is now enforced on the join connection (
VERIFYHOST=2); the legacy path used SNI only. Operators must ensure the target node's certificate SANs cover the address injoin.target_rpc_address(derived from the interfacepublished_address, or set vianode_certificate.subject_alt_names). Documented in the CHANGELOG andstart_network.rst.Error semantics
The legacy join's fatal error callback was effectively dead code: transport failures (connection refused, unresolved host, timeouts) were silently retried by the periodic join timer, while only TLS handshake failures were fatal. This split is preserved - transient transport
CURLcodes retry; TLS/certificate failures (including an untrusted service certificate) remain fatal and shut the node down.Retained (not part of the httpclient)
The in-process
NodeClient/HTTPNodeClient(used by retired-node cleanup) and thetls::Clientprimitive (server-side TLS + TLS unit tests) are kept.Testing
curl_test(incl. new POST test),http_test,node_frontend_test,tls_test,channels_test,frontend_test.reconfiguration.run_all- basic/snapshot/backup/redirect joins plus thetest_add_node_invalid_service_certsecurity gate (wrong service cert ->CURLE_PEER_FAILED_VERIFICATION-> node stops).lts_compatibilitypassed against previous LTSccf-6.0.28(cross-version join).sphinx --fail-on-warning.