feat(stream): support upstream mTLS client cert via C-API#114
Merged
Conversation
Mirror the http subsystem's set_cert_and_key mechanism for the stream proxy, so APISIX can present a client certificate to a TLS upstream over L4 without exposing the private key as an nginx variable. - ngx_stream_apisix_module: store parsed cert/pkey in the module ctx (set_cert_and_key), apply them at the upstream SSL handshake via set_upstream_ssl (SSL_use_certificate / SSL_add1_chain_cert / SSL_use_PrivateKey), with pool-cleanup-based freeing. - lib/resty/apisix/stream/upstream: set_cert_and_key Lua binding. - patch/1.29.2.4: inject ngx_stream_apisix_set_upstream_ssl(s, pc) right before ngx_ssl_handshake(pc) in ngx_stream_proxy_ssl_init_connection (the <ngx_stream_apisix_module.h> include is already added by nginx-tcp_over_tls.patch). This is the foundation for replacing the stream proxy_ssl_certificate 'data:' variable approach (apache/apisix#13596) with the same parse-once, no-key-in-variable path http already uses.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds upstream mTLS support for stream proxy. The per-request context gains SSL cert/key fields with pool cleanup. Two new C functions store and apply upstream client certificates at SSL handshake time. Nginx patches hook the apply function into ChangesStream upstream mTLS
Sequence DiagramsequenceDiagram
participant LuaPlugin as Lua Plugin
participant UpstreamLua as upstream.lua (set_cert_and_key)
participant CModule as ngx_stream_apisix_module.c
participant NginxSSL as ngx_stream_proxy_ssl_init_connection
participant OpenSSL as OpenSSL
LuaPlugin->>UpstreamLua: set_cert_and_key(cert, key)
UpstreamLua->>CModule: ngx_stream_apisix_upstream_set_cert_and_key(r, cert, key)
CModule->>CModule: free previous cert/key, deep-copy chain, refcount key
CModule->>CModule: store in ctx->upstream_cert / upstream_pkey
CModule-->>UpstreamLua: NGX_OK
UpstreamLua-->>LuaPlugin: true
Note over NginxSSL: upstream SSL handshake begins
NginxSSL->>CModule: ngx_stream_apisix_set_upstream_ssl(s, pc)
CModule->>CModule: read ctx->upstream_cert, upstream_pkey
CModule->>OpenSSL: SSL_use_certificate, SSL_add1_chain_cert, SSL_use_PrivateKey
OpenSSL-->>CModule: success
CModule-->>NginxSSL: return
NginxSSL->>OpenSSL: ngx_ssl_handshake(pc)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add t/stream/upstream_mtls.t exercising resty.apisix.stream.upstream set_cert_and_key over a real stream proxy to a TLS upstream that requires a client certificate (ssl_verify_client on): - valid client cert+key -> mTLS handshake succeeds (ssl_client_verify SUCCESS) - missing private key -> rejected by the Lua wrapper before handshake - wrong client cert (not signed by the upstream CA) -> upstream verify error - repeated set_cert_and_key -> still handshakes (covers the re-entrant cleanup path in ngx_stream_apisix_upstream_set_cert_and_key) Mirrors the existing http t/upstream_mtls.t for the stream subsystem; the feature previously had no test coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@t/stream/upstream_mtls.t`:
- Around line 65-89: The TEST 2 block currently validates that set_cert_and_key
rejects the missing private key argument, but does not verify that the upstream
mTLS handshake actually failed. Add a negative handshake assertion after the
error_log section to ensure the TLS handshake did not succeed, for example by
checking that a success confirmation pattern (such as "client verify: SUCCESS")
is absent from the logs or error output, or by asserting that an expected
handshake-failure pattern is present. This prevents the test from passing if
cert/key state from previous tests are accidentally reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 860978b0-a26c-4ae8-91b9-00f45beb7fe9
📒 Files selected for processing (1)
t/stream/upstream_mtls.t
The set_upstream_ssl injection that actually presents the client cert during the upstream TLS handshake lived only in patch/1.29.2.4/. Every other runtime that has nginx-tcp_over_tls.patch (1.19.9, 1.21.4, 1.21.4.1, 1.25.3.1, 1.27.1.1) was missing it, so on those builds set_cert_and_key stored the cert but it was never applied -- the upstream rejected the handshake with "400 No required SSL certificate was sent". This was invisible until t/stream/upstream_mtls.t was added; it surfaced on the api7ee-runtime build (nginx 1.21.4). The injection point (before ngx_ssl_handshake in ngx_stream_proxy_ssl_init_connection) is identical across these versions, so the patch body is shared; patch -p0 applies it by context.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack a concrete re-enable path for the excluded xrpc tests.
This skip is reasonable short-term, but without a tracked follow-up it can silently become permanent and reduce coverage on
apisix-runtime. Please add an issue reference/TODO with re-enable criteria in this comment block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 18 - 21, The comment block explaining why the xrpc tests are excluded (t/stream/xrpc/downstream.t and t/stream/xrpc/upstream.t in the exclude_tests configuration) lacks a tracked follow-up mechanism for re-enabling them. Add a TODO comment or issue reference to the existing comment block that specifies the concrete criteria or conditions for when these tests should be re-enabled on apisix-runtime, such as a specific version bump or functionality milestone. This ensures the exclusion is temporary and prevents the coverage reduction from becoming permanent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 18-21: The comment block explaining why the xrpc tests are
excluded (t/stream/xrpc/downstream.t and t/stream/xrpc/upstream.t in the
exclude_tests configuration) lacks a tracked follow-up mechanism for re-enabling
them. Add a TODO comment or issue reference to the existing comment block that
specifies the concrete criteria or conditions for when these tests should be
re-enabled on apisix-runtime, such as a specific version bump or functionality
milestone. This ensures the exclusion is temporary and prevents the coverage
reduction from becoming permanent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edb05870-8e9b-4ea9-8f35-3bc7113890c3
📒 Files selected for processing (2)
.github/workflows/ci.ymlt/ssl.t
t/ssl.t TEST 3 and TEST 5 assert error messages emitted by our own lua-resty-core-tlshandshake patch, whose wording differs between the OpenResty 1.21.x patch and the 1.29.2.4 patch. Detect the running openresty version via 'nginx -v' and assert the exact message for that version instead of a loose regex, so a wrong-message regression is still caught. xrpc downstream/upstream tests assert stream-lua internal buffer-allocation debug logs that changed in stream-lua 0.0.19 (OpenResty 1.29.2.4); exclude them on apisix-runtime only.
AlinsRan
force-pushed
the
feat/stream-upstream-mtls
branch
from
2d79dec to
f953f0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@t/ssl.t`:
- Around line 11-13: The version parsing in the block starting with the `eval`
statement and the regex match on `$version` does not guard against cases where
the pattern fails to match. When the openresty version regex pattern does not
find a match, `$major` and `$minor` will be undefined, causing the subsequent
numeric comparisons in the `$reworded` assignment to generate warnings and
potentially select the wrong code branch. Add a guard condition to check that
both `$major` and `$minor` are defined and have valid numeric values before
performing the numeric comparisons in the `$reworded` assignment, ensuring a
sensible default value is used when the version cannot be parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ed2d4da-d9a8-421d-9bd5-123c077bfe3d
📒 Files selected for processing (2)
.github/workflows/ci.ymlt/ssl.t
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
- t/ssl.t: guard the openresty version parse so an unexpected `nginx -V`
output falls back to the pre-1.29 wording instead of warning on an undef
numeric comparison.
- t/stream/upstream_mtls.t: TEST 2 now also asserts the upstream rejects the
request ("No required SSL certificate was sent"), proving the handshake did
not succeed when set_cert_and_key rejected the missing key, not just that the
argument was rejected.
AlinsRan
force-pushed
the
feat/stream-upstream-mtls
branch
from
e9304a8 to
43d6f9b
Compare
nic-6443
approved these changes
Jun 24, 2026
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
3 participants
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.
What
Add a stream-subsystem equivalent of the http
set_cert_and_keyC-API so APISIX can present a client certificate (mTLS) to a TLS upstream over the L4/stream proxy — mirroring how the http subsystem already does it.This is the foundation for replacing the interim
proxy_ssl_certificate data:$varapproach in apache/apisix#13596 (stream mTLS, issue #12472) with the same mechanism http uses.Why C-API instead of the
proxy_ssl_certificate data:directiveThe http subsystem deliberately uses a C-API rather than the
data:-in-variable directive, for two reasons this PR brings to stream:EVP_PKEYand never stringified — so it can't leak intoerror_log/access_log(thedata:approach puts the plaintext PEM into a variable, which nginx dumps to the error log on any SSL load failure).Changes
src/stream/ngx_stream_apisix_module.{c,h}: store parsed cert/pkey in the module ctx (ngx_stream_apisix_upstream_set_cert_and_key), apply at the upstream SSL handshake (ngx_stream_apisix_set_upstream_ssl→SSL_use_certificate/SSL_add1_chain_cert/SSL_use_PrivateKey), free via pool cleanup. Mirrorsngx_http_apisix_*exactly.lib/resty/apisix/stream/upstream.lua:set_cert_and_key(cert, key)binding.patch/1.29.2.4/nginx-stream_upstream_mtls.patch: injectngx_stream_apisix_set_upstream_ssl(s, pc)right beforengx_ssl_handshake(pc)inngx_stream_proxy_ssl_init_connection. Verified to apply cleanly against nginx 1.29.2 (patch -p0, applies at the correct location). The#include <ngx_stream_apisix_module.h>is already added bynginx-tcp_over_tls.patch.Follow-ups (not in this PR)
patch/<ver>/dirs (the C module + Lua binding are version-agnostic; only the proxy injection is per-nginx-version).set_stream_upstream_client_certtoresty.apisix.stream.upstream.set_cert_and_key(parsed + decrypted + cached viafetch_cert/fetch_pkey) and drop theproxy_ssl_certificate $upstream_mtls_certtemplate plumbing — after a runtime build ships this.Testing
Compile-validated by mirroring the http implementation; the patch was dry-run/applied against the pinned nginx 1.29.2 source. Functional mTLS verification happens downstream via apache/apisix
t/stream-node/upstream-mtls.tonce a runtime image includes this. (Cannot run the full openresty build locally.)Summary by CodeRabbit