Add support for automatic client authentication via TLS certificate fields

[omanges]

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

API Changes:

• New configuration directive tls-auth-clients-user, values CN | off, default off. CN means take username from the CommonName field in the client's certificate.
• New INFO field acl_access_denied_tls_cert under the Stats section, indicating the number of failed authentications using this feature, i.e. client certificates for which no matching username was found.
• New reason "tcl-cert" in the ACL log, logged when a client certificate's CommonName fails to match any existing username.

🔒 Feature Overview:

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

  • Allows specifying a certificate field (only CN for now) 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.

Closes #1866

[codecov]

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 ⚠️
src/connection.h	66.66%	1 Missing ⚠️

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%)	⬇️
src/config.c	78.47% <ø> (ø)
src/server.c	88.10% <100.00%> (+0.02%)	⬆️
src/server.h	100.00% <ø> (ø)
src/tls.c	100.00% <ø> (ø)
src/connection.h	87.09% <66.66%> (-0.69%)	⬇️
src/networking.c	88.39% <11.11%> (+<0.01%)	⬆️

... and 27 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.

[zuiderkwast]

Nice! I didn't do a complete review. For example I didn't look at the tests at all yet.

I'm not sure if the config should allow any field. Maybe only CN makes sense ever and then the config should maybe not be selecting a field. But let's discuss it and think about what we can do.

I found the corresponding features in ETCD and Kubernetes. The both use only common name as username:

• ETCD: https://etcd.io/docs/v3.4/op-guide/authentication/#using-tls-common-name

  As of version v3.2 if an etcd server is launched with the option --client-cert-auth=true, the field of Common Name (CN) in the client’s TLS cert will be used as an etcd user.

• Kubernetes: https://kubernetes.io/docs/reference/access-authn-authz/authentication/#x509-client-certificates

  If a client certificate is presented and verified, the common name of the subject is used as the user name for the request. As of Kubernetes 1.4, client certificates can also indicate a user's group memberships using the certificate's organization fields.

[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.

[zuiderkwast]

Almost there. I noticed the new INFO field. We may need to think about the naming.

[zuiderkwast]

Yeah, LGTM. Just one nit about a code comment.

[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.

[zuiderkwast]

Before merging I took a quick look. Noticed some config details to change. I will apply the change and merge. Thank you!

[madolson]

Sorry. I was late to review this. Would you be able to address some of these comments in a followup, otherwise I can address them.

[omanges]

Will address these comments by raising another PR

[omanges]

@madolson can you please review the PR :- #2364

I have addressed all the open comments