You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/10/16 05:24:52 UTC
[PR] fix(secret): never pair a certificate with a key from another secret version [apisix]
nic-6443 opened a new pull request, #14047:
URL: https://github.com/apache/apisix/pull/14047
### 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.
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. 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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(secret): never pair a certificate with a key from another secret version [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #14047:
URL: https://github.com/apache/apisix/pull/14047#discussion_r4291342662
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -243,8 +243,11 @@ function _M.set(matched_ssl, sni)
end
ngx_ssl.clear_certs()
- local new_ssl_value = secret.fetch_secrets(matched_ssl.value, true)
- or matched_ssl.value
+ local new_ssl_value
+ new_ssl_value, err = secret.fetch_ssl_secrets(matched_ssl.value)
+ if not new_ssl_value then
+ return false, err
Review Comment:
This isn't a regression: the old `fetch_secrets(matched_ssl.value, true)` call deep-copied the ssl object on every handshake as well. Skipping it is cheap and safe, though, because the resolved value is only read afterwards. So in 7f3ff8ad7 `fetch_ssl_secrets()` now returns the object as-is when it holds no secret refs. I put the check inside the function rather than at the call site, so the upstream client cert path gets it too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(secret): never pair a certificate with a key from another secret version [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #14047:
URL: https://github.com/apache/apisix/pull/14047#discussion_r4291296815
##########
apisix/secret.lua:
##########
@@ -171,6 +176,55 @@ end
_M.fetch_by_uri = fetch_by_uri_secret
+-- A provider resolves one field per call but reads the whole container (a
+-- vault path, a JSON secret, ...) to do it, so a certificate and its key are
+-- two reads that a rotation can fall between. This cache keeps a container read
+-- for a moment and makes concurrent readers in the worker share the read in
+-- flight, so fields resolved together come from the same read.
+local CONTAINER_TTL = 1
+local INFLIGHT_WAIT = 10
+
+function _M.new_container_cache()
+ local lru = lru_new(64)
+ local inflight = {}
+
+ return function (key, ver, read, ...)
+ local hit = lru:get(key)
+ if hit and hit.ver == ver then
+ return hit.val
+ end
+
+ local pending = inflight[key]
+ if pending and pending.ver == ver then
+ if pending.sema:wait(INFLIGHT_WAIT) then
+ return pending.val, pending.err, pending.extra
+ end
+ -- the reader may have been aborted along with its request
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ end
+
+ pending = {ver = ver, sema = semaphore.new()}
+ inflight[key] = pending
+ local val, err, extra = read(...)
+ pending.val, pending.err, pending.extra = val, err, extra
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ if val ~= nil then
+ lru:set(key, {val = val, ver = ver}, CONTAINER_TTL)
+ end
+
+ local waiting = -pending.sema:count()
+ if waiting > 0 then
+ pending.sema:post(waiting)
+ end
+ return val, err, extra
+ end
Review Comment:
The wake-up logic for in-flight readers is racy: computing `waiting = -pending.sema:count()` and posting that amount can miss waiters that start waiting after the `count()` snapshot but before `post()`, causing them to block until timeout even though the read completed. Consider switching to a completion flag + broadcast pattern that cannot miss late waiters (e.g., set `pending.done = true` and make waiters check `done` before waiting; use a chained semaphore release or an explicit waiters counter updated before sleeping), so completion reliably releases all current/future waiters for that `pending` instance.
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -243,8 +243,11 @@ function _M.set(matched_ssl, sni)
end
ngx_ssl.clear_certs()
- local new_ssl_value = secret.fetch_secrets(matched_ssl.value, true)
- or matched_ssl.value
+ local new_ssl_value
+ new_ssl_value, err = secret.fetch_ssl_secrets(matched_ssl.value)
+ if not new_ssl_value then
+ return false, err
Review Comment:
`fetch_ssl_secrets()` deep-copies and walks the SSL object; calling it unconditionally on every handshake can add avoidable overhead for inline PEM SSL objects (no secret refs). To prevent a per-handshake deepcopy in the common case, gate this call with a `has_secret_ref(matched_ssl.value)` check (fall back to `matched_ssl.value` when false), or alternatively change `fetch_ssl_secrets()` to fast-path return when none of the SSL material fields contain secret refs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(secret): never pair a certificate with a key from another secret version [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #14047:
URL: https://github.com/apache/apisix/pull/14047#discussion_r4291341265
##########
apisix/secret.lua:
##########
@@ -171,6 +176,55 @@ end
_M.fetch_by_uri = fetch_by_uri_secret
+-- A provider resolves one field per call but reads the whole container (a
+-- vault path, a JSON secret, ...) to do it, so a certificate and its key are
+-- two reads that a rotation can fall between. This cache keeps a container read
+-- for a moment and makes concurrent readers in the worker share the read in
+-- flight, so fields resolved together come from the same read.
+local CONTAINER_TTL = 1
+local INFLIGHT_WAIT = 10
+
+function _M.new_container_cache()
+ local lru = lru_new(64)
+ local inflight = {}
+
+ return function (key, ver, read, ...)
+ local hit = lru:get(key)
+ if hit and hit.ver == ver then
+ return hit.val
+ end
+
+ local pending = inflight[key]
+ if pending and pending.ver == ver then
+ if pending.sema:wait(INFLIGHT_WAIT) then
+ return pending.val, pending.err, pending.extra
+ end
+ -- the reader may have been aborted along with its request
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ end
+
+ pending = {ver = ver, sema = semaphore.new()}
+ inflight[key] = pending
+ local val, err, extra = read(...)
+ pending.val, pending.err, pending.extra = val, err, extra
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ if val ~= nil then
+ lru:set(key, {val = val, ver = ver}, CONTAINER_TTL)
+ end
+
+ local waiting = -pending.sema:count()
+ if waiting > 0 then
+ pending.sema:post(waiting)
+ end
+ return val, err, extra
+ end
Review Comment:
I don't think this race can happen. Everything after the read returns runs without a yield: store the result on `pending`, clear `inflight[key]`, `lru:set`, then `count()` and `post()`. None of those can yield, so no other coroutine runs between the `count()` snapshot and the `post()`, and nobody can start waiting in that window.
A caller can only get hold of `pending` through `inflight[key]`. That entry is cleared in the same non-yielding step, before the count, so anyone arriving later either hits the lru entry or creates a fresh `pending` with its own read. It never waits on the finished one. The head of this PR also moved the read into a timer under `pcall`, so every exit of the read goes through this same release path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org