Skip to content

feat(rest): add PQC curves to REST client#16233

Closed
hongalex wants to merge 2 commits into
googleapis:mainfrom
hongalex:feat-support-pqc
Closed

feat(rest): add PQC curves to REST client#16233
hongalex wants to merge 2 commits into
googleapis:mainfrom
hongalex:feat-support-pqc

Conversation

@hongalex

@hongalex hongalex commented Jul 2, 2026

Copy link
Copy Markdown
Member

Optionally enable PQC curves for REST client

Explicitly configure CURLOPT_SSL_EC_CURVES in the libcurl backend to
prefer X25519MLKEM768 (the hybrid post-quantum key exchange group),
followed by classical curves (X25519, P-256, P-384).

This enables Post-Quantum Cryptography (PQC) compliance for C++ REST
clients when connecting to servers that support it (such as Showcase
running on Go 1.24+ or Google Cloud services supporting PQC).

We use CURLOPT_SSL_EC_CURVES for compatibility with older libcurl
headers. If setting the option fails, we log an INFO message and
continue with libcurl/SSL library defaults, ensuring no regression
on older platforms.

TAG=agy
CONV=a6dbbcf8-b85b-4925-b9cf-7e1227fee2c0

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a call to configure SSL elliptic curves using CURLOPT_SSL_EC_CURVES in CurlImpl::MakeRequestImpl. The review feedback correctly points out that reusing the status variable for an option whose failure is ignored is risky, as it leaves status in a non-OK state temporarily. It is recommended to use a separate local variable for this check to prevent accidental propagation of the error state.

Comment on lines +604 to +607
status = handle_.SetOption(CURLOPT_SSL_EC_CURVES, "X25519MLKEM768:X25519:P-256:P-384");
if (!status.ok()) {
GCP_LOG(INFO) << "CURLOPT_SSL_EC_CURVES failed: " << status;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Reusing the status variable for an option whose failure is explicitly ignored is risky. If CURLOPT_SSL_EC_CURVES fails, status is left in a non-OK state until it is overwritten by the next SetOption call. If the code is refactored, reordered, or if another check is inserted in between, this temporary error state could be accidentally propagated or returned.

Using a separate local variable (e.g., ec_status) avoids polluting the main status variable and makes the intent to ignore the failure much clearer and safer.

  auto ec_status = handle_.SetOption(CURLOPT_SSL_EC_CURVES, "X25519MLKEM768:X25519:P-256:P-384");
  if (!ec_status.ok()) {
    GCP_LOG(INFO) << "CURLOPT_SSL_EC_CURVES failed: " << ec_status;
  }
References
  1. Prefer defensive code, such as explicit ok() checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.27%. Comparing base (5f006f3) to head (9f7838f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/internal/curl_impl.cc 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16233   +/-   ##
=======================================
  Coverage   92.26%   92.27%           
=======================================
  Files        2214     2214           
  Lines      206394   206397    +3     
=======================================
+ Hits       190437   190444    +7     
+ Misses      15957    15953    -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@scotthart

Copy link
Copy Markdown
Member

Superseded by #16243

@scotthart scotthart closed this Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants