Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set default value of ssl_trusted_certificate to system #11993

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Feb 25, 2025

Description

When testing AI plugins, we found that ssl_trusted_certificate should be set to system, otherwise APISIX will continuously report errors when accessing external AI services.

unable to get local issuer certificate

Based on the discussion below, this PR also removes support for combining multiple certs
Fixes # (issue)

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)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Feb 25, 2025
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Feb 25, 2025
nic-6443
nic-6443 previously approved these changes Feb 25, 2025
@nic-6443 nic-6443 changed the title feat: use "system" as default when ssl_trusted_certificate not passed feat: set default value of ssl_trusted_certificate to system Feb 25, 2025
@nic-6443 nic-6443 changed the title feat: set default value of ssl_trusted_certificate to system feat: set default value of ssl_trusted_certificate to system Feb 25, 2025
nic-6443
nic-6443 previously approved these changes Feb 25, 2025
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 25, 2025
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Feb 25, 2025
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 25, 2025
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Feb 27, 2025
nic-6443
nic-6443 previously approved these changes Feb 28, 2025
membphis
membphis previously approved these changes Feb 28, 2025
@@ -529,7 +531,7 @@ Please modify "admin_key" in conf/config.yaml .
end

local combined_cert_filepath = yaml_conf.apisix.ssl.ssl_trusted_combined_path
or "/usr/local/apisix/conf/ssl_trusted_combined.pem"
or profile.apisix_home .. "/conf/ssl_trusted_combined.pem"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use the system's CA file directly when it is set to system, but use the one processed by APISIX itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do use the system CA file. This path is to store the combined cert only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i got what you were saying. let me make that change

Copy link
Contributor

@bzp2010 bzp2010 Feb 28, 2025

Choose a reason for hiding this comment

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

Specifically, we should hardcode some list of paths based on the default system certificate paths of some of the major Linux distributions (Debian, Redhat) and find and use them at boot time.
As currently designed, this file will be copied and generated in /usr/local/apisix, a place where permissions management is typically more complex than in /etc/ssl/xxx.

This introduces security risks to APISIX that are not otherwise within its administrative purview. This is bad if a malicious user is not authorized to modify /etc and it can modify /usr/local/apisix.
A TLS CA file silently copied by APISIX to verify an authoritative certificate chain could be corrupted by a malicious user, unbeknownst to APISIX itself or the user, jeopardizing the user's system.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the system's own ca bundle is used, we just comply with whatever changes it makes, even though it may be broken as well, and that's not APISIX's problem.

Copy link
Member

Choose a reason for hiding this comment

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

start with a blank file, add their own self-signed CA to the end of the file, and specify a custom file directory inside the APISIX configuration file

In this case, the usage cost will increase, especially for containerized deployment. Users need to build their own image based on the APISIX container image or write corresponding shell scripts to complete this task before starting the APISIX image. Both methods are somewhat troublesome.
Imagine if the etcd deployed by users has mTLS enabled, in order to use custom CA certificates and system certificates simultaneously, they would need to handle the above steps themselves when deploying APISIX, which becomes inconvenient.

he user will know what they are doing and what the risks are, this cannot be a black box.

We can explain the actual functions of ssl_trusted_certificate and ssl_trusted_combined_path in the documentation for config.yaml. Just as APISIX generates nginx.conf at startup, we can explain the function of those temporary files.

Copy link
Member

Choose a reason for hiding this comment

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

As currently designed, this file will be copied and generated in /usr/local/apisix, a place where permissions management is typically more complex than in /etc/ssl/xxx.

yes, we should put it in $profile.apisix_home/conf like nginx.conf, using /usr/local/apisix is a mistake. cc @Revolyssup

Copy link
Contributor

Choose a reason for hiding this comment

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

@nic-6443

