Add support for automatic client authentication via TLS certificate fields

[omanges]

This PR implements support for automatic client authentication based on a configurable field in the client's TLS certificate.

🔒 Feature Overview:

• Introduces a new configuration directive: tls-auth-clients-field.

  • Allows specifying a certificate field (e.g., CN, O) to map to a Valkey user.

  • If a matching user is found, the client is automatically authenticated as that user upon TLS handshake.

  • Falls back to the default user if no match is found.

⚙️ Implementation Details:

• Added getCertFieldByName() utility to extract fields from peer certificates.

• Added autoAuthenticateClientFromCert() to handle automatic login logic post-handshake.

• Integrated automatic authentication into the TLSAccept function after handshake completion.

• Updated test suite (tests/integration/tls.tcl) to validate the feature.

✅ Benefits:

• Enables smoother integration with mTLS-based infrastructures.

• Reduces the need for clients to manually send AUTH commands.

• Enhances security by tightly coupling certificate identity with Valkey access control.

🔬 Example:

tls-auth-clients-user CN

This will authenticate the client as the user matching the certificate's Common Name.

#1866

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 71.46%. Comparing base (d37dc52) to head (4495f63). Report is 30 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/networking.c	11.11%	8 Missing :warning:
src/connection.h	66.66%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1920      +/-   ##
============================================
- Coverage     71.46%   71.46%   -0.01%     
============================================
  Files           123      123              
  Lines         66927    67132     +205     
============================================
+ Hits          47831    47973     +142     
- Misses        19096    19159      +63     

Files with missing lines	Coverage Δ
src/acl.c	90.58% <100.00%> (-0.14%)	:arrow_down:
src/config.c	78.47% <ø> (ø)
src/server.c	88.10% <100.00%> (+0.02%)	:arrow_up:
src/server.h	100.00% <ø> (ø)
src/tls.c	100.00% <ø> (ø)
src/connection.h	87.09% <66.66%> (-0.69%)	:arrow_down:
src/networking.c	88.39% <11.11%> (+<0.01%)	:arrow_up:

... and 27 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[madolson]

@valkey-io/core-team Trying to make an asynchronous decision.

You can read the feature at the high level. We are discussing adding a new config flag, tls-auth-clients-user (yes|no) which automatically authenticates the client to an ACL user based on Common Name (CN) field of an x.509 certificate. There was a discussion about making it more generic, and allow the admin to configure which field to use, which might have a different config like tls-auth-clients-user (CN|O|off).

Please 👍 if you are okay with the recommendation of just a simple yes/no bool. Otherwise, please comment.

[zuiderkwast]

I haven't seen any system using "O" as the user name, but I'm OK with it if we also anticipate that we'll support other fields. Common Name (CN) seems to be the most common, but I've seen read about systems and/or recommendations for using these:

• subjectAltName UPN (User Principal Name) - non-standard field?
  • https://knowledgebase.paloaltonetworks.com/KCSArticleDetail?id=kA10g000000PO1GCAW
• subjectAltName DNS
  • For identifying using a machine's hostname, https://security.stackexchange.com/questions/192979/client-certificate-common-name-subject-alternative-name

What about combinations? If we use an enum, we'd need to define enum values like "CN or SAN DNS". Or shouldn't we accept combinations?

[madolson]

Even if there is a remote chance of having some amount of customizations would lead me to recommend we should just use an enum with one option for now CN.

[PingXie]

@valkey-io/core-team Trying to make an asynchronous decision.

You can read the feature at the high level. We are discussing adding a new config flag, tls-auth-clients-user (yes|no) which automatically authenticates the client to an ACL user based on Common Name (CN) field of an x.509 certificate. There was a discussion about making it more generic, and allow the admin to configure which field to use, which might have a different config like tls-auth-clients-user (CN|O|off).

Please 👍 if you are okay with the recommendation of just a simple yes/no bool. Otherwise, please comment.

@madolson I thought you were advocating for using O but you voted for a fixed CN field? I would've been siding with a boolean flag if I didn't see your O comment. if we still see a need to support O in the future, is the plan to introduce a second config?

[madolson]

I thought you were advocating for using O but you voted for a fixed CN field? I would've been siding with a boolean flag if I didn't see your O comment. if we still see a need to support O in the future, is the plan to introduce a second config?

There is a team at amazon who wanted it off the O field, but they honestly said they didn't care. They wrote all this logic in a module so they probably won't adopt what we build regardless.

[PingXie]

I thought you were advocating for using O but you voted for a fixed CN field? I would've been siding with a boolean flag if I didn't see your O comment. if we still see a need to support O in the future, is the plan to introduce a second config?

There is a team at amazon who wanted it off the O field, but they honestly said they didn't care. They wrote all this logic in a module so they probably won't adopt what we build regardless.

gotcha. I am less concerned about a specific team's implementation decision but if they see this being a legit use case maybe we should consider a special config instead, just to leave that door open? I feel that authentication on a team/org level isn't a uncommon use case.

[madolson]

@omanges Will you have time to update this? We would like to have in Valkey 9.0, and just trying to gauge the status of open PRs.

[omanges]

@madolson Yeah I will close it next 1-2 days is it fine ?

[omanges]

@madolson found some time so updated the PR :)

[omanges]

@zuiderkwast can you help to understand like why are the test failing with Configuration issue prevented Valkey startup ?

[zuiderkwast]

@zuiderkwast can you help to understand like why are the test failing with Configuration issue prevented Valkey startup ?

Never seen this. Right now I don't see a link to the failed CI jobs. I started them now.

You don't need to force-push. You can just add more commits to the same PR. It makes it easier to review the changes since last review. We will squash-merge the PR into one commit in the end anyway.

[omanges]

@zuiderkwast @madolson can you please review, I have fixed all the open items

[omanges]

The test were passing earlier like I just changed the text, that shouldn't fail the test :) Is it because of flakiness of the tests ?

[zuiderkwast]

Flaky tests, yeah, don't worry about it. CI looks good for this change.

[omanges]

Will address these comments by raising another PR

[omanges]

@madolson can you please review the PR :- https://github.com/valkey-io/valkey/pull/2364

I have addressed all the open comments