-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Add improved handling for TLS certificates for static builds. #17605
Conversation
Flagging @netdata/agent as a reviewer for help with testing. |
After the latest change - this PR works. However, I'm afraid I have to disagree with using bundled on any curl error instead of only on 60
^^ which is what we are checking. I am not going to approve it as is. |
@ilyam8 Except it’s not just 60 that we need to fall back on. We’re validating the entire TLS configuration, not just the certs, which means we also need to fall back on half a dozen other error codes as well (at minimum 35 (this sometimes but not always gets returned for issues reading the local certificate files), 53/54/66 (issues with the TLS engine selection, which is runtime configurable with the openssl config file), 77 (issues with the certificate files that do not return 35), 82 (CRL related issues), and 83 (issuer check failures)). And I am still not convinced that this matters enough to justify the overhead of having to check against a bunch of different possible exit codes. |
- Persist certificate handling mode and check URL across reinstalls. - Properly consume the arguments for the certificate handling options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR works as expected, LGTM!
Summary
Our current static builds include a bundled copy of the CA certificates available in the build environment, and will automatically fall back to using those if they can’t find a usable set of CA certificates on the target system.
This works in a majority of cases, but it has a couple of issues:
This PR works to remedy those issues. It adds additional logic to the installation code embedded in the static builds that handles the TLS certificates, adding support both for checking the usability of the selected certificates and for skipping the system certificates and just using the bundled ones. This functionality is controlled by two new options and associated environment variables:
--certificates
andNETDATA_CERT_MODE
allow specifying the certificate handling mode.auto
andsystem
have the current behavior.check
expands the current behavior by testing the detected system certificate store for usability by connecting to a well-known URL.bundled
ignores any system certificates and unconditionally uses the bundled certificates. Defaults toauto
if unspecified (thus preserving the existing behavior).--certificate-test-url
andNETDATA_CERT_TEST_URL
allow overriding the URL used bycheck
mode for testing whether the system certificates are usable. Defaults tohttps://app.netdata.cloud
if unspecified.The options override any value specified by the environment variables.
Additionally, the kickstart script is updated to pass the claiming URL as the certificate test URL when installing a static build, and to make the certificate handling mode default to
check
for installs other than offline installs andauto
mode for offline installs. The values passed by the kickstart script can still be overridden by using the--static-install-options
option to add the--certificates
or--certificate-test-url
options as needed.Test Plan
Testing requires manually installing static builds produced from this PR on a variety of systems with various values for the
--certificates
and--certificate-test-url
options and observing how the symlink at/opt/netdata/etc/ssl
is updated.