In this case, the usage cost will increase, especially for containerized deployment. Users need to build their own image based on the APISIX container image or write corresponding shell scripts to complete this task before starting the APISIX image. Both methods are somewhat troublesome.
Imagine if the etcd deployed by users has mTLS enabled, in order to use custom CA certificates and system certificates simultaneously, they would need to handle the above steps themselves when deploying APISIX, which becomes inconvenient.

I don't agree with the idea that costs will go up, if a user decides to turn on mTLS and the server doesn't use trusted certificates, it should handle any resulting extra work itself, which should follow any debian or redhat guidelines that exist as well, more people will learn about standard packages (ca-certificates) than will learn and understand us weird black box customization mechanisms.
For debian, first make sure the ca-certificates package is installed on your system, then place your own CA certificates in /usr/local/share/ca-certificates (the container will need to be mounted), and then just run sudo update-ca-certificates and those customized certificates will be automatically added to the root certificate package by the script.

For containers, this can be done by custom ENTRYPOINT or CMD, such as
docker run -d apache/apisix:3.11.0-debian sh -c "update-ca-certificates && apisix start" (just an example), this doesn't require a script. I'm doing this for a couple containers and it's working fine.

The difficulties you mention should be improved by some better docker entrypoint scripts, for example, by providing some directory to place and execute user's custom scripts before startup, as many are doing https://docs.linuxserver.io/general/container-customization/#custom-scripts. this is outside the scope of APISIX, and it's enough for APISIX to ensure that its behavior is safe.

Copy link
Member

Choose a reason for hiding this comment

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

I maintain my viewpoint that, from my perspective, the design of allowing ssl_trusted_certificate to support multiple files is very intuitive.
Based on this, in order for nginx/openresty's ssl_trusted_certificate to correctly read multiple files, we have no choice but to merge the files.
This logic is very simple and can be clearly explained in the description of this configuration, so I don't consider it a black-box design.
It is merely an encapsulation and enhancement of nginx/openresty's ssl_trusted_certificate.
@membphis @moonming @juzhiyuan Need more discussion here.

Copy link
Contributor

@bzp2010 bzp2010 Mar 5, 2025

Choose a reason for hiding this comment

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

This is not a rebuttal to the "simple wrapping" statement, it is simple wrapping. The problem is that we shouldn't do it at all.

Any action other than reading the system certificate file (when this value is set to system) is outside the scope of the APISIX project. Modifying the system certificate chain is a matter for the user's system administrators to deal with, not ours.

In short, we do not endorse or take responsibility for any potential security risks from this behavior.

If the file being copied is controlled by APISIX in terms of content and lifecycle, the responsibility for ensuring its security is APISIX's. With great power comes great responsibility.
Unless the user reads the code, no one will know that we have copied the file and placed it in the unintended unknown, and I think it's fair for me to call it a black box. This is a serious discussion for any security and trust issues.

System administrators are familiar with the way certificates are managed in the Linux distributions their companies use. The ca-certificates is a well known software package, and there are many documents on how to work with it. And what about APISIX's documentation of this? I don't think there's more than that on the config file.


There is also more than one way to modify the certificate chain, as I've already mentioned.

  1. Follow the Linux guide and use the update-ca-certificates command
  2. Point ssl_trusted_certificate to a CA certificate bundle file that you know exactly what it is and want to trust.

It's intuitive, and whether users are more familiar with ca-certificates or nginx they will know what should be done.

@Revolyssup Revolyssup dismissed stale reviews from membphis and nic-6443 via 24de38f February 28, 2025 08:53
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 28, 2025

yaml_conf.apisix.ssl.ssl_trusted_certificate = combined_cert_filepath
yaml_conf.apisix.ssl.ssl_trusted_certificate = trusted_certs_path
elseif yaml_conf.apisix.ssl.ssl_trusted_certificate ~= nil then
Copy link
Member

Choose a reason for hiding this comment

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

This condition is redundant, the default value for ssl_trusted_certificate has already been set above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants