Add support for automatic client authentication via TLS certificate fields #1920

omanges · 2025-04-06T12:57:32Z

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-field CN

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

#1866

…g based TLS certifiacte field Signed-off-by: Omkar Mestry <[email protected]>

codecov · 2025-04-07T19:28:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.05%. Comparing base (add716b) to head (097de91).
Report is 3 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1920      +/-   ##
============================================
+ Coverage     71.02%   71.05%   +0.02%     
============================================
  Files           123      123              
  Lines         65683    65683              
============================================
+ Hits          46653    46668      +15     
+ Misses        19030    19015      -15

Files with missing lines	Coverage Δ
src/config.c	`78.58% <ø> (+0.19%)`	⬆️
src/connection.h	`87.77% <ø> (ø)`
src/server.h	`100.00% <ø> (ø)`
src/tls.c	`100.00% <ø> (ø)`

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

zuiderkwast · 2025-04-07T19:48:39Z

src/config.c

@@ -3361,6 +3361,7 @@ standardConfig static_configs[] = {
    createBoolConfig("tls-prefer-server-ciphers", NULL, MODIFIABLE_CONFIG, server.tls_ctx_config.prefer_server_ciphers, 0, NULL, applyTlsCfg),
    createBoolConfig("tls-session-caching", NULL, MODIFIABLE_CONFIG, server.tls_ctx_config.session_caching, 1, NULL, applyTlsCfg),
    createStringConfig("tls-cert-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.cert_file, NULL, NULL, applyTlsCfg),
+    createStringConfig("tls-auth-clients-field", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.client_auth_field, NULL, NULL, applyTlsCfg),


The config should probably be an enum config, so we can validate that only values that we can handle are allowed.

zuiderkwast · 2025-04-07T19:59:21Z

src/tls.c

+    if (!strcasecmp(field, "CN"))
+        nid = NID_commonName;
+    else if (!strcasecmp(field, "O"))
+        nid = NID_organizationName;
+    // Add more mappings here as needed


Using "O" makes no sense to me, just like "C" (Country) makes no sense. Fields like that are not unique. If we start supporting it, I suspect we might regret it in the future. @madolson you mentioned "O" in a comment. Was it a serious suggestion or just an example?

Out of the top-level fields, only CN (common name) makes sense to me.

Someone asked for O once inside AWS, but I don't know if they would have been okay with just CN as well. We can always just start with "CN" and see if anyone asks for more in the future? The enum just allows us to add more in the future.

The enum just allows us to add more in the future.

Yes, I agree enums in general are good for extensibility, but I doubt what we want to add in the future are top-level fields. It's more likely that we want to add

subject alt name: DNS name

an option to fail the handshake if the client cert doesn't match an existing user

But if you were asked once to use O for username, then I'm OK with it. It's an actual ask from a user.

I followed up with the folks, they did indeed ask for "O". They sign their certificates by team, and they want the auth to happen at the team level (imagine a bunch of apps with multiple services, they want all services to have the same permission). They said "CN" would be fine, they would just need to provision more users, but that would be "OK" for them.

Then, I think we shouldn't name the config tls-auth-clients-field. I prefer we just do like ETCD and use a boolean config. They have a config called client-cert-auth (true or false) for exactly the same feature: https://etcd.io/docs/v3.4/op-guide/authentication/#using-tls-common-name

How about boolean config tls-auth-clients-common-name?

I would prefer to focus on the fact we are authenticating the user of the client than on what field is being used. tls-auth-clients-user

tls-auth-clients-user SGTM

zuiderkwast · 2025-04-07T20:13:35Z

src/tls.c

+
+    X509 *cert = SSL_get_peer_certificate(conn->ssl);
+    if (!cert) {
+        serverLog(LL_WARNING, "TLS: No peer certificate found after handshake");


WARNING is a serious log level. It means the server is in trouble and needs an administrator's help.

Missing cert is not an error. Client certificate can be configured to be optional. I think we shouldn't log anything here, not even a notice.

zuiderkwast · 2025-04-07T20:14:41Z

src/tls.c

+
+    char field_value[256];
+    if (!getCertFieldByName(cert, field, field_value, sizeof(field_value))) {
+        serverLog(LL_WARNING, "TLS: Failed to extract field %s from certificate", field);


The should probably just be a NOTICE.

zuiderkwast · 2025-04-07T20:16:37Z

src/tls.c

+    serverLog(LL_NOTICE, "TLS: ACL lookup user: %s", u ? u->name : "NOT FOUND");
+    if (u) {
+        c->user = u;
+        c->flag.authenticated = true;
+        serverLog(LL_NOTICE, "TLS: Auto-authenticated client as %s", u->name);


Logging twice here is too verbose IMO. Let's log only the second one, when it has been found and authenticated.

If many clients connect successfully, are we're flooding the logs too much if we log here?

Is it better to log only in the NOT FOUND case? It's the exception case that shouldn't happen often. @madolson WDYT?

We also mark u->name as sensitive in other places, we should add it behind the hide user data flag. I would drop the log level to verbose if we want to keep the second one.

Good point. If we redact this log message, there isn't much to log at all. Shall we just skip logging here?

If we want it, it would be something like this:

serverLog(LL_VERBOSE, "TLS: Auto-authenticated client as %s", server.hide_user_data_from_log ? "*redacted*" : u->name);

madolson · 2025-04-08T20:23:23Z

src/connection.h

@@ -138,6 +138,7 @@ struct connection {
    ConnectionCallbackFunc conn_handler;
    ConnectionCallbackFunc write_handler;
    ConnectionCallbackFunc read_handler;
+    sds cert_user;


This should probably be on the underlying tlsConnection, not the higher level abstraction.

Actually, I'm not sure this needs to be on the struct, looks like it could just be stack allocated inside the function itself.

madolson · 2025-04-08T20:50:41Z

src/tls.c

 static void TLSAccept(void *_conn) {
    tls_connection *conn = (tls_connection *)_conn;
    ERR_clear_error();
    int ret = SSL_accept(conn->ssl);
    if (ret > 0) {
        conn->flags |= TLS_CONN_FLAG_ACCEPT_SUCCESS;
+        autoAuthenticateClientFromCert(conn);


We can't do this here, it's executed from the IO threads and the ACL code is not thread safe, we probably need to grab the user and stuff it on the connection to evaluate later.

omanges marked this pull request as draft April 6, 2025 13:02

Changes to add feature to auto login if the username match with confi…

097de91

…g based TLS certifiacte field Signed-off-by: Omkar Mestry <[email protected]>

omanges force-pushed the auto_auth_tls branch from 23432ec to 097de91 Compare April 6, 2025 13:03

omanges marked this pull request as ready for review April 6, 2025 13:04

omanges mentioned this pull request Apr 6, 2025

[NEW] Take username from client certificate #1866

Open

zuiderkwast self-requested a review April 7, 2025 19:10

zuiderkwast reviewed Apr 7, 2025

View reviewed changes

madolson reviewed Apr 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for automatic client authentication via TLS certificate fields #1920

Add support for automatic client authentication via TLS certificate fields #1920

omanges commented Apr 6, 2025

codecov bot commented Apr 7, 2025

zuiderkwast left a comment

zuiderkwast Apr 7, 2025

zuiderkwast Apr 7, 2025

madolson Apr 8, 2025

zuiderkwast Apr 8, 2025

madolson Apr 8, 2025

zuiderkwast Apr 8, 2025

zuiderkwast Apr 8, 2025

madolson Apr 8, 2025

zuiderkwast Apr 9, 2025

zuiderkwast Apr 7, 2025

zuiderkwast Apr 7, 2025

zuiderkwast Apr 7, 2025

zuiderkwast Apr 8, 2025

madolson Apr 8, 2025

zuiderkwast Apr 8, 2025

madolson Apr 8, 2025

madolson Apr 8, 2025

madolson Apr 8, 2025

Add support for automatic client authentication via TLS certificate fields #1920

Are you sure you want to change the base?

Add support for automatic client authentication via TLS certificate fields #1920

Conversation

omanges commented Apr 6, 2025

codecov bot commented Apr 7, 2025

Codecov Report

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment