Skip to content

Migrate OIDC discovery and key fetching from legacy HTTP client to new curl-based client#8005

Merged
achamayou merged 30 commits into
mainfrom
copilot/jwk-refresh-curl-multi-plan
Jul 2, 2026
Merged

Migrate OIDC discovery and key fetching from legacy HTTP client to new curl-based client#8005
achamayou merged 30 commits into
mainfrom
copilot/jwk-refresh-curl-multi-plan

Conversation

@achamayou

@achamayou achamayou commented Jun 29, 2026

Copy link
Copy Markdown
Member

Closes items 2 and 3 in #7262 by migrating JWK download and refresh to the curl client, plus a few fixes to the client along the way.

This leaves the Join client as the last httpclient user, to be removed in a separate PR.

Review follow-up:

  • Curl shutdown now aborts queued async requests instead of performing network I/O during close.
  • Curl response header capture now enforces the default HTTP response header size/count limits, and JWT auto-refresh coverage asserts oversized response headers are rejected before a normal retry succeeds.

achamayou and others added 2 commits June 25, 2026 18:37
…#7989)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 29, 2026 20:11
@achamayou achamayou requested a review from a team as a code owner June 29, 2026 20:11
@achamayou

Copy link
Copy Markdown
Member Author

@copilot create a summary of the changes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates CCF’s JWT/JWK auto-refresh implementation to use the existing libcurl multi client (improving observability of network/TLS failures) and introduces a new startup configuration to cap the maximum accepted response size when fetching OpenID metadata and JWKS.

Changes:

  • Switch JWT/JWK auto-refresh outbound fetches from RPCSessions::create_client() to the curl multi singleton, counting connection/TLS failures in refresh failure metrics.
  • Add jwt.key_refresh_max_response_size to limit OpenID metadata/JWKS response body sizes, wiring it through config, schema, CLI/test infra, and docs.
  • Expand JWT auto-refresh tests to cover connection failure, TLS failure, invalid metadata, cross-authority jwks_uri, and response-size limiting.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/programmability.py Sets a smaller max-response-size in a programmability test network config.
tests/jwt_test.py Adds several new auto-refresh negative/edge-case tests and adjusts metrics assertions.
tests/infra/remote.py Threads jwt_key_refresh_max_response_size through node startup config rendering.
tests/infra/network.py Forwards jwt_key_refresh_max_response_size as a supported node arg.
tests/infra/jwt_issuer.py Adds SAN to generated issuer certs (needed for strict curl hostname verification).
tests/infra/e2e_args.py Adds CLI arg --jwt-key-refresh-max-response-size.
tests/infra/crypto.py Adds optional SAN support to test certificate generation.
tests/config.jinja Emits jwt.key_refresh_max_response_size into generated node config JSON.
src/node/node_state.h Passes max-response-size config into JwtKeyAutoRefresh.
src/node/jwt_key_auto_refresh.h Reworks auto-refresh fetch path to curl multi, adds https validation and response size limiting.
src/http/curl.h Improves curl error handling/checking and shutdown logging; updates slist append behaviour.
src/common/configuration.h Adds JWT config JSON field key_refresh_max_response_size.
include/ccf/node/startup_config.h Defines new startup config field key_refresh_max_response_size.
doc/host_config_schema/host_config.json Documents the new JWT config option in the host config schema.
doc/build_apps/auth/jwt.rst Updates JWT docs to mention key_refresh_interval and the new max-response-size setting.
python/pyproject.toml Bumps Python package version to 7.0.7.
CHANGELOG.md Adds 7.0.7 entries describing the curl multi switch and max response size configuration.

Comment thread src/http/curl.h Outdated
Comment thread doc/build_apps/auth/jwt.rst Outdated
Comment thread tests/jwt_test.py Outdated
Comment thread tests/jwt_test.py

Copilot AI commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

@copilot create a summary of the changes.

  • Migrates JWT/JWK auto-refresh fetches for OpenID metadata and JWKS from RPCSessions::create_client() to the shared curl multi client path.
  • Counts connection- and TLS-level fetch failures in the JWT refresh error metrics, improving observability for refresh failures.
  • Adds jwt.key_refresh_max_response_size (default 1 MB) to cap the response body size accepted when fetching OpenID metadata and JWKS.
  • Wires that new setting through startup config/schema/docs and the Python test infra.
  • Expands JWT refresh coverage with tests for connection failure, TLS failure, invalid metadata, cross-authority jwks_uri, and oversized responses.

@achamayou achamayou changed the title Copilot/jwk refresh curl multi plan Migrate OIDC discovery and key fetching from legacy HTTP client to new curl-based client Jun 30, 2026
@achamayou achamayou requested a review from Copilot June 30, 2026 08:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread src/http/curl.h Outdated
Comment thread src/http/curl.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment thread tests/jwt_test.py
Comment thread tests/jwt_test.py Outdated
Comment thread src/http/curl.h Outdated
Comment thread src/http/curl.h
Comment thread src/node/jwt_key_auto_refresh.h
…quirement

Co-authored-by: maxtropets <16566519+maxtropets@users.noreply.github.com>
Copilot AI requested a review from maxtropets June 30, 2026 11:41
@maxtropets

Copy link
Copy Markdown
Collaborator

Not able to review this due to unfamiliarity with both libuv and libcurl, but here's maybe useful feedback from LLM

2. Shutdown can block on network I/O.
 src/http/curl.h:1069-1073  now drains pending async requests by calling  CurlRequest::synchronous_perform() ,
which calls  curl_easy_perform()  ( src/http/curl.h:510-526 ).
Some existing shared-curl users do not set curl timeouts, for example  recovery_decision_protocol.cpp:500-557 ,
so shutdown can hang on connect/transfer instead of aborting queued work.
Pending requests should be completed as aborted or discarded without performing network I/O during close.

3. JWT refresh lost the old response-header size protection.
The old  RPCSessions  path used CCF’s  ResponseParser , which enforces default header size/count limits ( src/http/http_parser.h:315-347 ).
The new curl path captures response headers via  ResponseHeaders  without equivalent limits ( src/http/curl.h:324-370 ), 
while only the body is capped ( src/node/jwt_key_auto_refresh.h:65-72 ).
The existing oversized-header test fixture sends a 32KB header ( tests/infra/jwt_issuer.py:55-59 ),
but the updated assertions no longer verify rejection.
Add header size/count limits to the curl response path,
or avoid storing headers for JWT refresh since they are unused.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

Comment thread src/http/curl.h
Comment thread src/http/curl.h Outdated
Comment thread src/http/curl.h Outdated
Comment thread src/http/curl.h
@achamayou achamayou enabled auto-merge (squash) July 2, 2026 20:01
@achamayou achamayou merged commit 6c41d92 into main Jul 2, 2026
17 checks passed
@achamayou achamayou deleted the copilot/jwk-refresh-curl-multi-plan branch July 2, 2026 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants