Skip to content

Fail fast on empty CA directory at TLS config load#3522

Open
yang-z-o wants to merge 1 commit intovalkey-io:unstablefrom
yang-z-o:validate-empty-ca-dir
Open

Fail fast on empty CA directory at TLS config load#3522
yang-z-o wants to merge 1 commit intovalkey-io:unstablefrom
yang-z-o:validate-empty-ca-dir

Conversation

@yang-z-o
Copy link
Copy Markdown
Contributor

Problem

When tls-ca-cert-dir is configured to a directory that exists but contains no certificates, the server starts successfully and logs no warning. This is because SSL_CTX_load_verify_locations with a directory argument is lazy, OpenSSL registers the path without scanning it, returning success even for an empty directory.

The failure is only discovered at handshake time, when every connecting client sees Server closed the connection and the server logs Error accepting a client connection: error:0A000086:SSL routines::certificate verify failed.

This is a silent misconfiguration that gives no actionable signal at startup or CONFIG SET time.

Config argument OpenSSL function Fails on empty path
tls-ca-cert-file SSL_CTX_load_verify_locations (file) Yes
tls-ca-cert-dir SSL_CTX_load_verify_locations (CApath) No

Fix

loadCaCertDir (introduced in #2999 for validity checking) already eagerly scans the directory to load certs. This PR extends it to count the certs loaded and fail immediately if zero certs were found.

Behavior change

Scenario Before After
Startup with empty tls-ca-cert-dir Server starts, clients fail at handshake Server refuses to start with a clear log message
CONFIG SET tls-ca-cert-dir <empty-dir> Returns OK, clients fail at handshake Returns error, previous config preserved

Signed-off-by: Yang Zhao <zymy701@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.43%. Comparing base (087280e) to head (42a927f).
⚠️ Report is 7 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3522      +/-   ##
============================================
- Coverage     76.47%   76.43%   -0.05%     
============================================
  Files           159      159              
  Lines         79840    79851      +11     
============================================
- Hits          61060    61036      -24     
- Misses        18780    18815      +35     
Files with missing lines Coverage Δ
src/tls.c 17.64% <ø> (ø)

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/tls.c
return false;
}

int loaded = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Can we rename this to be a little more descriptive like loaded_ca_certs_count?

@yang-z-o yang-z-o requested a review from zuiderkwast April 18, 2026 02:42
@zuiderkwast zuiderkwast added the major-decision-pending Major decision pending by TSC team label Apr 21, 2026
@zuiderkwast
Copy link
Copy Markdown
Contributor

Failing when no CA certificates exists is similar to failing when invalid certificates are present. But maybe it's not exactly the same?

If no CA certificates exist, no clients are trusted, but that's true also if there is a self-signed CA certificate that nobody uses. This will also result in no clients being trusted. But what we want to catch here is an administrator mistake...

If CA certificates in the directory are added and removed when the server is running and the auto-reload feature is enabled, we can get into the same situation? The auto-reload will fail if the directory is empty?

@yang-z-o
Copy link
Copy Markdown
Contributor Author

But what we want to catch here is an administrator mistake...

Yes, this change is specifically targeting administrator mistakes during configuration

The auto-reload will fail if the directory is empty?

Yes, the reload would fail with error and server keeps using original context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-pending Major decision pending by TSC team

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants