Skip to content

fix: when only tls.verify, skip the logic of judging client cert#12527

Merged
SkyeYoung merged 5 commits intoapache:masterfrom
SkyeYoung:young/fix/skip-when-only-tls-verify
Aug 22, 2025
Merged

fix: when only tls.verify, skip the logic of judging client cert#12527
SkyeYoung merged 5 commits intoapache:masterfrom
SkyeYoung:young/fix/skip-when-only-tls-verify

Conversation

@SkyeYoung
Copy link
Copy Markdown
Member

@SkyeYoung SkyeYoung commented Aug 15, 2025

Description

In #7046, a tls.verify property, only applicable to Kafka, was added to the upstream schema without being documented or reflected in the API. There's also a lack of handling for this property in other resources that use upstream.

So in this PR, I've added that when using non-Kafka HTTPS or gRPCs upstream, the client certificate related processing logic should be skipped when only tls.verify.

Considering that only the parts below in reference tls, and there are no filter-like functions provided by Lua, it's changed to check for the existence of tls.client_cert_id or tls.client_cert.

apisix/apisix/upstream.lua

Lines 265 to 273 in 0dde705

if (scheme == "https" or scheme == "grpcs") and tls_has_cert then
local client_cert, client_key
if up_conf.tls.client_cert_id then
client_cert = api_ctx.upstream_ssl.cert
client_key = api_ctx.upstream_ssl.key
else
client_cert = up_conf.tls.client_cert
client_key = up_conf.tls.client_key
end

Which issue(s) this PR fixes:

Fixes #12501

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 (If not, please discuss on the APISIX mailing list first)

@SkyeYoung SkyeYoung changed the title fix: judge cert in tls to avoid only tls.verify fix: judge cert in tls to skip when only tls.verify Aug 18, 2025
@SkyeYoung SkyeYoung changed the title fix: judge cert in tls to skip when only tls.verify fix: when only tls.verify, skip the logic of judging cert. Aug 19, 2025
@SkyeYoung SkyeYoung marked this pull request as ready for review August 19, 2025 08:46
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 19, 2025
@dosubot dosubot bot added the bug Something isn't working label Aug 19, 2025
Copy link
Copy Markdown

Copilot AI left a comment

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 fixes an issue where non-Kafka HTTPS/gRPCS upstreams were incorrectly processing TLS certificate logic when only tls.verify was specified. The change ensures that certificate-related processing is only triggered when actual client certificates are provided.

  • Modifies the TLS processing condition to check for the presence of client_cert or client_cert_id rather than just the existence of the tls object
  • Adds comprehensive test coverage for the tls.verify-only scenario
  • Updates documentation to clarify that tls.verify is currently only supported for Kafka upstreams

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
apisix/upstream.lua Updates condition to skip certificate processing when only tls.verify is present
t/node/upstream-mtls.t Adds test cases for tls.verify-only and combined scenarios
t/core/schema_def.t Adds schema validation test for tls.verify property
docs/en/latest/admin-api.md Documents the tls.verify property with Kafka limitation
docs/zh/latest/admin-api.md Documents the tls.verify property with Kafka limitation in Chinese

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@SkyeYoung SkyeYoung changed the title fix: when only tls.verify, skip the logic of judging cert. fix: when only tls.verify, skip the logic of judging client cert Aug 20, 2025
@SkyeYoung SkyeYoung requested a review from bzp2010 August 21, 2025 23:48
Copy link
Copy Markdown
Contributor

@bzp2010 bzp2010 left a comment

Choose a reason for hiding this comment

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

codes and tests lgtm

@bzp2010 bzp2010 requested a review from kayx23 August 22, 2025 05:18
@SkyeYoung SkyeYoung merged commit 370253d into apache:master Aug 22, 2025
44 of 48 checks passed
jizhuozhi pushed a commit to jizhuozhi/apisix that referenced this pull request Oct 18, 2025
…pache#12527)

* fix: judge cert in tls to avoid only `tls.verify`

* docs(admin-api): add missing tls.verify

* docs: typo

* test: add tls.verify only cases

* test: adjust cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Internal Server Error when call an API created with Admin UI to https upstream

5 participants