Skip to content

fix(secret): never pair a certificate with a key from another secret version - #14047

Open
nic-6443 wants to merge 4 commits into
apache:masterfrom
nic-6443:fix/ssl-secret-atomic-pair
Open

nic-6443 wants to merge 4 commits into
apache:masterfrom
nic-6443:fix/ssl-secret-atomic-pair

Conversation

@nic-6443

@nic-6443 nic-6443 commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Description

An SSL object whose cert and key are secret references can be served with the certificate of one secret version and the key of another, and the handshake then fails with failed to set PEM priv key: SSL_use_PrivateKey() failed.

The cause is that each field is resolved and cached on its own. radixtree_sni.set() and the upstream client cert path call fetch_secrets(value, true), which puts cert and key into two independent per-URI lrucache entries. The stale-refresh timer then refreshes them one key at a time, and each refresh is its own provider read: vault reads the whole path to return one field, and so do the AWS and GCP managers for JSON secrets. A rotation that lands between the two reads gives a mismatched pair, which then stays cached for the full TTL. The two entries can also drift apart for longer when one refresh fails, or when they were first fetched at different times.

This PR fixes it in two places, and both are needed:

  • secret.fetch_ssl_secrets() resolves the cert, key, certs and keys references of an ssl object as one unit. They are fetched together, cached as one entry keyed by the sorted set of references, and swapped together when the stale entry is refreshed. The cache has the same TTL, stale-while-revalidate behaviour and /secrets version as the per-URI cache, so ssl objects that carry the same references still share one entry and switch together. The server cert path in radixtree_sni.lua and resolve_upstream_client_cert() now use it. Other fields of the ssl object, and plugin, consumer and $env:// resolution, work as before.
  • vault, AWS and GCP keep each container they read (a vault path, a secret id) for one second, and concurrent readers in a worker wait for the read already in flight instead of starting their own. That way a cert and its key in the same container come from one read. This only ever removes provider requests. The shared read runs in a timer under pcall, so a caller whose handshake is aborted cannot strand the others, and a failed read releases every waiter at once with its error. A waiter never waits longer than the provider's own request timeout.

A behaviour change worth noting: if one of the cert/key references of an ssl object cannot be resolved, the handshake now fails with that resolution error. Before, the literal reference was passed on and the handshake failed later when parsing the PEM. Inline PEM ssl objects are not affected.

A cert and key kept in two different containers are still two independent reads. The docs now say to keep them in the same secret.

t/secret/ssl-rotation.t uses a mock vault that returns the next version of the pair on every read. It covers server handshakes and upstream mTLS requests across several refreshes, plus a cert fetched after its key had been cached separately, and callers of a shared container read being released when the read fails or its first caller is killed. On master the handshakes and upstream requests fail. With only the provider change, the separate-age case still fails. With only the unit cache, the rotation cases still fail.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

Copilot AI balanced review requested due to automatic review settings October 16, 2026 05:24

Copilot AI 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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open findings
What changed in this PR

Fixes TLS handshake failures caused by mismatched certificate/key pairs when secret-backed SSL materials rotate and are refreshed independently.

Changes:

  • Add unit-resolution + unit-cache for SSL cert/key/certs/keys so they refresh atomically.
  • Add short-lived provider “container” caching with in-flight de-duplication for Vault/AWS/GCP reads.
  • Add rotation regression tests and update docs to describe the new behavior and guidance.
File Description
apisix/​secret.lua Adds container cache helper and SSL material unit caching (fetch_ssl_secrets).
apisix/​ssl/​router/​radixtree_sni.lua Switches server cert path to fetch_ssl_secrets and fails handshake on resolution error.
apisix/​init.lua Switches upstream mTLS client cert resolution to fetch_ssl_secrets.
apisix/​secret/​vault.lua Uses container cache to avoid split reads across rotations.
apisix/​secret/​aws.lua Uses container cache to avoid split reads across rotations.
apisix/​secret/​gcp.lua Uses container cache to avoid split reads across rotations.
t/​secret/​ssl-rotation.t Adds regression coverage for rotation + stale refresh scenarios.
docs/​en/​latest/​terminology/​secret.md Documents atomic fetching/refresh and resolver failure behavior for SSL secrets.
docs/​zh/​latest/​terminology/​secret.md Same doc update in Chinese.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apisix/secret.lua Outdated
Comment thread apisix/ssl/router/radixtree_sni.lua
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